[EXT] Re: [PATCH v4 3/3] security: add IPsec option for IP reassembly

[Akhil Goyal]

> Hello Akhil,
> 
> 
> On Fri, Feb 4, 2022 at 11:14 PM Akhil Goyal <gakhil at marvell.com> wrote:
> >
> > A new option is added in IPsec to enable and attempt reassembly
> > of inbound packets.
> 
> First, about extending this structure.
> 
> Copying the header:
> 
>         /** Reserved bit fields for future extension
>          *
>          * User should ensure reserved_opts is cleared as it may change in
>          * subsequent releases to support new options.
>          *
>          * Note: Reduce number of bits in reserved_opts for every new option.
>          */
>         uint32_t reserved_opts : 18;
> 
> I did not follow the introduction of the reserved_opts field, but
> writing this comment in the API only is weak.
> Why can't the rte_security API enforce reserved_opts == 0 (like in
> rte_security_session_create)?
> 
This was discussed here.
http://patches.dpdk.org/project/dpdk/patch/20211008204516.3497060-3-gakhil@marvell.com/
rte_security_ipsec_sa_options is being used at multiple places as listed below in abiignore.
Checking a particular field in each of the API does not make sense to me.

> 
> 
> >
> > Signed-off-by: Akhil Goyal <gakhil at marvell.com>
> > Change-Id: I6f66f0b5a659550976a32629130594070cb16cb1
>   ^^^
>   Internal tag, please remove.
> 
Yes, missed that will remove.
> 
> > ---
> >  devtools/libabigail.abignore | 14 ++++++++++++++
> >  lib/security/rte_security.h  | 12 +++++++++++-
> >  2 files changed, 25 insertions(+), 1 deletion(-)
> >
> > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> > index 4b676f317d..3bd39042e8 100644
> > --- a/devtools/libabigail.abignore
> > +++ b/devtools/libabigail.abignore
> > @@ -11,3 +11,17 @@
> >  ; Ignore generated PMD information strings
> >  [suppress_variable]
> >          name_regexp = _pmd_info$
> > +
> > +; Ignore fields inserted in place of reserved_opts of
> rte_security_ipsec_sa_options
> > +[suppress_type]
> > +       name = rte_ipsec_sa_prm
> > +       name = rte_security_ipsec_sa_options
> > +       has_data_member_inserted_between = {offset_of(reserved_opts), end}
> > +
> > +[suppress_type]
> > +       name = rte_security_capability
> > +       has_data_member_inserted_between = {offset_of(reserved_opts),
> (offset_of(reserved_opts) + 18)}
> > +
> > +[suppress_type]
> > +       name = rte_security_session_conf
> > +       has_data_member_inserted_between = {offset_of(reserved_opts),
> (offset_of(reserved_opts) + 18)}
> 
> Now, about the suppression rule, I don't understand the intention of
> those 3 rules.
> 
> I would simply suppress modifications (after reserved_opts) to the
> rte_security_ipsec_sa_options struct.
> Like:
> 
> ; Ignore fields inserted in place of reserved_opts of
> rte_security_ipsec_sa_options
> [suppress_type]
>        name = rte_security_ipsec_sa_options
>        has_data_member_inserted_between = {offset_of(reserved_opts), end}
> 
I tried this in the first place but abi check was complaining in other structures which included
rte_security_ipsec_sa_options. So I had to add suppression for those as well.
Can you try at your end?