Hi Kazu,
On Mon, Nov 13, 2023 at 2:46 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab@xxxxxxx> wrote:
>
> 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.
>
Thanks for providing the feedback!
> A few slight comments:
>
> - lm->mod_init_module_ptr should has the address if available, this can
> be used?
Sorry I didn't get your point. The value here is only used as a flag
to indicate if the .init. section has been freed or not. Since the
value is important, other place in crash also used it and read it
there, symbol.c:store_module_symbols_v2() . Do you mean to share the
value in different places so crash only read it once?
> - STRNEQ() is more preferable than direct strncmp()
>
Agreed, will get it updated in v2.
Thanks,
Tao Liu
> 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
