[XFRM]: Always reroute in tunnel mode

[XFRM]: Always reroute in tunnel mode

[Patrick McHardy]

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

Please see Changeset comment for a description, patch is based on
your 2.6.12 tree.

Regards
Patrick


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

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/02/17 07:10:03+01:00 kaber@coreworks.de 
#   [XFRM]: Always reroute in tunnel mode
#   
#   Tunnel mode packets are rerouted if the tunnel destination
#   address is different from the original destination address,
#   otherwise the old route is used. This is inconsistent, the
#   old route might have been selected for a given output device
#   or using routing by tos/fwmark. Always choose a new route
#   in tunnel mode.
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv6/xfrm6_policy.c
#   2005/02/17 07:09:55+01:00 kaber@coreworks.de +3 -1
#   [XFRM]: Always reroute in tunnel mode
#   
#   Tunnel mode packets are rerouted if the tunnel destination
#   address is different from the original destination address,
#   otherwise the old route is used. This is inconsistent, the
#   old route might have been selected for a given output device
#   or using routing by tos/fwmark. Always choose a new route
#   in tunnel mode.
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv4/xfrm4_policy.c
#   2005/02/17 07:09:55+01:00 kaber@coreworks.de +3 -1
#   [XFRM]: Always reroute in tunnel mode
#   
#   Tunnel mode packets are rerouted if the tunnel destination
#   address is different from the original destination address,
#   otherwise the old route is used. This is inconsistent, the
#   old route might have been selected for a given output device
#   or using routing by tos/fwmark. Always choose a new route
#   in tunnel mode.
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/net/ipv4/xfrm4_policy.c b/net/ipv4/xfrm4_policy.c
--- a/net/ipv4/xfrm4_policy.c	2005-02-17 07:16:40 +01:00
+++ b/net/ipv4/xfrm4_policy.c	2005-02-17 07:16:40 +01:00
@@ -59,6 +59,7 @@
 	int err;
 	int header_len = 0;
 	int trailer_len = 0;
+	int tunnel = 0;
 
 	dst = dst_prev = NULL;
 
@@ -81,12 +82,13 @@
 		if (xfrm[i]->props.mode) {
 			remote = xfrm[i]->id.daddr.a4;
 			local  = xfrm[i]->props.saddr.a4;
+			tunnel = 1;
 		}
 		header_len += xfrm[i]->props.header_len;
 		trailer_len += xfrm[i]->props.trailer_len;
 	}
 
-	if (remote != fl->fl4_dst) {
+	if (tunnel) {
 		struct flowi fl_tunnel = { .nl_u = { .ip4_u =
 						     { .daddr = remote,
 						       .saddr = local }
diff -Nru a/net/ipv6/xfrm6_policy.c b/net/ipv6/xfrm6_policy.c
--- a/net/ipv6/xfrm6_policy.c	2005-02-17 07:16:40 +01:00
+++ b/net/ipv6/xfrm6_policy.c	2005-02-17 07:16:40 +01:00
@@ -76,6 +76,7 @@
 	int err = 0;
 	int header_len = 0;
 	int trailer_len = 0;
+	int tunnel = 0;
 
 	dst = dst_prev = NULL;
 
@@ -98,12 +99,13 @@
 		if (xfrm[i]->props.mode) {
 			remote = (struct in6_addr*)&xfrm[i]->id.daddr;
 			local  = (struct in6_addr*)&xfrm[i]->props.saddr;
+			tunnel = 1;
 		}
 		header_len += xfrm[i]->props.header_len;
 		trailer_len += xfrm[i]->props.trailer_len;
 	}
 
-	if (!ipv6_addr_equal(remote, &fl->fl6_dst)) {
+	if (tunnel) {
 		struct flowi fl_tunnel;
 
 		memset(&fl_tunnel, 0, sizeof(fl_tunnel));

Re: [XFRM]: Always reroute in tunnel mode

[Herbert Xu]

On Thu, Feb 17, 2005 at 07:22:23AM +0100, Patrick McHardy wrote:
>
> #   Tunnel mode packets are rerouted if the tunnel destination
> #   address is different from the original destination address,
> #   otherwise the old route is used. This is inconsistent, the
> #   old route might have been selected for a given output device
> #   or using routing by tos/fwmark. Always choose a new route
> #   in tunnel mode.

I understand the inconsistency and agree that it should be fixed.
However, I think the way you did it has created a new inconsistency.

Tunnel mode SAs are not always used to carry subnets.  It can also
be used for host-to-host configurations where the aim is to protect
the IP header.  Therefore it would be inconsistent to look up a
new route for host-to-host tunnel mode SAs.

Perhaps we can simply expand the check to include local as well,
i.e.,

	if (local != fl->fl4_src || remote != fl->fl4_dst) {

What do you think?

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]: Always reroute in tunnel mode

[Patrick McHardy]

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

Herbert Xu wrote:

>I understand the inconsistency and agree that it should be fixed.
>However, I think the way you did it has created a new inconsistency.
>
>Tunnel mode SAs are not always used to carry subnets.  It can also
>be used for host-to-host configurations where the aim is to protect
>the IP header.  Therefore it would be inconsistent to look up a
>new route for host-to-host tunnel mode SAs.
>
>Perhaps we can simply expand the check to include local as well,
>i.e.,
>
>	if (local != fl->fl4_src || remote != fl->fl4_dst) {
>
>What do you think?
>  
>

I don't think this solves the inconsistency. By reuseing routes in tunnel
mode we allow routing by different criteria when the inner packet is headed
for the remote gateway. Your suggestion limits this a bit further, but we
can still have a situation where all packets going through a tunnel take
one path, except when the inner packet is heading for the remote gateway
itself. I think it is logically correct to reroute all packets in tunnel
mode, if we want to allow full policy routing for tunnel mode packets we
should hand tos and fwmark to xfrm_dst_lookup(). I don't think we should
do this currently because of a different problem. __xfrm4_find_bundle()
uses a different key than routing for looking up cached bundles. When the
original route was reused it is used even if fwmark/tos don't match. Fixing
this is easy, but it causes alot more cached bundles to exist. My last patch
limits this situation to transport mode because we always choose a new route
in tunnel mode based only on src/dst. Please see the attached patch for a
possible fix (ugly and compile tested only). If we want to do this for 
tunnel
mode we probably need a hash for the cached bundles first, which has its
own set of problems.

Regards
Patrick


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

===== include/net/xfrm.h 1.76 vs edited =====
--- 1.76/include/net/xfrm.h	2005-02-15 22:46:16 +01:00
+++ edited/include/net/xfrm.h	2005-02-17 18:57:39 +01:00
@@ -857,7 +857,7 @@
 extern void xfrm_policy_flush(void);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
 extern int xfrm_flush_bundles(void);
-extern int xfrm_bundle_ok(struct xfrm_dst *xdst, struct flowi *fl, int family);
+extern int xfrm_bundle_ok(struct xfrm_dst *xdst, struct flowi *fl, int family, int *is_tunnel);
 
 extern wait_queue_head_t km_waitq;
 extern int km_new_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr, u16 sport);
===== net/ipv4/xfrm4_policy.c 1.15 vs edited =====
--- 1.15/net/ipv4/xfrm4_policy.c	2005-02-17 07:09:55 +01:00
+++ edited/net/ipv4/xfrm4_policy.c	2005-02-17 19:04:45 +01:00
@@ -26,6 +26,7 @@
 __xfrm4_find_bundle(struct flowi *fl, struct xfrm_policy *policy)
 {
 	struct dst_entry *dst;
+	int is_tunnel = 0;
 
 	read_lock_bh(&policy->lock);
 	for (dst = policy->bundles; dst; dst = dst->next) {
@@ -33,7 +34,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 &&
-		    xfrm_bundle_ok(xdst, fl, AF_INET)) {
+		    xfrm_bundle_ok(xdst, fl, AF_INET, &is_tunnel) &&
+		    (!is_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
+		                    ))) {
 			dst_clone(dst);
 			break;
 		}
===== net/ipv6/xfrm6_policy.c 1.26 vs edited =====
--- 1.26/net/ipv6/xfrm6_policy.c	2005-02-17 07:09:55 +01:00
+++ edited/net/ipv6/xfrm6_policy.c	2005-02-17 18:59:31 +01:00
@@ -50,7 +50,7 @@
 				 xdst->u.rt6.rt6i_src.plen);
 		if (ipv6_addr_equal(&xdst->u.rt6.rt6i_dst.addr, &fl_dst_prefix) &&
 		    ipv6_addr_equal(&xdst->u.rt6.rt6i_src.addr, &fl_src_prefix) &&
-		    xfrm_bundle_ok(xdst, fl, AF_INET6)) {
+		    xfrm_bundle_ok(xdst, fl, AF_INET6, NULL)) {
 			dst_clone(dst);
 			break;
 		}
===== net/xfrm/xfrm_policy.c 1.66 vs edited =====
--- 1.66/net/xfrm/xfrm_policy.c	2005-02-16 00:16:04 +01:00
+++ edited/net/xfrm/xfrm_policy.c	2005-02-17 18:59:03 +01:00
@@ -1021,7 +1021,7 @@
 
 static int stale_bundle(struct dst_entry *dst)
 {
-	return !xfrm_bundle_ok((struct xfrm_dst *)dst, NULL, AF_UNSPEC);
+	return !xfrm_bundle_ok((struct xfrm_dst *)dst, NULL, AF_UNSPEC, NULL);
 }
 
 static void xfrm_dst_destroy(struct dst_entry *dst)
@@ -1101,7 +1101,7 @@
  * still valid.
  */
 
-int xfrm_bundle_ok(struct xfrm_dst *xdst, struct flowi *fl, int family)
+int xfrm_bundle_ok(struct xfrm_dst *xdst, struct flowi *fl, int family, int *is_tunnel)
 {
 	struct dst_entry *dst = &xdst->u.dst;
 
@@ -1114,6 +1114,8 @@
 			return 0;
 		if (dst->xfrm->km.state != XFRM_STATE_VALID)
 			return 0;
+		if (is_tunnel && dst->xfrm->props.mode)
+			*is_tunnel = 1;
 		dst = dst->child;
 	} while (dst->xfrm);
 

Re: [XFRM]: Always reroute in tunnel mode

[Patrick McHardy]

Patrick McHardy wrote:

>===== include/net/xfrm.h 1.76 vs edited =====
>--- 1.76/include/net/xfrm.h	2005-02-15 22:46:16 +01:00
>+++ edited/include/net/xfrm.h	2005-02-17 18:57:39 +01:00
>@@ -857,7 +857,7 @@
> extern void xfrm_policy_flush(void);
> extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
> extern int xfrm_flush_bundles(void);
>-extern int xfrm_bundle_ok(struct xfrm_dst *xdst, struct flowi *fl, int family);
>+extern int xfrm_bundle_ok(struct xfrm_dst *xdst, struct flowi *fl, int family, int *is_tunnel);
> 
> extern wait_queue_head_t km_waitq;
> extern int km_new_mapping(struct xfrm_state *x, xfrm_address_t *ipaddr, u16 sport);
>===== net/ipv4/xfrm4_policy.c 1.15 vs edited =====
>--- 1.15/net/ipv4/xfrm4_policy.c	2005-02-17 07:09:55 +01:00
>+++ edited/net/ipv4/xfrm4_policy.c	2005-02-17 19:04:45 +01:00
>@@ -26,6 +26,7 @@
> __xfrm4_find_bundle(struct flowi *fl, struct xfrm_policy *policy)
> {
> 	struct dst_entry *dst;
>+	int is_tunnel = 0;
> 
> 	read_lock_bh(&policy->lock);
> 	for (dst = policy->bundles; dst; dst = dst->next) {
>@@ -33,7 +34,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 &&
>-		    xfrm_bundle_ok(xdst, fl, AF_INET)) {
>+		    xfrm_bundle_ok(xdst, fl, AF_INET, &is_tunnel) &&
>+		    (!is_tunnel || (!(xdst->u.rt.fl.fl4_tos ^ fl->fl4_tos) &
>
The '!' is wrong of course.

>+		                                (IPTOS_RT_MASK|RTO_ONLINK) &&
>+#ifdef CONFIG_IP_ROUTE_FWMARK
>+		                      xdst->u.rt.fl.fl4_fwmark == fl->fl4_fwmark
>+#endif
>+		                    ))) {
> 			dst_clone(dst);
> 			break;
> 	
>

Re: [XFRM]: Always reroute in tunnel mode

[Herbert Xu]

On Thu, Feb 17, 2005 at 07:15:55PM +0100, Patrick McHardy wrote:
>
> >Perhaps we can simply expand the check to include local as well,
> >i.e.,
> >
> >	if (local != fl->fl4_src || remote != fl->fl4_dst) {
> >
> >What do you think?
> 
> I don't think this solves the inconsistency. By reuseing routes in tunnel
> mode we allow routing by different criteria when the inner packet is headed
> for the remote gateway. Your suggestion limits this a bit further, but we
> can still have a situation where all packets going through a tunnel take
> one path, except when the inner packet is heading for the remote gateway
> itself.

That's right.  However, you should also look at it this way.  We start
with a policy with a transport mode SA.  In order to protect the IP
header we change it to use a tunnel mode SA with a host-to-host selector.
With your patch this will change the route that the packet uses.
-- 
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]: Always reroute in tunnel mode

[Patrick McHardy]

Herbert Xu wrote:

>On Thu, Feb 17, 2005 at 07:15:55PM +0100, Patrick McHardy wrote:
>  
>
>>I don't think this solves the inconsistency. By reuseing routes in tunnel
>>mode we allow routing by different criteria when the inner packet is headed
>>for the remote gateway. Your suggestion limits this a bit further, but we
>>can still have a situation where all packets going through a tunnel take
>>one path, except when the inner packet is heading for the remote gateway
>>itself.
>>    
>>
>
>That's right.  However, you should also look at it this way.  We start
>with a policy with a transport mode SA.  In order to protect the IP
>header we change it to use a tunnel mode SA with a host-to-host selector.
>With your patch this will change the route that the packet uses.
>

I don't consider this inconsistent, in fact it is consistent to what
happens with other tunnels. We could get the behaviour you want (my
patch + old behaviour for host-to-host tunnels) by looking at the
policy selector, but I would prefer to always reroute. The change
doesn't affect existing setups, as I said in my previous mail, it
doesn't work properly since __xfrm4_find_bundle() ignores tos/fwmark
and uses the route for src/dst that made the cache (first one used)
for all tos/fwmark values, even if other routes exist.

Regards
Patrick

Re: [XFRM]: Always reroute in tunnel mode

[Herbert Xu]

On Thu, Feb 17, 2005 at 10:23:02PM +0100, Patrick McHardy wrote:
> 
> I don't consider this inconsistent, in fact it is consistent to what
> happens with other tunnels. We could get the behaviour you want (my

Well we'll have to disagree on that.  IMHO the flow with the internal
addresses equal to the external addresses over a tunnel mode SA should
be treated the same as that over a transport mode SA.

> patch + old behaviour for host-to-host tunnels) by looking at the
> policy selector, but I would prefer to always reroute. The change
> doesn't affect existing setups, as I said in my previous mail, it
> doesn't work properly since __xfrm4_find_bundle() ignores tos/fwmark
> and uses the route for src/dst that made the cache (first one used)
> for all tos/fwmark values, even if other routes exist.

Are you sure that it doesn't change existing behaviour? Suppose that
I had a socket bound to a specific device, doesn't the current code
use that device as long as we're sending to the remote IPsec gateway?

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]: Always reroute in tunnel mode

[Patrick McHardy]

Herbert Xu wrote:

>On Thu, Feb 17, 2005 at 10:23:02PM +0100, Patrick McHardy wrote:
>  
>
>>I don't consider this inconsistent, in fact it is consistent to what
>>happens with other tunnels. We could get the behaviour you want (my
>>    
>>
>
>Well we'll have to disagree on that.  IMHO the flow with the internal
>addresses equal to the external addresses over a tunnel mode SA should
>be treated the same as that over a transport mode SA.
>  
>

Maybe Dave can help resolve this with a third opinion.

>>patch + old behaviour for host-to-host tunnels) by looking at the
>>policy selector, but I would prefer to always reroute. The change
>>doesn't affect existing setups, as I said in my previous mail, it
>>doesn't work properly since __xfrm4_find_bundle() ignores tos/fwmark
>>and uses the route for src/dst that made the cache (first one used)
>>for all tos/fwmark values, even if other routes exist.
>>    
>>
>
>Are you sure that it doesn't change existing behaviour? Suppose that
>I had a socket bound to a specific device, doesn't the current code
>use that device as long as we're sending to the remote IPsec gateway?
>  
>

You're right, if no other route using same src/dst/oif made the cache
first it will be used.

Regards
Patrick

Re: [XFRM]: Always reroute in tunnel mode

[David S. Miller]

On Fri, 18 Feb 2005 00:02:27 +0100
Patrick McHardy <kaber@trash.net> wrote:

> >Well we'll have to disagree on that.  IMHO the flow with the internal
> >addresses equal to the external addresses over a tunnel mode SA should
> >be treated the same as that over a transport mode SA.
> 
> Maybe Dave can help resolve this with a third opinion.

I have to side with Patrick on this one.

This is one of the fundamental tunnel behavior differences between
Transport mode and Tunnel mode SAs.

Re: [XFRM]: Always reroute in tunnel mode

[Herbert Xu]

On Thu, Feb 17, 2005 at 03:11:22PM -0800, David S. Miller wrote:
> 
> I have to side with Patrick on this one.

OK.

> This is one of the fundamental tunnel behavior differences between
> Transport mode and Tunnel mode SAs.

I don't see it that way.  To me IPsec is about encapsulating a packet
so that it gets to the destination securely.  It shouldn't affect the
routing of the packet on the host itself.  Of course, once the packet
leaves the host the IPsec encapsulation will certainly affect its
routing.

In this respect, the difference between transport mode SAs and tunnel
mode SAs is simply that one adds an IP header while the other doesn't.

Ideally I should be able to only look at the routing table and determine
which interface/gateway a packet is going to be sent through.  As it is
I need to look at the routing table plus the IPsec database.

Put it another way, my solution to Patrick's inconsistency would be to
always inherit the routing decision from the top to the bottom of the
bundle.  For example, suppose you had

ip ro add 192.168.0.0/16 \
	nexthop via 10.0.0.1 dev eth0 \
	nexthop via 10.0.0.2 dev eth0

Then the packets to 192.168.0.0/16 should be sent via 10.0.0.1/10.0.0.2
regardless of what IPsec protections are applied to it.

This is something that can't be done in a clean way with the
current setup.  I'd like to see that available in Linux at some
point.  It'll have to be optional of course (selectable per-policy).

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]: Always reroute in tunnel mode

[Patrick McHardy]

Herbert Xu wrote:

>Put it another way, my solution to Patrick's inconsistency would be to
>always inherit the routing decision from the top to the bottom of the
>bundle.  For example, suppose you had
>
>ip ro add 192.168.0.0/16 \
>	nexthop via 10.0.0.1 dev eth0 \
>	nexthop via 10.0.0.2 dev eth0
>
>Then the packets to 192.168.0.0/16 should be sent via 10.0.0.1/10.0.0.2
>regardless of what IPsec protections are applied to it.
>
I agree it is a nice alternative to the current way. It would solve
another inconsistency caused by overriding the routing result in
tunnel mode: on output we don't care about oif, so packets from a
socket will be tunneled independent of sk_bound_dev_if. On input
packets won't be delivered to the socket if the encapsulated
packet arrived on a different interface.

Regards
Patrick