[IPSEC] fix ref counting in __xfrm4_bundle_create()

[IPSEC] fix ref counting in __xfrm4_bundle_create()

[Eugene Surovegin]

Hi all!

I think there is a ref counting bug in error path of __xfrm4_bundle_create().

xfrm_lookup() assumes that xfrm_bundle_create() will take ownership of 
xfrm_state array passed to it. If xfrm_bundle_create() fails, xfrm_lookup() will 
call xfrm_state_put() to release this array. 

Unfortunately, error path in __xfrm4_bundle_create() also calls xfrm_state_put() 
(indirectly through dst_free() -> ... -> xfrm_dst_destroy()) and we end up 
calling xfrm_state_put() _twice_.

This causes "infamous" :) "x->km.state == XFRM_STATE_DEAD" assertion in some our 
tests when interface is removed when IPSec traffic is going through it.

IPv6 version (__xfrm6_bundle_create()) seems to have the same problem. I'll send 
IPv6 version if my analysis and fix are correct :).

Here is IPv4 version (patch is against current linux-2.5 BK).

Signed-off-by: Eugene Surovegin <ebs@ebshome.net>

===== net/ipv4/xfrm4_policy.c 1.8 vs edited =====
--- 1.8/net/ipv4/xfrm4_policy.c	Fri Mar 19 19:35:32 2004
+++ edited/net/ipv4/xfrm4_policy.c	Fri May 28 16:22:35 2004
@@ -87,6 +87,7 @@
 
 		if (unlikely(dst1 == NULL)) {
 			err = -ENOBUFS;
+			nx = i;
 			goto error;
 		}
 
@@ -157,8 +158,14 @@
 	return 0;
 
 error:
-	if (dst)
+	if (dst){
+		/* bump refcount because dst_free will decrement it
+		   and our caller (xfrm_lookup) will do the same on error.
+		*/
+		for (i = 0; i < nx; i++)
+			xfrm_state_hold(xfrm[i]);
 		dst_free(dst);
+	}	
 	return err;
 }
 

Re: [IPSEC] fix ref counting in __xfrm4_bundle_create()

[Herbert Xu]

Eugene Surovegin <ebs@ebshome.net> wrote:
> 
> Unfortunately, error path in __xfrm4_bundle_create() also calls xfrm_state_put() 
> (indirectly through dst_free() -> ... -> xfrm_dst_destroy()) and we end up 
> calling xfrm_state_put() _twice_.

Well spotted.  Hopefully this should finally put an end to all these
xfrm_state bug reports :)

However, can you see if the following patch fixes this problem as well?
It moves the dst->xfrm assignment to a spot where errors cannot occur.

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
--
===== net/ipv4/xfrm4_policy.c 1.8 vs edited =====
--- 1.8/net/ipv4/xfrm4_policy.c	2004-03-20 14:35:32 +11:00
+++ edited/net/ipv4/xfrm4_policy.c	2004-05-29 13:01:39 +10:00
@@ -90,7 +90,6 @@
 			goto error;
 		}
 
-		dst1->xfrm = xfrm[i];
 		if (!dst)
 			dst = dst1;
 		else {
@@ -120,10 +119,12 @@
 		dst_hold(&rt->u.dst);
 	}
 	dst_prev->child = &rt->u.dst;
+	i = 0;
 	for (dst_prev = dst; dst_prev != &rt->u.dst; dst_prev = dst_prev->child) {
 		struct xfrm_dst *x = (struct xfrm_dst*)dst_prev;
 		x->u.rt.fl = *fl;
 
+		dst_prev->xfrm = xfrm[i++];
 		dst_prev->dev = rt->u.dst.dev;
 		if (rt->u.dst.dev)
 			dev_hold(rt->u.dst.dev);
===== net/ipv6/xfrm6_policy.c 1.16 vs edited =====
--- 1.16/net/ipv6/xfrm6_policy.c	2004-03-20 14:35:32 +11:00
+++ edited/net/ipv6/xfrm6_policy.c	2004-05-29 13:01:51 +10:00
@@ -107,7 +107,6 @@
 			goto error;
 		}
 
-		dst1->xfrm = xfrm[i];
 		if (!dst)
 			dst = dst1;
 		else {
@@ -139,9 +138,11 @@
 		dst_hold(&rt->u.dst);
 	}
 	dst_prev->child = &rt->u.dst;
+	i = 0;
 	for (dst_prev = dst; dst_prev != &rt->u.dst; dst_prev = dst_prev->child) {
 		struct xfrm_dst *x = (struct xfrm_dst*)dst_prev;
 
+		dst_prev->xfrm = xfrm[i++];
 		dst_prev->dev = rt->u.dst.dev;
 		if (rt->u.dst.dev)
 			dev_hold(rt->u.dst.dev);
===== net/xfrm/xfrm_policy.c 1.50 vs edited =====
--- 1.50/net/xfrm/xfrm_policy.c	2004-05-23 06:35:06 +10:00
+++ edited/net/xfrm/xfrm_policy.c	2004-05-29 12:52:41 +10:00
@@ -1017,6 +1017,8 @@
 
 static void xfrm_dst_destroy(struct dst_entry *dst)
 {
+	if (!dst->xfrm)
+		return;
 	xfrm_state_put(dst->xfrm);
 	dst->xfrm = NULL;
 }

Re: [IPSEC] fix ref counting in __xfrm4_bundle_create()

[Eugene Surovegin]

On Sat, May 29, 2004 at 01:27:13PM +1000, Herbert Xu wrote:
> However, can you see if the following patch fixes this problem as well?
> It moves the dst->xfrm assignment to a spot where errors cannot occur.

Yeah, looks much nicer than my version.

I'll try to test it tomorrow if I find somebody from QA in the office on 
Saturday :) or only after a long weekend (test setup is quite complicated and I 
cannot reproduce it at home).

Eugene.

Re: [IPSEC] fix ref counting in __xfrm4_bundle_create()

[David S. Miller]

On Sat, 29 May 2004 13:27:13 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Well spotted.  Hopefully this should finally put an end to all these
> xfrm_state bug reports :)
> 
> However, can you see if the following patch fixes this problem as well?

I'm applying this patch.  If there is a fixup necessary, just let
me know.

Re: [IPSEC] fix ref counting in __xfrm4_bundle_create()

[Eugene Surovegin]

On Sat, May 29, 2004 at 01:27:13PM +1000, Herbert Xu wrote:
> However, can you see if the following patch fixes this problem as well?
> It moves the dst->xfrm assignment to a spot where errors cannot occur.

Yes, your version is OK. We haven't got the crash during our testing.

Thanks a lot, Herbert.

Eugene