possible recursive locking in udp4_lib_rcv

possible recursive locking in udp4_lib_rcv

[Michael S. Tsirkin]

Hi!
I noticed the following warnings when running on 2.6.27-rc2
with lockdep checker enabled:

[ 2912.004106] Initializing XFRM netlink socket
[ 2922.009629]
[ 2922.009632] =============================================
[ 2922.009640] [ INFO: possible recursive locking detected ]
[ 2922.009643] 2.6.27-rc2-mst-suspend #32
[ 2922.009645] ---------------------------------------------
[ 2922.009648] Xorg/5958 is trying to acquire lock:
[ 2922.009650]  (slock-AF_INET/1){-+..}, at: [<c02f105a>] __udp4_lib_rcv+0x34a/0x920
[ 2922.009660]
[ 2922.009661] but task is already holding lock:
[ 2922.009663]  (slock-AF_INET/1){-+..}, at: [<c02f105a>] __udp4_lib_rcv+0x34a/0x920
[ 2922.009669]
[ 2922.009669] other info that might help us debug this:
[ 2922.009672] 4 locks held by Xorg/5958:
[ 2922.009674]  #0:  (rcu_read_lock){..--}, at: [<c02af476>] netif_receive_skb+0x66/0x390
[ 2922.009681]  #1:  (rcu_read_lock){..--}, at: [<c02d1cc0>] ip_local_deliver_finish+0x30/0x1f0
[ 2922.009688]  #2:  (slock-AF_INET/1){-+..}, at: [<c02f105a>] __udp4_lib_rcv+0x34a/0x920
[ 2922.009694]  #3:  (rcu_read_lock){..--}, at: [<c02d1cc0>] ip_local_deliver_finish+0x30/0x1f0
[ 2922.009700]
 2922.009701] stack backtrace:
[ 2922.009704] Pid: 5958, comm: Xorg Not tainted 2.6.27-rc2-mst-suspend #32
[ 2922.009707]  [<c014acc6>] __lock_acquire+0xee6/0x1130
[ 2922.009713]  [<c014af71>] lock_acquire+0x61/0x80
[ 2922.009717]  [<c02f105a>] ? __udp4_lib_rcv+0x34a/0x920
[ 2922.009721]  [<c033008a>] _spin_lock_nested+0x2a/0x40
[ 2922.009726]  [<c02f105a>] ? __udp4_lib_rcv+0x34a/0x920
[ 2922.009731]  [<c02f105a>] __udp4_lib_rcv+0x34a/0x920
[ 2922.009738]  [<f8c81360>] ? ipv4_confirm+0x0/0xe0 [nf_conntrack_ipv4]
[ 2922.009746]  [<c02cb549>] ? nf_iterate+0x59/0x80
[ 2922.009751]  [<c02f1642>] udp_rcv+0x12/0x20
[ 2922.009755]  [<c02d1d3a>] ip_local_deliver_finish+0xaa/0x1f0
[ 2922.009758]  [<c02d1cc0>] ? ip_local_deliver_finish+0x30/0x1f0
[ 2922.009763]  [<c02d2250>] ip_local_deliver+0x30/0xa0
[ 2922.009766]  [<c02d1c90>] ? ip_local_deliver_finish+0x0/0x1f0
[ 2922.009770]  [<c030943b>] xfrm4_transport_finish+0x6b/0xf0
[ 2922.009776]  [<c0309170>] ? xfrm4_rcv_encap_finish+0x0/0x60
[ 2922.009780]  [<c031099d>] xfrm_input+0x22d/0x360
[ 2922.009784]  [<c03091f3>] xfrm4_rcv_encap+0x23/0x30
[ 2922.009788]  [<c030939b>] xfrm4_udp_encap_rcv+0x16b/0x1a0
[ 2922.009792]  [<c02f0bc4>] udp_queue_rcv_skb+0x174/0x2c0
[ 2922.009795]  [<c0330091>] ? _spin_lock_nested+0x31/0x40
[ 2922.009800]  [<c02f148e>] __udp4_lib_rcv+0x77e/0x920
[ 2922.009805]  [<f8c81360>] ? ipv4_confirm+0x0/0xe0 [nf_conntrack_ipv4]
[ 2922.009811]  [<c02cb549>] ? nf_iterate+0x59/0x80
[ 2922.009816]  [<c02f1642>] udp_rcv+0x12/0x20
[ 2922.009819]  [<c02d1d3a>] ip_local_deliver_finish+0xaa/0x1f0
[ 2922.009822]  [<c02d1cc0>] ? ip_local_deliver_finish+0x30/0x1f0
[ 2922.009827]  [<c02d2250>] ip_local_deliver+0x30/0xa0
[ 2922.009830]  [<c02d1c90>] ? ip_local_deliver_finish+0x0/0x1f0
[ 2922.009834]  [<c02d1a3e>] ip_rcv_finish+0xfe/0x350
[ 2922.009838]  [<c02cb719>] ? nf_hook_slow+0xd9/0x100
[ 2922.009842]  [<c02d1940>] ? ip_rcv_finish+0x0/0x350
[ 2922.009846]  [<c02d2136>] ip_rcv+0x1a6/0x290
[ 2922.009849]  [<c02d1940>] ? ip_rcv_finish+0x0/0x350
[ 2922.009853]  [<c02d1f90>] ? ip_rcv+0x0/0x290
[ 2922.009857]  [<c02af638>] netif_receive_skb+0x228/0x390
[ 2922.009860]  [<c02af476>] ? netif_receive_skb+0x66/0x390
[ 2922.009865]  [<f8cb9550>] ? packet_rcv_spkt+0x0/0x110 [af_packet]
[ 2922.009872]  [<c02b1fcc>] process_backlog+0x7c/0xe0
[ 2922.009876]  [<c02b1887>] net_rx_action+0xa7/0x150
[ 2922.009879]  [<c012ca47>] __do_softirq+0x87/0x100
[ 2922.009883]  [<c012cb17>] do_softirq+0x57/0x60
[ 2922.009887]  [<c012cea7>] irq_exit+0x77/0x90
[ 2922.009890]  [<c01060c5>] do_IRQ+0x45/0x80
[ 2922.009894]  [<c01498f4>] ? trace_hardirqs_on_caller+0xc4/0x150
[ 2922.009899]  [<c010463c>] common_interrupt+0x28/0x30
[ 2922.009903]  =======================
[ 2922.277171] PPP generic driver version 2.4.2

(I'm not sure what I did, something related to ipsec and/or ppp).

I have not seen this with v2.6.26 or earlier.
Any idea whether this is a real bug or lockdep false positive?

-- 
MST

Re: possible recursive locking in udp4_lib_rcv

[Herbert Xu]

Michael S. Tsirkin <m.s.tsirkin@gmail.com> wrote:
> I noticed the following warnings when running on 2.6.27-rc2
> with lockdep checker enabled:
> 
> [ 2912.004106] Initializing XFRM netlink socket

Argh, this was added by

commit 95766fff6b9a78d11fc2d3812dd035381690b55d
Author: Hideo Aoki <haoki@redhat.com>
Date:   Mon Dec 31 00:29:24 2007 -0800

    [UDP]: Add memory accounting.

@@ -1165,7 +1189,13 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct hlist_head udptable[],
                               inet_iif(skb), udptable);

        if (sk != NULL) {
-               int ret = udp_queue_rcv_skb(sk, skb);
+               int ret = 0;
+               bh_lock_sock_nested(sk);
+               if (!sock_owned_by_user(sk))
+                       ret = udp_queue_rcv_skb(sk, skb);
+               else
+                       sk_add_backlog(sk, skb);
+               bh_unlock_sock(sk);

Hmm, what was the reason for that lock?

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: possible recursive locking in udp4_lib_rcv

[Herbert Xu]

On Fri, Aug 08, 2008 at 10:28:41PM +0800, Herbert Xu wrote:
>
> @@ -1165,7 +1189,13 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct hlist_head udptable[],
>                                inet_iif(skb), udptable);
> 
>         if (sk != NULL) {
> -               int ret = udp_queue_rcv_skb(sk, skb);
> +               int ret = 0;
> +               bh_lock_sock_nested(sk);
> +               if (!sock_owned_by_user(sk))
> +                       ret = udp_queue_rcv_skb(sk, skb);
> +               else
> +                       sk_add_backlog(sk, skb);
> +               bh_unlock_sock(sk);
> 
> Hmm, what was the reason for that lock?

Never mind, I see the reason why it's there.  What we should do
is drop the lock when we hit an encapsulated socket since they
don't need it at all.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: possible recursive locking in udp4_lib_rcv

[Herbert Xu]

On Fri, Aug 08, 2008 at 10:37:41PM +0800, Herbert Xu wrote:
>
> Never mind, I see the reason why it's there.  What we should do
> is drop the lock when we hit an encapsulated socket since they
> don't need it at all.

Here is the patch:

udp: Drop socket lock for encapsulated packets

The socket lock is there to protect the normal UDP receive path.
Encapsulation UDP sockets don't need that protection.  In fact
the locking is deadly for them as they may contain another UDP
packet within, possibly with the same addresses.

Also the nested bit was copied from TCP.  TCP needs it because
of accept(2) spawning sockets.  This simply doesn't apply to UDP
so I've removed it.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 383d173..8e42fbb 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -989,7 +989,9 @@ int udp_queue_rcv_skb(struct sock * sk, struct sk_buff *skb)
 		    up->encap_rcv != NULL) {
 			int ret;
 
+			bh_unlock_sock(sk);
 			ret = (*up->encap_rcv)(sk, skb);
+			bh_lock_sock(sk);
 			if (ret <= 0) {
 				UDP_INC_STATS_BH(sock_net(sk),
 						 UDP_MIB_INDATAGRAMS,
@@ -1092,7 +1094,7 @@ static int __udp4_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 			if (skb1) {
 				int ret = 0;
 
-				bh_lock_sock_nested(sk);
+				bh_lock_sock(sk);
 				if (!sock_owned_by_user(sk))
 					ret = udp_queue_rcv_skb(sk, skb1);
 				else
@@ -1194,7 +1196,7 @@ int __udp4_lib_rcv(struct sk_buff *skb, struct hlist_head udptable[],
 
 	if (sk != NULL) {
 		int ret = 0;
-		bh_lock_sock_nested(sk);
+		bh_lock_sock(sk);
 		if (!sock_owned_by_user(sk))
 			ret = udp_queue_rcv_skb(sk, skb);
 		else
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index d1477b3..a6aecf7 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -379,7 +379,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 					uh->source, saddr, dif))) {
 		struct sk_buff *buff = skb_clone(skb, GFP_ATOMIC);
 		if (buff) {
-			bh_lock_sock_nested(sk2);
+			bh_lock_sock(sk2);
 			if (!sock_owned_by_user(sk2))
 				udpv6_queue_rcv_skb(sk2, buff);
 			else
@@ -387,7 +387,7 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 			bh_unlock_sock(sk2);
 		}
 	}
-	bh_lock_sock_nested(sk);
+	bh_lock_sock(sk);
 	if (!sock_owned_by_user(sk))
 		udpv6_queue_rcv_skb(sk, skb);
 	else
@@ -508,7 +508,7 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct hlist_head udptable[],
 
 	/* deliver */
 
-	bh_lock_sock_nested(sk);
+	bh_lock_sock(sk);
 	if (!sock_owned_by_user(sk))
 		udpv6_queue_rcv_skb(sk, skb);
 	else

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: possible recursive locking in udp4_lib_rcv

[Michael S. Tsirkin]

On Fri, Aug 08, 2008 at 11:01:08PM +0800, Herbert Xu wrote:
> On Fri, Aug 08, 2008 at 10:37:41PM +0800, Herbert Xu wrote:
> >
> > Never mind, I see the reason why it's there.  What we should do
> > is drop the lock when we hit an encapsulated socket since they
> > don't need it at all.
> 
> Here is the patch:

Nice. Open a bugzilla entry for this?

Re: possible recursive locking in udp4_lib_rcv

[Hideo AOKI]

Hello Herbert,

Herbert Xu wrote:
> On Fri, Aug 08, 2008 at 10:37:41PM +0800, Herbert Xu wrote:
>> Never mind, I see the reason why it's there.  What we should do
>> is drop the lock when we hit an encapsulated socket since they
>> don't need it at all.
> 
> Here is the patch:
> 
> udp: Drop socket lock for encapsulated packets

Thank very much for fixing the bug.

Best regards,
Hideo

-- 
Hitachi Computer Products (America) Inc.

Re: possible recursive locking in udp4_lib_rcv

[David Miller]

From: Hideo AOKI <haoki@redhat.com>
Date: Fri, 08 Aug 2008 23:50:17 -0400

> Hello Herbert,
> 
> Herbert Xu wrote:
> > On Fri, Aug 08, 2008 at 10:37:41PM +0800, Herbert Xu wrote:
> >> Never mind, I see the reason why it's there.  What we should do
> >> is drop the lock when we hit an encapsulated socket since they
> >> don't need it at all.
> > 
> > Here is the patch:
> > 
> > udp: Drop socket lock for encapsulated packets
> 
> Thank very much for fixing the bug.

Indeed, thanks Herbert.

Applied to net-2.6, and queued up for -stable.