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@xxxxxxxxxx

Adding Aaron and Luis into Cc.

Best Regards,
Petr