Message-ID: <DB6PR0501MB271023B10CD4A65C5F017A75C4160@DB6PR0501MB2710.eurprd05.prod.outlook.com>
Date:   Thu, 11 Jan 2018 08:28:51 +0000
From:   Yossi Kuperman <yossiku@...lanox.com>
To:     Shannon Nelson <shannon.nelson@...cle.com>
CC:     Yossef Efraim <yossefe@...lanox.com>,
        Jonathan Corbet <corbet@....net>,
        "David S. Miller" <davem@...emloft.net>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Boris Pismenny <borisp@...lanox.com>,
        Yevgeny Kliteynik <kliteyn@...lanox.com>
Subject: RE: [PATCH net-next v2] xfrm: Add ESN support for IPSec HW offload

> From: Shannon Nelson [mailto:shannon.nelson@...cle.com]
> Sent: Thursday, January 11, 2018 5:21 AM
> 
> On 1/10/2018 3:09 PM, Yossi Kuperman wrote:
> >> On 10 Jan 2018, at 19:36, Shannon Nelson <shannon.nelson@...cle.com> wrote:
> >>
> >>> On 1/10/2018 2:34 AM, yossefe@...lanox.com wrote:
> >>> From: Yossef Efraim <yossefe@...lanox.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@...lanox.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/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.
> >
> 
> If you say I'm right, then why do you say it should take place in the
> driver callback?  I just wrote that it should *not*.
> 

Sorry, I wasn't clear; you are right with respect that this change will break Intel's x540 driver.

However, I do think that this is the purpose of xdo_dev_state_add(). Again, As far as I can understand, and please correct me if I'm wrong, this shouldn’t be here in the first place.

Please have a look at mlx5e_xfrm_validate_state(). Currently, it return an error if the user requests ESN, regardless of the underlying device's capabilities. Subsequent patch to mlx5 driver, will allow such a request if the device does support it; maintaining backward compatibility.

Here is a code snippet:

-       if (x->props.flags & XFRM_STATE_ESN) {
+       if (x->props.flags & XFRM_STATE_ESN &&
+           !(mlx5_accel_ipsec_device_caps(priv->mdev) & MLX5_ACCEL_IPSEC_ESN)) {
                netdev_info(netdev, "Cannot offload ESN xfrm states\n");
                return -EINVAL;
        }

> This code seems to be assuming that all drivers/NICs with the offload
> will be able to do ESN, and this is not the case.  If this code is put
> into place, suddenly the ixgbe driver's offload will have a failure
> case: the driver doesn't support ESN, and doesn't know to NAK the
> state_add if the ESN bit is on.  This is a generic capabilities issue
> for which we already have a solution "pattern".
> 

We weren't assuming that, please see above.

>  > What do you suggest?
>  >
> 
> There should be a capabilities/feature flag for the driver to set and
> the XFRM code shouldn't try the state_add with ESN if the driver hasn't
> set an ESN bit in its capabilities.  Other capabilities that might make
> sense here are IPv6, TSO, and CSUM; there may be others.
> 
> >> 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?
> 
> Alternatively, just solve this by failing to add the SA that has ESN set
> if the driver hasn't defined your new xdo_dev_state_advance_esn().
> 
> sln
> 
> 
> >
> >> 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);

Powered by blists - more mailing lists