From mboxrd@z Thu Jan 1 00:00:00 1970 From: Akhil Goyal Subject: Re: [PATCH 01/11] lib/rte_security: add security library Date: Wed, 20 Sep 2017 17:05:52 +0530 Message-ID: <1b00fffb-4375-321f-3e1f-4ac90ca8b6bb@nxp.com> References: <20170914082651.26232-1-akhil.goyal@nxp.com> <20170914082651.26232-2-akhil.goyal@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 8bit Cc: , , , , , , , To: Hemant Agrawal , Return-path: Received: from NAM03-BY2-obe.outbound.protection.outlook.com (mail-by2nam03on0085.outbound.protection.outlook.com [104.47.42.85]) by dpdk.org (Postfix) with ESMTP id 85248DE0 for ; Wed, 20 Sep 2017 13:36:00 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Hemant, On 9/15/2017 11:02 AM, Hemant Agrawal wrote: > Hi, > > On 9/14/2017 1:56 PM, Akhil Goyal wrote: > .. > >> 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 >> +#include >> + >> +#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; >> +} >> + > > .. > >> +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