Hi, Kazu
Sorry for the late reply.
On Thu, May 11, 2023 at 12:35 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio...@nec.com>
wrote:
> Supported:
> - startup without "--no_modules"
> - "mod" command (without options)
>
> Signed-off-by: Kazuhito Hagio <k-hagio...@nec.com>
> ---
> 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
>
>
--
Crash-utility mailing list
Crash-utility@redhat.com
https://listman.redhat.com/mailman/listinfo/crash-utility
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
