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

Hi, Hatayama Thank you for the fix. > Date: Wed, 8 Dec 2021 12:07:34 +0000 > From: &quot;d.hatayama(a)fujitsu.com&quot; <d.hatayama(a)fujitsu.com&gt; > To: &quot;crash-utility(a)redhat.com&quot; <crash-utility(a)redhat.com&gt; > Subject: [Crash-utility] [PATCH] defs.h: fix breakage of compatibility > of struct symbol_table_data for extension modules > Message-ID: > <TYAPR01MB65079183F79B7F72B9486823956F9(a)TYAPR01MB6507.jpnprd01.prod.outlook.com&gt; > > 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.

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

> > > 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(a)fujitsu.com&gt; > --- > 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(a)redhat.com&gt;