Re: [Crash-utility] [RFC PATCH 01/15] Add support for struct module_memory on Linux 6.4 and later

Hi, Kazu Sorry for the late reply. On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com&gt; wrote: > 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) > > Similar to the KMOD_PAX, we should print the KMOD_MEMORY in the dump_kernel_table().

> #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. */ > Here, It may be good to add the source file: include/linux/module.h to code comment, which is convenient to track the future changes, no need to search for them with grep next time.

+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))) > +*/ > The above two macro definitions can be removed, they have been replaced with the _in_module() implementation, and for now the _in_module() is only called via IN_MODULE() and IN_MODULE_INIT(), so we need to check the MOD_TEXT and MOD_INIT_TEXT in _in_module() for non MODULE_MEMORY() cases. (will comment on the _in_module() later)

> #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 */ > Add the debug info for the KMOD_MEMORY flag2 as below: if (CRASHDEBUG(1)) error(INFO, "The module memory detected.\n");

> + 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 */ > This should reuse the above 4 symbols spaces, so it is 4 plus 10.

+ 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); > The function name "store_module_symbols_v3" is confusing to me, which makes me consider it as KMOD_V3, and it is put into the case KMOD_V2 code block. But it is really not KMOD_V3, just like adding the split module layout(see the crash commit 5a2645623eeb ("Basic support for PaX's split module layout")), for now we are adding the similar module memory. Given that, I would suggest changing the above function name to the store_module_symbols_v6_4() with the kernel version suffix. That can differentiate them and avoid confusion.

+ 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_" > +}; > For the above two definitions, I noticed that they should be in pairs, and associated with another definition mod_mem_type. In addition, this looks like hard code. How about the following changes? #define NAME(s) #s #define MODULE_TAG(TYPE, SE) NAME(_MODULE_ ## TYPE ## _## SE ##_) struct mod_node { char *s; char *e; }; static const struct mod_node module_tags[] = { {MODULE_TAG(TEXT, START), MODULE_TAG(TEXT, END)}, {MODULE_TAG(DATA, START), MODULE_TAG(DATA, END)}, {MODULE_TAG(RODATA, START), MODULE_TAG(RODATA, END)}, {MODULE_TAG(RO_AFTER_INIT, START), MODULE_TAG(RO_AFTER_INIT, END)}, {MODULE_TAG(INIT_TEXT, START), MODULE_TAG(INIT_TEXT, END)}, {MODULE_TAG(INIT_DATA, START), MODULE_TAG(INIT_DATA, END)}, {MODULE_TAG(INIT_RODATA, START), MODULE_TAG(INIT_RODATA, END)}, {NULL, NULL} };

And it can be easy to use as below: + for (i = MOD_TEXT; i < MOD_MEM_NUM_TYPES; i++) { ... + sprintf(buf1, "%s%s", module_tags[i].s, lm->mod_name); + sprintf(buf2, "%s%s", module_tags[i].e, lm->mod_name); + > +/* > + * 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; > Here the strbuf is initialized to a NULL pointer, and later we can drop the first else statement(see comment later). > + 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; > + } > The else code block can be dropped. > + } else > + strbuf = NULL; > The strbuf can be initialized at the beginning of this function.

+ > + > + 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. */ > I tried to remove it and tested again, seems that the sort result is not expected.

+ 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 */ > Could you please explain the details why this is 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; > The above if-else code block is to speed up the searching performance? Or are there overlapped address spaces in different module 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))

+ 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; > + } > For the old module layout(non module memory), only deal with two cases according to the IN_MODULE() and IN_MODULE_INIT(): MOD_TEXT and MOD_INIT_TEXT. Otherwise explicitly says that it is not supported. For example: switch (type) { case MOD_TEXT: base = lm->mod_base; size = lm->mod_size; break; case MOD_INIT_TEXT: base = lm->mod_init_module_ptr; size = lm->mod_init_size; break; default: error(FATAL, "unsupported module layout type!"); }

+ 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; > This function only handles the module memory caes, so need to add a check as below: if (!MODULE_MEMORY()) return 0; Although the module_mem_end() is only invoked in the store_module_symbols_v3(), we can not guarantee that the module_mem_end() won't be called in other functions in future.

+ 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; > +} > BTW: I noticed that they have the similar code between the _in_module() and module_mem_end(), if we can improve the _in_module() and return the end address of a module memory region from _in_module(), instead of only returning TRUE or FALSE, maybe the module_mem_end() can be removed.

I will continue to comment on the remaining patches next week.