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

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

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?
> 
> 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.

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;
>>