> -----Original Message----- > From: Boris Pismenny > Sent: Sunday, December 3, 2017 1:29 PM > 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>; 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.
Nope, we are not adding anything new to the data-path. Currently, xdo_dev_state_add configures the hardware with the new SA and then stores a reference to it in a hash table. We suggest here to split it into two parts: 1) configure the hardware 2) add to the hash table (activate). No change in the data-path whatsoever.
