[dpdk-dev] [PATCH 01/11] lib/rte_security: add security library
Radu Nicolau
radu.nicolau at intel.com
Fri Sep 22 13:55:52 CEST 2017
Hi,
I will address most of the issues in the v2, except the one related to
the multiprocess issue - we may need more discussions on that.
Thanks for reviewing,
Radu
On 9/18/2017 2:13 PM, Jerin Jacob wrote:
> -----Original Message-----
>> Date: Thu, 14 Sep 2017 13:56:41 +0530
>> From: Akhil Goyal <akhil.goyal at nxp.com>
>> To: dev at dpdk.org
>> CC: declan.doherty at intel.com, pablo.de.lara.guarch at intel.com,
>> hemant.agrawal at nxp.com, radu.nicolau at intel.com, borisp at mellanox.com,
>> aviadye at mellanox.com, thomas at monjalon.net, sandeep.malik at nxp.com,
>> jerin.jacob at 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 at nxp.com>
>> Signed-off-by: Boris Pismenny <borisp at mellanox.com>
>> Signed-off-by: Radu Nicolau <radu.nicolau at intel.com>
>> Signed-off-by: Declan Doherty <declan.doherty at 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);
>> +
More information about the dev
mailing list