possible recursive locking in udp4_lib_rcv
Subsystems (3)
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
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.txtQuoted 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.txtQuoted 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.
net/ipv4/udp.c +4 -2
@@ -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
@@ -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.txtQuoted 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?
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.
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.