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@xxxxxxx> 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
--
Crash-utility mailing list -- devel@xxxxxxxxxxxxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxxxxxxxxxxxx
%(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
Contribution Guidelines: https://github.com/crash-utility/crash/wiki
