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

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

On Tue, Feb 8, 2022 at 10:19 AM Akhil Goyal <gakhil@marvell.com> wrote:
>
> > Hello Akhil,
> >
> >
> > On Fri, Feb 4, 2022 at 11:14 PM Akhil Goyal <gakhil@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.

It's strange to me that a user may pass this structure as input in
multiple functions.
But if it's how the security lib works, ok.


>
> >
> >
> > >
> > > Signed-off-by: Akhil Goyal <gakhil@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?

I tried before suggesting, and it works with a single rule on this structure.

I'm using libabigail current master, which version are you using so I
can try with the same?


-- 
David Marchand