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

+ mailing list again.. (due to email client problem, sorry for the 
inconvenience).

On 11/28/2017 8:06 PM, Aviv Heller wrote:

+ mailing list

On 11/28/2017 7:55 PM, Aviv Heller wrote:

quoted

quoted

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

quoted

quoted

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.

quoted

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.

Okay, I'm submitting v2 with a solution proposed by Yossi Kuperman, 
which involves adding an activate() op to notify the driver that the 
state object is ready and added to kernel SADBs.

This way we can avoid the offload_handle problem, while not modifying 
km.state after insertion (which looks safer to me).

What do you think?

Thanks,
Aviv