lkml.org 
[lkml]   [2018]   [Jan]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH net-next v2] xfrm: Add ESN support for IPSec HW offload
From
Date
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);

\
 
 \ /
  Last update: 2018-01-14 23:22    [W:0.072 / U:0.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site