Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces

[Petr Mladek <pmladek@suse.com>]

On Tue 2021-04-13 15:57:49, Stephen Boyd wrote:
> Quoting Petr Mladek (2021-04-13 08:01:14)
> > On Fri 2021-04-09 18:52:52, Stephen Boyd wrote:
> > > Let's make kernel stacktraces easier to identify by including the build
> > > ID[1] of a module if the stacktrace is printing a symbol from a module.
> > > This makes it simpler for developers to locate a kernel module's full
> > > debuginfo for a particular stacktrace. Combined with
> > > scripts/decode_stracktrace.sh, a developer can download the matching
> > > debuginfo from a debuginfod[2] server and find the exact file and line
> > > number for the functions plus offsets in a stacktrace that match the
> > > module. This is especially useful for pstore crash debugging where the
> > > kernel crashes are recorded in something like console-ramoops and the
> > > recovery kernel/modules are different or the debuginfo doesn't exist on
> > > the device due to space concerns (the debuginfo can be too large for
> > > space limited devices).
> > > 
> > > diff --git a/include/linux/module.h b/include/linux/module.h
> > > index 59f094fa6f74..4bf869f6c944 100644
> > > --- a/include/linux/module.h
> > > +++ b/include/linux/module.h
> > > @@ -11,6 +11,7 @@
> > >  
> > >  #include <linux/list.h>
> > >  #include <linux/stat.h>
> > > +#include <linux/buildid.h>
> > >  #include <linux/compiler.h>
> > >  #include <linux/cache.h>
> > >  #include <linux/kmod.h>
> > > @@ -367,6 +368,9 @@ struct module {
> > >       /* Unique handle for this module */
> > >       char name[MODULE_NAME_LEN];
> > >  
> > > +     /* Module build ID */
> > > +     unsigned char build_id[BUILD_ID_SIZE_MAX];
> > 
> > Do we want to initialize/store the ID even when
> > CONFIG_STACKTRACE_BUILD_ID is disabled and nobody would
> > use it?
> > 
> > Most struct module members are added only when the related feature
> > is enabled.
> > 
> > I am not sure how it would complicate the code. It is possible
> > that it is not worth it. Well, I could imagine that the API
> > will always pass the buildid parameter and
> > module_address_lookup() might do something like
> > 
> > #ifndef CONFIG_STACKTRACE_BUILD_ID
> > static char empty_build_id[BUILD_ID_SIZE_MAX];
> > #endif
> > 
> >                 if (modbuildid) {
> >                         if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID))
> >                                 *modbuildid = mod->build_id;
> >                         else
> >                                 *modbuildid = empty_build_id;
> > 
> > IMHO, this is primary a call for Jessica as the module code maintainer.
> > 
> > Otherwise, I am fine with this patch. And it works as expected.
> > 
> 
> Does declaring mod->build_id as zero length work well enough?

It might be fine because it would actually never get displayed.
But yeah, it is kind of hack. The idea was to avoid too many
#ifdefs in the code.

I think that it is Jessica's call what she would prefer.

Best Regards,
Petr