module: fix __module_ref_addr()
in [Kernel]
|
Prev: [PATCH 1/2] x86/PCI: for host bridge address space collisions, show conflicting resource
Next: resource: shared I/O region support
From: Mathieu Desnoyers on 25 Mar 2010 11:40 (should really have CC'd lkml, here it is) * Mathieu Desnoyers (mathieu.desnoyers(a)efficios.com) wrote: > __module_ref_addr() should use per_cpu_ptr() to obfuscate the pointer > (RELOC_HIDE is needed for per cpu pointers). > > This non-standard per-cpu pointer use has been introduced by commit > 720eba31f47aeade8ec130ca7f4353223c49170f > > It causes a NULL pointer exception on some configurations when CONFIG_TRACING is > enabled on 2.6.33. This patch fixes the problem (acknowledged by Randy who > reported the bug). > > It did not appear to hurt previously because most of the accesses were done > through local_inc, which probably obfuscated the access enough that no compiler > optimizations were done. But with local_read() done when CONFIG_TRACING is > active, this becomes a problem. Non-CONFIG_TRACING is probably affected as well > (module.c contains local_set and local_read that use __module_ref_addr()), but I > guess nobody noticed because we've been lucky enough that the compiler did not > generate the inappropriate optimization pattern there. > > This patch should be queued for the 2.6.29.x through 2.6.33.x stable branches. > (tested on 2.6.33.1 x86_64) > > The __module_ref_addr() problem disappears in 2.6.34-rc kernels because these > percpu accesses were re-factored. > > It makes me wonder about other non-standard uses of per_cpu_offset: there is one > in module.c and two in lockdep.c, which are still in 2.6.34-rc. This should > probably be fixed by the code authors in separate patches. > > lockdep.c: commit 8e18257d29238311e82085152741f0c3aa18b74d > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3 > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers(a)efficios.com> > Tested-by: Randy Dunlap <randy.dunlap(a)oracle.com> > CC: Eric Dumazet <dada1(a)cosmosbay.com> > CC: Rusty Russell <rusty(a)rustcorp.com.au> > CC: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > CC: Tejun Heo <tj(a)kernel.org> > CC: Ingo Molnar <mingo(a)elte.hu> > CC: Andrew Morton <akpm(a)linux-foundation.org> > CC: Linus Torvalds <torvalds(a)linux-foundation.org> > CC: Greg Kroah-Hartman <gregkh(a)suse.de> > --- > include/linux/module.h | 2 +- > kernel/module.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > Index: linux-2.6-lttng/include/linux/module.h > =================================================================== > --- linux-2.6-lttng.orig/include/linux/module.h 2010-03-25 11:01:53.000000000 -0400 > +++ linux-2.6-lttng/include/linux/module.h 2010-03-25 11:01:59.000000000 -0400 > @@ -467,7 +467,7 @@ void symbol_put_addr(void *addr); > static inline local_t *__module_ref_addr(struct module *mod, int cpu) > { > #ifdef CONFIG_SMP > - return (local_t *) (mod->refptr + per_cpu_offset(cpu)); > + return (local_t *) per_cpu_ptr(mod->refptr, cpu); > #else > return &mod->ref; > #endif > > -- > Mathieu Desnoyers > Operating System Efficiency R&D Consultant > EfficiOS Inc. > http://www.efficios.com -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Steven Rostedt on 29 Mar 2010 15:30 This patch does not apply to 2.6.34-rc, and the code in upstream looks to have been fixed. Should this go to stable? -- Steve On Sat, 2010-03-27 at 10:31 -0400, Mathieu Desnoyers wrote: > __module_ref_addr() should use per_cpu_ptr() to obfuscate the pointer > (RELOC_HIDE is needed for per cpu pointers). > > This non-standard per-cpu pointer use has been introduced by commit > 720eba31f47aeade8ec130ca7f4353223c49170f > > It causes a NULL pointer exception on some configurations when CONFIG_TRACING is > enabled on 2.6.33. This patch fixes the problem (acknowledged by Randy who > reported the bug). > > It did not appear to hurt previously because most of the accesses were done > through local_inc, which probably obfuscated the access enough that no compiler > optimizations were done. But with local_read() done when CONFIG_TRACING is > active, this becomes a problem. Non-CONFIG_TRACING is probably affected as well > (module.c contains local_set and local_read that use __module_ref_addr()), but I > guess nobody noticed because we've been lucky enough that the compiler did not > generate the inappropriate optimization pattern there. > > This patch should be queued for the 2.6.29.x through 2.6.33.x stable branches. > (tested on 2.6.33.1 x86_64) > > The __module_ref_addr() problem disappears in 2.6.34-rc kernels because these > percpu accesses were re-factored. > > It makes me wonder about other non-standard uses of per_cpu_offset: there is one > in module.c and two in lockdep.c, which are still in 2.6.34-rc. This should > probably be fixed by the code authors in separate patches. > > lockdep.c: commit 8e18257d29238311e82085152741f0c3aa18b74d > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3 > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers(a)efficios.com> > Tested-by: Randy Dunlap <randy.dunlap(a)oracle.com> > CC: Eric Dumazet <dada1(a)cosmosbay.com> > CC: Rusty Russell <rusty(a)rustcorp.com.au> > CC: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > CC: Tejun Heo <tj(a)kernel.org> > CC: Ingo Molnar <mingo(a)elte.hu> > CC: Andrew Morton <akpm(a)linux-foundation.org> > CC: Linus Torvalds <torvalds(a)linux-foundation.org> > CC: Greg Kroah-Hartman <gregkh(a)suse.de> > CC: Steven Rostedt <rostedt(a)goodmis.org> > --- > include/linux/module.h | 2 +- > kernel/module.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > Index: linux-2.6-lttng/include/linux/module.h > =================================================================== > --- linux-2.6-lttng.orig/include/linux/module.h 2010-03-25 11:01:53.000000000 -0400 > +++ linux-2.6-lttng/include/linux/module.h 2010-03-25 11:01:59.000000000 -0400 > @@ -467,7 +467,7 @@ void symbol_put_addr(void *addr); > static inline local_t *__module_ref_addr(struct module *mod, int cpu) > { > #ifdef CONFIG_SMP > - return (local_t *) (mod->refptr + per_cpu_offset(cpu)); > + return (local_t *) per_cpu_ptr(mod->refptr, cpu); > #else > return &mod->ref; > #endif > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mathieu Desnoyers on 29 Mar 2010 16:20 * Steven Rostedt (rostedt(a)goodmis.org) wrote: > This patch does not apply to 2.6.34-rc, and the code in upstream looks > to have been fixed. Should this go to stable? Yes. 2.6.34-rc does not have this issue anymore, but the patch is needed in -stable. Thanks, Mathieu > > -- Steve > > > On Sat, 2010-03-27 at 10:31 -0400, Mathieu Desnoyers wrote: > > __module_ref_addr() should use per_cpu_ptr() to obfuscate the pointer > > (RELOC_HIDE is needed for per cpu pointers). > > > > This non-standard per-cpu pointer use has been introduced by commit > > 720eba31f47aeade8ec130ca7f4353223c49170f > > > > It causes a NULL pointer exception on some configurations when CONFIG_TRACING is > > enabled on 2.6.33. This patch fixes the problem (acknowledged by Randy who > > reported the bug). > > > > It did not appear to hurt previously because most of the accesses were done > > through local_inc, which probably obfuscated the access enough that no compiler > > optimizations were done. But with local_read() done when CONFIG_TRACING is > > active, this becomes a problem. Non-CONFIG_TRACING is probably affected as well > > (module.c contains local_set and local_read that use __module_ref_addr()), but I > > guess nobody noticed because we've been lucky enough that the compiler did not > > generate the inappropriate optimization pattern there. > > > > This patch should be queued for the 2.6.29.x through 2.6.33.x stable branches. > > (tested on 2.6.33.1 x86_64) > > > > The __module_ref_addr() problem disappears in 2.6.34-rc kernels because these > > percpu accesses were re-factored. > > > > It makes me wonder about other non-standard uses of per_cpu_offset: there is one > > in module.c and two in lockdep.c, which are still in 2.6.34-rc. This should > > probably be fixed by the code authors in separate patches. > > > > lockdep.c: commit 8e18257d29238311e82085152741f0c3aa18b74d > > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3 > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers(a)efficios.com> > > Tested-by: Randy Dunlap <randy.dunlap(a)oracle.com> > > CC: Eric Dumazet <dada1(a)cosmosbay.com> > > CC: Rusty Russell <rusty(a)rustcorp.com.au> > > CC: Peter Zijlstra <a.p.zijlstra(a)chello.nl> > > CC: Tejun Heo <tj(a)kernel.org> > > CC: Ingo Molnar <mingo(a)elte.hu> > > CC: Andrew Morton <akpm(a)linux-foundation.org> > > CC: Linus Torvalds <torvalds(a)linux-foundation.org> > > CC: Greg Kroah-Hartman <gregkh(a)suse.de> > > CC: Steven Rostedt <rostedt(a)goodmis.org> > > --- > > include/linux/module.h | 2 +- > > kernel/module.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > Index: linux-2.6-lttng/include/linux/module.h > > =================================================================== > > --- linux-2.6-lttng.orig/include/linux/module.h 2010-03-25 11:01:53.000000000 -0400 > > +++ linux-2.6-lttng/include/linux/module.h 2010-03-25 11:01:59.000000000 -0400 > > @@ -467,7 +467,7 @@ void symbol_put_addr(void *addr); > > static inline local_t *__module_ref_addr(struct module *mod, int cpu) > > { > > #ifdef CONFIG_SMP > > - return (local_t *) (mod->refptr + per_cpu_offset(cpu)); > > + return (local_t *) per_cpu_ptr(mod->refptr, cpu); > > #else > > return &mod->ref; > > #endif > > > > -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Jiri Benc on 28 Apr 2010 11:00 On Thu, 22 Apr 2010 12:10:22 -0700, Greg KH wrote: > __module_ref_addr() should use per_cpu_ptr() to obfuscate the pointer > (RELOC_HIDE is needed for per cpu pointers). Hi, this patch triggers NaT consumption exception on IA64, at least with gcc 4.3.4: ------------ modprobe[151]: NaT consumption 2216203124768 [1] Modules linked in: Supported: Yes Pid: 151, CPU 2, comm: modprobe psr : 0000121009526030 ifs : 8000000000001430 ip : [<a000000100129871>] Not tainted (2.6.32.12-0.2-default) ip is at load_module+0xdd1/0x2300 unat: 0000000000000000 pfs : 0000000000001430 rsc : 0000000000000003 rnat: a000000100b45e28 bsps: 0000000000000000 pr : 0000000000165a59 ldrs: 0000000000000000 ccv : 0000000000000000 fpsr: 0009804c0270033f csd : 0000000000000000 ssd : 0000000000000000 b0 : a000000100129830 b6 : a000000100122ae0 b7 : a000000100011370 f6 : 000000000000000000000 f7 : 1003ee21a291c077975b9 f8 : 1003e0000000000000089 f9 : 1003e0000000000000001 f10 : 1003e0000000000000089 f11 : 000000000000000000000 r1 : a0000001015befe0 r2 : 0000000000000000 r3 : 0000000000001000 r8 : 0000000000000000 r9 : a0000001013dcc70 r10 : a0000001013dcc70 r11 : 0000000000000008 r12 : e00000018b26fdc0 r13 : e00000018b260000 r14 : 0000000000000008 r15 : 0000000000000fc0 r16 : 0000000000000000 r17 : 0000000000000000 r18 : 0000000000000000 r19 : 0000000000000000 r20 : 00000000000000fe r21 : 0000000000000008 r22 : 0000000000004a68 r23 : 0000000000004a68 r24 : 0000000000004a68 r25 : 00000000ffff4a68 r26 : e0000001820f1ec0 r27 : 0000000000000000 r28 : 0000000000000000 r29 : 0000000000000003 r30 : 0000000000000004 r31 : 0000000000000002 Call Trace: [<a000000100017a80>] show_stack+0x80/0xa0 sp=e00000018b26f910 bsp=e00000018b261438 [<a0000001000180e0>] show_regs+0x640/0x920 sp=e00000018b26fae0 bsp=e00000018b2613d8 [<a000000100029470>] die+0x190/0x2e0 sp=e00000018b26faf0 bsp=e00000018b261398 [<a000000100029610>] die_if_kernel+0x50/0x80 sp=e00000018b26faf0 bsp=e00000018b261368 [<a0000001008ea750>] ia64_fault+0xf0/0xda0 sp=e00000018b26faf0 bsp=e00000018b261320 [<a00000010000cb60>] ia64_native_leave_kernel+0x0/0x270 sp=e00000018b26fbf0 bsp=e00000018b261320 [<a000000100129870>] load_module+0xdd0/0x2300 sp=e00000018b26fdc0 bsp=e00000018b261198 [<a00000010012ae70>] sys_init_module+0xd0/0x620 sp=e00000018b26fe30 bsp=e00000018b261120 [<a00000010000c9c0>] ia64_ret_from_syscall+0x0/0x20 sp=e00000018b26fe30 bsp=e00000018b261120 [<a000000000010720>] __kernel_syscall_via_break+0x0/0x20 sp=e00000018b270000 bsp=e00000018b261120 ------------ It's dying here: static inline local_t *__module_ref_addr(struct module *mod, int cpu) { #ifdef CONFIG_SMP ---> return (local_t *) (mod->refptr + per_cpu_offset(cpu)); #else return &mod->ref; #endif } This particular instance of __module_ref_addr is inlined in module_unload_init, which is inlined in load_module. I don't know whether this is a gcc or a kernel bug but the code produced on IA64 is not what has been intended. Comparing the code generated by gcc -S [1]. For the working case, i.e. (local_t *) (mod->refptr + per_cpu_offset(cpu)), per_cpu_offset(cpu) translates to: ld8.mov r35 = [r83], __per_cpu_offset# // tmp2056, tmp2056, .... sxt4 r18 = r8 // cpu, cpu .... shladd r17 = r18, 3, r35 // tmp1140, cpu,, tmp2056 For the crashing case, i.e. (local_t *) per_cpu_ptr(mod->refptr, cpu), per_cpu_offset(cpu) translates to: sxt4 r17 = r8 // cpu, cpu .... shladd r11 = r17, 3, r0 // tmp1140, cpu, Note that r0 is always zero, which means __per_cpu_offset is considered to be always NULL. The problem seems to be triggered by RELOC_HIDE. I don't understand gcc well enough to be able to tell what's going on here. Any ideas? Thanks, Jiri [1] http://labs.suse.cz/jbenc/kernel/reloc_hide-ia64-crash/ -- Jiri Benc SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mathieu Desnoyers on 28 Apr 2010 12:30
* Jiri Benc (jbenc(a)suse.cz) wrote: > On Thu, 22 Apr 2010 12:10:22 -0700, Greg KH wrote: > > __module_ref_addr() should use per_cpu_ptr() to obfuscate the pointer > > (RELOC_HIDE is needed for per cpu pointers). > > Hi, > > this patch triggers NaT consumption exception on IA64, at least with > gcc 4.3.4: > > ------------ > modprobe[151]: NaT consumption 2216203124768 [1] > Modules linked in: > Supported: Yes > Pid: 151, CPU 2, comm: modprobe > psr : 0000121009526030 ifs : 8000000000001430 ip : [<a000000100129871>] Not tainted (2.6.32.12-0.2-default) > ip is at load_module+0xdd1/0x2300 > unat: 0000000000000000 pfs : 0000000000001430 rsc : 0000000000000003 > rnat: a000000100b45e28 bsps: 0000000000000000 pr : 0000000000165a59 > ldrs: 0000000000000000 ccv : 0000000000000000 fpsr: 0009804c0270033f > csd : 0000000000000000 ssd : 0000000000000000 > b0 : a000000100129830 b6 : a000000100122ae0 b7 : a000000100011370 > f6 : 000000000000000000000 f7 : 1003ee21a291c077975b9 > f8 : 1003e0000000000000089 f9 : 1003e0000000000000001 > f10 : 1003e0000000000000089 f11 : 000000000000000000000 > r1 : a0000001015befe0 r2 : 0000000000000000 r3 : 0000000000001000 > r8 : 0000000000000000 r9 : a0000001013dcc70 r10 : a0000001013dcc70 > r11 : 0000000000000008 r12 : e00000018b26fdc0 r13 : e00000018b260000 > r14 : 0000000000000008 r15 : 0000000000000fc0 r16 : 0000000000000000 > r17 : 0000000000000000 r18 : 0000000000000000 r19 : 0000000000000000 > r20 : 00000000000000fe r21 : 0000000000000008 r22 : 0000000000004a68 > r23 : 0000000000004a68 r24 : 0000000000004a68 r25 : 00000000ffff4a68 > r26 : e0000001820f1ec0 r27 : 0000000000000000 r28 : 0000000000000000 > r29 : 0000000000000003 r30 : 0000000000000004 r31 : 0000000000000002 > > Call Trace: > [<a000000100017a80>] show_stack+0x80/0xa0 > sp=e00000018b26f910 bsp=e00000018b261438 > [<a0000001000180e0>] show_regs+0x640/0x920 > sp=e00000018b26fae0 bsp=e00000018b2613d8 > [<a000000100029470>] die+0x190/0x2e0 > sp=e00000018b26faf0 bsp=e00000018b261398 > [<a000000100029610>] die_if_kernel+0x50/0x80 > sp=e00000018b26faf0 bsp=e00000018b261368 > [<a0000001008ea750>] ia64_fault+0xf0/0xda0 > sp=e00000018b26faf0 bsp=e00000018b261320 > [<a00000010000cb60>] ia64_native_leave_kernel+0x0/0x270 > sp=e00000018b26fbf0 bsp=e00000018b261320 > [<a000000100129870>] load_module+0xdd0/0x2300 > sp=e00000018b26fdc0 bsp=e00000018b261198 > [<a00000010012ae70>] sys_init_module+0xd0/0x620 > sp=e00000018b26fe30 bsp=e00000018b261120 > [<a00000010000c9c0>] ia64_ret_from_syscall+0x0/0x20 > sp=e00000018b26fe30 bsp=e00000018b261120 > [<a000000000010720>] __kernel_syscall_via_break+0x0/0x20 > sp=e00000018b270000 bsp=e00000018b261120 > ------------ > > It's dying here: > > static inline local_t *__module_ref_addr(struct module *mod, int cpu) > { > #ifdef CONFIG_SMP > ---> return (local_t *) (mod->refptr + per_cpu_offset(cpu)); > #else > return &mod->ref; > #endif > } > > This particular instance of __module_ref_addr is inlined in > module_unload_init, which is inlined in load_module. > > I don't know whether this is a gcc or a kernel bug but the code > produced on IA64 is not what has been intended. > > Comparing the code generated by gcc -S [1]. For the working case, i.e. > (local_t *) (mod->refptr + per_cpu_offset(cpu)), > per_cpu_offset(cpu) translates to: > > ld8.mov r35 = [r83], __per_cpu_offset# // tmp2056, tmp2056, > ... > sxt4 r18 = r8 // cpu, cpu > ... > shladd r17 = r18, 3, r35 // tmp1140, cpu,, tmp2056 > > For the crashing case, i.e. > (local_t *) per_cpu_ptr(mod->refptr, cpu), > per_cpu_offset(cpu) translates to: > > sxt4 r17 = r8 // cpu, cpu > ... > shladd r11 = r17, 3, r0 // tmp1140, cpu, > > Note that r0 is always zero, which means __per_cpu_offset is considered > to be always NULL. > > The problem seems to be triggered by RELOC_HIDE. I don't understand gcc > well enough to be able to tell what's going on here. Any ideas? Can you try reverting commit c8d52465f95c4187871f8e65666c07806ca06d41 and see if it helps ? That would revert the =r constraint back to an =g in RELOC_HIDE. The move from =g to =r has originally been done to work around a ppc64 compiler bug in the first place. My gut feeling is that ia64 does something unexpected if we apply a =r constraint where we should not. If you have other compiler versions handy, that would also be helpful to see if the problem is specific to the gcc version you are using. Thanks, Mathieu > > Thanks, > > Jiri > > [1] http://labs.suse.cz/jbenc/kernel/reloc_hide-ia64-crash/ > > -- > Jiri Benc > SUSE Labs -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |