[PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Maninder Singh]

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 <v.narang@samsung.com>
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 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

Re: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Andy Shevchenko]

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 <v.narang@samsung.com>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>

Who is (are) the author(s)?

-- 
With Best Regards,
Andy Shevchenko

RE:(2) [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Maninder Singh]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

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 <v.narang@samsung.com>
>> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
 
> Who is (are) the author(s)?

Both are authors (Maninder and Vaneet).

Sorry, forgot to use below signature.

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>

Thanks,
Maninder Singh

[-- Attachment #2: rcptInfo.txt --]
[-- Type: application/octet-stream, Size: 1630 bytes --]


   =================================================================================================================================
      Subject    : Re: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled
      From       : null
      Sent Date  : 2022-02-01 15:16  GMT+5:30
   =================================================================================================================================
                  Name                Type          Job Title                       Dept.                               Company                
   =================================================================================================================================
      Maninder Singh                 TO         Staff Engineer             System S/W Group /SRI-Delhi               Samsung Electronics 
      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
      mbenes@suse.cz                 CC
      swboyd@chromium.org            CC
      ojeda@kernel.org               CC
      linux-kernel@vger.kernel.org   CC
      will@kernel.org                CC
      catalin.marinas@arm.com        CC
      Vaneet Narang                  CC         Associate Architect        System S/W Group /SRI-Delhi               Samsung Electronics
   =================================================================================================================================

Re: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Petr Mladek]

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/20220130213214.1042497-1-atomlin@redhat.com

Adding Aaron and Luis into Cc.

Best Regards,
Petr

Re: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Luis Chamberlain]

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/YgKyC4ZRud0JW1PF@bombadil.infradead.org

  Luis

Re: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Petr Mladek]

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/YgKyC4ZRud0JW1PF@bombadil.infradead.org

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/YdIfz+LMewetSaEB@gmail.com/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

RE: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Maninder Singh]

[-- Attachment #1: Type: text/plain, Size: 2372 bytes --]

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/YgKyC4ZRud0JW1PF@bombadil.infradead.org
>  
> 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

[-- Attachment #2: rcptInfo.txt --]
[-- Type: application/octet-stream, Size: 1753 bytes --]


   =================================================================================================================================
      Subject    : Re: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled
      From       : null
      Sent Date  : 2022-02-10 13:50  GMT+5:30
   =================================================================================================================================
                  Name                Type          Job Title                       Dept.                               Company                
   =================================================================================================================================
      mcgrof@kernel.org              TO
      avimalin@gmail.com             CC
      Maninder Singh                 CC         Staff Engineer             System S/W Group /SRI-Delhi               Samsung Electronics 
      rostedt@goodmis.org            CC
      senozhatsky@chromium.org       CC
      andriy.shevchenko@linux.i...   CC
      linux@rasmusvillemoes.dk       CC
      akpm@linux-foundation.org      CC
      wangkefeng.wang@huawei.com     CC
      mbenes@suse.cz                 CC
      swboyd@chromium.org            CC
      ojeda@kernel.org               CC
      linux-kernel@vger.kernel.org   CC
      will@kernel.org                CC
      catalin.marinas@arm.com        CC
      Vaneet Narang                  CC         Associate Architect        System S/W Group /SRI-Delhi               Samsung Electronics
      atomlin@redhat.com             CC
   =================================================================================================================================

Re: [PATCH 1/1] kallsyms: print module name in %ps/S case when KALLSYMS is disabled

[Luis Chamberlain]

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/YgKyC4ZRud0JW1PF@bombadil.infradead.org
> >  
> > 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