Re: [dpdk-dev] [PATCH 01/11] lib/rte_security: add security library

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Thu, 14 Sep 2017 13:56:41 +0530
> From: Akhil Goyal <akhil.goyal@nxp.com>
> To: dev@dpdk.org
> CC: declan.doherty@intel.com, pablo.de.lara.guarch@intel.com,
>  hemant.agrawal@nxp.com, radu.nicolau@intel.com, borisp@mellanox.com,
>  aviadye@mellanox.com, thomas@monjalon.net, sandeep.malik@nxp.com,
>  jerin.jacob@caviumnetworks.com
> Subject: [PATCH 01/11] lib/rte_security: add security library
> X-Mailer: git-send-email 2.9.3
> 
> rte_security library provides APIs for security session
> create/free for protocol offload or offloaded crypto
> operation to ethernet device.

Overall the API semantic looks good. A few comments inlined.
I think, This patch should split as minimum two. One just the
specification header file and other one implementation.


> 
> Signed-off-by: Akhil Goyal <akhil.goyal@nxp.com>
> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> Signed-off-by: Radu Nicolau <radu.nicolau@intel.com>
> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> ---
> +
> +#include <rte_malloc.h>
> +#include <rte_dev.h>
> +
> +#include "rte_security.h"
> +#include "rte_security_driver.h"
> +
> +#define RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ	(8)
> +
> +struct rte_security_ctx {
> +	uint16_t id;
> +	enum {
> +		RTE_SECURITY_INSTANCE_INVALID = 0,

explicit zero is not required.

> +		RTE_SECURITY_INSTANCE_VALID
> +	} state;
> +	void *device;
> +	struct rte_security_ops *ops;
> +};
> +
> +
> +int
> +rte_security_register(uint16_t *id, void *device,
> +		      struct rte_security_ops *ops)
> +{
> +	if (max_nb_security_instances == 0) {
> +		security_instances = rte_malloc(
> +				"rte_security_instances_ops",
> +				sizeof(*security_instances) *
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ, 0);
> +
> +		if (security_instances == NULL)
> +			return -ENOMEM;
> +		max_nb_security_instances =
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> +	} else if (nb_security_instances >= max_nb_security_instances) {
> +		uint16_t *instances = rte_realloc(security_instances,
> +				sizeof(struct rte_security_ops *) *
> +				(max_nb_security_instances +
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ), 0);
> +
> +		if (instances == NULL)
> +			return -ENOMEM;
> +
> +		max_nb_security_instances +=
> +				RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ;
> +	}
> +
> +	*id = nb_security_instances++;
> +
> +	security_instances[*id].id = *id;
> +	security_instances[*id].state = RTE_SECURITY_INSTANCE_VALID;
> +	security_instances[*id].device = device;
> +	security_instances[*id].ops = ops;

This whole thing will break in multi process case where ops needs to
cloned for each process. Check the mempool library as reference.


> +
> +	return 0;
> +}
> +
> +int
> +rte_security_unregister(__rte_unused uint16_t *id)
> +{
> +	/* To be implemented */

This should implemented before it reaches to master.

> +	return 0;
> +}
> +
> +struct rte_security_session *
> +int
> +rte_security_set_pkt_metadata(uint16_t id,
> +			      struct rte_security_session *sess,
> +			      struct rte_mbuf *m, void *params)
> +{
> +	struct rte_security_ctx *instance;
> +
> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
> +	instance = &security_instances[id];
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);

Do you need all this checking for a fastpath function?

> +	return instance->ops->set_pkt_metadata(instance->device,
> +					       sess, m, params);
> +}
> +
> +
> +/**
> + * @file rte_security.h
> + *
> + * RTE Security Common Definitions
> + *
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <netinet/in.h>
> +#include <netinet/ip.h>
> +#include <netinet/ip6.h>
> +
> +#include <rte_mbuf.h>
> +#include <rte_memory.h>
> +#include <rte_mempool.h>
> +#include <rte_common.h>
> +#include <rte_crypto.h>

Nice to have it in alphabetical order.

> +
> +/** IPSec protocol mode */
> +enum rte_security_ipsec_sa_mode {
> +	RTE_SECURITY_IPSEC_SA_MODE_TRANSPORT,
> +	/**< IPSec Transport mode */
> +	RTE_SECURITY_IPSEC_SA_MODE_TUNNEL,
> +	/**< IPSec Tunnel mode */
> +};
> +
> +/** IPSec Protocol */
> +enum rte_security_ipsec_sa_protocol {
> +	RTE_SECURITY_IPSEC_SA_PROTO_AH,
> +	/**< AH protocol */
> +	RTE_SECURITY_IPSEC_SA_PROTO_ESP,
> +	/**< ESP protocol */
> +};
> +
> +/** IPSEC tunnel type */
> +enum rte_security_ipsec_tunnel_type {
> +	RTE_SECURITY_IPSEC_TUNNEL_IPV4 = 0,

Explicit zero may not be required.

> +	/**< Outer header is IPv4 */
> +	RTE_SECURITY_IPSEC_TUNNEL_IPV6,
> +	/**< Outer header is IPv6 */
> +};


> +struct rte_security_ipsec_tunnel_param {
> +	enum rte_security_ipsec_tunnel_type type;
> +	/**< Tunnel type: IPv4 or IPv6 */
> +

Anonymous union, You need RTE_STD_C11 here.
> +
> +	union {

> +
> +
> +/**
> + * IPsec Security Association option flags
> + */
> +struct rte_security_ipsec_sa_options {
> +	/** Extended Sequence Numbers (ESN)

All the elements in this structure is missing the doxygen commenting scheme.
i.e starting with /**<

> +	  *
> +	  * * 1: Use extended (64 bit) sequence numbers
> +	  * * 0: Use normal sequence numbers
> +	  */
> +	uint32_t esn : 1;
> +
> +	/** UDP encapsulation
> +	  *
> +	  * * 1: Do UDP encapsulation/decapsulation so that IPSEC packets can
> +	  *      traverse through NAT boxes.
> +	  * * 0: No UDP encapsulation
> +	  */
> +	uint32_t udp_encap : 1;
> +
> +
> +struct rte_security_session {
> +	__extension__ void *sess_private_data;

Do we need an __extension__ here?

> +	/**< Private session material */
> +};
> +
> +/**
> + * Create security session as specified by the session configuration
> + *
> + * @param   id		security instance identifier id

Bad alignment. Check the doxygen alignment everywhere.

> + * @param   conf	session configuration parameters
> + * @param   mp		mempool to allocate session objects from
> + * @return
> + *  - On success, pointer to session
> + *  - On failure, NULL
> + */
> +struct rte_security_session *
> +rte_security_session_create(uint16_t id,
> +			    struct rte_security_session_conf *conf,

const struct rte_security_session_conf *conf ?

> +			    struct rte_mempool *mp);

const struct rte_mempool *mp?

> +
> +/**
> + * Update security session as specified by the session configuration
> + *
> + * @param   id		security instance identifier id
> + * @param   sess	session to update parameters
> + * @param   conf	update configuration parameters
> + * @return
> + *  - On success returns 0
> + *  - On failure return errno
> + */
> +int
> +rte_security_session_update(uint16_t id,
> +			    struct rte_security_session *sess,
> +			    struct rte_security_session_conf *conf);

const ?

> +
> +/**
> + * Free security session header and the session private data and
> + * return it to its original mempool.
> + *
> + * @param   id		security instance identifier id
> + * @param   sess	security session to freed
> + *
> + * @return
> + *  - 0 if successful.
> + *  - -EINVAL if session is NULL.
> + *  - -EBUSY if not all device private data has been freed.
> + */
> +int
> +rte_security_session_destroy(uint16_t id, struct rte_security_session *sess);
> +
> +/**
> + *  Updates the buffer with device-specific defined metadata
> + *

Mention that it needs to be called when DEV_TX_OFFLOAD_SEC_NEED_MDATA is set or
whatever name we are coming up for DEV_TX_OFFLOAD_SEC_NEED_MDATA.

> + * @param	id	security instance identifier id
> + * @param	sess	security session
> + * @param	m	packet mbuf to set metadata on.
> + * @param	params	device-specific defined parameters required for metadata
> + *
> + * @return
> + *  - On success, zero.
> + *  - On failure, a negative value.
> + */
> +int
> +rte_security_set_pkt_metadata(uint16_t id,
> +			      struct rte_security_session *sess,
> +			      struct rte_mbuf *mb, void *params);
> +
> +/**
> + * Attach a session to a crypto operation.
> + * This API is needed only in case of RTE_SECURITY_SESS_CRYPTO_PROTO_OFFLOAD
> + * For other rte_security_session_action_type, ol_flags in rte_mbuf may be
> + * defined to perform security operations.
> + *
> + * @param	op	crypto operation
> + * @param	sess	security session
> + */
> +static inline int
> +rte_security_attach_session(struct rte_crypto_op *op,
> +			    struct rte_security_session *sess)
> +{
> +	if (unlikely(op->type != RTE_CRYPTO_OP_TYPE_SYMMETRIC))
> +		return -1;

-EINVAL?

> +
> +	op->sess_type =  RTE_CRYPTO_OP_SECURITY_SESSION;
> +
> +	return __rte_security_attach_session(op->sym, sess);
> +}
> +
> +struct rte_security_macsec_stats {
> +	uint64_t reserved;
> +};
> +
> +struct rte_security_ipsec_stats {
> +	uint64_t reserved;
> +
> +};
> +
> +struct rte_security_stats {
> +	enum rte_security_session_protocol protocol;
> +	/**< Security protocol to be configured */
> +
> +	union {
> +		struct rte_security_macsec_stats macsec;
> +		struct rte_security_ipsec_stats ipsec;
> +	};
> +};
> +
> +/**
> + * Query security session statistics
> + *
> + * @param	id	security instance identifier id
> + * @param	sess	security session
> + * @param	stats	statistics
> + * @return
> + *  - On success return 0
> + *  - On failure errno
> + */
> +int
> +rte_security_session_query(uint16_t id,
> +			   struct rte_security_session *sess,
> +			   struct rte_security_stats *stats);

IMO, Changing to something with "stats" makes more sense and it will be
inline with another subsystems as well.

> +
> +/**
> + * Security capability definition
> + */
> +struct rte_security_capability {
> +	enum rte_security_session_action_type action;
> +	/**< Security action type*/
> +	enum rte_security_session_protocol protocol;
> +	/**< Security protocol */
> +	RTE_STD_C11
> +	union {
> +		struct {
> +			enum rte_security_ipsec_sa_protocol proto;
> +			/**< IPsec SA protocol */
> +			enum rte_security_ipsec_sa_mode mode;
> +			/**< IPsec SA mode */
> +			enum rte_security_ipsec_sa_direction direction;
> +			/**< IPsec SA direction */
> +			struct rte_security_ipsec_sa_options options;
> +			/**< IPsec SA supported options */
> +		} ipsec;
> +		/**< IPsec capability */
> +		struct {
> +			/* To be Filled */
> +		} macsec;
> +		/**< MACsec capability */
> +	};
> +
> +	const struct rte_cryptodev_capabilities *crypto_capabilities;
> +	/**< Corresponding crypto capabilities for security capability  */
> +};
> +
> +/**
> + * Security capability index used to query a security instance for a specific
> + * security capability
> + */
> +struct rte_security_capability_idx {
> +	enum rte_security_session_action_type action;
> +	enum rte_security_session_protocol protocol;
> +
> +	union {
> +		struct {
> +			enum rte_security_ipsec_sa_protocol proto;
> +			enum rte_security_ipsec_sa_mode mode;
> +			enum rte_security_ipsec_sa_direction direction;
> +		} ipsec;

Why to duplicate elements in this structure. Can we have common structure
which can be used for rte_security_capability and
rte_security_capability_idx


> +	};
> +};
> +
> +/**
> + *  Returns array of security instance capabilities
> + *
> + * @param	id	Security instance identifier.
> + *
> + * @return
> + *   - Returns array of security capabilities.
> + *   - Return NULL if no capabilities available.
> + */
> +const struct rte_security_capability *
> +rte_security_capabilities_get(uint16_t id);
> +
> +/**
> + * Query if a specific capability is available on security instance
> + *
> + * @param	id	security instance identifier.
> + * @param	idx	security capability index to match against
> + *
> + * @return
> + *   - Returns pointer to security capability on match of capability
> + *     index criteria.
> + *   - Return NULL if the capability not matched on security instance.
> + */
> +const struct rte_security_capability *
> +rte_security_capability_get(uint16_t id,
> +			    struct rte_security_capability_idx *idx);

const struct rte_security_capability_idx *idx ?

> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_SECURITY_H_ */
> +/**
> + * Query stats from the PMD.
> + *
> + * @param	device		Crypto/eth device pointer
> + * @param	sess		Pointer to Security private session structure
> + * @param	stats		Security stats of the driver
> + *
> + * @return
> + *  - Returns 0 if private session structure have been updated successfully.
> + *  - Returns -EINVAL if session parameters are invalid.
> + */
> +typedef int (*security_session_query_t)(void *device,
> +		struct rte_security_session *sess,
> +		struct rte_security_stats *stats);
> +
> +/**
> + * Update buffer with provided metadata.

Update the mbuf ?

> + *
> + * @param	sess		Security session structure
> + * @param	mb		Packet buffer
> + * @param	mt		Metadata
> + *
> + * @return
> + *  - Returns 0 if metadata updated successfully.
> + *  - Returns -ve value for errors.
> + */
> +typedef int (*security_set_pkt_metadata_t)(void *device,
> +		struct rte_security_session *sess, struct rte_mbuf *m,
> +		void *params);
> +