* [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled
[not found] <CGME20220315095129epcas5p230b94ea10517d148c9cae0669229b0fc@epcas5p2.samsung.com>
@ 2022-03-15 9:51 ` Maninder Singh
2022-03-15 10:30 ` Andy Shevchenko
[not found] ` <CGME20220315095129epcas5p230b94ea10517d148c9cae0669229b0fc@epcms5p5>
0 siblings, 2 replies; 3+ messages in thread
From: Maninder Singh @ 2022-03-15 9:51 UTC (permalink / raw)
To: mcgrof, pmladek, rostedt, senozhatsky, andriy.shevchenko, linux,
akpm, wangkefeng.wang
Cc: v.narang, swboyd, ojeda, linux-kernel, linux-modules, avimalin,
atomlin, Maninder Singh
print module information when KALLSYMS is disabled.
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 without KALLSYMS:
[7.870286] ps 0xffff800000f2008c [crash] // similar to original
[7.870716] pS 0x(____ptrval____)+0x8c [crash] // base address hashed and offset is without hash
[7.871025] pSb 0x(____ptrval____)+0x8c [crash 3423a8993a7033fb79e5add14bf9d8d6b56330ca]
[7.871321] pB 0x(____ptrval____)+0x8c [crash]
[7.871512] pBb 0x(____ptrval____)+0x8c [crash 3423a8993a7033fb79e5add14bf9d8d6b56330ca]
[7.872191] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
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>
---
v1->v2: hash base address of module, change *fmt to fmt[0] and removed
copy paste.
include/linux/kallsyms.h | 2 +
include/linux/module.h | 22 +++++++++++
kernel/kallsyms.c | 26 ++++++-------
kernel/module.c | 4 +-
lib/vsprintf.c | 84 +++++++++++++++++++++++++++++++++-------
5 files changed, 109 insertions(+), 29 deletions(-)
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index e5ad6e31697d..c24fa627ab6e 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 1e135fd5c076..26a49e663351 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -678,6 +678,22 @@ static inline bool is_livepatch_module(struct module *mod)
bool is_module_sig_enforced(void);
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
+ len += sprintf(buffer + len, "]");
+
+ return len;
+}
#else /* !CONFIG_MODULES... */
static inline struct module *__module_address(unsigned long addr)
@@ -818,6 +834,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 57213e1d2349..cdbcaf86252a 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -479,19 +479,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;
}
@@ -586,6 +575,17 @@ 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);
+ else if (symbol)
+ return __sprint_symbol(buffer, address, 0, 1, build_id);
+ else
+ 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.c b/kernel/module.c
index 6cea788fd965..5756d31a024b 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 7adb8fd4d804..064b0cc45a37 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1000,33 +1000,91 @@ 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, int modbuildid, int backtrace, int symbol)
+{
+ struct module *mod;
+ unsigned long offset = 0;
+ unsigned long base;
+ char *modname;
+ int len;
+ const unsigned char *buildid = NULL;
+
+ if (is_ksym_addr(value))
+ return 0;
+
+ if (backtrace || symbol)
+ offset = 1;
+
+ 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 (offset) {
+ base = (unsigned long)mod->core_layout.base;
+ offset = value - base;
+ }
+ }
+
+ preempt_enable();
+ if (!mod)
+ return 0;
+
+ /* address belongs to module */
+ if (offset)
+ len = sprintf(buf, "0x%p+0x%lx", (void *)base, offset);
+ else
+ len = sprintf(buf, "0x%lx", (void *)value);
+
+ len += fill_name_build_id(buf, modname, modbuildid, buildid, 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)
{
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, end, value, fmt, build_id, backtrace, symbol))
+ return string_nocheck(buf, end, sym, spec);
+
return special_hex_number(buf, end, value, sizeof(void *));
#endif
}
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled
2022-03-15 9:51 ` [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled Maninder Singh
@ 2022-03-15 10:30 ` Andy Shevchenko
[not found] ` <CGME20220315095129epcas5p230b94ea10517d148c9cae0669229b0fc@epcms5p5>
1 sibling, 0 replies; 3+ messages in thread
From: Andy Shevchenko @ 2022-03-15 10:30 UTC (permalink / raw)
To: Maninder Singh
Cc: mcgrof, pmladek, rostedt, senozhatsky, linux, akpm,
wangkefeng.wang, v.narang, swboyd, ojeda, linux-kernel,
linux-modules, avimalin, atomlin
On Tue, Mar 15, 2022 at 03:21:12PM +0530, Maninder Singh wrote:
> print module information when KALLSYMS is disabled.
>
> No change for %pB, as it needs to know symbol name to adjust address
> value which can't be done without KALLSYMS.
...
> +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);
> + else if (symbol)
> + return __sprint_symbol(buffer, address, 0, 1, build_id);
> + else
> + return __sprint_symbol(buffer, address, 0, 0, 0);
Redundant 'else' in both cases.
> +}
...
> +static int sprint_module_info(char *buf, char *end, unsigned long value,
> + const char *fmt, int modbuildid, int backtrace, int symbol)
fmt is not used.
> +{
> + struct module *mod;
> + unsigned long offset = 0;
> + unsigned long base;
Can it be the same type as core_layout.base? Why not?
> + char *modname;
> + int len;
> + const unsigned char *buildid = NULL;
> +
> + if (is_ksym_addr(value))
> + return 0;
> +
> + if (backtrace || symbol)
> + offset = 1;
I would expect here to have
else
offset = 0;
But see below.
> + 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 (offset) {
This seems quite confusing because semantically you use offset as a boolean
flag and offset. Why not add a boolean variable with a clear name?
> + base = (unsigned long)mod->core_layout.base;
> + offset = value - base;
> + }
> + }
> +
Probably you can drop this blank line to group entire critical section,
or add a blank line above.
> + preempt_enable();
> + if (!mod)
> + return 0;
> +
> + /* address belongs to module */
> + if (offset)
> + len = sprintf(buf, "0x%p+0x%lx", (void *)base, offset);
> + else
> + len = sprintf(buf, "0x%lx", (void *)value);
What this casting is for? Don't you have a compilation warning?
> + len += fill_name_build_id(buf, modname, modbuildid, buildid, len);
> + return len;
return len + ...;
?
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled
[not found] ` <CGME20220315095129epcas5p230b94ea10517d148c9cae0669229b0fc@epcms5p5>
@ 2022-03-15 10:47 ` Maninder Singh
0 siblings, 0 replies; 3+ messages in thread
From: Maninder Singh @ 2022-03-15 10:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: mcgrof, pmladek, rostedt, senozhatsky, linux, akpm,
wangkefeng.wang, Vaneet Narang, swboyd, ojeda, linux-kernel,
linux-modules, avimalin, atomlin
[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]
Hi,
> > +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);
>
> > + else if (symbol)
> > + return __sprint_symbol(buffer, address, 0, 1, build_id);
> > + else
> > + return __sprint_symbol(buffer, address, 0, 0, 0);
>
> Redundant 'else' in both cases.
>
Ok, will change it
> > +}
>
> ...
>
> > +static int sprint_module_info(char *buf, char *end, unsigned long value,
> > + const char *fmt, int modbuildid, int backtrace, int symbol)
>
> fmt is not used.
Yes, did not notice it.(will remove both end and gmt)
> > +{
> > + struct module *mod;
> > + unsigned long offset = 0;
>
> > + unsigned long base;
>
> Can it be the same type as core_layout.base? Why not?
>
> > + char *modname;
> > + int len;
> > + const unsigned char *buildid = NULL;
> > +
> > + if (is_ksym_addr(value))
> > + return 0;
> > +
> > + if (backtrace || symbol)
> > + offset = 1;
>
> I would expect here to have
>
> else
> offset = 0;
>
> But see below.
>
> > + 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 (offset) {
>
> This seems quite confusing because semantically you use offset as a boolean
> flag and offset. Why not add a boolean variable with a clear name?
>
Ok, will add 2 separate variables.
> > + base = (unsigned long)mod->core_layout.base;
> > + offset = value - base;
> > + }
> > + }
>
> > +
>
> Probably you can drop this blank line to group entire critical section,
> or add a blank line above.
>
> > + preempt_enable();
> > + if (!mod)
> > + return 0;
> > +
> > + /* address belongs to module */
> > + if (offset)
> > + len = sprintf(buf, "0x%p+0x%lx", (void *)base, offset);
> > + else
>
> > + len = sprintf(buf, "0x%lx", (void *)value);
>
> What this casting is for? Don't you have a compilation warning?
My Bad, earlier I made patch with hashing this value also (%p), but after that
changed it to %lx to have same original behavior in case of %ps, forgot to update final patch
to remove typecast.
>
> > + len += fill_name_build_id(buf, modname, modbuildid, buildid, len);
> > + return len;
>
> return len + ...;
>
> ?
>
> > +}
Will modify patch with all changes.
Thanks,
Maninder Singh
[-- Attachment #2: rcptInfo.txt --]
[-- Type: application/octet-stream, Size: 1664 bytes --]
=================================================================================================================================
Subject : Re: [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled
From : null
Sent Date : 2022-03-15 16:02 GMT+5:30
=================================================================================================================================
Name Type Job Title Dept. Company
=================================================================================================================================
Maninder Singh TO Staff Engineer System S/W Group /SRI-Delhi Samsung Electronics
mcgrof@kernel.org CC
pmladek@suse.com CC
rostedt@goodmis.org CC
senozhatsky@chromium.org CC
linux@rasmusvillemoes.dk CC
akpm@linux-foundation.org CC
wangkefeng.wang@huawei.com CC
Vaneet Narang CC Associate Architect System S/W Group /SRI-Delhi Samsung Electronics
swboyd@chromium.org CC
ojeda@kernel.org CC
linux-kernel@vger.kernel.org CC
linux-modules@vger.kernel... CC
avimalin@gmail.com CC
atomlin@redhat.com CC
=================================================================================================================================
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-15 10:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20220315095129epcas5p230b94ea10517d148c9cae0669229b0fc@epcas5p2.samsung.com>
2022-03-15 9:51 ` [PATCH v2] kallsyms: enhance %pS/s/b printing when KALLSYSMS is disabled Maninder Singh
2022-03-15 10:30 ` Andy Shevchenko
[not found] ` <CGME20220315095129epcas5p230b94ea10517d148c9cae0669229b0fc@epcms5p5>
2022-03-15 10:47 ` Maninder Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).