Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932881AbeAKNwD (ORCPT + 1 other); Thu, 11 Jan 2018 08:52:03 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:45455 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754207AbeAKNwB (ORCPT ); Thu, 11 Jan 2018 08:52:01 -0500 X-Google-Smtp-Source: ACJfBovXuP7d6Qx5eJqUlBFCPfxPInPiEDAellVFt21dXsFnp3SKbiLDx0LUmAWl4UOPezMVMMDAkw== Subject: Re: [PATCH net-next v2] xfrm: Add ESN support for IPSec HW offload To: Yossi Kuperman , Shannon Nelson Cc: Yossef Efraim , Jonathan Corbet , "David S. Miller" , Steffen Klassert , Herbert Xu , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "netdev@vger.kernel.org" , Boris Pismenny , Yevgeny Kliteynik References: <1515580453-18470-1-git-send-email-yossefe@mellanox.com> From: Aviad Yehezkel Message-ID: Date: Thu, 11 Jan 2018 15:51:56 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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 wrote: >>>> >>>>> On 1/10/2018 2:34 AM, yossefe@mellanox.com wrote: >>>>> From: Yossef Efraim >>>>> This patch adds ESN support to IPsec device offload. >>>>> Adding new xfrm device operation to synchronize device ESN. >>>>> Signed-off-by: Yossef Efraim >>>>> --- >>>>> 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);