* [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() @ 2010-03-27 14:31 Mathieu Desnoyers 2010-03-29 19:21 ` Steven Rostedt 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Desnoyers @ 2010-03-27 14:31 UTC (permalink / raw) To: Steven Rostedt Cc: Randy Dunlap, Eric Dumazet, Rusty Russell, Peter Zijlstra, Tejun Heo, Ingo Molnar, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, linux-kernel __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@efficios.com> Tested-by: Randy Dunlap <randy.dunlap@oracle.com> CC: Eric Dumazet <dada1@cosmosbay.com> CC: Rusty Russell <rusty@rustcorp.com.au> CC: Peter Zijlstra <a.p.zijlstra@chello.nl> CC: Tejun Heo <tj@kernel.org> CC: Ingo Molnar <mingo@elte.hu> CC: Andrew Morton <akpm@linux-foundation.org> CC: Linus Torvalds <torvalds@linux-foundation.org> CC: Greg Kroah-Hartman <gregkh@suse.de> CC: Steven Rostedt <rostedt@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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-27 14:31 [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() Mathieu Desnoyers @ 2010-03-29 19:21 ` Steven Rostedt 2010-03-29 20:09 ` Mathieu Desnoyers 0 siblings, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2010-03-29 19:21 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Randy Dunlap, Eric Dumazet, Rusty Russell, Peter Zijlstra, Tejun Heo, Ingo Molnar, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, linux-kernel, stable 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@efficios.com> > Tested-by: Randy Dunlap <randy.dunlap@oracle.com> > CC: Eric Dumazet <dada1@cosmosbay.com> > CC: Rusty Russell <rusty@rustcorp.com.au> > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > CC: Tejun Heo <tj@kernel.org> > CC: Ingo Molnar <mingo@elte.hu> > CC: Andrew Morton <akpm@linux-foundation.org> > CC: Linus Torvalds <torvalds@linux-foundation.org> > CC: Greg Kroah-Hartman <gregkh@suse.de> > CC: Steven Rostedt <rostedt@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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-29 19:21 ` Steven Rostedt @ 2010-03-29 20:09 ` Mathieu Desnoyers 2010-03-29 21:07 ` [stable] " Greg KH 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Desnoyers @ 2010-03-29 20:09 UTC (permalink / raw) To: Steven Rostedt Cc: Randy Dunlap, Eric Dumazet, Rusty Russell, Peter Zijlstra, Tejun Heo, Ingo Molnar, Andrew Morton, Linus Torvalds, Greg Kroah-Hartman, linux-kernel, stable * Steven Rostedt (rostedt@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@efficios.com> > > Tested-by: Randy Dunlap <randy.dunlap@oracle.com> > > CC: Eric Dumazet <dada1@cosmosbay.com> > > CC: Rusty Russell <rusty@rustcorp.com.au> > > CC: Peter Zijlstra <a.p.zijlstra@chello.nl> > > CC: Tejun Heo <tj@kernel.org> > > CC: Ingo Molnar <mingo@elte.hu> > > CC: Andrew Morton <akpm@linux-foundation.org> > > CC: Linus Torvalds <torvalds@linux-foundation.org> > > CC: Greg Kroah-Hartman <gregkh@suse.de> > > CC: Steven Rostedt <rostedt@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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [stable] [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-29 20:09 ` Mathieu Desnoyers @ 2010-03-29 21:07 ` Greg KH 2010-03-30 1:08 ` Steven Rostedt 2010-03-30 2:12 ` Tejun Heo 0 siblings, 2 replies; 11+ messages in thread From: Greg KH @ 2010-03-29 21:07 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Steven Rostedt, Randy Dunlap, Greg Kroah-Hartman, Peter Zijlstra, stable, Rusty Russell, linux-kernel, Eric Dumazet, Tejun Heo, Ingo Molnar, Linus Torvalds, Andrew Morton On Mon, Mar 29, 2010 at 04:09:46PM -0400, Mathieu Desnoyers wrote: > * Steven Rostedt (rostedt@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. Why is this not in .34-rc2? Can you find the specific patch in Linus's tree that solves this and let stable@kernel.org know about it? thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [stable] [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-29 21:07 ` [stable] " Greg KH @ 2010-03-30 1:08 ` Steven Rostedt 2010-03-30 2:22 ` Mathieu Desnoyers 2010-03-30 2:12 ` Tejun Heo 1 sibling, 1 reply; 11+ messages in thread From: Steven Rostedt @ 2010-03-30 1:08 UTC (permalink / raw) To: Greg KH Cc: Mathieu Desnoyers, Randy Dunlap, Greg Kroah-Hartman, Peter Zijlstra, stable, Rusty Russell, linux-kernel, Eric Dumazet, Tejun Heo, Ingo Molnar, Linus Torvalds, Andrew Morton On Mon, 2010-03-29 at 14:07 -0700, Greg KH wrote: > On Mon, Mar 29, 2010 at 04:09:46PM -0400, Mathieu Desnoyers wrote: > > * Steven Rostedt (rostedt@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. > > Why is this not in .34-rc2? Can you find the specific patch in Linus's > tree that solves this and let stable@kernel.org know about it? > Looks like it was this commit: commit e1783a240f491fb233f04edc042e16b18a7a79ba Author: Christoph Lameter <cl@linux-foundation.org> Date: Tue Jan 5 15:34:50 2010 +0900 module: Use this_cpu_xx to dynamically allocate counters Mathieu's fix was: - return (local_t *) (mod->refptr + per_cpu_offset(cpu)); + return (local_t *) per_cpu_ptr(mod->refptr, cpu); As Mathieu states in his change log, the bug is that the mod->refptr is outside the assembly obfuscation of the per_cpu_offset(). This allows the compiler to optimize and cause a NULL pointer dereference with the manipulation of per cpu data. Christoph Lameter's change fixes this bug as a side effect: -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 -} - /* Sometimes we know we already have a refcount, and it's easier not to handle the error case (which only happens with rmmod --wait). */ static inline void __module_get(struct module *module) { if (module) { - unsigned int cpu = get_cpu(); - local_inc(__module_ref_addr(module, cpu)); + preempt_disable(); + __this_cpu_inc(module->refptr->count); trace_module_get(module, _THIS_IP_, - local_read(__module_ref_addr(module, cpu))); - put_cpu(); + __this_cpu_read(module->refptr->count)); + preempt_enable(); } } By removing the buggy code all together. -- Steve ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [stable] [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-30 1:08 ` Steven Rostedt @ 2010-03-30 2:22 ` Mathieu Desnoyers 2010-04-19 18:26 ` Greg KH 0 siblings, 1 reply; 11+ messages in thread From: Mathieu Desnoyers @ 2010-03-30 2:22 UTC (permalink / raw) To: Steven Rostedt Cc: Greg KH, Randy Dunlap, Greg Kroah-Hartman, Peter Zijlstra, stable, Rusty Russell, linux-kernel, Eric Dumazet, Tejun Heo, Ingo Molnar, Linus Torvalds, Andrew Morton * Steven Rostedt (rostedt@goodmis.org) wrote: > On Mon, 2010-03-29 at 14:07 -0700, Greg KH wrote: > > On Mon, Mar 29, 2010 at 04:09:46PM -0400, Mathieu Desnoyers wrote: > > > * Steven Rostedt (rostedt@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. > > > > Why is this not in .34-rc2? Can you find the specific patch in Linus's > > tree that solves this and let stable@kernel.org know about it? > > > > Looks like it was this commit: > > commit e1783a240f491fb233f04edc042e16b18a7a79ba > Author: Christoph Lameter <cl@linux-foundation.org> > Date: Tue Jan 5 15:34:50 2010 +0900 > module: Use this_cpu_xx to dynamically allocate counters > > Mathieu's fix was: > > - return (local_t *) (mod->refptr + per_cpu_offset(cpu)); > + return (local_t *) per_cpu_ptr(mod->refptr, cpu); > > As Mathieu states in his change log, the bug is that the mod->refptr is > outside the assembly obfuscation of the per_cpu_offset(). This allows > the compiler to optimize and cause a NULL pointer dereference with the > manipulation of per cpu data. > > Christoph Lameter's change fixes this bug as a side effect: > > -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 > -} > - > /* Sometimes we know we already have a refcount, and it's easier not > to handle the error case (which only happens with rmmod --wait). */ > static inline void __module_get(struct module *module) > { > if (module) { > - unsigned int cpu = get_cpu(); > - local_inc(__module_ref_addr(module, cpu)); > + preempt_disable(); > + __this_cpu_inc(module->refptr->count); > trace_module_get(module, _THIS_IP_, > - local_read(__module_ref_addr(module, cpu))); > - put_cpu(); > + __this_cpu_read(module->refptr->count)); > + preempt_enable(); > } > } > > By removing the buggy code all together. Exactly. Steven has beaten me on the start line on this one. ;) Thanks, Mathieu > > -- Steve > > -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [stable] [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-30 2:22 ` Mathieu Desnoyers @ 2010-04-19 18:26 ` Greg KH 2010-04-20 14:38 ` Mathieu Desnoyers 0 siblings, 1 reply; 11+ messages in thread From: Greg KH @ 2010-04-19 18:26 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Steven Rostedt, Randy Dunlap, Greg Kroah-Hartman, Peter Zijlstra, stable, Rusty Russell, linux-kernel, Eric Dumazet, Tejun Heo, Ingo Molnar, Linus Torvalds, Andrew Morton On Mon, Mar 29, 2010 at 10:22:38PM -0400, Mathieu Desnoyers wrote: > * Steven Rostedt (rostedt@goodmis.org) wrote: > > On Mon, 2010-03-29 at 14:07 -0700, Greg KH wrote: > > > On Mon, Mar 29, 2010 at 04:09:46PM -0400, Mathieu Desnoyers wrote: > > > > * Steven Rostedt (rostedt@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. > > > > > > Why is this not in .34-rc2? Can you find the specific patch in Linus's > > > tree that solves this and let stable@kernel.org know about it? > > > > > > > Looks like it was this commit: > > > > commit e1783a240f491fb233f04edc042e16b18a7a79ba > > Author: Christoph Lameter <cl@linux-foundation.org> > > Date: Tue Jan 5 15:34:50 2010 +0900 > > module: Use this_cpu_xx to dynamically allocate counters > > > > Mathieu's fix was: > > > > - return (local_t *) (mod->refptr + per_cpu_offset(cpu)); > > + return (local_t *) per_cpu_ptr(mod->refptr, cpu); > > > > As Mathieu states in his change log, the bug is that the mod->refptr is > > outside the assembly obfuscation of the per_cpu_offset(). This allows > > the compiler to optimize and cause a NULL pointer dereference with the > > manipulation of per cpu data. > > > > Christoph Lameter's change fixes this bug as a side effect: > > > > -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 > > -} > > - > > /* Sometimes we know we already have a refcount, and it's easier not > > to handle the error case (which only happens with rmmod --wait). */ > > static inline void __module_get(struct module *module) > > { > > if (module) { > > - unsigned int cpu = get_cpu(); > > - local_inc(__module_ref_addr(module, cpu)); > > + preempt_disable(); > > + __this_cpu_inc(module->refptr->count); > > trace_module_get(module, _THIS_IP_, > > - local_read(__module_ref_addr(module, cpu))); > > - put_cpu(); > > + __this_cpu_read(module->refptr->count)); > > + preempt_enable(); > > } > > } > > > > By removing the buggy code all together. > > Exactly. Steven has beaten me on the start line on this one. ;) Ok, I'm totally confused now :( What patch should I apply to a stable release, and which stable release? thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [stable] [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-04-19 18:26 ` Greg KH @ 2010-04-20 14:38 ` Mathieu Desnoyers 0 siblings, 0 replies; 11+ messages in thread From: Mathieu Desnoyers @ 2010-04-20 14:38 UTC (permalink / raw) To: Greg KH Cc: Steven Rostedt, Randy Dunlap, Greg Kroah-Hartman, Peter Zijlstra, stable, Rusty Russell, linux-kernel, Eric Dumazet, Tejun Heo, Ingo Molnar, Linus Torvalds, Andrew Morton * Greg KH (greg@kroah.com) wrote: > On Mon, Mar 29, 2010 at 10:22:38PM -0400, Mathieu Desnoyers wrote: > > * Steven Rostedt (rostedt@goodmis.org) wrote: > > > On Mon, 2010-03-29 at 14:07 -0700, Greg KH wrote: > > > > On Mon, Mar 29, 2010 at 04:09:46PM -0400, Mathieu Desnoyers wrote: > > > > > * Steven Rostedt (rostedt@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. > > > > > > > > Why is this not in .34-rc2? Can you find the specific patch in Linus's > > > > tree that solves this and let stable@kernel.org know about it? > > > > > > > > > > Looks like it was this commit: > > > > > > commit e1783a240f491fb233f04edc042e16b18a7a79ba > > > Author: Christoph Lameter <cl@linux-foundation.org> > > > Date: Tue Jan 5 15:34:50 2010 +0900 > > > module: Use this_cpu_xx to dynamically allocate counters > > > > > > Mathieu's fix was: > > > > > > - return (local_t *) (mod->refptr + per_cpu_offset(cpu)); > > > + return (local_t *) per_cpu_ptr(mod->refptr, cpu); > > > > > > As Mathieu states in his change log, the bug is that the mod->refptr is > > > outside the assembly obfuscation of the per_cpu_offset(). This allows > > > the compiler to optimize and cause a NULL pointer dereference with the > > > manipulation of per cpu data. > > > > > > Christoph Lameter's change fixes this bug as a side effect: > > > > > > -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 > > > -} > > > - > > > /* Sometimes we know we already have a refcount, and it's easier not > > > to handle the error case (which only happens with rmmod --wait). */ > > > static inline void __module_get(struct module *module) > > > { > > > if (module) { > > > - unsigned int cpu = get_cpu(); > > > - local_inc(__module_ref_addr(module, cpu)); > > > + preempt_disable(); > > > + __this_cpu_inc(module->refptr->count); > > > trace_module_get(module, _THIS_IP_, > > > - local_read(__module_ref_addr(module, cpu))); > > > - put_cpu(); > > > + __this_cpu_read(module->refptr->count)); > > > + preempt_enable(); > > > } > > > } > > > > > > By removing the buggy code all together. > > > > Exactly. Steven has beaten me on the start line on this one. ;) > > Ok, I'm totally confused now :( > > What patch should I apply to a stable release, and which stable release? > > thanks, > > greg k-h Unless you take all the per cpu refactoring from 2.6.34-rc, the following patch should be taken into -stable. Please see the changelog for the list of stable releases that need it. module: fix __module_ref_addr() __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. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Tested-by: Randy Dunlap <randy.dunlap@oracle.com> CC: Eric Dumazet <dada1@cosmosbay.com> CC: Rusty Russell <rusty@rustcorp.com.au> CC: Peter Zijlstra <a.p.zijlstra@chello.nl> CC: Tejun Heo <tj@kernel.org> CC: Ingo Molnar <mingo@elte.hu> CC: Andrew Morton <akpm@linux-foundation.org> CC: Linus Torvalds <torvalds@linux-foundation.org> CC: Greg Kroah-Hartman <gregkh@suse.de> CC: Steven Rostedt <rostedt@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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [stable] [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-29 21:07 ` [stable] " Greg KH 2010-03-30 1:08 ` Steven Rostedt @ 2010-03-30 2:12 ` Tejun Heo 2010-03-30 2:34 ` Tejun Heo 1 sibling, 1 reply; 11+ messages in thread From: Tejun Heo @ 2010-03-30 2:12 UTC (permalink / raw) To: Greg KH Cc: Mathieu Desnoyers, Steven Rostedt, Randy Dunlap, Greg Kroah-Hartman, Peter Zijlstra, stable, Rusty Russell, linux-kernel, Eric Dumazet, Ingo Molnar, Linus Torvalds, Andrew Morton Hello, Greg. On 03/30/2010 06:07 AM, Greg KH wrote: >> Yes. 2.6.34-rc does not have this issue anymore, but the patch is needed in >> -stable. > > Why is this not in .34-rc2? Can you find the specific patch in Linus's > tree that solves this and let stable@kernel.org know about it? For 2.6.34, the following two patches should remove the problem. They're not in mainline yet but should appear in linux-next soon and after a few days, I'll push them to Linus. http://thread.gmane.org/gmane.linux.kernel/958794/focus=959493 Thanks. -- tejun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [stable] [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-30 2:12 ` Tejun Heo @ 2010-03-30 2:34 ` Tejun Heo 2010-03-30 3:04 ` Mathieu Desnoyers 0 siblings, 1 reply; 11+ messages in thread From: Tejun Heo @ 2010-03-30 2:34 UTC (permalink / raw) To: Greg KH Cc: Mathieu Desnoyers, Steven Rostedt, Randy Dunlap, Greg Kroah-Hartman, Peter Zijlstra, stable, Rusty Russell, linux-kernel, Eric Dumazet, Ingo Molnar, Linus Torvalds, Andrew Morton On 03/30/2010 11:12 AM, Tejun Heo wrote: > Hello, Greg. > > On 03/30/2010 06:07 AM, Greg KH wrote: >>> Yes. 2.6.34-rc does not have this issue anymore, but the patch is needed in >>> -stable. >> >> Why is this not in .34-rc2? Can you find the specific patch in Linus's >> tree that solves this and let stable@kernel.org know about it? > > For 2.6.34, the following two patches should remove the problem. > They're not in mainline yet but should appear in linux-next soon and > after a few days, I'll push them to Linus. > > http://thread.gmane.org/gmane.linux.kernel/958794/focus=959493 Oh, Steven is right. The ones which are removed by the above patch are different ones. That said, it might be a good idea to apply fixes for them to -stable too. Mathieu, are you interested in submitting similar -stable fixes for kernel/module.c::percpu_modcopy() and lockdep.c::static_obj()? Thanks. -- tejun ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [stable] [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() 2010-03-30 2:34 ` Tejun Heo @ 2010-03-30 3:04 ` Mathieu Desnoyers 0 siblings, 0 replies; 11+ messages in thread From: Mathieu Desnoyers @ 2010-03-30 3:04 UTC (permalink / raw) To: Tejun Heo Cc: Greg KH, Steven Rostedt, Randy Dunlap, Greg Kroah-Hartman, Peter Zijlstra, stable, Rusty Russell, linux-kernel, Eric Dumazet, Ingo Molnar, Linus Torvalds, Andrew Morton * Tejun Heo (tj@kernel.org) wrote: > On 03/30/2010 11:12 AM, Tejun Heo wrote: > > Hello, Greg. > > > > On 03/30/2010 06:07 AM, Greg KH wrote: > >>> Yes. 2.6.34-rc does not have this issue anymore, but the patch is needed in > >>> -stable. > >> > >> Why is this not in .34-rc2? Can you find the specific patch in Linus's > >> tree that solves this and let stable@kernel.org know about it? > > > > For 2.6.34, the following two patches should remove the problem. > > They're not in mainline yet but should appear in linux-next soon and > > after a few days, I'll push them to Linus. > > > > http://thread.gmane.org/gmane.linux.kernel/958794/focus=959493 > > Oh, Steven is right. The ones which are removed by the above patch > are different ones. That said, it might be a good idea to apply fixes > for them to -stable too. Mathieu, are you interested in submitting > similar -stable fixes for kernel/module.c::percpu_modcopy() and > lockdep.c::static_obj()? Sure, will do. I'm doing a bit of testing and I'll post these patches shortly. Thanks, Mathieu > > Thanks. > > -- > tejun -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2010-04-20 14:38 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-03-27 14:31 [PATCH 2.6.29.x - 2.6.31.1] module: fix __module_ref_addr() Mathieu Desnoyers 2010-03-29 19:21 ` Steven Rostedt 2010-03-29 20:09 ` Mathieu Desnoyers 2010-03-29 21:07 ` [stable] " Greg KH 2010-03-30 1:08 ` Steven Rostedt 2010-03-30 2:22 ` Mathieu Desnoyers 2010-04-19 18:26 ` Greg KH 2010-04-20 14:38 ` Mathieu Desnoyers 2010-03-30 2:12 ` Tejun Heo 2010-03-30 2:34 ` Tejun Heo 2010-03-30 3:04 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).