Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages

From: Helge Deller <deller@gmx.de>
To: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>,
	"Luck, Tony" <tony.luck@intel.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>,
	Petr Mladek <pmladek@suse.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	"Yu, Fenghua" <fenghua.yu@intel.com>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	Paul Mackerras <paulus@samba.org>,
	Michael Ellerman <mpe@ellerman.id.au>
Subject: Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
Date: Fri, 8 Sep 2017 22:49:51 +0200	[thread overview]
Message-ID: <6fdd62aa-e9e7-8954-da6b-6fa5e73983c5@gmx.de> (raw)
In-Reply-To: <20170908061830.GA496@jagdpanzerIV.localdomain>

On 08.09.2017 08:18, Sergey Senozhatsky wrote:
> On (09/07/17 16:05), Luck, Tony wrote:
> [..]
>>>> 	if (not_a_function_descriptor(ptr))
>>>> 		return ptr;
>>>
>>> I'm not sure if it's possible on ia64/ppc64/parisc64
>>> to reliably detect if it's a function descriptor or not.
>>
>> Agreed. I don't know how to write this test (without changing the compiler to
>> put the pointers in a separate section ... and then changing the module loader
>> to keep a list of all these sections).
> 
> let me try one more time :)
> 
> so below is a number of assumptions, let me know if anything is wrong
> there.... and let's try to fix the "wrong bits" ;)
> 
> 
> RFC
> 
> 
> 1) function descriptor table is in .data, not in .text
>    correct?
> 
> 2) symbol resolution consists of 3 steps:
> 
>    a) we check if this is a kernel symbol and resolve it if so
>    b) we check if the addr belongs to any module and resolve the addr
>       if so
>    c) we check if the addr is bpf and resolve it if so. let's skip this part.
> 
> 
>    so, for (a) we probably can do something like below. can't we?
>    // not tested, as usual.
> 
> 
> ---
> 
> diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
> index 127e7cfafa55..4807e204428e 100644
> --- a/kernel/kallsyms.c
> +++ b/kernel/kallsyms.c
> @@ -319,6 +319,16 @@ const char *kallsyms_lookup(unsigned long addr,
>         namebuf[KSYM_NAME_LEN - 1] = 0;
>         namebuf[0] = 0;
>  
> +#if defined(CONFIG_IA64) || defined(CONFIG_PPC64) || defined(CONFIG_PARISC)
> +       if (!is_ksym_addr(addr)) {
> +               unsigned long deref_addr;
> +
> +               deref_addr = dereference_function_descriptor(addr);
> +               if (is_ksym_addr(deref_addr))
> +                       addr = deref_addr;
> +       }
> +#endif
> +
>         if (is_ksym_addr(addr)) {
>                 unsigned long pos;
>  
> 
> ----
> 
> if the addr is not in kernel .text, then try dereferencing it and check
> if the dereferenced addr is in kernel .text.
> 
> 
> 
>    now, for (b) we can do something like below... probably.
> 
>    if the addr is not module .text (not .data), then check if dereferenced
>    address is module .text (not .data).
> 
> 
> ---
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index de66ec825992..f81c67b745ff 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3865,6 +3865,16 @@ static inline int within(unsigned long addr, void *start, unsigned long size)
>         return ((void *)addr >= start && (void *)addr < start + size);
>  }
>  
> +static inline bool __mod_text_address(struct module *mod,
> +                                     unsigned long addr)
> +{
> +       /* Make sure it's within the text section. */
> +       if (!within(addr, mod->init_layout.base, mod->init_layout.text_size)
> +           && !within(addr, mod->core_layout.base, mod->core_layout.text_size))
> +               return false;
> +       return true;
> +}
> +
>  #ifdef CONFIG_KALLSYMS
>  /*
>   * This ignores the intensely annoying "mapping symbols" found
> @@ -3942,6 +3952,14 @@ const char *module_address_lookup(unsigned long addr,
>         preempt_disable();
>         mod = __module_address(addr);
>         if (mod) {
> +#if defined(CONFIG_IA64) || defined(CONFIG_PPC64) || defined(CONFIG_PARISC)
> +               unsigned long deref_addr;
> +
> +               if (!__mod_text_address(mod, addr))
> +                       deref_addr = dereference_function_descriptor(addr);
> +               if (__mod_text_address(mod, deref_addr))
> +                       addr = deref_addr;
> +#endif
>                 if (modname)
>                         *modname = mod->name;
>                 ret = get_ksymbol(mod, addr, size, offset);
> 
> ---
> 
> so there are probably some broken parts there. like...
> I don't know. something.
> 
> so - what is broken, and how can we fix/tweak it? help me out.

Sergey, I'm sure there is a way how you can get it somehow to work the way
you describe above, but even then nobody can guarantee you that it
will work in 100% of the cases.

It's somehow like "we have %lu and %c specifiers, and it's basically 
the same, so let's try to figure out at runtime which one should be
used based on analysis of what was given as argument".
It may work somehow, but not always.

What about the idea of a %luS specifier (or something other) ?

Helge