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

[Akhil Goyal]

Hi Hemant,
On 9/15/2017 11:02 AM, Hemant Agrawal wrote:
> Hi,
> 
> On 9/14/2017 1:56 PM, Akhil Goyal wrote:
> <snip>..
> 
>> diff --git a/lib/librte_security/rte_security.c 
>> b/lib/librte_security/rte_security.c
>> new file mode 100644
>> index 0000000..5776246
>> --- /dev/null
>> +++ b/lib/librte_security/rte_security.c
>> @@ -0,0 +1,252 @@
>> +/*-
>> + *   BSD LICENSE
>> + *
>> + *   Copyright 2017 NXP.
>> + *   Copyright(c) 2017 Intel Corporation. All rights reserved.
>> + *
>> + *   Redistribution and use in source and binary forms, with or without
>> + *   modification, are permitted provided that the following conditions
>> + *   are met:
>> + *
>> + *     * Redistributions of source code must retain the above copyright
>> + *       notice, this list of conditions and the following disclaimer.
>> + *     * Redistributions in binary form must reproduce the above 
>> copyright
>> + *       notice, this list of conditions and the following disclaimer in
>> + *       the documentation and/or other materials provided with the
>> + *       distribution.
>> + *     * Neither the name of NXP nor the names of its
>> + *       contributors may be used to endorse or promote products derived
>> + *       from this software without specific prior written permission.
>> + *
>> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND 
>> FITNESS FOR
>> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE 
>> COPYRIGHT
>> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, 
>> INCIDENTAL,
>> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF 
>> USE,
>> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND 
>> ON ANY
>> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF 
>> THE USE
>> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH 
>> DAMAGE.
>> + */
>> +
>> +#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,
>> +        RTE_SECURITY_INSTANCE_VALID
>> +    } state;
>> +    void *device;
>> +    struct rte_security_ops *ops;
>> +};
>> +
>> +static struct rte_security_ctx *security_instances;
>> +static uint16_t max_nb_security_instances;
>> +static uint16_t nb_security_instances;
>> +
>> +static int
>> +rte_security_is_valid_id(uint16_t id)
>> +{
>> +    if (id >= nb_security_instances ||
>> +        (security_instances[id].state != RTE_SECURITY_INSTANCE_VALID))
>> +        return 0;
>> +    else
>> +        return 1;
>> +}
>> +
>> +/* Macros to check for valid id */
>> +#define RTE_SEC_VALID_ID_OR_ERR_RET(id, retval) do { \
>> +    if (!rte_security_is_valid_id(id)) { \
>> +        RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
>> +        return retval; \
>> +    } \
>> +} while (0)
>> +
>> +#define RTE_SEC_VALID_ID_OR_RET(id) do { \
>> +    if (!rte_security_is_valid_id(id)) { \
>> +        RTE_PMD_DEBUG_TRACE("Invalid sec_id=%d\n", id); \
>> +        return; \
>> +    } \
>> +} while (0)
>> +
>> +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);
> 
> I think "RTE_SECURITY_INSTANCES_BLOCK_ALLOC_SZ" value as 8 is relatively 
> small. you may want to keep it "64" or more.
> 
> you may change it into two parts
> - Initial block size and incremental block size for realloc.
> 
> Also, do you want to make it a configurable variable. as some 
> implementation may need really large number of SAs.
Security Instances are not per SA, these are per eth/crypto device which 
support security offload.
> 
>> +
>> +        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;
>> +
>> +    return 0;
>> +}
>> +
>> +int
>> +rte_security_unregister(__rte_unused uint16_t *id)
>> +{
>> +    /* To be implemented */
>> +    return 0;
>> +}
>> +
>> +struct rte_security_session *
>> +rte_security_session_create(uint16_t id,
>> +                struct rte_security_session_conf *conf,
>> +                struct rte_mempool *mp)
>> +{
>> +    struct rte_security_ctx *instance;
>> +    struct rte_security_session *sess = NULL;
>> +
>> +    RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
>> +    instance = &security_instances[id];
>> +
>> +    if (conf == NULL)
>> +        return NULL;
>> +
>> +    if (rte_mempool_get(mp, (void *)&sess))
>> +        return NULL;
>> +
>> +    RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
> 
> it will leak the sess memory, if returned on error.
ok I will fix this.

> 
>> +    if (instance->ops->session_create(instance->device, conf, sess, 
>> mp)) {
>> +        rte_mempool_put(mp, (void *)sess);
>> +        return NULL;
>> +    }
> 
> can the mempool operations be part of session_create api?
No, this is used for struct rte_security_session. session_create() would 
take another object for its private data which it would free in the 
session_destroy() in the driver.
> 
> it will be similar to destroy, which is expected to free the 'sess' 
> object to mempool?
rte_security_session_destroy should free the mempool object used for 
struct rte_security_session in the rte_security_session_create

I will fix this in the next version.
> 
>> +    return sess;
>> +}
>> +
> 
> <snip>..
> 
>> +struct rte_security_ipsec_xform {
>> +    uint32_t spi;
>> +    /**< SA security parameter index */
>> +    uint32_t salt;
>> +    /**< SA salt */
>> +    struct rte_security_ipsec_sa_options options;
>> +    /**< various SA options */
>> +    enum rte_security_ipsec_sa_direction direction;
>> +    /**< IPSec SA Direction - Egress/Ingress */
>> +    enum rte_security_ipsec_sa_protocol proto;
>> +    /**< IPsec SA Protocol - AH/ESP */
>> +    enum rte_security_ipsec_sa_mode mode;
>> +    /**< IPsec SA Mode - transport/tunnel */
>> +    struct rte_security_ipsec_tunnel_param tunnel;
>> +    /**< Tunnel parameters, NULL for transport mode */
>> +};
>> +
>> +/**
>> + * MACsec security session configuration
>> + */
>> +struct rte_security_macsec_xform {
>> +    /** To be Filled */
>> +};
>> +
>> +/**
>> + * Security session action type.
>> + */
>> +enum rte_security_session_action_type {
>> +    RTE_SECURITY_ACTION_TYPE_NONE,
>> +    /**< No security actions */
> 
> This is not being used, it seems that you are only using it as marker to 
> indicate end of capability set?
Yes.
> 
>> +    RTE_SECURITY_ACTION_TYPE_INLINE_CRYPTO,
>> +    /**< Crypto processing for security protocol is processed inline
>> +     * during transmission */
>> +    RTE_SECURITY_ACTION_TYPE_INLINE_PROTOCOL,
>> +    /**< All security protocol processing is performed inline during
>> +     * transmission */
>> +    RTE_SECURITY_ACTION_TYPE_LOOKASIDE_PROTOCOL
>> +    /**< All security protocol processing including crypto is performed
>> +     * on a lookaside accelerator */
>> +};
>> +
>> +/** Security session protocol definition */
>> +enum rte_security_session_protocol {
>> +    RTE_SECURITY_PROTOCOL_IPSEC,
>> +    /**< IPsec Protocol */
>> +    RTE_SECURITY_PROTOCOL_MACSEC,
>> +    /**< MACSec Protocol */
>> +};
>> +
>> +/**
>> + * Security session configuration
>> + */
>> +struct rte_security_session_conf {
>> +    enum rte_security_session_action_type action_type;
>> +    /**< Type of action to be performed on the session */
>> +    enum rte_security_session_protocol protocol;
>> +    /**< Security protocol to be configured */
>> +    union {
>> +        struct rte_security_ipsec_xform ipsec;
>> +        struct rte_security_macsec_xform macsec;
>> +    };
>> +    /**< Configuration parameters for security session */
>> +    struct rte_crypto_sym_xform *crypto_xform;
>> +    /**< Security Session Crypto Transformations */
>> +};
>> +
>> +struct rte_security_session {
>> +    __extension__ void *sess_private_data;
>> +    /**< Private session material */
>> +};
>> +
> 
> 
> Do you need specific error handling for security sessions as well?
> In case of full protocol offloads, you will need indications for
> 1. SEQ number overflow (egress side, if the SA is not refreshed on time)
> 2. Anti replay window config and err handlings?
> 
This is in our TODO list for future.
> 
>> +/**
>> + * Create security session as specified by the session configuration
>> + *
>> + * @param   id        security instance identifier id
>> + * @param   conf    session configuration parameters
> 
> fix the indentation here and other places in this file.
ok.
> 
>> + * @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,
>> +                struct rte_mempool *mp);
>> +
>> +/**
> 


Regards,
Akhil