: Fix invalid key for lookup of cached bundles

PATCH In review Patrick McHardy 3 participants 16 messages Mar 5, 2005 – Mar 15, 2005

Subsystems (3)

Networking Network drivers Networking IPsec

lore

Share & tools

lore

Patrick McHardy

author

2005-03-05 13:59 UTC #0 lore raw

PATCH 3/3 : Fix invalid key for lookup of cached bundles

# This is a BitKeeper generated diff -Nru style patch. #
# ChangeSet
# 2005/03/05 13:01:49+01:00 [email protected]
# [XFRM4]: Fix invalid key for lookup of cached bundles
#

#   __xfrm4_find_bundle() uses a different key than routing for
#   looking up cached bundles. When the original route was
#   reused in transport mode it is used even if fwmark/tos
#   don't match. Also compare tos/fwmark for transport mode
#   bundles.
# 
# net/ipv4/xfrm4_policy.c
#   2005/03/05 13:01:41+01:00 [email protected] +8 -0
#   [XFRM4]: Fix invalid key for lookup of cached bundles
#   
#   __xfrm4_find_bundle() uses a different key than routing for
#   looking up cached bundles. When the original route was
#   reused in transport mode it is used even if fwmark/tos
#   don't match. Also compare tos/fwmark for transport mode
#   bundles.
# 
# include/net/dst.h
#   2005/03/05 13:01:41+01:00 [email protected] +1 -0
#   [XFRM4]: Fix invalid key for lookup of cached bundles
#   
#   __xfrm4_find_bundle() uses a different key than routing for
#   looking up cached bundles. When the original route was
#   reused in transport mode it is used even if fwmark/tos
#   don't match. Also compare tos/fwmark for transport mode
#   bundles.
# 
diff -Nru a/include/net/dst.h b/include/net/dst.h
--- a/include/net/dst.h	2005-03-05 13:03:37 +01:00
+++ b/include/net/dst.h	2005-03-05 13:03:37 +01:00
@@ -48,6 +48,7 @@
 #define DST_NOXFRM		2
 #define DST_NOPOLICY		4
 #define DST_NOHASH		8
+#define DST_XFRM_TUNNEL		16
 	unsigned long		lastuse;
 	unsigned long		expires;

net/ipv4/xfrm4_policy.c +8 -0

diff -Nru a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
@@ -33,6 +33,13 @@
 		if (xdst->u.rt.fl.oif == fl->oif &&	/*XXX*/
 		    xdst->u.rt.fl.fl4_dst == fl->fl4_dst &&
 	    	    xdst->u.rt.fl.fl4_src == fl->fl4_src &&
+		    (dst->path->flags & DST_XFRM_TUNNEL ||
+		     (!(xdst->u.rt.fl.fl4_tos ^ fl->fl4_tos) & 
+		                  (IPTOS_RT_MASK|RTO_ONLINK)
+#ifdef CONFIG_IP_ROUTE_FWMARK
+		      && xdst->u.rt.fl.fl4_fwmark == fl->fl4_fwmark
+#endif
+		      )) &&
 		    xfrm_bundle_ok(xdst, fl, AF_INET)) {
 			dst_clone(dst);
 			break;

@@ -97,6 +104,7 @@
 		err = xfrm_dst_lookup((struct xfrm_dst**)&rt, &fl_tunnel, AF_INET);
 		if (err)
 			goto error;
+		rt->u.dst.flags |= DST_XFRM_TUNNEL;
 	} else {
 		dst_hold(&rt->u.dst);
 	}

Herbert Xu

maintainer

2005-03-06 10:29 UTC #1 lore raw

net/ipv4/xfrm4_policy.c line 107

Quoted 6 lines · Patrick McHardy

net/ipv4/xfrm4_policy.c +1 -0

@@ -97,6 +104,7 @@
 		err = xfrm_dst_lookup((struct xfrm_dst**)&rt, &fl_tunnel, AF_INET);
 		if (err)
 			goto error;
+		rt->u.dst.flags |= DST_XFRM_TUNNEL;

This line doesn't look right. rt is an entry in the IPv4 routing cache, right? If so why should its flags change when some bundle is created?

After all, it could also be used at the bottom of a transport mode bundle.

Besides, I think IPv4 routing cache entry will never show up at the top of a bundle anyway which means that this flags value will never be read in the find_bundle function.

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.txt

Patrick McHardy

author

2005-03-06 10:54 UTC #2 lore raw

Quoted 13 lines · Herbert Xu

net/ipv4/xfrm4_policy.c line 107
Quoted 5 lines · Patrick McHardy
net/ipv4/xfrm4_policy.c +1 -0
@@ -97,6 +104,7 @@
 		err = xfrm_dst_lookup((struct xfrm_dst**)&rt, &fl_tunnel, AF_INET);
 		if (err)
 			goto error;
+		rt->u.dst.flags |= DST_XFRM_TUNNEL;
This line doesn't look right. rt is an entry in the IPv4 routing cache, right? If so why should its flags change when some bundle is created?

Quoted 1 line

After all, it could also be used at the bottom of a transport mode bundle.

Oops, that is correct of course. I wanted to kill the ugly int *is_tunnel argument to xfrm_bundle_ok() in my last patch, but I need to look for a different way.

Quoted 3 lines

Besides, I think IPv4 routing cache entry will never show up at the top of a bundle anyway which means that this flags value will never be read in the find_bundle function.

At least that part was correct, __xfrm4_find_bundle() uses dst->path->flags in my patch.

Thanks,
Patrick

Patrick McHardy

author

2005-03-06 12:34 UTC #3 lore raw

In reply to Herbert Xu

Quoted 12 lines · Herbert Xu

Review line 107
Quoted 5 lines · Patrick McHardy
(quoted diff) +1 -0
@@ -97,6 +104,7 @@
 		err = xfrm_dst_lookup((struct xfrm_dst**)&rt, &fl_tunnel, AF_INET);
 		if (err)
 			goto error;
+		rt->u.dst.flags |= DST_XFRM_TUNNEL;
This line doesn't look right. rt is an entry in the IPv4 routing cache, right? If so why should its flags change when some bundle is created?

How about this one ? It keeps the DST_XFRM_TUNNEL flag and sets it on the first xfrm_dst in a bundle. I know it doesn't really belong there, but the alternatives are walking through the bundle an additional time or having xfrm_bundle_ok() return if it is a tunnel-mode bundle, but in that case we can only compare tos etc after the call to xfrm_bundle_ok(), which is rather expensive. I also moved the oif check to the checks performed only in transport mode, this reduces the number of cached bundles in tunnel mode to one per src/dst if the selector isn't narrower than that.

Signed-off-by Patrick McHardy <[email protected]>

===== include/net/dst.h 1.26 vs edited =====

include/net/dst.h +1 -0

@@ -48,6 +48,7 @@
 #define DST_NOXFRM		2
 #define DST_NOPOLICY		4
 #define DST_NOHASH		8
+#define DST_XFRM_TUNNEL		16
 	unsigned long		lastuse;
 	unsigned long		expires;

===== net/ipv4/xfrm4_policy.c 1.15 vs edited =====

net/ipv4/xfrm4_policy.c +9 -2

@@ -30,9 +30,15 @@
 	read_lock_bh(&policy->lock);
 	for (dst = policy->bundles; dst; dst = dst->next) {
 		struct xfrm_dst *xdst = (struct xfrm_dst*)dst;
-		if (xdst->u.rt.fl.oif == fl->oif &&	/*XXX*/
-		    xdst->u.rt.fl.fl4_dst == fl->fl4_dst &&
+		if (xdst->u.rt.fl.fl4_dst == fl->fl4_dst &&
 	    	    xdst->u.rt.fl.fl4_src == fl->fl4_src &&
+		    (dst->flags & DST_XFRM_TUNNEL ||
+		     (!(xdst->u.rt.fl.fl4_tos ^ fl->fl4_tos) & 
+		                  (IPTOS_RT_MASK|RTO_ONLINK) &&
+#ifdef CONFIG_IP_ROUTE_FWMARK
+		      xdst->u.rt.fl.fl4_fwmark == fl->fl4_fwmark &&
+#endif
+		      xdst->u.rt.fl.oif == fl->oif)) &&
 		    xfrm_bundle_ok(xdst, fl, AF_INET)) {
 			dst_clone(dst);
 			break;

@@ -97,6 +103,7 @@
 		err = xfrm_dst_lookup((struct xfrm_dst**)&rt, &fl_tunnel, AF_INET);
 		if (err)
 			goto error;
+		dst->flags |= DST_XFRM_TUNNEL;
 	} else {
 		dst_hold(&rt->u.dst);
 	}

Patrick McHardy

author

2005-03-06 17:55 UTC #4 lore raw

Fixed a missing pair of parentheses - ! has precedence over bitwise AND.

Quoted 11 lines · Patrick McHardy

How about this one ? It keeps the DST_XFRM_TUNNEL flag and sets it on the first xfrm_dst in a bundle. I know it doesn't really belong there, but the alternatives are walking through the bundle an additional time or having xfrm_bundle_ok() return if it is a tunnel-mode bundle, but in that case we can only compare tos etc after the call to xfrm_bundle_ok(), which is rather expensive. I also moved the oif check to the checks performed only in transport mode, this reduces the number of cached bundles in tunnel mode to one per src/dst if the selector isn't narrower than that.
Signed-off-by Patrick McHardy <[email protected]>

===== include/net/dst.h 1.26 vs edited =====

include/net/dst.h +1 -0

@@ -48,6 +48,7 @@
 #define DST_NOXFRM		2
 #define DST_NOPOLICY		4
 #define DST_NOHASH		8
+#define DST_XFRM_TUNNEL		16
 	unsigned long		lastuse;
 	unsigned long		expires;

===== net/ipv4/xfrm4_policy.c 1.15 vs edited =====

net/ipv4/xfrm4_policy.c +9 -2

@@ -30,9 +30,15 @@
 	read_lock_bh(&policy->lock);
 	for (dst = policy->bundles; dst; dst = dst->next) {
 		struct xfrm_dst *xdst = (struct xfrm_dst*)dst;
-		if (xdst->u.rt.fl.oif == fl->oif &&	/*XXX*/
-		    xdst->u.rt.fl.fl4_dst == fl->fl4_dst &&
+		if (xdst->u.rt.fl.fl4_dst == fl->fl4_dst &&
 	    	    xdst->u.rt.fl.fl4_src == fl->fl4_src &&
+		    (dst->flags & DST_XFRM_TUNNEL ||
+		     (!((xdst->u.rt.fl.fl4_tos ^ fl->fl4_tos) & 
+		                   (IPTOS_RT_MASK|RTO_ONLINK)) &&
+#ifdef CONFIG_IP_ROUTE_FWMARK
+		      xdst->u.rt.fl.fl4_fwmark == fl->fl4_fwmark &&
+#endif
+		      xdst->u.rt.fl.oif == fl->oif)) &&
 		    xfrm_bundle_ok(xdst, fl, AF_INET)) {
 			dst_clone(dst);
 			break;

@@ -97,6 +103,7 @@
 		err = xfrm_dst_lookup((struct xfrm_dst**)&rt, &fl_tunnel, AF_INET);
 		if (err)
 			goto error;
+		dst->flags |= DST_XFRM_TUNNEL;
 	} else {
 		dst_hold(&rt->u.dst);
 	}

Herbert Xu

maintainer

2005-03-07 01:24 UTC #5 lore raw

In reply to Patrick McHardy

Quoted 3 lines · Patrick McHardy

How about this one ? It keeps the DST_XFRM_TUNNEL flag and sets it on the first xfrm_dst in a bundle. I know it doesn't really belong there,

Actually, why do we need to treat tunnel mode differently here? In other words, why not just do the mark/tos checks unconditionally.

Forwarded packets don't get a proper tos/mark setting for IPsec but that's a bug in itself.

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.txt

Patrick McHardy

author

2005-03-07 01:41 UTC #6 lore raw

Quoted 11 lines · Herbert Xu

Quoted 2 lines · Patrick McHardy
How about this one ? It keeps the DST_XFRM_TUNNEL flag and sets it on the first xfrm_dst in a bundle. I know it doesn't really belong there,
Actually, why do we need to treat tunnel mode differently here? In other words, why not just do the mark/tos checks unconditionally.
Forwarded packets don't get a proper tos/mark setting for IPsec but that's a bug in itself.

Mainly to avoid excessive long lists of cached bundles in tunnel mode. The use of a single list for the cache is questionable, but the patch was supposed to fix a different issue. Restricting use of tos/mark to transport mode avoids having exploding lists that are easily remotely triggerable.

Regards
Patrick

Herbert Xu

maintainer

2005-03-07 01:43 UTC #7 lore raw

Quoted 9 lines · Patrick McHardy

Quoted 3 lines
Actually, why do we need to treat tunnel mode differently here? In other words, why not just do the mark/tos checks unconditionally.
Mainly to avoid excessive long lists of cached bundles in tunnel mode. The use of a single list for the cache is questionable, but the patch was supposed to fix a different issue. Restricting use of tos/mark to transport mode avoids having exploding lists that are easily remotely triggerable.

That's a different problem. You can already create arbitrarily long bundle lists by spoofing src/dst addresses...

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.txt

Patrick McHardy

author

2005-03-07 01:55 UTC #8 lore raw

Quoted 11 lines · Herbert Xu

Quoted 6 lines · Patrick McHardy
Mainly to avoid excessive long lists of cached bundles in tunnel mode. The use of a single list for the cache is questionable, but the patch was supposed to fix a different issue. Restricting use of tos/mark to transport mode avoids having exploding lists that are easily remotely triggerable.
That's a different problem. You can already create arbitrarily long bundle lists by spoofing src/dst addresses...

But I don't want to make it worse. The number is still restricted by the scope of the selector, using tos and fwmark makes the worst case a lot worse.

Regards
Patrick

Herbert Xu

maintainer

2005-03-07 01:59 UTC #9 lore raw

Quoted 4 lines · Patrick McHardy

But I don't want to make it worse. The number is still restricted by the scope of the selector, using tos and fwmark makes the worst case a lot worse.

How about we fix the bundle problem first, and then add the fwmark/tos stuff? I think fixing the bundle list scalability is probably more important than having working TOS/fwmark at this point in time.

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.txt

Patrick McHardy

author

2005-03-07 02:30 UTC #10 lore raw

Quoted 3 lines · Herbert Xu

How about we fix the bundle problem first, and then add the fwmark/tos stuff? I think fixing the bundle list scalability is probably more important than having working TOS/fwmark at this point in time.

I agree that it is more important, but I don't see any harm in fixing the other problem for transport mode first. Fixing the scalability problem requires a dynamically resized hash, anything static will lead to different scalability problems with a large number of policies. The tos/fwmark part looks comparatively small, simply reroute all packets based on src/dst/fwmark/predicted final tos if they differ. But since both of this is not done yet, I think it would be better to fix the smaller problem first.

Regards
Patrick

Herbert Xu

maintainer

2005-03-07 02:57 UTC #11 lore raw

Quoted 9 lines · Patrick McHardy

I agree that it is more important, but I don't see any harm in fixing the other problem for transport mode first. Fixing the scalability problem requires a dynamically resized hash, anything static will lead to different scalability problems with a large number of policies. The tos/fwmark part looks comparatively small, simply reroute all packets based on src/dst/fwmark/predicted final tos if they differ. But since both of this is not done yet, I think it would be better to fix the smaller problem first.

The reason I'm asking is because the places where you're most likely to use tos/fwmark is in IPsec gateways. In other words, it isn't very useful unless it works in tunnel mode. This plus the fact that the check for tunnel mode is a bit of a hack makes me think that it's not worth it at the moment.

On the subject of fixing the scalability issue, we should just use the flow cache directly for each bundle.

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.txt

Patrick McHardy

author

2005-03-07 03:11 UTC #12 lore raw

Quoted 5 lines · Herbert Xu

The reason I'm asking is because the places where you're most likely to use tos/fwmark is in IPsec gateways. In other words, it isn't very useful unless it works in tunnel mode. This plus the fact that the check for tunnel mode is a bit of a hack makes me think that it's not worth it at the moment.

Ok, let's drop it for now. One of my reasons for fixing it was that it gives clearly defined behaviour, which makes it easier for me to make sure the changes I made for xfrm resolution are correct. I'm simply going to assume it will be working correctly sometime.

Quoted 2 lines

On the subject of fixing the scalability issue, we should just use the flow cache directly for each bundle.

Let me think about it.

Regards
Patrick

David S. Miller

maintainer

2005-03-15 05:51 UTC #13 lore raw

Quoted 8 lines · On Mon, 07 Mar 2005 04:11:17 +0100 Patrick McHardy <[email protected]> wrote:

Quoted 5 lines · Herbert Xu
The reason I'm asking is because the places where you're most likely to use tos/fwmark is in IPsec gateways. In other words, it isn't very useful unless it works in tunnel mode. This plus the fact that the check for tunnel mode is a bit of a hack makes me think that it's not worth it at the moment.
Ok, let's drop it for now.

Can someone send me an updated patch (with changelog msg please)? Thanks.

Herbert Xu

maintainer

2005-03-15 06:01 UTC #14 lore raw

Quoted 10 lines · David S. Miller

Quoted 7 lines · On Mon, 07 Mar 2005 04:11:17 +0100 Patrick McHardy <[email protected]> wrote:
Quoted 5 lines · Herbert Xu
The reason I'm asking is because the places where you're most likely to use tos/fwmark is in IPsec gateways. In other words, it isn't very useful unless it works in tunnel mode. This plus the fact that the check for tunnel mode is a bit of a hack makes me think that it's not worth it at the moment.

Quoted 3 lines

Quoted 1 line
Ok, let's drop it for now.
Can someone send me an updated patch (with changelog msg please)?

I think the conclusion is that until we fix the bundle scalability problem there is no point in adding tos/fwmark support here.

The reason is that if we do it now then tunnel mode SAs will potentially be targets of DoS attacks that create a large number of bundles off one policy. However, tunnel mode SAs would be the main users of this feature so if we can't do it for them then we might as well not do it.

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.txt

David S. Miller

maintainer

2005-03-15 06:14 UTC #15 lore raw

Quoted 2 lines · On Tue, 15 Mar 2005 17:01:53 +1100 Herbert Xu <[email protected]>…

I think the conclusion is that until we fix the bundle scalability problem there is no point in adding tos/fwmark support here.

Aha, I see. I misinterpreted the end of the discussion.

Thanks for clearing things up.