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

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

[Patrick McHardy]

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



[-- Attachment #2: 03.diff --]
[-- Type: text/x-patch, Size: 2428 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/05 13:01:49+01:00 kaber@coreworks.de 
#   [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 kaber@coreworks.de +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 kaber@coreworks.de +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;
 
diff -Nru a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
--- a/net/ipv4/xfrm4_policy.c	2005-03-05 13:03:37 +01:00
+++ b/net/ipv4/xfrm4_policy.c	2005-03-05 13:03:37 +01:00
@@ -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);
 	}

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Herbert Xu]

Patrick McHardy <kaber@trash.net> wrote:
>
> @@ -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,
-- 
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: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Patrick McHardy]

Herbert Xu wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> 
>>@@ -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.

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.

> 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

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Patrick McHardy]

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

Herbert Xu wrote:
> Patrick McHardy <kaber@trash.net> wrote:
> 
>>@@ -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 <kaber@trash.net>

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

===== include/net/dst.h 1.26 vs edited =====
--- 1.26/include/net/dst.h	2005-02-15 23:23:10 +01:00
+++ edited/include/net/dst.h	2005-03-06 12:50:54 +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 1.15 vs edited =====
--- 1.15/net/ipv4/xfrm4_policy.c	2005-03-05 01:58:41 +01:00
+++ edited/net/ipv4/xfrm4_policy.c	2005-03-06 13:26:44 +01:00
@@ -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);
 	}

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Patrick McHardy]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 765 bytes --]

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

On Sun, 6 Mar 2005, Patrick McHardy wrote:
> 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 <kaber@trash.net>

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1435 bytes --]

===== include/net/dst.h 1.26 vs edited =====
--- 1.26/include/net/dst.h	2005-02-15 23:23:10 +01:00
+++ edited/include/net/dst.h	2005-03-06 12:50:54 +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 1.15 vs edited =====
--- 1.15/net/ipv4/xfrm4_policy.c	2005-03-05 01:58:41 +01:00
+++ edited/net/ipv4/xfrm4_policy.c	2005-03-06 18:49:23 +01:00
@@ -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);
 	}

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Herbert Xu]

On Sun, Mar 06, 2005 at 01:34:24PM +0100, Patrick McHardy wrote:
> 
> 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,
-- 
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: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Patrick McHardy]

Herbert Xu wrote:
> On Sun, Mar 06, 2005 at 01:34:24PM +0100, Patrick McHardy wrote:
> 
>>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

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Herbert Xu]

On Mon, Mar 07, 2005 at 02:41:30AM +0100, Patrick McHardy wrote:
> >
> >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,
-- 
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: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Patrick McHardy]

Herbert Xu wrote:
> On Mon, Mar 07, 2005 at 02:41:30AM +0100, Patrick McHardy wrote:
>>
>>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

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Herbert Xu]

On Mon, Mar 07, 2005 at 02:55:03AM +0100, Patrick McHardy wrote:
> 
> 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,
-- 
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: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Patrick McHardy]

Herbert Xu wrote:
> 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

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Herbert Xu]

On Mon, Mar 07, 2005 at 03:30:26AM +0100, Patrick McHardy wrote:
> 
> 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,
-- 
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: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Patrick McHardy]

Herbert Xu wrote:
> 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.

> 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

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[David S. Miller]

On Mon, 07 Mar 2005 04:11:17 +0100
Patrick McHardy <kaber@trash.net> wrote:

> Herbert Xu wrote:
> > 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.

Re: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[Herbert Xu]

On Mon, Mar 14, 2005 at 09:51:22PM -0800, David S. Miller wrote:
> On Mon, 07 Mar 2005 04:11:17 +0100
> Patrick McHardy <kaber@trash.net> wrote:
> 
> > Herbert Xu wrote:
> > > 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)?

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,
-- 
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: [PATCH 3/3 XFRM]: Fix invalid key for lookup of cached bundles

[David S. Miller]

On Tue, 15 Mar 2005 17:01:53 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

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