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 62970A0531; Tue, 4 Feb 2020 11:32:05 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 984A81C001; Tue, 4 Feb 2020 11:32:04 +0100 (CET) Received: from EUR05-AM6-obe.outbound.protection.outlook.com (mail-am6eur05on2077.outbound.protection.outlook.com [40.107.22.77]) by dpdk.org (Postfix) with ESMTP id 03BD31C000 for ; Tue, 4 Feb 2020 11:32:02 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TXFdH9TbI+tfG9a8Q3kwjX0+MqEZPxf160Ne/G9tg+LIkhlGoM8n2FGZAt9QvI/pUjbJ1jLj7tG/xhQCnrGU20pICaYL9o6yGDIMqa0C47fjZEvWsqdU2OcuE6X1fsZNeCfP5J0k1NsfG2CIAdr+tYjdq9qSVY9LYb3bNFGU51wPtqmt2IqD6arCWopBf5xkj77bczn6F2q8MtiU5A7VLAAp1tETHi9gnAnef0rgFK+MDnBbMvPAZuxr9ueTjzb7FnMhFdqytaIO3EwV1sw1MePp4UtkDKO/6QF10zxLsLgmq/cu01d7dAvknQvX4INXrgAc53VD0DZ6cNGaR2TLJg== 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=8RyvXVFsOcv9cHnxKzqmTy41JeTjlRJJR+TfJT/v2Dw=; b=j13Y2ZO+8YVP2B0GjRbZzYH2Gp9++kHnjYsivFrHcOgUM17fcVJJTCRYl90uK2dwN0CUHrwMzrGCLwV6CQJUXzp/jBfHn7P2YqUjcLioUaIz8HnwYwLX/LFiM6AOS6AH25mqFHEv6bDkoQ+hVDrowWXC9MTb6Lzr0kQGmy5kZdCWNtGWMocJSalYjp9STya6VzSLnEADkKT00mVE3Rd1C+JRbvaIt1W6hz9K4TSWT9rZckYLbNJ8jPDj1st5RA6MNa00YLP7Smjd7qyYaLFp4JtqzFXmQAwl2c3Y87uSe+GemirmC7Cy4RL+TgfoZfBJa/u6zgbp0reH2CvF3BTjdw== 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=8RyvXVFsOcv9cHnxKzqmTy41JeTjlRJJR+TfJT/v2Dw=; b=asLkVyNp+uOOJWIR88YPziShi1+SdWuLcDrgwxi+Hgv6ohwJU4XBalHADOW8vQ0C6dDfEYtxoAYN2H31wFG1kSHlfaBkXDwwToum+ds5Oxj47OGUYu21RK7hS0035bRFeOseknCC0LrEbfBWJ9DvN4/NnTi151TOevhBWC9I6zw= Received: from VE1PR04MB6639.eurprd04.prod.outlook.com (10.255.118.11) by VE1PR04MB6656.eurprd04.prod.outlook.com (20.179.235.95) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2686.30; Tue, 4 Feb 2020 10:32:01 +0000 Received: from VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::25b0:b1ac:aed0:63e1]) by VE1PR04MB6639.eurprd04.prod.outlook.com ([fe80::25b0:b1ac:aed0:63e1%7]) with mapi id 15.20.2686.031; Tue, 4 Feb 2020 10:32:01 +0000 From: Akhil Goyal To: Thomas Monjalon CC: Ferruh Yigit , "Ananyev, Konstantin" , "Trahe, Fiona" , "dev@dpdk.org" , David Marchand , Anoob Joseph , "Kusztal, ArkadiuszX" , "dev@dpdk.org" , "Richardson, Bruce" , "nhorman@tuxdriver.com" , "Mcnamara, John" , "dodji@seketeli.net" , Andrew Rybchenko , "aconole@redhat.com" , "bluca@debian.org" , "ktraynor@redhat.com" Thread-Topic: [dpdk-dev] [PATCH v2 4/4] add ABI checks Thread-Index: AQHV1slU1EtoEiCqh0WvE9LTeudm8KgB6XuAgAAHogCAAB+GAIAAAYCwgAFPoQCAAEVxAIABLWUAgAMQ1gCAABrKgIABOz0AgACAVgCAAAiiAIABE6PQgAAF/wCAAABe4A== Date: Tue, 4 Feb 2020 10:32:01 +0000 Message-ID: References: <20191220152058.10739-1-david.marchand@redhat.com> <78e8ecf2-2239-897e-e34c-aee7227f3d42@intel.com> <3830195.LM0AJKV5NW@xps> In-Reply-To: <3830195.LM0AJKV5NW@xps> 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: [92.120.1.72] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: df659d53-16cb-4a1e-fd2f-08d7a95d788d x-ms-traffictypediagnostic: VE1PR04MB6656: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 03030B9493 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(136003)(39860400002)(396003)(366004)(376002)(346002)(189003)(199004)(71200400001)(4326008)(81166006)(66946007)(66446008)(54906003)(186003)(33656002)(81156014)(6916009)(55016002)(9686003)(7416002)(7696005)(5660300002)(86362001)(2906002)(8936002)(52536014)(6506007)(53546011)(44832011)(66556008)(66476007)(64756008)(76116006)(26005)(478600001)(316002)(8676002); DIR:OUT; SFP:1101; SCL:1; SRVR:VE1PR04MB6656; H:VE1PR04MB6639.eurprd04.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; 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: vgD7N1caqU0Ydnia9T4oKpTidbzOk47KRHqWht+6h7JuUIMCxEpTjDg4u0iPQZKHQtvJVYnmdzHG7OTkWeDreyUeIcsIUITkn+2DB7XMSyxL0ADj8Fq5x3SWURTivGX5HyGz+I/GT4nlEXDaJfqM1R36rnjN4Heljb/LFIlhqTSFOQDmjEWMPVCy1w52bU2F0eTOlMkuKHv1BGsNH8zp49RTkracJP35opQFF7E8u9AhFCOaDWPPYiyZyfDglWVDUQptISzCTmDD0dzGQE5e0EDCKEpE3io2eH+ZLuXrDksYxBPbB9EDkfkbgpic7riCrvSRANT35SRBXbYdb9bdHz270Z4RsIrzt4E7IdN4jBdx/lHPeJU4IxxDqhnfkk+gFTbYt0wYQ+TWUAseFRHOpsHyyd8jizngDAn0OLlA04iKmTO7V7tmpJ31ykUczUF2 x-ms-exchange-antispam-messagedata: gcCWY08TeNK8szmqIpUw5STrhcPVOB2A/zdHWvqZ/dPfyXZUuxrdLJfsjral64wIsLaFVNODfRzkmGLAPUZ92MEJAXx63sKmuMV6ctfk/HffImA0leQei4SYB+W1R873lzcFtaWyfBKzh6u1AuK4wQ== 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: df659d53-16cb-4a1e-fd2f-08d7a95d788d X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Feb 2020 10:32:01.4009 (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: MFA/NZKVlLYywIQN/0dhY8mHbEcU4WKTijHggji3bdNs1qs03vjn3K/t+6CALEByKxzpQSM/XQ87VKo+gSMLkA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: VE1PR04MB6656 Subject: Re: [dpdk-dev] [PATCH v2 4/4] add ABI checks 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 > 04/02/2020 11:16, Akhil Goyal: > > Hi, > > > On 2/3/2020 5:09 PM, Thomas Monjalon wrote: > > > > 03/02/2020 10:30, Ferruh Yigit: > > > >> On 2/2/2020 2:41 PM, Ananyev, Konstantin wrote: > > > >>> 02/02/2020 14:05, Thomas Monjalon: > > > >>>> 31/01/2020 15:16, Trahe, Fiona: > > > >>>>> On 1/30/2020 8:18 PM, Thomas Monjalon wrote: > > > >>>>>> 30/01/2020 17:09, Ferruh Yigit: > > > >>>>>>> On 1/29/2020 8:13 PM, Akhil Goyal wrote: > > > >>>>>>>> > > > >>>>>>>> I believe these enums will be used only in case of ASYM case= which > is > > > experimental. > > > >>>>>>> > > > >>>>>>> Independent from being experiment and not, this shouldn't be = a > > > problem, I think > > > >>>>>>> this is a false positive. > > > >>>>>>> > > > >>>>>>> The ABI break can happen when a struct has been shared betwee= n > the > > > application > > > >>>>>>> and the library (DPDK) and the layout of that memory know > differently > > > by > > > >>>>>>> application and the library. > > > >>>>>>> > > > >>>>>>> Here in all cases, there is no layout/size change. > > > >>>>>>> > > > >>>>>>> As to the value changes of the enums, since application compi= led > with > > > old DPDK, > > > >>>>>>> it will know only up to '6', 7 and more means invalid to the > application. > > > So it > > > >>>>>>> won't send these values also it should ignore these values fr= om > library. > > > Only > > > >>>>>>> consequence is old application won't able to use new features > those > > > new enums > > > >>>>>>> provide but that is expected/normal. > > > >>>>>> > > > >>>>>> If library give higher value than expected by the application, > > > >>>>>> if the application uses this value as array index, > > > >>>>>> there can be an access out of bounds. > > > >>>>> > > > >>>>> [Fiona] All asymmetric APIs are experimental so above shouldn't= be a > > > problem. > > > >>>>> But for the same issue with sym crypto below, I believe Ferruh'= s > > > explanation makes > > > >>>>> sense and I don't see how there can be an API breakage. > > > >>>>> So if an application hasn't compiled against the new lib it wil= l be still > using > > > the old value > > > >>>>> which will be within bounds. If it's picking up the higher new = value > from > > > the lib it must > > > >>>>> have been compiled against the lib so shouldn't have problems. > > > >>>> > > > >>>> You say there is no ABI issue because the application will be re= - > compiled > > > >>>> for the updated library. Indeed, compilation fixes compatibility= issues. > > > >>>> But this is not relevant for ABI compatibility. > > > >>>> ABI compatibility means we can upgrade the library without > recompiling > > > >>>> the application and it must work. > > > >>>> You think it is a false positive because you assume the applicat= ion > > > >>>> "picks" the new value. I think you miss the case where the new v= alue > > > >>>> is returned by a function in the upgraded library. > > > >>>> > > > >>>>> There are also no structs on the API which contain arrays using= this > > > >>>>> for sizing, so I don't see an opportunity for an appl to have a > > > >>>>> mismatch in memory addresses. > > > >>>> > > > >>>> Let me demonstrate where the API may "use" the new value > > > >>>> RTE_CRYPTO_AEAD_CHACHA20_POLY1305 and how it impacts the > > > application. > > > >>>> > > > >>>> Once upon a time a DPDK application counting the number of devic= es > > > >>>> supporting each AEAD algo (in order to find the best supported a= lgo). > > > >>>> It is done in an array indexed by algo id: > > > >>>> int aead_dev_count[RTE_CRYPTO_AEAD_LIST_END]; > > > >>>> The application is compiled with DPDK 19.11, > > > >>>> where RTE_CRYPTO_AEAD_LIST_END =3D 3. > > > >>>> So the size of the application array aead_dev_count is 3. > > > >>>> This binary is run with DPDK 20.02, > > > >>>> where RTE_CRYPTO_AEAD_CHACHA20_POLY1305 =3D 3. > > > >>>> When calling rte_cryptodev_info_get() on a device QAT_GEN3, > > > >>>> rte_cryptodev_info.capabilities.sym.aead.algo is set to > > > >>>> RTE_CRYPTO_AEAD_CHACHA20_POLY1305 (=3D 3). > > > >>>> The application uses this value: > > > >>>> ++ aead_dev_count[info.capabilities.sym.aead.algo]; > > > >>>> The application is crashing because of out of bound access. > > > >>> > > > >>> I'd say this is an example of bad written app. > > > >>> It probably should check that returned by library value doesn't > > > >>> exceed its internal array size. > > > >> > > > >> +1 > > > >> > > > >> Application should ignore values >=3D MAX. > > > > > > > > Of course, blaming the API user is a lot easier than looking at the= API. > > > > Here the API has RTE_CRYPTO_AEAD_LIST_END which can be understood > > > > as the max value for the application. > > > > Value ranges are part of the ABI compatibility contract. > > > > It seems you expect the application developer to be aware that > > > > DPDK could return a higher value, so the application should > > > > check every enum values after calling an API. CRAZY. > > > > > > > > When we decide to announce an ABI compatibility and do some marketi= ng, > > > > everyone is OK. But when we need to really make our ABI compatible, > > > > I see little or no effort. DISAPPOINTING. > > > > > > This is not to blame the user or to do less work, this is more sane a= pproach > > > that library provides the _END/_MAX value and application uses it as = valid > range > > > check. > > > > > > > > > > >> Do you suggest we don't extend any enum or define between ABI > breakage > > > releases > > > >> to be sure bad written applications not affected? > > > > > > > > I suggest we must consider not breaking any assumption made on the = API. > > > > Here we are breaking the enum range because nothing mentions > _LIST_END > > > > is not really the absolute end of the enum. > > > > The solution is to make the change below in 20.02 + backport in 19.= 11.1: > > > > > > > > - _LIST_END > > > > + _LIST_END, /* an ABI-compatible version may increase this value *= / > > > > + _LIST_MAX =3D _LIST_END + 42 /* room for ABI-compatible additions= */ > > > > }; > > > > > > > > > > What is the point of "_LIST_MAX" here? > > > > > > Application should know the "_LIST_END" of when it has been compiled = for > the > > > valid range check. Next time it is compiled "_LIST_END" may be differ= ent > value > > > but same logic applies. > > > > > > When "_LIST_END" is missing, application can't protect itself, in tha= t case > > > library should send only the values application knows when it is comp= iled, > this > > > means either we can't extend our enum/defines until next ABI breakage= , or > we > > > need to do ABI versioning to the functions that returns an enum each = time > enum > > > value extended. > > > > > > I believe it is saner to provide _END/_MAX values to the application = to use. > And > > > if required comment them to clarify the expected usage. > > > > > > But in above suggestion application can't use or rely on "_LIST_MAX",= it > doesn't > > > mean anything to application. > > > > > > > Can we have something like > > enum rte_crypto_aead_algorithm { > > RTE_CRYPTO_AEAD_AES_CCM =3D 1, > > /**< AES algorithm in CCM mode. */ > > RTE_CRYPTO_AEAD_AES_GCM, > > /**< AES algorithm in GCM mode. */ > > RTE_CRYPTO_AEAD_LIST_END, > > /**< List end for 19.11 ABI compatibility */ > > RTE_CRYPTO_AEAD_CHACHA20_POLY1305, > > /**< Chacha20 cipher with poly1305 authenticator */ > > RTE_CRYPTO_AEAD_LIST_END_2011 > > /**< List end for 20.11 ABI compatibility */ > > }; > > > > And in 20.11 release we alter the RTE_CRYPTO_AEAD_LIST_END to the end > and remove RTE_CRYPTO_AEAD_LIST_END_2011 > > > > I believe it will be ok for any application which need to use the chach= a poly > assume that this algo is > > Experimental and will move to formal list in 20.11. This can be documen= ted in > the documentation. > > I believe there is no way to add a new enum as experimental so far. Thi= s way > we can formalize this > > requirement as well. > > > > I believe this way effect of ABI breakage will be nullified. >=20 > This is a possibility in the (a) proposal. > But it breaks API (and ABI) because a high value is returned > while not expected by the application. >=20 > I guess ABI and release maintainers will vote no to such breakage. > Note: I vote no. >=20 If that is the case, I would say we should go with b). Versioned APIs does not look good and adds more confusion. >=20 > > > > Then *_LIST_END values could be ignored by libabigail with such a c= hange. > > > > > > > > If such a patch is not done by tomorrow, I will have to revert > > > > Chacha-Poly commits before 20.02-rc2, because > > > > > > > > 1/ LIST_END, without any comment, means "size of range" > > > > 2/ we do not blame users for undocumented ABI changes > > > > 3/ we respect the ABI compatibility contract >=20 >=20 >=20 >=20