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

Petr Mladek
2021-05-11T11:35:25Z

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@suse.com>

Best Regards,
Petr