| Message ID | 20220421041542.9195-1-maninder1.s@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [modules-next,1/1] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled | expand |
On Thu, Apr 21, 2022 at 09:45:42AM +0530, Maninder Singh wrote: > print module information when KALLSYMS is disabled. > > init_build_id() function is moved to module/main.c as it can be > independent of kallsyms. > > No change for %pB, as it needs to know symbol name to adjust address > value which can't be done without KALLSYMS. > > (A) original output with KALLSYMS: > [8.842129] ps function_1 [crash] > [8.842735] pS function_1+0x4/0x2c [crash] > [8.842890] pSb function_1+0x4/0x2c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > [8.843175] pB function_1+0x4/0x2c [crash] > [8.843362] pBb function_1+0x4/0x2c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > > (B) original output without KALLSYMS: > [12.487424] ps 0xffff800000eb008c > [12.487598] pS 0xffff800000eb008c > [12.487723] pSb 0xffff800000eb008c > [12.487850] pB 0xffff800000eb008c > [12.487967] pBb 0xffff800000eb008c > > (C) With patched kernel > with KALLYSMS: > [41.974576] ps function_1 [crash] > [41.975173] pS function_1+0x4/0x2c [crash] > [41.975386] pSb function_1+0x4/0x2c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > [41.975879] pB function_1+0x4/0x2c [crash] > [41.976076] pBb function_1+0x4/0x2c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > > without KALLSYMS: > [9.624152] ps 0xffff800001bd008c [crash] // similar to original, no changes > [9.624548] pS 0x(____ptrval____)+0x8c [crash] // base address hashed and offset is without hash > [9.624847] pSb 0x(____ptrval____)+0x8c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > [9.625388] pB 0x(____ptrval____)+0x8c [crash] > [9.625594] pBb 0x(____ptrval____)+0x8c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > > with disable hashing: > [8.563916] ps 0xffff800000f2008c [crash] > [8.564574] pS 0xffff800000f20000+0x8c [crash] > [8.564749] pSb 0xffff800000f20000+0x8c [crash 3423a8993a7033fb79e5add14bf9d8d6b56330ca] > [8.565008] pB 0xffff800000f20000+0x8c [crash] > [8.565154] pBb 0xffff800000f20000+0x8c [crash 3423a8993a7033fb79e5add14bf9d8d6b56330ca] > > Suggested-by: Petr Mladek <pmladek@suse.com> > Co-developed-by: Vaneet Narang <v.narang@samsung.com> > Signed-off-by: Vaneet Narang <v.narang@samsung.com> > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > Reviewed-by: Petr Mladek <pmladek@suse.com> > Tested-by: Petr Mladek <pmladek@suse.com> Thanks! Queued onto modules-testing. If there are no complaints from the build I'll move this to modules-next. Luis
On Thu, Apr 21, 2022 at 09:45:42AM +0530, Maninder Singh wrote: > print module information when KALLSYMS is disabled. I'd like this patch reverted from -next. - too many logical changes is a single patch - addition of dangerous API usage - duplicated logic (maybe? hard to review due to the changes) Details below... > > init_build_id() function is moved to module/main.c as it can be > independent of kallsyms. > > No change for %pB, as it needs to know symbol name to adjust address > value which can't be done without KALLSYMS. > > (A) original output with KALLSYMS: > [8.842129] ps function_1 [crash] > [8.842735] pS function_1+0x4/0x2c [crash] > [8.842890] pSb function_1+0x4/0x2c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > [8.843175] pB function_1+0x4/0x2c [crash] > [8.843362] pBb function_1+0x4/0x2c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > > (B) original output without KALLSYMS: (and ptr hashing disabled?) > [12.487424] ps 0xffff800000eb008c > [12.487598] pS 0xffff800000eb008c > [12.487723] pSb 0xffff800000eb008c > [12.487850] pB 0xffff800000eb008c > [12.487967] pBb 0xffff800000eb008c > > (C) With patched kernel > with KALLYSMS: > [41.974576] ps function_1 [crash] > [41.975173] pS function_1+0x4/0x2c [crash] > [41.975386] pSb function_1+0x4/0x2c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > [41.975879] pB function_1+0x4/0x2c [crash] > [41.976076] pBb function_1+0x4/0x2c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > > without KALLSYMS: > [9.624152] ps 0xffff800001bd008c [crash] // similar to original, no changes Why is this not hashed? > [9.624548] pS 0x(____ptrval____)+0x8c [crash] // base address hashed and offset is without hash > [9.624847] pSb 0x(____ptrval____)+0x8c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > [9.625388] pB 0x(____ptrval____)+0x8c [crash] > [9.625594] pBb 0x(____ptrval____)+0x8c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] Why is "____ptrval____" visible here after 9 seconds of boot? I would expect a hashed value to be present. > > with disable hashing: > [8.563916] ps 0xffff800000f2008c [crash] > [8.564574] pS 0xffff800000f20000+0x8c [crash] > [8.564749] pSb 0xffff800000f20000+0x8c [crash 3423a8993a7033fb79e5add14bf9d8d6b56330ca] > [8.565008] pB 0xffff800000f20000+0x8c [crash] > [8.565154] pBb 0xffff800000f20000+0x8c [crash 3423a8993a7033fb79e5add14bf9d8d6b56330ca] > > Suggested-by: Petr Mladek <pmladek@suse.com> > Co-developed-by: Vaneet Narang <v.narang@samsung.com> > Signed-off-by: Vaneet Narang <v.narang@samsung.com> > Signed-off-by: Maninder Singh <maninder1.s@samsung.com> > Reviewed-by: Petr Mladek <pmladek@suse.com> > Tested-by: Petr Mladek <pmladek@suse.com> > --- > include/linux/kallsyms.h | 2 + > include/linux/module.h | 20 ++++++++++ > kernel/kallsyms.c | 27 +++++++------ > kernel/module/internal.h | 11 +++--- > kernel/module/kallsyms.c | 20 ---------- > kernel/module/main.c | 20 ++++++++++ > lib/vsprintf.c | 85 ++++++++++++++++++++++++++++++++++------ > 7 files changed, 133 insertions(+), 52 deletions(-) > > diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h > index ce1bd2fbf23e..72bd24e80545 100644 > --- a/include/linux/kallsyms.h > +++ b/include/linux/kallsyms.h > @@ -89,6 +89,8 @@ extern int sprint_symbol_build_id(char *buffer, unsigned long address); > extern int sprint_symbol_no_offset(char *buffer, unsigned long address); > extern int sprint_backtrace(char *buffer, unsigned long address); > extern int sprint_backtrace_build_id(char *buffer, unsigned long address); > +extern int sprint_kallsym_common(char *buffer, unsigned long address, int build_id, > + int backtrace, int symbol); > > int lookup_symbol_name(unsigned long addr, char *symname); > int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); > diff --git a/include/linux/module.h b/include/linux/module.h > index 46d4d5f2516e..66f4491249c5 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -682,6 +682,20 @@ static inline bool is_livepatch_module(struct module *mod) > > void set_module_sig_enforced(void); > > +static inline int fill_name_build_id(char *buffer, char *modname, > + int add_buildid, const unsigned char *buildid, > + int len) > +{ > + len += sprintf(buffer + len, " [%s", modname); This patch uses sprintf() everywhere. This needs to be using scnprintf(). https://lwn.net/Articles/69419/ > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + if (add_buildid && buildid) { > + /* build ID should match length of sprintf */ > + static_assert(sizeof(typeof_member(struct module, build_id)) == 20); > + len += sprintf(buffer + len, " %20phN", buildid); > + } > +#endif > + return len + sprintf(buffer + len, "]"); > +} > #else /* !CONFIG_MODULES... */ > > static inline struct module *__module_address(unsigned long addr) > @@ -818,6 +832,12 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr) > return ptr; > } > > +static inline int fill_name_build_id(char *buffer, char *modname, > + int add_buildid, const unsigned char *buildid, > + int len) > +{ > + return 0; > +} > #endif /* CONFIG_MODULES */ > > #ifdef CONFIG_SYSFS > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 79f2eb617a62..e6e96b2e0257 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -465,19 +465,8 @@ static int __sprint_symbol(char *buffer, unsigned long address, > if (add_offset) > len += sprintf(buffer + len, "+%#lx/%#lx", offset, size); > > - 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 > - len += sprintf(buffer + len, "]"); > - } > + if (modname) > + len += fill_name_build_id(buffer, modname, add_buildid, buildid, len); Build ID is now part of __sprint_symbol? > > return len; > } > @@ -572,6 +561,18 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address) > return __sprint_symbol(buffer, address, -1, 1, 1); > } > > +int sprint_kallsym_common(char *buffer, unsigned long address, int build_id, > + int backtrace, int symbol) I find the build_id argument order unexpected: I'd think it'd match the ordering of __sprint_symbol? > +{ > + if (backtrace) > + return __sprint_symbol(buffer, address, -1, 1, build_id); > + > + if (symbol) > + return __sprint_symbol(buffer, address, 0, 1, build_id); > + > + return __sprint_symbol(buffer, address, 0, 0, 0); > +} > + > /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */ > struct kallsym_iter { > loff_t pos; > diff --git a/kernel/module/internal.h b/kernel/module/internal.h > index 3e23bef5884d..cfff130f7c5f 100644 > --- a/kernel/module/internal.h > +++ b/kernel/module/internal.h > @@ -206,21 +206,20 @@ static inline void kmemleak_load_module(const struct module *mod, > #endif /* CONFIG_DEBUG_KMEMLEAK */ > > #ifdef CONFIG_KALLSYMS > -void init_build_id(struct module *mod, const struct load_info *info); > void layout_symtab(struct module *mod, struct load_info *info); > void add_kallsyms(struct module *mod, const struct load_info *info); > unsigned long find_kallsyms_symbol_value(struct module *mod, const char *name); > > -static inline bool sect_empty(const Elf_Shdr *sect) > -{ > - return !(sect->sh_flags & SHF_ALLOC) || sect->sh_size == 0; > -} > #else /* !CONFIG_KALLSYMS */ > -static inline void init_build_id(struct module *mod, const struct load_info *info) { } > static inline void layout_symtab(struct module *mod, struct load_info *info) { } > static inline void add_kallsyms(struct module *mod, const struct load_info *info) { } > #endif /* CONFIG_KALLSYMS */ > > +static inline bool sect_empty(const Elf_Shdr *sect) > +{ > + return !(sect->sh_flags & SHF_ALLOC) || sect->sh_size == 0; > +} > + > #ifdef CONFIG_SYSFS > int mod_sysfs_setup(struct module *mod, const struct load_info *info, > struct kernel_param *kparam, unsigned int num_params); > diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c > index 3e11523bc6f6..576a597615a7 100644 > --- a/kernel/module/kallsyms.c > +++ b/kernel/module/kallsyms.c > @@ -209,26 +209,6 @@ void add_kallsyms(struct module *mod, const struct load_info *info) > mod->core_kallsyms.num_symtab = ndst; > } > > -#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > -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; > - } > -} > -#else > -void init_build_id(struct module *mod, const struct load_info *info) > -{ > -} > -#endif > - > /* > * This ignores the intensely annoying "mapping symbols" found > * in ARM ELF files: $a, $t and $d. > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 05a42d8fcd7a..d511a9c62b53 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -2657,6 +2657,26 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname, > > static void cfi_init(struct module *mod); > > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > +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; > + } > +} > +#else > +static inline void init_build_id(struct module *mod, const struct load_info *info) > +{ > +} > +#endif > + > /* > * Allocate and load the module: note that size of section 0 is always > * zero, and we rely on this for optional sections. > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 40d26a07a133..e0aba2c80b8e 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -999,33 +999,92 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev, > } > #endif > > +#if !defined(CONFIG_KALLSYMS) && defined(CONFIG_MODULES) > +static int sprint_module_info(char *buf, unsigned long value, > + int modbuildid, int backtrace, int symbol) > +{ > + struct module *mod; > + unsigned long offset; > + void *base; > + char *modname; > + int len; > + const unsigned char *buildid = NULL; > + bool add_offset; > + > + if (is_ksym_addr(value)) > + return 0; > + > + if (backtrace || symbol) > + add_offset = true; > + else > + add_offset = false; > + > + preempt_disable(); > + mod = __module_address(value); > + if (mod) { > + modname = mod->name; > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + if (modbuildid) > + buildid = mod->build_id; > +#endif > + if (add_offset) { > + base = mod->core_layout.base; > + offset = value - (unsigned long)base; > + } > + } > + preempt_enable(); > + if (!mod) > + return 0; > + > + /* address belongs to module */ > + if (add_offset) > + len = sprintf(buf, "0x%p+0x%lx", base, offset); > + else > + len = sprintf(buf, "0x%lx", value); > + > + return len + fill_name_build_id(buf, modname, modbuildid, buildid, len); Doesn't this duplicate a bunch of logic elsewhere? > +} > +#else > +static inline int sprint_module_info(char *buf, unsigned long value, > + int modbuildid, int backtrace, int symbol) > +{ > + return 0; > +} > +#endif > + > static noinline_for_stack > char *symbol_string(char *buf, char *end, void *ptr, > struct printf_spec spec, const char *fmt) > { > unsigned long value; > -#ifdef CONFIG_KALLSYMS > char sym[KSYM_SYMBOL_LEN]; > -#endif > + int backtrace = 0, symbol = 0, build_id = 0; > > if (fmt[1] == 'R') > ptr = __builtin_extract_return_addr(ptr); > value = (unsigned long)ptr; > > -#ifdef CONFIG_KALLSYMS > - if (*fmt == 'B' && fmt[1] == 'b') > - sprint_backtrace_build_id(sym, value); > - else if (*fmt == 'B') > - sprint_backtrace(sym, value); > - else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) > - sprint_symbol_build_id(sym, value); > - else if (*fmt != 's') > - sprint_symbol(sym, value); > - else > - sprint_symbol_no_offset(sym, value); > + if (fmt[0] == 'B' && fmt[1] == 'b') { > + backtrace = 1; > + build_id = 1; > + } else if (fmt[0] == 'B') > + backtrace = 1; > + else if (fmt[0] == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) { > + symbol = 1; > + build_id = 1; > + } else if (fmt[0] != 's') > + symbol = 1; > + else { > + /* Do Nothing, no offset */ > + } > > +#ifdef CONFIG_KALLSYMS > + sprint_kallsym_common(sym, value, build_id, backtrace, symbol); > return string_nocheck(buf, end, sym, spec); > #else > + if (sprint_module_info(sym, value, build_id, backtrace, symbol)) > + return string_nocheck(buf, end, sym, spec); These take the same arguments, and only differ about their ability to look things up. > + > return special_hex_number(buf, end, value, sizeof(void *)); > #endif > } > -- > 2.17.1 >
Hi, > On Wed, May 11, 2022 at 01:36:56PM +0530, Maninder Singh wrote: > > kallsyms_show_value return false if KALLSYMS is disabled, > > but its usage is done by module.c also. > > Thus when KALLSYMS is disabled, system will not print module > > load address: > > Eek, I hadn't see the other changes this depends on. I think those > changes need to be reworked first. Notably in the other patch, this is > no good: > > /* address belongs to module */ > if (add_offset) > len = sprintf(buf, "0x%p+0x%lx", base, offset); > else > len = sprintf(buf, "0x%lx", value); > > This is printing raw kernel addresses with no hashing, as far as I can > tell. That's not okay at all. > yes same was suggested by Petr also, because earlier we were printing base address also as raw address. https://lkml.org/lkml/2022/2/28/847 but then modified approach to print base address as hash when we are going to show offset of module address, but when we print complete address then we thought of keeping it same as it was: original: [12.487424] ps 0xffff800000eb008c with patch: [9.624152] ps 0xffff800001bd008c [crash] But if its has to be hashed, will fix that also. > Once that other patch gets fixed, this one then can be revisited. > I will check detailed comments on that also > And just on naming: "kallsyms_tiny" is a weird name: it's just "ksyms" > -- there's no "all". :) Ok :) Will name it as knosyms.c (if it seems ok). Thanks, Maninder Singh
On Wed, May 11, 2022 at 03:25:14PM -0700, Kees Cook wrote: > On Thu, Apr 21, 2022 at 09:45:42AM +0530, Maninder Singh wrote: > > print module information when KALLSYMS is disabled. > > I'd like this patch reverted from -next. > - too many logical changes is a single patch > - addition of dangerous API usage > - duplicated logic (maybe? hard to review due to the changes) Yanked out. Luis
Hi Kees, > > I'd like this patch reverted from -next. > - too many logical changes is a single patch ok, will try to break patch in separate patches. > - addition of dangerous API usage sprintf was alraedy there, just changed its position and in current logic it seems not possible to change it. Because sprint_symbol interface is made without len of array int sprint_symbol(char *buffer, unsigned long address) { return __sprint_symbol(buffer, address, 0, 1, 0); } EXPORT_SYMBOL_GPL(sprint_symbol); So either we need to change API declaration for all cases. please suggest, then I can make one separate change to include size of buffer as argument. otherwise there is no benefit to take care of size at some places only. > - duplicated logic (maybe? hard to review due to the changes) We tried to avoid all duplicacies, but will check to clean patch more. > Details below... > > > .. > > > > (C) With patched kernel > > with KALLYSMS: > > [41.974576] ps function_1 [crash] > > [41.975173] pS function_1+0x4/0x2c [crash] > > [41.975386] pSb function_1+0x4/0x2c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > > [41.975879] pB function_1+0x4/0x2c [crash] > > [41.976076] pBb function_1+0x4/0x2c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > > > > without KALLSYMS: > > [9.624152] ps 0xffff800001bd008c [crash] // similar to original, no changes > > Why is this not hashed? > Because without patch also we print complete address in case of %ps, only addition is module name now. if we made it to hashed, then I will make one patch after this to hash it as Andy told it will change current behaviour. > > [9.624548] pS 0x(____ptrval____)+0x8c [crash] // base address hashed and offset is without hash > > [9.624847] pSb 0x(____ptrval____)+0x8c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > > [9.625388] pB 0x(____ptrval____)+0x8c [crash] > > [9.625594] pBb 0x(____ptrval____)+0x8c [crash a8b20caaec9635b316cf4812f6b55598fe2b7cee] > > Why is "____ptrval____" visible here after 9 seconds of boot? I would > expect a hashed value to be present. > Actually I verified it on QEMU setup with minimal roots and in this case hashed values were not present. I tested it on my target;s old kernel for actual hash values. But pasted the logs of latets kernel only with __ptr___ > > > > with disable hashing: > > [8.563916] ps 0xffff800000f2008c [crash] .. > > > > int lookup_symbol_name(unsigned long addr, char *symname); > > int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); > > diff --git a/include/linux/module.h b/include/linux/module.h > > index 46d4d5f2516e..66f4491249c5 100644 > > --- a/include/linux/module.h > > +++ b/include/linux/module.h > > @@ -682,6 +682,20 @@ static inline bool is_livepatch_module(struct module *mod) > > > > void set_module_sig_enforced(void); > > > > +static inline int fill_name_build_id(char *buffer, char *modname, > > + int add_buildid, const unsigned char *buildid, > > + int len) > > +{ > > + len += sprintf(buffer + len, " [%s", modname); > > This patch uses sprintf() everywhere. This needs to be using > scnprintf(). https://lwn.net/Articles/69419/ > These were already present . Moved sprintf from __sprint_symbol to new function fill_name_build_id and same logic added when KALLSYMS is disabled. new API sprint_module_info added 2 new sprintf, which can be taken care of with passing size of buffer. But as already explained either we should take care of all APIs with one separate patch. or we can allow these 2 sprintfs also to make same dsign with KALLSYMS. > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > index 79f2eb617a62..e6e96b2e0257 100644 > > --- a/kernel/kallsyms.c > > +++ b/kernel/kallsyms.c > > @@ -465,19 +465,8 @@ static int __sprint_symbol(char *buffer, unsigned long address, > > if (add_offset) > > len += sprintf(buffer + len, "+%#lx/%#lx", offset, size); > > > > - 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 > > - len += sprintf(buffer + len, "]"); > > - } > > + if (modname) > > + len += fill_name_build_id(buffer, modname, add_buildid, buildid, len); > > Build ID is now part of sprint_module_info? > It was part of sprint_module_info earlier also. just modular the code with new API fill_name_build_id() to call it in case of disabled KALLSYMS also. > > > > return len; > > } > > @@ -572,6 +561,18 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address) > > return __sprint_symbol(buffer, address, -1, 1, 1); > > } > > > > +int sprint_kallsym_common(char *buffer, unsigned long address, int build_id, > > + int backtrace, int symbol) > > I find the build_id argument order unexpected: I'd think it'd match the > ordering of __sprint_symbol? > > > +{ > > + if (backtrace) > > + return __sprint_symbol(buffer, address, -1, 1, build_id); > > + > > + if (symbol) > > + return __sprint_symbol(buffer, address, 0, 1, build_id); > > + > > + return __sprint_symbol(buffer, address, 0, 0, 0); > > +} > > + > > /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */ > > struct kallsym_iter { > > loff_t pos; .. > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index 40d26a07a133..e0aba2c80b8e 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -999,33 +999,92 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev, > > } > > #endif > > .. > > + preempt_enable(); > > + if (!mod) > > + return 0; > > + > > + /* address belongs to module */ > > + if (add_offset) > > + len = sprintf(buf, "0x%p+0x%lx", base, offset); > > + else > > + len = sprintf(buf, "0x%lx", value); > > + > > + return len + fill_name_build_id(buf, modname, modbuildid, buildid, len); > > Doesn't this duplicate a bunch of logic elsewhere? > Yes little bit similar code in kallsyms.c for adding offset but purpose is different. in 1 we need to hash. but will check more if we can make some clean patch set with 2-3 patches rather than from 1 single patch change. > > +} > > +#else > > +static inline int sprint_module_info(char *buf, unsigned long value, > > + int modbuildid, int backtrace, int symbol) > > +{ > > + return 0; > > +} > > +#endif > > + > > static noinline_for_stack > > char *symbol_string(char *buf, char *end, void *ptr, > > struct printf_spec spec, const char *fmt) > > { > > unsigned long value; > > -#ifdef CONFIG_KALLSYMS > > char sym[KSYM_SYMBOL_LEN]; > > -#endif > > + int backtrace = 0, symbol = 0, build_id = 0; > > > > if (fmt[1] == 'R') > > ptr = __builtin_extract_return_addr(ptr); > > value = (unsigned long)ptr; > > > > -#ifdef CONFIG_KALLSYMS > > - if (*fmt == 'B' && fmt[1] == 'b') > > - sprint_backtrace_build_id(sym, value); > > - else if (*fmt == 'B') > > - sprint_backtrace(sym, value); > > - else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) > > - sprint_symbol_build_id(sym, value); > > - else if (*fmt != 's') > > - sprint_symbol(sym, value); > > - else > > - sprint_symbol_no_offset(sym, value); > > + if (fmt[0] == 'B' && fmt[1] == 'b') { > > + backtrace = 1; > > + build_id = 1; > > + } else if (fmt[0] == 'B') > > + backtrace = 1; > > + else if (fmt[0] == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) { > > + symbol = 1; > > + build_id = 1; > > + } else if (fmt[0] != 's') > > + symbol = 1; > > + else { > > + /* Do Nothing, no offset */ > > + } > > > > +#ifdef CONFIG_KALLSYMS > > + sprint_kallsym_common(sym, value, build_id, backtrace, symbol); > > return string_nocheck(buf, end, sym, spec); > > #else > > + if (sprint_module_info(sym, value, build_id, backtrace, symbol)) > > + return string_nocheck(buf, end, sym, spec); > > These take the same arguments, and only differ about their ability to > look things up. > Yes, Will make both API with same name, it will clean the code more. > > + > > return special_hex_number(buf, end, value, sizeof(void *)); > > #endif > > } > > -- > > 2.17.1 > > > > -- Thanks for input, please suggest about sprintf changes, then I will send new patches with more clean code. ---- Maninder Singh
On Tue 2022-05-17 09:28:10, Maninder Singh wrote: > Hi Kees, > > > > > I'd like this patch reverted from -next. > > - too many logical changes is a single patch > > ok, will try to break patch in separate patches. > > > - addition of dangerous API usage > > sprintf was alraedy there, just changed its position > and in current logic it seems not possible to change it. > > Because sprint_symbol interface is made without len of array > > int sprint_symbol(char *buffer, unsigned long address) > { > return __sprint_symbol(buffer, address, 0, 1, 0); > } > EXPORT_SYMBOL_GPL(sprint_symbol); Sigh, the kallsyms API is not safe in general. For example, the following functions have the same problem: unsigned long kallsyms_lookup_name(const char *name); const char *kallsyms_lookup(unsigned long addr, unsigned long *symbolsize, unsigned long *offset, char **modname, char *namebuf); int lookup_symbol_name(unsigned long addr, char *symname); int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); > So either we need to change API declaration for all cases. > please suggest, then I can make one separate change to include > size of buffer as argument. It would be lovely if you could fix the API. > otherwise there is no benefit to take care of size at some places only. Well, the sprint_*() APIs are more dangerous because the underlying __sprint_symbol() could do several sprintf() calls. It is not easy to compute the sufficient buffer size. The *lookup*() APIs are slightly more safe because the buffer is just for the symbol name. The size always should be KSYM_NAME_LEN. Anyway, it would be great to make it safe as well. Best Regards, Petr
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h index ce1bd2fbf23e..72bd24e80545 100644 --- a/include/linux/kallsyms.h +++ b/include/linux/kallsyms.h @@ -89,6 +89,8 @@ extern int sprint_symbol_build_id(char *buffer, unsigned long address); extern int sprint_symbol_no_offset(char *buffer, unsigned long address); extern int sprint_backtrace(char *buffer, unsigned long address); extern int sprint_backtrace_build_id(char *buffer, unsigned long address); +extern int sprint_kallsym_common(char *buffer, unsigned long address, int build_id, + int backtrace, int symbol); int lookup_symbol_name(unsigned long addr, char *symname); int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long *offset, char *modname, char *name); diff --git a/include/linux/module.h b/include/linux/module.h index 46d4d5f2516e..66f4491249c5 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -682,6 +682,20 @@ static inline bool is_livepatch_module(struct module *mod) void set_module_sig_enforced(void); +static inline int fill_name_build_id(char *buffer, char *modname, + int add_buildid, const unsigned char *buildid, + int len) +{ + len += sprintf(buffer + len, " [%s", modname); +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) + if (add_buildid && buildid) { + /* build ID should match length of sprintf */ + static_assert(sizeof(typeof_member(struct module, build_id)) == 20); + len += sprintf(buffer + len, " %20phN", buildid); + } +#endif + return len + sprintf(buffer + len, "]"); +} #else /* !CONFIG_MODULES... */ static inline struct module *__module_address(unsigned long addr) @@ -818,6 +832,12 @@ void *dereference_module_function_descriptor(struct module *mod, void *ptr) return ptr; } +static inline int fill_name_build_id(char *buffer, char *modname, + int add_buildid, const unsigned char *buildid, + int len) +{ + return 0; +} #endif /* CONFIG_MODULES */ #ifdef CONFIG_SYSFS diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 79f2eb617a62..e6e96b2e0257 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -465,19 +465,8 @@ static int __sprint_symbol(char *buffer, unsigned long address, if (add_offset) len += sprintf(buffer + len, "+%#lx/%#lx", offset, size); - 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 - len += sprintf(buffer + len, "]"); - } + if (modname) + len += fill_name_build_id(buffer, modname, add_buildid, buildid, len); return len; } @@ -572,6 +561,18 @@ int sprint_backtrace_build_id(char *buffer, unsigned long address) return __sprint_symbol(buffer, address, -1, 1, 1); } +int sprint_kallsym_common(char *buffer, unsigned long address, int build_id, + int backtrace, int symbol) +{ + if (backtrace) + return __sprint_symbol(buffer, address, -1, 1, build_id); + + if (symbol) + return __sprint_symbol(buffer, address, 0, 1, build_id); + + return __sprint_symbol(buffer, address, 0, 0, 0); +} + /* To avoid using get_symbol_offset for every symbol, we carry prefix along. */ struct kallsym_iter { loff_t pos; diff --git a/kernel/module/internal.h b/kernel/module/internal.h index 3e23bef5884d..cfff130f7c5f 100644 --- a/kernel/module/internal.h +++ b/kernel/module/internal.h @@ -206,21 +206,20 @@ static inline void kmemleak_load_module(const struct module *mod, #endif /* CONFIG_DEBUG_KMEMLEAK */ #ifdef CONFIG_KALLSYMS -void init_build_id(struct module *mod, const struct load_info *info); void layout_symtab(struct module *mod, struct load_info *info); void add_kallsyms(struct module *mod, const struct load_info *info); unsigned long find_kallsyms_symbol_value(struct module *mod, const char *name); -static inline bool sect_empty(const Elf_Shdr *sect) -{ - return !(sect->sh_flags & SHF_ALLOC) || sect->sh_size == 0; -} #else /* !CONFIG_KALLSYMS */ -static inline void init_build_id(struct module *mod, const struct load_info *info) { } static inline void layout_symtab(struct module *mod, struct load_info *info) { } static inline void add_kallsyms(struct module *mod, const struct load_info *info) { } #endif /* CONFIG_KALLSYMS */ +static inline bool sect_empty(const Elf_Shdr *sect) +{ + return !(sect->sh_flags & SHF_ALLOC) || sect->sh_size == 0; +} + #ifdef CONFIG_SYSFS int mod_sysfs_setup(struct module *mod, const struct load_info *info, struct kernel_param *kparam, unsigned int num_params); diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c index 3e11523bc6f6..576a597615a7 100644 --- a/kernel/module/kallsyms.c +++ b/kernel/module/kallsyms.c @@ -209,26 +209,6 @@ void add_kallsyms(struct module *mod, const struct load_info *info) mod->core_kallsyms.num_symtab = ndst; } -#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) -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; - } -} -#else -void init_build_id(struct module *mod, const struct load_info *info) -{ -} -#endif - /* * This ignores the intensely annoying "mapping symbols" found * in ARM ELF files: $a, $t and $d. diff --git a/kernel/module/main.c b/kernel/module/main.c index 05a42d8fcd7a..d511a9c62b53 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2657,6 +2657,26 @@ static int unknown_module_param_cb(char *param, char *val, const char *modname, static void cfi_init(struct module *mod); +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) +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; + } +} +#else +static inline void init_build_id(struct module *mod, const struct load_info *info) +{ +} +#endif + /* * Allocate and load the module: note that size of section 0 is always * zero, and we rely on this for optional sections. diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 40d26a07a133..e0aba2c80b8e 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -999,33 +999,92 @@ char *bdev_name(char *buf, char *end, struct block_device *bdev, } #endif +#if !defined(CONFIG_KALLSYMS) && defined(CONFIG_MODULES) +static int sprint_module_info(char *buf, unsigned long value, + int modbuildid, int backtrace, int symbol) +{ + struct module *mod; + unsigned long offset; + void *base; + char *modname; + int len; + const unsigned char *buildid = NULL; + bool add_offset; + + if (is_ksym_addr(value)) + return 0; + + if (backtrace || symbol) + add_offset = true; + else + add_offset = false; + + preempt_disable(); + mod = __module_address(value); + if (mod) { + modname = mod->name; +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) + if (modbuildid) + buildid = mod->build_id; +#endif + if (add_offset) { + base = mod->core_layout.base; + offset = value - (unsigned long)base; + } + } + preempt_enable(); + if (!mod) + return 0; + + /* address belongs to module */ + if (add_offset) + len = sprintf(buf, "0x%p+0x%lx", base, offset); + else + len = sprintf(buf, "0x%lx", value); + + return len + fill_name_build_id(buf, modname, modbuildid, buildid, len); +} +#else +static inline int sprint_module_info(char *buf, unsigned long value, + int modbuildid, int backtrace, int symbol) +{ + return 0; +} +#endif + static noinline_for_stack char *symbol_string(char *buf, char *end, void *ptr, struct printf_spec spec, const char *fmt) { unsigned long value; -#ifdef CONFIG_KALLSYMS char sym[KSYM_SYMBOL_LEN]; -#endif + int backtrace = 0, symbol = 0, build_id = 0; if (fmt[1] == 'R') ptr = __builtin_extract_return_addr(ptr); value = (unsigned long)ptr; -#ifdef CONFIG_KALLSYMS - if (*fmt == 'B' && fmt[1] == 'b') - sprint_backtrace_build_id(sym, value); - else if (*fmt == 'B') - sprint_backtrace(sym, value); - else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) - sprint_symbol_build_id(sym, value); - else if (*fmt != 's') - sprint_symbol(sym, value); - else - sprint_symbol_no_offset(sym, value); + if (fmt[0] == 'B' && fmt[1] == 'b') { + backtrace = 1; + build_id = 1; + } else if (fmt[0] == 'B') + backtrace = 1; + else if (fmt[0] == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) { + symbol = 1; + build_id = 1; + } else if (fmt[0] != 's') + symbol = 1; + else { + /* Do Nothing, no offset */ + } +#ifdef CONFIG_KALLSYMS + sprint_kallsym_common(sym, value, build_id, backtrace, symbol); return string_nocheck(buf, end, sym, spec); #else + if (sprint_module_info(sym, value, build_id, backtrace, symbol)) + return string_nocheck(buf, end, sym, spec); + return special_hex_number(buf, end, value, sizeof(void *)); #endif }