From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shannon Nelson Subject: Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op Date: Sat, 2 Dec 2017 16:38:11 -0800 Message-ID: References: <1511891742-84759-1-git-send-email-avivh@mellanox.com> <1511891742-84759-2-git-send-email-avivh@mellanox.com> <20171201070919.mbyzbgud7ud42uqi@gauss3.secunet.de> <4767ACB7-EBCF-407C-95C8-7345109D7544@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Aviv Heller , Herbert Xu , Boris Pismenny , Yevgeny Kliteynik , "netdev@vger.kernel.org" To: Yossi Kuperman , Steffen Klassert Return-path: Received: from userp1040.oracle.com ([156.151.31.81]:34786 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752216AbdLCAi2 (ORCPT ); Sat, 2 Dec 2017 19:38:28 -0500 In-Reply-To: <4767ACB7-EBCF-407C-95C8-7345109D7544@mellanox.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 12/2/2017 2:33 PM, Yossi Kuperman wrote: > > >>> On 1 Dec 2017, at 9:09, Steffen Klassert wrote: >>> >>> On Tue, Nov 28, 2017 at 07:55:41PM +0200, avivh@mellanox.com wrote: >>> From: Aviv Heller >>> >>> 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