> -----Original Message----- > From: Yossi Kuperman > Sent: Sunday, December 03, 2017 00:34 > To: 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 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? >
I would prefer we do not add additional checks in the data-path. IMO, the SA provided to the driver should be fully initialized. Do not insert it to HW before it is activated.
