[Crash-utility] Re: [PATCH 2/2] symbols: fix the error belonging of the kernel modules symbols

On 2023/11/11 11:39, Tao Liu wrote: > Hi Kazu, > > On Fri, Nov 10, 2023 at 1:19 PM HAGIO KAZUHITO(萩尾　一仁) > <k-hagio-ab(a)nec.com&gt; wrote: >> >> On 2023/11/10 12:15, Tao Liu wrote: >> >>>> If a module already does not have its init memory range, it might be >>>> a bit better to not specify "-s .init.text <addr>" to add-symbol-file.. >>>> >>> >>> Thanks a lot for finding the root cause. My patch is just a >>> work-around, and I think your "trial" patch is better to be applied. I >>> agree the ".init.text" should not be added by add-symbol-file, since >>> this section will be freed and will be occupied by other modules after >>> kernel init, which will cause symbols overlap. Could you please draft >>> the "trial" patch to be the formal one? >> >> The trial patch just doesn't add the .init.text unconditionally, but it >> should be required occasionally e.g. a panic when module initialization? >> As crash has a symbol table for a module init range: >> >> crash> help -s >> ... >> mod_base: ffffffffc0092000 >> module_struct: ffffffffc009db00 >> mod_name: floppy >> mod_size: 69417 >> mod_namelist: /home/vmcore/symbol_err/... >> mod_flags: 2a (MOD_LOAD_SYMS|MOD_KALLSYMS|MOD_NOPATCH) >> mod_symtable: 6a28170 >> mod_symend: 6a2a268 >> mod_init_symtable: 0 <<< >> mod_init_symend: 0 <<< >> >> >> So to finish a patch, we will need to look into a few more things: >> >> - It looks like the .init.{text,data} sections are always added on that >> kernel version, whether this is intended or not. For example, they >> might be wrongly added due to a name change of a struct member. >> >> - If it's an intended behavior, how we can exclude the .init sections >> only when it's unnecessary. >> > I agree the .init.{text,data} sections should be loaded if the module > crashes during init. > > Here is a trial patch which worked for me: > > diff --git a/symbols.c b/symbols.c > index 8e8b4c3..24b879d 100644 > --- a/symbols.c > +++ b/symbols.c > @@ -13096,6 +13096,7 @@ add_symbol_file_kallsyms(struct load_module > *lm, struct gnu_request *req) > char buf[BUFSIZE]; > char section_name[BUFSIZE/2]; > ulong section_vaddr; > + long mod_init_module_ptr; > > #if defined(GDB_5_3) || defined(GDB_6_0) || defined(GDB_6_1) > return FALSE; > @@ -13191,6 +13192,10 @@ add_symbol_file_kallsyms(struct load_module > *lm, struct gnu_request *req) > req->buf = GETBUF(buflen = 1024); > retval = FALSE; > > + readmem(lm->module_struct + MODULE_OFFSET2(module_module_init, > rx), KVADDR, > + &mod_init_module_ptr, sizeof(void *), "module.module_init", > FAULT_ON_ERROR); > + fprintf(fp, ">>>>>>>> %lx\n", mod_init_module_ptr); > + > for (done = FALSE; !done; array_entry += SIZE(module_sect_attr)) { > > switch (st->flags & MODSECT_VMASK) > @@ -13283,7 +13288,7 @@ add_symbol_file_kallsyms(struct load_module > *lm, struct gnu_request *req) > shift_string_right(req->buf, strlen(buf)); > BCOPY(buf, req->buf, strlen(buf)); > retval = TRUE; > - } else { > + } else if (mod_init_module_ptr || > strncmp(section_name, ".init.", 6)) { > sprintf(buf, " -s %s 0x%lx", section_name, > section_vaddr); > while ((len + strlen(buf)) >= buflen) { > RESIZEBUF(req->buf, buflen, buflen * 2); > > According to kernel source kernel/module.c:do_init_module, after > module been init successfully, module_init etc will be freed, and it > will be reset to NULL: > > unset_module_init_ro_nx(mod); > module_free(mod, mod->module_init); > mod->module_init = NULL; <<---------- > mod->init_size = 0; > mod->init_ro_size = 0; > mod->init_text_size = 0; > > So in crash, if the module_init member of struct module is NULL, we > can be sure this module has been init successfully and the .init. > section has been freed, so there is no need to load .init. section by > add-symbol-file. However if we encounter the module_init with a value, > the .init. section is not freed and should be loaded by > add-symbol-file. > > Here is a kernel module which will fail during init for testing: > > hello/hello.c: > static int __init hello_world_init(void) { > int ret = 0; > printk(KERN_INFO "Hello, World!\n"); > *(int *)ret = 100; > > return 0; // Initialization successful > } > > crash> mod -s hello /root/hello/hello.ko >>>>>>>>> ffffffffc05e0000 <<----------module_init with value > MODULE NAME BASE > SIZE OBJECT FILE > ffffffffc05dd000 hello ffffffffc05db000 > 12431 /root/hello/hello.ko > > What do you think of the solution? Thank you for considering this. I think a minimal change is better for now, so your solution looks fine to me.

A few slight comments: - lm->mod_init_module_ptr should has the address if available, this can be used?

- STRNEQ() is more preferable than direct strncmp()

Thanks, Kazu