Messages in this thread | | | Subject | Re: [PATCH net-next v2] xfrm: Add ESN support for IPSec HW offload | From | Shannon Nelson <> | Date | Wed, 10 Jan 2018 19:20:46 -0800 |
| |
On 1/10/2018 3:09 PM, Yossi Kuperman wrote: >> 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/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*.
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".
> 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);
| |