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

Friday, 19 May 2023

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?

+               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.

Thanks.
Lianbo

-- 
...
 2.31.1

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

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