Hi, I pushed the 2nd version of patches. I hope I understood all your comments well.
Many thanks, Lukasz W dniu 07.04.2020 o 08:20, Anoob Joseph pisze: > Hi Lukasz, > > Please see inline. > > Thanks, > Anoob > >> -----Original Message----- >> From: Lukasz Wojciechowski <l.wojciec...@partner.samsung.com> >> Sent: Tuesday, April 7, 2020 12:19 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: Re: [EXT] Re: [dpdk-dev] [PATCH 01/13] librte_security: fix >> verification >> of parameters >> >> Dear Anoob, >> >> Thank you for your reply and hints. > [Anoob] No mention! > >> Now I have patches ready to send as version 2, but I hesitate, because I >> don't >> like the idea of placing all checks in #ifdefs for RTE_LIBRTE_SECURITY_DEBUG >> config option. >> >> Let me explain here why: The config flag as a debug one will be disabled by >> default, which means, that normally nobody will use the checks. >> I believe that it is much better to have checks enabled as most of them will >> save >> the user of librte_security from segmentation faults, when trying to run >> instance->ops functions that are not supported or use invalid mempool >> object. I >> believe it will cause much less trouble to verify the error codes than to >> fight the >> segfault. > [Anoob] No disagreement. In fact this is exactly what is done with control > path APIs. But for datapath APIs, the penalty of such checks is not trivial. > I believe that's the argument for having the DEBUG config option in ethdev > data path ops. Also, for cases like instance->ops being NULL, the user would > get a seg fault with the first call itself and he can enable the config > option to debug the segfault he is observing. > > As for invalid mempool object, I do agree that is one case where we are > better off with per packet checks, but still perf impact would be there. In > ethdev, the library doesn't do these checks and it would be upto the driver > to have these checks if required. Same is the case with > rte_cryptodev_enqueue_burst(). > >> It is also mentioned in the API description in few places that specific >> codes are >> returned in case some operation is not supported. Can we make such a changes >> in API, changing the current behavior from an error return code to >> segmentation >> fault during execution? > [Anoob] Did you mean, if we have to allow seg fault as a valid error case, > better document it and remove the error codes getting returned? Again, I'm > not sure whether we can document that seg fault is the error case. Atleast, > in ethdev improper conditions can lead to seg fault but is not documented. > May be, maintainers should comment on it. > >> That's why I would like to keep all of the checks enabled and not placed >> inside >> config option. >> >> However it would be nice to add the RTE_LIBRTE_SECURITY_DEBUG flag that >> you mentioned for changing checks behavior, to additionally provide logs of >> checks. This way a devloper using libret_security won't get a segmentation >> faults but error codes. If [s]he wants to check the details he'll rebuild >> the library >> with debug config option enabled and will be able to see all the details in >> logs, >> so [s]he will be able to fix the code. >> >> What do you think about such usage of the config debug flag? > [Anoob] I totally agree to your suggestions on preventing seg faults. But my > only concern is the additional check and the corresponding perf penalty. > > May be let's look at the APIs and discuss what need to be handled how, > > int > rte_security_set_pkt_metadata(struct rte_security_ctx *instance, > struct rte_security_session *sess, > struct rte_mbuf *mb, void *params); > > instance -> NULL checks under config option > sess -> NULL checks under config option > mb -> possibly a datapath check. But can leave it to driver as well. > (rte_cryptodev_enqueue_burst() also doesn't do these checks). > Params -> can be NULL (so no check required). > > __rte_experimental > void * > rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md); > > instance -> NULL checks under config option > md -> can be NULL (device specific values). > > Does this make sense to you? > >> Best regards >> >> Lukasz >> >> >> W dniu 05.04.2020 o 14:54, Anoob Joseph pisze: >> >>> 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://protect2.fireeye.com/url?k=457094cb-18a428a3-45711f84-0cc47a3356b2-a0d1cb1b92aa19e8&u=https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye. >>> com_url-3Fk-3D13e80549-2D4e769ea3-2D13e98e06-2D0cc47a6cba04- >> 2D78f48282 >>> e990b416-26q-3D1-26u-3Dhttps-253A-252F-252Fdoc.dpdk.org-252Fguides- >> 252 >>> Fcontributing-252Fpatches.html-2523commit-2Dmessages- >> 2Dbody&d=DwIDaQ&c >>> =nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B- >> WYLn1v9SyTMrT5EQqh2TU& >>> m=fTGxo5Mh9jPA0-xv8XSAtZpHD9TZebCJWW9PGcElYmA&s=g2HlI0z81E0M5- >> txF2U5Ag >>> xEg0l3MHs4JT0O8AiHvm8&e= >>> >>>>>> 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-3Deee8020a-2Db37699e0-2Deee98945- >> 2D0cc47a6cba04-2D561954f3424eceea-26q-3D1-26u-3Dhttps-253A-252F- >> 252Furldefense.proofpoint.com-252Fv2-252Furl-253Fu-253Dhttps-2D3A-5F- >> 5Fprotect2.fireeye&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRS >> xyLWs2n6B-WYLn1v9SyTMrT5EQqh2TU&m=fTGxo5Mh9jPA0- >> xv8XSAtZpHD9TZebCJWW9PGcElYmA&s=vENm9fIBE5DENCAaeYTeYFwK__g06Jv >> K-lyOiJh-VL0&e= . >>>>> 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 >> -- >> >> Lukasz Wojciechowski >> Principal Software Engineer >> >> Samsung R&D Institute Poland >> Samsung Electronics >> Office +48 22 377 88 25 >> l.wojciec...@partner.samsung.com -- Lukasz Wojciechowski Principal Software Engineer Samsung R&D Institute Poland Samsung Electronics Office +48 22 377 88 25 l.wojciec...@partner.samsung.com
