[PATCH][IPv6][IPsec] stop infinite loop

[PATCH][IPv6][IPsec] stop infinite loop

[Kazunori Miyazawa]

Hello,

IPv6 IPsec tunnel mode of linux-2.6.12-rc3 does not work
on my PCs. It seems to be infinitely looped.

I think dst_check causes it because it exptects the obsolete
of normal dst (I mean not xfrm) is 0 but ip6_route_add sets it -1.
This patch changes the default value of the obsolete.
This patch is for linux-2.6.12-rc3.

I will not be able to response quickly because I'm in holidays :-)

Signed-off-by Kazunori Miyazawa <kazunori@miyazawa.org>

diff -ruN a/net/ipv6/route.c b/net/ipv6/route.c
--- a/net/ipv6/route.c	2005-04-30 10:27:49.000000000 +0900
+++ b/net/ipv6/route.c	2005-04-30 10:34:04.000000000 +0900
@@ -113,7 +113,7 @@
 			.__refcnt	= ATOMIC_INIT(1),
 			.__use		= 1,
 			.dev		= &loopback_dev,
-			.obsolete	= -1,
+			.obsolete	= 0,
 			.error		= -ENETUNREACH,
 			.metrics	= { [RTAX_HOPLIMIT - 1] = 255, },
 			.input		= ip6_pkt_discard,
@@ -825,7 +825,7 @@
 		goto out;
 	}

-	rt->u.dst.obsolete = -1;
+	rt->u.dst.obsolete = 0;
 	rt->rt6i_expires = clock_t_to_jiffies(rtmsg->rtmsg_info);
 	if (nlh && (r = NLMSG_DATA(nlh))) {
 		rt->rt6i_protocol = r->rtm_protocol;
@@ -1407,7 +1407,7 @@
 	rt->u.dst.metrics[RTAX_MTU-1] = ipv6_get_mtu(rt->rt6i_dev);
 	rt->u.dst.metrics[RTAX_ADVMSS-1] = ipv6_advmss(dst_mtu(&rt->u.dst));
 	rt->u.dst.metrics[RTAX_HOPLIMIT-1] = -1;
-	rt->u.dst.obsolete = -1;
+	rt->u.dst.obsolete = 0;

 	rt->rt6i_flags = RTF_UP | RTF_NONEXTHOP;
 	if (!anycast)

Re: [PATCH][IPv6][IPsec] stop infinite loop

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4272EC95.20401@miyazawa.org> (at Sat, 30 Apr 2005 11:25:25 +0900), Kazunori Miyazawa <kazunori@miyazawa.org> says:

> I think dst_check causes it because it exptects the obsolete
> of normal dst (I mean not xfrm) is 0 but ip6_route_add sets it -1.
> This patch changes the default value of the obsolete.
> This patch is for linux-2.6.12-rc3.

No, ipv6 depends on obsolete == -1; ip6_dst_check checks if dst is 
up-to-date or not.

Please fix xfrm side; assumption that obsolete == 0.

Thanks.

--yoshfuji

Re: [PATCH][IPv6][IPsec] stop infinite loop

[Herbert Xu]

On Sat, Apr 30, 2005 at 12:07:36PM +0900, YOSHIFUJI Hideaki / ?$B5HF#1QL@ wrote:
> 
> No, ipv6 depends on obsolete == -1; ip6_dst_check checks if dst is 
> up-to-date or not.
> 
> Please fix xfrm side; assumption that obsolete == 0.

Agreed.  I'll try to track it down.

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

[IPSEC] Store idev entries (was: [PATCH][IPv6][IPsec] stop infinite loop)

[Herbert Xu]

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

Hi:

I had no luck in reproducing the infinite loop so I'll need someone
to send me a back trace or better instructions in getting it to go.

However, I did find a bug that stopped IPsec/IPv6 from working.  About
a month ago IPv6 started using rt6i_idev->dev on the cached socket dst
entries.  If the cached socket dst entry is IPsec, then rt6i_idev will
be NULL.

Since we want to look at the rt6i_idev of the original route in this
case, the easiest fix is to store rt6i_idev in the IPsec dst entry just
as we do for a number of other IPv6 route attributes.  Unfortunately
this means that we need some new code to handle the references to
rt6i_idev.  That's why this patch is bigger than it would otherwise be.

I've also done the same thing for IPv4 since it is conceivable that
once these idev attributes start getting used for accounting, we
probably need to dereference them for IPv4 IPsec entries too.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

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

[-- Attachment #2: xfrm-idev --]
[-- Type: text/plain, Size: 6074 bytes --]

--- k/include/net/xfrm.h  (mode:100644)
+++ l/include/net/xfrm.h  (mode:100644)
@@ -1,6 +1,7 @@
 #ifndef _NET_XFRM_H
 #define _NET_XFRM_H
 
+#include <linux/compiler.h>
 #include <linux/xfrm.h>
 #include <linux/spinlock.h>
 #include <linux/list.h>
@@ -516,6 +517,15 @@ struct xfrm_dst
 	u32 child_mtu_cached;
 };
 
+static inline void xfrm_dst_destroy(struct xfrm_dst *xdst)
+{
+	dst_release(xdst->route);
+	if (likely(xdst->u.dst.xfrm))
+		xfrm_state_put(xdst->u.dst.xfrm);
+}
+
+extern void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev);
+
 /* Decapsulation state, used by the input to store data during
  * decapsulation procedure, to be used later (during the policy
  * check
--- k/net/ipv4/xfrm4_policy.c  (mode:100644)
+++ l/net/ipv4/xfrm4_policy.c  (mode:100644)
@@ -8,7 +8,10 @@
  * 	
  */
 
+#include <asm/bug.h>
+#include <linux/compiler.h>
 #include <linux/config.h>
+#include <linux/inetdevice.h>
 #include <net/xfrm.h>
 #include <net/ip.h>
 
@@ -152,6 +155,8 @@ __xfrm4_bundle_create(struct xfrm_policy
 		x->u.rt.rt_dst = rt0->rt_dst;
 		x->u.rt.rt_gateway = rt->rt_gateway;
 		x->u.rt.rt_spec_dst = rt0->rt_spec_dst;
+		x->u.rt.idev = rt0->idev;
+		in_dev_hold(rt0->idev);
 		header_len -= x->u.dst.xfrm->props.header_len;
 		trailer_len -= x->u.dst.xfrm->props.trailer_len;
 	}
@@ -243,11 +248,45 @@ static void xfrm4_update_pmtu(struct dst
 	path->ops->update_pmtu(path, mtu);
 }
 
+static void xfrm4_dst_destroy(struct dst_entry *dst)
+{
+	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
+
+	if (likely(xdst->u.rt.idev))
+		in_dev_put(xdst->u.rt.idev);
+	xfrm_dst_destroy(xdst);
+}
+
+static void xfrm4_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
+			     int unregister)
+{
+	struct xfrm_dst *xdst;
+
+	if (!unregister)
+		return;
+
+	xdst = (struct xfrm_dst *)dst;
+	if (xdst->u.rt.idev->dev == dev) {
+		struct in_device *loopback_idev = in_dev_get(&loopback_dev);
+		BUG_ON(!loopback_idev);
+
+		do {
+			in_dev_put(xdst->u.rt.idev);
+			xdst->u.rt.idev = loopback_idev;
+			xdst = (struct xfrm_dst *)xdst->u.dst.child;
+		} while (xdst->u.dst.xfrm);
+	}
+
+	xfrm_dst_ifdown(dst, dev);
+}
+
 static struct dst_ops xfrm4_dst_ops = {
 	.family =		AF_INET,
 	.protocol =		__constant_htons(ETH_P_IP),
 	.gc =			xfrm4_garbage_collect,
 	.update_pmtu =		xfrm4_update_pmtu,
+	.destroy =		xfrm4_dst_destroy,
+	.ifdown =		xfrm4_dst_ifdown,
 	.gc_thresh =		1024,
 	.entry_size =		sizeof(struct xfrm_dst),
 };
--- k/net/ipv6/xfrm6_policy.c  (mode:100644)
+++ l/net/ipv6/xfrm6_policy.c  (mode:100644)
@@ -11,7 +11,11 @@
  * 
  */
 
+#include <asm/bug.h>
+#include <linux/compiler.h>
 #include <linux/config.h>
+#include <linux/netdevice.h>
+#include <net/addrconf.h>
 #include <net/xfrm.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
@@ -166,6 +170,8 @@ __xfrm6_bundle_create(struct xfrm_policy
 		memcpy(&x->u.rt6.rt6i_gateway, &rt0->rt6i_gateway, sizeof(x->u.rt6.rt6i_gateway)); 
 		x->u.rt6.rt6i_dst      = rt0->rt6i_dst;
 		x->u.rt6.rt6i_src      = rt0->rt6i_src;	
+		x->u.rt6.rt6i_idev     = rt0->rt6i_idev;
+		in6_dev_hold(rt0->rt6i_idev);
 		header_len -= x->u.dst.xfrm->props.header_len;
 		trailer_len -= x->u.dst.xfrm->props.trailer_len;
 	}
@@ -251,11 +257,45 @@ static void xfrm6_update_pmtu(struct dst
 	path->ops->update_pmtu(path, mtu);
 }
 
+static void xfrm6_dst_destroy(struct dst_entry *dst)
+{
+	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
+
+	if (likely(xdst->u.rt6.rt6i_idev))
+		in6_dev_put(xdst->u.rt6.rt6i_idev);
+	xfrm_dst_destroy(xdst);
+}
+
+static void xfrm6_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
+			     int unregister)
+{
+	struct xfrm_dst *xdst;
+
+	if (!unregister)
+		return;
+
+	xdst = (struct xfrm_dst *)dst;
+	if (xdst->u.rt6.rt6i_idev->dev == dev) {
+		struct inet6_dev *loopback_idev = in6_dev_get(&loopback_dev);
+		BUG_ON(!loopback_idev);
+
+		do {
+			in6_dev_put(xdst->u.rt6.rt6i_idev);
+			xdst->u.rt6.rt6i_idev = loopback_idev;
+			xdst = (struct xfrm_dst *)xdst->u.dst.child;
+		} while (xdst->u.dst.xfrm);
+	}
+
+	xfrm_dst_ifdown(dst, dev);
+}
+
 static struct dst_ops xfrm6_dst_ops = {
 	.family =		AF_INET6,
 	.protocol =		__constant_htons(ETH_P_IPV6),
 	.gc =			xfrm6_garbage_collect,
 	.update_pmtu =		xfrm6_update_pmtu,
+	.destroy =		xfrm6_dst_destroy,
+	.ifdown =		xfrm6_dst_ifdown,
 	.gc_thresh =		1024,
 	.entry_size =		sizeof(struct xfrm_dst),
 };
--- k/net/xfrm/xfrm_policy.c  (mode:100644)
+++ l/net/xfrm/xfrm_policy.c  (mode:100644)
@@ -1028,30 +1028,15 @@ static int stale_bundle(struct dst_entry
 	return !xfrm_bundle_ok((struct xfrm_dst *)dst, NULL, AF_UNSPEC);
 }
 
-static void xfrm_dst_destroy(struct dst_entry *dst)
+void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev)
 {
-	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
-
-	dst_release(xdst->route);
-
-	if (!dst->xfrm)
-		return;
-	xfrm_state_put(dst->xfrm);
-	dst->xfrm = NULL;
-}
-
-static void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
-			    int unregister)
-{
-	if (!unregister)
-		return;
-
 	while ((dst = dst->child) && dst->xfrm && dst->dev == dev) {
 		dst->dev = &loopback_dev;
 		dev_hold(&loopback_dev);
 		dev_put(dev);
 	}
 }
+EXPORT_SYMBOL(xfrm_dst_ifdown);
 
 static void xfrm_link_failure(struct sk_buff *skb)
 {
@@ -1262,10 +1247,6 @@ int xfrm_policy_register_afinfo(struct x
 			dst_ops->kmem_cachep = xfrm_dst_cache;
 		if (likely(dst_ops->check == NULL))
 			dst_ops->check = xfrm_dst_check;
-		if (likely(dst_ops->destroy == NULL))
-			dst_ops->destroy = xfrm_dst_destroy;
-		if (likely(dst_ops->ifdown == NULL))
-			dst_ops->ifdown = xfrm_dst_ifdown;
 		if (likely(dst_ops->negative_advice == NULL))
 			dst_ops->negative_advice = xfrm_negative_advice;
 		if (likely(dst_ops->link_failure == NULL))
@@ -1297,8 +1278,6 @@ int xfrm_policy_unregister_afinfo(struct
 			xfrm_policy_afinfo[afinfo->family] = NULL;
 			dst_ops->kmem_cachep = NULL;
 			dst_ops->check = NULL;
-			dst_ops->destroy = NULL;
-			dst_ops->ifdown = NULL;
 			dst_ops->negative_advice = NULL;
 			dst_ops->link_failure = NULL;
 			dst_ops->get_mss = NULL;

Re: [IPSEC] Store idev entries (was: [PATCH][IPv6][IPsec] stop infinite loop)

[Herbert Xu]

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

Hi:

The ref counting on loopback_idev wasn't quite right.  Here is the
corrected version.


However, I did find a bug that stopped IPsec/IPv6 from working.  About
a month ago IPv6 started using rt6i_idev->dev on the cached socket dst
entries.  If the cached socket dst entry is IPsec, then rt6i_idev will
be NULL.

Since we want to look at the rt6i_idev of the original route in this
case, the easiest fix is to store rt6i_idev in the IPsec dst entry just
as we do for a number of other IPv6 route attributes.  Unfortunately
this means that we need some new code to handle the references to
rt6i_idev.  That's why this patch is bigger than it would otherwise be.

I've also done the same thing for IPv4 since it is conceivable that
once these idev attributes start getting used for accounting, we
probably need to dereference them for IPv4 IPsec entries too.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

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

[-- Attachment #2: xfrm-idev --]
[-- Type: text/plain, Size: 6208 bytes --]

--- k/include/net/xfrm.h  (mode:100644)
+++ l/include/net/xfrm.h  (mode:100644)
@@ -1,6 +1,7 @@
 #ifndef _NET_XFRM_H
 #define _NET_XFRM_H
 
+#include <linux/compiler.h>
 #include <linux/xfrm.h>
 #include <linux/spinlock.h>
 #include <linux/list.h>
@@ -516,6 +517,15 @@ struct xfrm_dst
 	u32 child_mtu_cached;
 };
 
+static inline void xfrm_dst_destroy(struct xfrm_dst *xdst)
+{
+	dst_release(xdst->route);
+	if (likely(xdst->u.dst.xfrm))
+		xfrm_state_put(xdst->u.dst.xfrm);
+}
+
+extern void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev);
+
 /* Decapsulation state, used by the input to store data during
  * decapsulation procedure, to be used later (during the policy
  * check
--- k/net/ipv4/xfrm4_policy.c  (mode:100644)
+++ l/net/ipv4/xfrm4_policy.c  (mode:100644)
@@ -8,7 +8,10 @@
  * 	
  */
 
+#include <asm/bug.h>
+#include <linux/compiler.h>
 #include <linux/config.h>
+#include <linux/inetdevice.h>
 #include <net/xfrm.h>
 #include <net/ip.h>
 
@@ -152,6 +155,8 @@ __xfrm4_bundle_create(struct xfrm_policy
 		x->u.rt.rt_dst = rt0->rt_dst;
 		x->u.rt.rt_gateway = rt->rt_gateway;
 		x->u.rt.rt_spec_dst = rt0->rt_spec_dst;
+		x->u.rt.idev = rt0->idev;
+		in_dev_hold(rt0->idev);
 		header_len -= x->u.dst.xfrm->props.header_len;
 		trailer_len -= x->u.dst.xfrm->props.trailer_len;
 	}
@@ -243,11 +248,48 @@ static void xfrm4_update_pmtu(struct dst
 	path->ops->update_pmtu(path, mtu);
 }
 
+static void xfrm4_dst_destroy(struct dst_entry *dst)
+{
+	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
+
+	if (likely(xdst->u.rt.idev))
+		in_dev_put(xdst->u.rt.idev);
+	xfrm_dst_destroy(xdst);
+}
+
+static void xfrm4_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
+			     int unregister)
+{
+	struct xfrm_dst *xdst;
+
+	if (!unregister)
+		return;
+
+	xdst = (struct xfrm_dst *)dst;
+	if (xdst->u.rt.idev->dev == dev) {
+		struct in_device *loopback_idev = in_dev_get(&loopback_dev);
+		BUG_ON(!loopback_idev);
+
+		do {
+			in_dev_put(xdst->u.rt.idev);
+			xdst->u.rt.idev = loopback_idev;
+			in_dev_hold(loopback_idev);
+			xdst = (struct xfrm_dst *)xdst->u.dst.child;
+		} while (xdst->u.dst.xfrm);
+
+		__in_dev_put(loopback_idev);
+	}
+
+	xfrm_dst_ifdown(dst, dev);
+}
+
 static struct dst_ops xfrm4_dst_ops = {
 	.family =		AF_INET,
 	.protocol =		__constant_htons(ETH_P_IP),
 	.gc =			xfrm4_garbage_collect,
 	.update_pmtu =		xfrm4_update_pmtu,
+	.destroy =		xfrm4_dst_destroy,
+	.ifdown =		xfrm4_dst_ifdown,
 	.gc_thresh =		1024,
 	.entry_size =		sizeof(struct xfrm_dst),
 };
--- k/net/ipv6/xfrm6_policy.c  (mode:100644)
+++ l/net/ipv6/xfrm6_policy.c  (mode:100644)
@@ -11,7 +11,11 @@
  * 
  */
 
+#include <asm/bug.h>
+#include <linux/compiler.h>
 #include <linux/config.h>
+#include <linux/netdevice.h>
+#include <net/addrconf.h>
 #include <net/xfrm.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
@@ -166,6 +170,8 @@ __xfrm6_bundle_create(struct xfrm_policy
 		memcpy(&x->u.rt6.rt6i_gateway, &rt0->rt6i_gateway, sizeof(x->u.rt6.rt6i_gateway)); 
 		x->u.rt6.rt6i_dst      = rt0->rt6i_dst;
 		x->u.rt6.rt6i_src      = rt0->rt6i_src;	
+		x->u.rt6.rt6i_idev     = rt0->rt6i_idev;
+		in6_dev_hold(rt0->rt6i_idev);
 		header_len -= x->u.dst.xfrm->props.header_len;
 		trailer_len -= x->u.dst.xfrm->props.trailer_len;
 	}
@@ -251,11 +257,48 @@ static void xfrm6_update_pmtu(struct dst
 	path->ops->update_pmtu(path, mtu);
 }
 
+static void xfrm6_dst_destroy(struct dst_entry *dst)
+{
+	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
+
+	if (likely(xdst->u.rt6.rt6i_idev))
+		in6_dev_put(xdst->u.rt6.rt6i_idev);
+	xfrm_dst_destroy(xdst);
+}
+
+static void xfrm6_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
+			     int unregister)
+{
+	struct xfrm_dst *xdst;
+
+	if (!unregister)
+		return;
+
+	xdst = (struct xfrm_dst *)dst;
+	if (xdst->u.rt6.rt6i_idev->dev == dev) {
+		struct inet6_dev *loopback_idev = in6_dev_get(&loopback_dev);
+		BUG_ON(!loopback_idev);
+
+		do {
+			in6_dev_put(xdst->u.rt6.rt6i_idev);
+			xdst->u.rt6.rt6i_idev = loopback_idev;
+			in6_dev_hold(loopback_idev);
+			xdst = (struct xfrm_dst *)xdst->u.dst.child;
+		} while (xdst->u.dst.xfrm);
+
+		__in6_dev_put(loopback_idev);
+	}
+
+	xfrm_dst_ifdown(dst, dev);
+}
+
 static struct dst_ops xfrm6_dst_ops = {
 	.family =		AF_INET6,
 	.protocol =		__constant_htons(ETH_P_IPV6),
 	.gc =			xfrm6_garbage_collect,
 	.update_pmtu =		xfrm6_update_pmtu,
+	.destroy =		xfrm6_dst_destroy,
+	.ifdown =		xfrm6_dst_ifdown,
 	.gc_thresh =		1024,
 	.entry_size =		sizeof(struct xfrm_dst),
 };
--- k/net/xfrm/xfrm_policy.c  (mode:100644)
+++ l/net/xfrm/xfrm_policy.c  (mode:100644)
@@ -1028,30 +1028,15 @@ static int stale_bundle(struct dst_entry
 	return !xfrm_bundle_ok((struct xfrm_dst *)dst, NULL, AF_UNSPEC);
 }
 
-static void xfrm_dst_destroy(struct dst_entry *dst)
+void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev)
 {
-	struct xfrm_dst *xdst = (struct xfrm_dst *)dst;
-
-	dst_release(xdst->route);
-
-	if (!dst->xfrm)
-		return;
-	xfrm_state_put(dst->xfrm);
-	dst->xfrm = NULL;
-}
-
-static void xfrm_dst_ifdown(struct dst_entry *dst, struct net_device *dev,
-			    int unregister)
-{
-	if (!unregister)
-		return;
-
 	while ((dst = dst->child) && dst->xfrm && dst->dev == dev) {
 		dst->dev = &loopback_dev;
 		dev_hold(&loopback_dev);
 		dev_put(dev);
 	}
 }
+EXPORT_SYMBOL(xfrm_dst_ifdown);
 
 static void xfrm_link_failure(struct sk_buff *skb)
 {
@@ -1262,10 +1247,6 @@ int xfrm_policy_register_afinfo(struct x
 			dst_ops->kmem_cachep = xfrm_dst_cache;
 		if (likely(dst_ops->check == NULL))
 			dst_ops->check = xfrm_dst_check;
-		if (likely(dst_ops->destroy == NULL))
-			dst_ops->destroy = xfrm_dst_destroy;
-		if (likely(dst_ops->ifdown == NULL))
-			dst_ops->ifdown = xfrm_dst_ifdown;
 		if (likely(dst_ops->negative_advice == NULL))
 			dst_ops->negative_advice = xfrm_negative_advice;
 		if (likely(dst_ops->link_failure == NULL))
@@ -1297,8 +1278,6 @@ int xfrm_policy_unregister_afinfo(struct
 			xfrm_policy_afinfo[afinfo->family] = NULL;
 			dst_ops->kmem_cachep = NULL;
 			dst_ops->check = NULL;
-			dst_ops->destroy = NULL;
-			dst_ops->ifdown = NULL;
 			dst_ops->negative_advice = NULL;
 			dst_ops->link_failure = NULL;
 			dst_ops->get_mss = NULL;

Re: [IPSEC] Store idev entries (was: [PATCH][IPv6][IPsec] stop infinite loop)

[David S. Miller]

On Sat, 30 Apr 2005 21:43:49 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> The ref counting on loopback_idev wasn't quite right.  Here is the
> corrected version.

Applied, thanks Herbert.

Re: [IPSEC] Store idev entries (was: [PATCH][IPv6][IPsec] stop infinite loop)

[Herbert Xu]

On Tue, May 03, 2005 at 04:26:28PM -0700, David S. Miller wrote:
> 
> Applied, thanks Herbert.

Thanks Dave.

BTW, I was reviewing the rt6i_idev usage in IPv6 and it seems to me
that they need some protection with locking or RCU.  As it is they
are simply dereferenced in user/softirq context with no locking.

For example, the call chain udpv6_sendmsg -> ip6_dst_lookup ->
ipv6_get_saddr occurs in user context and simply dereferences
rt6i_idev.  The race condition is that another CPU could come
in and free rt6i_idev.  The reference count held by the dst
entry doesn't help since the other CPU will drop the reference
count for us through dst_ifdown.

Yoshifuji-san, does this look like a real problem to you?

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: [IPSEC] Store idev entries

[Patrick McHardy]

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

David S. Miller wrote:
> On Sat, 30 Apr 2005 21:43:49 +1000
> Herbert Xu <herbert@gondor.apana.org.au> wrote:
> 
>>The ref counting on loopback_idev wasn't quite right.  Here is the
>>corrected version.
> 
> 
> Applied, thanks Herbert.

The patch causes a compile error here:

  CC      net/ipv6/xfrm6_policy.o
In file included from net/ipv6/xfrm6_policy.c:18:
include/net/addrconf.h:40: error: field `prefix' has incomplete type

This patch fixes it.

Signed-off-by: Patrick McHardy <kaber@trash.net>

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 483 bytes --]

Index: net/ipv6/xfrm6_policy.c
===================================================================
--- 591ce19741741438606ab75a45ac9f973cbb4787/net/ipv6/xfrm6_policy.c  (mode:100644 sha1:4429b1a1fe5fe2528318b7417e539279548d9988)
+++ uncommitted/net/ipv6/xfrm6_policy.c  (mode:100644)
@@ -15,6 +15,7 @@
 #include <linux/compiler.h>
 #include <linux/config.h>
 #include <linux/netdevice.h>
+#include <linux/in6.h>
 #include <net/addrconf.h>
 #include <net/xfrm.h>
 #include <net/ip.h>

Re: [IPSEC] Store idev entries

[Herbert Xu]

On Wed, May 04, 2005 at 03:06:45AM +0200, Patrick McHardy wrote:
> 
> The patch causes a compile error here:
> 
>   CC      net/ipv6/xfrm6_policy.o
> In file included from net/ipv6/xfrm6_policy.c:18:
> include/net/addrconf.h:40: error: field `prefix' has incomplete type

Hmm, I wonder why I didn't see this.  Anyway, could you please add
the include to addrconf.h instead?

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] Store idev entries

[Herbert Xu]

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

On Wed, May 04, 2005 at 11:12:04AM +1000, herbert wrote:
> On Wed, May 04, 2005 at 03:06:45AM +0200, Patrick McHardy wrote:
> > 
> > The patch causes a compile error here:
> > 
> >   CC      net/ipv6/xfrm6_policy.o
> > In file included from net/ipv6/xfrm6_policy.c:18:
> > include/net/addrconf.h:40: error: field `prefix' has incomplete type
> 
> Hmm, I wonder why I didn't see this.  Anyway, could you please add
> the include to addrconf.h instead?

Aha, I can see it if I do something like this.
-- 
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

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 591 bytes --]

===== include/asm-i386/checksum.h 1.14 vs edited =====
--- 1.14/include/asm-i386/checksum.h	2005-01-16 22:58:42 +11:00
+++ edited/include/asm-i386/checksum.h	2005-05-04 11:17:09 +10:00
@@ -1,8 +1,6 @@
 #ifndef _I386_CHECKSUM_H
 #define _I386_CHECKSUM_H
 
-#include <linux/in6.h>
-
 /*
  * computes the checksum of a memory block at buff, length len,
  * and adds in "sum" (32-bit)
@@ -145,6 +143,7 @@
 }
 
 #define _HAVE_ARCH_IPV6_CSUM
+struct in6_addr;
 static __inline__ unsigned short int csum_ipv6_magic(struct in6_addr *saddr,
 						     struct in6_addr *daddr,
 						     __u32 len,

Re: [IPSEC] Store idev entries

[Patrick McHardy]

Herbert Xu wrote:
>>Hmm, I wonder why I didn't see this.  Anyway, could you please add
>>the include to addrconf.h instead?
> 
> Aha, I can see it if I do something like this.
> 
> 
> ------------------------------------------------------------------------
> 
> ===== include/asm-i386/checksum.h 1.14 vs edited =====
> --- 1.14/include/asm-i386/checksum.h	2005-01-16 22:58:42 +11:00
> +++ edited/include/asm-i386/checksum.h	2005-05-04 11:17:09 +10:00
> @@ -1,8 +1,6 @@
>  #ifndef _I386_CHECKSUM_H
>  #define _I386_CHECKSUM_H
>  
> -#include <linux/in6.h>
> -
>  /*
>   * computes the checksum of a memory block at buff, length len,
>   * and adds in "sum" (32-bit)
> @@ -145,6 +143,7 @@
>  }
>  
>  #define _HAVE_ARCH_IPV6_CSUM
> +struct in6_addr;
>  static __inline__ unsigned short int csum_ipv6_magic(struct in6_addr *saddr,
>  						     struct in6_addr *daddr,
>  						     __u32 len,

I use x86_64, which explains why I saw the error:

struct in6_addr;

#define _HAVE_ARCH_IPV6_CSUM 1
csum_ipv6_magic(struct in6_addr *saddr, struct in6_addr *daddr,
                __u32 len, unsigned short proto, unsigned int sum);

Regards
Patrick

Re: [IPSEC] Store idev entries

[Patrick McHardy]

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

Herbert Xu wrote:
> Hmm, I wonder why I didn't see this.  Anyway, could you please add
> the include to addrconf.h instead?

Sure, I missed the includes below and thought users of the file were
supposed to include it first.

Signed-off-by: Patrick McHardy <kaber@trash.net>

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 560 bytes --]

Index: include/net/addrconf.h
===================================================================
--- 591ce19741741438606ab75a45ac9f973cbb4787/include/net/addrconf.h  (mode:100644 sha1:f1e5af4be98e4a9cde314a5c1ceeaf3a61cf87ad)
+++ uncommitted/include/net/addrconf.h  (mode:100644)
@@ -17,6 +17,8 @@
 
 #define IPV6_MAX_ADDRESSES		16
 
+#include <linux/in6.h>
+
 struct prefix_info {
 	__u8			type;
 	__u8			length;
@@ -43,7 +45,6 @@
 
 #ifdef __KERNEL__
 
-#include <linux/in6.h>
 #include <linux/netdevice.h>
 #include <net/if_inet6.h>
 #include <net/ipv6.h>

Re: [IPSEC] Store idev entries

[David S. Miller]

On Wed, 04 May 2005 03:20:11 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Herbert Xu wrote:
> > Hmm, I wonder why I didn't see this.  Anyway, could you please add
> > the include to addrconf.h instead?
> 
> Sure, I missed the includes below and thought users of the file were
> supposed to include it first.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied, thanks guys.