Quoting Jessica Yu (2021-04-15 06:04:35)
> +++ Stephen Boyd [09/04/21 18:52 -0700]:
> >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];
>
> Hi Stephen,
>
> Since this field is not used when !CONFIG_STACKTRACE_BUILD_ID, I
> would prefer to wrap this in an ifdef, similar to the other
> CONFIG-dependent fields in struct module. This makes it explicit under
> what conditions (i.e. config) the field is meant to be used.
Ok will do.
> >diff --git a/kernel/module.c b/kernel/module.c
> >index 30479355ab85..6f5bc1b046a5 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -2770,6 +2771,20 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
> > }
> > mod->core_kallsyms.num_symtab = ndst;
> > }
> >+
> >+static void init_build_id(struct module *mod, const struct load_info *info)
> >+{
> >+ const Elf_Shdr *sechdr;
> >+ unsigned int i;
> >+
> >+ for (i = 0; i < info->hdr->e_shnum; i++) {
> >+ sechdr = &info->sechdrs[i];
> >+ if (!sect_empty(sechdr) && sechdr->sh_type == SHT_NOTE &&
> >+ !build_id_parse_buf((void *)sechdr->sh_addr, mod->build_id,
> >+ sechdr->sh_size))
> >+ break;
> >+ }
>
> If mod->build_id is not used when !CONFIG_STACKTRACE_BUILD_ID, then we
> don't need to look for it. I would be fine with wrapping the function
> body in an ifdef (similar to what we currently do in
> del_usage_links() and do_mod_ctors()).
Ok, done.
>
> >+}
> > #else
> > static inline void layout_symtab(struct module *mod, struct load_info *info)
> > {
> >@@ -2778,6 +2793,10 @@ static inline void layout_symtab(struct module *mod, struct load_info *info)
> > static void add_kallsyms(struct module *mod, const struct load_info *info)
> > {
> > }
> >+
> >+static void init_build_id(struct module *mod, const struct load_info *info)
> >+{
> >+}
> > #endif /* CONFIG_KALLSYMS */
> >
> > static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug, unsigned int num)
> >@@ -4004,6 +4023,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > goto free_arch_cleanup;
> > }
> >
> >+ init_build_id(mod, info);
> > dynamic_debug_setup(mod, info->debug, info->num_debug);
> >
> > /* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
> >@@ -4235,7 +4255,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,
> > char *namebuf)
> > {
> > const char *ret = NULL;
> >@@ -4246,6 +4266,8 @@ const char *module_address_lookup(unsigned long addr,
> > if (mod) {
> > if (modname)
> > *modname = mod->name;
> >+ if (modbuildid)
> >+ *modbuildid = mod->build_id;
>
> Then maybe we can set *modbuildid = NULL in the case of
> !CONFIG_STACKTRACE_BUILD_ID, similar to the kernel case in
> kallsyms_lookup_buildid().
>
Sounds good. It means that some more ifdefs are probably required vs.
making the array size be 0 when the config is disabled but that isn't a
big problem for me. I'm reworking the code now and will test and then
send v5 shortly. Thanks!
