Patchwork net-next/unix: BUG: using smp_processor_id() in preemptible

login
register
mail settings
Submitter Eric Dumazet
Date Nov. 23, 2008, 3:32 a.m.
Message ID <4928CECE.602@cosmosbay.com>
Download mbox | patch
Permalink /patch/10276/
State Rejected
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Nov. 23, 2008, 3:32 a.m.
Ilpo Järvinen a écrit :
> I got plenty of these from sock_prot_inuse_add:
> 
> BUG: using smp_processor_id() in preemptible [00000000] code: rcS/1146
> 
> caller is sock_prot_inuse_add+0x24/0x42
> Pid: 1146, comm: rcS Not tainted 2.6.28-rc6-01121-g89a2f15 #76
> Call Trace:
>  [<ffffffff80365767>] debug_smp_processor_id+0xd3/0xe8
>  [<ffffffff80485a0f>] sock_prot_inuse_add+0x24/0x42
>  [<ffffffff80514cdd>] unix_create1+0x161/0x176
>  [<ffffffff80514d52>] unix_create+0x60/0x6b
>  [<ffffffff80483f66>] __sock_create+0x144/0x1bd
>  [<ffffffff80483edb>] ? __sock_create+0xb9/0x1bd
>  [<ffffffff8048402d>] sock_create+0x2d/0x2f
>  [<ffffffff80484274>] sys_socket+0x29/0x5b
>  [<ffffffff8020c10a>] system_call_fastpath+0x16/0x1b
> 
> 

Thanks Ilpo for this report

I guess the following is necessary.

Once Christopher and Rusty work on percpu variables is finished,
the preempt_enable()/disable() wont be necessary anymore, so
its a temporary workaround anyway.

[PATCH] net: make sock_prot_inuse_add() preempt safe

Ilpo Järvinen reported that commit a8076d8db98de6da61394b2e942320e4612643ac
(net: af_unix should update its inuse counter) was triggering
a warning in smp_processor_id(), being called in a preemptible code.

Fix is to make sock_prot_inuse_add() safe in this regard. This fix
can be reverted when new percpu infrastructure is ready, allowing
a cpu to safely do a increment/decrement on a percpu var.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
Ilpo Järvinen - Nov. 23, 2008, 9:40 p.m.
On Sun, 23 Nov 2008, Eric Dumazet wrote:

> Ilpo Järvinen a écrit :
> > I got plenty of these from sock_prot_inuse_add:
> > 
> > BUG: using smp_processor_id() in preemptible [00000000] code: rcS/1146
> > 
> > caller is sock_prot_inuse_add+0x24/0x42
> > Pid: 1146, comm: rcS Not tainted 2.6.28-rc6-01121-g89a2f15 #76
> > Call Trace:
> >  [<ffffffff80365767>] debug_smp_processor_id+0xd3/0xe8
> >  [<ffffffff80485a0f>] sock_prot_inuse_add+0x24/0x42
> >  [<ffffffff80514cdd>] unix_create1+0x161/0x176
> >  [<ffffffff80514d52>] unix_create+0x60/0x6b
> >  [<ffffffff80483f66>] __sock_create+0x144/0x1bd
> >  [<ffffffff80483edb>] ? __sock_create+0xb9/0x1bd
> >  [<ffffffff8048402d>] sock_create+0x2d/0x2f
> >  [<ffffffff80484274>] sys_socket+0x29/0x5b
> >  [<ffffffff8020c10a>] system_call_fastpath+0x16/0x1b
> > 
> > 
> 
> Thanks Ilpo for this report
> 
> I guess the following is necessary.
> 
> Once Christopher and Rusty work on percpu variables is finished,
> the preempt_enable()/disable() wont be necessary anymore, so
> its a temporary workaround anyway.
> 
> [PATCH] net: make sock_prot_inuse_add() preempt safe
> 
> Ilpo Järvinen reported that commit a8076d8db98de6da61394b2e942320e4612643ac
> (net: af_unix should update its inuse counter) was triggering
> a warning in smp_processor_id(), being called in a preemptible code.
> 
> Fix is to make sock_prot_inuse_add() safe in this regard. This fix
> can be reverted when new percpu infrastructure is ready, allowing
> a cpu to safely do a increment/decrement on a percpu var.
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

Works.

Tested-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
David Miller - Nov. 24, 2008, 1:20 a.m.
From: Eric Dumazet <dada1@cosmosbay.com>
Date: Sun, 23 Nov 2008 04:32:30 +0100

> [PATCH] net: make sock_prot_inuse_add() preempt safe
> 
> Ilpo Järvinen reported that commit a8076d8db98de6da61394b2e942320e4612643ac
> (net: af_unix should update its inuse counter) was triggering
> a warning in smp_processor_id(), being called in a preemptible code.
> 
> Fix is to make sock_prot_inuse_add() safe in this regard. This fix
> can be reverted when new percpu infrastructure is ready, allowing
> a cpu to safely do a increment/decrement on a percpu var.
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

Eric, you added this bug by starting to use this interface in
situations where BH's were not disabled.

Ever existing use adhered to that rule.

If you therefore want to call this interface in new locations,
you have to make sure those locations follow the rule too.

Making it expensive for all the existing cases which were already
safe, is not the way to fix this.  And saying some future not-merged
change justifies this added cost is besides the point.

What is valid is getting rid of the BH disables you need to add
to net/unix/af_unix.c et al.'s call sites, once those percpu patches
are added.  Not the other way around.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/core/sock.c b/net/core/sock.c
index a4e840e..d4f5ad7 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1942,8 +1942,8 @@  static DECLARE_BITMAP(proto_inuse_idx, PROTO_INUSE_NR);
 #ifdef CONFIG_NET_NS
 void sock_prot_inuse_add(struct net *net, struct proto *prot, int val)
 {
-	int cpu = smp_processor_id();
-	per_cpu_ptr(net->core.inuse, cpu)->val[prot->inuse_idx] += val;
+	per_cpu_ptr(net->core.inuse, get_cpu())->val[prot->inuse_idx] += val;
+	put_cpu();
 }
 EXPORT_SYMBOL_GPL(sock_prot_inuse_add);
 
@@ -1989,7 +1989,9 @@  static DEFINE_PER_CPU(struct prot_inuse, prot_inuse);
 
 void sock_prot_inuse_add(struct net *net, struct proto *prot, int val)
 {
+	preempt_disable();
 	__get_cpu_var(prot_inuse).val[prot->inuse_idx] += val;
+	preempt_enable();
 }
 EXPORT_SYMBOL_GPL(sock_prot_inuse_add);