From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 430E1A00C2; Thu, 23 Apr 2020 12:54:46 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id AC0051C237; Thu, 23 Apr 2020 12:54:45 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 9562D1C222 for ; Thu, 23 Apr 2020 12:54:43 +0200 (CEST) IronPort-SDR: 9TgZ+eUspr0XLcZwIgJ0K1//IIB8PEKhX7QsubgcNSSyxv2LS99JsJDX7OKwI0sVKTbWiU1/XG CO+ip4m06D4Q== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2020 03:54:42 -0700 IronPort-SDR: FnbGJZBg9BQdjZ0mBASR6uz3pZX0klzJYLu40xFmQiP/BRS/dMu9/9kHsoLKeJTohg6WkHh8xF dJXOXxHNTfTg== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,307,1583222400"; d="scan'208";a="280362273" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by fmsmga004.fm.intel.com with ESMTP; 23 Apr 2020 03:54:42 -0700 Received: from orsmsx126.amr.corp.intel.com (10.22.240.126) by ORSMSX109.amr.corp.intel.com (10.22.240.7) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 23 Apr 2020 03:54:42 -0700 Received: from ORSEDG002.ED.cps.intel.com (10.7.248.5) by ORSMSX126.amr.corp.intel.com (10.22.240.126) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 23 Apr 2020 03:54:42 -0700 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (104.47.56.173) by edgegateway.intel.com (134.134.137.101) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 23 Apr 2020 03:54:42 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=RDmSTDpt6TXVd+rOHqCh2o1DeFow6N0CHwRYQsLnlSjkMNhXwIDb/2jvDKWqs1vsFdgtGVL6GXYZt+1qNj+kFVJgqTQ5fv8QqD1K0iihMIJ64NK1PD0FhZ8QR5tTjIvtOrcBzWjfPaYlD7zAAxDSN0aw70bIsQ2Jv8RMN2GXfF6oelwi/6LIVJAYe9syq0VF0vYscFu3w9qIuCfW65NZwAULc6WXOk11zu0Ulf1LD2iTl2aF0Cm1KGkJdkI4uaTCQX1SVDUgxoE5zzFFlC7yyVTE1TAXCXG3hEMghLJKwEYkvirlf6FBifb9kxYhcoG1X9pGSg58cjevyNFlKSAdSA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=AIL1t7BGP3SWbbMpgOD3EqO3MgTEPmpEDl3TZWDsq78=; b=O1iLtNSMuX9lMh0uFjimmzFxZQqxq9oNRFtmW6JRGM4Fsig2oyFsIJDaiZ6GRJbSkU80kMcDuinD2Yh/sOiqUn1mQ9gz9CoIMjAtik0aQbqb0QDD4qPwfaOFyF7Qz6jUmIcVpLKnN4yHGBmrDBpVO2TQ5CvLGiZeaU43eI4cac1Kw12s9mY/gxiXHMxHsonerKZEfA5rbEGrbyBlLCGq9zBbl3AXTJF5YRj+EeqA2WiPNmatc03bvLggiV3nkNpDkj/WbXE1LGcc2IkXxNStJKCI+/IbJ4T221DSpFdbbmdn0dDhhpQi7yE+aN4bEH6hJePAu505wyvYWqORiyTREQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=intel.onmicrosoft.com; s=selector2-intel-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=AIL1t7BGP3SWbbMpgOD3EqO3MgTEPmpEDl3TZWDsq78=; b=A4bF+vrjLivWiByjCcbx/aSB0C4AXbdHZiQISGRkg6fev+tpe/qyD+byJKYelF3ys0bovGPpb+ifdv/PmMO0HgYG+9SA1f6SsXEQ83mZdNKJsLJnAZHEehl+NE1MIpmYN68hVVbE9/KpFHBiYa4SJp/JQmtYgaOJVzLav6vDBYI= Received: from BYAPR11MB3301.namprd11.prod.outlook.com (2603:10b6:a03:7f::26) by BYAPR11MB3480.namprd11.prod.outlook.com (2603:10b6:a03:79::27) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2937.13; Thu, 23 Apr 2020 10:54:40 +0000 Received: from BYAPR11MB3301.namprd11.prod.outlook.com ([fe80::f8cb:58cd:e958:fff4]) by BYAPR11MB3301.namprd11.prod.outlook.com ([fe80::f8cb:58cd:e958:fff4%6]) with mapi id 15.20.2937.020; Thu, 23 Apr 2020 10:54:40 +0000 From: "Ananyev, Konstantin" To: Anoob Joseph , "dev@dpdk.org" , "Lukasz Wojciechowski" CC: "akhil.goyal@nxp.com" , "Doherty, Declan" Thread-Topic: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops Thread-Index: AQHWGSTLO6APWAgcfEqbxThOaNZf1KiGUA6ggAAbjICAABhdIA== Date: Thu, 23 Apr 2020 10:54:40 +0000 Message-ID: References: <20200422235158.24497-1-konstantin.ananyev@intel.com> In-Reply-To: Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.2.0.6 authentication-results: spf=none (sender IP is ) smtp.mailfrom=konstantin.ananyev@intel.com; x-originating-ip: [192.198.151.169] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 2470e409-bf04-4a74-5a4e-08d7e774b962 x-ms-traffictypediagnostic: BYAPR11MB3480: x-ld-processed: 46c98d88-e344-4ed4-8496-4ed7712e255d,ExtAddr x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:2276; x-forefront-prvs: 03827AF76E x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BYAPR11MB3301.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(376002)(39860400002)(136003)(346002)(366004)(396003)(7696005)(4326008)(33656002)(81156014)(107886003)(19627235002)(54906003)(8936002)(71200400001)(86362001)(110136005)(6506007)(53546011)(316002)(186003)(55016002)(15650500001)(9686003)(5660300002)(26005)(966005)(52536014)(76116006)(2906002)(66946007)(478600001)(64756008)(66446008)(66476007)(66556008); DIR:OUT; SFP:1102; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: JlOy538BT4tH87NcfLB0E+/7aBIV3RfrOt2jfg6gWov5dFbJgZRAGK9uibG4cJEYK8TcUWBZhFLs9T5hklRlGJjEwc7p/Fyv2ov9vgBYAI1hSX34YCCXODWFfk7y58lB5z0fXaKszdiPeErKFuivxKzQX51newCBaT6Nc1YWLZSe5Qf1qqa9NRPyf2o95Iw1QEMZvEm69cQEegROPVooQXlsLHMPX6utUpfS2bGRf592ZFX1ajyC5T2vvaUN2wsedZaPvJAU/K9NUsXNpWatioitwGTKltMXqLCG+LIoucJV8sh8LGpdrwPn0NTkb+iYlKXREc89wYRyrVLPUXMCn+aw7Qc8t8njRa1DnZ0cUkcALOzrhAf+K7aUEyrx7S+kcF47go1xj4MmwVWDnsXCf9luwSzYOWbeD0BfGxoYFgb563LJlzmWHVpJkRbgoGDB7nPVEK/qmwfk42Ill9G9KEelnD7RdBNvDXQzvAXcxAfbRKiQI435x5AqwtUWGfqTl1ntTU2EQIGWrLrGvlAqQQ== x-ms-exchange-antispam-messagedata: 0Bfbk0G7tbrWf6meMOQ2k5KtyLJwBStsNo/Gy6F64LWs8io5STXsxxACpdwxbLfL5Ugcb8E1bXVKH4DVXWVvEkwNglWouXIMKpbZO6De2ZKdond+5cJ1TriwBHd28CwvwIXJxEF5HQ5hs80BdCz3bQ== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 2470e409-bf04-4a74-5a4e-08d7e774b962 X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Apr 2020 10:54:40.5929 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: u8wDMo8Twjk7HIXl7PlXBYN3yNiJeM6hLlgTWDAqZJMPtYrMfx6/gwX7fNrZ1LZk0ICR2RSUyo/00Taf8QZ1KmxHWnZ5IbqLefMny5Q74HE= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR11MB3480 X-OriginatorOrg: intel.com Subject: Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > > > > > > These are data path ops and so it will be better if we can avoid such= checks in > > the datapath. The same is done in ethdev also. > > > > AFAIK, get_userdata is an *optional* dev-ops function that can be used= by data- > > path. > > So far there was no strict requirement for the rte_security PMDs to *al= ways* > > implement it. >=20 > [Anoob] I don't think DPDK categorizes dev-ops as *optional* and *always*= . If yes, can you point me? > My understanding is, all ops are optional. For example, I could implement= a crypto PMD which is doing packet delivery only via event device > (using crypto adapter). So dequeue op will not be implemented in that cas= e and DPDK spec allows it. Your PMD can have enqueue_burst/dequeue_burst as NOP, but you still have to provide valid function pointers: they are stored inside crypto_dev structure itself and will be called unconditionally (without any extra checking) by rte_cryptodev_enqueue_burst/rte_cryptodev_dequeue_burst. For all other calls (both data and control path) there is a check that actual function pointer is a valid one. Same story for eth dev: pkt_rx_burst/pkt_tx_burst and rest of dev-ops. =20 > > So what you guys did is a silent change of public API behaviour. >=20 > [Anoob] I believe Lukasz had submitted 3 or 4 revisions and it was all in= the ML. RTE_DEBUG was suggested by Thomas I guess. I believe it is not a right procedure to change existing behaviour of rte_s= ecurity framework. I think you have to communicate clear and loudly in advance (at least one r= elease in advance). Plus RTE_DEBUG has nothing to do with changing non-debug behaviour. =20 > > As result ixgbe, (and probably some others rte_security PMDs) stopped w= orking > > properly. >=20 > [Anoob] set_pkt_metadata() is the only one of interest to IXGBE. And I be= lieve the function is implemented as well. So what exactly is the > concern? Check that ops->get_userdata is a valid function pointer will be compiled o= ut. So PMDs that don't implement this function will crash in rte_security_get_u= serdata(). In our particular case - ixgbe. Same story with rte_security_set_pkt_metadata() - see the patch.=20 >=20 > > I don't see any point in these changes, but if you'd like to do that, a= t least our > > usual procedure has to be followed: > > 1. Send and RFC to get an agreement with rte_security PMDs maintainers = (one > > release ahead) 2. send a deprecation note (one release ahead) 3. change= the > > behaviour of the public API 4. update release notes > > > > AFAIK 1), 2), 4) wasn't done. > > So I think right now we need to revert original behaviour. > > > > > > > > https://urldefense.proofpoint.com/v2/url?u=3Dhttp-3A__code.dpdk.org_d= pdk > > > _v20.02_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4372&d=3DDwIFAg&= c=3Dn > > > KjWec2b6R0mOyPaz7xtfQ&r=3DjPfB8rwwviRSxyLWs2n6B- > > WYLn1v9SyTMrT5EQqh2TU&m=3D > > > 6ObfSanVVuHOsiqVlWxXsFWi- > > 2XNp76HCOX0vbUfma4&s=3DjDVyDDEILmgY1Yb9ZBswBVbn > > > 8FpZuQI5ukH_osmtUiI&e=3D > > > > > > Datapath functions in cryptodev (enqueue/dequeue) doesn't even have s= uch > > checks. > > > https://urldefense.proofpoint.com/v2/url?u=3Dhttp-3A__code.dpdk.org_d= pdk > > > _v20.02_source_lib_librte-5Fcryptodev_rte-5Fcryptodev.h-23L962&d=3DDw= IFA > > > g&c=3DnKjWec2b6R0mOyPaz7xtfQ&r=3DjPfB8rwwviRSxyLWs2n6B- > > WYLn1v9SyTMrT5EQqh2 > > > TU&m=3D6ObfSanVVuHOsiqVlWxXsFWi- > > 2XNp76HCOX0vbUfma4&s=3DLEWQOKs0r2Im_zL95VI > > > df4kQ2Pu0iRHV9Co2J1gsNBE&e=3D > > > > That's a different story: > > rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions that= have > > to be implemented by each ethdev/cryptodev API. >=20 > [Anoob] I couldn't find any reference stating that way. If you can point = me, I can update that to include datapath ops required for inline > protocol processing. Look at the code. >=20 > > > > > > > > > > > Thanks, > > > Anoob > > > > > > > -----Original Message----- > > > > From: dev On Behalf Of Konstantin Ananyev > > > > Sent: Thursday, April 23, 2020 5:22 AM > > > > To: dev@dpdk.org > > > > Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin > > > > Ananyev > > > > Subject: [dpdk-dev] [PATCH] security: fix crash at accessing > > > > non-implemented ops > > > > > > > > Valid checks for optional function pointers inside dev-ops were > > > > disabled by undefined macro. > > > > > > > > Fixes: b6ee98547847 ("security: fix verification of parameters") > > > > > > > > Signed-off-by: Konstantin Ananyev > > > > --- > > > > lib/librte_security/rte_security.c | 4 ---- > > > > 1 file changed, 4 deletions(-) > > > > > > > > diff --git a/lib/librte_security/rte_security.c > > > > b/lib/librte_security/rte_security.c > > > > index d475b0977..b65430ce2 100644 > > > > --- a/lib/librte_security/rte_security.c > > > > +++ b/lib/librte_security/rte_security.c > > > > @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct > > > > rte_security_ctx *instance, > > > > struct rte_security_session *sess, > > > > struct rte_mbuf *m, void *params) { -#ifdef > > RTE_DEBUG > > > > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, - > > > > EINVAL, > > > > -ENOTSUP); > > > > RTE_PTR_OR_ERR_RET(sess, -EINVAL); -#endif > > > > return instance->ops->set_pkt_metadata(instance->device, > > > > sess, m, params); > > > > } > > > > @@ -121,9 +119,7 @@ rte_security_get_userdata(struct > > > > rte_security_ctx *instance, uint64_t md) { > > > > void *userdata =3D NULL; > > > > > > > > -#ifdef RTE_DEBUG > > > > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, > > > > NULL); -#endif > > > > if (instance->ops->get_userdata(instance->device, md, &userdata)) > > > > return NULL; > > > > > > > > -- > > > > 2.17.1