original:
With KALLSYMS
%pS %ps
[16.4200] hello_init+0x0/0x24 [crash] hello_init [crash]
Without KALLSYMS:
[16.2200] 0xbe200040 0xbe200040
With Patch (Without KALLSYMS:) load address + current offset [Module Name]
[13.5993] 0xbe200000+0x40 [crash] 0xbe200000+0x40 [crash]
It will help in better debugging and checking when KALLSYMS is disabled,
user will get information about module name and load address of module.
verified for arm64:
/ # insmod /crash.ko
[ 19.263556] 0xffff800000ec0000+0x38 [crash]
..
[ 19.276023] Call trace:
[ 19.276277] 0xffff800000ec0000+0x28 [crash]
[ 19.276567] 0xffff800000ec0000+0x58 [crash]
[ 19.276727] 0xffff800000ec0000+0x74 [crash]
[ 19.276866] 0xffff8000080127d0
[ 19.276978] 0xffff80000812d95c
[ 19.277085] 0xffff80000812f554
Signed-off-by: Vaneet Narang <[email protected]>
Signed-off-by: Maninder Singh <[email protected]>
---
include/linux/kallsyms.h | 27 +++++++++++++++++++++++++++
lib/vsprintf.c | 5 +++--
2 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 5fb17dd4b6fa..ebfeb6099c28 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -163,6 +163,33 @@ 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/lib/vsprintf.c b/lib/vsprintf.c
index 61528094ec87..41c74abb1726 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -985,9 +985,7 @@ 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
if (fmt[1] == 'R')
ptr = __builtin_extract_return_addr(ptr);
@@ -1007,6 +1005,9 @@ 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))
+ return string_nocheck(buf, end, sym, spec);
+
return special_hex_number(buf, end, value, sizeof(void *));
#endif
}
--
2.17.1
On Tue, Feb 01, 2022 at 09:30:44AM +0530, Maninder Singh wrote:
> original:
> With KALLSYMS
> %pS %ps
> [16.4200] hello_init+0x0/0x24 [crash] hello_init [crash]
>
> Without KALLSYMS:
> [16.2200] 0xbe200040 0xbe200040
>
> With Patch (Without KALLSYMS:) load address + current offset [Module Name]
>
> [13.5993] 0xbe200000+0x40 [crash] 0xbe200000+0x40 [crash]
>
> It will help in better debugging and checking when KALLSYMS is disabled,
> user will get information about module name and load address of module.
>
> verified for arm64:
> / # insmod /crash.ko
>
> [ 19.263556] 0xffff800000ec0000+0x38 [crash]
>
> ..
>
> [ 19.276023] Call trace:
> [ 19.276277] 0xffff800000ec0000+0x28 [crash]
> [ 19.276567] 0xffff800000ec0000+0x58 [crash]
> [ 19.276727] 0xffff800000ec0000+0x74 [crash]
> [ 19.276866] 0xffff8000080127d0
> [ 19.276978] 0xffff80000812d95c
> [ 19.277085] 0xffff80000812f554
> Signed-off-by: Vaneet Narang <[email protected]>
> Signed-off-by: Maninder Singh <[email protected]>
Who is (are) the author(s)?
--
With Best Regards,
Andy Shevchenko
Hi,
>> ..
>>
>> [ 19.276023] Call trace:
>> [ 19.276277] 0xffff800000ec0000+0x28 [crash]
>> [ 19.276567] 0xffff800000ec0000+0x58 [crash]
>> [ 19.276727] 0xffff800000ec0000+0x74 [crash]
>> [ 19.276866] 0xffff8000080127d0
>> [ 19.276978] 0xffff80000812d95c
>> [ 19.277085] 0xffff80000812f554
>> Signed-off-by: Vaneet Narang <[email protected]>
>> Signed-off-by: Maninder Singh <[email protected]>
> Who is (are) the author(s)?
Both are authors (Maninder and Vaneet).
Sorry, forgot to use below signature.
Co-developed-by: Vaneet Narang <[email protected]>
Signed-off-by: Vaneet Narang <[email protected]>
Signed-off-by: Maninder Singh <[email protected]>
Thanks,
Maninder Singh
On Tue 2022-02-01 09:30:44, Maninder Singh wrote:
> original:
> With KALLSYMS
> %pS %ps
> [16.4200] hello_init+0x0/0x24 [crash] hello_init [crash]
>
> Without KALLSYMS:
> [16.2200] 0xbe200040 0xbe200040
>
> With Patch (Without KALLSYMS:) load address + current offset [Module Name]
>
> [13.5993] 0xbe200000+0x40 [crash] 0xbe200000+0x40 [crash]
>
> It will help in better debugging and checking when KALLSYMS is disabled,
> user will get information about module name and load address of module.
>
> verified for arm64:
> / # insmod /crash.ko
>
> [ 19.263556] 0xffff800000ec0000+0x38 [crash]
>
> ..
>
> [ 19.276023] Call trace:
> [ 19.276277] 0xffff800000ec0000+0x28 [crash]
> [ 19.276567] 0xffff800000ec0000+0x58 [crash]
> [ 19.276727] 0xffff800000ec0000+0x74 [crash]
> [ 19.276866] 0xffff8000080127d0
> [ 19.276978] 0xffff80000812d95c
> [ 19.277085] 0xffff80000812f554
The idea is great. But the patch will need some changes, see below.
> --- a/include/linux/kallsyms.h
> +++ b/include/linux/kallsyms.h
> @@ -163,6 +163,33 @@ 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;
> +}
It looks too big for an inlined function. Anyway, we will need
something even more complex, see below.
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 61528094ec87..41c74abb1726 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1007,6 +1005,9 @@ 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))
> + return string_nocheck(buf, end, sym, spec);
The behavior should be different for different modifiers. Namely:
+ the offset is not printed for %ps // lower-case 's'
+ the address must be searched with offset -1 for %pB // on stack
+ build ID should be appended when 'b' modifier is appeanded
IMHO, we should implement a generic __sprint_symbol() that will
find the information using kallsyms_lookup_buildid() when available
and fallback to the mininalized approach when kallsyms are not available.
It might require moving the code out of kallsyms.c. It should be
co-ordinated with the other patchset that is moving these sources
into kernel/module/*, see
https://lore.kernel.org/r/[email protected]
Adding Aaron and Luis into Cc.
Best Regards,
Petr
On Wed, Feb 09, 2022 at 12:40:38PM +0100, Petr Mladek wrote:
> > --- a/include/linux/kallsyms.h
> > +++ b/include/linux/kallsyms.h
> > @@ -163,6 +163,33 @@ 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;
> > +}
>
> It looks too big for an inlined function. Anyway, we will need
> something even more complex, see below.
Interesting, these observations might apply to Vimal's work as well [0].
[0] https://lkml.kernel.org/r/[email protected]
Luis
On Wed 2022-02-09 15:02:06, Luis Chamberlain wrote:
> On Wed, Feb 09, 2022 at 12:40:38PM +0100, Petr Mladek wrote:
> > > --- a/include/linux/kallsyms.h
> > > +++ b/include/linux/kallsyms.h
> > > @@ -163,6 +163,33 @@ 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;
> > > +}
> >
> > It looks too big for an inlined function. Anyway, we will need
> > something even more complex, see below.
>
> Interesting, these observations might apply to Vimal's work as well [0].
>
> [0] https://lkml.kernel.org/r/[email protected]
Honestly, I am not sure what is the best practice. My understanding is
that inlined functions are used primary for speed up at runtime.
Anyway, there is the huge patchset that tries to optimize kernel build
time by optimizing headers, see
https://lore.kernel.org/lkml/[email protected]/T/
This is from the cover letter:
<paste>
Techniques used by the fast-headers tree to reduce header size & dependencies:
[...]
- Uninlining: there's a number of unnecessary inline functions that also
couple otherwise unrelated headers to each other. The fast-headers tree
contains over 100 uninlining commits.
</paste>
It is probably less important for some local includes. I am not sure how
widely is kallsyms.h included.
Best Regards,
Petr
Hi All,
Thanks for your inputs.
> On Wed 2022-02-09 15:02:06, Luis Chamberlain wrote:
> > On Wed, Feb 09, 2022 at 12:40:38PM +0100, Petr Mladek wrote:
> > > > --- a/include/linux/kallsyms.h
> > > > +++ b/include/linux/kallsyms.h
> > > > @@ -163,6 +163,33 @@ 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;
> > > > +}
> > >
> > > It looks too big for an inlined function. Anyway, we will need
> > > something even more complex, see below.
> >
> > Interesting, these observations might apply to Vimal's work as well [0].
> >
> > [0] https://lkml.kernel.org/r/[email protected]
>
> Honestly, I am not sure what is the best practice. My understanding is
> that inlined functions are used primary for speed up at runtime.
>
Main reason of making it inline was:
(1) kallsysm.c was not getting compiled(with disabled config), so could not add defination there.
(2) lib/vsnprintf.c was not correct place to define new function of kallsyms(fill_minimal_module_info)
(3) I thought static int will be part of each .c file which includes kallsyms.h and compiler can make noise for unused functions,
and also increase code size, where as static inline will be added only if some code is calling that function otherwise will be discarded.
But as peter said better version will be to make a new defination of __sprint_symbol (probably in kernel/module.c)
to handle all cases of %ps/S/B/b when KALLSYSMS is disabled.
I will try to prepare changes and share V2 patch.
Thanks,
Maninder Singh
On Thu, Feb 10, 2022 at 02:18:23PM +0530, Maninder Singh wrote:
> Hi All,
>
> Thanks for your inputs.
>
> > On Wed 2022-02-09 15:02:06, Luis Chamberlain wrote:
> > > On Wed, Feb 09, 2022 at 12:40:38PM +0100, Petr Mladek wrote:
> > > > > --- a/include/linux/kallsyms.h
> > > > > +++ b/include/linux/kallsyms.h
> > > > > @@ -163,6 +163,33 @@ 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;
> > > > > +}
> > > >
> > > > It looks too big for an inlined function. Anyway, we will need
> > > > something even more complex, see below.
> > >
> > > Interesting, these observations might apply to Vimal's work as well [0].
> > >
> > > [0] https://lkml.kernel.org/r/[email protected]
> >
> > Honestly, I am not sure what is the best practice. My understanding is
> > that inlined functions are used primary for speed up at runtime.
> >
>
> Main reason of making it inline was:
> (1) kallsysm.c was not getting compiled(with disabled config), so could not add defination there.
> (2) lib/vsnprintf.c was not correct place to define new function of kallsyms(fill_minimal_module_info)
> (3) I thought static int will be part of each .c file which includes kallsyms.h and compiler can make noise for unused functions,
> and also increase code size, where as static inline will be added only if some code is calling that function otherwise will be discarded.
>
> But as peter said better version will be to make a new defination of __sprint_symbol (probably in kernel/module.c)
> to handle all cases of %ps/S/B/b when KALLSYSMS is disabled.
>
> I will try to prepare changes and share V2 patch.
If you do please Cc me and Vimal, and Aaron.
Luis