Skip to contentSkip to conversation

possible recursive locking in udp4_lib_rcv

DISCUSSION Applied Michael S. Tsirkin 4 participants 7 messages Aug 7, 2008 – Aug 9, 2008

Subsystems (3)
Networking Network drivers UDP
lore
Share & tools
lore

Michael S. Tsirkin

author
#0 lore raw

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?

Signature · 1 line
MST
#1 lore raw
Quoted 4 lines · Michael S. Tsirkin

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 <[email protected]>
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,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#2 lore raw
Quoted 14 lines · Herbert Xu
(quoted diff) +7 -1
@@ -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);
Quoted 1 line

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,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#3 lore raw
Quoted 4 lines · Herbert Xu

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 <[email protected]>
net/ipv4/udp.c +4 -2
index 383d173..8e42fbb 100644
@@ -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
net/ipv6/udp.c +3 -3
index d1477b3..a6aecf7 100644
@@ -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,

Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#4 lore raw
Quoted 7 lines · Herbert Xu
Quoted 4 lines · Herbert Xu

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?

#5 lore raw
In reply to Herbert Xu

Hello Herbert,

Quoted 8 lines · Herbert Xu
Quoted 3 lines · Herbert Xu

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

Signature · 1 line
Hitachi Computer Products (America) Inc.
maintainer Applied
#6 lore raw

From: Hideo AOKI <[email protected]>
Date: Fri, 08 Aug 2008 23:50:17 -0400

Quoted 8 lines

Hello Herbert,

Quoted 5 lines · Herbert Xu
Quoted 3 lines · Herbert Xu

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.

Quoted 5 lines
Quoted 3 lines

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.