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 AC3CFA00C4; Thu, 23 Apr 2020 16:08:37 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 1AD101B13C; Thu, 23 Apr 2020 16:08:37 +0200 (CEST) Received: from EUR05-DB8-obe.outbound.protection.outlook.com (mail-db8eur05on2084.outbound.protection.outlook.com [40.107.20.84]) by dpdk.org (Postfix) with ESMTP id 6F0584CBD for ; Thu, 23 Apr 2020 16:08:35 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NXOMPR4r37lpu8q4ISIUSnY3cY9Rr+IA20MSgtuJUiA+318/5vnXyMm2O+/hOtwTEE/maKvJQ3t38+7v5NhkHMOlw3Cw1m0NvvaCKo4WBAY5OADj/RUV5+z2IPCJ5OFBmtJrr6irvXTfr5+Jiu7GAjr9i6bKVGaB5W5e/B1friH/t4fPx2dsaQu2/C+FmK2kRRBXxoRb6HTsKksE0+wr77WLcOMu8P6F3EUA/a85KvwmlAtk/S+IEU1MPBV2MDRGixzatfdwYOFdn8Krw7q/6wgfT3tHn6M0W872Is5yShCVCrd1C4uS5wyGcYg0aG8j/W0Aylic75suN2HMoBWUhg== 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=PY9DR5FcBGoQ3SZ62d1x1M52sOZDt+QeRLeKi4iDNd4=; b=Scha2Moukq0JKasv8BeH1h2weL79uTWz21ptEX7zXBvwU2b7G4/dcFQqKnk/Hg6MPh4PQOzqAJbHbKy/E9yfhLCSPY+tJHHC5UBWIBQqbQttA7S47Q2X4pBgeyKLhZRylOapQ/D8fjGxmJaLIiPZE6sWb4bq3EBQtPqOBje1lWePw9Ep4oZeyuIrde8NVDDu8vSJXOZHhIR4rCvEKAxVU2uvMz0gdRAxcr9rKg9sTcMjNY+IFKiPS6E3E3Asc1iinL2V+DoPIOX9ZhYFc3ViWafy/bbXd9m6Bl2ZGWGc5G1rdw4W7R34lIris7q20IqZkWlXUQbJfiiyjMM9N7CWfA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nxp.com; dmarc=pass action=none header.from=nxp.com; dkim=pass header.d=nxp.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nxp.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=PY9DR5FcBGoQ3SZ62d1x1M52sOZDt+QeRLeKi4iDNd4=; b=merKlln6mcNxIsUNTam2kq3+kjCwK9a5bSSSooHQqEqTfqg8ih8bO+1+lXgEluGdDV7baNBdtaho2MFvqOBDtahANNkZJCPvqJiBUVWNhHrh5057FRkgOIZ+m2Yn2KXxqXb8cgDYNTnHZMPAKY031WAxIfJFP+K0rfrbtN5qUmw= Received: from VI1PR04MB3168.eurprd04.prod.outlook.com (2603:10a6:802:6::10) by VI1PR04MB3136.eurprd04.prod.outlook.com (2603:10a6:802:10::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2921.29; Thu, 23 Apr 2020 14:08:33 +0000 Received: from VI1PR04MB3168.eurprd04.prod.outlook.com ([fe80::8c03:2f5:3b48:ba74]) by VI1PR04MB3168.eurprd04.prod.outlook.com ([fe80::8c03:2f5:3b48:ba74%7]) with mapi id 15.20.2921.030; Thu, 23 Apr 2020 14:08:33 +0000 From: Akhil Goyal To: "Ananyev, Konstantin" , Anoob Joseph , "dev@dpdk.org" , Lukasz Wojciechowski CC: "Doherty, Declan" Thread-Topic: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops Thread-Index: AQHWGSSzXmaTexK7xEqwxn6qrened6iGVnIAgAAVKICAAB1JAIAACCYAgAAO16CAABlMgIAAAM4Q Date: Thu, 23 Apr 2020 14:08:33 +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: authentication-results: spf=none (sender IP is ) smtp.mailfrom=akhil.goyal@nxp.com; x-originating-ip: [45.118.167.83] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: cadbf1a9-b6a4-4923-fe25-08d7e78fcf14 x-ms-traffictypediagnostic: VI1PR04MB3136: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 03827AF76E x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:VI1PR04MB3168.eurprd04.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(10009020)(4636009)(346002)(396003)(366004)(376002)(39860400002)(136003)(4326008)(5660300002)(9686003)(478600001)(316002)(52536014)(110136005)(55016002)(15650500001)(44832011)(66476007)(64756008)(71200400001)(66946007)(2906002)(66556008)(66446008)(81156014)(33656002)(76116006)(7696005)(26005)(8936002)(186003)(86362001)(6506007); DIR:OUT; SFP:1101; received-spf: None (protection.outlook.com: nxp.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: glpHMFqezR3gkGxUaJXXucty+H3xplWO6DhzXLKNY+hU/UG4BS4XHmraaFJBF9qAiJriyp/uJ13iwyC+HgwMyHEqGwh2SwCe5hWdP1f3N7l7UxSkTK1xgtm96y9HkveCe84M5Z2UuChm+4/Nm8mCtVFPtoRsGjLtXz3jEPDSz6woXQjY0h8gLBhX9Pz6FlefjuWm9KVXi6UEw3eLFxaHk2EotApkuASJPjV8U/wl2KbzMdG21391cSf5YLTDEeBwNhK4IP6fYdFgBsFoshyNI+1jeXJdXwtbFykR89aciSuggOGWNVz/4CnmEUHKIbf6AV2pXHZx9nDtY5jq6HdHN6wWxL1LUBK8JtnT09oriHZD2EZ2fCzoi6SPxHyZWSYsdSiydz3186MDnoK5RXLJuYfidRrGypAF/QOn9BYUil0wx2/ZrnFNwqvXg71Udsce x-ms-exchange-antispam-messagedata: XkJKOS07FkatIz6tcTy3h5PxkGRcW2tTJzREUk0IeERb01bPJDLUoaSA3eG1iacmJhhHL4gYTq3HvULqx+VuPsS7/O6n8nCoPTUbxYembuWlKrw5pVhQWbARzNfbIREMp7Rf1ltfNIxr1t/mm08dbw== x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: nxp.com X-MS-Exchange-CrossTenant-Network-Message-Id: cadbf1a9-b6a4-4923-fe25-08d7e78fcf14 X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Apr 2020 14:08:33.5704 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: uisr2Ji1wdp3xPbRuXk1aciXbwVXvJIx7ei+aF/4MdXUgTTYXulNCN3NsqZ+gYrXf8/VMIPvErWH9/UHN4xQmg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB3136 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" >=20 > Hi Akhil, >=20 > > > > Hi Anoob/Konstantin, > > > > > > > > Check that ops->get_userdata is a valid function pointer will be co= mpiled > 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 prim= ary > > > consumer of this API (rte_security_get_userdata()). So what is the tr= ouble? > > > > > > 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 > > > states it needs MDATA but fails to register a function pointer for do= ing the > same, > > > it is a control path problem. Checking for that in the datapath is an= overkill. > > > > > Whatever your concern is, we can resolve it later, but for now we shoul= d have > the same > > Unconditional checks that were there earlier. We need to make RC1 > today/tomorrow. > > And this cannot go as an issue. > > > > These are optional APIs and every PMD may not have supported that. > > > > Konstantin, > > Please send an update to your patch reverting the original patch for th= ese 2 > functions. > > Currently it is adding 2 extra checks. > > >=20 > I am afraid we can't do just that. > As in that case /app/test/test_security.c build wih -DRE_DEBUG will start > crashing. >=20 > I think we have 3 alternative how to fix it: >=20 > 1. Keep all these 3 checks for debug and non-debug mode (that what my cur= rent > patch does). > 2. Have both: existed 1 check in non-debug mode, plus new checks in debug > mode, i.e.: > rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md) > { > void *userdata =3D NULL; >=20 > +#ifdef RTE_DEBUG > + RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, > NULL); > +#else > RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL); > +#endif >=20 > .... >=20 > 3. Keep only 1 existed check in non-debug mode and remove cases in > app/test/test_security.c > that would crash with -DRTE_DEBUG. >=20 > My preference is 1), I don't think these 2 extra checks will affect perfo= rmance > greatly. > Also with 1) we can make these new test-case to be executed for non-debug > mode too. > 2) is probably also ok - but I think RTE_DEBUG concept should be a separa= te > patch series, > and I don't want to mix things. > What is your opinion here? >=20 I am OK with both 1 and 2. Anoob may be concerned about the performance. But if we go with 2, it would be better to have rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md) { void *userdata =3D NULL; =20 +#ifdef RTE_DEBUG + RTE_PTR_OR_ERR_RET(instance, NULL); =20 + RTE_PTR_OR_ERR_RET(instance->ops, NULL); =20 +#endif RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL); .... } And for security test, we can have a separate patch. Lukasz or you can send= that later if not now.