Hi Lukasz, Please see inline.
Thanks, Anoob > -----Original Message----- > From: Lukasz Wojciechowski <l.wojciec...@partner.samsung.com> > Sent: Saturday, April 4, 2020 12:06 AM > To: Anoob Joseph <ano...@marvell.com>; dev@dpdk.org > Cc: Narayana Prasad Raju Athreya <pathr...@marvell.com>; Lukas Bartosik [C] > <lbarto...@marvell.com> > Subject: [EXT] Re: [dpdk-dev] [PATCH 01/13] librte_security: fix verification > of > parameters > > External Email > > ---------------------------------------------------------------------- > Hi Anoob, > > Thank you very much for your review. > Please see my answers inline. > > Best regards, > Lukasz > > > W dniu 17.03.2020 o 13:59, Anoob Joseph pisze: > > Hi Lukasz, > > > > Please see inline. > > > > Thanks, > > Anoob > > > >> -----Original Message----- > >> From: dev <dev-boun...@dpdk.org> On Behalf Of Lukasz Wojciechowski > >> Sent: Thursday, March 12, 2020 8:47 PM > >> To: dev@dpdk.org > >> Subject: [dpdk-dev] [PATCH 01/13] librte_security: fix verification > >> of parameters > > [Anoob] I believe the title has to be: "security: fix verification of > > parameters" > > > > Also, you can add "Fixes" as well. > > I changed the title and will push the new on in 2nd version of the paches > after I'll > fix all other issues. > > How do you add a "Fixes" tag to a patch? [Anoob] Check the below link. It explains the format of the patch with fixes. https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body > > > > >> This patch adds verification of the parameters to the ret_security API > functions. > >> All required parameters are checked if they are not NULL. > >> > >> Checks verify full chain of pointers, e.g. in case of verification of > >> "instance->ops- > >>> session_XXX", they check also "instance" and "instance->ops". > >> Signed-off-by: Lukasz Wojciechowski > >> <l.wojciec...@partner.samsung.com> > >> Change-Id: I1724c926a1a0a13fd16d76f19842a0b40fbea1b2 > >> --- > >> lib/librte_security/rte_security.c | 58 +++++++++++++++++++++++------- > >> 1 file changed, 45 insertions(+), 13 deletions(-) > >> > >> diff --git a/lib/librte_security/rte_security.c > >> b/lib/librte_security/rte_security.c > >> index bc81ce15d..40a0e9ce5 100644 > >> --- a/lib/librte_security/rte_security.c > >> +++ b/lib/librte_security/rte_security.c > >> @@ -1,6 +1,7 @@ > >> /* SPDX-License-Identifier: BSD-3-Clause > >> * Copyright 2017 NXP. > >> * Copyright(c) 2017 Intel Corporation. > >> + * Copyright (c) 2020 Samsung Electronics Co., Ltd All Rights > >> + Reserved > >> */ > >> > >> #include <rte_malloc.h> > >> @@ -9,6 +10,12 @@ > >> #include "rte_security.h" > >> #include "rte_security_driver.h" > >> > >> +/* Macro to check for invalid pointers */ > >> +#define RTE_PTR_OR_ERR_RET(ptr, retval) do { \ > >> + if ((ptr) == NULL) \ > >> + return retval; \ > >> +} while (0) > >> + > >> struct rte_security_session * > >> rte_security_session_create(struct rte_security_ctx *instance, > >> struct rte_security_session_conf *conf, @@ -16,10 > >> +23,11 @@ rte_security_session_create(struct rte_security_ctx > >> +*instance, { > >> struct rte_security_session *sess = NULL; > >> > >> - if (conf == NULL) > >> - return NULL; > >> - > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL); > >> + RTE_PTR_OR_ERR_RET(instance, NULL); > >> + RTE_PTR_OR_ERR_RET(instance->ops, NULL); > >> + RTE_PTR_OR_ERR_RET(instance->ops->session_create, NULL); > > [Anoob] The above three lines are repeated for every op NULL check. Can we > introduce one macro for doing all the three checks? In case if it doesn't come > off well, we can stick to individual checks. > > > Done. Will appear in 2nd version of patches. > >> + RTE_PTR_OR_ERR_RET(conf, NULL); > >> + RTE_PTR_OR_ERR_RET(mp, NULL); > >> > >> if (rte_mempool_get(mp, (void **)&sess)) > >> return NULL; > >> @@ -38,14 +46,20 @@ rte_security_session_update(struct > >> rte_security_ctx *instance, > >> struct rte_security_session *sess, > >> struct rte_security_session_conf *conf) { > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_update, - > >> ENOTSUP); > >> + RTE_PTR_OR_ERR_RET(instance, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(instance->ops->session_update, -ENOTSUP); > >> + RTE_PTR_OR_ERR_RET(sess, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(conf, -EINVAL); > >> return instance->ops->session_update(instance->device, sess, > >> conf); } > >> > >> unsigned int > >> rte_security_session_get_size(struct rte_security_ctx *instance) { > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_get_size, 0); > >> + RTE_PTR_OR_ERR_RET(instance, 0); > >> + RTE_PTR_OR_ERR_RET(instance->ops, 0); > >> + RTE_PTR_OR_ERR_RET(instance->ops->session_get_size, 0); > >> return instance->ops->session_get_size(instance->device); > >> } > >> > >> @@ -54,7 +68,11 @@ rte_security_session_stats_get(struct > >> rte_security_ctx *instance, > >> struct rte_security_session *sess, > >> struct rte_security_stats *stats) { > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_stats_get, - > >> ENOTSUP); > >> + RTE_PTR_OR_ERR_RET(instance, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(instance->ops->session_stats_get, -ENOTSUP); > >> + // Parameter sess can be NULL in case of getting global statistics. > > [Anoob] Checkpatch error. > > ERROR:C99_COMMENTS: do not use C99 // comments > Done. Will appear in 2nd version of patches. > > > >> + RTE_PTR_OR_ERR_RET(stats, -EINVAL); > >> return instance->ops->session_stats_get(instance->device, sess, > >> stats); } > >> > >> @@ -64,7 +82,10 @@ rte_security_session_destroy(struct > >> rte_security_ctx *instance, { > >> int ret; > >> > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, - > >> ENOTSUP); > >> + RTE_PTR_OR_ERR_RET(instance, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(instance->ops->session_destroy, -ENOTSUP); > >> + RTE_PTR_OR_ERR_RET(sess, -EINVAL); > >> > >> if (instance->sess_cnt) > >> instance->sess_cnt--; > >> @@ -81,7 +102,11 @@ rte_security_set_pkt_metadata(struct > >> rte_security_ctx *instance, > >> struct rte_security_session *sess, > >> struct rte_mbuf *m, void *params) { > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, - > >> ENOTSUP); > >> + RTE_PTR_OR_ERR_RET(instance, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(instance->ops->set_pkt_metadata, -ENOTSUP); > > [Anoob] set_pkt_metadata() and get_userdata() are datapath ops. So can you > introduce a config option to enable/disable the checks. > > > > Please check, > > https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye. > > com_url-3Fk-3Dc52d8c32-2D98e14097-2Dc52c077d-2D0cc47a30d446- > 2Dc1b9d873 > > e3e59cc4-26u-3Dhttp-3A__code.dpdk.org_dpdk_latest_source_lib_librte-5F > > ethdev_rte-5Fethdev.h- > 23L4372&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB > > 8rwwviRSxyLWs2n6B- > WYLn1v9SyTMrT5EQqh2TU&m=aTo18FDvqHQBghOAhbi7x0f6EuX7 > > > wZHTUtsRRloZ9Bw&s=TXpv6uQZW1WwB_Av3vCaHeUaibQzA0ypUUqnPy5aQlE > &e= > Could you explain a bit further? > > Do you propose to make checks inside #ifdef RTE_LIBRTE_SECURITY_DEBUG or > so? [Anoob] Yes. You will need to introduce a new config flag (RTE_LIBRTE_SECURITY_DEBUG) and based on that, the error checks can be enabled/disabled. > And do you have all checks or just sess and m on mind? [Anoob] I think we should have all checks under the config option. > > The instance->ops->function checks were already there without any config > options in all API functions. [Anoob] Must have slipped through. Thanks for pointing it out. > > > > >> + RTE_PTR_OR_ERR_RET(sess, -EINVAL); > >> + RTE_PTR_OR_ERR_RET(m, -EINVAL); > >> return instance->ops->set_pkt_metadata(instance->device, > >> sess, m, params); > >> } > >> @@ -91,7 +116,9 @@ rte_security_get_userdata(struct rte_security_ctx > >> *instance, uint64_t md) { > >> void *userdata = NULL; > >> > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL); > >> + RTE_PTR_OR_ERR_RET(instance, NULL); > >> + RTE_PTR_OR_ERR_RET(instance->ops, NULL); > >> + RTE_PTR_OR_ERR_RET(instance->ops->get_userdata, NULL); > >> if (instance->ops->get_userdata(instance->device, md, &userdata)) > >> return NULL; > >> > >> @@ -101,7 +128,9 @@ rte_security_get_userdata(struct rte_security_ctx > >> *instance, uint64_t md) const struct rte_security_capability * > >> rte_security_capabilities_get(struct rte_security_ctx *instance) { > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL); > >> + RTE_PTR_OR_ERR_RET(instance, NULL); > >> + RTE_PTR_OR_ERR_RET(instance->ops, NULL); > >> + RTE_PTR_OR_ERR_RET(instance->ops->capabilities_get, NULL); > >> return instance->ops->capabilities_get(instance->device); > >> } > >> > >> @@ -113,7 +142,10 @@ rte_security_capability_get(struct > >> rte_security_ctx *instance, > >> const struct rte_security_capability *capability; > >> uint16_t i = 0; > >> > >> - RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL); > >> + RTE_PTR_OR_ERR_RET(instance, NULL); > >> + RTE_PTR_OR_ERR_RET(instance->ops, NULL); > >> + RTE_PTR_OR_ERR_RET(instance->ops->capabilities_get, NULL); > >> + RTE_PTR_OR_ERR_RET(idx, NULL); > >> capabilities = instance->ops->capabilities_get(instance->device); > >> > >> if (capabilities == NULL) > >> @@ -121,7 +153,7 @@ rte_security_capability_get(struct > >> rte_security_ctx *instance, > >> > >> while ((capability = &capabilities[i++])->action > >> != RTE_SECURITY_ACTION_TYPE_NONE) { > >> - if (capability->action == idx->action && > >> + if (capability->action == idx->action && > >> capability->protocol == idx->protocol) { > >> if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) > { > >> if (capability->ipsec.proto == > >> -- > >> 2.17.1 > > -- > > Lukasz Wojciechowski > Principal Software Engineer > > Samsung R&D Institute Poland > Samsung Electronics > Office +48 22 377 88 25 > l.wojciec...@partner.samsung.com
