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 E1E5BA00C2; Thu, 23 Apr 2020 13:23:58 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 449E31C2EC; Thu, 23 Apr 2020 13:23:58 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0a-0016f401.pphosted.com [67.231.148.174]) by dpdk.org (Postfix) with ESMTP id 7BF121C222 for ; Thu, 23 Apr 2020 13:23:56 +0200 (CEST) Received: from pps.filterd (m0045849.ppops.net [127.0.0.1]) by mx0a-0016f401.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 03NBKMk5002736; Thu, 23 Apr 2020 04:23:55 -0700 Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-0016f401.pphosted.com with ESMTP id 30fxwppn6r-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 23 Apr 2020 04:23:55 -0700 Received: from m0045849.ppops.net (m0045849.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 03NBNsDG005831; Thu, 23 Apr 2020 04:23:54 -0700 Received: from sc-exch04.marvell.com ([199.233.58.184]) by mx0a-0016f401.pphosted.com with ESMTP id 30fxwppn6m-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Thu, 23 Apr 2020 04:23:54 -0700 Received: from SC-EXCH04.marvell.com (10.93.176.84) by SC-EXCH04.marvell.com (10.93.176.84) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Thu, 23 Apr 2020 04:23:53 -0700 Received: from NAM12-DM6-obe.outbound.protection.outlook.com (104.47.59.169) by SC-EXCH04.marvell.com (10.93.176.84) with Microsoft SMTP Server (TLS) id 15.0.1497.2 via Frontend Transport; Thu, 23 Apr 2020 04:23:53 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Xcf+u0mWhnO4WUA9F00Y4kPFvuV/i3kw+C1KGmmAdt5wPtoQcUKMnrU9Gs52qQn1Z/WFftG3cBp0F9iDFB8FzLYoIFB5AeCJr+D7xy0F7bfGLPE+wvkRt/31Qz7SYgsFgcVD9TmxZQGLd+hMHWmNRx0mwCUWV/XbO1RxSiuoq6WFDfFJgQcLFLM0jKeLFSC0y8tyjohz7KZO4gdCeJEkigdYU0vB9wPGQkHmSL3vHEM47kbgyadaNfCRuGkvqSeA0HJ1iReze7TQmtL+qRPUis5BtUUdoPJ7tZoSc1FkcyXNY2F/c20TpbRxxsfu7EiVUxCO4e3g99NbVCQKgNzPCA== 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=OduKOTkPRM2V1hKK0JS1GX7BzIJSQ92KyGETdV1FyZs=; b=gAHynIjTfQqwZln2sc/NqFRxw/mGo4hzXl3DhO9g6oTY0FrcENZoWb4fUAbb8gRxKkKIgfJVwUXs3fuI9R0ccn04u9Mnp8A4w8FF7tDZlAvIVpPlnF4JpZx+L2BOcqpIVOiGr2F+eQMLERRJwltXNZUzAxCTjKp7JZKmlMBTVPuG3Y01nz+mh2fD4eojK5io0ocSlv5TOQYlapf7elK2b6xmjMrlr0XVcw8aQezvBQKMLE/Jhti80pTRtAi79OJxXw6ErXgpK6fK9S7ohqJfSsHcb7FFHuuXk7bQoNg1cBkEZVlvF1352jsxjp3uFJ4OTa6nq/ooINq/VKd+F079+g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=marvell.com; dmarc=pass action=none header.from=marvell.com; dkim=pass header.d=marvell.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.onmicrosoft.com; s=selector1-marvell-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=OduKOTkPRM2V1hKK0JS1GX7BzIJSQ92KyGETdV1FyZs=; b=gSW7Ak/7ajsmqddCiE+3zadkuURgnj3svgXToQGiRxXBnq45tUeImrL73K7PB3h0qPiSS189Ch1T/Z7ElLjVkcOh9ei9b4Iz4tY1Wr/DrJQ2KuyqgRINfjEDuW8OX14toNShczG2/Mn4szzrWW1P+WOgfVkmQasQTgXXLd+U3XQ= Received: from MN2PR18MB2877.namprd18.prod.outlook.com (2603:10b6:208:3b::26) by MN2PR18MB2368.namprd18.prod.outlook.com (2603:10b6:208:ac::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2921.25; Thu, 23 Apr 2020 11:23:51 +0000 Received: from MN2PR18MB2877.namprd18.prod.outlook.com ([fe80::648f:e7fa:f95e:191b]) by MN2PR18MB2877.namprd18.prod.outlook.com ([fe80::648f:e7fa:f95e:191b%2]) with mapi id 15.20.2921.030; Thu, 23 Apr 2020 11:23:51 +0000 From: Anoob Joseph To: "Ananyev, Konstantin" , "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: AQHWGQET5ssCrfrdCkGeU0EkO7BVz6iGFf8AgABAugCAABAnMIAAIkoAgAAAnxA= Date: Thu, 23 Apr 2020 11:23:50 +0000 Message-ID: References: <20200422235158.24497-1-konstantin.ananyev@intel.com> In-Reply-To: Accept-Language: en-IN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [27.34.246.85] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 073fe065-bc04-451e-adf2-08d7e778cc95 x-ms-traffictypediagnostic: MN2PR18MB2368: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:2958; x-forefront-prvs: 03827AF76E x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:MN2PR18MB2877.namprd18.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(10009020)(4636009)(346002)(396003)(366004)(136003)(376002)(39860400002)(71200400001)(4326008)(66946007)(66446008)(64756008)(66556008)(66476007)(26005)(86362001)(53546011)(7696005)(966005)(186003)(478600001)(81156014)(6506007)(15650500001)(9686003)(55016002)(8936002)(76116006)(316002)(110136005)(5660300002)(2906002)(52536014)(33656002)(54906003)(19627235002); DIR:OUT; SFP:1101; received-spf: None (protection.outlook.com: marvell.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: qhe3Gha1oHT4u5BSxpRZ2o0OdCaRmD7Mv1sJjHrOQRsP8TSIR6YoerpurmRdalmUKCquIbcuNpwLubemuOPoYTpUhFKZqr0zkCXoRGo3zIWORhPKw2BwjFt6wpJDhyzu7AgFGILaD2C8UF51cOiG5/mn/Tndg9y7nTUuW0LB4vnRh25UFjpcck9822H9ahTwKbWmsAamDFD6B9LYkuI4eebLc3IQAWowR0q5pI37FsH0dB1U1SNGkcpnHFH9a/NSRN7WXfxXleFk6D9LjWypa6XiDyvsJtMo4cuZpB5w4b4jdAOaVd2V9beNXqq5oAPFNLjTwZZJv4IK/Nc8ufm7ZhR5M43st+W6H0JHnmoLDOpPYH6LfgW4n0owW+s5c/7sKwbJcizzdepHvaaJd86EVKTXDkb42me7jdsCrg6Xgj/ZnlY6drH3Ua+s8E/PpA//oZuV1VOG+wBnH+c5FLepgXTseHo7I489YkBo5K6l47y1nRcholTn3JrcnHwZNyDv7MzK3am3LwSJ8n77ZWImaQ== x-ms-exchange-antispam-messagedata: GukmkXd+FxziGygp/4kU006rejw9rGpu4MRYzjO54GltWb3SyzrJk6RfxUbYn1Xbl3X+/765nnf3KZ0r4RXUxdTSYfe8hSMMSh04JONNk2fLtLqZ/FGdxTSjDQUJryghi+3JqE1qS9duGJcA3mSqxA== x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 073fe065-bc04-451e-adf2-08d7e778cc95 X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Apr 2020 11:23:50.9538 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: knHM4g+3ihDgIu6JeLFVFjH/EwEnu0i47TkLw6KrjYv60t/kl6jn+nhKAIGonn+AchkHMWHZ78i4wvU/btiaxQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR18MB2368 X-OriginatorOrg: marvell.com X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138, 18.0.676 definitions=2020-04-23_07:2020-04-22, 2020-04-23 signatures=0 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" Hi Konstantin, Please see inline. Thanks, Anoob > -----Original Message----- > From: Ananyev, Konstantin > Sent: Thursday, April 23, 2020 4:25 PM > To: Anoob Joseph ; dev@dpdk.org; Lukasz > Wojciechowski > Cc: akhil.goyal@nxp.com; Doherty, Declan > Subject: [EXT] RE: [dpdk-dev] [PATCH] security: fix crash at accessing no= n- > implemented ops >=20 > External Email >=20 > ---------------------------------------------------------------------- >=20 > > > > > > > > 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 > > > *always* implement it. > > > > [Anoob] I don't think DPDK categorizes dev-ops as *optional* and *alway= s*. If > yes, can you point me? >=20 > > My understanding is, all ops are optional. For example, I could > > implement a crypto PMD which is doing packet delivery only via event de= vice > (using crypto adapter). So dequeue op will not be implemented in that cas= e and > DPDK spec allows it. >=20 > 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. [Anoob] I think there is a basic misunderstanding here. You are treating un= conditional calls as mandatory implementations. If that is the case rte_eth= _tx_burst() & rte_eth_rx_burst() shouldn't check for function pointers even= when DEBUG is enabled. static inline uint16_t rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id, struct rte_mbuf **rx_pkts, const uint16_t nb_pkts) { struct rte_eth_dev *dev =3D &rte_eth_devices[port_id]; uint16_t nb_rx; #ifdef RTE_LIBRTE_ETHDEV_DEBUG RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0); RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0); if (queue_id >=3D dev->data->nb_rx_queues) { RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=3D%u\n", queue_id); return 0; } #endif nb_rx =3D (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id], rx_pkts, nb_pkts); >From my view point, function pointer checks and argument checks are require= d in every API for stability. But having such checks in the datapath advers= ely affects the performance. And for cases where function pointers are not = set, application would get one crash in the first run. And that can be debu= gged after having the required options enabled. =20 > For all other calls (both data and control path) there is a check that ac= tual > 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. > > > > [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. >=20 > I believe it is not a right procedure to change existing behaviour of rte= _security > framework. > I think you have to communicate clear and loudly in advance (at least one > release 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 working properly. > > > > [Anoob] set_pkt_metadata() is the only one of interest to IXGBE. And I > > believe the function is implemented as well. So what exactly is the con= cern? >=20 > Check that ops->get_userdata is a valid function pointer will be compiled= out. > So PMDs that don't implement this function will crash in > rte_security_get_userdata(). > In our particular case - ixgbe. > Same story with rte_security_set_pkt_metadata() - see the patch. [Anoob] But ixgbe doesn't implement inline protocol which is the primary co= nsumer of this API (rte_security_get_userdata()). So what is the trouble?=20 Also, application is expected to call rte_security_set_pkt_metadata() only = on devices with offload flag RTE_SECURITY_TX_OLOAD_NEED_MDATA. If a PMD sta= tes it needs MDATA but fails to register a function pointer for doing the s= ame, it is a control path problem. Checking for that in the datapath is an = overkill. >=20 > > > > > I don't see any point in these changes, but if you'd like to do > > > that, at 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= _ > > > > dpdk > > > > _v20.02_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4372&d=3DDwIFA= g > > > > &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 such > > > checks. > > > > https://urldefense.proofpoint.com/v2/url?u=3Dhttp-3A__code.dpdk.org= _ > > > > dpdk > > > > _v20.02_source_lib_librte-5Fcryptodev_rte-5Fcryptodev.h-23L962&d=3D= D > > > > wIFA > > > > 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. > > > > [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. >=20 > Look at the code. [Anoob] Code is not conclusive for this as I've explained above for rte_eth= _rx/tx_burst() case. =20 >=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