Re: [dpdk-dev] [PATCH v2 1/6] security: add SA lifetime configuration

[Anoob Joseph <anoobj@marvell.com>]

Hi Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, September 16, 2021 4:36 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> <gakhil@marvell.com>; Doherty, Declan <declan.doherty@intel.com>;
> Zhang, Roy Fan <roy.fan.zhang@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Archana Muniganti
> <marchana@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>;
> Hemant Agrawal <hemant.agrawal@nxp.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; Power, Ciara <ciara.power@intel.com>;
> Gagandeep Singh <g.singh@nxp.com>; dev@dpdk.org
> Subject: [EXT] RE: [PATCH v2 1/6] security: add SA lifetime configuration
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> > Add SA lifetime configuration to register soft and hard expiry limits.
> > Expiry can be in units of number of packets or bytes. Crypto op status
> > is also updated to include new field, aux_flags, which can be used to
> > indicate cases such as soft expiry in case of lookaside protocol
> > operations.
> >
> > In case of soft expiry, the packets are successfully IPsec processed
> > but the soft expiry would indicate that SA needs to be reconfigured.
> > For inline protocol capable ethdev, this would result in an eth event
> > while for lookaside protocol capable cryptodev, this can be
> > communicated via `rte_crypto_op.aux_flags` field.
> >
> > In case of hard expiry, the packets will not be IPsec processed and
> > would result in error.
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > ---
> >  .../test_cryptodev_security_ipsec_test_vectors.h   |  3 ---
> >  doc/guides/rel_notes/deprecation.rst               |  5 ----
> >  doc/guides/rel_notes/release_21_11.rst             | 13 ++++++++++
> >  examples/ipsec-secgw/ipsec.c                       |  2 +-
> >  examples/ipsec-secgw/ipsec.h                       |  2 +-
> >  lib/cryptodev/rte_crypto.h                         | 18 +++++++++++++-
> >  lib/security/rte_security.h                        | 28 ++++++++++++++++++++--
> >  7 files changed, 58 insertions(+), 13 deletions(-)
> >
> > diff --git a/app/test/test_cryptodev_security_ipsec_test_vectors.h
> > b/app/test/test_cryptodev_security_ipsec_test_vectors.h
> > index ae9cd24..38ea43d 100644
> > --- a/app/test/test_cryptodev_security_ipsec_test_vectors.h
> > +++ b/app/test/test_cryptodev_security_ipsec_test_vectors.h
> > @@ -98,7 +98,6 @@ struct ipsec_test_data pkt_aes_128_gcm = {
> >  		.proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP,
> >  		.mode = RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
> >  		.tunnel.type = RTE_SECURITY_IPSEC_TUNNEL_IPV4,
> > -		.esn_soft_limit = 0,
> >  		.replay_win_sz = 0,
> >  	},
> >
> > @@ -195,7 +194,6 @@ struct ipsec_test_data pkt_aes_192_gcm = {
> >  		.proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP,
> >  		.mode = RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
> >  		.tunnel.type = RTE_SECURITY_IPSEC_TUNNEL_IPV4,
> > -		.esn_soft_limit = 0,
> >  		.replay_win_sz = 0,
> >  	},
> >
> > @@ -295,7 +293,6 @@ struct ipsec_test_data pkt_aes_256_gcm = {
> >  		.proto = RTE_SECURITY_IPSEC_SA_PROTO_ESP,
> >  		.mode = RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
> >  		.tunnel.type = RTE_SECURITY_IPSEC_TUNNEL_IPV4,
> > -		.esn_soft_limit = 0,
> >  		.replay_win_sz = 0,
> >  	},
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index 76a4abf..6118f06 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -282,8 +282,3 @@ Deprecation Notices
> >  * security: The functions ``rte_security_set_pkt_metadata`` and
> >    ``rte_security_get_userdata`` will be made inline functions and additional
> >    flags will be added in structure ``rte_security_ctx`` in DPDK 21.11.
> > -
> > -* cryptodev: The structure ``rte_crypto_op`` would be updated to
> > reduce
> > -  reserved bytes to 2 (from 3), and use 1 byte to indicate warnings
> > and other
> > -  information from the crypto/security operation. This field will be
> > used to
> > -  communicate events such as soft expiry with IPsec in lookaside mode.
> > diff --git a/doc/guides/rel_notes/release_21_11.rst
> > b/doc/guides/rel_notes/release_21_11.rst
> > index 9b14c84..0e3ed28 100644
> > --- a/doc/guides/rel_notes/release_21_11.rst
> > +++ b/doc/guides/rel_notes/release_21_11.rst
> > @@ -102,6 +102,13 @@ API Changes
> >     Also, make sure to start the actual text at the margin.
> >     =======================================================
> >
> > +* cryptodev: use 1 reserved byte from ``rte_crypto_op`` for aux flags
> > +
> > +  * Updated the structure ``rte_crypto_op`` to reduce reserved bytes
> > + to
> > +  2 (from 3), and use 1 byte to indicate warnings and other
> > + information from  the crypto/security operation. This field will be
> > + used to communicate events  such as soft expiry with IPsec in lookaside
> mode.
> > +
> >
> >  ABI Changes
> >  -----------
> > @@ -123,6 +130,12 @@ ABI Changes
> >    * Added IPsec SA option to disable IV generation to allow known vector
> >      tests as well as usage of application provided IV on supported PMDs.
> >
> > +* security: add IPsec SA lifetime configuration
> > +
> > +  * Added IPsec SA lifetime configuration to allow applications to configure
> > +    soft and hard SA expiry limits. Limits can be either in units of packets or
> > +    bytes.
> > +
> >
> >  Known Issues
> >  ------------
> > diff --git a/examples/ipsec-secgw/ipsec.c
> > b/examples/ipsec-secgw/ipsec.c index 5b032fe..4868294 100644
> > --- a/examples/ipsec-secgw/ipsec.c
> > +++ b/examples/ipsec-secgw/ipsec.c
> > @@ -49,7 +49,7 @@ set_ipsec_conf(struct ipsec_sa *sa, struct
> rte_security_ipsec_xform *ipsec)
> >  		}
> >  		/* TODO support for Transport */
> >  	}
> > -	ipsec->esn_soft_limit = IPSEC_OFFLOAD_ESN_SOFTLIMIT;
> > +	ipsec->life.packets_soft_limit = IPSEC_OFFLOAD_PKTS_SOFTLIMIT;
> >  	ipsec->replay_win_sz = app_sa_prm.window_size;
> >  	ipsec->options.esn = app_sa_prm.enable_esn;
> >  	ipsec->options.udp_encap = sa->udp_encap; diff --git
> > a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h index
> > ae5058d..90c81c1 100644
> > --- a/examples/ipsec-secgw/ipsec.h
> > +++ b/examples/ipsec-secgw/ipsec.h
> > @@ -23,7 +23,7 @@
> >
> >  #define MAX_DIGEST_SIZE 32 /* Bytes -- 256 bits */
> >
> > -#define IPSEC_OFFLOAD_ESN_SOFTLIMIT 0xffffff00
> > +#define IPSEC_OFFLOAD_PKTS_SOFTLIMIT 0xffffff00
> >
> >  #define IV_OFFSET		(sizeof(struct rte_crypto_op) + \
> >  				sizeof(struct rte_crypto_sym_op)) diff --git
> > a/lib/cryptodev/rte_crypto.h b/lib/cryptodev/rte_crypto.h index
> > fd5ef3a..d602183 100644
> > --- a/lib/cryptodev/rte_crypto.h
> > +++ b/lib/cryptodev/rte_crypto.h
> > @@ -66,6 +66,17 @@ enum rte_crypto_op_sess_type {  };
> >
> >  /**
> > + * Auxiliary flags to indicate additional info from the operation */
> > +
> > +/**
> > + * Auxiliary flags related to IPsec offload with RTE_SECURITY  */
> 
> Duplicate comments.

[Anoob] The proposal is to make auxiliary flags custom to operation. Like, flags related to IPsec offload may not be applicable for PDCP offload (and vice versa). But then, I agree these could be updated as we add new fields related to other kinds of operations. I'll drop the extra comments in the next version.
 
> 
> > +
> > +#define RTE_CRYPTO_OP_AUX_FLAGS_IPSEC_SOFT_EXPIRY (1 << 0) /**<
> SA
> > +soft expiry limit has been reached */
> > +
> > +/**
> >   * Cryptographic Operation.
> >   *
> >   * This structure contains data relating to performing cryptographic
> > @@ -93,7 +104,12 @@ struct rte_crypto_op {
> >  			 */
> >  			uint8_t sess_type;
> >  			/**< operation session type */
> > -			uint8_t reserved[3];
> > +			uint8_t aux_flags;
> > +			/**< Operation specific auxiliary/additional flags.
> > +			 * These flags carry additional information from the
> > +			 * operation. Processing of the same is optional.
> > +			 */
> > +			uint8_t reserved[2];
> >  			/**< Reserved bytes to fill 64 bits for
> >  			 * future additions
> >  			 */
> > diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h
> > index b4b6776..95c169d 100644
> > --- a/lib/security/rte_security.h
> > +++ b/lib/security/rte_security.h
> > @@ -206,6 +206,30 @@ enum rte_security_ipsec_sa_direction {  };
> >
> >  /**
> > + * Configure soft and hard lifetime of an IPsec SA
> > + *
> > + * Lifetime of an IPsec SA would specify the maximum number of
> > +packets or bytes
> > + * that can be processed. IPsec operations would start failing once
> > +any hard
> > + * limit is reached.
> > + *
> > + * Soft limits can be specified to generate notification when the SA
> > +is
> > + * approaching hard limits for lifetime. For inline operations,
> > +reaching soft
> > + * expiry limit would result in raising an eth event for the same.
> > +For lookaside
> > + * operations, this would result in a warning returned in
> > + * ``rte_crypto_op.aux_flags``.
> > + */
> > +struct rte_security_ipsec_lifetime {
> > +	uint64_t packets_soft_limit;
> > +	/**< Soft expiry limit in number of packets */
> > +	uint64_t bytes_soft_limit;
> > +	/**< Soft expiry limit in bytes */
> > +	uint64_t packets_hard_limit;
> > +	/**< Soft expiry limit in number of packets */
> > +	uint64_t bytes_hard_limit;
> > +	/**< Soft expiry limit in bytes */
> > +};
> > +
> > +/**
> >   * IPsec security association configuration data.
> >   *
> >   * This structure contains data required to create an IPsec SA security
> session.
> > @@ -225,8 +249,8 @@ struct rte_security_ipsec_xform {
> >  	/**< IPsec SA Mode - transport/tunnel */
> >  	struct rte_security_ipsec_tunnel_param tunnel;
> >  	/**< Tunnel parameters, NULL for transport mode */
> > -	uint64_t esn_soft_limit;
> > -	/**< ESN for which the overflow event need to be raised */
> > +	struct rte_security_ipsec_lifetime life;
> > +	/**< IPsec SA lifetime */
> >  	uint32_t replay_win_sz;
> >  	/**< Anti replay window size to enable sequence replay attack
> handling.
> >  	 * replay checking is disabled if the window size is 0.
> > --
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> 
> > 2.7.4