On 2/3/2020 9:07 PM, Thomas Monjalon wrote: > 03/02/2020 19:55, Ray Kinsella: >> On 03/02/2020 17:34, Thomas Monjalon wrote: >>> 03/02/2020 18:09, Thomas Monjalon: >>>> 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: >>>>>>>>> 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. >>>> >>>>> 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: >>> >>> Thinking twice, merging such change before 20.11 is breaking the >>> ABI assumption based on the API 19.11.0. >>> I ask the release maintainers (Luca, Kevin, David and me) and >>> the ABI maintainers (Neil and Ray) to vote for a or b solution: >>> a) add comment and LIST_MAX as below in 20.02 + 19.11.1 >> >> That would still be an ABI breakage though right. >> >>> b) wait 20.11 and revert Chacha-Poly from 20.02 >> >> Thanks for analysis above Fiona, Ferruh and all. >> >> That is a nasty one alright - there is no "good" answer here. >> I agree with Ferruh's sentiments overall, we should rethink this API for >> 20.11. >> Could do without an enumeration? >> >> There a c) though right. >> We could work around the issue by api versioning rte_cryptodev_info_get() >> and friends. >> So they only support/acknowledge the existence of Chacha-Poly for >> applications build against > 20.02. > > I agree there is a c) as I proposed in another email: > http://mails.dpdk.org/archives/dev/2020-February/156919.html > " > In this case, the proper solution is to implement > rte_cryptodev_info_get_v1911() so it filters out > RTE_CRYPTO_AEAD_CHACHA20_POLY1305 capability. > With this solution, an application compiled with DPDK 19.11 will keep > seeing the same range as before, while a 20.02 application could > see and use ChachaPoly. > " > >> It would be painful I know. > > Not so painful in my opinion. > Just need to call rte_cryptodev_info_get() from > rte_cryptodev_info_get_v1911() and filter the value > in the 19.11 range: [0..AES_GCM]. > >> It would also mean that Chacha-Poly would only be available to >> those building against >= 20.02. > > Yes exactly. > > The addition of comments and LIST_MAX like below are still valid > to avoid versioning after 20.11. > >>>> - _LIST_END >>>> + _LIST_END, /* an ABI-compatible version may increase this value */ >>>> + _LIST_MAX = _LIST_END + 42 /* room for ABI-compatible additions */ >>>> }; >>>> >>>> Then *_LIST_END values could be ignored by libabigail with such a change. > > In order to avoid ABI check complaining, the best is to completely > remove LIST_END in DPDK 20.11.
We can remove LIST_END only if we go with option (c). Two different approach, - Provide the LIST_END and expect application protect itself against new values can be coming in newer version of the library - Do ABI versioning to prevent application receive new values at all, (c). We can select one, but I believe the selection shouldn't be based on just silencing the ABI check tool. > > >>>> 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 > > >
