RE: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion

-----Original message-----

From: Steffen Klassert
Sent: Thursday, October 26 2017, 9:16 am
To: Aviv Heller
Cc: netdev-owner@vger.kernel.org; avivh@mellanox.com; Herbert Xu; Boris Pismenny; Yossi Kuperman; Yevgeny Kliteynik; netdev@vger.kernel.org
Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion

On Wed, Oct 25, 2017 at 01:09:44PM +0000, Aviv Heller wrote:

quoted

-----Original message-----

quoted

From: Steffen Klassert
Sent: Wednesday, October 25 2017, 10:22 am
To: avivh@mellanox.com
Cc: Herbert Xu; Boris Pismenny; Yossi Kuperman; Yevgeny Kliteynik; netdev@vger.kernel.org
Subject: Re: [PATCH net-next 2/3] xfrm: Fix offload dev state addition to occur after insertion

On Tue, Oct 24, 2017 at 06:10:30PM +0300, avivh@mellanox.com wrote:

quoted

From: Aviv Heller <redacted>

Adding the state to the offload device prior to replay init in
xfrm_state_construct() will result in NULL dereference if a matching
ESP packet is received in between.

Adding it after insertion also has the benefit of the driver not having
to check whether a state with the same match criteria already exists,
but forces us to use an atomic type for the offload_handle, to make
certain a stack-read/driver-write race won't result in reading corrupt
data.

No, this will add multiple atomic operations to the packet path,
even in the non offloaded case.

I think the problem is that we set XFRM_STATE_VALID to early.
This was not a problem before we had offloading because
it was not possible to lookup this state before we inserted
it into the SADB. Now that the driver holds a subset of states
too, we need to make sure the state is fully initialized
before we mark it as valid.

The patch below should do it, in combination with your patch 1/3.

Could you please test this?

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b997f13..96eb263 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c

@@ -587,10 +587,6 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 	if (attrs[XFRMA_OUTPUT_MARK])
 		x->props.output_mark = nla_get_u32(attrs[XFRMA_OUTPUT_MARK]);
 
-	err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
-	if (err)
-		goto error;
-
 	if (attrs[XFRMA_SEC_CTX]) {
 		err = security_xfrm_state_alloc(x,
 						nla_data(attrs[XFRMA_SEC_CTX]));

@@ -620,6 +616,10 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 	/* override default values from above */
 	xfrm_update_ae_params(x, attrs, 0);
 
+	err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
+	if (err)
+		goto error;
+
 	return x;
 
 error:

Hi Steffen,

This patch does not work, due to:
	if (!x->type_offload)
		return -EINVAL;

test in xfrm_dev_state_add().

There is certainly a way arround that :)
The easiest I can think of would be to propagate XFRM_STATE_VALID
only after the state is inserted into the SADBs. I.e. move the
setting of XFRM_STATE_VALID out of __xfrm_init_state() and let the
callers do it.

This does seem like the easiest solution, if we don't move state addition to occur after insertion.
I'm waiting for our regression results (probably on Monday) for the patch below, and would appreciate your comments:

From 4da5c12abb59113428668afffc42eb51c48fa894 Mon Sep 17 00:00:00 2001
From: Aviv Heller <redacted>
Date: Thu, 26 Oct 2017 16:30:41 +0300
Subject: [PATCH] Temp

Signed-off-by: Aviv Heller <redacted>
---
 net/ipv4/ipcomp.c     |  1 +
 net/ipv6/ipcomp6.c    |  1 +
 net/key/af_key.c      |  1 +
 net/xfrm/xfrm_state.c |  6 +++---
 net/xfrm/xfrm_user.c  | 16 +++++++++-------
 5 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/net/ipv4/ipcomp.c b/net/ipv4/ipcomp.c
index d97f4f2..0016162 100644
--- a/net/ipv4/ipcomp.c
+++ b/net/ipv4/ipcomp.c

@@ -80,6 +80,7 @@ static struct xfrm_state *ipcomp_tunnel_create(struct xfrm_state *x)
 	if (xfrm_init_state(t))
 		goto error;
 
+	t->km.state = XFRM_STATE_VALID;
 	atomic_set(&t->tunnel_users, 1);
 out:
 	return t;

diff --git a/net/ipv6/ipcomp6.c b/net/ipv6/ipcomp6.c
index 54d165b..95b67f3 100644
--- a/net/ipv6/ipcomp6.c
+++ b/net/ipv6/ipcomp6.c

@@ -107,6 +107,7 @@ static struct xfrm_state *ipcomp6_tunnel_create(struct xfrm_state *x)
 	if (xfrm_init_state(t))
 		goto error;
 
+	t->km.state = XFRM_STATE_VALID;
 	atomic_set(&t->tunnel_users, 1);
 
 out:

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 10d7133..d1d43f7 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c

@@ -1276,6 +1276,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 		goto out;
 
 	x->km.seq = hdr->sadb_msg_seq;
+	x->km.state = XFRM_STATE_VALID;
 	return x;
 
 out:

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index a41e2ef..38b90fd 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c

@@ -1401,10 +1401,12 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
 	x->km.seq = orig->km.seq;
 	x->replay = orig->replay;
 	x->preplay = orig->preplay;
+	x->km.state = XFRM_STATE_VALID;
 
 	return x;
 
- error:
+error:
+	x->km.state = XFRM_STATE_DEAD;
 	xfrm_state_put(x);
 out:
 	return NULL;

@@ -2256,8 +2258,6 @@ int __xfrm_init_state(struct xfrm_state *x, bool init_replay, bool offload)
 			goto error;
 	}
 
-	x->km.state = XFRM_STATE_VALID;
-
 error:
 	return err;
 }

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index ffe8d5e..df304c2 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c

@@ -595,13 +595,6 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 			goto error;
 	}
 
-	if (attrs[XFRMA_OFFLOAD_DEV]) {
-		err = xfrm_dev_state_add(net, x,
-					 nla_data(attrs[XFRMA_OFFLOAD_DEV]));
-		if (err)
-			goto error;
-	}
-
 	if ((err = xfrm_alloc_replay_state_esn(&x->replay_esn, &x->preplay_esn,
 					       attrs[XFRMA_REPLAY_ESN_VAL])))
 		goto error;

@@ -617,6 +610,15 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 	/* override default values from above */
 	xfrm_update_ae_params(x, attrs, 0);
 
+	x->km.state = XFRM_STATE_VALID;
+
+	if (attrs[XFRMA_OFFLOAD_DEV]) {
+		err = xfrm_dev_state_add(net, x,
+					 nla_data(attrs[XFRMA_OFFLOAD_DEV]));
+		if (err)
+			goto error;
+	}
+
 	return x;
 
 error:

-- 
1.8.3.1