Re: [PATCH net v2 2/3] xfrm: Add an activate() offload dev op

[Shannon Nelson <shannon.nelson@oracle.com>]

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 <avivh@mellanox.com>; Steffen Klassert
>> <steffen.klassert@secunet.com>
>> Cc: Herbert Xu <herbert@gondor.apana.org.au>; Boris Pismenny
>> <borisp@mellanox.com>; Yossi Kuperman <yossiku@mellanox.com>;
>> Yevgeny Kliteynik <kliteyn@mellanox.com>; 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 <avivh@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.
>>>
>>> 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 <avivh@mellanox.com>
>>>> Signed-off-by: Yossi Kuperman <yossiku@mellanox.com>
>>>> ---
>>>> 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;
>>>>