| Message ID | 20220228053447.1584704-1-maninder1.s@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled | expand |
On Mon, Feb 28, 2022 at 11:04:47AM +0530, Maninder Singh wrote: > with commit '82b37e632513 ("kallsyms: print module name in %ps/S > case when KALLSYMS is disabled"), module name printing was enhanced. > > As per suggestion from Petr Mladek <pmladek@suse.com>, covering > other flavours also to print build id also. > > for %pB no change as it needs to know symbol name to adjust address > value which can't be done without KALLSYMS. > > 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] > > original output without KALLSYMS: > [12.487424] ps 0xffff800000eb008c > [12.487598] pS 0xffff800000eb008c > [12.487723] pSb 0xffff800000eb008c > [12.487850] pB 0xffff800000eb008c > [12.487967] pBb 0xffff800000eb008c > > With patched kernel without KALLSYMS: > [9.205207] ps 0xffff800000eb008c [crash] > [9.205564] pS 0xffff800000eb0000+0x8c [crash] > [9.205757] pSb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > [9.206066] pB 0xffff800000eb0000+0x8c [crash] > [9.206257] pBb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] ... > +static int sprint_module_info(char *buf, char *end, unsigned long value, > + const char *fmt) > +{ > + struct module *mod; > + unsigned long offset = 1; > + unsigned long base; > + int ret = 0; This is hard to find if it's not close to the first use. Since you are using positive numbers... > + const char *modname; > + int modbuildid = 0; > + int len; > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + const unsigned char *buildid = NULL; > +#endif > + > + if (is_ksym_addr(value)) > + return 0; > + if (*fmt == 'B' && fmt[1] == 'b') > + modbuildid = 1; > + else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) Why not to split to two conditionals? Would be easier to get, > + modbuildid = 1; > + else if (*fmt != 's') { These all are inconsistent, please switch to fmt[0]. > + /* > + * do nothing. > + */ > + } else > + offset = 0; > + > + preempt_disable(); > + mod = __module_address(value); > + if (mod) { > + ret = 1; > + modname = mod->name; > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + if (modbuildid) > + buildid = mod->build_id; > +#endif > + if (offset) { > + base = (unsigned long)mod->core_layout.base; > + offset = value - base; > + } > + } > + > + preempt_enable(); > + if (!ret) This looks a bit strange, but okay, I'm not familiar with the function of this code. > + return 0; > + > + /* address belongs to module */ > + if (offset) > + len = sprintf(buf, "0x%lx+0x%lx", base, offset); > + else > + len = sprintf(buf, "0x%lx", value); > + > + len += sprintf(buf + len, " [%s", modname); > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > + if (modbuildid && buildid) { > + /* build ID should match length of sprintf */ > + static_assert(sizeof(typeof_member(struct module, build_id)) == 20); > + len += sprintf(buf + len, " %20phN", buildid); > + } > +#endif > + len += sprintf(buf + len, "]"); > + > + return len; > +}
On Mon, Feb 28, 2022 at 11:04:47AM +0530, Maninder Singh wrote: > with commit '82b37e632513 ("kallsyms: print module name in %ps/S > case when KALLSYMS is disabled"), module name printing was enhanced. > > As per suggestion from Petr Mladek <pmladek@suse.com>, covering > other flavours also to print build id also. > > for %pB no change as it needs to know symbol name to adjust address > value which can't be done without KALLSYMS. > > 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] > > original output without KALLSYMS: > [12.487424] ps 0xffff800000eb008c > [12.487598] pS 0xffff800000eb008c > [12.487723] pSb 0xffff800000eb008c > [12.487850] pB 0xffff800000eb008c > [12.487967] pBb 0xffff800000eb008c > > With patched kernel without KALLSYMS: > [9.205207] ps 0xffff800000eb008c [crash] > [9.205564] pS 0xffff800000eb0000+0x8c [crash] > [9.205757] pSb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > [9.206066] pB 0xffff800000eb0000+0x8c [crash] > [9.206257] pBb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > > 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> > --- > earlier discussion: https://lkml.org/lkml/2022/2/10/185 > > include/linux/kallsyms.h | 27 -------------- > kernel/module.c | 4 +-- See Aaron's work which you'll need to base your work on: https://lkml.kernel.org/r/20220222141303.1392190-1-atomlin@redhat.com Soon Aaron will post a v9. Luis
Adding Kees into Cc. This patch allows to see non-hashed base address of the module and eventually of vmlinux, see below. On Mon 2022-02-28 14:03:30, Andy Shevchenko wrote: > On Mon, Feb 28, 2022 at 11:04:47AM +0530, Maninder Singh wrote: > > with commit '82b37e632513 ("kallsyms: print module name in %ps/S > > case when KALLSYMS is disabled"), module name printing was enhanced. The commit does not exist. Note that linux-next is regularly rebased. Commit IDs might still be stable when they are merged from a maintainer git tree. But Andrew's -mm tree is imported from quilt and the patches always get new commit ID. The best solution is to handle the changes in a single patchset. > > As per suggestion from Petr Mladek <pmladek@suse.com>, covering > > other flavours also to print build id also. > > > > for %pB no change as it needs to know symbol name to adjust address > > value which can't be done without KALLSYMS. > > > > 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] > > > > original output without KALLSYMS: > > [12.487424] ps 0xffff800000eb008c > > [12.487598] pS 0xffff800000eb008c > > [12.487723] pSb 0xffff800000eb008c > > [12.487850] pB 0xffff800000eb008c > > [12.487967] pBb 0xffff800000eb008c > > > > With patched kernel without KALLSYMS: > > [9.205207] ps 0xffff800000eb008c [crash] > > [9.205564] pS 0xffff800000eb0000+0x8c [crash] > > [9.205757] pSb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > > [9.206066] pB 0xffff800000eb0000+0x8c [crash] > > [9.206257] pBb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] > > ... > > > +static int sprint_module_info(char *buf, char *end, unsigned long value, > > + const char *fmt) > > +{ > > + struct module *mod; > > + unsigned long offset = 1; > > + unsigned long base; > > > + int ret = 0; > > This is hard to find if it's not close to the first use. > Since you are using positive numbers... The name of the variable is misleading. It is not a return value. It is set when: if (mod) { ret = 1; and used: if (!ret) return 0; In fact, we do not need the value at all. It is enough to do: if (!mod) return 0; > > + const char *modname; > > + int modbuildid = 0; > > + int len; > > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > > + const unsigned char *buildid = NULL; > > +#endif > > + > > + if (is_ksym_addr(value)) > > + return 0; > > > + if (*fmt == 'B' && fmt[1] == 'b') > > + modbuildid = 1; > > + else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) > > Why not to split to two conditionals? Would be easier to get, This is copy&paste from symbol_string(). > > + modbuildid = 1; > > + else if (*fmt != 's') { > > These all are inconsistent, please switch to fmt[0]. > > > + /* > > + * do nothing. > > + */ > > + } else > > + offset = 0; > > + > > + preempt_disable(); > > + mod = __module_address(value); > > + if (mod) { > > + ret = 1; > > + modname = mod->name; > > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > > + if (modbuildid) > > + buildid = mod->build_id; > > +#endif > > + if (offset) { > > + base = (unsigned long)mod->core_layout.base; > > + offset = value - base; > > + } > > + } > > + > > + preempt_enable(); > > > + if (!ret) > > This looks a bit strange, but okay, I'm not familiar with the function of this > code. Yes, this can be replaced by /* We handle offset only against module base. */ if (!mod) return 0; Hmm, why don't we compute offset against vmlinux base when the symbol is from vmlinux? Wait, this would show base address of vmlinux. It would be security hole. Wait, if the base address of vmlinux is security hole then the base address of module is security hole as well. IMHO, we must hash the base address when the hashing is not disabled! > > + return 0; > > + > > + /* address belongs to module */ > > + if (offset) > > + len = sprintf(buf, "0x%lx+0x%lx", base, offset); > > + else > > + len = sprintf(buf, "0x%lx", value); > > + > > + len += sprintf(buf + len, " [%s", modname); > > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > > + if (modbuildid && buildid) { > > + /* build ID should match length of sprintf */ > > + static_assert(sizeof(typeof_member(struct module, build_id)) == 20); > > + len += sprintf(buf + len, " %20phN", buildid); > > + } > > +#endif > > + len += sprintf(buf + len, "]"); And all these sprint() calls are copy&pasted from __sprint_symbol(). We really should reduce the cut&pasting. > > + > > + return len; > > +} Best Regards, Petr
Hi Maninder,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on hnaz-mm/master]
[also build test WARNING on next-20220225]
[cannot apply to mcgrof/modules-next linus/master v5.17-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Maninder-Singh/kallsyms-enhance-pS-s-b-printing-when-KALLSYSMS-is-disabled/20220228-140105
base: https://github.com/hnaz/linux-mm master
config: alpha-randconfig-r032-20220227 (https://download.01.org/0day-ci/archive/20220228/202202281853.EofvQRmv-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/fbad94837350bb7c5b1b0c33648f8b20eff0150a
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Maninder-Singh/kallsyms-enhance-pS-s-b-printing-when-KALLSYSMS-is-disabled/20220228-140105
git checkout fbad94837350bb7c5b1b0c33648f8b20eff0150a
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
lib/vsprintf.c: In function 'sprint_module_info':
>> lib/vsprintf.c:993:13: warning: variable 'modbuildid' set but not used [-Wunused-but-set-variable]
993 | int modbuildid = 0;
| ^~~~~~~~~~
lib/vsprintf.c: In function 'va_format':
lib/vsprintf.c:1761:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
1761 | buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
| ^~~
vim +/modbuildid +993 lib/vsprintf.c
983
984 #if !defined(CONFIG_KALLSYMS) && defined(CONFIG_MODULES)
985 static int sprint_module_info(char *buf, char *end, unsigned long value,
986 const char *fmt)
987 {
988 struct module *mod;
989 unsigned long offset = 1;
990 unsigned long base;
991 int ret = 0;
992 const char *modname;
> 993 int modbuildid = 0;
994 int len;
995 #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
996 const unsigned char *buildid = NULL;
997 #endif
998
999 if (is_ksym_addr(value))
1000 return 0;
1001
1002 if (*fmt == 'B' && fmt[1] == 'b')
1003 modbuildid = 1;
1004 else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b')))
1005 modbuildid = 1;
1006 else if (*fmt != 's') {
1007 /*
1008 * do nothing.
1009 */
1010 } else
1011 offset = 0;
1012
1013 preempt_disable();
1014 mod = __module_address(value);
1015 if (mod) {
1016 ret = 1;
1017 modname = mod->name;
1018 #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
1019 if (modbuildid)
1020 buildid = mod->build_id;
1021 #endif
1022 if (offset) {
1023 base = (unsigned long)mod->core_layout.base;
1024 offset = value - base;
1025 }
1026 }
1027
1028 preempt_enable();
1029 if (!ret)
1030 return 0;
1031
1032 /* address belongs to module */
1033 if (offset)
1034 len = sprintf(buf, "0x%lx+0x%lx", base, offset);
1035 else
1036 len = sprintf(buf, "0x%lx", value);
1037
1038 len += sprintf(buf + len, " [%s", modname);
1039 #if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
1040 if (modbuildid && buildid) {
1041 /* build ID should match length of sprintf */
1042 static_assert(sizeof(typeof_member(struct module, build_id)) == 20);
1043 len += sprintf(buf + len, " %20phN", buildid);
1044 }
1045 #endif
1046 len += sprintf(buf + len, "]");
1047
1048 return len;
1049 }
1050 #else
1051 static inline int sprint_module_info(char *buf, char *end, unsigned long value,
1052 const char *fmt)
1053 {
1054 return 0;
1055 }
1056 #endif
1057
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h index ebfeb6099c28..5fb17dd4b6fa 100644 --- a/include/linux/kallsyms.h +++ b/include/linux/kallsyms.h @@ -163,33 +163,6 @@ static inline bool kallsyms_show_value(const struct cred *cred) return false; } -#ifdef CONFIG_MODULES -static inline int fill_minimal_module_info(char *sym, int size, unsigned long value) -{ - struct module *mod; - unsigned long offset; - int ret = 0; - - preempt_disable(); - mod = __module_address(value); - if (mod) { - offset = value - (unsigned long)mod->core_layout.base; - snprintf(sym, size - 1, "0x%lx+0x%lx [%s]", - (unsigned long)mod->core_layout.base, offset, mod->name); - - sym[size - 1] = '\0'; - ret = 1; - } - - preempt_enable(); - return ret; -} -#else -static inline int fill_minimal_module_info(char *sym, int size, unsigned long value) -{ - return 0; -} -#endif /*CONFIG_MODULES*/ #endif /*CONFIG_KALLSYMS*/ static inline void print_ip_sym(const char *loglvl, unsigned long ip) diff --git a/kernel/module.c b/kernel/module.c index 46a5c2ed1928..ccccb135c7fe 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -1465,12 +1465,10 @@ resolve_symbol_wait(struct module *mod, return ksym; } -#ifdef CONFIG_KALLSYMS static inline bool sect_empty(const Elf_Shdr *sect) { return !(sect->sh_flags & SHF_ALLOC) || sect->sh_size == 0; } -#endif /* * /sys/module/foo/sections stuff @@ -2799,7 +2797,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info) } #endif /* CONFIG_KALLSYMS */ -#if IS_ENABLED(CONFIG_KALLSYMS) && IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) static void init_build_id(struct module *mod, const struct load_info *info) { const Elf_Shdr *sechdr; diff --git a/lib/vsprintf.c b/lib/vsprintf.c index cb7345ff57f3..d4a24fd29494 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -980,6 +980,80 @@ 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, char *end, unsigned long value, + const char *fmt) +{ + struct module *mod; + unsigned long offset = 1; + unsigned long base; + int ret = 0; + const char *modname; + int modbuildid = 0; + int len; +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) + const unsigned char *buildid = NULL; +#endif + + if (is_ksym_addr(value)) + return 0; + + if (*fmt == 'B' && fmt[1] == 'b') + modbuildid = 1; + else if (*fmt == 'S' && (fmt[1] == 'b' || (fmt[1] == 'R' && fmt[2] == 'b'))) + modbuildid = 1; + else if (*fmt != 's') { + /* + * do nothing. + */ + } else + offset = 0; + + preempt_disable(); + mod = __module_address(value); + if (mod) { + ret = 1; + modname = mod->name; +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) + if (modbuildid) + buildid = mod->build_id; +#endif + if (offset) { + base = (unsigned long)mod->core_layout.base; + offset = value - base; + } + } + + preempt_enable(); + if (!ret) + return 0; + + /* address belongs to module */ + if (offset) + len = sprintf(buf, "0x%lx+0x%lx", base, offset); + else + len = sprintf(buf, "0x%lx", value); + + len += sprintf(buf + len, " [%s", modname); +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) + if (modbuildid && buildid) { + /* build ID should match length of sprintf */ + static_assert(sizeof(typeof_member(struct module, build_id)) == 20); + len += sprintf(buf + len, " %20phN", buildid); + } +#endif + len += sprintf(buf + len, "]"); + + return len; +} +#else +static inline int sprint_module_info(char *buf, char *end, unsigned long value, + const char *fmt) +{ + return 0; +} +#endif + static noinline_for_stack char *symbol_string(char *buf, char *end, void *ptr, struct printf_spec spec, const char *fmt) @@ -1005,7 +1079,7 @@ char *symbol_string(char *buf, char *end, void *ptr, return string_nocheck(buf, end, sym, spec); #else - if (fill_minimal_module_info(sym, KSYM_SYMBOL_LEN, value)) + if (sprint_module_info(sym, end, value, fmt)) return string_nocheck(buf, end, sym, spec); return special_hex_number(buf, end, value, sizeof(void *));
with commit '82b37e632513 ("kallsyms: print module name in %ps/S case when KALLSYMS is disabled"), module name printing was enhanced. As per suggestion from Petr Mladek <pmladek@suse.com>, covering other flavours also to print build id also. for %pB no change as it needs to know symbol name to adjust address value which can't be done without KALLSYMS. 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] original output without KALLSYMS: [12.487424] ps 0xffff800000eb008c [12.487598] pS 0xffff800000eb008c [12.487723] pSb 0xffff800000eb008c [12.487850] pB 0xffff800000eb008c [12.487967] pBb 0xffff800000eb008c With patched kernel without KALLSYMS: [9.205207] ps 0xffff800000eb008c [crash] [9.205564] pS 0xffff800000eb0000+0x8c [crash] [9.205757] pSb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] [9.206066] pB 0xffff800000eb0000+0x8c [crash] [9.206257] pBb 0xffff800000eb0000+0x8c [crash b367e79021b9f3b0172f9a36d4261c1f528ca1b3] 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> --- earlier discussion: https://lkml.org/lkml/2022/2/10/185 include/linux/kallsyms.h | 27 -------------- kernel/module.c | 4 +-- lib/vsprintf.c | 76 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 76 insertions(+), 31 deletions(-)