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

Ananyev, Konstantin konstantin.ananyev at intel.com
Mon Oct 9 15:42:10 CEST 2017


Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Friday, October 6, 2017 7:11 PM
> To: Ananyev, Konstantin <konstantin.ananyev at intel.com>; dev at dpdk.org
> Cc: Doherty, Declan <declan.doherty at intel.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; hemant.agrawal at nxp.com;
> Nicolau, Radu <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; Mcnamara, John <john.mcnamara at intel.com>; olivier.matz at 6wind.com
> Subject: Re: [PATCH v2 01/12] lib/rte_security: add security library
> 
> Hi Konstantin,
> 
> Thanks for your comments.
> On 10/5/2017 10:00 PM, Ananyev, Konstantin wrote:
> > Hi lads,
> >
> >>
> >> rte_security library provides APIs for security session
> >> create/free for protocol offload or offloaded crypto
> >> operation to ethernet device.
> >>
> >> 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>
> >> ---
> >>   lib/librte_security/Makefile                 |  53 +++
> >>   lib/librte_security/rte_security.c           | 275 +++++++++++++++
> >>   lib/librte_security/rte_security.h           | 495 +++++++++++++++++++++++++++
> >>   lib/librte_security/rte_security_driver.h    | 182 ++++++++++
> >>   lib/librte_security/rte_security_version.map |  13 +
> >>   5 files changed, 1018 insertions(+)
> >>   create mode 100644 lib/librte_security/Makefile
> >>   create mode 100644 lib/librte_security/rte_security.c
> >>   create mode 100644 lib/librte_security/rte_security.h
> >>   create mode 100644 lib/librte_security/rte_security_driver.h
> >>   create mode 100644 lib/librte_security/rte_security_version.map
> >>
> >> diff --git a/lib/librte_security/Makefile b/lib/librte_security/Makefile
> >> new file mode 100644
> >> index 0000000..af87bb2
> >> --- /dev/null
> >> +++ b/lib/librte_security/Makefile
> >> @@ -0,0 +1,53 @@
> >> +#   BSD LICENSE
> >> +#
> >> +#   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 Intel Corporation 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_SDK)/mk/rte.vars.mk
> >> +
> >> +# library name
> >> +LIB = librte_security.a
> >> +
> >> +# library version
> >> +LIBABIVER := 1
> >> +
> >> +# build flags
> >> +CFLAGS += -O3
> >> +CFLAGS += $(WERROR_FLAGS)
> >> +
> >> +# library source files
> >> +SRCS-y += rte_security.c
> >> +
> >> +# export include files
> >> +SYMLINK-y-include += rte_security.h
> >> +SYMLINK-y-include += rte_security_driver.h
> >> +
> >> +# versioning export map
> >> +EXPORT_MAP := rte_security_version.map
> >> +
> >> +include $(RTE_SDK)/mk/rte.lib.mk
> >> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> >> new file mode 100644
> >> index 0000000..97d3857
> >> --- /dev/null
> >> +++ b/lib/librte_security/rte_security.c
> >> @@ -0,0 +1,275 @@
> >> +/*-
> >> + *   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,
> >> +		RTE_SECURITY_INSTANCE_VALID
> >> +	} state;
> >> +	void *device;
> >> +	struct rte_security_ops *ops;
> >> +	uint16_t sess_cnt;
> >> +};
> >> +
> >> +static struct rte_security_ctx *security_instances;
> >> +static uint16_t max_nb_security_instances;
> >> +static uint16_t nb_security_instances;
> >
> > Probably a dumb question - but why do you need a global security_instances []
> > and why security_instance has to be refrenced by index?
> > As I understand, with proposed model all drivers have to do something like:
> > rte_security_register(&eth_dev->data->sec_id,  (void *)eth_dev, &ixgbe_security_ops);
> > and then all apps would have to:
> > rte_eth_dev_get_sec_id(portid)
> > to retrieve that security_instance index.
> > Why not just treat struct rte_security_ctx* as opaque pointer and make
> > all related API get/accept it as a paratemer.
> > To retrieve sec_ctx from device just:
> > struct rte_security_ctx* rte_ethdev_get_sec_ctx(portid);
> > struct rte_security_ctx* rte_cryptodev_get_sec_ctx(portid);
> > ?
> We would look into this separately.

Could you clarify what does it mean?
It will be addressed in v4 or don't plan to address it at all or something else?
  

> >
> > Another question how currently proposed model with global static array and friends,
> > supposed to work for DPDK MP model?
> > Or MP support is not planned?
> multi process case is planned for future enhancement. This is mentioned
> in the cover note.

Great, then I suppose you should have a generic idea how that model will work
for DPDK multi-process model?
If so, can you probably share your thoughts, because it is not clear to me.
Let say user has an ethdev device with ipsec capability and  wants to use it
from both primary and secondary process.
What would be a procedure?
Can user use the same security instance from both processes?
If yes, then  
- how secondary process will get security_instance_id for that device
from primary process?
 By calling rte_eth_dev_get_sec_id(), or something else? 
- how guarantee that all these func pointers inside ops will be mapped to the
same address inside  processes?  
If not, then does it mean each process has to call rte_security_register()
for each device?
But right now you have only one sec_id  inside rte_eth_dev_data.

Would secondary process be allowed to register/unregister/update security instances?
 if yes, how the will synchronize?
Same question for session ops.

> >
> >> +
> >> +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)
> >> +{

Probably const  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) {
> >
> > You probably need try to reuse unregistered entries first?
> > Konstantin
> >
> These APIs are experimental at this moment as mentioned in the patchset.
> We will try accommodate your comments in future.

Again could you clarify what do you mean by 'future' here?

> >
> >> +		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;

BTW, I think you need to copy actual contents of ops.
Otherwise you assuming that ops are static
(would be valid though all processes lifetimes). 

Konstantin

> >> +	security_instances[*id].sess_cnt = 0;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +int
> >> +rte_security_unregister(uint16_t id)
> >> +{
> >> +	struct rte_security_ctx *instance;
> >> +
> >> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
> >> +	instance = &security_instances[id];
> >> +
> >> +	if (instance->sess_cnt)
> >> +		return -EBUSY;
> >> +
> >> +	memset(instance, 0, sizeof(*instance));
> >> +	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;
> >> +
> >> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
> >> +
> >> +	if (rte_mempool_get(mp, (void *)&sess))
> >> +		return NULL;
> >> +
> >> +	if (instance->ops->session_create(instance->device, conf, sess, mp)) {
> >> +		rte_mempool_put(mp, (void *)sess);
> >> +		return NULL;
> >> +	}
> >> +	instance->sess_cnt++;
> >> +
> >> +	return sess;
> >> +}
> >> +
> >> +int
> >> +rte_security_session_update(uint16_t id,
> >> +			    struct rte_security_session *sess,
> >> +			    struct rte_security_session_conf *conf)
> >> +{
> >> +	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->session_update, -ENOTSUP);
> >> +	return instance->ops->session_update(instance->device, sess, conf);
> >> +}
> >> +
> >> +int
> >> +rte_security_session_stats_get(uint16_t id,
> >> +			       struct rte_security_session *sess,
> >> +			       struct rte_security_stats *stats)
> >> +{
> >> +	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->session_stats_get, -ENOTSUP);
> >> +	return instance->ops->session_stats_get(instance->device, sess, stats);
> >> +}
> >> +
> >> +int
> >> +rte_security_session_destroy(uint16_t id, struct rte_security_session *sess)
> >> +{
> >> +	int ret;
> >> +	struct rte_security_ctx *instance;
> >> +	struct rte_mempool *mp = rte_mempool_from_obj(sess);
> >> +
> >> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, -ENODEV);
> >> +	instance = &security_instances[id];
> >> +
> >> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, -ENOTSUP);
> >> +
> >> +	if (instance->sess_cnt)
> >> +		instance->sess_cnt--;
> >> +
> >> +	ret = instance->ops->session_destroy(instance->device, sess);
> >> +	if (!ret)
> >> +		rte_mempool_put(mp, (void *)sess);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +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);
> >> +	return instance->ops->set_pkt_metadata(instance->device,
> >> +					       sess, m, params);
> >> +}
> >> +
> >> +const struct rte_security_capability *
> >> +rte_security_capabilities_get(uint16_t id)
> >> +{
> >> +	struct rte_security_ctx *instance;
> >> +
> >> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
> >> +	instance = &security_instances[id];
> >> +
> >> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
> >> +	return instance->ops->capabilities_get(instance->device);
> >> +}
> >> +
> >> +const struct rte_security_capability *
> >> +rte_security_capability_get(uint16_t id,
> >> +			    struct rte_security_capability_idx *idx)
> >> +{
> >> +	struct rte_security_ctx *instance;
> >> +	const struct rte_security_capability *capabilities;
> >> +	const struct rte_security_capability *capability;
> >> +	uint16_t i = 0;
> >> +
> >> +	RTE_SEC_VALID_ID_OR_ERR_RET(id, NULL);
> >> +	instance = &security_instances[id];
> >> +
> >> +	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
> >> +	capabilities = instance->ops->capabilities_get(instance->device);
> >> +
> >> +	if (capabilities == NULL)
> >> +		return NULL;
> >> +
> >> +	while ((capability = &capabilities[i++])->action
> >> +			!= RTE_SECURITY_ACTION_TYPE_NONE) {
> >> +		if (capability->action  == idx->action &&
> >> +				capability->protocol == idx->protocol) {
> >> +			if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) {
> >> +				if (capability->ipsec.proto ==
> >> +						idx->ipsec.proto &&
> >> +					capability->ipsec.mode ==
> >> +							idx->ipsec.mode &&
> >> +					capability->ipsec.direction ==
> >> +							idx->ipsec.direction)
> >> +					return capability;
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return NULL;
> >> +}
> >
> 
> Regards,
> Akhil


More information about the dev mailing list