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

- From: Aviv Heller \<hidden\>
- Date: 2017-11-28 18:17:42 
- List: netdev
- Page: https://ratatoskr.run/netdev/2017/11/11074778
- Thread: https://ratatoskr.run/netdev/2017/11/11074778/t.md

```text
+ 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:
>>> On Wed, Oct 25, 2017 at 01:09:44PM +0000, Aviv Heller wrote:
>>>> -----Original message-----
>>>>> From: Steffen Klassert
>>>>> Sent: Wednesday, October 25 2017, 10:22 am
>>>>> To: <redacted>
>>>>> 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:
>>>>>> 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.
>>>
>>
>> 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
>
```