[XFRM] Call dst_check() with appropriate cookie

[XFRM] Call dst_check() with appropriate cookie

[YOSHIFUJI Hideaki / 吉藤英明]

From: Kazunori Miyazawa <kazunori@miyazawa.org>

[XFRM] Call dst_check() with appropriate cookie.

This fixes infinite loop issue with IPv6 tunnel mode.

Signed-off-by: Kazunori Miyazawa <kazunori@miyazawa.org>
Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

diff -ruN linux-2.6.12-rc4-git7/include/net/xfrm.h linux-2.6.12-rc4-git7-ipv6ipsec/include/net/xfrm.h
--- linux-2.6.12-rc4-git7/include/net/xfrm.h	2005-05-24 00:15:26.000000000 +0900
+++ linux-2.6.12-rc4-git7-ipv6ipsec/include/net/xfrm.h	2005-05-24 00:19:24.000000000 +0900
@@ -515,6 +515,8 @@
 	struct dst_entry *route;
 	u32 route_mtu_cached;
 	u32 child_mtu_cached;
+	u32 route_cookie;
+	u32 path_cookie;
 };

 static inline void xfrm_dst_destroy(struct xfrm_dst *xdst)
diff -ruN linux-2.6.12-rc4-git7/net/ipv4/xfrm4_policy.c linux-2.6.12-rc4-git7-ipv6ipsec/net/ipv4/xfrm4_policy.c
--- linux-2.6.12-rc4-git7/net/ipv4/xfrm4_policy.c	2005-05-24 00:15:27.000000000 +0900
+++ linux-2.6.12-rc4-git7-ipv6ipsec/net/ipv4/xfrm4_policy.c	2005-05-24 00:20:29.000000000 +0900
@@ -95,6 +95,7 @@

 		xdst = (struct xfrm_dst *)dst1;
 		xdst->route = &rt->u.dst;
+		xdst->route_cookie = 0;

 		dst1->next = dst_prev;
 		dst_prev = dst1;
@@ -119,6 +120,7 @@

 	dst_prev->child = &rt->u.dst;
 	dst->path = &rt->u.dst;
+	((struct xfrm_dst*)dst)->path_cookie = 0;

 	*dst_p = dst;
 	dst = dst_prev;
diff -ruN linux-2.6.12-rc4-git7/net/ipv6/xfrm6_policy.c linux-2.6.12-rc4-git7-ipv6ipsec/net/ipv6/xfrm6_policy.c
--- linux-2.6.12-rc4-git7/net/ipv6/xfrm6_policy.c	2005-05-24 00:15:27.000000000 +0900
+++ linux-2.6.12-rc4-git7-ipv6ipsec/net/ipv6/xfrm6_policy.c	2005-05-24 00:22:15.000000000 +0900
@@ -113,6 +113,7 @@

 		xdst = (struct xfrm_dst *)dst1;
 		xdst->route = &rt->u.dst;
+		xdst->route_cookie = rt->rt6i_node->fn_sernum;

 		dst1->next = dst_prev;
 		dst_prev = dst1;
@@ -137,6 +138,7 @@

 	dst_prev->child = &rt->u.dst;
 	dst->path = &rt->u.dst;
+	((struct xfrm_dst*)dst)->path_cookie = rt->rt6i_node->fn_sernum;

 	*dst_p = dst;
 	dst = dst_prev;
diff -ruN linux-2.6.12-rc4-git7/net/xfrm/xfrm_policy.c linux-2.6.12-rc4-git7-ipv6ipsec/net/xfrm/xfrm_policy.c
--- linux-2.6.12-rc4-git7/net/xfrm/xfrm_policy.c	2005-05-24 00:15:28.000000000 +0900
+++ linux-2.6.12-rc4-git7-ipv6ipsec/net/xfrm/xfrm_policy.c	2005-05-24 00:22:57.000000000 +0900
@@ -1136,7 +1136,7 @@
 	struct xfrm_dst *last;
 	u32 mtu;

-	if (!dst_check(dst->path, 0) ||
+	if (!dst_check(dst->path, ((struct xfrm_dst*)dst)->path_cookie) ||
 	    (dst->dev && !netif_running(dst->dev)))
 		return 0;

@@ -1156,7 +1156,7 @@
 			xdst->child_mtu_cached = mtu;
 		}

-		if (!dst_check(xdst->route, 0))
+		if (!dst_check(xdst->route, xdst->route_cookie))
 			return 0;
 		mtu = dst_mtu(xdst->route);
 		if (xdst->route_mtu_cached != mtu) {

-- 
YOSHIFUJI Hideaki @ USAGI Project  <yoshfuji@linux-ipv6.org>
GPG-FP  : 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [XFRM] Call dst_check() with appropriate cookie

[Herbert Xu]

YOSHIFUJI Hideaki / ???? <yoshfuji@linux-ipv6.org> wrote:
> From: Kazunori Miyazawa <kazunori@miyazawa.org>
> 
> [XFRM] Call dst_check() with appropriate cookie.
>
> This fixes infinite loop issue with IPv6 tunnel mode.

Can someone please tell me where this infinite loop is?

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: [XFRM] Call dst_check() with appropriate cookie

[YOSHIFUJI Hideaki / 吉藤英明]

In article <E1DaTeW-00085B-00@gondolin.me.apana.org.au> (at Tue, 24 May 2005 17:16:36 +1000), Herbert Xu <herbert@gondor.apana.org.au> says:

> > This fixes infinite loop issue with IPv6 tunnel mode.
> 
> Can someone please tell me where this infinite loop is?

Honestly speaking, we haven't tracked down deeply where it is,
but that patch is logically reauired and it definetely solves grave
"freeze" issue with ipv6 tunnel mode xfrm.

--yoshfuji

Re: [XFRM] Call dst_check() with appropriate cookie

[Herbert Xu]

On Tue, May 24, 2005 at 04:30:18PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> Honestly speaking, we haven't tracked down deeply where it is,
> but that patch is logically reauired and it definetely solves grave
> "freeze" issue with ipv6 tunnel mode xfrm.

I'm happy with your patch as it is.  However, I'd like us to track
this freeze down first because if it is caused by another bug then
this patch may make it harder to find it.

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: [XFRM] Call dst_check() with appropriate cookie

[Kazunori Miyazawa]

Hello,

Without our patch, the dst_check in xfrm_bundle_ok returns NULL
when it is IPv6 route, then xfrm_bundle_ok always fails.
xfrm_lookup calls xfrm_bundle_ok via stalbe_bundle so that
it goes to restart and recreate bundles.
But stale_bundle always fails. It results the infinite loop accordingly.

This bug only occurs on IPv6. IPv4 works correctly without the patch.

I can not show the evidence of the infinite loop
because the kernel freezes :-<


Herbert Xu wrote:
> YOSHIFUJI Hideaki / ???? <yoshfuji@linux-ipv6.org> wrote:
> 
>>From: Kazunori Miyazawa <kazunori@miyazawa.org>
>>
>>[XFRM] Call dst_check() with appropriate cookie.
>>
>>This fixes infinite loop issue with IPv6 tunnel mode.
> 
> 
> Can someone please tell me where this infinite loop is?
> 
> Thanks,


--
Kazunori Miyazawa

Re: [XFRM] Call dst_check() with appropriate cookie

[Herbert Xu]

On Tue, May 24, 2005 at 05:26:37PM +0900, Kazunori Miyazawa wrote:
> 
> Without our patch, the dst_check in xfrm_bundle_ok returns NULL
> when it is IPv6 route, then xfrm_bundle_ok always fails.
> xfrm_lookup calls xfrm_bundle_ok via stalbe_bundle so that
> it goes to restart and recreate bundles.
> But stale_bundle always fails. It results the infinite loop accordingly.

Yes this makes perfect sense.  However, I'd still like to know why I
have never seen it myself.  Let me double-check my test setup.

Anyway, I'm happy with the patch apart from the comments I gave elsewhere
in this thread.

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: [XFRM] Call dst_check() with appropriate cookie

[Herbert Xu]

On Tue, May 24, 2005 at 06:34:19PM +1000, herbert wrote:
> 
> Yes this makes perfect sense.  However, I'd still like to know why I
> have never seen it myself.  Let me double-check my test setup.

Aha, not all IPv6 dst entries have obsolete set to -1.  Obviously
your ones were and mine weren't :)
-- 
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: [XFRM] Call dst_check() with appropriate cookie

[Herbert Xu]

On Tue, May 24, 2005 at 05:42:34PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
>
> Probably, it should be better to introduce some upper limit of number of 
> recreation.

Yep, that's why a simple packet blackhole bug became a kernel hang.

In fact, we don't need to retry at all.  If the policy goes dead on us
or if the route becomes invalid after we passed the check in xfrm_lookup,
the packet is dropped anyway.  There is no point in retrying the lookup
here since that only covers the cases where we detect the problem before
the bundle is inserted into the list.

So we should simply return an error instead of retrying.

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: [XFRM] Call dst_check() with appropriate cookie

[Herbert Xu]

On Tue, May 24, 2005 at 03:49:19AM +0000, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
>
> diff -ruN linux-2.6.12-rc4-git7/net/ipv4/xfrm4_policy.c linux-2.6.12-rc4-git7-ipv6ipsec/net/ipv4/xfrm4_policy.c
> --- linux-2.6.12-rc4-git7/net/ipv4/xfrm4_policy.c	2005-05-24 00:15:27.000000000 +0900
> +++ linux-2.6.12-rc4-git7-ipv6ipsec/net/ipv4/xfrm4_policy.c	2005-05-24 00:20:29.000000000 +0900
> @@ -95,6 +95,7 @@
> 
>  		xdst = (struct xfrm_dst *)dst1;
>  		xdst->route = &rt->u.dst;
> +		xdst->route_cookie = 0;
> 
>  		dst1->next = dst_prev;
>  		dst_prev = dst1;
> @@ -119,6 +120,7 @@
> 
>  	dst_prev->child = &rt->u.dst;
>  	dst->path = &rt->u.dst;
> +	((struct xfrm_dst*)dst)->path_cookie = 0;

This isn't necessary since dst_alloc zeros the entire xfrm_dst entry.

>  	dst_prev->child = &rt->u.dst;
>  	dst->path = &rt->u.dst;
> +	((struct xfrm_dst*)dst)->path_cookie = rt->rt6i_node->fn_sernum;

Please add a space between xfrm_dst and the asterisk, i.e.

(struct xfrm_dst *)dst

Also, can rt6i_node be NULL?

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: [XFRM] Call dst_check() with appropriate cookie

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20050524082021.GA31446@gondor.apana.org.au> (at Tue, 24 May 2005 18:20:21 +1000), Herbert Xu <herbert@gondor.apana.org.au> says:

> Also, can rt6i_node be NULL?

Good point.  Here's updated patch.

-----------------
From: Kazunori Miyazawa <kazunori@miyazawa.org>

[XFRM] Call dst_check() with appropriate cookie

This fixes infinite loop issue with IPv6 tunnel mode.

Signed-off-by: Kazunori Miyazawa <kazunori@miyazawa.org>
Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

Index: include/net/xfrm.h
===================================================================
--- 5b713315560487f8c288820b17061fe27016c2cc/include/net/xfrm.h  (mode:100644)
+++ uncommitted/include/net/xfrm.h  (mode:100644)
@@ -515,6 +515,8 @@
 	struct dst_entry *route;
 	u32 route_mtu_cached;
 	u32 child_mtu_cached;
+	u32 route_cookie;
+	u32 path_cookie;
 };
 
 static inline void xfrm_dst_destroy(struct xfrm_dst *xdst)
Index: net/ipv6/xfrm6_policy.c
===================================================================
--- 5b713315560487f8c288820b17061fe27016c2cc/net/ipv6/xfrm6_policy.c  (mode:100644)
+++ uncommitted/net/ipv6/xfrm6_policy.c  (mode:100644)
@@ -113,6 +113,8 @@
 
 		xdst = (struct xfrm_dst *)dst1;
 		xdst->route = &rt->u.dst;
+		if (rt->rt6i_node)
+			xdst->route_cookie = rt->rt6i_node->fn_sernum;
 
 		dst1->next = dst_prev;
 		dst_prev = dst1;
@@ -137,6 +139,8 @@
 
 	dst_prev->child = &rt->u.dst;
 	dst->path = &rt->u.dst;
+	if (rt->rt6i_node)
+		((struct xfrm_dst *)dst)->path_cookie = rt->rt6i_node->fn_sernum;
 
 	*dst_p = dst;
 	dst = dst_prev;
Index: net/xfrm/xfrm_policy.c
===================================================================
--- 5b713315560487f8c288820b17061fe27016c2cc/net/xfrm/xfrm_policy.c  (mode:100644)
+++ uncommitted/net/xfrm/xfrm_policy.c  (mode:100644)
@@ -1136,7 +1136,7 @@
 	struct xfrm_dst *last;
 	u32 mtu;
 
-	if (!dst_check(dst->path, 0) ||
+	if (!dst_check(dst->path, ((struct xfrm_dst *)dst)->path_cookie) ||
 	    (dst->dev && !netif_running(dst->dev)))
 		return 0;
 
@@ -1156,7 +1156,7 @@
 			xdst->child_mtu_cached = mtu;
 		}
 
-		if (!dst_check(xdst->route, 0))
+		if (!dst_check(xdst->route, xdst->route_cookie))
 			return 0;
 		mtu = dst_mtu(xdst->route);
 		if (xdst->route_mtu_cached != mtu) {

-- 
YOSHIFUJI Hideaki @ USAGI Project  <yoshfuji@linux-ipv6.org>
GPG-FP  : 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [XFRM] Call dst_check() with appropriate cookie

[Herbert Xu]

On Tue, May 24, 2005 at 05:49:31PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> [XFRM] Call dst_check() with appropriate cookie
> 
> This fixes infinite loop issue with IPv6 tunnel mode.
> 
> Signed-off-by: Kazunori Miyazawa <kazunori@miyazawa.org>
> Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>

Looks good.

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
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: [XFRM] Call dst_check() with appropriate cookie

[David S. Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Tue, 24 May 2005 21:59:14 +1000

> On Tue, May 24, 2005 at 05:49:31PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> > 
> > [XFRM] Call dst_check() with appropriate cookie
> > 
> > This fixes infinite loop issue with IPv6 tunnel mode.
> > 
> > Signed-off-by: Kazunori Miyazawa <kazunori@miyazawa.org>
> > Signed-off-by: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
> 
> Looks good.
> 
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Applied, thanks everyone.