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*.
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
