[v2,3/3] security: add reserved bitfields
Checks
Commit Message
In struct rte_security_ipsec_sa_options, for every new option
added, there is an ABI breakage, to avoid, a reserved_opts
bitfield is added to for the remaining bits available in the
structure.
Now for every new sa option, these reserved_opts can be reduced
and new option can be added.
Signed-off-by: Akhil Goyal <gakhil@marvell.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
v2: rebase and removed libabigail.abignore change.
Exception may be added when there is a need for change.
lib/security/rte_security.h | 6 ++++++
1 file changed, 6 insertions(+)
Comments
08/10/2021 22:45, Akhil Goyal:
> In struct rte_security_ipsec_sa_options, for every new option
> added, there is an ABI breakage, to avoid, a reserved_opts
> bitfield is added to for the remaining bits available in the
> structure.
> Now for every new sa option, these reserved_opts can be reduced
> and new option can be added.
How do you make sure this field is initialized to 0?
> 08/10/2021 22:45, Akhil Goyal:
> > In struct rte_security_ipsec_sa_options, for every new option
> > added, there is an ABI breakage, to avoid, a reserved_opts
> > bitfield is added to for the remaining bits available in the
> > structure.
> > Now for every new sa option, these reserved_opts can be reduced
> > and new option can be added.
>
> How do you make sure this field is initialized to 0?
>
Struct rte_security_ipsec_xform Is part of rte_security_capability as well
As a configuration structure in session create.
User, should ensure that if a device support that option(in capability), then
only these options will take into effect or else it will be don't care for the PMD.
The initial values of capabilities are set by PMD statically based on the features
that it support.
So if someone sets a bit in reserved_opts, it will work only if PMD support it
And sets the corresponding field in capabilities.
But yes, if a new field is added in future, and user sets the reserved_opts by mistake
And the PMD supports that feature as well, then that feature will be enabled.
This may or may not create issue depending on the feature which is enabled.
Should I add a note in the comments to clarify that reserved_opts should be set as 0
And future releases may change this without notice(But reserved in itself suggest that)?
Adding an explicit check in session_create does not make sense to me.
What do you suggest?
Regards,
Akhil
On Mon, 11 Oct 2021 16:58:24 +0000
Akhil Goyal <gakhil@marvell.com> wrote:
> > 08/10/2021 22:45, Akhil Goyal:
> > > In struct rte_security_ipsec_sa_options, for every new option
> > > added, there is an ABI breakage, to avoid, a reserved_opts
> > > bitfield is added to for the remaining bits available in the
> > > structure.
> > > Now for every new sa option, these reserved_opts can be reduced
> > > and new option can be added.
> >
> > How do you make sure this field is initialized to 0?
> >
> Struct rte_security_ipsec_xform Is part of rte_security_capability as well
> As a configuration structure in session create.
> User, should ensure that if a device support that option(in capability), then
> only these options will take into effect or else it will be don't care for the PMD.
> The initial values of capabilities are set by PMD statically based on the features
> that it support.
> So if someone sets a bit in reserved_opts, it will work only if PMD support it
> And sets the corresponding field in capabilities.
> But yes, if a new field is added in future, and user sets the reserved_opts by mistake
> And the PMD supports that feature as well, then that feature will be enabled.
> This may or may not create issue depending on the feature which is enabled.
>
> Should I add a note in the comments to clarify that reserved_opts should be set as 0
> And future releases may change this without notice(But reserved in itself suggest that)?
> Adding an explicit check in session_create does not make sense to me.
> What do you suggest?
>
> Regards,
> Akhil
>
The problem is if user creates an on stack variable and sets the unreserved
fields to good values but other parts are garbage. This passes API/ABI unless
you strictly enforce that all reserved fields are zero.
11/10/2021 18:58, Akhil Goyal:
> > 08/10/2021 22:45, Akhil Goyal:
> > > In struct rte_security_ipsec_sa_options, for every new option
> > > added, there is an ABI breakage, to avoid, a reserved_opts
> > > bitfield is added to for the remaining bits available in the
> > > structure.
> > > Now for every new sa option, these reserved_opts can be reduced
> > > and new option can be added.
> >
> > How do you make sure this field is initialized to 0?
> >
> Struct rte_security_ipsec_xform Is part of rte_security_capability as well
> As a configuration structure in session create.
> User, should ensure that if a device support that option(in capability), then
> only these options will take into effect or else it will be don't care for the PMD.
> The initial values of capabilities are set by PMD statically based on the features
> that it support.
> So if someone sets a bit in reserved_opts, it will work only if PMD support it
> And sets the corresponding field in capabilities.
> But yes, if a new field is added in future, and user sets the reserved_opts by mistake
> And the PMD supports that feature as well, then that feature will be enabled.
> This may or may not create issue depending on the feature which is enabled.
>
> Should I add a note in the comments to clarify that reserved_opts should be set as 0
> And future releases may change this without notice(But reserved in itself suggest that)?
> Adding an explicit check in session_create does not make sense to me.
> What do you suggest?
Yes at the minimum you should add a comment.
You could also initialize it in the lib, but it is not always possible.
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday 11 October 2021 23:16
> To: Akhil Goyal <gakhil@marvell.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>; dev@dpdk.org;
> david.marchand@redhat.com; hemant.agrawal@nxp.com; Anoob Joseph
> <anoobj@marvell.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>;
> Doherty, Declan <declan.doherty@intel.com>; matan@nvidia.com;
> g.singh@nxp.com; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> jianjay.zhou@huawei.com; asomalap@amd.com; ruifeng.wang@arm.com;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; ajit.khaparde@broadcom.com; Nagadheeraj
> Rottela <rnagadheeraj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; Power, Ciara <ciara.power@intel.com>; Kinsella,
> Ray <ray.kinsella@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Subject: Re: [EXT] Re: [PATCH v2 3/3] security: add reserved bitfields
>
> On Mon, 11 Oct 2021 16:58:24 +0000
> Akhil Goyal <gakhil@marvell.com> wrote:
>
> > > 08/10/2021 22:45, Akhil Goyal:
> > > > In struct rte_security_ipsec_sa_options, for every new option
> > > > added, there is an ABI breakage, to avoid, a reserved_opts
> > > > bitfield is added to for the remaining bits available in the
> > > > structure.
> > > > Now for every new sa option, these reserved_opts can be reduced
> > > > and new option can be added.
> > >
> > > How do you make sure this field is initialized to 0?
> > >
> > Struct rte_security_ipsec_xform Is part of rte_security_capability as
> > well As a configuration structure in session create.
> > User, should ensure that if a device support that option(in
> > capability), then only these options will take into effect or else it
> will be don't care for the PMD.
> > The initial values of capabilities are set by PMD statically based on
> > the features that it support.
> > So if someone sets a bit in reserved_opts, it will work only if PMD
> > support it And sets the corresponding field in capabilities.
> > But yes, if a new field is added in future, and user sets the
> > reserved_opts by mistake And the PMD supports that feature as well,
> then that feature will be enabled.
> > This may or may not create issue depending on the feature which is
> enabled.
> >
> > Should I add a note in the comments to clarify that reserved_opts
> > should be set as 0 And future releases may change this without
> notice(But reserved in itself suggest that)?
> > Adding an explicit check in session_create does not make sense to me.
> > What do you suggest?
> >
> > Regards,
> > Akhil
> >
>
> The problem is if user creates an on stack variable and sets the
> unreserved fields to good values but other parts are garbage. This
> passes API/ABI unless you strictly enforce that all reserved fields are
> zero.
Right, but that is no better or worse than the current struct, in that respect, right?
User case be careless there also - declare it on the stack and forget to memset.
struct rte_security_ipsec_sa_options {
uint32_t esn : 1;
uint32_t udp_encap : 1;
uint32_t copy_dscp : 1;
uint32_t copy_flabel : 1;
uint32_t copy_df : 1;
uint32_t dec_ttl : 1;
uint32_t ecn : 1;
uint32_t stats : 1;
uint32_t iv_gen_disable : 1;
uint32_t tunnel_hdr_verify : 2;
};
On 08/10/2021 21:45, Akhil Goyal wrote:
> In struct rte_security_ipsec_sa_options, for every new option
> added, there is an ABI breakage, to avoid, a reserved_opts
> bitfield is added to for the remaining bits available in the
> structure.
> Now for every new sa option, these reserved_opts can be reduced
> and new option can be added.
>
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> v2: rebase and removed libabigail.abignore change.
> Exception may be added when there is a need for change.
>
> lib/security/rte_security.h | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
> index 7eb9f109ae..c0ea13892e 100644
> --- a/lib/security/rte_security.h
> +++ b/lib/security/rte_security.h
> @@ -258,6 +258,12 @@ struct rte_security_ipsec_sa_options {
> * PKT_TX_UDP_CKSUM or PKT_TX_L4_MASK in mbuf.
> */
> uint32_t l4_csum_enable : 1;
> +
> + /** Reserved bit fields for future extension
> + *
> + * Note: reduce number of bits in reserved_opts for every new option
> + */
> + uint32_t reserved_opts : 18;
> };
>
> /** IPSec security association direction */
>
Acked-by: Ray Kinsella <mdr@ashroe.eu>
On 12/10/2021 07:59, Thomas Monjalon wrote:
> 11/10/2021 18:58, Akhil Goyal:
>>> 08/10/2021 22:45, Akhil Goyal:
>>>> In struct rte_security_ipsec_sa_options, for every new option
>>>> added, there is an ABI breakage, to avoid, a reserved_opts
>>>> bitfield is added to for the remaining bits available in the
>>>> structure.
>>>> Now for every new sa option, these reserved_opts can be reduced
>>>> and new option can be added.
>>>
>>> How do you make sure this field is initialized to 0?
>>>
>> Struct rte_security_ipsec_xform Is part of rte_security_capability as well
>> As a configuration structure in session create.
>> User, should ensure that if a device support that option(in capability), then
>> only these options will take into effect or else it will be don't care for the PMD.
>> The initial values of capabilities are set by PMD statically based on the features
>> that it support.
>> So if someone sets a bit in reserved_opts, it will work only if PMD support it
>> And sets the corresponding field in capabilities.
>> But yes, if a new field is added in future, and user sets the reserved_opts by mistake
>> And the PMD supports that feature as well, then that feature will be enabled.
>> This may or may not create issue depending on the feature which is enabled.
>>
>> Should I add a note in the comments to clarify that reserved_opts should be set as 0
>> And future releases may change this without notice(But reserved in itself suggest that)?
>> Adding an explicit check in session_create does not make sense to me.
>> What do you suggest?
>
> Yes at the minimum you should add a comment.
> You could also initialize it in the lib, but it is not always possible.
>
Provide a macro for initialization perhaps ... but there would be no way to enforce using it.
Ray K
@@ -258,6 +258,12 @@ struct rte_security_ipsec_sa_options {
* PKT_TX_UDP_CKSUM or PKT_TX_L4_MASK in mbuf.
*/
uint32_t l4_csum_enable : 1;
+
+ /** Reserved bit fields for future extension
+ *
+ * Note: reduce number of bits in reserved_opts for every new option
+ */
+ uint32_t reserved_opts : 18;
};
/** IPSec security association direction */