Re: [PATCH] defs.h: fix breakage of compatibility of struct symbol_table_data for extension modules

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Dec 10, 2021 at 12:24 PM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab@xxxxxxx> wrote:
>
> -----Original Message-----
> > Hi, Hatayama
> > Thank you for the fix.
> >
> > > Date: Wed, 8 Dec 2021 12:07:34 +0000
> > > From: "d.hatayama@xxxxxxxxxxx" <d.hatayama@xxxxxxxxxxx>
> > > To: "crash-utility@xxxxxxxxxx" <crash-utility@xxxxxxxxxx>
> > > Subject:  [PATCH] defs.h: fix breakage of compatibility
> > >         of struct symbol_table_data for extension modules
> > > Message-ID:
> > >         <TYAPR01MB65079183F79B7F72B9486823956F9@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > >
> > > Content-Type: text/plain; charset="iso-2022-jp"
> > >
> > > Commit 2fab8fbc0c4f1c4cbe889de4cead5f7457a19f77 (symbols: Implement
> > > install and remove operations for mod_symname_hash) added new member
> > > variable mod_symname_hash in the middle of struct symbol_table_data:
> > >
> > >     diff --git a/defs.h b/defs.h
> > >     index cbd45e5..bbdca79 100644
> > >     --- a/defs.h
> > >     +++ b/defs.h
> > >     @@ -2755,6 +2755,7 @@ struct symbol_table_data {
> > >              double val_hash_searches;
> > >              double val_hash_iterations;
> > >              struct syment *symname_hash[SYMNAME_HASH];
> > >     +       struct syment *mod_symname_hash[SYMNAME_HASH];
> > >             struct symbol_namespace kernel_namespace;
> > >             struct syment *ext_module_symtable;
> > >             struct syment *ext_module_symend;
> > >
> > > which breaks compatibility of struct symbol_table_data for extension
> > > modules. In general, new member variables must be added at the end of
> > > structures to maintain compatibility of data structures shared among
> > > extension modules.
>
> oh, I've only checked a part of tables, thanks for pointing this out.
>
> Acked-by: Kazuhito hagio <k-hagio-ab@xxxxxxx>
>

Applied and modified the patch log.
https://github.com/crash-utility/crash/commit/6968345893178d2750b8872055498d2a6010a861

> Lianbo, we need to backport this to RHEL8.6 and RHEL9.0 crash before GA.
>
Sure. Thanks.

Lianbo

> >
> > Yes. Crash utility has documented the rules, see wiki:
> > https://github.com/crash-utility/crash/wiki. But, this seems to be
> > ignored again.
> > "If you add offset/size/array_length to each table, they have to be
> > appended to the end of the tables.This is to avoid breaking previously
> > compiled extension modules. Also, please add them to
> > dump_offset_table() (in arbitrarily order)."
>
> I'll improve the guideline to cover more tables..
>
> Thanks,
> Kazu
>
> >
> > >
> > >
> > > For example, as the result, crash trace command results in
> > > segmentation fault:
> > >
> > >     crash> trace show
> > >
> > >     Segmentation fault (core dumped)
> > >
> > > in context of save_proc_kallsyms():
> > >
> > >     (gdb) bt
> > >     #0  save_proc_kallsyms (fd=<optimized out>) at
> > /usr/src/debug/crash-trace-command-3.0-4.el9.x86_64/trace.c:2234
> > >     #1  __trace_cmd_data_output (fd=<optimized out>) at
> > /usr/src/debug/crash-trace-command-3.0-4.el9.x86_64/trace.c:2538
> > >     #2  0x00007f804e068dd5 in trace_cmd_data_output (fd=<optimized out>) at
> > /usr/src/debug/crash-trace-command-3.0-4.el9.x86_64/trace.c:2588
> > >     #3  ftrace_show (argc=919286707, argv=<optimized out>, argv=<optimized out>) at
> > /usr/src/debug/crash-trace-command-3.0-4.el9.x86_64/trace.c:1825
> > >     #4  0x00000000005010de in exec_command () at main.c:892
> > >     #5  0x0000000000500ed0 in main_loop () at main.c:839
> > >     #6  0x000000000085590d in captured_main (data=<optimized out>) at main.c:1284
> > >     #7  gdb_main (args=<optimized out>) at main.c:1313
> > >     #8  0x0000000000855965 in gdb_main_entry (argc=<optimized out>, argv=<optimized out>) at main.c:1338
> > >     #9  0x00000000005b3db7 in gdb_main_loop (argc=2, argv=0x7ffc5afb3ed8) at gdb_interface.c:81
> > >     #10 0x0000000000500b8d in main (argc=3, argv=0x7ffc5afb3ed8) at main.c:720
> > >
> > > when referring to sp->name:
> > >
> > >     static int save_proc_kallsyms(int fd)
> > >     {
> > >             int i;
> > >             struct syment *sp;
> > >
> > >             for (sp = st->symtable; sp < st->symend; sp++)
> > >                     tmp_fprintf("%lx %c %s\n", sp->value, sp->type, sp->name);
> > >
> > >             for (i = 0; i < st->mods_installed; i++) {
> > >                     struct load_module *lm = &st->load_modules[i];
> > >
> > >                     for (sp = lm->mod_symtable; sp <= lm->mod_symend; sp++) {
> > >     =>                  if (!strncmp(sp->name, "_MODULE_", strlen("_MODULE_")))
> > >                                     continue;
> > >
> > > where sp->name contains an odd address:
> > >
> > >     (gdb) p sp->name
> > >     Cannot access memory at address 0x20047
> > >
> > > As seen above, save_proc_kallsyms() refers to st->load_modules and its
> > > position is behind the position where mod_symname_hash was added at:
> > >
> > >         double val_hash_iterations;
> > >         struct syment *symname_hash[SYMNAME_HASH];
> > >         struct syment *mod_symname_hash[SYMNAME_HASH];
> > >         struct symbol_namespace kernel_namespace;
> > >         struct syment *ext_module_symtable;
> > >         struct syment *ext_module_symend;
> > >         long ext_module_symcnt;
> > >         struct symbol_namespace ext_module_namespace;
> > >         int mods_installed;
> > >         struct load_module *current;
> > >         struct load_module *load_modules;
> > >
> > > Signed-off-by: HATAYAMA Daisuke <d.hatayama@xxxxxxxxxxx>
> > > ---
> > >  defs.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/defs.h b/defs.h
> > > index 7e2a16e..e9e8143 100644
> > > --- a/defs.h
> > > +++ b/defs.h
> > > @@ -2753,7 +2753,6 @@ struct symbol_table_data {
> > >          double val_hash_searches;
> > >          double val_hash_iterations;
> > >          struct syment *symname_hash[SYMNAME_HASH];
> > > -       struct syment *mod_symname_hash[SYMNAME_HASH];
> > >         struct symbol_namespace kernel_namespace;
> > >         struct syment *ext_module_symtable;
> > >         struct syment *ext_module_symend;
> > > @@ -2780,6 +2779,7 @@ struct symbol_table_data {
> > >         ulong kaiser_init_vmlinux;
> > >         int kernel_symbol_type;
> > >         ulong linux_banner_vmlinux;
> > > +       struct syment *mod_symname_hash[SYMNAME_HASH];
> > >  };
> > >
> > >  /* flags for st */
> > > --
> > > 2.31.1
> >
> > Ackec-by: Lianbo Jiang <lijiang@xxxxxxxxxx>


--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility




[Index of Archives]     [Fedora Development]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]

 

Powered by Linux