Re: [PATCH net-next v2] xfrm: Add ESN support for IPSec HW offload

[Yossi Kuperman <yossiku@mellanox.com>]

> On 10 Jan 2018, at 19:36, Shannon Nelson <shannon.nelson@oracle.com> wrote:
> 
>> On 1/10/2018 2:34 AM, yossefe@mellanox.com wrote:
>> From: Yossef Efraim <yossefe@mellanox.com>
>> This patch adds ESN support to IPsec device offload.
>> Adding new xfrm device operation to synchronize device ESN.
>> Signed-off-by: Yossef Efraim <yossefe@mellanox.com>
>> ---
>> Changes from v1:
>>  - Added documentation
>> ---
>>  Documentation/networking/xfrm_device.txt |  3 +++
>>  include/linux/netdevice.h                |  1 +
>>  include/net/xfrm.h                       | 12 ++++++++++++
>>  net/xfrm/xfrm_device.c                   |  4 ++--
>>  net/xfrm/xfrm_replay.c                   |  2 ++
>>  5 files changed, 20 insertions(+), 2 deletions(-)
>> diff --git a/Documentation/networking/xfrm_device.txt b/Documentation/networking/xfrm_device.txt
>> index 2d9d588c..50c34ca 100644
>> --- a/Documentation/networking/xfrm_device.txt
>> +++ b/Documentation/networking/xfrm_device.txt
>> @@ -41,6 +41,7 @@ struct xfrmdev_ops {
>>      void    (*xdo_dev_state_free) (struct xfrm_state *x);
>>      bool    (*xdo_dev_offload_ok) (struct sk_buff *skb,
>>                         struct xfrm_state *x);
>> +    void    (*xdo_dev_state_advance_esn) (struct xfrm_state *x);
>>  };
>>    The NIC driver offering ipsec offload will need to implement these
>> @@ -117,6 +118,8 @@ the stack in xfrm_input().
>>        hand the packet to napi_gro_receive() as usual
>>  +In ESN mode, xdo_dev_state_advance_esn() is called from xfrm_replay_advance_esn().
>> +Driver will check packet seq number and update HW ESN state machine if needed.
>>    When the SA is removed by the user, the driver's xdo_dev_state_delete()
>>  is asked to disable the offload.  Later, xdo_dev_state_free() is called
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 352066e..3c81cd7 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -842,6 +842,7 @@ struct xfrmdev_ops {
>>      void    (*xdo_dev_state_free) (struct xfrm_state *x);
>>      bool    (*xdo_dev_offload_ok) (struct sk_buff *skb,
>>                         struct xfrm_state *x);
>> +    void    (*xdo_dev_state_advance_esn) (struct xfrm_state *x);
>>  };
>>  #endif
>>  diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index 079ea94..1ca2e6e 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -1901,6 +1901,14 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>>                 struct xfrm_user_offload *xuo);
>>  bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x);
>>  +static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
>> +{
>> +    struct xfrm_state_offload *xso = &x->xso;
>> +
>> +    if (xso->dev && xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn)
>> +        xso->dev->xfrmdev_ops->xdo_dev_state_advance_esn(x);
>> +}
>> +
>>  static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
>>  {
>>      struct xfrm_state *x = dst->xfrm;
>> @@ -1971,6 +1979,10 @@ static inline bool xfrm_dev_offload_ok(struct sk_buff *skb, struct xfrm_state *x
>>      return false;
>>  }
>>  +static inline void xfrm_dev_state_advance_esn(struct xfrm_state *x)
>> +{
>> +}
>> +
>>  static inline bool xfrm_dst_offload_ok(struct dst_entry *dst)
>>  {
>>      return false;
>> diff --git a/net/xfrm/xfrm_device.c b/net/xfrm/xfrm_device.c
>> index 7598250..704a055 100644
>> --- a/net/xfrm/xfrm_device.c
>> +++ b/net/xfrm/xfrm_device.c
>> @@ -147,8 +147,8 @@ int xfrm_dev_state_add(struct net *net, struct xfrm_state *x,
>>      if (!x->type_offload)
>>          return -EINVAL;
>>  -    /* We don't yet support UDP encapsulation, TFC padding and ESN. */
>> -    if (x->encap || x->tfcpad || (x->props.flags & XFRM_STATE_ESN))
>> +    /* We don't yet support UDP encapsulation and TFC padding. */
>> +    if (x->encap || x->tfcpad)
> 
> As I mentioned before, this will cause issues when working with hardware that has no ESN support, such as Intel's x540: the stack will expect the driver to do ESN, and nothing actually happens but a rollover of the numbers.  Sure, the driver could look for the ESN attribute and fail the add, but that's a mode where we have to update every driver to fend off problems every time we add a new feature.  Much better is to only update drivers that actively support the new feature.
> 

You are right.

I’m not sure why this check is here in the first place. IMO it should take place in xdo_dev_state_add—a driver-specific callback.

What do you suggest?

> Look at how feature bits are added to netdev->features to signify what the driver can do.  I think that's a much better approach.
> 

It looks like an overkill?

> sln
> 
> 
>>          return -EINVAL;
>>        dev = dev_get_by_index(net, xuo->ifindex);
>> diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
>> index 0250181..1d38c6a 100644
>> --- a/net/xfrm/xfrm_replay.c
>> +++ b/net/xfrm/xfrm_replay.c
>> @@ -551,6 +551,8 @@ static void xfrm_replay_advance_esn(struct xfrm_state *x, __be32 net_seq)
>>              bitnr = replay_esn->replay_window - (diff - pos);
>>      }
>>  +    xfrm_dev_state_advance_esn(x);
>> +
>>      nr = bitnr >> 5;
>>      bitnr = bitnr & 0x1F;
>>      replay_esn->bmp[nr] |= (1U << bitnr);