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

[Petr Mladek]

On Mon 2021-05-10 17:38:37, 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.

> --- 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>
> @@ -364,6 +365,11 @@ struct module {
>  	/* Unique handle for this module */
>  	char name[MODULE_NAME_LEN];
>  
> +#ifdef CONFIG_STACKTRACE_BUILD_ID
> +	/* Module build ID */
> +	unsigned char build_id[BUILD_ID_SIZE_MAX];
> +#endif
> +
>  	/* Sysfs stuff. */
>  	struct module_kobject mkobj;
>  	struct module_attribute *modinfo_attrs;
> @@ -627,7 +633,7 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr);
>  const char *module_address_lookup(unsigned long addr,
>  			    unsigned long *symbolsize,
>  			    unsigned long *offset,
> -			    char **modname,
> +			    char **modname, const unsigned char **modbuildid,

Nit: It would have been better to follow the existing style and put only one
     parameter per-line.

Please, fix it in the next version if there is any. But no need
to resend it just because of this unless Jessica asks for it.

>  			    char *namebuf);
>  int lookup_module_symbol_name(unsigned long addr, char *symname);
>  int lookup_module_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name);

> diff --git a/kernel/module.c b/kernel/module.c
> index 30479355ab85..a5eaf5188a83 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4235,7 +4257,7 @@ void * __weak dereference_module_function_descriptor(struct module *mod,
>  const char *module_address_lookup(unsigned long addr,
>  			    unsigned long *size,
>  			    unsigned long *offset,
> -			    char **modname,
> +			    char **modname, const unsigned char **modbuildid,

Same here.

>  			    char *namebuf)
>  {
>  	const char *ret = NULL;

Otherwise, it looks good.

Reviewed-by: Petr Mladek <pmladek@xxxxxxxx>

Best Regards,
Petr