> From: Shannon Nelson [mailto:shannon.nelson@xxxxxxxxxx] > 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@xxxxxxxxxx> wrote: > >> > >>> On 1/10/2018 2:34 AM, yossefe@xxxxxxxxxxxx wrote: > >>> From: Yossef Efraim <yossefe@xxxxxxxxxxxx> > >>> This patch adds ESN support to IPsec device offload. > >>> Adding new xfrm device operation to synchronize device ESN. > >>> Signed-off-by: Yossef Efraim <yossefe@xxxxxxxxxxxx> > >>> --- > >>> 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); ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
