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: Mon, 4 Dec 2017 11:40:56 -0800 Message-ID: References: <1511891742-84759-1-git-send-email-avivh@mellanox.com> <1511891742-84759-2-git-send-email-avivh@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: Herbert Xu , Boris Pismenny , Yevgeny Kliteynik , "netdev@vger.kernel.org" To: Yossi Kuperman , Aviv Heller , Steffen Klassert Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:23897 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750854AbdLDTlN (ORCPT ); Mon, 4 Dec 2017 14:41:13 -0500 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 12/3/2017 2:16 PM, Yossi Kuperman wrote: >> -----Original Message----- >> From: Shannon Nelson [mailto:shannon.nelson@oracle.com] >> Sent: Sunday, December 3, 2017 12:11 AM >> To: Aviv Heller ; Steffen Klassert >> >> Cc: Herbert Xu ; Boris Pismenny >> ; Yossi Kuperman ; >> Yevgeny Kliteynik ; netdev@vger.kernel.org >> Subject: Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op >> >> On 12/1/2017 11:47 AM, Shannon Nelson wrote: >>> On 11/28/2017 9:55 AM, 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. >>> >>> Are there also conditions where you would want to temporarily >>> deactivate, or pause, the incoming driver offload, followed then by >>> another activate? >>> > > Nope. > >>> sln >> >> Instead of setting up a half-ready state that needs the activate() operation to >> finish, can we instead just move the xfrm_dev_state_add() call to after the >> xfrm_init_replay()? Especially since this really only makes sense for the >> inbound, and makes no sense for the outbound path. >> > > It might solve the problem this time, but still the state is not fully initialized and it might break > in the future. Adding an SA to hardware can fail, and if we were to place xfrm_dev_state_add() > after the SA is fully initialized, we will be forced to roll-back. Let alone the possible things that could > go wrong once an SA is fully active while the hardware is oblivious. The core issue here is that we > can't construct the SA and configure the hardware atomically, hence the two steps. The first step is > to ensure the hardware can be configured properly, if not abort before the SA is initialized, and the > second step is to just "mark" the state as active. Please note that the we don't explicitly mark it, it > is active just by its existence in the hash table, which we already have and maintain. > > > Out of curiosity, what's wrong with adding yet another callback (not on the critical-path)? The existing model for hardware offloads is a single call to the driver to enable or disable - either the hardware does the offload or it doesn't. The closest examples are probably mac filters and vlan tags, as well as flow filters: in all three cases the stack hands the offload request to the driver and expects the offload to start at that point. There is no in-between step of "here's an offload, but don't do it yet". In reference the comment elsewhere about being "symmetric with xdo_dev_state_delete and xdo_dev_state_free", I'm not convinced that the xdo_dev_state_free is necessary: if the driver was already asked to delete the SA, why should it care about a free later? The driver model should be kept simple. There's no reason that I can see for the driver to need a two-step commit, either for the add or for the delete. When the stack asks the driver to do an offload, it should be ready for the offload to happen. sln > > Thanks for your comments. > >> sln >> >>> >>>> >>>> Signed-off-by: Aviv Heller >>>> Signed-off-by: Yossi Kuperman >>>> --- >>>> v1 -> v2: >>>>     - Separate to state addition and then activation, instead >>>>       of relocating dev state addition call. >>>> --- >>>>   include/linux/netdevice.h |  1 + >>>>   include/net/xfrm.h        | 12 ++++++++++++ >>>>   net/xfrm/xfrm_user.c      |  5 +++++ >>>>   3 files changed, 18 insertions(+) >>>> >>>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h >>>> index 2eaac7d..c6ca356 100644 >>>> --- a/include/linux/netdevice.h >>>> +++ b/include/linux/netdevice.h >>>> @@ -819,6 +819,7 @@ struct netdev_xdp { >>>>   #ifdef CONFIG_XFRM_OFFLOAD >>>>   struct xfrmdev_ops { >>>>       int    (*xdo_dev_state_add) (struct xfrm_state *x); >>>> +    void    (*xdo_dev_state_activate) (struct xfrm_state *x); >>>>       void    (*xdo_dev_state_delete) (struct xfrm_state *x); >>>>       void    (*xdo_dev_state_free) (struct xfrm_state *x); >>>>       bool    (*xdo_dev_offload_ok) (struct sk_buff *skb, diff --git >>>> a/include/net/xfrm.h b/include/net/xfrm.h index e015e16..324374e >>>> 100644 >>>> --- a/include/net/xfrm.h >>>> +++ b/include/net/xfrm.h >>>> @@ -1877,6 +1877,14 @@ static inline bool xfrm_dst_offload_ok(struct >>>> dst_entry *dst) >>>>       return false; >>>>   } >>>> +static inline void xfrm_dev_state_activate(struct xfrm_state *x) { >>>> +    struct xfrm_state_offload *xso = &x->xso; >>>> + >>>> +    if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_activate) >>>> +        xso->dev->xfrmdev_ops->xdo_dev_state_activate(x); >>>> +} >>>> + >>>>   static inline void xfrm_dev_state_delete(struct xfrm_state *x) >>>>   { >>>>       struct xfrm_state_offload *xso = &x->xso; @@ -1907,6 +1915,10 >>>> @@ static inline int xfrm_dev_state_add(struct net *net, struct >>>> xfrm_state *x, stru >>>>       return 0; >>>>   } >>>> +static inline void xfrm_dev_state_activate(struct xfrm_state *x) { } >>>> + >>>>   static inline void xfrm_dev_state_delete(struct xfrm_state *x) >>>>   { >>>>   } >>>> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index >>>> e44a0fe..d06f579 100644 >>>> --- a/net/xfrm/xfrm_user.c >>>> +++ b/net/xfrm/xfrm_user.c >>>> @@ -662,6 +662,11 @@ static int xfrm_add_sa(struct sk_buff *skb, >>>> struct nlmsghdr *nlh, >>>>           goto out; >>>>       } >>>> +    spin_lock_bh(&x->lock); >>>> +    if (x->km.state == XFRM_STATE_VALID) >>>> +        xfrm_dev_state_activate(x); >>>> +    spin_unlock_bh(&x->lock); >>>> + >>>>       c.seq = nlh->nlmsg_seq; >>>>       c.portid = nlh->nlmsg_pid; >>>>       c.event = nlh->nlmsg_type; >>>>