| Message ID | 20170930025319.987-7-sergey.senozhatsky@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream, archived |
| Headers | show |
On Sat 2017-09-30 11:53:18, Sergey Senozhatsky wrote: > Call appropriate function descriptor dereference ARCH callbacks: > - dereference_kernel_function_descriptor() if the pointer is a > kernel symbol; > > - dereference_module_function_descriptor() if the pointer is a > module symbol. > > This patch also removes dereference_function_descriptor() from > '%pF/%pf' vsprintf handler, because it has the same behavior with > '%pS/%ps' now. The description is pretty criptic. It should explain why the dereference was moved from vsprintf to the symbol lookup and if it is safe. Note that kallsyms_lookup() and module_address_lookup() is used in many other situations. Also I would not be afraid to repeat description of the big picture from the 2nd patch. > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > Tested-by: Helge Deller <deller@gmx.de> # parisc64 > Tested-by: Santosh Sivaraj <santosh@fossix.org> # powerpc64 > Acked-by: Michael Ellerman <mpe@ellerman.id.au> # powerpc64 > Tested-by: Tony Luck <tony.luck@intel.com> # ia64 > --- > Documentation/printk-formats.txt | 20 ++++++++++---------- > kernel/kallsyms.c | 1 + > kernel/module.c | 1 + > lib/vsprintf.c | 5 +---- > 4 files changed, 13 insertions(+), 14 deletions(-) > > diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt > index 361789df51ec..3adbc4fdd482 100644 > --- a/Documentation/printk-formats.txt > +++ b/Documentation/printk-formats.txt > @@ -50,26 +50,28 @@ Symbols/Function Pointers > > :: > > + %pS versatile_init+0x0/0x110 > + %ps versatile_init > %pF versatile_init+0x0/0x110 > %pf versatile_init > - %pS versatile_init+0x0/0x110 > %pSR versatile_init+0x9/0x110 > (with __builtin_extract_return_addr() translation) > - %ps versatile_init > %pB prev_fn_of_versatile_init+0x88/0x88 > > -The ``F`` and ``f`` specifiers are for printing function pointers, > -for example, f->func, &gettimeofday. They have the same result as > -``S`` and ``s`` specifiers. But they do an extra conversion on > -ia64, ppc64 and parisc64 architectures where the function pointers > -are actually function descriptors. > - > The ``S`` and ``s`` specifiers can be used for printing symbols > from direct addresses, for example, __builtin_return_address(0), > (void *)regs->ip. They result in the symbol name with (``S``) or > without (``s``) offsets. If KALLSYMS are disabled then the symbol > address is printed instead. This paragraph makes the feeling that ``S`` is still only for direct adresses. We should update it as well. > +Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``) > +and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and > +parisc64 function pointers are indirect and, in fact, are function > +descriptors, which require additional dereferencing before we can lookup > +the symbol. As of now, ``S`` and ``s`` perform dereferencing on those > +platforms (when needed), so ``F`` and ``f`` exist for compatibility > +reasons only. > + > The ``B`` specifier results in the symbol name with offsets and should be > used when printing stack backtraces. The specifier takes into > consideration the effect of compiler optimisations which may occur > @@ -77,8 +79,6 @@ when tail-call``s are used and marked with the noreturn GCC attribute. > > Examples:: > > - printk("Going to call: %pF\n", gettimeofday); > - printk("Going to call: %pF\n", p->func); > printk("%s: called from %pS\n", __func__, (void *)_RET_IP_); > printk("%s: called from %pS\n", __func__, > (void *)__builtin_return_address(0)); We should either replace %pF with %pS or remove all examples. It is strange to keep only half of them. > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > index 127e7cfafa55..e2fc09ea9509 100644 > --- a/kernel/kallsyms.c > +++ b/kernel/kallsyms.c > @@ -322,6 +322,7 @@ const char *kallsyms_lookup(unsigned long addr, > if (is_ksym_addr(addr)) { is_ksym_addr() ignores the special .opd elf sections if CONFIG_KALLSYMS_ALL is disabled. We should dereference before this call. > unsigned long pos; > > + addr = dereference_kernel_function_descriptor(addr); > pos = get_symbol_pos(addr, symbolsize, offset); I still wonder if doing the dereference in the widely used kallsyms might cause any regression. One possible problem is that this function returns "offset". One might expect that it is offset against "addr" but it is not if the dereference happens here. Also get_symbol_pos() is called in several other helpers but the dereference is done only here. It would be confusing if for example kallsyms_lookup_size_offset() and kallsyms_lookup() give different result. I would feel much more comfortable if we keep the derefenrece only in vsprintf. In each case, we need approval from Jessica for the change in module.c. Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sorry for the delay and thanks for taking a look. I'll try to re-spin the patch set by the end of this week/early next week. On (10/04/17 13:53), Petr Mladek wrote: [..] > Note that kallsyms_lookup() and module_address_lookup() is used > in many other situations. we dereference only things that can be dereferenced. so calling it on already dereferenced address, or address that does need to be dereferenced is OK. besides, not all of those "other" places are available on ppc64, ia64, parisc. [..] > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > index 127e7cfafa55..e2fc09ea9509 100644 > > --- a/kernel/kallsyms.c > > +++ b/kernel/kallsyms.c > > @@ -322,6 +322,7 @@ const char *kallsyms_lookup(unsigned long addr, > > if (is_ksym_addr(addr)) { > > is_ksym_addr() ignores the special .opd elf sections if > CONFIG_KALLSYMS_ALL is disabled. We should dereference before > this call. I'll move it. > > unsigned long pos; > > > > + addr = dereference_kernel_function_descriptor(addr); > > pos = get_symbol_pos(addr, symbolsize, offset); > > I still wonder if doing the dereference in the widely used kallsyms > might cause any regression. more testing wouldn't hurt, yes. > Also get_symbol_pos() is called in several other helpers > but the dereference is done only here. It would be > confusing if for example kallsyms_lookup_size_offset() > and kallsyms_lookup() give different result. hm, so there is no change in this regard, right? there was no deference before, there is no dereference now. what am I missing? I'm touching the pf/pF part in this patch set. if there are cases of missing dereferences anywhere else then we need to address it in a separate patch set, I think. > I would feel much more comfortable if we keep the derefenrece > only in vsprintf. at a price of extra module lookup, because we need `struct module *' for module address dereference. -ss -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 2017-10-19 15:42:35, Sergey Senozhatsky wrote: > Sorry for the delay and thanks for taking a look. > > I'll try to re-spin the patch set by the end of this week/early next > week. > > > On (10/04/17 13:53), Petr Mladek wrote: > [..] > > Note that kallsyms_lookup() and module_address_lookup() is used > > in many other situations. > > we dereference only things that can be dereferenced. > so calling it on already dereferenced address, or address > that does need to be dereferenced is OK. My concern is that it changes the behavior. It will suddenly return another information for addresses that were not dereference before. > [..] > > > diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c > > > index 127e7cfafa55..e2fc09ea9509 100644 > > > --- a/kernel/kallsyms.c > > > +++ b/kernel/kallsyms.c > > > @@ -322,6 +322,7 @@ const char *kallsyms_lookup(unsigned long addr, > > > if (is_ksym_addr(addr)) { > > > > is_ksym_addr() ignores the special .opd elf sections if > > CONFIG_KALLSYMS_ALL is disabled. We should dereference before > > this call. > > I'll move it. > > > > unsigned long pos; > > > > > > + addr = dereference_kernel_function_descriptor(addr); > > > pos = get_symbol_pos(addr, symbolsize, offset); > > > > I still wonder if doing the dereference in the widely used kallsyms > > might cause any regression. > > more testing wouldn't hurt, yes. > > > Also get_symbol_pos() is called in several other helpers > > but the dereference is done only here. It would be > > confusing if for example kallsyms_lookup_size_offset() > > and kallsyms_lookup() give different result. > > hm, so there is no change in this regard, right? there was no > deference before, there is no dereference now. what am I missing? But there was no dereference in kallsyms_lookup() before and there is dereference now. I mean that both kallsyms_lookup_size_offset() and kallsyms_lookup() always returned the same @symbolsize and @offset before this patch. But they might give different results now because kallsyms_lookup() might be newly working with dereferenced value. It is non-consistent, unexpected behavior and might cause problems. > I'm touching the pf/pF part in this patch set. if there are cases > of missing dereferences anywhere else then we need to address it > in a separate patch set, I think. You are changing the behavior of kallsyms_lookup() and introduce a possible inconsistency in this patchset. It might be innocent if kallsyms are used only to display debug messages. But there are even functional dependencies, for example kallsyms_lookup() is called in ftrace_match_record(). > > I would feel much more comfortable if we keep the derefenrece > > only in vsprintf. > > at a price of extra module lookup, because we need `struct module *' > for module address dereference. It would be more code but it should not be slower. The module lookup is just hidden in the kallsyms call now. Another solution would be to add another helper function into kallsyms that does the dereference and keep the current one as is. I think that the dereference might make sense even in the kallsyms code. But we need to make sure that it is safe and consistent. This complicates review of this patchset. Best Regards, Petr -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On (10/20/17 15:08), Petr Mladek wrote: > On Thu 2017-10-19 15:42:35, Sergey Senozhatsky wrote: > > Sorry for the delay and thanks for taking a look. > > > > I'll try to re-spin the patch set by the end of this week/early next > > week. > > > > > > On (10/04/17 13:53), Petr Mladek wrote: > > [..] > > > Note that kallsyms_lookup() and module_address_lookup() is used > > > in many other situations. > > > > we dereference only things that can be dereferenced. > > so calling it on already dereferenced address, or address > > that does need to be dereferenced is OK. > > My concern is that it changes the behavior. It will suddenly return > another information for addresses that were not dereference before. OK. I'd be really-really surprised to find out that anyone did kallsyms_lookup()/module_address_lookup() on func descriptors, but I understand your concerns. I'll try to keep everything within vsprintf(). -ss -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt index 361789df51ec..3adbc4fdd482 100644 --- a/Documentation/printk-formats.txt +++ b/Documentation/printk-formats.txt @@ -50,26 +50,28 @@ Symbols/Function Pointers :: + %pS versatile_init+0x0/0x110 + %ps versatile_init %pF versatile_init+0x0/0x110 %pf versatile_init - %pS versatile_init+0x0/0x110 %pSR versatile_init+0x9/0x110 (with __builtin_extract_return_addr() translation) - %ps versatile_init %pB prev_fn_of_versatile_init+0x88/0x88 -The ``F`` and ``f`` specifiers are for printing function pointers, -for example, f->func, &gettimeofday. They have the same result as -``S`` and ``s`` specifiers. But they do an extra conversion on -ia64, ppc64 and parisc64 architectures where the function pointers -are actually function descriptors. - The ``S`` and ``s`` specifiers can be used for printing symbols from direct addresses, for example, __builtin_return_address(0), (void *)regs->ip. They result in the symbol name with (``S``) or without (``s``) offsets. If KALLSYMS are disabled then the symbol address is printed instead. +Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``) +and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and +parisc64 function pointers are indirect and, in fact, are function +descriptors, which require additional dereferencing before we can lookup +the symbol. As of now, ``S`` and ``s`` perform dereferencing on those +platforms (when needed), so ``F`` and ``f`` exist for compatibility +reasons only. + The ``B`` specifier results in the symbol name with offsets and should be used when printing stack backtraces. The specifier takes into consideration the effect of compiler optimisations which may occur @@ -77,8 +79,6 @@ when tail-call``s are used and marked with the noreturn GCC attribute. Examples:: - printk("Going to call: %pF\n", gettimeofday); - printk("Going to call: %pF\n", p->func); printk("%s: called from %pS\n", __func__, (void *)_RET_IP_); printk("%s: called from %pS\n", __func__, (void *)__builtin_return_address(0)); diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c index 127e7cfafa55..e2fc09ea9509 100644 --- a/kernel/kallsyms.c +++ b/kernel/kallsyms.c @@ -322,6 +322,7 @@ const char *kallsyms_lookup(unsigned long addr, if (is_ksym_addr(addr)) { unsigned long pos; + addr = dereference_kernel_function_descriptor(addr); pos = get_symbol_pos(addr, symbolsize, offset); /* Grab name */ kallsyms_expand_symbol(get_symbol_offset(pos), diff --git a/kernel/module.c b/kernel/module.c index b792e814150a..63361de377ad 100644 --- a/kernel/module.c +++ b/kernel/module.c @@ -3948,6 +3948,7 @@ const char *module_address_lookup(unsigned long addr, preempt_disable(); mod = __module_address(addr); if (mod) { + addr = dereference_module_function_descriptor(mod, addr); if (modname) *modname = mod->name; ret = get_ksymbol(mod, addr, size, offset); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index bcd906a39010..bf04b4f5d8e7 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -40,7 +40,6 @@ #include "../mm/internal.h" /* For the trace_print_flags arrays */ #include <asm/page.h> /* for PAGE_SIZE */ -#include <asm/sections.h> /* for dereference_function_descriptor() */ #include <asm/byteorder.h> /* cpu_to_le16 */ #include <linux/string_helpers.h> @@ -1721,10 +1720,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, } switch (*fmt) { - case 'F': + case 'F': /* %pF and %pf are kept for compatibility reasons only */ case 'f': - ptr = (void *)dereference_function_descriptor((unsigned long)ptr); - /* Fallthrough */ case 'S': case 's': case 'B':