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 E3CDDA034E; Thu, 23 Apr 2020 16:49:16 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8C2931C257; Thu, 23 Apr 2020 16:49:16 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 1BEF11C242 for ; Thu, 23 Apr 2020 16:49:14 +0200 (CEST) IronPort-SDR: Ipf07M++/pXBMDxC/N6afj7WsEJH7vA9p5D+3Q3rjR8bpxjUkdHmktM00XdLsbluMp9ZCi36A5 XJKVZj7umMow== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Apr 2020 07:49:14 -0700 IronPort-SDR: V2oWdQ94HwZzX1exDZeQ7ALN7kvR0hBZSMrmhb1Cfdhj/OLyeSvLSYgc6ge4ARgBD0IWUtZclH hKWCmisPIT2Q== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.73,307,1583222400"; d="scan'208";a="366022858" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga001.fm.intel.com with ESMTP; 23 Apr 2020 07:49:14 -0700 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 23 Apr 2020 07:48:53 -0700 Received: from FMSEDG002.ED.cps.intel.com (10.1.192.134) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 23 Apr 2020 07:48:53 -0700 Received: from NAM02-BL2-obe.outbound.protection.outlook.com (104.47.38.54) by edgegateway.intel.com (192.55.55.69) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 23 Apr 2020 07:48:53 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=nDUhIeiVFy2KMwtzg6rONAiRKDP2Rwfbc/cHnt8qDbRzgst6G+5ZQ+uZdUhUy9N1g+j/sReWx35y89rWvB73SYdMazt5nfkEvmGGPBJh50sH535R5y23ZRe29zf0EDX8HC6x9vRbBUUNKd+JSkEt+QZR97ZbxE4Gii/NpzdAzL5yngJuCu5lFCMP3/SBX9AhPYPEIeFnUUwCsWN1V15RzKOZCrKnR6nXORTerqjw1sijcE9NwJAdYDStxLfQ9RK9AKm3wnsGxnCHTWInBA7Ipu7uf3V7cUGdyONfGKP9dvvu86kM74XR8bpQm0kpgJTWLuVswOCv4/DyMGM/crLopw== 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=+ALooStSFtrW7GSlA7qOfuTaXNvOTcowxBeb8HhcQ2M=; b=SIjdne32z9GRMqruqd4L395Z2NAQELUryRSAAMhBYhJ9or2aLHrL3JEE4wh4NseB+qa1ANYrzlYjTaCBp5rn7sb1aIKI5aRSina/45QLxNptWLWroSUImcY2jrDepgszU7620yEKtn0iNfNqT5wgj6GnTQnQs1hPCNJCapcVQOoatKXt2ddS2ocj3qxS4XH1l9AWli1J9ymfGpIrmTtT70b0IAwIY26IBK1UPtap1sU3ugu/ssAP1PR1Rjaeo4QMMS7ow1kfhYdnsPYboOxbZPa8+WV679jvd083RuWI41B2EmWQRa6Efr2sS99eJwJvsOWyVv3JXPgNMhpsOla2fw== 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=+ALooStSFtrW7GSlA7qOfuTaXNvOTcowxBeb8HhcQ2M=; b=Qa364sJNoYX8Y6KwJCTSo3Dk4LXGe3zKOj8XKyvbk5D5VO3SM3gK5wAVaUoTdSoNcSs4kSLZmZepKR1ezTcHC9pQvm4+ELTr353BtTMeV2Y/wQ7XhfPF1PdTERlHqOV/wak0rx+UaEvVYIuFbu/CZGoVBOKswh4TYsrx3PVc/u4= 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 14:48:50 +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 14:48:49 +0000 From: "Ananyev, Konstantin" To: Akhil Goyal , 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: AQHWGSTLO6APWAgcfEqbxThOaNZf1KiGUA6ggAAbjICAABhdIIAADRIAgAAZmYCAAAmjIIAACsmAgAAD3OA= Date: Thu, 23 Apr 2020 14:48:49 +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: 42cd3de9-b90d-46e0-6f3f-08d7e7956f35 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: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:BYAPR11MB3301.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:(396003)(346002)(136003)(39860400002)(376002)(366004)(26005)(7696005)(4326008)(81156014)(33656002)(107886003)(8936002)(71200400001)(86362001)(110136005)(6506007)(316002)(186003)(55016002)(15650500001)(9686003)(5660300002)(76116006)(2906002)(66446008)(66946007)(64756008)(66476007)(478600001)(52536014)(66556008); DIR:OUT; SFP:1102; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: PzuDMMEC7oAJiMeEkySEFP2tHQy/H5n3uZYhRqkIvT2vD16q6t7zlaDe1XC3tJzvQynu5z+gDVfNH0KWBKXSp+3A4slMRDiOZb5IqXURo8xZ9e0Jw53bqKqwE5IPOHcok4L/0fzODaevKyrHri1Em19wxTF37sipj38pVNnV4wgN0HiIipjZlbwjX81avIYl13ZbX0wN953dGm9IpKk3/Pokxx2zF3Hm/4PWGPmlqlVFEhMh9URXZq/dajWTlamzBgsa0durTCEgUu7xdbiSJLFXq5BU06v7kBSwLs21y/LKQN3dD+zwwVvSrM4cV4dmKCM8mSzGwIQxHD305mqmVI0QOQldWh9uJlK0v8/cBnLxmD+f2EG9VwndgsDFveE0lDc3wMiHDplXtqXBqkHNAgLRVbt5YkTYnsiOLejxFK0RONlmH9i66qGjLvQmWyI2 x-ms-exchange-antispam-messagedata: uNg2ByainPDzadyXrCDrzIdH+XEKA9FYHx5BF8jYPIW4mbscqCdpiPtQIhRia4QjikX2taLq92YmKVQ5lmwoeenCxYhQxgIc1xhnJDG+H8et2sCyDskkb46OVwgnLft95SsSUeOXnzs5i6szSeJdHA== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-Network-Message-Id: 42cd3de9-b90d-46e0-6f3f-08d7e7956f35 X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Apr 2020 14:48:49.7361 (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: pyjSs9KofxXizNCZ8Df2XMuBfGmNQkrNO8QkYf8IuhVn4bJynC8JLJW+QGeO97lFrdZAJI1XRNdYXsCwlQb8zoaUZazjzpg4DterhTZzy6A= 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" > > > > Hi Akhil, > > > > > > > > Hi Anoob/Konstantin, > > > > > > > > > > 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 pr= imary > > > > consumer of this API (rte_security_get_userdata()). So what is the = trouble? > > > > > > > > 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 = doing 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 sho= uld 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 = these 2 > > functions. > > > Currently it is adding 2 extra checks. > > > > > > > I am afraid we can't do just that. > > As in that case /app/test/test_security.c build wih -DRE_DEBUG will sta= rt > > crashing. > > > > I think we have 3 alternative how to fix it: > > > > 1. Keep all these 3 checks for debug and non-debug mode (that what my c= urrent > > patch does). > > 2. Have both: existed 1 check in non-debug mode, plus new checks in deb= ug > > mode, i.e.: > > rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t m= d) > > { > > void *userdata =3D NULL; > > > > +#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 > > > > .... > > > > 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. > > > > My preference is 1), I don't think these 2 extra checks will affect per= formance > > greatly. > > Also with 1) we can make these new test-case to be executed for non-deb= ug > > mode too. > > 2) is probably also ok - but I think RTE_DEBUG concept should be a sepa= rate > > patch series, > > and I don't want to mix things. > > What is your opinion here? > > > 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); > + RTE_PTR_OR_ERR_RET(instance->ops, NULL); > +#endif > RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL); > .... > } >=20 > And for security test, we can have a separate patch. Lukasz or you can se= nd that later if not now. Ok, then to keep everyone happy will go with your code snippet above.