Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/14/17 16:01), Luck, Tony wrote:
>
> > let's hear from ia64 and ppc64 guys.
>
> If you write a patch, I can try it on some ia64 h/w.
>
> Please include some test cases (perhaps as a second patch that adds a few
> good/bad %pF and %pS
> to some code (both in base kernel, and in a module).
Hello,
I sent out an RFC patch set.
would the following test case work for you?
kernel)
echo 1 > /proc/sys/vm/drop_caches
module)
modprobe zram
echo 100M > /sys/block/zram0/disksize
---
drivers/block/zram/zram_drv.c | 15 +++
fs/drop_caches.c | 11 +++
2 files changed, 26 insertions(+)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 2981c27d3aae..ac92aaeaced0 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -1508,6 +1508,12 @@ static int zram_add(void)
struct request_queue *queue;
int ret, device_id;
+ printk("printk#13 %pF\n", (void *)_RET_IP_);
+ printk("printk#14 %pS\n", (void *)_RET_IP_);
+
+ printk("printk#15 %pF\n", __builtin_return_address(0));
+ printk("printk#16 %pS\n", __builtin_return_address(0));
+
zram = kzalloc(sizeof(struct zram), GFP_KERNEL);
if (!zram)
return -ENOMEM;
@@ -1730,6 +1736,15 @@ static int __init zram_init(void)
{
int ret;
+ printk("printk#7 %pF\n", zram_init);
+ printk("printk#8 %pS\n", zram_init);
+
+ printk("printk#9 %pF\n", (void *)_RET_IP_);
+ printk("printk#10 %pS\n", (void *)_RET_IP_);
+
+ printk("printk#11 %pF\n", __builtin_return_address(0));
+ printk("printk#12 %pS\n", __builtin_return_address(0));
+
ret = cpuhp_setup_state_multi(CPUHP_ZCOMP_PREPARE, "block/zram:prepare",
zcomp_cpu_up_prepare, zcomp_cpu_dead);
if (ret < 0)
diff --git a/fs/drop_caches.c b/fs/drop_caches.c
index d72d52b90433..3db4adcac78d 100644
--- a/fs/drop_caches.c
+++ b/fs/drop_caches.c
@@ -39,11 +39,22 @@ static void drop_pagecache_sb(struct super_block *sb, void
*unused)
iput(toput_inode);
}
+#include
+
int drop_caches_sysctl_handler(struct ctl_table *table, int write,
void __user *buffer, size_t *length, loff_t *ppos)
{
int ret;
+ printk("printk#1 %pF\n", schedule_timeout);
+ printk("printk#2 %pS\n", schedule_timeout);
+
+ printk("printk#3 %pF\n", (void *)_RET_IP_);
+ printk("printk#4 %pS\n", (void *)_RET_IP_);
+
+ printk("printk#5 %pF\n", __builtin_return_address(0));
+ printk("printk#6 %pS\n", __builtin_return_address(0));
+
ret = proc_dointvec_minmax(table, write, buffer, length, ppos);
if (ret)
return ret;
RE: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
> let's hear from ia64 and ppc64 guys. If you write a patch, I can try it on some ia64 h/w. Please include some test cases (perhaps as a second patch that adds a few good/bad %pF and %pS to some code (both in base kernel, and in a module). -Tony
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On 14.09.2017 11:27, Sergey Senozhatsky wrote: On (09/14/17 10:39), Helge Deller wrote: [..] The basic concept of your proposal may work, and since it will avoid such coding issues in the future I think it's probably the best solution. Will you come up with a patch ? (I won't have time the next few days). If yes,I'd be happy to test it on parisc. cool. I think I can try to come up with something. I don't have any access to affected H/W, so if I won't be able to bring up qemu images There is no qemu for parisc yet, but cross-compiling the kernel on Fedora or Debian from x86_64 is easy and ready-to-go. Otherwise it's not a problem, I can try in a few days. I'll be happy to just hand it over to platform maintainers: arch-s like parisc64 are way to exotic ;) my aim is a removal of %pf/%pF here. let's hear from ia64 and ppc64 guys. Ok too. Helge
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/14/17 10:39), Helge Deller wrote: [..] > The basic concept of your proposal may work, and since it will avoid such > coding issues in the future I think it's probably the best solution. > > Will you come up with a patch ? (I won't have time the next few days). > If yes,I'd be happy to test it on parisc. cool. I think I can try to come up with something. I don't have any access to affected H/W, so if I won't be able to bring up qemu images I'll be happy to just hand it over to platform maintainers: arch-s like parisc64 are way to exotic ;) my aim is a removal of %pf/%pF here. let's hear from ia64 and ppc64 guys. -ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On 14.09.2017 10:03, Sergey Senozhatsky wrote:
On (09/14/17 16:40), Sergey Senozhatsky wrote:
[..]
powerpc and parisc handle kernel .opd section as well:
arch/powerpc/kernel/vmlinux.lds.S: .opd
arch/parisc/kernel/vmlinux.lds.S: .opd
for modules, arch-s define mod_arch_specific struct.
parisc has .opd
(fdesc offset should be the start of .opd,
fdesc_offset + sizeof(fdesc) * fdesc_count should be the .opd address range)
struct mod_arch_specific
{
unsigned long got_offset, got_count, got_max;
unsigned long fdesc_offset, fdesc_count, fdesc_max;
struct {
unsigned long stub_offset;
unsigned int stub_entries;
} *section;
int unwind_section;
struct unwind_table *unwind;
};
ia64 has .opd
struct mod_arch_specific {
struct elf64_shdr *core_plt;/* core PLT section */
struct elf64_shdr *init_plt;/* init PLT section */
struct elf64_shdr *got; /* global offset table */
struct elf64_shdr *opd; /* official procedure descriptors */
struct elf64_shdr *unwind; /* unwind-table section */
unsigned long gp; /* global-pointer for module */
void *core_unw_table; /* core unwind-table cookie returned
by unwinder */
void *init_unw_table; /* init unwind-table cookie returned
by unwinder */
unsigned int next_got_entry;/* index of next available got entry */
};
powerpc does not keep track of .opd, need to add
struct mod_arch_specific {
#ifdef __powerpc64__
unsigned int stubs_section; /* Index of stubs section in module */
unsigned int toc_section; /* What section is the TOC? */
bool toc_fixed; /* Have we fixed up .TOC.? */
#ifdef CONFIG_DYNAMIC_FTRACE
unsigned long toc;
unsigned long tramp;
#endif
#else /* powerpc64 */
/* Indices of PLT sections within module. */
unsigned int core_plt_section;
unsigned int init_plt_section;
#ifdef CONFIG_DYNAMIC_FTRACE
unsigned long tramp;
#endif
#endif /* powerpc64 */
/* List of BUG addresses, source line numbers and filenames */
struct list_head bug_list;
struct bug_entry *bug_table;
unsigned int num_bugs;
};
seems like we are looking at a solution here.
thoughts?
The basic concept of your proposal may work, and since it will avoid such
coding issues in the future I think it's probably the best solution.
Will you come up with a patch ? (I won't have time the next few days).
If yes,I'd be happy to test it on parisc.
Helge
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/14/17 16:40), Sergey Senozhatsky wrote:
[..]
> powerpc and parisc handle kernel .opd section as well:
>
> arch/powerpc/kernel/vmlinux.lds.S: .opd
> arch/parisc/kernel/vmlinux.lds.S: .opd
for modules, arch-s define mod_arch_specific struct.
parisc has .opd
(fdesc offset should be the start of .opd,
fdesc_offset + sizeof(fdesc) * fdesc_count should be the .opd address range)
struct mod_arch_specific
{
unsigned long got_offset, got_count, got_max;
unsigned long fdesc_offset, fdesc_count, fdesc_max;
struct {
unsigned long stub_offset;
unsigned int stub_entries;
} *section;
int unwind_section;
struct unwind_table *unwind;
};
ia64 has .opd
struct mod_arch_specific {
struct elf64_shdr *core_plt;/* core PLT section */
struct elf64_shdr *init_plt;/* init PLT section */
struct elf64_shdr *got; /* global offset table */
struct elf64_shdr *opd; /* official procedure descriptors */
struct elf64_shdr *unwind; /* unwind-table section */
unsigned long gp; /* global-pointer for module */
void *core_unw_table; /* core unwind-table cookie returned by
unwinder */
void *init_unw_table; /* init unwind-table cookie returned by
unwinder */
unsigned int next_got_entry;/* index of next available got entry */
};
powerpc does not keep track of .opd, need to add
struct mod_arch_specific {
#ifdef __powerpc64__
unsigned int stubs_section; /* Index of stubs section in module */
unsigned int toc_section; /* What section is the TOC? */
bool toc_fixed; /* Have we fixed up .TOC.? */
#ifdef CONFIG_DYNAMIC_FTRACE
unsigned long toc;
unsigned long tramp;
#endif
#else /* powerpc64 */
/* Indices of PLT sections within module. */
unsigned int core_plt_section;
unsigned int init_plt_section;
#ifdef CONFIG_DYNAMIC_FTRACE
unsigned long tramp;
#endif
#endif /* powerpc64 */
/* List of BUG addresses, source line numbers and filenames */
struct list_head bug_list;
struct bug_entry *bug_table;
unsigned int num_bugs;
};
seems like we are looking at a solution here.
thoughts?
-ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/08/17 20:28), Helge Deller wrote:
[..]
> I don't like this kind of trying to figure out at runtime at all.
> It's too much guessing in here IMHO.
well, may be we can avoid any guessing by checking that the
pointer belongs to .opd section.
for kernel we can add 2 new unsigned longs - __opd_start __opd_end -- and
tweak the corresponding arch/${FOO}/kernel/vmlinux.lds.S file to set those
two, the same way text, unwinding, etc. are set.
wait a second.
arch/ia64/kernel/vmlinux.lds.S already handles .opd section
.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
*(.opd)
}
it just doesn't save start/end addresses. so all we need to to
is
.opd : AT(ADDR(.opd) - LOAD_OFFSET) {
+ __opd_start = .;
*(.opd)
+ __opd_end = .;
}
and tweak symbol dereference
static inline void *dereference_function_descriptor(void *ptr)
{
struct fdesc *desc = ptr;
void *p;
+ if (prt < (void *)__start_opd || (void *)__end_opd < ptr)
+ return ptr;
+
if (!probe_kernel_address(&desc->ip, p))
ptr = p;
return ptr;
now, the modules.
module_frob_arch_sections() has the following lines
else if (strcmp(".opd", secstrings + s->sh_name) == 0)
mod->arch.opd = s;
so, once, again, we keep the .opd section info in memory. and we
also have the size of that section
mod->arch.opd->sh_size = fdescs * sizeof(struct fdesc);
so it seems that we've got what we need. need to provide arch callback
(same way as we do with dereference_function_descriptor() to properly
dereference modules' symbols).
so I think we almost have what we need to make ps/pS smart enough
on ppc64/ia64/parisc.
powerpc and parisc handle kernel .opd section as well:
arch/powerpc/kernel/vmlinux.lds.S: .opd
arch/parisc/kernel/vmlinux.lds.S: .opd
need to check more.
> What about this idea:
> For %pF we always have pointers to functions, e.g.:
> printk("Going to call: %pF\n", gettimeofday);
> printk("Going to call: %pF\n", p->func);
>
> and for %pS most (if not all) usages use some kind of casting
> from "unsigned long" to "void *", e.g.:
> printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
> printk("%s: called from %pS\n", __func__, (void
> *)__builtin_return_address(0));
> printk("Faulted at %pS\n", (void *)regs->ip);
>
> So, what if we for the %pS case simply take the type as it is
> (unsigned long) and introduce a new printk-format, e.g. "%luS" ?
> The %pS examples above then become:
> printk("%s: called from %luS\n", __func__, _RET_IP_);
> printk("%s: called from %luS\n", __func__,
> __builtin_return_address(0));
> printk("Faulted at %luS\n", regs->ip);
>
> That way we don't need type-casting, gain compile-time type
> checks from the compiler, and we could add a checkpatch (or occinelle)
> check which checks for the combination of %pF/%pS and "void*" keyword
> and suggest to use %luS.
>
> Opinions?
hm. sounds interesting. but I'm afraid people won't be so happy
to learn a new printk format specifier.
-ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/08/17 10:25), Luck, Tony wrote: > On Fri, Sep 08, 2017 at 03:18:30PM +0900, Sergey Senozhatsky wrote: > > if the addr is not in kernel .text, then try dereferencing it and check > > if the dereferenced addr is in kernel .text. > > If it really is a function pointer, then we know that it is safe > to dereference. But if it isn't, then maybe not? yes. I thought about it - we can do %pS on a pointer to a global structure. so that simple heuristic would not work reliably. we parse ELF sections, and we know the address range of .opd section, so we can check if the supplied pointer is within the .opd section or not. .opd does exist on ia64. not sure what's the name of ELF descriptor section on ppc64/parisc64. if we will be able to simply do .opd->address <= ptr <= .opd->address + .opd->size then it mostly should work for us. I guess. > If it is a function pointer then dereferening will indeed give > us a .text address. But if it isn't, it might still give us a > .text address (we could reduce the probability of a false hit > by checking that the .text address was exactly on a symbol with > no offset ... but data values that happen to be the addresses of > function entry points are possible). hm. yes, need to think more. -ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
Hi, On (09/08/17 22:49), Helge Deller wrote: [..] > 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) ? the idea is to have less format specifiers ;) %pF/%pf is a subtle ABI detail, which made it to API. I'm OK to keep %pf/%pF, if we won't be able to improve %ps/%pS. otherwise, I'd prefer to get rid of it. -ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
Hi,
On (09/08/17 22:23), Yu, Fenghua wrote:
> > From: Sergey Senozhatsky [mailto:sergey.senozhatsky.w...@gmail.com]
> > On (09/07/17 16:05), Luck, Tony wrote:
> > +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;
> > +}
>
> The __mod_text_address() may be defined only on IA64, PPC64 and PARISC since
> it's only called in those cases.
sure. well, I didn't post the exact patch. __mod_text_address() was
supposed to be used from __module_text_address() /* I factored out
__mod_text_address() from that function, basically */
---
@@ -4305,9 +4323,7 @@ struct module *__module_text_address(unsigned long addr)
{
struct module *mod = __module_address(addr);
if (mod) {
- /* 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))
+ if (!__mod_text_address(mod, addr))
mod = NULL;
}
return mod;
---
-ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On Wed 2017-09-06 22:27:47, Helge Deller wrote: > This patch series fixes the wrong usages of the %pF and %pS printk format > specifiers throughout the kernel code. > > Both specifiers have the same result on most architectures. But on ia64, ppc64 > and parisc64 architectures the %pF specifier does an extra conversion because > there function pointers are actually function descriptors. > > Helge > > Helge Deller (14): > arm: Use %pS printk format for symbols from direct addresses > um: Use %pS printk format for symbols from direct addresses IMHO, we should use %pB in this patch. > x86: Use %pS printk format for symbols from direct addresses > ti_sci: Use %pS printk format for direct addresses > i915: Use %pS printk format for direct addresses > md/bcache: Use %pS printk format for direct addresses > power/avs: Use %pS printk format for direct addresses > fs/f2fs: Use %pS printk format for direct addresses > fs/pstore: Use %pS printk format for direct addresses > fs/xfs: Use %pS printk format for direct addresses > smp: Use %pF printk format specifier for function pointers > mm/memblock: Use %pS printk format for direct addresses > netfilter/ipvs: Use %pS printk format for direct addresses > sound/core: Use %pS printk format for direct addresses All other patches look fine. For the entire patchset, except for the "um: " patch, feel free to use: Reviewed-by: Petr Mladek Let me known if this is enough or if I should answer each patch individually. Best Regards, Petr
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On Fri 2017-09-08 22:49:51, Helge Deller wrote: > 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. > > > > > > 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 seems that dereferencing an invalid function descriptor is rather safe because probe_kernel_address() prevents crashes. The question is if we could get wrong results by the autodetection. The following possibilities come to my mind: First, if the variable used to store the function descriptor is on stack and is not initialized. Then there is a non-trivial chance that the garbage on the stack will be a real return address to an existing function. Then the autodetection would help to hide this. Second, if wonder if the address of the function descriptor might be in callsyms as well. Note that global variables are in kallsyms as well. Then we would always print the name of this variable. I do not have a strong opinion here. On one hand, it is clear that %pS and %pF are often misused. But I am not sure if the above possible problems are acceptable. > 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. I am not sure if I miss something. But the different output of %lu and %c should be easy to distinguish. Also the difference is the same on all architectures and should be well known. This is not true for the %pS vs. %pF species. > What about the idea of a %luS specifier (or something other) ? I am not a big fun of this. IMHO, the relation between a pointer and symbol name makes more sense that a relation between an unsigned long and a symbol name. IMHO, this would just add even more confusion. Best Regards, Petr PS: I wonder if the improved documentation and fixing all occurrences might be enough to reduce this mistake. I guess that most of them were caused by copying the same pattern from an already broken code.
RE: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
> From: Sergey Senozhatsky [mailto:sergey.senozhatsky.w...@gmail.com]
> On (09/07/17 16:05), Luck, Tony wrote:
> +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;
> +}
The __mod_text_address() may be defined only on IA64, PPC64 and PARISC since
it's only called in those cases.
> +
> #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
Thanks.
-Fenghua
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
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
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On 08.09.2017 08:23, Sergey Senozhatsky wrote: > On (09/06/17 22:27), Helge Deller wrote: >> This patch series fixes the wrong usages of the %pF and %pS printk format >> specifiers throughout the kernel code. >> >> Both specifiers have the same result on most architectures. But on ia64, >> ppc64 >> and parisc64 architectures the %pF specifier does an extra conversion because >> there function pointers are actually function descriptors. > > Helge, > > did you also grep for %pf? or just for %pF? I grepped only for %pF and %pS. There might be wrong %ps/%pf usages too. Helge
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On 08.09.2017 19:25, Luck, Tony wrote:
> On Fri, Sep 08, 2017 at 03:18:30PM +0900, Sergey Senozhatsky wrote:
>> if the addr is not in kernel .text, then try dereferencing it and check
>> if the dereferenced addr is in kernel .text.
>
> If it really is a function pointer, then we know that it is safe
> to dereference. But if it isn't, then maybe not?
>
> If it is a function pointer then dereferening will indeed give
> us a .text address. But if it isn't, it might still give us a
> .text address (we could reduce the probability of a false hit
> by checking that the .text address was exactly on a symbol with
> no offset ... but data values that happen to be the addresses of
> function entry points are possible).
I don't like this kind of trying to figure out at runtime at all.
It's too much guessing in here IMHO.
What about this idea:
For %pF we always have pointers to functions, e.g.:
printk("Going to call: %pF\n", gettimeofday);
printk("Going to call: %pF\n", p->func);
and for %pS most (if not all) usages use some kind of casting
from "unsigned long" to "void *", e.g.:
printk("%s: called from %pS\n", __func__, (void *)_RET_IP_);
printk("%s: called from %pS\n", __func__, (void
*)__builtin_return_address(0));
printk("Faulted at %pS\n", (void *)regs->ip);
So, what if we for the %pS case simply take the type as it is
(unsigned long) and introduce a new printk-format, e.g. "%luS" ?
The %pS examples above then become:
printk("%s: called from %luS\n", __func__, _RET_IP_);
printk("%s: called from %luS\n", __func__, __builtin_return_address(0));
printk("Faulted at %luS\n", regs->ip);
That way we don't need type-casting, gain compile-time type
checks from the compiler, and we could add a checkpatch (or occinelle)
check which checks for the combination of %pF/%pS and "void*" keyword
and suggest to use %luS.
Opinions?
Helge
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On Fri, Sep 08, 2017 at 03:18:30PM +0900, Sergey Senozhatsky wrote: > if the addr is not in kernel .text, then try dereferencing it and check > if the dereferenced addr is in kernel .text. If it really is a function pointer, then we know that it is safe to dereference. But if it isn't, then maybe not? If it is a function pointer then dereferening will indeed give us a .text address. But if it isn't, it might still give us a .text address (we could reduce the probability of a false hit by checking that the .text address was exactly on a symbol with no offset ... but data values that happen to be the addresses of function entry points are possible). -Tony
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/06/17 22:27), Helge Deller wrote: > This patch series fixes the wrong usages of the %pF and %pS printk format > specifiers throughout the kernel code. > > Both specifiers have the same result on most architectures. But on ia64, ppc64 > and parisc64 architectures the %pF specifier does an extra conversion because > there function pointers are actually function descriptors. Helge, did you also grep for %pf? or just for %pF? -ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
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.
btw, get_ksymbol() is actually interesting. it scans module's sections,
so if we are able to distinguish descriptor ELF sections, then we can
dereference addr only if it belong to descriptor table ELF section.
-ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On Thu, 2017-09-07 at 14:38 +0200, Helge Deller wrote: > Instead, maybe adding some checks to scripts/checkpatch.pl can help? > E.g. warn if %pF is used in combination with the keywords like > _builtin_return_address, _RET_IP_, and similar. coccinelle is probably a better tool for that.
RE: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
>> 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). -Tony
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On 07.09.2017 11:51, Sergey Senozhatsky wrote:
> On (09/07/17 18:36), Sergey Senozhatsky wrote:
> [..]
>>> I can look into adding such check-code, but even then the warning will
>>> only show up if you run on ia64, ppc64 and parisc64.
>
> sorry, not sure I understand the "warning" part.
I was thinking about adding code which warns at runtime if %pF/%pS is
presumable used wrongly.
You are thinking about code to work around the complexity by some
kind of autodetection.
> what I'm thinking about is:
>
> - every platform that needs descriptor dereference defines its own
> function. otherwise dereference_descriptor(p) is just (p).
>
> - so it's something like
>
> arch/platform_abc/include/asm/sections.h
>
> #undef dereference_function_descriptor
> static inline void *dereference_function_descriptor(void *ptr)
> {
> 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.
> if (!probe_kernel_address())
> return function_ip;
> return ptr;
> }
>
> - so then in lib/vsprintf.c we can do unconditionally
>
> case F:
> case f:
> case S:
> case s:
> case B:
> ptr = dereference_function_descriptor(ptr);
> return symbol_string();
>
> because platforms will take care of proper descriptor dereference,
> when needed.
Ok, but...
> - and ideally we even can drop %pF-%pf. because there won't
> be any difference between `S' and `F'.
> something like this.
> let's see if this is possible.
> any thoughts?
I see your idea, nevertheless, there *is* a difference between
a "pointer to some assembler statement" (%pS), and a
"pointer to a function" (%pF) on some architectures.
That's why %pF and %pS printk specifiers were introduced in 2008
by Linus in commit 0fe1ef24f7bd0020f29ffe287dfdb9ead33ca0b2.
People will probably get it wrong sometimes, and to try to avoid this
by some magic autodetection is IMHO the wrong solution.
Instead, maybe adding some checks to scripts/checkpatch.pl can help?
E.g. warn if %pF is used in combination with the keywords like
_builtin_return_address, _RET_IP_, and similar.
Helge
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/07/17 18:36), Sergey Senozhatsky wrote:
[..]
> > I can look into adding such check-code, but even then the warning will
> > only show up if you run on ia64, ppc64 and parisc64.
sorry, not sure I understand the "warning" part.
what I'm thinking about is:
- every platform that needs descriptor dereference defines its own
function. otherwise dereference_descriptor(p) is just (p).
- so it's something like
arch/platform_abc/include/asm/sections.h
#undef dereference_function_descriptor
static inline void *dereference_function_descriptor(void *ptr)
{
if (not_a_function_descriptor(ptr))
return ptr;
if (!probe_kernel_address())
return function_ip;
return ptr;
}
- so then in lib/vsprintf.c we can do unconditionally
case F:
case f:
case S:
case s:
case B:
ptr = dereference_function_descriptor(ptr);
return symbol_string();
because platforms will take care of proper descriptor dereference,
when needed.
- and ideally we even can drop %pF-%pf. because there won't
be any difference between `S' and `F'.
something like this.
let's see if this is possible.
any thoughts?
-ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
(Cc Tony, Fenghua, Benjamin, Paul, Michael) a brief description: original patch set: lkml.kernel.org/r/1504729681-3504-1-git-send-email-del...@gmx.de start of this discussion: lkml.kernel.org/r/20170907075653.GA533@jagdpanzerIV.localdomain basically we are looking at possibilities to make %pF/%pS differences less disturbing. Helge has discovered a number of wrong usages in the kernel and has fixed the currently existing call sites; the question is what we can do to avoid this type of the patch sets in the future? /* assuming that no one reads printk documentation */ Hopefully you guys can help. On (09/07/17 11:12), Helge Deller wrote: [..] > > - ptr = dereference_function_descriptor(ptr); > > + ptr = __dereference_function_descriptor(ptr); > > This is not needed. > All affected arches (ia64, ppc64, parisc64) already call > probe_kernel_address() inside their dereference_function_descriptor() > function. > So this patch just adds unnecessary overhead for all arches. good, thanks. honestly, I obviously didn't check what each platform does. guilty! sort of. > > ... here is a question, does function descriptor belong to a special > > section? can we check that supplied ptr belongs to a descriptor section > > and avoid dereference_function_descriptor() if it doesn't? (just fall > > through directly to symbol_string() in this case). is this possible? > > I think theoretically yes. > On parisc ptr does *not* point to any code segment, and most likely it > points to the .data segment. I don't know if that's the case for ia64 and > ppc64 too. > I can look into adding such check-code, but even then the warning will > only show up if you run on ia64, ppc64 and parisc64. ok. personally I think that we need to start doing "does ptr belong to descriptor segment/section/etc" thing and skip dereference_function_descriptor() when it doesn't. [well, where possible. hopefully on every affected platform... if this problem actually bothers any one]. that seems like the way to fix the root cause of the problem. because it's you, Petr and may be 7 more guys who knows the difference between %pF/%pS. no one else has any idea at all. and no one actually reads the printk() documentation, let's be real :) -ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On 07.09.2017 10:32, Sergey Senozhatsky wrote:
> On (09/07/17 16:56), Sergey Senozhatsky wrote:
> [..]
> probe_kernel_address() handles the page fault and returns -EFAULT if
> you give it bad pointer. module_address_lookup() and get_symbol_pos()
> seems to be smart enough not to crash on bad pointer as well. what am
> I missing? could you please explain where we will crash?
Actually I never faced a kernel crash because of this on parisc.
Don't know for ia64 and ppc64.
>> BTW, are we sure we can crash? when attempt to deference IP from
>> the given descriptor? shall we handle page fault in this case and
>> do something sane? just asking.
>
> I don't know... does the below code make any sense?
No. Read below.
> basically it checks that it's safe to access ptr (we can access it
> without page fault in __dereference_function_descriptor()). then
> we do ptr->ip, and also check if it's safe, but in
> dereference_function_descriptor().
>
> I suppose somethign like
>
> pr_err("%pF\n", 1);
>
> can crash ia64, etc. correct?
On parisc it will not crash because we handle that one already.
For others I don't know.
But something like
pr_err("%pF\n", (unsigned long) (-1));
or any address above 0xULL might do bad things
on parisc, because it touches the I/O space and we don't check that yet.
> lib/vsprintf.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 86c3385b9eb3..0dc39b95e1d9 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1593,6 +1593,16 @@ char *device_node_string(char *buf, char *end, struct
> device_node *dn,
>
> int kptr_restrict __read_mostly;
>
> +static void *__dereference_function_descriptor(void *ptr)
> +{
> + void *p;
> +
> + if (!probe_kernel_address(ptr, p))
> + return dereference_function_descriptor(ptr);
> +
> + return ptr;
> +}
> +
> /*
> * Show a '%p' thing. A kernel extension is that the '%p' is followed
> * by an extra set of alphanumeric characters that are extended format
> @@ -1723,7 +1733,7 @@ char *pointer(const char *fmt, char *buf, char *end,
> void *ptr,
> switch (*fmt) {
> case 'F':
> case 'f':
> - ptr = dereference_function_descriptor(ptr);
> + ptr = __dereference_function_descriptor(ptr);
This is not needed.
All affected arches (ia64, ppc64, parisc64) already call
probe_kernel_address() inside their dereference_function_descriptor() function.
So this patch just adds unnecessary overhead for all arches.
> ... here is a question, does function descriptor belong to a special
> section? can we check that supplied ptr belongs to a descriptor section
> and avoid dereference_function_descriptor() if it doesn't? (just fall
> through directly to symbol_string() in this case). is this possible?
I think theoretically yes.
On parisc ptr does *not* point to any code segment, and most likely it
points to the .data segment. I don't know if that's the case for ia64 and
ppc64 too.
I can look into adding such check-code, but even then the warning will
only show up if you run on ia64, ppc64 and parisc64.
Helge
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/07/17 16:56), Sergey Senozhatsky wrote:
[..]
> BTW, are we sure we can crash? when attempt to deference IP from
> the given descriptor? shall we handle page fault in this case and
> do something sane? just asking.
I don't know... does the below code make any sense?
quick and dirty. NOT TESTED at all (not even compile tested).
we can avoid extra probe_kernel_address() on anything that is
not ia64, ppc64, etc.
basically it checks that it's safe to access ptr (we can access it
without page fault in __dereference_function_descriptor()). then
we do ptr->ip, and also check if it's safe, but in
dereference_function_descriptor().
I suppose somethign like
pr_err("%pF\n", 1);
can crash ia64, etc. correct?
well. not tested.
---
lib/vsprintf.c | 12 +++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 86c3385b9eb3..0dc39b95e1d9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1593,6 +1593,16 @@ char *device_node_string(char *buf, char *end, struct
device_node *dn,
int kptr_restrict __read_mostly;
+static void *__dereference_function_descriptor(void *ptr)
+{
+ void *p;
+
+ if (!probe_kernel_address(ptr, p))
+ return dereference_function_descriptor(ptr);
+
+ return ptr;
+}
+
/*
* Show a '%p' thing. A kernel extension is that the '%p' is followed
* by an extra set of alphanumeric characters that are extended format
@@ -1723,7 +1733,7 @@ char *pointer(const char *fmt, char *buf, char *end, void
*ptr,
switch (*fmt) {
case 'F':
case 'f':
- ptr = dereference_function_descriptor(ptr);
+ ptr = __dereference_function_descriptor(ptr);
/* Fallthrough */
case 'S':
case 's':
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
Hello Helge, On (09/07/17 08:01), Helge Deller wrote: [..] > > hm... > > can we fix it in lib/vsprintf.c instead? thanks for a quick reply. > There is nothing to fix in vsprintf, because it is already providing > both %pF and %pS for the two different architecture-specific API call > implementations. [..] > ia64, ppc64 and parisc64 architectures will be wrong and may lead > to kernel crashes in the worst case. ^ I was thinking about this part. sorry, I don't have access to ia64/ppc64/parisc64 so can't check it or test it. here is a question, does function descriptor belong to a special section? can we check that supplied ptr belongs to a descriptor section and avoid dereference_function_descriptor() if it doesn't? (just fall through directly to symbol_string() in this case). is this possible? I mean, there is no mechanism to prevent this type of wrongdoings in the future, we can't scan the entire kernel for wrong pF/pS all the time. BTW, are we sure we can crash? when attempt to deference IP from the given descriptor? shall we handle page fault in this case and do something sane? just asking. -ss
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On 07.09.2017 02:45, Sergey Senozhatsky wrote: > On (09/06/17 22:27), Helge Deller wrote: >> This patch series fixes the wrong usages of the %pF and %pS printk format >> specifiers throughout the kernel code. >> >> Both specifiers have the same result on most architectures. But on ia64, >> ppc64 >> and parisc64 architectures the %pF specifier does an extra conversion because >> there function pointers are actually function descriptors. > > hm... > can we fix it in lib/vsprintf.c instead? There is nothing to fix in vsprintf, because it is already providing both %pF and %pS for the two different architecture-specific API call implementations. > a) the patch set has "there is no problem on platform A, but still >let's just change it" > >which is probably fine. but > > b) you tweak/fix the currently existing users, OK. but there, most >likely, will be new users that will require fixes in the future. Please read the documentation in Documentation/printk-formats.txt. It's really as simple that if you use function pointers you *need to* use %pF, and if you use raw addresses of functions, you *need to* use %pS. If you use the correct specifier the output will automatically be correct for all architectures. If you don't, then the output on ia64, ppc64 and parisc64 architectures will be wrong and may lead to kernel crashes in the worst case. My patch series simply fixes that code which has it wrong. It has no impact at all on users on x86, ARM, mips and other platforms. Helge
Re: [PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
On (09/06/17 22:27), Helge Deller wrote: > This patch series fixes the wrong usages of the %pF and %pS printk format > specifiers throughout the kernel code. > > Both specifiers have the same result on most architectures. But on ia64, ppc64 > and parisc64 architectures the %pF specifier does an extra conversion because > there function pointers are actually function descriptors. hm... can we fix it in lib/vsprintf.c instead? a) the patch set has "there is no problem on platform A, but still let's just change it" which is probably fine. but b) you tweak/fix the currently existing users, OK. but there, most likely, will be new users that will require fixes in the future. -ss
[PATCH 00/14] Fix wrong %pF and %pS printk format specifier usages
This patch series fixes the wrong usages of the %pF and %pS printk format specifiers throughout the kernel code. Both specifiers have the same result on most architectures. But on ia64, ppc64 and parisc64 architectures the %pF specifier does an extra conversion because there function pointers are actually function descriptors. Helge Helge Deller (14): arm: Use %pS printk format for symbols from direct addresses um: Use %pS printk format for symbols from direct addresses x86: Use %pS printk format for symbols from direct addresses ti_sci: Use %pS printk format for direct addresses i915: Use %pS printk format for direct addresses md/bcache: Use %pS printk format for direct addresses power/avs: Use %pS printk format for direct addresses fs/f2fs: Use %pS printk format for direct addresses fs/pstore: Use %pS printk format for direct addresses fs/xfs: Use %pS printk format for direct addresses smp: Use %pF printk format specifier for function pointers mm/memblock: Use %pS printk format for direct addresses netfilter/ipvs: Use %pS printk format for direct addresses sound/core: Use %pS printk format for direct addresses arch/arm/mm/alignment.c | 2 +- arch/um/kernel/sysrq.c | 2 +- arch/x86/mm/extable.c| 4 ++-- arch/x86/xen/multicalls.c| 2 +- drivers/firmware/ti_sci.c| 2 +- drivers/gpu/drm/i915/intel_breadcrumbs.c | 2 +- drivers/md/bcache/closure.c | 4 ++-- drivers/power/avs/smartreflex.c | 10 +- fs/f2fs/f2fs.h | 2 +- fs/pstore/inode.c| 2 +- fs/xfs/xfs_error.c | 2 +- kernel/smp.c | 2 +- mm/memblock.c| 14 +++--- net/netfilter/ipvs/ip_vs_conn.c | 2 +- net/netfilter/ipvs/ip_vs_ctl.c | 4 ++-- sound/core/device.c | 4 ++-- 16 files changed, 30 insertions(+), 30 deletions(-) -- 2.1.0
