lkml.org 
[lkml]   [2018]   [Jan]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH net-next v2] xfrm: Add ESN support for IPSec HW offload
From
Date

On 1/11/2018 10:28 AM, Yossi Kuperman wrote:
>> From: Shannon Nelson [mailto:shannon.nelson@oracle.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@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*.
>>
> 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".
>>
I guess you are right but ixgbe driver is already checking many other
caps during add_sa callback (below code from v3 patches for ixgbe ipsec):

+ if (xs->id.proto != IPPROTO_ESP && xs->id.proto != IPPROTO_AH) {
+ netdev_err(dev, "Unsupported protocol 0x%04x for ipsec offload\n",
+ xs->id.proto);
+ return -EINVAL;
+ }
+
+ if (xs->xso.flags & XFRM_OFFLOAD_INBOUND) {
+ struct rx_sa rsa;
+
+ if (xs->calg) {
+ netdev_err(dev, "Compression offload not supported\n");
+ return -EINVAL;
+ }


What is the difference for checking xs->calg exists in state to ESN?

I think in long term we can refactor to cap mask declaration by the
driver and call add_sa only if mask exists but
this can be a totally different patch.


> 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);

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