xfrm: Fix inter family IPsec tunnel handling again

Move the selector family initialization behind the check for AF_UNSPEC and call xfrm_ip2inner_mode() in any case. So the selector family is intitalized and we can choose for the right inner_mode.

---
 net/xfrm/xfrm_input.c |   19 ++++---------------
 net/xfrm/xfrm_state.c |    2 ++
 net/xfrm/xfrm_user.c  |    4 ----
 3 files changed, 6 insertions(+), 19 deletions(-)

index 7527940..d220ecf 100644
@@ -91,11 +91,9 @@ int xfrm_prepare_input(struct xfrm_state *x, struct sk_buff *skb)
 	if (err)
 		return err;
 
-	if (x->sel.family == AF_UNSPEC) {
-		inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
-		if (inner_mode == NULL)
-			return -EAFNOSUPPORT;
-	}
+	inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
+	if (inner_mode == NULL)
+		return -EAFNOSUPPORT;
 
 	skb->protocol = inner_mode->afinfo->eth_proto;
 	return inner_mode->input2(x, skb);

@@ -108,7 +106,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	__be32 seq;
 	struct xfrm_state *x;
 	xfrm_address_t *daddr;
-	struct xfrm_mode *inner_mode;
 	unsigned int family;
 	int decaps = 0;
 	int async = 0;

@@ -215,15 +212,7 @@ resume:
 
 		XFRM_MODE_SKB_CB(skb)->protocol = nexthdr;
 
-		inner_mode = x->inner_mode;
-
-		if (x->sel.family == AF_UNSPEC) {
-			inner_mode = xfrm_ip2inner_mode(x, XFRM_MODE_SKB_CB(skb)->protocol);
-			if (inner_mode == NULL)
-				goto drop;
-		}
-
-		if (inner_mode->input(x, skb)) {
+		if (x->inner_mode->input(x, skb)) {
 			XFRM_INC_STATS(LINUX_MIB_XFRMINSTATEMODEERROR);
 			goto drop;
 		}

index 72fddaf..9ea1008 100644
@@ -2030,6 +2030,8 @@ int xfrm_init_state(struct xfrm_state *x)
 			x->inner_mode = inner_mode_iaf;
 			x->inner_mode_iaf = inner_mode;
 		}
+
+		x->sel.family = family;
 	}
 
 	x->type = xfrm_get_type(x->id.proto, family);

index b976d9e..dae0956 100644
@@ -276,10 +276,6 @@ static void copy_from_user_state(struct xfrm_state *x, struct xfrm_usersa_info *
 	x->props.family = p->family;
 	memcpy(&x->props.saddr, &p->saddr, sizeof(x->props.saddr));
 	x->props.flags = p->flags;
-
-	if (!x->sel.family)
-		x->sel.family = p->family;
-
 }
 
 /*

1.5.3

Actually we are setting sel.family to the outer mode family if it is unspecified from userspace, so it is always specified at this point. This was changed by
commit bcf0dda8 ([XFRM]: xfrm_user: fix selector family initialization) because openswan seems to have problems if sel.family is not initialized.

But now the checks for AF_UNSPEC that are introduced by
commit df9dcb45 ([IPSEC]: Fix inter address family IPsec tunnel handling) return false. So the code that tries to choose for the right inner mode if sel.family was not specified from userspace is not executed and an inter family tunnel does not work in this case.

I tried to move the selector family initialization behind the check for AF_UNSPEC in xfrm_init_state(). This brings an inter family tunnel to work if no selector family is specified from userspace. But yes, if a selector family is specified we may send a packet that is not allowed to go. So this patch is probaply not what we want.

But what's the right thing to do for inter family tunnels?

Is it expected that the selector family is set explicitly from userspace in this case?
Unfortunalely many userspace applications don't do this by default. In this case there is nothing to fix here but most of the code introduced by commit df9dcb45 is obsolete then.

Or should the kernel try to do the right thing if the selector is not specified?
This was probaply the idea of commit df9dcb45. But if we have to initialize the selector family with something that is not AF_UNSPEC at initialization time of the xfrm_state, we can't know whether we got a selector family from userspace or whether it was set by the kernel when we process packets with that xfrm_state. Thus, we can't decide whether we are allowed to modify the inner mode. At the moment I don't see an easy solution for this case, but perhaps somebody else has an idea.

Steffen

I discussed this with Miyazawa-san earlier in the year. In order to maintain backwards compatibility, the use of AF_UNSPEC needs to be combined with an SA flag. So let's create a new flag that can be set by the applications which needs the AF_UNSPEC behaviour.

That way you get what you want without breaking existing users such as Openswan.

Cheers,

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

Just to get you right, do you mean something like the patch below? With this and a patched iproute2 I've got the inter family tunnels running.

Steffen

---
 include/linux/xfrm.h |    1 +
 net/xfrm/xfrm_user.c |    3 +--
 2 files changed, 2 insertions(+), 2 deletions(-)

index 2ca6bae..fb0c215 100644
@@ -339,6 +339,7 @@ struct xfrm_usersa_info {
 #define XFRM_STATE_NOPMTUDISC	4
 #define XFRM_STATE_WILDRECV	8
 #define XFRM_STATE_ICMP		16
+#define XFRM_STATE_AF_UNSPEC	32
 };
 
 struct xfrm_usersa_id {

index b976d9e..04c4150 100644
@@ -277,9 +277,8 @@ static void copy_from_user_state(struct xfrm_state *x, struct xfrm_usersa_info *
 	memcpy(&x->props.saddr, &p->saddr, sizeof(x->props.saddr));
 	x->props.flags = p->flags;
 
-	if (!x->sel.family)
+	if (!x->sel.family && !(p->flags & XFRM_STATE_AF_UNSPEC))
 		x->sel.family = p->family;
-
 }
 
 /*

1.5.3

Precisely.

Thanks,

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

So I'll write a commit message and send it later today.
For the case that this one makes it in I will send a patch for iproute2 too. Then we would have at least one application that uses the new flag for now.

Thanks for all the review,

Steffen