> -----Original Message----- > From: Shannon Nelson [mailto:shannon.nel...@oracle.com] > Sent: Sunday, December 3, 2017 2:38 AM > To: Yossi Kuperman <yoss...@mellanox.com>; Steffen Klassert > <steffen.klass...@secunet.com> > Cc: Aviv Heller <av...@mellanox.com>; Herbert Xu > <herb...@gondor.apana.org.au>; Boris Pismenny <bor...@mellanox.com>; > Yevgeny Kliteynik <klit...@mellanox.com>; netdev@vger.kernel.org > Subject: Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op > > On 12/2/2017 2:33 PM, Yossi Kuperman wrote: > > > > > >>> On 1 Dec 2017, at 9:09, Steffen Klassert <steffen.klass...@secunet.com> > wrote: > >>> > >>> On Tue, Nov 28, 2017 at 07:55:41PM +0200, 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. > >>> > >>> 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.
It is not something new, the driver already "tracks" any SA that uses crypto offload, we just do it in two phases; please see other answers. Anyway it is symmetric with xdo_dev_state_delete and xdo_dev_state_free, J > > sln
