From c69e42869cc4f82c18b25ad41ca6aac8198ee8e6 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 5 Nov 2025 15:23:13 +0100 Subject: [PATCH 1/6] module: Add helper function for reading module_buildid() Add a helper function for reading the optional "build_id" member of struct module. It is going to be used also in ftrace_mod_address_lookup(). Use "#ifdef" instead of "#if IS_ENABLED()" to match the declaration of the optional field in struct module. Signed-off-by: Petr Mladek Reviewed-by: Petr Pavlu Reviewed-by: Daniel Gomez --- include/linux/module.h | 9 +++++++++ kernel/module/kallsyms.c | 9 ++------- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/include/linux/module.h b/include/linux/module.h index e135cc79aceea..4decae2b16755 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -747,6 +747,15 @@ static inline void __module_get(struct module *module) __mod ? __mod->name : "kernel"; \ }) +static inline const unsigned char *module_buildid(struct module *mod) +{ +#ifdef CONFIG_STACKTRACE_BUILD_ID + return mod->build_id; +#else + return NULL; +#endif +} + /* Dereference module function descriptor */ void *dereference_module_function_descriptor(struct module *mod, void *ptr); diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c index 00a60796327c0..0fc11e45df9b9 100644 --- a/kernel/module/kallsyms.c +++ b/kernel/module/kallsyms.c @@ -334,13 +334,8 @@ int module_address_lookup(unsigned long addr, if (mod) { if (modname) *modname = mod->name; - if (modbuildid) { -#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) - *modbuildid = mod->build_id; -#else - *modbuildid = NULL; -#endif - } + if (modbuildid) + *modbuildid = module_buildid(mod); sym = find_kallsyms_symbol(mod, addr, size, offset); From 57278dcc36cfefbc2190c99690186a5810f6156d Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 5 Nov 2025 15:23:14 +0100 Subject: [PATCH 2/6] kallsyms: Cleanup code for appending the module buildid Put the code for appending the optional "buildid" into a helper function, It makes __sprint_symbol() better readable. Also print a warning when the "modname" is set and the "buildid" isn't. It might catch a situation when some lookup function in kallsyms_lookup_buildid() does not handle the "buildid". Use pr_*_once() to avoid an infinite recursion when the function is called from printk(). The recursion is rather theoretical but better be on the safe side. Signed-off-by: Petr Mladek --- kernel/kallsyms.c | 42 +++++++++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 1e76358641247..9455e3bb07fcc 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -423,6 +423,37 @@ int lookup_symbol_name(unsigned long addr, char *symname) return lookup_module_symbol_name(addr, symname); } +#ifdef CONFIG_STACKTRACE_BUILD_ID + +static int append_buildid(char *buffer, const char *modname, + const unsigned char *buildid) +{ + if (!modname) + return 0; + + if (!buildid) { + pr_warn_once("Undefined buildid for the module %s\n", modname); + return 0; + } + + /* build ID should match length of sprintf */ +#ifdef CONFIG_MODULES + static_assert(sizeof(typeof_member(struct module, build_id)) == 20); +#endif + + return sprintf(buffer, " %20phN", buildid); +} + +#else /* CONFIG_STACKTRACE_BUILD_ID */ + +static int append_buildid(char *buffer, const char *modname, + const unsigned char *buildid) +{ + return 0; +} + +#endif /* CONFIG_STACKTRACE_BUILD_ID */ + /* Look up a kernel symbol and return it in a text buffer. */ static int __sprint_symbol(char *buffer, unsigned long address, int symbol_offset, int add_offset, int add_buildid) @@ -445,15 +476,8 @@ static int __sprint_symbol(char *buffer, unsigned long address, if (modname) { len += sprintf(buffer + len, " [%s", modname); -#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) - if (add_buildid && buildid) { - /* build ID should match length of sprintf */ -#if IS_ENABLED(CONFIG_MODULES) - static_assert(sizeof(typeof_member(struct module, build_id)) == 20); -#endif - len += sprintf(buffer + len, " %20phN", buildid); - } -#endif + if (add_buildid) + len += append_buildid(buffer + len, modname, buildid); len += sprintf(buffer + len, "]"); } From 2d220046b96ec128139e04ce80255970c168d3af Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 5 Nov 2025 15:23:15 +0100 Subject: [PATCH 3/6] kallsyms/bpf: Set module buildid in bpf_address_lookup() Make bpf_address_lookup() compatible with module_address_lookup() and clear the pointer to @modbuildid together with @modname. It is not strictly needed because __sprint_symbol() reads @modbuildid only when @modname is set. But better be on the safe side and make the API more safe. Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces") Signed-off-by: Petr Mladek --- include/linux/filter.h | 15 +++++++++++---- kernel/kallsyms.c | 4 ++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/include/linux/filter.h b/include/linux/filter.h index f5c859b8131a3..b7b95840250a5 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -1362,12 +1362,18 @@ struct bpf_prog *bpf_prog_ksym_find(unsigned long addr); static inline int bpf_address_lookup(unsigned long addr, unsigned long *size, - unsigned long *off, char **modname, char *sym) + unsigned long *off, char **modname, + const unsigned char **modbuildid, char *sym) { int ret = __bpf_address_lookup(addr, size, off, sym); - if (ret && modname) - *modname = NULL; + if (ret) { + if (modname) + *modname = NULL; + if (modbuildid) + *modbuildid = NULL; + } + return ret; } @@ -1433,7 +1439,8 @@ static inline struct bpf_prog *bpf_prog_ksym_find(unsigned long addr) static inline int bpf_address_lookup(unsigned long addr, unsigned long *size, - unsigned long *off, char **modname, char *sym) + unsigned long *off, char **modname, + const unsigned char **modbuildid, char *sym) { return 0; } diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 9455e3bb07fcc..efb12b077220c 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -374,8 +374,8 @@ static int kallsyms_lookup_buildid(unsigned long addr, ret = module_address_lookup(addr, symbolsize, offset, modname, modbuildid, namebuf); if (!ret) - ret = bpf_address_lookup(addr, symbolsize, - offset, modname, namebuf); + ret = bpf_address_lookup(addr, symbolsize, offset, + modname, modbuildid, namebuf); if (!ret) ret = ftrace_mod_address_lookup(addr, symbolsize, From 3f956593341b6b2aa3e2ab8a73998eba6412e9ec Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 5 Nov 2025 15:23:16 +0100 Subject: [PATCH 4/6] kallsyms/ftrace: Set module buildid in ftrace_mod_address_lookup() __sprint_symbol() might access an invalid pointer when kallsyms_lookup_buildid() returns a symbol found by ftrace_mod_address_lookup(). The ftrace lookup function must set both @modname and @modbuildid the same way as module_address_lookup(). Fixes: 9294523e3768 ("module: add printk formats to add module build ID to stacktraces") Signed-off-by: Petr Mladek Acked-by: Steven Rostedt (Google) --- include/linux/ftrace.h | 6 ++++-- kernel/kallsyms.c | 4 ++-- kernel/trace/ftrace.c | 5 ++++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 7ded7df6e9b50..a003cf1b32d07 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -87,11 +87,13 @@ struct ftrace_hash; defined(CONFIG_DYNAMIC_FTRACE) int ftrace_mod_address_lookup(unsigned long addr, unsigned long *size, - unsigned long *off, char **modname, char *sym); + unsigned long *off, char **modname, + const unsigned char **modbuildid, char *sym); #else static inline int ftrace_mod_address_lookup(unsigned long addr, unsigned long *size, - unsigned long *off, char **modname, char *sym) + unsigned long *off, char **modname, + const unsigned char **modbuildid, char *sym) { return 0; } diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index efb12b077220c..71868a76e9a13 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -378,8 +378,8 @@ static int kallsyms_lookup_buildid(unsigned long addr, modname, modbuildid, namebuf); if (!ret) - ret = ftrace_mod_address_lookup(addr, symbolsize, - offset, modname, namebuf); + ret = ftrace_mod_address_lookup(addr, symbolsize, offset, + modname, modbuildid, namebuf); return ret; } diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 42bd2ba68a821..11f5096fb60c7 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -7678,7 +7678,8 @@ ftrace_func_address_lookup(struct ftrace_mod_map *mod_map, int ftrace_mod_address_lookup(unsigned long addr, unsigned long *size, - unsigned long *off, char **modname, char *sym) + unsigned long *off, char **modname, + const unsigned char **modbuildid, char *sym) { struct ftrace_mod_map *mod_map; int ret = 0; @@ -7690,6 +7691,8 @@ ftrace_mod_address_lookup(unsigned long addr, unsigned long *size, if (ret) { if (modname) *modname = mod_map->mod->name; + if (modbuildid) + *modbuildid = module_buildid(mod_map->mod); break; } } From dc4dd69c3a7618e768741feb59c77cb62ab06906 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 5 Nov 2025 15:23:17 +0100 Subject: [PATCH 5/6] kallsyms: Clean up @namebuf initialization in kallsyms_lookup_buildid() The function kallsyms_lookup_buildid() initializes the given @namebuf by clearing the first and the last byte. It is not clear why. The 1st byte makes sense because some callers ignore the return code and expect that the buffer contains a valid string, for example: - function_stat_show() - kallsyms_lookup() - kallsyms_lookup_buildid() The initialization of the last byte does not make much sense because it can later be overwritten. Fortunately, it seems that all called functions behave correctly: - kallsyms_expand_symbol() explicitly adds the trailing '\0' at the end of the function. - All *__address_lookup() functions either use the safe strscpy() or they do not touch the buffer at all. Document the reason for clearing the first byte. And remove the useless initialization of the last byte. Signed-off-by: Petr Mladek --- kernel/kallsyms.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 71868a76e9a13..ff7017337535f 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -352,7 +352,12 @@ static int kallsyms_lookup_buildid(unsigned long addr, { int ret; - namebuf[KSYM_NAME_LEN - 1] = 0; + /* + * kallsyms_lookus() returns pointer to namebuf on success and + * NULL on error. But some callers ignore the return value. + * Instead they expect @namebuf filled either with valid + * or empty string. + */ namebuf[0] = 0; if (is_ksym_addr(addr)) { From bd6d77f1cda38a9dcc5187d612bb0d2bb7b8ed50 Mon Sep 17 00:00:00 2001 From: Petr Mladek Date: Wed, 5 Nov 2025 15:23:18 +0100 Subject: [PATCH 6/6] kallsyms: Prevent module removal when printing module name and buildid kallsyms_lookup_buildid() copies the symbol name into the given buffer so that it can be safely read anytime later. But it just copies pointers to mod->name and mod->build_id which might get reused after the related struct module gets removed. The lifetime of struct module is synchronized using RCU. Take the rcu read lock for the entire __sprint_symbol(). Signed-off-by: Petr Mladek --- kernel/kallsyms.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index ff7017337535f..1fda06b6638c4 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -468,6 +468,9 @@ static int __sprint_symbol(char *buffer, unsigned long address, unsigned long offset, size; int len; + /* Prevent module removal until modname and modbuildid are printed */ + guard(rcu)(); + address += symbol_offset; len = kallsyms_lookup_buildid(address, &size, &offset, &modname, &buildid, buffer);