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:

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

I agree with your analysis, and that we take a little performance hit due to the atomics, but we get the benefit of calling xfrm_dev_state_add() after the state is completely initialized, and passed the criteria for addition by xfrm_state_add().

We already have too many of these atomic operatons in the
packet path, adding more is a no go.