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 E938CA0531; Tue, 4 Feb 2020 11:08:52 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 22DE21C06C; Tue, 4 Feb 2020 11:08:45 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 5923A1C000 for ; Tue, 4 Feb 2020 11:08:41 +0100 (CET) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Feb 2020 02:08:40 -0800 X-IronPort-AV: E=Sophos;i="5.70,401,1574150400"; d="scan'208";a="224247463" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.79]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-SHA; 04 Feb 2020 02:08:35 -0800 Date: Tue, 4 Feb 2020 10:08:32 +0000 From: Bruce Richardson To: Ferruh Yigit Cc: Thomas Monjalon , dev@dpdk.org, "Ananyev, Konstantin" , Akhil Goyal , "Trahe, Fiona" , David Marchand , Anoob Joseph , "Kusztal, ArkadiuszX" , nhorman@tuxdriver.com, "Mcnamara, John" , dodji@seketeli.net, Andrew Rybchenko , aconole@redhat.com, bluca@debian.org, ktraynor@redhat.com Message-ID: <20200204100832.GA637@bricha3-MOBL.ger.corp.intel.com> References: <20191220152058.10739-1-david.marchand@redhat.com> <2219936.atdPhlSkOF@xps> <0f85d878-c238-8531-e629-e41d49f5f05b@intel.com> <13361272.RDIVbhacDa@xps> <6c630641-d229-daff-92d5-fb80eae4bd16@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6c630641-d229-daff-92d5-fb80eae4bd16@intel.com> User-Agent: Mutt/1.12.1 (2019-06-15) 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" On Tue, Feb 04, 2020 at 09:56:31AM +0000, Ferruh Yigit wrote: > On 2/4/2020 9:45 AM, Thomas Monjalon wrote: > > 04/02/2020 10:19, Ferruh Yigit: > >> On 2/3/2020 6:40 PM, Thomas Monjalon wrote: > >>> 03/02/2020 18:40, Ferruh Yigit: > >>>> 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? > >>> > >>> _LIST_MAX is range of value that DPDK can return in the ABI contract. > >>> So the appplication can rely on the range 0.._LIST_MAX. > >>> > >>>> 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. > >>> > >>> No, ABI compatibility contract means you can compile your application > >>> with DPDK 19.11.0 and run it with DPDK 20.02. > >>> So _LIST_END comes from 19.11 and does not include ChachaPoly. > >> > >> That is what I mean, let me try to give a sample. > >> > >> DPDK19.11 returns, A=1, B=2, END=3 > >> > >> Application compiled with DPDK19.11, it will process A, B and ignore anything ">= 3" > > > > No, the application will not ignore anything ">=3" as I explained above, > > and you blamed the application for it. > > Nothing in the API says the application must filter value higher than 3, > > because as of now, values higher than 3 are PMD bug. > > When application compiled, that is the END value, anything bigger than this > value is not valid, if any application use the return value directly I think it > is doing something wrong. > But yes there may be applications relying on library will always send in the > range. We never communicated this. But we can add comments to clarify this. > I don't think we should do so, as for any function returning an enum by definition it should never return an out-of-range value. I strongly agree with the suggestion of versioning the functions so that the ranges seen by apps are clamped to the expected 19.11 compatible values.