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

[Ferruh Yigit]

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:
>>>
>>>
>>>>
>>>> On Wed, Jan 29, 2020 at 7:10 PM Anoob Joseph <anoobj at marvell.com> wrote:
>>>>> The asymmetric crypto library is experimental. Changes to experimental code
>>>> paths is allowed, right?
>>>>
>>>> The asymmetric crypto enum is referenced by a function part of the stable ABI.
>>>> It is possible to waive this enum, if we are sure no use out of the
>>>> experimental asym crypto APIs is possible.
>>>>
>>>> The rest of the changes touch stable symbols.
>>>>
>>>> Adding the abidiff report:
>>>>
>>>>   [C]'function void rte_cryptodev_info_get(uint8_t,
>>>> rte_cryptodev_info*)' at rte_cryptodev.c:1115:1 has some indirect
>>>> sub-type changes:
>>>>     parameter 2 of type 'rte_cryptodev_info*' has sub-type changes:
>>>>       in pointed to type 'struct rte_cryptodev_info' at rte_cryptodev.h:468:1:
>>>>         type size hasn't changed
>>>>         1 data member change:
>>>>          type of 'const rte_cryptodev_capabilities*
>>>> rte_cryptodev_info::capabilities' changed:
>>>>            in pointed to type 'const rte_cryptodev_capabilities':
>>>>              in unqualified underlying type 'struct
>>>> rte_cryptodev_capabilities' at rte_cryptodev.h:176:1:
>>>>                type size hasn't changed
>>>>                1 data member change:
>>>>                 type of '__anonymous_union__ ' changed:
>>>>                   type size hasn't changed
>>>>                   1 data member change:
>>>>                    type of 'rte_cryptodev_asymmetric_capability
>>>> __anonymous_union__::asym' changed:
>>>>                      type size hasn't changed
>>>>                      1 data member change:
>>>>                       type of
>>>> 'rte_cryptodev_asymmetric_xform_capability
>>>> rte_cryptodev_asymmetric_capability::xform_capa' changed:
>>>>                         type size hasn't changed
>>>>                         1 data member change:
>>>>                          type of 'rte_crypto_asym_xform_type
>>>> rte_cryptodev_asymmetric_xform_capability::xform_type' changed:
>>>>                            type size hasn't changed
>>>>                            2 enumerator insertions:
>>>>
>>>> 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_ECDSA' value '7'
>>>>
>>>> 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_ECPM' value '8'
>>>>                            1 enumerator change:
>>>>
>>>> 'rte_crypto_asym_xform_type::RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END'
>>>> from
>>>> value '7' to '9' at rte_crypto_asym.h:60:1
>>>>
>>>
>>> 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.

First this concern is not an ABI break concern, but application should ignore
any value bigger than the MAX value it knows.
Otherwise this would mean we can't add any new enum or define to the project,
which is wrong I believe.

> 
> 
>>>>   [C]'function int
>>>> rte_cryptodev_get_aead_algo_enum(rte_crypto_aead_algorithm*, const
>>>> char*)' at rte_cryptodev.c:239:1 has some indirect sub-type changes:
>>>>     parameter 1 of type 'rte_crypto_aead_algorithm*' has sub-type changes:
>>>>       in pointed to type 'enum rte_crypto_aead_algorithm' at
>>>> rte_crypto_sym.h:346:1:
>>>>         type size hasn't changed
>>>>         1 enumerator insertion:
>>>>           'rte_crypto_aead_algorithm::RTE_CRYPTO_AEAD_CHACHA20_POLY1305'
>>>> value '3'
>>>>         1 enumerator change:
>>>>           'rte_crypto_aead_algorithm::RTE_CRYPTO_AEAD_LIST_END' from
>>>> value '3' to '4' at rte_crypto_sym.h:346:1
>>
>> Same as above, no layout change.
>>
>>>>
>>>>
>>>>   [C]'const char* rte_crypto_aead_algorithm_strings[1]' was changed at
>>>> rte_crypto_sym.h:358:1:
>>>>     size of symbol (in bytes) changed from 24 to 32
>>>>
>>
>> The shared memory size changes, but this is global variable in the library, and
>> the values application can request 'RTE_CRYPTO_AEAD_AES_CCM' &
>> 'RTE_CRYPTO_AEAD_AES_GCM' is already there, so there is no backward
>> compatibility issue here.
> 
> For this one, I don't know what is the breakage.
> 
> 
>>> +Fiona and Arek 
>>>
>>> We may need to revert the chacha-poly patches.
>>>
>>
>> I don't see any ABI break in this case, can someone explain if I am missing
>> anything here?
> 
> 
> 
> 
>