[PATCH -stable] modules fix incorrect percpu usage

[PATCH -stable] modules fix incorrect percpu usage

[Mathieu Desnoyers]

Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
for per cpu pointers).

Introduced by commit:

module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3

This patch should be queued for the stable branch, for kernels 2.6.29.x to
2.6.33.x.
(tested on 2.6.33.1 x86_64)

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: 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>
CC: stable <stable@kernel.org>
---
 kernel/module.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-lttng/kernel/module.c
===================================================================
--- linux-2.6-lttng.orig/kernel/module.c	2010-03-29 23:47:05.000000000 -0400
+++ linux-2.6-lttng/kernel/module.c	2010-03-29 23:47:27.000000000 -0400
@@ -412,7 +412,7 @@ static void percpu_modcopy(void *pcpudes
 	int cpu;
 
 	for_each_possible_cpu(cpu)
-		memcpy(pcpudest + per_cpu_offset(cpu), from, size);
+		memcpy(per_cpu_ptr(pcpudest, cpu), from, size);
 }
 
 #else /* ... !CONFIG_SMP */

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH] modules fix incorrect percpu usage

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> for per cpu pointers).
> 
> Introduced by commit:
> 
> module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> 
> This patch should be queued for the stable branch, for kernels 2.6.29.x to
> 2.6.33.x.
> (tested on 2.6.33.1 x86_64)

It's actually needed for mainline too (2.6.34-rc2).

Thanks,

Mathieu

> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: 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>
> CC: stable <stable@kernel.org>
> ---
>  kernel/module.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6-lttng/kernel/module.c
> ===================================================================
> --- linux-2.6-lttng.orig/kernel/module.c	2010-03-29 23:47:05.000000000 -0400
> +++ linux-2.6-lttng/kernel/module.c	2010-03-29 23:47:27.000000000 -0400
> @@ -412,7 +412,7 @@ static void percpu_modcopy(void *pcpudes
>  	int cpu;
>  
>  	for_each_possible_cpu(cpu)
> -		memcpy(pcpudest + per_cpu_offset(cpu), from, size);
> +		memcpy(per_cpu_ptr(pcpudest, cpu), from, size);
>  }
>  
>  #else /* ... !CONFIG_SMP */
> 
> -- 
> 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

Re: [PATCH] modules fix incorrect percpu usage

[Steven Rostedt]

On Mon, 2010-03-29 at 23:36 -0400, Mathieu Desnoyers wrote:
> * Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> > Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> > for per cpu pointers).
> > 
> > Introduced by commit:
> > 
> > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> > 
> > This patch should be queued for the stable branch, for kernels 2.6.29.x to
> > 2.6.33.x.
> > (tested on 2.6.33.1 x86_64)
> 
> It's actually needed for mainline too (2.6.34-rc2).

Then all you need to do is submit it for mainline with a 
"Cc: stable@kernel.org" as you did below. It will then automatically be
added to the stable queue when Linus pulls it.

-- Steve

> 
> Thanks,
> 
> Mathieu
> 
> > 
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > CC: 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>
> > CC: stable <stable@kernel.org>
> > ---
> >  kernel/module.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 

Re: [PATCH] modules fix incorrect percpu usage

[Mathieu Desnoyers]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Mon, 2010-03-29 at 23:36 -0400, Mathieu Desnoyers wrote:
> > * Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> > > Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> > > for per cpu pointers).
> > > 
> > > Introduced by commit:
> > > 
> > > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> > > 
> > > This patch should be queued for the stable branch, for kernels 2.6.29.x to
> > > 2.6.33.x.
> > > (tested on 2.6.33.1 x86_64)
> > 
> > It's actually needed for mainline too (2.6.34-rc2).
> 
> Then all you need to do is submit it for mainline with a 
> "Cc: stable@kernel.org" as you did below. It will then automatically be
> added to the stable queue when Linus pulls it.

OK, thanks for the info.

Mathieu

> 
> -- Steve
> 
> > 
> > Thanks,
> > 
> > Mathieu
> > 
> > > 
> > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > > CC: 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>
> > > CC: stable <stable@kernel.org>
> > > ---
> > >  kernel/module.c |    2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [stable] [PATCH] modules fix incorrect percpu usage

[Greg KH]

On Tue, Mar 30, 2010 at 09:50:47AM -0400, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> > On Mon, 2010-03-29 at 23:36 -0400, Mathieu Desnoyers wrote:
> > > * Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> > > > Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> > > > for per cpu pointers).
> > > > 
> > > > Introduced by commit:
> > > > 
> > > > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> > > > 
> > > > This patch should be queued for the stable branch, for kernels 2.6.29.x to
> > > > 2.6.33.x.
> > > > (tested on 2.6.33.1 x86_64)
> > > 
> > > It's actually needed for mainline too (2.6.34-rc2).
> > 
> > Then all you need to do is submit it for mainline with a 
> > "Cc: stable@kernel.org" as you did below. It will then automatically be
> > added to the stable queue when Linus pulls it.
> 
> OK, thanks for the info.

And I need to wait until it goes into Linus's tree before it can be
applied to the stable tree.  This hasn't happened yet, right?

thanks,

greg k-h

Re: [stable] [PATCH] modules fix incorrect percpu usage

[Mathieu Desnoyers]

* Greg KH (greg@kroah.com) wrote:
> On Tue, Mar 30, 2010 at 09:50:47AM -0400, Mathieu Desnoyers wrote:
> > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > On Mon, 2010-03-29 at 23:36 -0400, Mathieu Desnoyers wrote:
> > > > * Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> > > > > Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> > > > > for per cpu pointers).
> > > > > 
> > > > > Introduced by commit:
> > > > > 
> > > > > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> > > > > 
> > > > > This patch should be queued for the stable branch, for kernels 2.6.29.x to
> > > > > 2.6.33.x.
> > > > > (tested on 2.6.33.1 x86_64)
> > > > 
> > > > It's actually needed for mainline too (2.6.34-rc2).
> > > 
> > > Then all you need to do is submit it for mainline with a 
> > > "Cc: stable@kernel.org" as you did below. It will then automatically be
> > > added to the stable queue when Linus pulls it.
> > 
> > OK, thanks for the info.
> 
> And I need to wait until it goes into Linus's tree before it can be
> applied to the stable tree.  This hasn't happened yet, right?
> 
> thanks,
> 
> greg k-h

Same here, for module.c fix. Feel free to choose between this fix or the
corresponding refactoring from Tejun.


modules fix incorrect percpu usage

Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
for per cpu pointers).

Introduced by commit:

module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3

This patch should be queued for the stable branch, for kernels 2.6.29.x to
2.6.33.x.  (tested on 2.6.33.1 x86_64)

Mainline does not need this fix, as commit
259354deaaf03d49a02dbb9975d6ec2a54675672 fixed the problem by refactoring.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: 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>
CC: stable <stable@kernel.org>
---
 kernel/module.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux.trees.git/kernel/module.c
===================================================================
--- linux.trees.git.orig/kernel/module.c	2010-03-27 11:27:23.000000000 -0400
+++ linux.trees.git/kernel/module.c	2010-03-30 09:48:41.000000000 -0400
@@ -405,7 +405,7 @@ static void percpu_modcopy(void *pcpudes
 	int cpu;
 
 	for_each_possible_cpu(cpu)
-		memcpy(pcpudest + per_cpu_offset(cpu), from, size);
+		memcpy(per_cpu_ptr(pcpudest, cpu), from, size);
 }
 
 #else /* ... !CONFIG_SMP */


-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [stable] [PATCH] modules fix incorrect percpu usage

[Tejun Heo]

Hello,

On 04/20/2010 04:34 PM, Mathieu Desnoyers wrote:
> Same here, for module.c fix. Feel free to choose between this fix or the
> corresponding refactoring from Tejun.

Please take this one for -stable.  I don't think the percpu
refactoring is worthwhile to backport to -stable.  It only affects
debug feature and the worst outcome is spurious warning messages.

Thanks.

-- 
tejun

Re: [stable] [PATCH] modules fix incorrect percpu usage

[Greg KH]

On Tue, Apr 20, 2010 at 10:34:57AM -0400, Mathieu Desnoyers wrote:
> * Greg KH (greg@kroah.com) wrote:
> > On Tue, Mar 30, 2010 at 09:50:47AM -0400, Mathieu Desnoyers wrote:
> > > * Steven Rostedt (rostedt@goodmis.org) wrote:
> > > > On Mon, 2010-03-29 at 23:36 -0400, Mathieu Desnoyers wrote:
> > > > > * Mathieu Desnoyers (mathieu.desnoyers@efficios.com) wrote:
> > > > > > Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> > > > > > for per cpu pointers).
> > > > > > 
> > > > > > Introduced by commit:
> > > > > > 
> > > > > > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> > > > > > 
> > > > > > This patch should be queued for the stable branch, for kernels 2.6.29.x to
> > > > > > 2.6.33.x.
> > > > > > (tested on 2.6.33.1 x86_64)
> > > > > 
> > > > > It's actually needed for mainline too (2.6.34-rc2).
> > > > 
> > > > Then all you need to do is submit it for mainline with a 
> > > > "Cc: stable@kernel.org" as you did below. It will then automatically be
> > > > added to the stable queue when Linus pulls it.
> > > 
> > > OK, thanks for the info.
> > 
> > And I need to wait until it goes into Linus's tree before it can be
> > applied to the stable tree.  This hasn't happened yet, right?
> > 
> > thanks,
> > 
> > greg k-h
> 
> Same here, for module.c fix. Feel free to choose between this fix or the
> corresponding refactoring from Tejun.

Thanks, I took this one.

greg k-h

Re: [PATCH] modules fix incorrect percpu usage

[Tejun Heo]

On 03/31/2010 11:03 AM, Steven Rostedt wrote:
> I don't know. A possible "NULL pointer dereference" seems to me to be a
> pretty big user visible impact.
> 
> I guess the question is, what's the risk of adding this change?

AFAICS, the risk is fairly low.  per_cpu_ptr(pcpudest, cpu) is
SHIFT_PERCPU_PTR((ptr), per_cpu_offset((cpu))) which is just a fancy
way of saying "typeof(ptr)((unsigned long)(ptr) + per_cpu_offset(cpu))"
with enough obfuscation to prevent gcc from optimizing it incorrectly.

Thanks.

-- 
tejun

Re: [PATCH] modules fix incorrect percpu usage

[Steven Rostedt]

On Tue, 2010-03-30 at 16:24 -0400, Mathieu Desnoyers wrote:

> > Why do you beleive this should be backported to -stable?  What are the
> > user-visible effects of this change?
> > 
> 
> As for the user-visible impact of this specific patch, I guess nobody noticed
> any problem because we've been lucky enough that the compiler did not generate
> the inappropriate optimization pattern there.
> 
> This inappropriate use of per_cpu_ptr() elsewhere (in __module_ref_addr() from
> module.h) caused a NULL pointer exception on Randy's machine.
> 
> So either we consider that the code is better left untouched, or we apply this
> patch to module.c in order to prevent compiler optimizations from subtly
> breaking the generated assembly with specific configurations of the current or
> future versions of the compiler. At that level, it becomes a policy question
> about what should go in -stable, for which I will defer to Greg and you. I would
> perfectly understand if you consider that it does not belong to -stable, because
> there is no perceived user impact so far.

I don't know. A possible "NULL pointer dereference" seems to me to be a
pretty big user visible impact.

I guess the question is, what's the risk of adding this change?

-- Steve

Re: [PATCH] modules fix incorrect percpu usage

[Tejun Heo]

On 03/30/2010 10:52 PM, Mathieu Desnoyers wrote:
> Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> for per cpu pointers).
> 
> Introduced by commit:
> 
> module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> 
> It applies to mainline as of 2.6.34-rc2. This patch should be queued for the
> stable branch, for kernels 2.6.29.x to 2.6.33.x.
> (based on 2.6.33.1, also applies to 2.6.34-rc2 -tip)
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: 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>
> CC: stable <stable@kernel.org>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] modules fix incorrect percpu usage

[Mathieu Desnoyers]

* Andrew Morton (akpm@linux-foundation.org) wrote:
> On Tue, 30 Mar 2010 09:52:08 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> > Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> > for per cpu pointers).
> > 
> > Introduced by commit:
> > 
> > module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> > 
> > It applies to mainline as of 2.6.34-rc2. This patch should be queued for the
> > stable branch, for kernels 2.6.29.x to 2.6.33.x.
> > (based on 2.6.33.1, also applies to 2.6.34-rc2 -tip)
> 
> Why do you beleive this should be backported to -stable?  What are the
> user-visible effects of this change?
> 

As for the user-visible impact of this specific patch, I guess nobody noticed
any problem because we've been lucky enough that the compiler did not generate
the inappropriate optimization pattern there.

This inappropriate use of per_cpu_ptr() elsewhere (in __module_ref_addr() from
module.h) caused a NULL pointer exception on Randy's machine.

So either we consider that the code is better left untouched, or we apply this
patch to module.c in order to prevent compiler optimizations from subtly
breaking the generated assembly with specific configurations of the current or
future versions of the compiler. At that level, it becomes a policy question
about what should go in -stable, for which I will defer to Greg and you. I would
perfectly understand if you consider that it does not belong to -stable, because
there is no perceived user impact so far.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH] modules fix incorrect percpu usage

[Andrew Morton]

On Tue, 30 Mar 2010 09:52:08 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
> for per cpu pointers).
> 
> Introduced by commit:
> 
> module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3
> 
> It applies to mainline as of 2.6.34-rc2. This patch should be queued for the
> stable branch, for kernels 2.6.29.x to 2.6.33.x.
> (based on 2.6.33.1, also applies to 2.6.34-rc2 -tip)

Why do you beleive this should be backported to -stable?  What are the
user-visible effects of this change?

[PATCH] modules fix incorrect percpu usage

[Mathieu Desnoyers]

Should use per_cpu_ptr() to obfuscate the per cpu pointers (RELOC_HIDE is needed
for per cpu pointers).

Introduced by commit:

module.c: commit 6b588c18f8dacfa6d7957c33c5ff832096e752d3

It applies to mainline as of 2.6.34-rc2. This patch should be queued for the
stable branch, for kernels 2.6.29.x to 2.6.33.x.
(based on 2.6.33.1, also applies to 2.6.34-rc2 -tip)

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: 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>
CC: stable <stable@kernel.org>
---
 kernel/module.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6-lttng/kernel/module.c
===================================================================
--- linux-2.6-lttng.orig/kernel/module.c	2010-03-29 23:47:05.000000000 -0400
+++ linux-2.6-lttng/kernel/module.c	2010-03-29 23:47:27.000000000 -0400
@@ -412,7 +412,7 @@ static void percpu_modcopy(void *pcpudes
 	int cpu;
 
 	for_each_possible_cpu(cpu)
-		memcpy(pcpudest + per_cpu_offset(cpu), from, size);
+		memcpy(per_cpu_ptr(pcpudest, cpu), from, size);
 }
 
 #else /* ... !CONFIG_SMP */
-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com