Supported: - startup without "--no_modules" - "mod" command (without options) Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- defs.h | 36 ++++ kernel.c | 26 ++- symbols.c | 482 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 527 insertions(+), 17 deletions(-) diff --git a/defs.h b/defs.h index 12ad6aaa0998..655cd2add163 100644 --- a/defs.h +++ b/defs.h @@ -675,6 +675,7 @@ struct new_utsname { #define IRQ_DESC_TREE_RADIX (0x40ULL) #define IRQ_DESC_TREE_XARRAY (0x80ULL) #define KMOD_PAX (0x100ULL) +#define KMOD_MEMORY (0x200ULL) #define XEN() (kt->flags & ARCH_XEN) #define OPENVZ() (kt->flags & ARCH_OPENVZ) @@ -682,6 +683,7 @@ struct new_utsname { #define PVOPS_XEN() (kt->flags & ARCH_PVOPS_XEN) #define PAX_MODULE_SPLIT() (kt->flags2 & KMOD_PAX) +#define MODULE_MEMORY() (kt->flags2 & KMOD_MEMORY) #define XEN_MACHINE_TO_MFN(m) ((ulonglong)(m) >> PAGESHIFT()) #define XEN_PFN_TO_PSEUDO(p) ((ulonglong)(p) << PAGESHIFT()) @@ -2216,6 +2218,9 @@ struct offset_table { /* stash of commonly-used offsets */ long in6_addr_in6_u; long kset_kobj; long subsys_private_subsys; + long module_mem; + long module_memory_base; + long module_memory_size; }; struct size_table { /* stash of commonly-used sizes */ @@ -2389,6 +2394,7 @@ struct size_table { /* stash of commonly-used sizes */ long percpu_counter; long maple_tree; long maple_node; + long module_memory; }; struct array_table { @@ -2921,6 +2927,25 @@ struct mod_section_data { int flags; }; +/* This is unlikely to change, so imported from kernel for now. */ +enum mod_mem_type { + MOD_TEXT = 0, + MOD_DATA, + MOD_RODATA, + MOD_RO_AFTER_INIT, + MOD_INIT_TEXT, + MOD_INIT_DATA, + MOD_INIT_RODATA, + + MOD_MEM_NUM_TYPES, + MOD_INVALID = -1, +}; + +struct module_memory { + ulong base; + uint size; +}; + struct load_module { ulong mod_base; ulong module_struct; @@ -2954,13 +2979,23 @@ struct load_module { ulong mod_percpu; ulong mod_percpu_size; struct objfile *loaded_objfile; + + /* For 6.4 module_memory */ + struct module_memory mem[MOD_MEM_NUM_TYPES]; + struct syment *symtable[MOD_MEM_NUM_TYPES]; + struct syment *symend[MOD_MEM_NUM_TYPES]; }; +#define IN_MODULE(A,L) (_in_module(A, L, MOD_TEXT)) +#define IN_MODULE_INIT(A,L) (_in_module(A, L, MOD_INIT_TEXT)) + +/* #define IN_MODULE(A,L) \ (((ulong)(A) >= (L)->mod_base) && ((ulong)(A) < ((L)->mod_base+(L)->mod_size))) #define IN_MODULE_INIT(A,L) \ (((ulong)(A) >= (L)->mod_init_module_ptr) && ((ulong)(A) < ((L)->mod_init_module_ptr+(L)->mod_init_size))) +*/ #define IN_MODULE_PERCPU(A,L) \ (((ulong)(A) >= (L)->mod_percpu) && ((ulong)(A) < ((L)->mod_percpu+(L)->mod_percpu_size))) @@ -5581,6 +5616,7 @@ void dump_struct_member(char *, ulong, unsigned); void dump_union(char *, ulong, unsigned); void store_module_symbols_v1(ulong, int); void store_module_symbols_v2(ulong, int); +void store_module_symbols_v3(ulong, int); int is_datatype_command(void); int is_typedef(char *); int arg_to_datatype(char *, struct datatype_member *, ulong); diff --git a/kernel.c b/kernel.c index 6e98f5f6f6b1..62afff25bca8 100644 --- a/kernel.c +++ b/kernel.c @@ -3571,7 +3571,18 @@ module_init(void) MEMBER_OFFSET_INIT(module_num_gpl_syms, "module", "num_gpl_syms"); - if (MEMBER_EXISTS("module", "module_core")) { + if (MEMBER_EXISTS("module", "mem")) { /* 6.4 and later */ + kt->flags2 |= KMOD_MEMORY; /* MODULE_MEMORY() can be used. */ + + MEMBER_OFFSET_INIT(module_mem, "module", "mem"); + MEMBER_OFFSET_INIT(module_memory_base, "module_memory", "base"); + MEMBER_OFFSET_INIT(module_memory_size, "module_memory", "size"); + STRUCT_SIZE_INIT(module_memory, "module_memory"); + + if (get_array_length("module.mem", NULL, 0) != MOD_MEM_NUM_TYPES) + error(WARNING, "module memory types have changed!\n"); + + } else if (MEMBER_EXISTS("module", "module_core")) { MEMBER_OFFSET_INIT(module_core_size, "module", "core_size"); MEMBER_OFFSET_INIT(module_init_size, "module", @@ -3757,6 +3768,8 @@ module_init(void) total += nsyms; total += 2; /* store the module's start/ending addresses */ total += 2; /* and the init start/ending addresses */ + if (MODULE_MEMORY()) /* 7 regions at most -> 14 */ + total += 10; /* * If the module has kallsyms, set up to grab them as well. @@ -3784,7 +3797,11 @@ module_init(void) case KALLSYMS_V2: if (THIS_KERNEL_VERSION >= LINUX(2,6,27)) { numksyms = UINT(modbuf + OFFSET(module_num_symtab)); - size = UINT(modbuf + MODULE_OFFSET2(module_core_size, rx)); + if (MODULE_MEMORY()) + /* check mem[MOD_TEXT].size only */ + size = UINT(modbuf + OFFSET(module_mem) + OFFSET(module_memory_size)); + else + size = UINT(modbuf + MODULE_OFFSET2(module_core_size, rx)); } else { numksyms = ULONG(modbuf + OFFSET(module_num_symtab)); size = ULONG(modbuf + MODULE_OFFSET2(module_core_size, rx)); @@ -3822,7 +3839,10 @@ module_init(void) store_module_symbols_v1(total, kt->mods_installed); break; case KMOD_V2: - store_module_symbols_v2(total, kt->mods_installed); + if (MODULE_MEMORY()) + store_module_symbols_v3(total, kt->mods_installed); + else + store_module_symbols_v2(total, kt->mods_installed); break; } diff --git a/symbols.c b/symbols.c index f0721023816d..88849833bada 100644 --- a/symbols.c +++ b/symbols.c @@ -103,6 +103,8 @@ static void free_structure(struct struct_elem *); static unsigned char is_right_brace(const char *); static struct struct_elem *find_node(struct struct_elem *, char *); static void dump_node(struct struct_elem *, char *, unsigned char, unsigned char); +static int _in_module(ulong, struct load_module *, int); +static ulong module_mem_end(ulong, struct load_module *); /* @@ -1788,6 +1790,387 @@ modsym_value(ulong syms, union kernel_symbol *modsym, int i) return 0; } +static const char *module_start_tags[] = { + "_MODULE_TEXT_START_", + "_MODULE_DATA_START_", + "_MODULE_RODATA_START_", + "_MODULE_RO_AFTER_INIT_START_", + "_MODULE_INIT_TEXT_START_", + "_MODULE_INIT_DATA_START_", + "_MODULE_INIT_RODATA_START_" +}; +static const char *module_end_tags[] = { + "_MODULE_TEXT_END_", + "_MODULE_DATA_END_", + "_MODULE_RODATA_END_", + "_MODULE_RO_AFTER_INIT_END_", + "_MODULE_INIT_TEXT_END_", + "_MODULE_INIT_DATA_END_", + "_MODULE_INIT_RODATA_END_" +}; + +/* + * Linux 6.4 introduced module.mem memory layout + */ +void +store_module_symbols_v3(ulong total, int mods_installed) +{ + int i, m; + ulong mod, mod_next; + char *mod_name; + uint nsyms, ngplsyms; + ulong syms, gpl_syms; + ulong nksyms; + long strbuflen; + ulong size; + int mcnt, lm_mcnt; + union kernel_symbol *modsym; + size_t kernel_symbol_size; + struct load_module *lm; + char buf1[BUFSIZE]; + char buf2[BUFSIZE]; + char *strbuf, *modbuf, *modsymbuf; + struct syment *sp; + ulong first, last; + + st->mods_installed = mods_installed; + + if (!st->mods_installed) { + st->flags &= ~MODULE_SYMS; + return; + } + + /* + * If we've been here before, free up everything and start over. + */ + if (st->flags & MODULE_SYMS) + error(FATAL, "re-initialization of module symbols not implemented yet!\n"); + + kernel_symbol_size = kernel_symbol_type_init(); + + if ((st->ext_module_symtable = (struct syment *) + calloc(total, sizeof(struct syment))) == NULL) + error(FATAL, "v2 module syment space malloc (%ld symbols): %s\n", + total, strerror(errno)); + + if (!namespace_ctl(NAMESPACE_INIT, &st->ext_module_namespace, + (void *)total, NULL)) + error(FATAL, "module namespace malloc: %s\n", strerror(errno)); + + if ((st->load_modules = (struct load_module *)calloc + (st->mods_installed, sizeof(struct load_module))) == NULL) + error(FATAL, "load_module array malloc: %s\n", strerror(errno)); + + modbuf = GETBUF(SIZE(module)); + modsymbuf = NULL; + m = mcnt = mod_next = 0; + + for (mod = kt->module_list; mod != kt->kernel_module; mod = mod_next) { + + readmem(mod, KVADDR, modbuf, SIZE(module), + "module buffer", FAULT_ON_ERROR); + + syms = ULONG(modbuf + OFFSET(module_syms)); + gpl_syms = ULONG(modbuf + OFFSET(module_gpl_syms)); + nsyms = UINT(modbuf + OFFSET(module_num_syms)); + ngplsyms = UINT(modbuf + OFFSET(module_num_gpl_syms)); + + nksyms = UINT(modbuf + OFFSET(module_num_symtab)); + + mod_name = modbuf + OFFSET(module_name); + + lm = &st->load_modules[m++]; + BZERO(lm, sizeof(struct load_module)); + + size = 0; + for (i = MOD_TEXT; i < MOD_MEM_NUM_TYPES; i++) { + lm->mem[i].base = ULONG(modbuf + OFFSET(module_mem) + + SIZE(module_memory) * i + OFFSET(module_memory_base)); + lm->mem[i].size = UINT(modbuf + OFFSET(module_mem) + + SIZE(module_memory) * i + OFFSET(module_memory_size)); + if (i < MOD_INIT_TEXT) + size += lm->mem[i].size; + } + /* mem[MOD_TEXT].base for now */ + lm->mod_base = lm->mem[MOD_TEXT].base; + /* The entire module size */ + lm->mod_size = size; + lm->module_struct = mod; + + if (strlen(mod_name) < MAX_MOD_NAME) + strcpy(lm->mod_name, mod_name); + else { + error(INFO, "module name greater than MAX_MOD_NAME: %s\n", mod_name); + strncpy(lm->mod_name, mod_name, MAX_MOD_NAME-1); + } + if (CRASHDEBUG(3)) + fprintf(fp, "%lx (%lx): %s syms: %d gplsyms: %d ksyms: %ld\n", + mod, lm->mod_base, lm->mod_name, nsyms, ngplsyms, nksyms); + + lm->mod_flags = MOD_EXT_SYMS; + lm->mod_ext_symcnt = mcnt; + + lm->mod_text_start = lm->mod_base; + lm->mod_etext_guess = lm->mod_base + lm->mem[MOD_TEXT].size; + + lm->mod_init_module_ptr = lm->mem[MOD_INIT_TEXT].base; + lm->mod_init_size = lm->mem[MOD_INIT_TEXT].size; /* tentative.. */ + lm->mod_init_text_size = lm->mem[MOD_INIT_TEXT].size; + + if (VALID_MEMBER(module_percpu)) + lm->mod_percpu = ULONG(modbuf + OFFSET(module_percpu)); + + lm_mcnt = mcnt; + for (i = MOD_TEXT; i < MOD_MEM_NUM_TYPES; i++) { + if (!lm->mem[i].base) + continue; + + st->ext_module_symtable[mcnt].value = lm->mem[i].base; + st->ext_module_symtable[mcnt].type = 'm'; + st->ext_module_symtable[mcnt].flags |= MODULE_SYMBOL; + sprintf(buf2, "%s%s", module_start_tags[i], mod_name); + namespace_ctl(NAMESPACE_INSTALL, &st->ext_module_namespace, + &st->ext_module_symtable[mcnt], buf2); + lm_mcnt = mcnt; + mcnt++; + + if (i >= MOD_INIT_TEXT) + lm->mod_flags |= MOD_INIT; + } + + if (nsyms && !IN_MODULE(syms, lm)) { + error(WARNING, + "[%s] module.syms outside of module " "address space (%lx)\n\n", + lm->mod_name, syms); + nsyms = 0; + } + + if (nsyms) { + modsymbuf = GETBUF(kernel_symbol_size*nsyms); + readmem((ulong)syms, KVADDR, modsymbuf, + nsyms * kernel_symbol_size, + "module symbols", FAULT_ON_ERROR); + } + + for (i = first = last = 0; i < nsyms; i++) { + modsym = (union kernel_symbol *) + (modsymbuf + (i * kernel_symbol_size)); + if (!first + || first > modsym_name(syms, modsym, i)) + first = modsym_name(syms, modsym, i); + if (modsym_name(syms, modsym, i) > last) + last = modsym_name(syms, modsym, i); + } + + if (last > first) { + /* The buffer should not go over the block. */ + ulong end = module_mem_end(first, lm); + + strbuflen = (last-first) + BUFSIZE; + if ((first + strbuflen) >= end) { + strbuflen = end - first; + + } + strbuf = GETBUF(strbuflen); + + if (!readmem(first, KVADDR, strbuf, strbuflen, + "module symbol strings", RETURN_ON_ERROR)) { + FREEBUF(strbuf); + strbuf = NULL; + } + } else + strbuf = NULL; + + + for (i = 0; i < nsyms; i++) { + modsym = (union kernel_symbol *)(modsymbuf + (i * kernel_symbol_size)); + + BZERO(buf1, BUFSIZE); + + if (strbuf) + strcpy(buf1, &strbuf[modsym_name(syms, modsym, i) - first]); + else + read_string(modsym_name(syms, modsym, i), buf1, BUFSIZE-1); + + if (strlen(buf1)) { + st->ext_module_symtable[mcnt].value = + modsym_value(syms, modsym, i); + st->ext_module_symtable[mcnt].type = '?'; + st->ext_module_symtable[mcnt].flags |= MODULE_SYMBOL; + strip_module_symbol_end(buf1); + strip_symbol_end(buf1, NULL); + namespace_ctl(NAMESPACE_INSTALL, + &st->ext_module_namespace, + &st->ext_module_symtable[mcnt], buf1); + + mcnt++; + } + } + + if (modsymbuf) { + FREEBUF(modsymbuf); + modsymbuf = NULL; + } + + if (strbuf) + FREEBUF(strbuf); + + if (ngplsyms) { + modsymbuf = GETBUF(kernel_symbol_size * ngplsyms); + readmem((ulong)gpl_syms, KVADDR, modsymbuf, + ngplsyms * kernel_symbol_size, + "module gpl symbols", FAULT_ON_ERROR); + } + + for (i = first = last = 0; i < ngplsyms; i++) { + modsym = (union kernel_symbol *) + (modsymbuf + (i * kernel_symbol_size)); + if (!first + || first > modsym_name(gpl_syms, modsym, i)) + first = modsym_name(gpl_syms, modsym, i); + if (modsym_name(gpl_syms, modsym, i) > last) + last = modsym_name(gpl_syms, modsym, i); + } + + if (last > first) { + ulong end = module_mem_end(first, lm); + + strbuflen = (last-first) + BUFSIZE; + if ((first + strbuflen) >= end) { + strbuflen = end - first; + + } + strbuf = GETBUF(strbuflen); + + if (!readmem(first, KVADDR, strbuf, strbuflen, + "module gpl symbol strings", RETURN_ON_ERROR)) { + FREEBUF(strbuf); + strbuf = NULL; + } + } else + strbuf = NULL; + + for (i = 0; i < ngplsyms; i++) { + modsym = (union kernel_symbol *) (modsymbuf + (i * kernel_symbol_size)); + + BZERO(buf1, BUFSIZE); + + if (strbuf) + strcpy(buf1, &strbuf[modsym_name(gpl_syms, modsym, i) - first]); + else + read_string(modsym_name(gpl_syms, modsym, i), buf1, BUFSIZE-1); + + if (strlen(buf1)) { + st->ext_module_symtable[mcnt].value = + modsym_value(gpl_syms, modsym, i); + st->ext_module_symtable[mcnt].type = '?'; + st->ext_module_symtable[mcnt].flags |= MODULE_SYMBOL; + strip_module_symbol_end(buf1); + strip_symbol_end(buf1, NULL); + namespace_ctl(NAMESPACE_INSTALL, + &st->ext_module_namespace, + &st->ext_module_symtable[mcnt], buf1); + + mcnt++; + } + } + + if (modsymbuf) { + FREEBUF(modsymbuf); + modsymbuf = NULL; + } + + if (strbuf) + FREEBUF(strbuf); + + /* + * If the module was compiled with kallsyms, add them in. + */ + switch (kt->flags & (KALLSYMS_V1|KALLSYMS_V2)) + { + case KALLSYMS_V1: /* impossible, I hope... */ + mcnt += store_module_kallsyms_v1(lm, lm_mcnt, mcnt, modbuf); + break; + case KALLSYMS_V2: + mcnt += store_module_kallsyms_v2(lm, lm_mcnt, mcnt, modbuf); + break; + } + + for (i = MOD_TEXT; i < MOD_MEM_NUM_TYPES; i++) { + if (!lm->mem[i].base) + continue; + + st->ext_module_symtable[mcnt].value = lm->mem[i].base + lm->mem[i].size; + st->ext_module_symtable[mcnt].type = 'm'; + st->ext_module_symtable[mcnt].flags |= MODULE_SYMBOL; + sprintf(buf2, "%s%s", module_end_tags[i], mod_name); + namespace_ctl(NAMESPACE_INSTALL, + &st->ext_module_namespace, + &st->ext_module_symtable[mcnt], buf2); + mcnt++; + } + + lm->mod_ext_symcnt = mcnt - lm->mod_ext_symcnt; + + if (!lm->mod_etext_guess) + find_mod_etext(lm); + + NEXT_MODULE(mod_next, modbuf); + } + + FREEBUF(modbuf); + + st->ext_module_symcnt = mcnt; + st->ext_module_symend = &st->ext_module_symtable[mcnt]; + + namespace_ctl(NAMESPACE_COMPLETE, &st->ext_module_namespace, + st->ext_module_symtable, st->ext_module_symend); + + qsort(st->ext_module_symtable, mcnt, sizeof(struct syment), + compare_syms); + + /* not sure whether this sort is meaningful anymore. */ + qsort(st->load_modules, m, sizeof(struct load_module), compare_mods); + + for (m = 0; m < st->mods_installed; m++) { + lm = &st->load_modules[m]; + + /* TODO: make more efficient */ + for (i = MOD_TEXT; i < MOD_MEM_NUM_TYPES; i++) { + if (!lm->mem[i].base) + continue; + + sprintf(buf1, "%s%s", module_start_tags[i], lm->mod_name); + sprintf(buf2, "%s%s", module_end_tags[i], lm->mod_name); + + for (sp = st->ext_module_symtable; sp < st->ext_module_symend; sp++) { + if (STREQ(sp->name, buf1)) { + lm->symtable[i] = sp; + break; + } + } + for ( ; sp < st->ext_module_symend; sp++) { + if (STREQ(sp->name, buf2)) { + lm->symend[i] = sp; + break; + } + } + + if (lm->symtable[i] && lm->symend[i]) + mod_symtable_hash_install_range(lm->symtable[i], lm->symend[i]); + } + } + + st->flags |= MODULE_SYMS; + + /* probably not needed anymore */ + if (symbol_query("__insmod_", NULL, NULL)) + st->flags |= INSMOD_BUILTIN; + + if (mcnt > total) + error(FATAL, "store_module_symbols_v3: total: %ld mcnt: %d\n", total, mcnt); +} + void store_module_symbols_v2(ulong total, int mods_installed) { @@ -2384,6 +2767,7 @@ store_module_kallsyms_v2(struct load_module *lm, int start, int curr, int mcnt; int mcnt_idx; char *module_buf_init = NULL; + ulong base, base_init, size, size_init; if (!(kt->flags & KALLSYMS_V2)) return 0; @@ -2394,9 +2778,22 @@ store_module_kallsyms_v2(struct load_module *lm, int start, int curr, ns = &st->ext_module_namespace; ec = &elf_common; - module_buf = GETBUF(lm->mod_size); + /* kallsyms data looks to be in MOD_DATA region. */ + if (MODULE_MEMORY()) { + base = lm->mem[MOD_DATA].base; + size = lm->mem[MOD_DATA].size; + base_init = lm->mem[MOD_INIT_DATA].base; + size_init = lm->mem[MOD_INIT_DATA].size; + } else { + base = lm->mod_base; + size = lm->mod_size; + base_init = lm->mod_init_module_ptr; + size_init = lm->mod_init_size; + } + + module_buf = GETBUF(size); - if (!readmem(lm->mod_base, KVADDR, module_buf, lm->mod_size, + if (!readmem(base, KVADDR, module_buf, size, "module (kallsyms)", RETURN_ON_ERROR|QUIET)) { error(WARNING,"cannot access module kallsyms\n"); FREEBUF(module_buf); @@ -2404,10 +2801,10 @@ store_module_kallsyms_v2(struct load_module *lm, int start, int curr, } if (lm->mod_init_size > 0) { - module_buf_init = GETBUF(lm->mod_init_size); + module_buf_init = GETBUF(size_init); - if (!readmem(lm->mod_init_module_ptr, KVADDR, module_buf_init, lm->mod_init_size, - "module init (kallsyms)", RETURN_ON_ERROR|QUIET)) { + if (!readmem(base_init, KVADDR, module_buf_init, size_init, + "module init (kallsyms)", RETURN_ON_ERROR|QUIET)) { error(WARNING,"cannot access module init kallsyms\n"); FREEBUF(module_buf_init); } @@ -2429,9 +2826,9 @@ store_module_kallsyms_v2(struct load_module *lm, int start, int curr, return 0; } if (IN_MODULE(ksymtab, lm)) - locsymtab = module_buf + (ksymtab - lm->mod_base); + locsymtab = module_buf + (ksymtab - base); else - locsymtab = module_buf_init + (ksymtab - lm->mod_init_module_ptr); + locsymtab = module_buf_init + (ksymtab - base_init); kstrtab = ULONG(modbuf + OFFSET(module_strtab)); if (!IN_MODULE(kstrtab, lm) && !IN_MODULE_INIT(kstrtab, lm)) { @@ -2444,9 +2841,9 @@ store_module_kallsyms_v2(struct load_module *lm, int start, int curr, return 0; } if (IN_MODULE(kstrtab, lm)) - locstrtab = module_buf + (kstrtab - lm->mod_base); + locstrtab = module_buf + (kstrtab - base); else - locstrtab = module_buf_init + (kstrtab - lm->mod_init_module_ptr); + locstrtab = module_buf_init + (kstrtab - base_init); for (i = 1; i < nksyms; i++) { /* ELF starts real symbols at 1 */ switch (BITS()) @@ -2461,11 +2858,8 @@ store_module_kallsyms_v2(struct load_module *lm, int start, int curr, break; } - if (((ec->st_value < lm->mod_base) || - (ec->st_value > (lm->mod_base + lm->mod_size))) && - ((ec->st_value < lm->mod_init_module_ptr) || - (ec->st_value > (lm->mod_init_module_ptr + lm->mod_init_size)))) - continue; + if (!IN_MODULE(ec->st_value, lm) && !IN_MODULE_INIT(ec->st_value, lm)) + continue; if (ec->st_shndx == SHN_UNDEF) continue; @@ -3531,6 +3925,13 @@ dump_symbol_table(void) (ulong)lm->mod_section_data, lm->mod_section_data ? "" : "(not allocated)"); + if (MODULE_MEMORY()) { + int j; + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + fprintf(fp, " mem[%d]: %lx (%x)\n", + j, lm->mem[j].base, lm->mem[j].size); + } + } for (s = 0; s < lm->mod_sections; s++) { fprintf(fp, @@ -9228,6 +9629,9 @@ dump_offset_table(char *spec, ulong makestruct) OFFSET(module_strtab)); fprintf(fp, " module_percpu: %ld\n", OFFSET(module_percpu)); + fprintf(fp, " module_mem: %ld\n", OFFSET(module_mem)); + fprintf(fp, " module_memory_base: %ld\n", OFFSET(module_memory_base)); + fprintf(fp, " module_memory_size: %ld\n", OFFSET(module_memory_size)); fprintf(fp, " module_sect_attrs: %ld\n", OFFSET(module_sect_attrs)); @@ -10851,6 +11255,7 @@ dump_offset_table(char *spec, ulong makestruct) SIZE(super_block)); fprintf(fp, " irqdesc: %ld\n", SIZE(irqdesc)); fprintf(fp, " module: %ld\n", SIZE(module)); + fprintf(fp, " module_memory: %ld\n", SIZE(module_memory)); fprintf(fp, " module_sect_attr: %ld\n", SIZE(module_sect_attr)); fprintf(fp, " list_head: %ld\n", SIZE(list_head)); fprintf(fp, " hlist_head: %ld\n", SIZE(hlist_head)); @@ -13520,3 +13925,52 @@ symbol_complete_match(const char *match, struct syment *sp_last) return NULL; } + +static int +_in_module(ulong addr, struct load_module *lm, int type) +{ + ulong base, size; + int i, last; + + if (MODULE_MEMORY()) { + if (type == MOD_TEXT) + last = MOD_RO_AFTER_INIT; + else + last = MOD_INIT_RODATA; + for (i = type ; i <= last; i++) { + base = lm->mem[i].base; + size = lm->mem[i].size; + if (!base) + continue; + if ((addr >= base) && (addr < (base + size))) + return TRUE; + } + return FALSE; + } + + if (type == MOD_TEXT) { + base = lm->mod_base; + size = lm->mod_size; + } else { + base = lm->mod_init_module_ptr; + size = lm->mod_init_size; + } + return ((addr >= base) && (addr < (base + size))); +} + +/* Returns the end address of the module memory region. */ +static ulong +module_mem_end(ulong addr, struct load_module *lm) +{ + ulong base, end; + int i; + for (i = MOD_TEXT; i < MOD_MEM_NUM_TYPES; i++) { + base = lm->mem[i].base; + if (!base) + continue; + end = base + lm->mem[i].size; + if ((addr >= base) && (addr < end)) + return end; + } + return 0; +} -- 2.31.1

Hi Lianbo, thank you for the comments. On 2023/05/19 18:06, lijiang wrote: Yes. OK. Yes, I just left them as a comment for reference during this development, and they will be removed. OK. Yes, right. Yes, I had the same impression about this. I would pefer store_module_symbols_6_4() for kernel version alignment. I don't like complicated code, but will think about something like this. OK. I thought when writing this, now the memory regions of a module are cattered, what is the good point of sorting modules only by their text start address? and it might be fine to preserve the order of the "modules" list. However, I sorted them after all, with a header change in the patch 15/15. Because it looks to be a very old facility. Some code comments show "2.2.7" kernel version and I've not seen such symbols on recent kernels. So I thought that it will not be needed for 6.4 and later at least. * Later versons of insmod store basic address information of each * module in a format that looks like the following example of the * nfsd module: * * d004d000 __insmod_nfsd_O/lib/modules/2.2.17/fs/nfsd.o_M3A7EE300_V131601 * d004d054 __insmod_nfsd_S.text_L30208 But do you know about them? To realize these: but finally, these are replaced in 5/15: https://listman.redhat.com/archives/crash-utility/2023-May/010653.html That's not needed because no other user than IN_MODULE() and IN_MODULE_INIT(), but ok, I will add it for future possible use. OK. um, these funcions are changed some later, so I said "fixes are piled up". sorry about that. https://listman.redhat.com/archives/crash-utility/2023-May/010658.html Thanks! Kazu

On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: Good understanding. But I'm concerned if the name "_MODULE_" is too short to distinguish kernel symbols from (pseudo)module symbols, although I do not see any kernel symbols with the prefix "_MODULE_". # objdump -t vmlinux|grep _MODULE_ And in crash-utility, most of the time it uses the strncmp()/strcmp() to compare pseudo symbols, but sometimes it also uses strstr() to search for the pseudo symbols. Maybe the following symbols may not be loaded by crash-utility, only some strings. # objdump -s vmlinux |grep _MODULE_ 015a20 52494659 494e475f 4d4f4455 4c455f53 RIFYING_MODULE_S 03ee70 574e5f4d 4f44554c 455f5349 474e4154 WN_MODULE_SIGNAT 03f180 444f574e 5f4d4f44 554c455f 50415241 DOWN_MODULE_PARA 1bb530 45544854 4f4f4c5f 4d4f4455 4c455f50 ETHTOOL_MODULE_P 1bba20 4f4f4c5f 4d4f4455 4c455f50 4f574552 OOL_MODULE_POWER 1bba80 54455f4d 4f44554c 455f434d 49535f4e TE_MODULE_CMIS_N ... +/* Indent issues. The variable "j" should be defined as enum mod_mem_type. The variable start is set to FALSE, looks strange, why not just set it to zero? MODULE_MEM_START(sp, i) + p1 = module_start_strs[j]; MODULE_MEM_END(sp, i) As I mentioned in the [PATCH 01/15], similarly the above strings are in pairs, so they can be defined as one array or macro substitution. /* Indent issues. The variable "j" should be defined as enum mod_mem_type. Currently, I'm still building the kernel for aarch64, will check it next time. + if (value == sp->value) { It's hard to track the change log for an old kernel and crash-utility. But for the kernel 6.4 and later, it could be safe to drop it, crash won't go into this code path. Why is it needed? The splast will also store the "__insmod_"-type symbols? + if (!MODULE_PSEUDO_SYMBOL(sp)) { Here, the suffix _v2 also looks fine to me, but if it can be aligned with the kernel version suffix, that will keep the same code style as the [patch 01/15]. The variable "i" should be defined as enum mod_mem_type, the compiler will be helpful to check for any potential errors(warnings). + Kernel usually tends to check the lm->mem[i].size instead of lm->mem[i].base, for example: ulong size; ... size = lm->mem[i].size; if (!size) continue; base = lm->mem[i].base; if ((addr >= base) && (addr < base + size)) The module_mem_type() will ensure that the type does not exceed the range of type, just like the code comment. So no need to check for the condition "type >= MOD_MEM_NUM_TYPES" as below. Thanks Lianbo + if (type == MOD_INVALID || type >= MOD_MEM_NUM_TYPES)

On Thu, May 25, 2023 at 10:36 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: Here, the enums are more flexible than macros, once the kernel changes the enums, crash-utility is easy to follow up kernel changes. #define MOD_TEXT 0 If the value is out of the enums ranges, it may have a warning from the compiler, just a guess, not confirmed. Also looks good to me. Thanks. Thank you for the explanation, Kazu. Lianbo

fix verify_module() and next_module_vaddr() Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- kernel.c | 19 ++++++++++++++++--- memory.c | 36 ++++++++++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/kernel.c b/kernel.c index 62afff25bca8..013a9a4d08f4 100644 --- a/kernel.c +++ b/kernel.c @@ -3913,8 +3913,13 @@ verify_modules(void) mod_base = mod; break; case KMOD_V2: - mod_base = ULONG(modbuf + - MODULE_OFFSET2(module_module_core, rx)); + if (MODULE_MEMORY()) + /* mem[MOD_TEXT].base */ + mod_base = ULONG(modbuf + OFFSET(module_mem) + + OFFSET(module_memory_base)); + else + mod_base = ULONG(modbuf + + MODULE_OFFSET2(module_module_core, rx)); break; } @@ -3936,7 +3941,15 @@ verify_modules(void) case KMOD_V2: module_name = modbuf + OFFSET(module_name); - if (THIS_KERNEL_VERSION >= LINUX(2,6,27)) + if (MODULE_MEMORY()) { + int j; + mod_size = 0; + for (j = MOD_TEXT; j < MOD_INIT_TEXT; j++) { + mod_size += UINT(modbuf + OFFSET(module_mem) + + SIZE(module_memory) * i + + OFFSET(module_memory_size)); + } + } else if (THIS_KERNEL_VERSION >= LINUX(2,6,27)) mod_size = UINT(modbuf + MODULE_OFFSET2(module_core_size, rx)); else diff --git a/memory.c b/memory.c index 0568f18eb9b7..d8a37e7a0c4f 100644 --- a/memory.c +++ b/memory.c @@ -15687,10 +15687,13 @@ in_vmlist_segment(ulong vaddr) static int next_module_vaddr(ulong vaddr, ulong *nextvaddr) { - int i; - ulong start, end; + int i, j; + ulong start, end, min = (ulong)-1; struct load_module *lm; + if (MODULE_MEMORY()) + goto module_memory; + for (i = 0; i < st->mods_installed; i++) { lm = &st->load_modules[i]; start = lm->mod_base; @@ -15707,6 +15710,35 @@ next_module_vaddr(ulong vaddr, ulong *nextvaddr) return TRUE; } + return FALSE; + +module_memory: /* TODO: test this */ + for (i = 0; i < st->mods_installed; i++) { + lm = &st->load_modules[i]; + + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + start = lm->mem[j].base; + end = start + lm->mem[j].size; + + if (vaddr >= end) + continue; + + if (vaddr < start) { + if (start < min) /* replace candidate */ + min = start; + continue; + } + + *nextvaddr = vaddr; + return TRUE; + } + } + + if (min != (ulong)-1) { + *nextvaddr = min; + return TRUE; + } + return FALSE; } -- 2.31.1

On 2023/05/25 22:59, lijiang wrote: Right, lm->mod_base is now equal to mem[MOD_TEXT].base. Thanks, I didn't notice it because I'm using a vmcore to test so far. (btw, isn't it possible for you to send emails only with plain text part? Your emails have two parts, html and plain text, but the html one is shown with proportional font and indents are fuzzy. The plain text one looks auto-generated(?) and is wrapped excessively.. Or is my Thunderbird setting not good?) sorry, I don't understand your questions. This function searches for the next lowest module address from the vaddr. Now the regions of "a module" are not one block and even not sorted, this function needs to search all regions of all modules. min is a candidate, so when a lower start address than min is found, min is replaced with the start address. Thanks, Kazu

To "sym -m" print the symbols of a module in address order. (but "sym -l" and "sym -M" still print modules in text address order.) Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- defs.h | 2 ++ symbols.c | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/defs.h b/defs.h index 655cd2add163..4051eb8d134e 100644 --- a/defs.h +++ b/defs.h @@ -2984,6 +2984,8 @@ struct load_module { struct module_memory mem[MOD_MEM_NUM_TYPES]; struct syment *symtable[MOD_MEM_NUM_TYPES]; struct syment *symend[MOD_MEM_NUM_TYPES]; + int address_order[MOD_MEM_NUM_TYPES]; + int nr_mems; }; #define IN_MODULE(A,L) (_in_module(A, L, MOD_TEXT)) diff --git a/symbols.c b/symbols.c index 669fa2e2f3da..5edc4844b3d8 100644 --- a/symbols.c +++ b/symbols.c @@ -1300,7 +1300,7 @@ symname_hash_search(struct syment *table[], char *name) static void module_symbol_dump(char *module) { - int i, j, start, percpu_syms; + int i, j, m, start, percpu_syms; struct syment *sp, *sp_end; struct load_module *lm; const char *p1, *p2; @@ -1317,7 +1317,8 @@ module_symbol_dump(char *module) if (received_SIGINT() || output_closed()) return; - for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + for (m = 0; m < lm->nr_mems; m++) { + j = lm->address_order[m]; if (!lm->symtable[j]) continue; @@ -1936,7 +1937,7 @@ static const char *module_end_strs[] = { void store_module_symbols_v3(ulong total, int mods_installed) { - int i, m; + int i, j, m; ulong mod, mod_next; char *mod_name; uint nsyms, ngplsyms; @@ -2009,6 +2010,9 @@ store_module_symbols_v3(ulong total, int mods_installed) SIZE(module_memory) * i + OFFSET(module_memory_base)); lm->mem[i].size = UINT(modbuf + OFFSET(module_mem) + SIZE(module_memory) * i + OFFSET(module_memory_size)); + if (lm->mem[i].base) + lm->nr_mems++; + if (i < MOD_INIT_TEXT) size += lm->mem[i].size; } @@ -2018,6 +2022,24 @@ store_module_symbols_v3(ulong total, int mods_installed) lm->mod_size = size; lm->module_struct = mod; + /* For "sym -m" to print in address order */ + if (lm->nr_mems) { + int idx = 0; + ulong base, prev_min = 0, min = (ulong)-1; + for (i = 0; i < lm->nr_mems; i++) { + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + base = lm->mem[j].base; + if (base > prev_min && base < min) { + min = base; + idx = j; + } + } + lm->address_order[i] = idx; + prev_min = min; + min = (ulong)-1; + } + } + if (strlen(mod_name) < MAX_MOD_NAME) strcpy(lm->mod_name, mod_name); else { @@ -4052,6 +4074,11 @@ dump_symbol_table(void) fprintf(fp, " mem[%d]: %lx (%x)\n", j, lm->mem[j].base, lm->mem[j].size); } + fprintf(fp, " nr_mems: %d\n", lm->nr_mems); + fprintf(fp, " address_order:"); + for (j = 0; j < lm->nr_mems; j++) + fprintf(fp, " %d", lm->address_order[j]); + fprintf(fp, "\n"); } for (s = 0; s < lm->mod_sections; s++) { -- 2.31.1

On Fri, May 26, 2023 at 9:56 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: Yes. In fact, the above output is sorted naturally by the enum mod_mem_type order. At least for me, the *text address order* is more friendly and readable to me. Because you mentioned that(in patch log): "(but "sym -l" and "sym -M" still print modules in text address order.)" Without this change, the "sym -m" has also the same output style as the "sym -l/-M" options. This patch's "sym -m" is already consistent with the "sym -l|-M" options: will use it to speed up searching addresses. I'm not sure how much it would benefit as I do not have the test(performance) data. I have another issue: the variable address_order is initialized once in the store_module_symbols_v3()(called by the module_init()), how does it sort the modules loaded by "mod -s" command in address order? The relevant data can be updated to the "address_order", or not needed? btw, this facility is helpful to speed up searching addresses in some I don't have any performance data about it, although it might speed up searching addresses from the code perspective. If the performance can significantly benefit from these changes, it's worth doing that. But we can still keep the text address order, no need to make the following changes, seems they have no conflicts? @@ -1317,7 +1317,8 @@ module_symbol_dump(char *module) if (received_SIGINT() || output_closed()) return; - for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + for (m = 0; m < lm->nr_mems; m++) { + j = lm->address_order[m]; Thanks. Lianbo

fix {lowest,highest}_module_address() and is_kernel_text() Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- defs.h | 6 ++++-- symbols.c | 29 ++++++++++++++++++----------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/defs.h b/defs.h index 4051eb8d134e..95e44e8cb87c 100644 --- a/defs.h +++ b/defs.h @@ -2988,8 +2988,10 @@ struct load_module { int nr_mems; }; -#define IN_MODULE(A,L) (_in_module(A, L, MOD_TEXT)) -#define IN_MODULE_INIT(A,L) (_in_module(A, L, MOD_INIT_TEXT)) +#define IN_MODULE(A,L) (_in_module_range(A, L, MOD_TEXT, MOD_RO_AFTER_INIT)) +#define IN_MODULE_INIT(A,L) (_in_module_range(A, L, MOD_INIT_TEXT, MOD_INIT_RODATA)) +#define IN_MODULE_TEXT(A,L) (_in_module_range(A, L, MOD_TEXT, MOD_TEXT) || \ + _in_module_range(A, L, MOD_INIT_TEXT, MOD_INIT_TEXT)) /* #define IN_MODULE(A,L) \ diff --git a/symbols.c b/symbols.c index 5edc4844b3d8..ef00ce0b79ca 100644 --- a/symbols.c +++ b/symbols.c @@ -106,7 +106,7 @@ static void dump_node(struct struct_elem *, char *, unsigned char, unsigned char static int module_mem_type(ulong, struct load_module *); static ulong module_mem_end(ulong, struct load_module *); -static int _in_module(ulong, struct load_module *, int); +static int _in_module_range(ulong, struct load_module *, int, int); struct syment *value_search_module_v2(ulong, ulong *); static const char *module_start_tags[]; @@ -3119,7 +3119,10 @@ lowest_module_address(void) lowest = (ulong)(-1); for (i = 0; i < st->mods_installed; i++) { lm = &st->load_modules[i]; - low = lm->mod_base; + if (MODULE_MEMORY()) + low = lm->mem[lm->address_order[0]].base; + else + low = lm->mod_base; if (low < lowest) lowest = low; } @@ -3133,11 +3136,16 @@ highest_module_address(void) int i; struct load_module *lm; ulong high, highest; + struct module_memory *mem; highest = 0; for (i = 0; i < st->mods_installed; i++) { lm = &st->load_modules[i]; - high = lm->mod_base + lm->mod_size; + if (MODULE_MEMORY()) { + mem = &lm->mem[lm->address_order[lm->nr_mems-1]]; + high = mem->base + mem->size; + } else + high = lm->mod_base + lm->mod_size; if (high > highest) highest = high; } @@ -3530,6 +3538,9 @@ is_kernel_text(ulong value) if ((value >= start) && (value < end)) return TRUE; } + } else if (MODULE_MEMORY()) { + if (IN_MODULE_TEXT(value, lm)) + return TRUE; } else { switch (kt->flags & (KMOD_V1|KMOD_V2)) { @@ -14157,17 +14168,13 @@ symbol_complete_match(const char *match, struct syment *sp_last) } static int -_in_module(ulong addr, struct load_module *lm, int type) +_in_module_range(ulong addr, struct load_module *lm, int start, int end) { ulong base, size; - int i, last; + int i; if (MODULE_MEMORY()) { - if (type == MOD_TEXT) - last = MOD_RO_AFTER_INIT; - else - last = MOD_INIT_RODATA; - for (i = type ; i <= last; i++) { + for (i = start ; i <= end; i++) { base = lm->mem[i].base; size = lm->mem[i].size; if (!base) @@ -14178,7 +14185,7 @@ _in_module(ulong addr, struct load_module *lm, int type) return FALSE; } - if (type == MOD_TEXT) { + if (start == MOD_TEXT) { base = lm->mod_base; size = lm->mod_size; } else { -- 2.31.1

On 2023/05/25 23:39, lijiang wrote: oh, so I misunderstood your comment on the patch 03/15, I thought that patch was good. will comment to that 03/15 thread. Thanks, Kazu > the {lowest,highest}_modules_address() need to walk through all module > memory types. I will comment on it next time when reimplementing these two > functions. > > Thanks. > Lianbo > > + else >> + low = lm->mod_base; >> if (low < lowest) >> lowest = low; >> } >> @@ -3133,11 +3136,16 @@ highest_module_address(void) >> int i; >> struct load_module *lm; >> ulong high, highest; >> + struct module_memory *mem; >> >> highest = 0; >> for (i = 0; i < st->mods_installed; i++) { >> lm = &st->load_modules[i]; >> - high = lm->mod_base + lm->mod_size; >> + if (MODULE_MEMORY()) { >> + mem = &lm->mem[lm->address_order[lm->nr_mems-1]]; >> + high = mem->base + mem->size; >> + } else >> + high = lm->mod_base + lm->mod_size; >> if (high > highest) >> highest = high; >> } >> @@ -3530,6 +3538,9 @@ is_kernel_text(ulong value) >> if ((value >= start) && (value < end)) >> return TRUE; >> } >> + } else if (MODULE_MEMORY()) { >> + if (IN_MODULE_TEXT(value, lm)) >> + return TRUE; >> } else { >> switch (kt->flags & (KMOD_V1|KMOD_V2)) >> { >> @@ -14157,17 +14168,13 @@ symbol_complete_match(const char *match, struct >> syment *sp_last) >> } >> >> static int >> -_in_module(ulong addr, struct load_module *lm, int type) >> +_in_module_range(ulong addr, struct load_module *lm, int start, int end) >> { >> ulong base, size; >> - int i, last; >> + int i; >> >> if (MODULE_MEMORY()) { >> - if (type == MOD_TEXT) >> - last = MOD_RO_AFTER_INIT; >> - else >> - last = MOD_INIT_RODATA; >> - for (i = type ; i <= last; i++) { >> + for (i = start ; i <= end; i++) { >> base = lm->mem[i].base; >> size = lm->mem[i].size; >> if (!base) >> @@ -14178,7 +14185,7 @@ _in_module(ulong addr, struct load_module *lm, int >> type) >> return FALSE; >> } >> >> - if (type == MOD_TEXT) { >> + if (start == MOD_TEXT) { >> base = lm->mod_base; >> size = lm->mod_size; >> } else { >> -- >> 2.31.1 >> >> >>

On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: Is it possible to reuse the member offset in module memory patches? I noticed that the offset is not used in the calculate_load_order_v3(). If it is doable to reuse the offset, that may avoid modifying the gdb patch? I haven't investigated the details. }; - struct syment *symtable[MOD_MEM_NUM_TYPES]; Some similar member definitions are in the struct symbol_table_data and struct load_module, it looks confusing to me. But I'm not sure if it is better to move some of them to the struct symbol_talbe_data. + struct syment *ext_symtable[MOD_MEM_NUM_TYPES]; static void check_insmod_builtin(struct load_module *, int, ulong *); The above for-loop is frequently used in these patches, can we introduce the following macro definition from the kernel? #define for_each_mod_mem_type(type) \ for (enum mod_mem_type (type) = 0; \ (type) < MOD_MEM_NUM_TYPES; (type)++) Looks more convenient to me. + s1 = lm->symtable[j]; Ditto. Ditto. Ditto. Ditto. Ditto. Ditto. Thanks. Lianbo + if (!lm->symtable[i])

On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: Is it possible to put the [patch 09/15] and [patch 10/15] together? They have similar changes. Similarly, recursive function again. I have one question about it: how does it affect the performance when the recursion is deep enough? If it is not too deep, that should be good. Thanks. Lianbo

and fix off-by-one bug in "help -s" output with CRASHDEBUG(1) Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- symbols.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 2 deletions(-) diff --git a/symbols.c b/symbols.c index 5399c7494cb1..a432909ff28e 100644 --- a/symbols.c +++ b/symbols.c @@ -110,6 +110,7 @@ static int module_mem_type(ulong, struct load_module *); static ulong module_mem_end(ulong, struct load_module *); static int _in_module_range(ulong, struct load_module *, int, int); struct syment *value_search_module_v2(ulong, ulong *); +struct syment *next_module_symbol(char *, struct syment *, ulong); static const char *module_start_tags[]; static const char *module_start_strs[]; @@ -4116,9 +4117,9 @@ dump_symbol_table(void) fprintf(fp, " loaded_objfile: %lx\n", (ulong)lm->loaded_objfile); - if (CRASHDEBUG(1)) { + if (CRASHDEBUG(1) && lm->mod_load_symtable) { for (sp = lm->mod_load_symtable; - sp < lm->mod_load_symend; sp++) { + sp <= lm->mod_load_symend; sp++) { fprintf(fp, " %lx %s\n", sp->value, sp->name); } @@ -5857,6 +5858,106 @@ closest_symbol_value(ulong value) return(0); } +/* Only for 6.4 and later */ +struct syment * +next_module_symbol(char *symbol, struct syment *sp_in, ulong val_in) +{ + int i, j, k; + struct load_module *lm; + struct syment *sp, *sp_end; + + if (symbol) + goto symbol_search; + if (val_in) + goto value_search; + + /* for sp_in */ + for (i = 0; i < st->mods_installed; i++) { + lm = &st->load_modules[i]; + + /* quick check: sp_in is not in the module range. */ + if (sp_in < lm->symtable[lm->address_order[0]] || + sp_in > lm->symend[lm->address_order[lm->nr_mems-1]]) + continue; + + for (j = 0; j < lm->nr_mems; j++) { + k = lm->address_order[j]; + if (sp_in < lm->symtable[k] || sp_in > lm->symend[k]) + continue; + + if (sp_in == lm->symend[k]) + return next_module_symbol(NULL, NULL, sp_in->value); + + sp = sp_in + 1; + if (MODULE_PSEUDO_SYMBOL(sp)) + return next_module_symbol(NULL, NULL, sp->value); + + return sp; + } + } + return NULL; + +value_search: + sp = sp_end = NULL; + for (i = 0; i < st->mods_installed; i++) { + lm = &st->load_modules[i]; + + /* quick check: val_in is higher than the highest in the module. */ + if (val_in > lm->symend[lm->address_order[lm->nr_mems-1]]->value) + continue; + + for (j = 0; j < lm->nr_mems; j++) { + k = lm->address_order[j]; + if (val_in < lm->symtable[k]->value && + (sp == NULL || lm->symtable[k]->value < sp->value)) { + sp = lm->symtable[k]; + sp_end = lm->symend[k]; + break; + } + } + } + for ( ; sp < sp_end; sp++) { + if (MODULE_PSEUDO_SYMBOL(sp)) + continue; + if (sp->value > val_in) + return sp; + } + return NULL; + +symbol_search: + /* + * Deal with a few special cases... + * + * hmm, looks like crash now does not use these special cases. + * + if (strstr(symbol, " module)")) { + sprintf(buf, "_MODULE_START_"); + strcat(buf, &symbol[1]); + p1 = strstr(buf, " module)"); + *p1 = NULLCHAR; + symbol = buf; + } + + if (STREQ(symbol, "_end")) { + if (!st->mods_installed) + return NULL; + + lm = &st->load_modules[0]; + + return lm->mod_symtable; + } + */ + if ((sp = symbol_search(symbol))) { + sp++; + if (MODULE_PSEUDO_SYMBOL(sp) && strstr(sp->name, "_END")) + return next_module_symbol(NULL, NULL, sp->value); + + return sp; + } + + return NULL; +} + /* * For a given symbol, return a pointer to the next (higher) symbol's syment. * Either a symbol name or syment pointer may be passed as an argument. @@ -5886,6 +5987,9 @@ next_symbol(char *symbol, struct syment *sp_in) search_init = FALSE; + if (MODULE_MEMORY()) + return next_module_symbol(NULL, sp_in, 0); + for (i = 0; i < st->mods_installed; i++) { lm = &st->load_modules[i]; if (lm->mod_flags & MOD_INIT) @@ -5928,6 +6032,9 @@ next_symbol(char *symbol, struct syment *sp_in) } + if (MODULE_MEMORY()) + return next_module_symbol(symbol, NULL, 0); + /* * Deal with a few special cases... */ -- 2.31.1

On 2023/05/29 20:11, lijiang wrote: sorry, I don't understand this. There is the read problem here. lm->mod_load_symend is the last valid syment address. crash> help -s ... ffffffffc0146838 shared_memory_lock ffffffffc014683c dm_stat_need_rcu_barrier ffffffffc014e000 _MODULE_END_dm_mod mod_base: ffffffffc014f000 Without the patch, _MODULE_END_* are not displayed: ffffffffc0146838 shared_memory_lock ffffffffc014683c dm_stat_need_rcu_barrier mod_base: ffffffffc014f000 It looks to be a recursive call, but actually it's just a composite of three functions, i.e. symbol, syment and value search, and the value search does not do a recursive call. So I think it's not a real recursive call. Yes. Isn't it pointless? If we do so, we will need something like "goto syment_search" instead. True, will fix. Thanks, Kazu > > Thanks. > Lianbo > > for (i = 0; i < st->mods_installed; i++) { >> lm = &st->load_modules[i]; >> if (lm->mod_flags & MOD_INIT) >> @@ -5928,6 +6032,9 @@ next_symbol(char *symbol, struct syment *sp_in) >> } >> >> >> + if (MODULE_MEMORY()) >> + return next_module_symbol(symbol, NULL, 0); >> + >> /* >> * Deal with a few special cases... >> */ >> -- >> 2.31.1 >> >> >> >> On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾　一仁) <k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com>> wrote: >> >> and fix off-by-one bug in "help -s" output with CRASHDEBUG(1) >> >> Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com>> >> --- >>  symbols.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++++++++- >>  1 file changed, 109 insertions(+), 2 deletions(-) >> >> diff --git a/symbols.c b/symbols.c >> index 5399c7494cb1..a432909ff28e 100644 >> --- a/symbols.c >> +++ b/symbols.c >> @@ -110,6 +110,7 @@ static int module_mem_type(ulong, struct load_module *); >>  static ulong module_mem_end(ulong, struct load_module *); >>  static int _in_module_range(ulong, struct load_module *, int, int); >>  struct syment *value_search_module_v2(ulong, ulong *); >> +struct syment *next_module_symbol(char *, struct syment *, ulong); >> >>  static const char *module_start_tags[]; >>  static const char *module_start_strs[]; >> @@ -4116,9 +4117,9 @@ dump_symbol_table(void) >> >>                 fprintf(fp, "        loaded_objfile: %lx\n", (ulong)lm->loaded_objfile); >> >> -               if (CRASHDEBUG(1)) { >> +               if (CRASHDEBUG(1) && lm->mod_load_symtable) { >>                         for (sp = lm->mod_load_symtable; >> -                            sp < lm->mod_load_symend; sp++) { >> +                            sp <= lm->mod_load_symend; sp++) { >> >> >> The real problem might not be here? Some member variables are not properly initialized? >> >>                                 fprintf(fp, "  %lx  %s\n", >>                                         sp->value, sp->name); >>                         } >> @@ -5857,6 +5858,106 @@ closest_symbol_value(ulong value) >>                  return(0); >>  } >> >> +/* Only for 6.4 and later */ >> +struct syment * >> +next_module_symbol(char *symbol, struct syment *sp_in, ulong val_in) >> +{ >> +       int i, j, k; >> +       struct load_module *lm; >> +       struct syment *sp, *sp_end; >> + >> +       if (symbol) >> +               goto symbol_search; >> +       if (val_in) >> +               goto value_search; >> + >> +       /* for sp_in */ >> +       for (i = 0; i < st->mods_installed; i++) { >> +               lm = &st->load_modules[i]; >> + >> +               /* quick check: sp_in is not in the module range. */ >> +               if (sp_in < lm->symtable[lm->address_order[0]] || >> +                   sp_in > lm->symend[lm->address_order[lm->nr_mems-1]]) >> +                       continue; >> + >> +               for (j = 0; j < lm->nr_mems; j++) { >> +                       k = lm->address_order[j]; >> +                       if (sp_in < lm->symtable[k] || sp_in > lm->symend[k]) >> +                               continue; >> + >> +                       if (sp_in == lm->symend[k]) >> +                               return next_module_symbol(NULL, NULL, sp_in->value); >> + >> >> >> This means it has to be invoked recursively. >> >> +                       sp = sp_in + 1; >> +                       if (MODULE_PSEUDO_SYMBOL(sp)) >> +                               return next_module_symbol(NULL, NULL, sp->value); >> + >> +                       return sp; >> +               } >> +       } >> +       return NULL; >> + >> +value_search: >> +       sp = sp_end = NULL; >> +       for (i = 0; i < st->mods_installed; i++) { >> +               lm = &st->load_modules[i]; >> + >> +               /* quick check: val_in is higher than the highest in the module. */ >> +               if (val_in > lm->symend[lm->address_order[lm->nr_mems-1]]->value) >> +                       continue; >> + >> +               for (j = 0; j < lm->nr_mems; j++) { >> +                       k = lm->address_order[j]; >> +                       if (val_in < lm->symtable[k]->value && >> +                           (sp == NULL || lm->symtable[k]->value < sp->value)) { >> +                               sp = lm->symtable[k]; >> +                               sp_end = lm->symend[k]; >> +                               break; >> +                       } >> +               } >> +       } >> +       for ( ; sp < sp_end; sp++) { >> +               if (MODULE_PSEUDO_SYMBOL(sp)) >> +                       continue; >> +               if (sp->value > val_in) >> +                       return sp; >> +       } >> +       return NULL; >> + >> +symbol_search: >> +       /* >> +        *  Deal with a few special cases... >> +        * >> +        * hmm, looks like crash now does not use these special cases. >> +        * >> +       if (strstr(symbol, " module)")) { >> +                sprintf(buf, "_MODULE_START_"); >> +                strcat(buf, &symbol[1]); >> +                p1 = strstr(buf, " module)"); >> +                *p1 = NULLCHAR; >> +                symbol = buf; >> +       } >> + >> +       if (STREQ(symbol, "_end")) { >> +               if (!st->mods_installed) >> +                       return NULL; >> + >> +                lm = &st->load_modules[0]; >> + >> +               return lm->mod_symtable; >> +       } >> +       */ >> >> >> I tried to find the old code, but still did not get the changed history, and did not see the similar symbols  " module)" in the latest kernel. >> >> The symbol_search code block can be moved to the beginning of this function, the current code has become very simple, that can avoid the goto statement. >> >> +        if ((sp = symbol_search(symbol))) { >> +               sp++; >> +               if (MODULE_PSEUDO_SYMBOL(sp) && strstr(sp->name, "_END")) >> +                       return next_module_symbol(NULL, NULL, sp->value); >> + >> +               return sp; >> +       } >> + >> +       return NULL; >> +} >> + >>  /* >>   *  For a given symbol, return a pointer to the next (higher) symbol's syment. >>   *  Either a symbol name or syment pointer may be passed as an argument. >> @@ -5886,6 +5987,9 @@ next_symbol(char *symbol, struct syment *sp_in) >> >>                 search_init = FALSE; >> >> +               if (MODULE_MEMORY()) >> +                       return next_module_symbol(NULL, sp_in, 0); >> + >> >> >> The above if-code block should be moved before the "search_init = FALSE". >> >> Thanks. >> Lianbo >> >>                 for (i = 0; i < st->mods_installed; i++) { >>                         lm = &st->load_modules[i]; >>                         if (lm->mod_flags & MOD_INIT) >> @@ -5928,6 +6032,9 @@ next_symbol(char *symbol, struct syment *sp_in) >>         } >> >> >> +       if (MODULE_MEMORY()) >> +               return next_module_symbol(symbol, NULL, 0); >> + >>         /* >>          *  Deal with a few special cases... >>          */ >> -- >> 2.31.1 >>

Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- symbols.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 9 deletions(-) diff --git a/symbols.c b/symbols.c index 40e992e9ee12..5399c7494cb1 100644 --- a/symbols.c +++ b/symbols.c @@ -13565,7 +13565,7 @@ append_section_symbols: void delete_load_module(ulong base_addr) { - int i; + int i, j; struct load_module *lm; struct gnu_request request, *req; @@ -13580,7 +13580,23 @@ delete_load_module(ulong base_addr) req->name = lm->mod_namelist; gdb_interface(req); } - mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend); + if (MODULE_MEMORY()) { + if (lm->mod_load_symtable) { + mod_symtable_hash_remove_range(lm->mod_load_symtable, lm->mod_load_symend); + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + lm->load_symtable[j] = NULL; + lm->load_symend[j] = NULL; + } + } else { /* somehow this function runs for unloaded modules */ + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + if (!lm->symtable[j]) + continue; + mod_symtable_hash_remove_range(lm->symtable[j], lm->symend[j]); + } + } + } else + mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend); + if (lm->mod_load_symtable) { free(lm->mod_load_symtable); namespace_ctl(NAMESPACE_FREE, @@ -13588,9 +13604,19 @@ delete_load_module(ulong base_addr) } if (lm->mod_flags & MOD_REMOTE) unlink_module(lm); - lm->mod_symtable = lm->mod_ext_symtable; - lm->mod_symend = lm->mod_ext_symend; - mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend); + if (MODULE_MEMORY()) { + lm->symtable = lm->ext_symtable; + lm->symend = lm->ext_symend; + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + if (!lm->symtable[j]) + continue; + mod_symtable_hash_install_range(lm->symtable[j], lm->symend[j]); + } + } else { + lm->mod_symtable = lm->mod_ext_symtable; + lm->mod_symend = lm->mod_ext_symend; + mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend); + } lm->mod_flags &= ~(MOD_LOAD_SYMS|MOD_REMOTE|MOD_NOPATCH); lm->mod_flags |= MOD_EXT_SYMS; lm->mod_load_symtable = NULL; @@ -13619,7 +13645,23 @@ delete_load_module(ulong base_addr) req->name = lm->mod_namelist; gdb_interface(req); } - mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend); + if (MODULE_MEMORY()) + if (lm->mod_load_symtable) { + mod_symtable_hash_remove_range(lm->mod_load_symtable, lm->mod_load_symend); + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + lm->load_symtable[j] = NULL; + lm->load_symend[j] = NULL; + } + } else { /* somehow this function runs for unloaded modules */ + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + if (!lm->symtable[j]) + continue; + mod_symtable_hash_remove_range(lm->symtable[j], lm->symend[j]); + } + } + else + mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend); + if (lm->mod_load_symtable) { free(lm->mod_load_symtable); namespace_ctl(NAMESPACE_FREE, @@ -13627,9 +13669,19 @@ delete_load_module(ulong base_addr) } if (lm->mod_flags & MOD_REMOTE) unlink_module(lm); - lm->mod_symtable = lm->mod_ext_symtable; - lm->mod_symend = lm->mod_ext_symend; - mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend); + if (MODULE_MEMORY()) { + lm->symtable = lm->ext_symtable; + lm->symend = lm->ext_symend; + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + if (!lm->symtable[j]) + continue; + mod_symtable_hash_install_range(lm->symtable[j], lm->symend[j]); + } + } else { + lm->mod_symtable = lm->mod_ext_symtable; + lm->mod_symend = lm->mod_ext_symend; + mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend); + } lm->mod_flags &= ~(MOD_LOAD_SYMS|MOD_REMOTE|MOD_NOPATCH); lm->mod_flags |= MOD_EXT_SYMS; lm->mod_load_symtable = NULL; -- 2.31.1

On 2023/05/26 21:40, lijiang wrote: ok, thanks. This path is used by "mod -d|-D" and reinit_modules() for unloaded modules. The old procedure always does the cleanup and reinstall of the hash entries, so kept it as it is. Thanks, Kazu > > >> + for (j = MOD_TEXT; j < >> MOD_MEM_NUM_TYPES; j++) { >> + if (!lm->symtable[j]) >> + continue; >> + >> mod_symtable_hash_remove_range(lm->symtable[j], lm->symend[j]); >> + } >> + } >> + } else >> + >> mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend); >> + >> if (lm->mod_load_symtable) { >> free(lm->mod_load_symtable); >> namespace_ctl(NAMESPACE_FREE, >> @@ -13588,9 +13604,19 @@ delete_load_module(ulong base_addr) >> } >> if (lm->mod_flags & MOD_REMOTE) >> unlink_module(lm); >> - lm->mod_symtable = lm->mod_ext_symtable; >> - lm->mod_symend = lm->mod_ext_symend; >> - mod_symtable_hash_install_range(lm->mod_symtable, >> lm->mod_symend); >> + if (MODULE_MEMORY()) { >> + lm->symtable = lm->ext_symtable; >> + lm->symend = lm->ext_symend; >> + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; >> j++) { >> + if (!lm->symtable[j]) >> + continue; >> + >> mod_symtable_hash_install_range(lm->symtable[j], lm->symend[j]); >> + } >> + } else { >> + lm->mod_symtable = lm->mod_ext_symtable; >> + lm->mod_symend = lm->mod_ext_symend; >> + >> mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend); >> + } >> lm->mod_flags &= >> ~(MOD_LOAD_SYMS|MOD_REMOTE|MOD_NOPATCH); >> lm->mod_flags |= MOD_EXT_SYMS; >> lm->mod_load_symtable = NULL; >> @@ -13619,7 +13645,23 @@ delete_load_module(ulong base_addr) >> req->name = lm->mod_namelist; >> gdb_interface(req); >> } >> - mod_symtable_hash_remove_range(lm->mod_symtable, >> lm->mod_symend); >> + if (MODULE_MEMORY()) >> + if (lm->mod_load_symtable) { >> + >> mod_symtable_hash_remove_range(lm->mod_load_symtable, lm->mod_load_symend); >> + for (j = MOD_TEXT; j < >> MOD_MEM_NUM_TYPES; j++) { >> > > Ditto. > > >> + lm->load_symtable[j] = >> NULL; >> + lm->load_symend[j] = NULL; >> + } >> + } else { /* somehow this function runs for >> unloaded modules */ >> > > Ditto. > > Thanks. > Lianbo > > >> + for (j = MOD_TEXT; j < >> MOD_MEM_NUM_TYPES; j++) { >> + if (!lm->symtable[j]) >> + continue; >> + >> mod_symtable_hash_remove_range(lm->symtable[j], lm->symend[j]); >> + } >> + } >> + else >> + >> mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend); >> + >> if (lm->mod_load_symtable) { >> free(lm->mod_load_symtable); >> namespace_ctl(NAMESPACE_FREE, >> @@ -13627,9 +13669,19 @@ delete_load_module(ulong base_addr) >> } >> if (lm->mod_flags & MOD_REMOTE) >> unlink_module(lm); >> - lm->mod_symtable = lm->mod_ext_symtable; >> - lm->mod_symend = lm->mod_ext_symend; >> - mod_symtable_hash_install_range(lm->mod_symtable, >> lm->mod_symend); >> + if (MODULE_MEMORY()) { >> + lm->symtable = lm->ext_symtable; >> + lm->symend = lm->ext_symend; >> + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; >> j++) { >> + if (!lm->symtable[j]) >> + continue; >> + >> mod_symtable_hash_install_range(lm->symtable[j], lm->symend[j]); >> + } >> + } else { >> + lm->mod_symtable = lm->mod_ext_symtable; >> + lm->mod_symend = lm->mod_ext_symend; >> + >> mod_symtable_hash_install_range(lm->mod_symtable, lm->mod_symend); >> + } >> lm->mod_flags &= >> ~(MOD_LOAD_SYMS|MOD_REMOTE|MOD_NOPATCH); >> lm->mod_flags |= MOD_EXT_SYMS; >> lm->mod_load_symtable = NULL; >> -- >> 2.31.1 >> >> >>

On 2023/05/30 21:30, lijiang wrote: With this change, --- a/symbols.c +++ b/symbols.c @@ -13951,6 +13951,7 @@ delete_load_module(ulong base_addr) lm->load_symend[j] = NULL; } } else { /* somehow this function runs for unloaded modules */ + fprintf(fp, "in the else block (-D)\n"); for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { if (!lm->symtable[j]) continue; @@ -14016,6 +14017,7 @@ delete_load_module(ulong base_addr) lm->load_symend[j] = NULL; } } else { /* somehow this function runs for unloaded modules */ + fprintf(fp, "in the else block (-d)\n"); for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { if (!lm->symtable[j]) continue; These should be printed: crash-6.4> mod -s dm_mod MODULE NAME TEXT_BASE SIZE OBJECT FILE ffffffffc042fd00 dm_mod ffffffffc0418000 237568 dm-mod.ko crash-6.4> mod -d dm_mod crash-6.4> mod -d dm_mod in the else block (-d) crash-6.4> mod -d dm_log in the else block (-d) crash-6.4> mod -D | head in the else block (-D) in the else block (-D) in the else block (-D) in the else block (-D) in the else block (-D) in the else block (-D) in the else block (-D) in the else block (-D) in the else block (-D) in the else block (-D) Thanks, Kazu

On 2023/06/02 13:49, lijiang wrote: ok, good. but I tested it without the clean-up and re-install, it looked ok. So I'm thinking to remove them. Thanks, Kazu > > Thank you for the explanation, Kazu. > > >> in the else block (-d) >> crash-6.4> mod -d dm_log >> in the else block (-d) >> crash-6.4> mod -D | head >> in the else block (-D) >> in the else block (-D) >> in the else block (-D) >> in the else block (-D) >> in the else block (-D) >> in the else block (-D) >> in the else block (-D) >> in the else block (-D) >> in the else block (-D) >> in the else block (-D) >> >> Thanks, >> Kazu >>

Support percpu symbols for "sym" options and etc. crash> mod -s kvm_intel MODULE NAME BASE SIZE OBJECT FILE ffffffffc0b1b480 kvm_intel ffffffffc0d0c000 372736 /home/kernel/linux-next-next-20230310/arch/x86/kvm/kvm-intel.ko crash> sym -m kvm_intel | head 35020 MODULE PERCPU START: kvm_intel 35020 (d) loaded_vmcss_on_cpu 35030 (d) vmxarea 35038 (D) current_vmcs 35040 (d) wakeup_vcpus_on_cpu_lock 35050 (d) wakeup_vcpus_on_cpu 35060 MODULE PERCPU END: kvm_intel ffffffffc0aae000 MODULE RO_AFTER_INIT START: kvm_intel ffffffffc0ab1720 (D) vmx_capability ffffffffc0ab1740 (D) vmcs_config crash> sym vmxarea 35030 (d) vmxarea [kvm_intel] Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- symbols.c | 82 ++++++++++++++++++++++++++----------------------------- 1 file changed, 38 insertions(+), 44 deletions(-) diff --git a/symbols.c b/symbols.c index 8343081f51f7..40e992e9ee12 100644 --- a/symbols.c +++ b/symbols.c @@ -1302,7 +1302,7 @@ symname_hash_search(struct syment *table[], char *name) static void module_symbol_dump(char *module) { - int i, j, m, start, percpu_syms; + int i, j, m; struct syment *sp, *sp_end; struct load_module *lm; const char *p1, *p2; @@ -1319,6 +1319,19 @@ module_symbol_dump(char *module) if (received_SIGINT() || output_closed()) return; + /* The percpu area isn't included in any module memory area. */ + if (MODULE_PERCPU_SYMS_LOADED(lm)) { + p1 = "MODULE PERCPU START"; + p2 = lm->mod_name; + fprintf(fp, "%lx %s: %s\n", lm->mod_percpu, p1, p2); + + dump_percpu_symbols(lm); + + p1 = "MODULE PERCPU END"; + fprintf(fp, "%lx %s: %s\n", + lm->mod_percpu + lm->mod_percpu_size, p1, p2); + } + for (m = 0; m < lm->nr_mems; m++) { j = lm->address_order[m]; @@ -1327,21 +1340,8 @@ module_symbol_dump(char *module) sp = lm->symtable[j]; sp_end = lm->symend[j]; - percpu_syms = 0; - for (start = FALSE; sp <= sp_end; sp++) { - /* TODO - if (IN_MODULE_PERCPU(sp->value, lm)) { - if (percpu_syms == DISPLAYED) - continue; - if (!start) { - percpu_syms = TBD; - continue; - } - dump_percpu_symbols(lm); - percpu_syms = DISPLAYED; - } - */ + for ( ; sp <= sp_end; sp++) { if (MODULE_PSEUDO_SYMBOL(sp)) { if (MODULE_SECTION_START(sp)) { p1 = sp->name + strlen("_MODULE_SECTION_START "); @@ -1352,30 +1352,15 @@ module_symbol_dump(char *module) } else if (STRNEQ(sp->name, module_start_tags[j])) { p1 = module_start_strs[j]; p2 = sp->name + strlen(module_start_tags[j]); - start = TRUE; } else if (STRNEQ(sp->name, module_end_tags[j])) { p1 = module_end_strs[j]; p2 = sp->name + strlen(module_end_tags[j]); - /* TODO - if (MODULE_PERCPU_SYMS_LOADED(lm) && - !percpu_syms) { - dump_percpu_symbols(lm); - percpu_syms = DISPLAYED; - } - */ } else { p1 = "unknown tag"; p2 = sp->name; } fprintf(fp, "%lx %s: %s\n", sp->value, p1, p2); - - /* TODO - if (percpu_syms == TBD) { - dump_percpu_symbols(lm); - percpu_syms = DISPLAYED; - } - */ } else show_symbol(sp, 0, SHOW_RADIX()); } @@ -1497,8 +1482,14 @@ dump_percpu_symbols(struct load_module *lm) struct syment *sp, *sp_end; if (MODULE_PERCPU_SYMS_LOADED(lm)) { - sp = lm->mod_symtable; - sp_end = lm->mod_symend; + if (MODULE_MEMORY()) { + /* The lm should have mod_load_symtable. */ + sp = lm->mod_load_symtable; + sp_end = lm->mod_load_symend; + } else { + sp = lm->mod_symtable; + sp_end = lm->mod_symend; + } for ( ; sp <= sp_end; sp++) { if (IN_MODULE_PERCPU(sp->value, lm)) show_symbol(sp, 0, SHOW_RADIX()); @@ -2314,6 +2305,11 @@ store_module_symbols_v3(ulong total, int mods_installed) if (symbol_query("__insmod_", NULL, NULL)) st->flags |= INSMOD_BUILTIN; + if (CRASHDEBUG(2)) { + for (sp = st->ext_module_symtable; sp < st->ext_module_symend; sp++) + fprintf(fp, "%16lx %s\n", sp->value, sp->name); + } + if (mcnt > total) error(FATAL, "store_module_symbols_v3: total: %ld mcnt: %d\n", total, mcnt); } @@ -12128,6 +12124,13 @@ store_section_data(struct load_module *lm, bfd *bfd, asection *section) lm->mod_section_data[i].section = section; lm->mod_section_data[i].priority = prio; lm->mod_section_data[i].flags = section->flags & ~SEC_FOUND; + lm->mod_section_data[i].size = bfd_section_size(section); + lm->mod_section_data[i].offset = 0; + lm->mod_section_data[i].addr = 0; + if (strlen(name) < MAX_MOD_SEC_NAME) + strcpy(lm->mod_section_data[i].name, name); + else + strncpy(lm->mod_section_data[i].name, name, MAX_MOD_SEC_NAME-1); /* * The percpu section isn't included in kallsyms or module_core area. */ @@ -12135,14 +12138,8 @@ store_section_data(struct load_module *lm, bfd *bfd, asection *section) (STREQ(name,".data.percpu") || STREQ(name, ".data..percpu"))) { lm->mod_percpu_size = bfd_section_size(section); lm->mod_section_data[i].flags |= SEC_FOUND; + lm->mod_section_data[i].addr = lm->mod_percpu; } - lm->mod_section_data[i].size = bfd_section_size(section); - lm->mod_section_data[i].offset = 0; - lm->mod_section_data[i].addr = 0; - if (strlen(name) < MAX_MOD_SEC_NAME) - strcpy(lm->mod_section_data[i].name, name); - else - strncpy(lm->mod_section_data[i].name, name, MAX_MOD_SEC_NAME-1); lm->mod_sections += 1; } @@ -13546,11 +13543,8 @@ append_section_symbols: } lm->symtable = lm->load_symtable; lm->symend = lm->load_symend; - for (i = MOD_TEXT; i < MOD_MEM_NUM_TYPES; i++) { - if (!lm->symtable[i]) - continue; - mod_symtable_hash_install_range(lm->symtable[i], lm->symend[i]); - } + /* percpu symbols is out of the ranges.. */ + mod_symtable_hash_install_range(lm->mod_load_symtable, lm->mod_load_symend); } else { mod_symtable_hash_remove_range(lm->mod_symtable, lm->mod_symend); lm->mod_symtable = lm->mod_load_symtable; -- 2.31.1

On 2023/05/26 19:37, lijiang wrote: ok thanks, will improve the comment. (Some of my comments were just memos, intended to improve or remove them.) Thanks, Kazu

Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- symbols.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/symbols.c b/symbols.c index ba9cf607f256..9714453400bf 100644 --- a/symbols.c +++ b/symbols.c @@ -2049,6 +2049,7 @@ store_module_symbols_v3(ulong total, int mods_installed) lm->mod_ext_symcnt = mcnt; lm->mod_text_start = lm->mod_base; + /* not used */ lm->mod_etext_guess = lm->mod_base + lm->mem[MOD_TEXT].size; lm->mod_init_module_ptr = lm->mem[MOD_INIT_TEXT].base; @@ -2250,9 +2251,6 @@ store_module_symbols_v3(ulong total, int mods_installed) lm->mod_ext_symcnt = mcnt - lm->mod_ext_symcnt; - if (!lm->mod_etext_guess) - find_mod_etext(lm); - NEXT_MODULE(mod_next, modbuf); } -- 2.31.1

On 2023/05/29 21:34, lijiang wrote: Agree to set an invalid value, but I would like 0. -1 hits this: fprintf(fp, " mod_etext_guess: %lx (%lx)\n", lm->mod_etext_guess, lm->mod_etext_guess ? lm->mod_etext_guess - lm->mod_base : 0); Thanks, Kazu > > Thanks. > Lianbo > > >> lm->mod_init_module_ptr = lm->mem[MOD_INIT_TEXT].base; >> @@ -2250,9 +2251,6 @@ store_module_symbols_v3(ulong total, int >> mods_installed) >> >> lm->mod_ext_symcnt = mcnt - lm->mod_ext_symcnt; >> >> - if (!lm->mod_etext_guess) >> - find_mod_etext(lm); >> - >> NEXT_MODULE(mod_next, modbuf); > > } >> >> -- >> 2.31.1 >> >>

Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- symbols.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 78 insertions(+), 3 deletions(-) diff --git a/symbols.c b/symbols.c index db22f3c4e8b3..4c7fe06b3aad 100644 --- a/symbols.c +++ b/symbols.c @@ -5370,7 +5370,7 @@ old_module: struct syment * symbol_search_next(char *s, struct syment *spstart) { - int i; + int i, j; struct syment *sp, *sp_end; struct load_module *lm; int found_start; @@ -5390,6 +5390,31 @@ symbol_search_next(char *s, struct syment *spstart) } } + if (!MODULE_MEMORY()) + goto old_module; + + for (i = 0; i < st->mods_installed; i++) { + lm = &st->load_modules[i]; + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + if (!lm->symtable[j]) + continue; + + sp = lm->symtable[j]; + sp_end = lm->symend[j]; + for (; sp < sp_end; sp++) { + if (sp == spstart) { + found_start = TRUE; + continue; + } else if (!found_start) + continue; + + if (STREQ(s, sp->name)) + return sp; + } + } + } + +old_module: pseudos = (strstr(s, "_MODULE_START_") || strstr(s, "_MODULE_END_")); search_init = FALSE; @@ -6536,7 +6561,7 @@ kernel_symbol_search(char *symbol) int get_syment_array(char *symbol, struct syment **sp_array, int max) { - int i, cnt; + int i, j, cnt; struct syment *sp, *sp_end; struct load_module *lm; @@ -6561,6 +6586,31 @@ get_syment_array(char *symbol, struct syment **sp_array, int max) } } + if (!MODULE_MEMORY()) + goto old_module; + + for (i = 0; i < st->mods_installed; i++) { + lm = &st->load_modules[i]; + + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + if (!lm->symtable[j]) + continue; + + sp = lm->symtable[j]; + sp_end = lm->symend[j]; + for (; sp < sp_end; sp++) { + if (STREQ(symbol, sp->name)) { + if (max && (cnt < max)) + sp_array[cnt] = sp; + cnt++; + } + } + } + } + + return cnt; + +old_module: for (i = 0; i < st->mods_installed; i++) { lm = &st->load_modules[i]; sp = lm->mod_symtable; @@ -14712,7 +14762,7 @@ is_downsized(char *name) struct syment * symbol_complete_match(const char *match, struct syment *sp_last) { - int i; + int i, j; struct syment *sp, *sp_end, *sp_start; struct load_module *lm; int search_init; @@ -14732,6 +14782,31 @@ symbol_complete_match(const char *match, struct syment *sp_last) sp_start = NULL; } + if (!MODULE_MEMORY()) + goto old_module; + + for (i = 0; i < st->mods_installed; i++) { + lm = &st->load_modules[i]; + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { + sp_end = lm->symend[j]; + if (!sp_start) + sp_start = lm->symtable[j]; + + if (sp_start < lm->symtable[j] || sp_start > sp_end) + continue; + + for (sp = sp_start; sp < sp_end; sp++) { + if (MODULE_PSEUDO_SYMBOL(sp)) + continue; + + if (STRNEQ(sp->name, match)) + return sp; + } + sp_start = NULL; + } + } + +old_module: search_init = FALSE; for (i = 0; i < st->mods_installed; i++) { -- 2.31.1

On 2023/05/30 14:36, lijiang wrote: Good catch! Thanks. Ugh, will fix. Thanks, Kazu > > Thanks > Lianbo > > + >> +old_module: >> search_init = FALSE; >> >> for (i = 0; i < st->mods_installed; i++) { >> -- >> 2.31.1 >>

On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: This means that it will also skip searching for any *_INIT_* , is it expected behavior? Other changes in this patch are fine to me. Thanks. Lianbo + if (strstr(sp->name, s)) { > + if (print_pad) { > + if (strlen(print_pad)) > + fprintf(fp, "%s", > print_pad); > + show_symbol(sp, 0, > SHOW_RADIX()|SHOW_MODULE); > + } > + if (spp) > + *spp = sp; > + cnt++; > + } > + } > + } else { > + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { > + if (!lm->symtable[j]) > + continue; > + sp = lm->symtable[j]; > + sp_end = lm->symend[j]; > + for (; sp < sp_end; sp++) { > + if (MODULE_PSEUDO_SYMBOL(sp)) > + continue; > + if (strstr(sp->name, s)) { > + if (print_pad) { > + if > (strlen(print_pad)) > fprintf(fp, "%s", print_pad); > + show_symbol(sp, 0, > SHOW_RADIX()|SHOW_MODULE); > + } > + if (spp) > + *spp = sp; > + cnt++; > + } > + } > + } > + } > + } > + return cnt; > +old_module: > search_init = FALSE; > > for (i = 0; i < st->mods_installed; i++) { > @@ -5214,7 +5276,7 @@ symbol_search(char *s) > int > symbol_name_count(char *s) > { > - int i; > + int i, j; > struct syment *sp, *sp_end; > struct load_module *lm; > int count, pseudos, search_init; > @@ -5228,6 +5290,37 @@ symbol_name_count(char *s) > } > } > > + if (!MODULE_MEMORY()) > + goto old_module; > + for (i = 0; i < st->mods_installed; i++) { > + lm = &st->load_modules[i]; > + if (lm->mod_flags & MOD_LOAD_SYMS) { > + sp = lm->mod_load_symtable; > + sp_end = lm->mod_load_symend; > + for (; sp < sp_end; sp++) { > + if (STREQ(s, sp->name)) > + count++; > + } > + } else { > + for (j = MOD_TEXT; j < MOD_MEM_NUM_TYPES; j++) { > + if (!lm->symtable[j]) > + continue; > + sp = lm->symtable[j]; > + sp_end = lm->symend[j]; > + for (; sp < sp_end; sp++) { > + if (STREQ(s, sp->name)) > + count++; > + } > + } > + } > + } > + return count++; > +old_module: > pseudos = (strstr(s, "_MODULE_START_") || strstr(s, > "_MODULE_END_")); > search_init = FALSE; > > -- > 2.31.1 > >

On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: This is a simple fix, could you please put the [patch 15/15] to [patch 01/15] together? BTW: I saw several indent issues in these patches, and also a warning as below: symtab.c: In function ‘void gdb_command_funnel_1(gnu_request*)’: symtab.c:7519:64: warning: ‘%lx’ directive writing between 1 and 16 bytes into a region of size between 10 and 73 [-Wformat-overflow=] 7519 | sprintf(buf, " -s %s 0x%lx", secname, lm->mod_section_data[i].addr); | ^~~ symtab.c:7519:54: note: directive argument in the range [1, 18446744073709551615] 7519 | sprintf(buf, " -s %s 0x%lx", secname, lm->mod_section_data[i].addr); | ^~~~~~~~~~~~~~ In addition, is it possible to put these fixes into their corresponding patches in the next post? I know it might be additional work, but it is very helpful for maintaining and reviewing them. Thanks. Lianbo mkstring(buf3, maxsizelen, RJUST, "SIZE"));

From: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; Without the patch, a __pfx symbol having the same address as its module text pseudo symbol is located at before the pseudo symbol and it is not displayed by "sym" command, after loading module symbols. For example, crash> sym -m dm_mod | head -n 3 ffffffffc07e8000 MODULE TEXT START: dm_mod ffffffffc07e8000 (T) __pfx_dm_send_uevents ffffffffc07e8010 (T) dm_send_uevents crash> mod -s dm_mod crash> sym -m dm_mod | head -n 3 ffffffffc07e8000 MODULE TEXT START: dm_mod ffffffffc07e8010 (T) dm_send_uevents ffffffffc07e81a0 (T) __pfx_dm_path_uevent crash> set debug 1 crash> help -s ... loaded_objfile: 54dd800 ffffffffc07e8000 __pfx_dm_send_uevents << out of MOD_TEXT range ffffffffc07e8000 _MODULE_TEXT_START_dm_mod ffffffffc07e8010 dm_send_uevents ffffffffc07e81a0 __pfx_dm_path_uevent Signed-off-by: Kazuhito Hagio <k-hagio-ab(a)nec.com&gt; --- symbols.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/symbols.c b/symbols.c index a1e54c6f9f98..7153bf9a2dc1 100644 --- a/symbols.c +++ b/symbols.c @@ -3433,7 +3433,8 @@ compare_syms(const void *v1, const void *v2) return -1; if (STRNEQ(s2->name, "__insmod")) return 1; - if (STRNEQ(s2->name, "_MODULE_START_")) + if (MODULE_MEM_START(s2, MOD_TEXT) || + STRNEQ(s2->name, "_MODULE_START_")) return 1; /* Get pseudo section name. */ if (MODULE_SECTION_START(s1)) -- 2.31.1