Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op

[Shannon Nelson <shannon.nelson@oracle.com>]

On 12/2/2017 2:33 PM, Yossi Kuperman wrote:
> 
> 
>>> On 1 Dec 2017, at 9:09, Steffen Klassert <steffen.klassert@secunet.com> wrote:
>>>
>>> On Tue, Nov 28, 2017 at 07:55:41PM +0200, avivh@mellanox.com wrote:
>>> From: Aviv Heller <avivh@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.
>>>
>>> In order to inhibit driver offload logic from processing the state's
>>> packets prior to the xfrm_state object being completely initialized and
>>> added to the SADBs, a new activate() operation was added to inform the
>>> driver the aforementioned conditions have been met.
>>
>> We discussed this already some time ago, and I still think that
>> we should fix this by setting XFRM_STATE_VALID only after the
>> state is fully initialized.
> 
> An upcoming patch will refactor the if statement (encap_type < 0) in xfrm_input, in order to support crypto offload with GRO disabled. Currently it doesn’t work. This entails yet another check for the validity of the state. Resulting in total of 3 copies: 1) for normal traffic, 2) GRO and 3) crypto offload.
> 
> Anyway, IMO it is not right that we (the driver) allow an incoming packet to be delivered while the SA is not yet ready. Rather than checking for an invalid input I prefer to make sure that such a case won’t happen in the first place.
> 
> To complete the picture, there is another patch to the driver which simply drop incoming packets that underwent successful decryption and haven’t been activated yet. Active state merely means that the SA is present in the driver’s hash table.
> 
> We can make a separate patch to set the state to valid once it is fully initialized, it make sense on its own.
> 
> What do you think?
> 

If the SA isn't ready, just don't tell the driver about it.  Please 
don't add yet another state for the driver to track.  This should be as 
simple as possible, and shouldn't be any more complex than the model 
already used by ndo_vlan_rx_add_vid and ndo_vlan_rx_kill_vid.

sln