[v4,3/3] security: add IPsec option for IP reassembly
Checks
Commit Message
A new option is added in IPsec to enable and attempt reassembly
of inbound packets.
Signed-off-by: Akhil Goyal <gakhil@marvell.com>
Change-Id: I6f66f0b5a659550976a32629130594070cb16cb1
---
devtools/libabigail.abignore | 14 ++++++++++++++
lib/security/rte_security.h | 12 +++++++++++-
2 files changed, 25 insertions(+), 1 deletion(-)
Comments
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)?
>
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> Change-Id: I6f66f0b5a659550976a32629130594070cb16cb1
^^^
Internal tag, please 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}
> 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.
>
>
> >
> > 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?
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?
> > > > 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?
>
I am currently using 1.6 version. I will try with latest version.
$ abidiff --version
abidiff: 1.6.0
and I get following issue after removing the last two suppress rules.
Functions changes summary: 0 Removed, 1 Changed (8 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:158:1 has some indirect sub-type changes:
return type changed:
in pointed to type 'const rte_security_capability':
in unqualified underlying type 'struct rte_security_capability' at rte_security.h:808:1:
type size hasn't changed
1 data member change:
parameter 1 of type 'rte_security_ctx*' has sub-type changes:
in pointed to type 'struct rte_security_ctx' at rte_security.h:72:1:
type size hasn't changed
1 data member change:
type of 'const rte_security_ops* rte_security_ctx::ops' changed:
in pointed to type 'const rte_security_ops':
in unqualified underlying type 'struct rte_security_ops' at rte_security_driver.h:140:1:
type size hasn't changed
1 data member changes (2 filtered):
type of 'security_session_create_t rte_security_ops::session_create' changed:
underlying type 'int (void*, rte_security_session_conf*, rte_security_session*, rte_mempool*)*' changed:
in pointed to type 'function type int (void*, rte_security_session_conf*, rte_security_session*, rte_mempool*)':
parameter 2 of type 'rte_security_session_conf*' has sub-type changes:
in pointed to type 'struct rte_security_session_conf' at rte_security.h:502:1:
type size hasn't changed
1 data member change:
Hi David,
> > > > > 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?
> >
> I am currently using 1.6 version. I will try with latest version.
> $ abidiff --version
> abidiff: 1.6.0
>
It seems the latest version 2.0 is not compatible with Ubuntu 20.04.
It is not getting compiled.
Can you check with 1.6.0 version?
On Tue, Feb 8, 2022 at 2:19 PM Akhil Goyal <gakhil@marvell.com> wrote:
> > > > 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?
> > >
> > I am currently using 1.6 version. I will try with latest version.
> > $ abidiff --version
> > abidiff: 1.6.0
> >
> It seems the latest version 2.0 is not compatible with Ubuntu 20.04.
> It is not getting compiled.
I am using the HEAD of libabigail master branch, so maybe something
got fixed between 2.0 and the current master.
> Can you check with 1.6.0 version?
I tried 1.6 in GHA (Ubuntu 18.04), and I can reproduce the warnings
you reported.
But in the end, we use 1.8 in GHA:
https://git.dpdk.org/dpdk/tree/.github/workflows/build.yml#n23
The simplest rule (on rte_security_ipsec_sa_options only) passes fine
with this version of libabigail:
https://github.com/david-marchand/dpdk/runs/5109221298?check_suite_focus=true
Hi David,
> On Tue, Feb 8, 2022 at 2:19 PM Akhil Goyal <gakhil@marvell.com> wrote:
> > > > > 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?
> > > >
> > > I am currently using 1.6 version. I will try with latest version.
> > > $ abidiff --version
> > > abidiff: 1.6.0
> > >
> > It seems the latest version 2.0 is not compatible with Ubuntu 20.04.
> > It is not getting compiled.
>
> I am using the HEAD of libabigail master branch, so maybe something
> got fixed between 2.0 and the current master.
>
>
> > Can you check with 1.6.0 version?
>
> I tried 1.6 in GHA (Ubuntu 18.04), and I can reproduce the warnings
> you reported.
>
> But in the end, we use 1.8 in GHA:
> https://git.dpdk.org/dpdk/tree/.github/workflows/build.yml#n23
>
> The simplest rule (on rte_security_ipsec_sa_options only) passes fine
> with this version of libabigail:
> https://github.com/david-marchand/dpdk/runs/5109221298?check_suite_focus=true
Thanks for trying it out. I will remove the last two rules and send next version.
@@ -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)}
@@ -264,6 +264,16 @@ struct rte_security_ipsec_sa_options {
*/
uint32_t l4_csum_enable : 1;
+ /** Enable reassembly on incoming packets.
+ *
+ * * 1: Enable driver to try reassembly of encrypted IP packets for
+ * this SA, if supported by the driver. This feature will work
+ * only if rx_offload RTE_ETH_RX_OFFLOAD_IP_REASSEMBLY is set in
+ * inline Ethernet device.
+ * * 0: Disable reassembly of packets (default).
+ */
+ uint32_t reass_en : 1;
+
/** Reserved bit fields for future extension
*
* User should ensure reserved_opts is cleared as it may change in
@@ -271,7 +281,7 @@ struct rte_security_ipsec_sa_options {
*
* Note: Reduce number of bits in reserved_opts for every new option.
*/
- uint32_t reserved_opts : 18;
+ uint32_t reserved_opts : 17;
};
/** IPSec security association direction */