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

[Thomas Monjalon <thomas@monjalon.net>]

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@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.


> >>   [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?