Re: [dpdk-dev] [PATCH v2 4/4] add ABI checks

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 2/2/2020 2:41 PM, Ananyev, Konstantin wrote:
> 
>> 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.

Do you suggest we don't extend any enum or define between ABI breakage releases
to be sure bad written applications not affected?