Hi Lianbo,
thank you for the comments.
On 2023/05/19 18:06, lijiang wrote:
> Hi, Kazu
> Sorry for the late reply.
> On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx>
> wrote:
>
>> Supported:
>> - startup without "--no_modules"
>> - "mod" command (without options)
>>
>> Signed-off-by: Kazuhito Hagio <k-hagio-ab@xxxxxxx>
>> ---
>> 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().
Yes.
>
>
>> #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.
OK.
>
> +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)
Yes, I just left them as a comment for reference during this
development, and they will be removed.
>
>
>> #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");
OK.
>
>
>> + 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.
Yes, right.
>
> + 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.
Yes, I had the same impression about this.
I would pefer store_module_symbols_6_4() for kernel version alignment.
>
> + 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}
> };
I don't like complicated code, but will think about something like this.
>
> 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.
OK.
>
> +
>> +
>> + 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.
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.
>
> + 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?
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?
>
>
>> + 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?
To realize these:
>> +#define IN_MODULE(A,L) (_in_module(A, L, MOD_TEXT))
>> +#define IN_MODULE_INIT(A,L) (_in_module(A, L, MOD_INIT_TEXT))
but finally, these are replaced in 5/15:
https://listman.redhat.com/archives/crash-utility/2023-May/010653.html
>
> + 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!");
> }
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.
>
> + 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.
OK.
>
> + 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.
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
>
> I will continue to comment on the remaining patches next week.
Thanks!
Kazu
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
