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

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

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

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

>> 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 (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

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