-----Original message-----
> From: Steffen Klassert
> Sent: Wednesday, October 25 2017, 10:22 am
> To: av...@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, av...@mellanox.com wrote:
> > From: Aviv Heller <av...@mellanox.com>
> >
> > 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().
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().
Another approach I thought of is to insert the state with an invalid km.state,
and only after the HW state addition succeeds, we move km.state to valid.
I did not go for this approach because here again we need to use atomics (or
not?), and since state testing is done in quite a few places, was afraid of
unexpected consequences.
What do you think?
Thanks,
Aviv
