Re: [dpdk-dev] [PATCH v4 05/10] ipsec: add SA data-path API

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Konstantin,

Sorry for a late review. I was on unplanned leave for more than 2 weeks.

On 12/14/2018 9:53 PM, Konstantin Ananyev wrote:
> Introduce Security Association (SA-level) data-path API
> Operates at SA level, provides functions to:
>      - initialize/teardown SA object
>      - process inbound/outbound ESP/AH packets associated with the given SA
>        (decrypt/encrypt, authenticate, check integrity,
>        add/remove ESP/AH related headers and data, etc.).
>
> Signed-off-by: Mohammad Abdul Awal <mohammad.abdul.awal@intel.com>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Acked-by: Declan Doherty <declan.doherty@intel.com>
> ---
>   lib/librte_ipsec/Makefile              |   2 +
>   lib/librte_ipsec/meson.build           |   4 +-
>   lib/librte_ipsec/rte_ipsec.h           | 151 +++++++++++++++++++++++++
>   lib/librte_ipsec/rte_ipsec_version.map |   3 +
>   lib/librte_ipsec/sa.c                  |  21 +++-
>   lib/librte_ipsec/sa.h                  |   4 +
>   lib/librte_ipsec/ses.c                 |  45 ++++++++
>   7 files changed, 227 insertions(+), 3 deletions(-)
>   create mode 100644 lib/librte_ipsec/rte_ipsec.h
>   create mode 100644 lib/librte_ipsec/ses.c
>
> diff --git a/lib/librte_ipsec/Makefile b/lib/librte_ipsec/Makefile
> index 7758dcc6d..79f187fae 100644
> --- a/lib/librte_ipsec/Makefile
> +++ b/lib/librte_ipsec/Makefile
> @@ -17,8 +17,10 @@ LIBABIVER := 1
>   
>   # all source are stored in SRCS-y
>   SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += sa.c
> +SRCS-$(CONFIG_RTE_LIBRTE_IPSEC) += ses.c
>   
>   # install header files
> +SYMLINK-$(CONFIG_RTE_LIBRTE_IPSEC)-include += rte_ipsec.h
>   SYMLINK-$(CONFIG_RTE_LIBRTE_IPSEC)-include += rte_ipsec_sa.h
>   
>   include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/lib/librte_ipsec/meson.build b/lib/librte_ipsec/meson.build
> index 52c78eaeb..6e8c6fabe 100644
> --- a/lib/librte_ipsec/meson.build
> +++ b/lib/librte_ipsec/meson.build
> @@ -3,8 +3,8 @@
>   
>   allow_experimental_apis = true
>   
> -sources=files('sa.c')
> +sources=files('sa.c', 'ses.c')
>   
> -install_headers = files('rte_ipsec_sa.h')
> +install_headers = files('rte_ipsec.h', 'rte_ipsec_sa.h')
>   
>   deps += ['mbuf', 'net', 'cryptodev', 'security']
> diff --git a/lib/librte_ipsec/rte_ipsec.h b/lib/librte_ipsec/rte_ipsec.h
> new file mode 100644
> index 000000000..cbcd861b5
> --- /dev/null
> +++ b/lib/librte_ipsec/rte_ipsec.h
> @@ -0,0 +1,151 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#ifndef _RTE_IPSEC_H_
> +#define _RTE_IPSEC_H_
> +
> +/**
> + * @file rte_ipsec.h
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * RTE IPsec support.
> + * librte_ipsec provides a framework for data-path IPsec protocol
> + * processing (ESP/AH).
> + */
> +
> +#include <rte_ipsec_sa.h>
> +#include <rte_mbuf.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct rte_ipsec_session;
> +
> +/**
> + * IPsec session specific functions that will be used to:
> + * - prepare - for input mbufs and given IPsec session prepare crypto ops
> + *   that can be enqueued into the cryptodev associated with given session
> + *   (see *rte_ipsec_pkt_crypto_prepare* below for more details).
> + * - process - finalize processing of packets after crypto-dev finished
> + *   with them or process packets that are subjects to inline IPsec offload
> + *   (see rte_ipsec_pkt_process for more details).
> + */
> +struct rte_ipsec_sa_pkt_func {
> +	uint16_t (*prepare)(const struct rte_ipsec_session *ss,
> +				struct rte_mbuf *mb[],
> +				struct rte_crypto_op *cop[],
> +				uint16_t num);
> +	uint16_t (*process)(const struct rte_ipsec_session *ss,
> +				struct rte_mbuf *mb[],
> +				uint16_t num);
> +};
> +
> +/**
> + * rte_ipsec_session is an aggregate structure that defines particular
> + * IPsec Security Association IPsec (SA) on given security/crypto device:
> + * - pointer to the SA object
> + * - security session action type
> + * - pointer to security/crypto session, plus other related data
> + * - session/device specific functions to prepare/process IPsec packets.
> + */
> +struct rte_ipsec_session {
> +
extra line
> +	/**
> +	 * SA that session belongs to.
> +	 * Note that multiple sessions can belong to the same SA.
> +	 */
> +	struct rte_ipsec_sa *sa;
> +	/** session action type */
> +	enum rte_security_session_action_type type;
> +	/** session and related data */
> +	union {
> +		struct {
> +			struct rte_cryptodev_sym_session *ses;
> +		} crypto;
> +		struct {
> +			struct rte_security_session *ses;
> +			struct rte_security_ctx *ctx;
> +			uint32_t ol_flags;
> +		} security;
> +	};
> +	/** functions to prepare/process IPsec packets */
> +	struct rte_ipsec_sa_pkt_func pkt_func;
> +} __rte_cache_aligned;
> +
> +/**
> + * Checks that inside given rte_ipsec_session crypto/security fields
> + * are filled correctly and setups function pointers based on these values.
it means user need not fill rte_ipsec_sa_pkt_fun, specify this in the 
comments.
> + * @param ss
> + *   Pointer to the *rte_ipsec_session* object
> + * @return
> + *   - Zero if operation completed successfully.
> + *   - -EINVAL if the parameters are invalid.
> + */
> +int __rte_experimental
> +rte_ipsec_session_prepare(struct rte_ipsec_session *ss);
> +
> +/**
> + * For input mbufs and given IPsec session prepare crypto ops that can be
> + * enqueued into the cryptodev associated with given session.
> + * expects that for each input packet:
> + *      - l2_len, l3_len are setup correctly
> + * Note that erroneous mbufs are not freed by the function,
> + * but are placed beyond last valid mbuf in the *mb* array.
> + * It is a user responsibility to handle them further.
How will the user know how many mbufs are correctly processed.
> + * @param ss
> + *   Pointer to the *rte_ipsec_session* object the packets belong to.
> + * @param mb
> + *   The address of an array of *num* pointers to *rte_mbuf* structures
> + *   which contain the input packets.
> + * @param cop
> + *   The address of an array of *num* pointers to the output *rte_crypto_op*
> + *   structures.
> + * @param num
> + *   The maximum number of packets to process.
> + * @return
> + *   Number of successfully processed packets, with error code set in rte_errno.
> + */
> +static inline uint16_t __rte_experimental
> +rte_ipsec_pkt_crypto_prepare(const struct rte_ipsec_session *ss,
> +	struct rte_mbuf *mb[], struct rte_crypto_op *cop[], uint16_t num)
> +{
> +	return ss->pkt_func.prepare(ss, mb, cop, num);
> +}
> +
> +/**
> + * Finalise processing of packets after crypto-dev finished with them or
> + * process packets that are subjects to inline IPsec offload.
> + * Expects that for each input packet:
> + *      - l2_len, l3_len are setup correctly
> + * Output mbufs will be:
> + * inbound - decrypted & authenticated, ESP(AH) related headers removed,
> + * *l2_len* and *l3_len* fields are updated.
> + * outbound - appropriate mbuf fields (ol_flags, tx_offloads, etc.)
> + * properly setup, if necessary - IP headers updated, ESP(AH) fields added,
> + * Note that erroneous mbufs are not freed by the function,
> + * but are placed beyond last valid mbuf in the *mb* array.
same question
> + * It is a user responsibility to handle them further.
> + * @param ss
> + *   Pointer to the *rte_ipsec_session* object the packets belong to.
> + * @param mb
> + *   The address of an array of *num* pointers to *rte_mbuf* structures
> + *   which contain the input packets.
> + * @param num
> + *   The maximum number of packets to process.
> + * @return
> + *   Number of successfully processed packets, with error code set in rte_errno.
> + */
> +static inline uint16_t __rte_experimental
> +rte_ipsec_pkt_process(const struct rte_ipsec_session *ss, struct rte_mbuf *mb[],
> +	uint16_t num)
> +{
> +	return ss->pkt_func.process(ss, mb, num);
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_IPSEC_H_ */
> diff --git a/lib/librte_ipsec/rte_ipsec_version.map b/lib/librte_ipsec/rte_ipsec_version.map
> index 1a66726b8..d1c52d7ca 100644
> --- a/lib/librte_ipsec/rte_ipsec_version.map
> +++ b/lib/librte_ipsec/rte_ipsec_version.map
> @@ -1,6 +1,9 @@
>   EXPERIMENTAL {
>   	global:
>   
> +	rte_ipsec_pkt_crypto_prepare;
> +	rte_ipsec_session_prepare;
> +	rte_ipsec_pkt_process;
alphabetical order incorrect
>   	rte_ipsec_sa_fini;
>   	rte_ipsec_sa_init;
>   	rte_ipsec_sa_size;
> diff --git a/lib/librte_ipsec/sa.c b/lib/librte_ipsec/sa.c
> index f927a82bf..e4c5361e7 100644
> --- a/lib/librte_ipsec/sa.c
> +++ b/lib/librte_ipsec/sa.c
> @@ -2,7 +2,7 @@
>    * Copyright(c) 2018 Intel Corporation
>    */
>   
> -#include <rte_ipsec_sa.h>
> +#include <rte_ipsec.h>
>   #include <rte_esp.h>
>   #include <rte_ip.h>
>   #include <rte_errno.h>
> @@ -325,3 +325,22 @@ rte_ipsec_sa_init(struct rte_ipsec_sa *sa, const struct rte_ipsec_sa_prm *prm,
>   
>   	return sz;
>   }
> +
> +int
> +ipsec_sa_pkt_func_select(const struct rte_ipsec_session *ss,
> +	const struct rte_ipsec_sa *sa, struct rte_ipsec_sa_pkt_func *pf)
> +{
> +	int32_t rc;
> +
> +	RTE_SET_USED(sa);
> +
> +	rc = 0;
> +	pf[0] = (struct rte_ipsec_sa_pkt_func) { 0 };
> +
> +	switch (ss->type) {
> +	default:
> +		rc = -ENOTSUP;
> +	}
> +
> +	return rc;
> +}
Is this a dummy function? Will it be updated later? I believe should 
have appropriate comments in that case.
> diff --git a/lib/librte_ipsec/sa.h b/lib/librte_ipsec/sa.h
> index 5d113891a..050a6d7ae 100644
> --- a/lib/librte_ipsec/sa.h
> +++ b/lib/librte_ipsec/sa.h
> @@ -74,4 +74,8 @@ struct rte_ipsec_sa {
>   
>   } __rte_cache_aligned;
>   
> +int
> +ipsec_sa_pkt_func_select(const struct rte_ipsec_session *ss,
> +	const struct rte_ipsec_sa *sa, struct rte_ipsec_sa_pkt_func *pf);
> +
>   #endif /* _SA_H_ */
> diff --git a/lib/librte_ipsec/ses.c b/lib/librte_ipsec/ses.c
> new file mode 100644
> index 000000000..562c1423e
> --- /dev/null
> +++ b/lib/librte_ipsec/ses.c
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Intel Corporation
> + */
> +
> +#include <rte_ipsec.h>
> +#include "sa.h"
> +
> +static int
> +session_check(struct rte_ipsec_session *ss)
> +{
> +	if (ss == NULL || ss->sa == NULL)
> +		return -EINVAL;
> +
> +	if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE) {
> +		if (ss->crypto.ses == NULL)
> +			return -EINVAL;
> +	} else if (ss->security.ses == NULL || ss->security.ctx == NULL)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +int __rte_experimental
> +rte_ipsec_session_prepare(struct rte_ipsec_session *ss)
> +{
> +	int32_t rc;
> +	struct rte_ipsec_sa_pkt_func fp;
> +
> +	rc = session_check(ss);
> +	if (rc != 0)
> +		return rc;
> +
> +	rc = ipsec_sa_pkt_func_select(ss, ss->sa, &fp);
> +	if (rc != 0)
> +		return rc;
> +
> +	ss->pkt_func = fp;
> +
> +	if (ss->type == RTE_SECURITY_ACTION_TYPE_NONE)
> +		ss->crypto.ses->opaque_data = (uintptr_t)ss;
> +	else
> +		ss->security.ses->opaque_data = (uintptr_t)ss;
> +
> +	return 0;
> +}