Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753528AbeAKDVP (ORCPT + 1 other); Wed, 10 Jan 2018 22:21:15 -0500 Received: from userp2130.oracle.com ([156.151.31.86]:47076 "EHLO userp2130.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753169AbeAKDVN (ORCPT ); Wed, 10 Jan 2018 22:21:13 -0500 Subject: Re: [PATCH net-next v2] xfrm: Add ESN support for IPSec HW offload To: Yossi Kuperman 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: Shannon Nelson Organization: Oracle Corporation Message-ID: Date: Wed, 10 Jan 2018 19:20:46 -0800 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-Language: en-US Content-Transfer-Encoding: 8bit X-Proofpoint-Virus-Version: vendor=nai engine=5900 definitions=8770 signatures=668652 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0 phishscore=0 bulkscore=0 spamscore=0 mlxscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1801110040 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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*. 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);