Re: [PATCH v2 01/13] security: add direction in SA/SC configuration

[David Marchand <david.marchand@redhat.com>]

On Wed, Jun 7, 2023 at 5:20 PM Akhil Goyal <gakhil@marvell.com> wrote:
>
> MACsec SC/SA ids are created based on direction of the flow.
> Hence, added the missing field for configuration and cleanup
> of the SCs and SAs.
>
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> ---
>  devtools/libabigail.abignore       |  7 +++++++
>  lib/security/rte_security.c        | 16 ++++++++++------
>  lib/security/rte_security.h        | 14 ++++++++++----
>  lib/security/rte_security_driver.h | 12 ++++++++++--
>  4 files changed, 37 insertions(+), 12 deletions(-)
>

Looking at the report with no supression rule:
$ abidiff --suppr .../next-cryptodev/devtools/libabigail.abignore
--no-added-syms --headers-dir1
.../abi/v23.03/build-gcc-shared/usr/local/include --headers-dir2
.../next-cryptodev/build-gcc-shared/install/usr/local/include
.../abi/v23.03/build-gcc-shared/usr/local/lib64/librte_security.so.23.1
.../next-cryptodev/build-gcc-shared/install/usr/local/lib64/librte_security.so.23.2
Functions changes summary: 0 Removed, 1 Changed (13 filtered out), 0
Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

1 function with some indirect sub-type change:

  [C] 'function const rte_security_capability*
rte_security_capabilities_get(rte_security_ctx*)' at
rte_security.c:241:1 has some indirect sub-type changes:
    parameter 1 of type 'rte_security_ctx*' has sub-type changes:
      in pointed to type 'struct rte_security_ctx' at rte_security.h:69:1:
        type size hasn't changed
        1 data member change:
          type of 'const rte_security_ops* ops' changed:
            in pointed to type 'const rte_security_ops':
              in unqualified underlying type 'struct rte_security_ops'
at rte_security_driver.h:230:1:
                type size hasn't changed
                4 data member changes (4 filtered):
                  type of 'security_macsec_sc_create_t
macsec_sc_create' changed:
                    underlying type 'int (void*,
rte_security_macsec_sc*)*' changed:
                      in pointed to type 'function type int (void*,
rte_security_macsec_sc*)':
                        parameter 2 of type 'rte_security_macsec_sc*'
has sub-type changes:
                          in pointed to type 'struct
rte_security_macsec_sc' at rte_security.h:399:1:
                            type size changed from 256 to 320 (in bits)
                            1 data member insertion:
                              'union {struct {uint16_t sa_id[4];
uint8_t sa_in_use[4]; uint8_t active; uint8_t is_xpn; uint8_t
reserved;} sc_rx; struct {uint16_t sa_id; uint16_t sa_id_rekey;
uint64_t sci; uint8_t active; uint8_t re_key_en; uint8_t is_xpn;
uint8_t reserved;} sc_tx;}', at offset 128 (in bits)
                            1 data member change:
                              anonymous data member union {struct
{uint16_t sa_id[4]; uint8_t sa_in_use[4]; uint8_t active; uint8_t
reserved;} sc_rx; struct {uint16_t sa_id; uint16_t sa_id_rekey;
uint64_t sci; uint8_t active; uint8_t re_key_en; uint8_t reserved;}
sc_tx;} at offset 64 (in bits) became data member 'uint64_t
pn_threshold'
                              and size changed from 192 to 64 (in
bits) (by -128 bits)
                  type of 'security_macsec_sc_destroy_t
macsec_sc_destroy' changed:
                    underlying type 'int (void*, typedef uint16_t)*' changed:
                      in pointed to type 'function type int (void*,
typedef uint16_t)':
                        parameter 3 of type 'enum
rte_security_macsec_direction' was added
                  type of 'security_macsec_sc_stats_get_t
macsec_sc_stats_get' changed:
                    underlying type 'int (void*, typedef uint16_t,
rte_security_macsec_sc_stats*)*' changed:
                      in pointed to type 'function type int (void*,
typedef uint16_t, rte_security_macsec_sc_stats*)':
                        parameter 3 of type
'rte_security_macsec_sc_stats*' changed:
                          entity changed from
'rte_security_macsec_sc_stats*' to 'enum
rte_security_macsec_direction' at rte_security.h:361:1
                          type size changed from 64 to 32 (in bits)
                          type alignment changed from 0 to 32
                        parameter 4 of type
'rte_security_macsec_sc_stats*' was added
                  type of 'security_macsec_sa_stats_get_t
macsec_sa_stats_get' changed:
                    underlying type 'int (void*, typedef uint16_t,
rte_security_macsec_sa_stats*)*' changed:
                      in pointed to type 'function type int (void*,
typedef uint16_t, rte_security_macsec_sa_stats*)':
                        parameter 3 of type
'rte_security_macsec_sa_stats*' changed:
                          entity changed from
'rte_security_macsec_sa_stats*' to 'enum
rte_security_macsec_direction' at rte_security.h:361:1
                          type size changed from 64 to 32 (in bits)
                          type alignment changed from 0 to 32
                        parameter 4 of type
'rte_security_macsec_sa_stats*' was added

The report complains about the macsec ops type changes.

> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index c0361bfc7b..14d8fa4293 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -37,6 +37,13 @@
>  [suppress_type]
>          type_kind = enum
>          changed_enumerators = RTE_CRYPTO_ASYM_XFORM_ECPM, RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> +; Ignore changes to rte_security_ops MACsec APIs which are experimental
> +[suppress_type]
> +        name = rte_security_ops
> +        has_data_member_inserted_between =
> +        {
> +                offset_of(security_macsec_sc_create_t), offset_of(security_macsec_sa_stats_get_t)
> +        }

So I don't get the intent with this rule.
There is no field named neither security_macsec_sc_create_t nor
security_macsec_sa_stats_get_t in the rte_security_ops struct.

Now.. why is this rule making the check pass... it is a mystery to me.
I already hit a case when libabigail ignored statements that are
invalid or make no sense, so my guess is that this rule is actually
applied as a simple:
[suppress_type]
        name = rte_security_ops

And well, this rule is ok from my pov: this rte_security_ops struct
does not change in size.
An application is not supposed to know about its content (that is
defined in a driver header) and all accesses to those ops are supposed
to be through symbols from the security library.
So I would go with this "larger" and simpler rule.


Just a small addition to this, as discussed offlist, this is going to
be reworked in v23.11 and this rule on rte_security_ops will be
unneccesary, so please move it in the relevant block (at the end) of
the libabigail.abignore file.


-- 
David Marchand