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 7398DA0531; Tue, 4 Feb 2020 11:28:28 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 052B61C001; Tue, 4 Feb 2020 11:28:28 +0100 (CET) Received: from new1-smtp.messagingengine.com (new1-smtp.messagingengine.com [66.111.4.221]) by dpdk.org (Postfix) with ESMTP id 97E721C000 for ; Tue, 4 Feb 2020 11:28:26 +0100 (CET) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailnew.nyi.internal (Postfix) with ESMTP id 531B161CC; Tue, 4 Feb 2020 05:28:24 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Tue, 04 Feb 2020 05:28:24 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=mesmtp; bh=W//OlV3ma3zd0w/WDn+AS/84lNY7X/QK0mvGgsnVWto=; b=ICM4EA70KgHA tP8WLSZfwoajhz4R2ssUSX0gz7OaTrZmoalVXXX98SUDb7018yM2h3rKl7a7g+O7 s3bP1wrSPTPExyh3UyFprdA6lynlnCyY+WhOOOtz2jWaMhbkWN/dzGkqZkZlyeHT N7AHMVGInXQQ+A0dkVnPUYn7ryLe2ag= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm1; bh=W//OlV3ma3zd0w/WDn+AS/84lNY7X/QK0mvGgsnVW to=; b=Pc7tTOduInFm4Lu2PDy6yquJPJKMb7oweO0NTuSySowcCTCtjhjcYB0kw B2D7ovW+0O2izyPZ+wMC4qWFNt4Vjq0b2mMRsA2tqW0VLx7NCyJMDBrG3V7Hy6mI Uqt1mWmweLXOIcP7b6OlaFMcEGzLmApQ7Sj4CQBZLdLH3dTr8gz6IHJvLFnpoXdF rf4l/MB3Fm3+rzBMkuUcgNDzhF6ZZLmpt8QV3caho1QB0jFgbX24qmaKsIyJKwT9 LO9U5h2KUbZhcwmqG3eqznwsNwkqfPM85h+jobkzP+xBOrhUV3KzY/w/76Tc83kR TkAQjd2BEkO/oagWx48T8Ywur4lEA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedugedrgeelgddujecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucfkph epjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrhfuihiivgeptdenucfrrghr rghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 0400A3280063; Tue, 4 Feb 2020 05:28:21 -0500 (EST) From: Thomas Monjalon To: Akhil Goyal 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" Date: Tue, 04 Feb 2020 11:28:20 +0100 Message-ID: <3830195.LM0AJKV5NW@xps> In-Reply-To: References: <20191220152058.10739-1-david.marchand@redhat.com> <78e8ecf2-2239-897e-e34c-aee7227f3d42@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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" 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 between 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 compiled 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 from 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 will 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 application > > >>>> "picks" the new value. I think you miss the case where the new value > > >>>> 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 devices > > >>>> supporting each AEAD algo (in order to find the best supported algo). > > >>>> 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 = 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 = 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 (= 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 >= 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 marketing, > > > 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 approach > > 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 = _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 different value > > but same logic applies. > > > > When "_LIST_END" is missing, application can't protect itself, in that case > > library should send only the values application knows when it is compiled, 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 = 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 chacha poly assume that this algo is > Experimental and will move to formal list in 20.11. This can be documented in the documentation. > I believe there is no way to add a new enum as experimental so far. This way we can formalize this > requirement as well. > > I believe this way effect of ABI breakage will be nullified. 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. I guess ABI and release maintainers will vote no to such breakage. Note: I vote no. > > > Then *_LIST_END values could be ignored by libabigail with such a change. > > > > > > 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