[IPSEC] xfrm_state locking problem in xfrm_input.c

[IPSEC] xfrm_state locking problem in xfrm_input.c

[Yury Polyanskiy]

[-- Attachment #1: Type: text/plain, Size: 1917 bytes --]

xfrm_input() code uses spin_lock() where it must be using
spin_lock_bh() instead. The corresponding code in xfrm_output.c
correctly uses spin_lock_bh(). 

Note that if the locally generated packet is sent to a local ip,
dev_queue_xmit() calls loopback_xmit() and the xfrm_input() will be
called with softirqs enabled.

This might lead to a deadlock since x->lock is acquired in
xfrm_timer_handler().

Signed-off-by: Yury Polyanskiy <polyanskiy@gmail.com>
---
 net/xfrm/xfrm_input.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index b4a1317..4494918 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -161,7 +161,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 		skb->sp->xvec[skb->sp->len++] = x;
 
-		spin_lock(&x->lock);
+		spin_lock_bh(&x->lock);
 		if (unlikely(x->km.state != XFRM_STATE_VALID)) {
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEINVALID);
 			goto drop_unlock;
@@ -182,7 +182,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			goto drop_unlock;
 		}
 
-		spin_unlock(&x->lock);
+		spin_unlock_bh(&x->lock);
 
 		XFRM_SKB_CB(skb)->seq.input = seq;
 
@@ -192,7 +192,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			return 0;
 
 resume:
-		spin_lock(&x->lock);
+		spin_lock_bh(&x->lock);
 		if (nexthdr <= 0) {
 			if (nexthdr == -EBADMSG) {
 				xfrm_audit_state_icvfail(x, skb,
@@ -212,7 +212,7 @@ resume:
 		x->curlft.bytes += skb->len;
 		x->curlft.packets++;
 
-		spin_unlock(&x->lock);
+		spin_unlock_bh(&x->lock);
 
 		XFRM_MODE_SKB_CB(skb)->protocol = nexthdr;
 
@@ -260,7 +260,7 @@ resume:
 	}
 
 drop_unlock:
-	spin_unlock(&x->lock);
+	spin_unlock_bh(&x->lock);
 drop:
 	kfree_skb(skb);
 	return 0;
-- 
1.6.2.4




[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [IPSEC] xfrm_state locking problem in xfrm_input.c

[Herbert Xu]

On Mon, May 11, 2009 at 06:47:39PM -0400, Yury Polyanskiy wrote:
> xfrm_input() code uses spin_lock() where it must be using
> spin_lock_bh() instead. The corresponding code in xfrm_output.c
> correctly uses spin_lock_bh(). 
> 
> Note that if the locally generated packet is sent to a local ip,
> dev_queue_xmit() calls loopback_xmit() and the xfrm_input() will be
> called with softirqs enabled.

dev_queue_xmit always disables BH before calling the device xmit
function.  So how can this happen?

Thanks,
-- 
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: [IPSEC] xfrm_state locking problem in xfrm_input.c

[Yury Polyanskiy]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

On Tue, 12 May 2009 13:48:13 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Mon, May 11, 2009 at 06:47:39PM -0400, Yury Polyanskiy wrote:
> > xfrm_input() code uses spin_lock() where it must be using
> > spin_lock_bh() instead. The corresponding code in xfrm_output.c
> > correctly uses spin_lock_bh(). 
> > 
> > Note that if the locally generated packet is sent to a local ip,
> > dev_queue_xmit() calls loopback_xmit() and the xfrm_input() will be
> > called with softirqs enabled.
> 
> dev_queue_xmit always disables BH before calling the device xmit
> function.  So how can this happen?
> 

Oops, you right. Thanks for the explanation!

YP

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [IPSEC] xfrm_state locking problem in xfrm_input.c

[David Miller]

From: Yury Polyanskiy <ypolyans@Princeton.EDU>
Date: Tue, 12 May 2009 00:05:16 -0400

> On Tue, 12 May 2009 13:48:13 +1000
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
>> On Mon, May 11, 2009 at 06:47:39PM -0400, Yury Polyanskiy wrote:
>> > xfrm_input() code uses spin_lock() where it must be using
>> > spin_lock_bh() instead. The corresponding code in xfrm_output.c
>> > correctly uses spin_lock_bh(). 
>> > 
>> > Note that if the locally generated packet is sent to a local ip,
>> > dev_queue_xmit() calls loopback_xmit() and the xfrm_input() will be
>> > called with softirqs enabled.
>> 
>> dev_queue_xmit always disables BH before calling the device xmit
>> function.  So how can this happen?
>> 
> 
> Oops, you right. Thanks for the explanation!

So you didn't actually hit a bug that led you to write that
patch?

Please state this explicitly next time, and tell us that you are
"fixing" something based purely upon code inspection rather than
hitting a bug yourself.

Re: [IPSEC] xfrm_state locking problem in xfrm_input.c

[Yury Polyanskiy]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

On Mon, 11 May 2009 22:07:23 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> >> > Note that if the locally generated packet is sent to a local ip,
> >> > dev_queue_xmit() calls loopback_xmit() and the xfrm_input() will
> >> > be called with softirqs enabled.
> >> 
> >> dev_queue_xmit always disables BH before calling the device xmit
> >> function.  So how can this happen?
> >> 
> > 
> > Oops, you right. Thanks for the explanation!
> 
> So you didn't actually hit a bug that led you to write that
> patch?
> 
> Please state this explicitly next time, and tell us that you are
> "fixing" something based purely upon code inspection rather than
> hitting a bug yourself.
> 

Well, I hit some bug (occasional hard lockup of the box on ipsec'ed
data transfers), and this seemed as an excellent fit, because I
frequently test it on local->local trafic. Anyways, sorry for bothering.

YP

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 197 bytes --]