security: fix crash at accessing non-implemented ops
Checks
Commit Message
Valid checks for optional function pointers inside dev-ops
were disabled by undefined macro.
Fixes: b6ee98547847 ("security: fix verification of parameters")
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
lib/librte_security/rte_security.c | 4 ----
1 file changed, 4 deletions(-)
Comments
Actually looking at app/test/test_security.c
I also see a few '#ifdef RTE_DEBUG's.
Let say:
+static int
+test_get_userdata_inv_context(void)
+{
+#ifdef RTE_DEBUG
+ uint64_t md = 0xDEADBEEF;
+
+ void *ret = rte_security_get_userdata(NULL, md);
+ TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_get_userdata,
+ ret, NULL, "%p");
+ TEST_ASSERT_MOCK_CALLS(mock_get_userdata_exp, 0);
+
+ return TEST_SUCCESS;
+#else
+ return TEST_SKIPPED;
+#endif
+}
What is the point?
Why not always run the test unconditionally?
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, April 23, 2020 12:52 AM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: [PATCH] security: fix crash at accessing non-implemented ops
>
> Valid checks for optional function pointers inside dev-ops
> were disabled by undefined macro.
>
> Fixes: b6ee98547847 ("security: fix verification of parameters")
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> lib/librte_security/rte_security.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> index d475b0977..b65430ce2 100644
> --- a/lib/librte_security/rte_security.c
> +++ b/lib/librte_security/rte_security.c
> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> struct rte_security_session *sess,
> struct rte_mbuf *m, void *params)
> {
> -#ifdef RTE_DEBUG
> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -EINVAL,
> -ENOTSUP);
> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> -#endif
> return instance->ops->set_pkt_metadata(instance->device,
> sess, m, params);
> }
> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
> {
> void *userdata = NULL;
>
> -#ifdef RTE_DEBUG
> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, NULL);
> -#endif
> if (instance->ops->get_userdata(instance->device, md, &userdata))
> return NULL;
>
> --
> 2.17.1
Hi Konstantin,
These are data path ops and so it will be better if we can avoid such checks in the datapath. The same is done in ethdev also.
http://code.dpdk.org/dpdk/v20.02/source/lib/librte_ethdev/rte_ethdev.h#L4372
Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such checks.
http://code.dpdk.org/dpdk/v20.02/source/lib/librte_cryptodev/rte_cryptodev.h#L962
Thanks,
Anoob
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> Sent: Thursday, April 23, 2020 5:22 AM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin Ananyev
> <konstantin.ananyev@intel.com>
> Subject: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented
> ops
>
> Valid checks for optional function pointers inside dev-ops were disabled by
> undefined macro.
>
> Fixes: b6ee98547847 ("security: fix verification of parameters")
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> lib/librte_security/rte_security.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> index d475b0977..b65430ce2 100644
> --- a/lib/librte_security/rte_security.c
> +++ b/lib/librte_security/rte_security.c
> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
> *instance,
> struct rte_security_session *sess,
> struct rte_mbuf *m, void *params) { -#ifdef
> RTE_DEBUG
> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> EINVAL,
> -ENOTSUP);
> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> -#endif
> return instance->ops->set_pkt_metadata(instance->device,
> sess, m, params);
> }
> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx
> *instance, uint64_t md) {
> void *userdata = NULL;
>
> -#ifdef RTE_DEBUG
> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> NULL); -#endif
> if (instance->ops->get_userdata(instance->device, md, &userdata))
> return NULL;
>
> --
> 2.17.1
>
> Hi Konstantin,
>
> These are data path ops and so it will be better if we can avoid such checks in the datapath. The same is done in ethdev also.
AFAIK, get_userdata is an *optional* dev-ops function that can be used by data-path.
So far there was no strict requirement for the rte_security PMDs to *always* implement it.
So what you guys did is a silent change of public API behaviour.
As result ixgbe, (and probably some others rte_security PMDs) stopped working properly.
I don't see any point in these changes, but if you'd like to do that, at
least our usual procedure has to be followed:
1. Send and RFC to get an agreement with rte_security PMDs maintainers (one release ahead)
2. send a deprecation note (one release ahead)
3. change the behaviour of the public API
4. update release notes
AFAIK 1), 2), 4) wasn't done.
So I think right now we need to revert original behaviour.
>
> http://code.dpdk.org/dpdk/v20.02/source/lib/librte_ethdev/rte_ethdev.h#L4372
>
> Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such checks.
> http://code.dpdk.org/dpdk/v20.02/source/lib/librte_cryptodev/rte_cryptodev.h#L962
That's a different story:
rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions that
have to be implemented by each ethdev/cryptodev API.
>
>
> Thanks,
> Anoob
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> > Sent: Thursday, April 23, 2020 5:22 AM
> > To: dev@dpdk.org
> > Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin Ananyev
> > <konstantin.ananyev@intel.com>
> > Subject: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented
> > ops
> >
> > Valid checks for optional function pointers inside dev-ops were disabled by
> > undefined macro.
> >
> > Fixes: b6ee98547847 ("security: fix verification of parameters")
> >
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> > lib/librte_security/rte_security.c | 4 ----
> > 1 file changed, 4 deletions(-)
> >
> > diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> > index d475b0977..b65430ce2 100644
> > --- a/lib/librte_security/rte_security.c
> > +++ b/lib/librte_security/rte_security.c
> > @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
> > *instance,
> > struct rte_security_session *sess,
> > struct rte_mbuf *m, void *params) { -#ifdef
> > RTE_DEBUG
> > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> > EINVAL,
> > -ENOTSUP);
> > RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> > -#endif
> > return instance->ops->set_pkt_metadata(instance->device,
> > sess, m, params);
> > }
> > @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx
> > *instance, uint64_t md) {
> > void *userdata = NULL;
> >
> > -#ifdef RTE_DEBUG
> > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> > NULL); -#endif
> > if (instance->ops->get_userdata(instance->device, md, &userdata))
> > return NULL;
> >
> > --
> > 2.17.1
W dniu 23.04.2020 o 02:11, Ananyev, Konstantin pisze:
> Actually looking at app/test/test_security.c
> I also see a few '#ifdef RTE_DEBUG's.
> Let say:
>
> +static int
> +test_get_userdata_inv_context(void)
> +{
> +#ifdef RTE_DEBUG
> + uint64_t md = 0xDEADBEEF;
> +
> + void *ret = rte_security_get_userdata(NULL, md);
> + TEST_ASSERT_MOCK_FUNCTION_CALL_RET(rte_security_get_userdata,
> + ret, NULL, "%p");
> + TEST_ASSERT_MOCK_CALLS(mock_get_userdata_exp, 0);
> +
> + return TEST_SUCCESS;
> +#else
> + return TEST_SKIPPED;
> +#endif
> +}
>
> What is the point?
> Why not always run the test unconditionally?
If there is no RTE_DEBUG defined, the tested functionality is not
compiled, so the tests won't work.
They must be wrapped with same macro as library code.
>
>
>> -----Original Message-----
>> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Sent: Thursday, April 23, 2020 12:52 AM
>> To: dev@dpdk.org
>> Cc: akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Subject: [PATCH] security: fix crash at accessing non-implemented ops
>>
>> Valid checks for optional function pointers inside dev-ops
>> were disabled by undefined macro.
>>
>> Fixes: b6ee98547847 ("security: fix verification of parameters")
>>
>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> ---
>> lib/librte_security/rte_security.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
>> index d475b0977..b65430ce2 100644
>> --- a/lib/librte_security/rte_security.c
>> +++ b/lib/librte_security/rte_security.c
>> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
>> struct rte_security_session *sess,
>> struct rte_mbuf *m, void *params)
>> {
>> -#ifdef RTE_DEBUG
>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -EINVAL,
>> -ENOTSUP);
>> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
>> -#endif
>> return instance->ops->set_pkt_metadata(instance->device,
>> sess, m, params);
>> }
>> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
>> {
>> void *userdata = NULL;
>>
>> -#ifdef RTE_DEBUG
>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, NULL);
>> -#endif
>> if (instance->ops->get_userdata(instance->device, md, &userdata))
>> return NULL;
>>
>> --
>> 2.17.1
W dniu 23.04.2020 o 06:07, Anoob Joseph pisze:
> Hi Konstantin,
>
> These are data path ops and so it will be better if we can avoid such checks in the datapath. The same is done in ethdev also.
>
> https://protect2.fireeye.com/url?k=d44931cf-89d2cdac-d448ba80-0cc47a31cdbc-8281a62b4c91d848&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_ethdev%2Frte_ethdev.h%23L4372
>
> Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such checks.
> https://protect2.fireeye.com/url?k=51324200-0ca9be63-5133c94f-0cc47a31cdbc-11f88758fc12c996&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_cryptodev%2Frte_cryptodev.h%23L962
>
>
> Thanks,
> Anoob
Hi Konstantine,
It's my fault. Sorry.
These checks need to be disabled in non-debug code, so they should be
wrapped in a macro. It's just not the valid macro.
The discussion about rte_debug mode is ongoing
(https://patchwork.dpdk.org/patch/68815/)
and currently the v2 version of patches is prepared to gather
maintainers opinion.
After the rte_debug is introduced the proper macro to use will be
RTE_DEBUG_SECURITY.
Until then, the RTE_DEBUG macro can stay as like Anoob mentioned the
checks will have impact on dataplane performance.
If you want to enable this code, please use CFLAGS="-DRTE_DEBUG"
>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
>> Sent: Thursday, April 23, 2020 5:22 AM
>> To: dev@dpdk.org
>> Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin Ananyev
>> <konstantin.ananyev@intel.com>
>> Subject: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented
>> ops
>>
>> Valid checks for optional function pointers inside dev-ops were disabled by
>> undefined macro.
>>
>> Fixes: b6ee98547847 ("security: fix verification of parameters")
>>
>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> ---
>> lib/librte_security/rte_security.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
>> index d475b0977..b65430ce2 100644
>> --- a/lib/librte_security/rte_security.c
>> +++ b/lib/librte_security/rte_security.c
>> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
>> *instance,
>> struct rte_security_session *sess,
>> struct rte_mbuf *m, void *params) { -#ifdef
>> RTE_DEBUG
>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
>> EINVAL,
>> -ENOTSUP);
>> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
>> -#endif
>> return instance->ops->set_pkt_metadata(instance->device,
>> sess, m, params);
>> }
>> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx
>> *instance, uint64_t md) {
>> void *userdata = NULL;
>>
>> -#ifdef RTE_DEBUG
>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
>> NULL); -#endif
>> if (instance->ops->get_userdata(instance->device, md, &userdata))
>> return NULL;
>>
>> --
>> 2.17.1
>
W dniu 23.04.2020 o 09:54, Ananyev, Konstantin pisze:
>> Hi Konstantin,
>>
>> These are data path ops and so it will be better if we can avoid such checks in the datapath. The same is done in ethdev also.
> AFAIK, get_userdata is an *optional* dev-ops function that can be used by data-path.
> So far there was no strict requirement for the rte_security PMDs to *always* implement it.
> So what you guys did is a silent change of public API behaviour.
> As result ixgbe, (and probably some others rte_security PMDs) stopped working properly.
> I don't see any point in these changes, but if you'd like to do that, at
> least our usual procedure has to be followed:
> 1. Send and RFC to get an agreement with rte_security PMDs maintainers (one release ahead)
> 2. send a deprecation note (one release ahead)
> 3. change the behaviour of the public API
> 4. update release notes
>
> AFAIK 1), 2), 4) wasn't done.
> So I think right now we need to revert original behaviour.
The current changes were made in patch: b6ee9854784 security: fix
verification of parameters
<snip>
@@ -91,7 +119,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);
+#ifdef RTE_DEBUG
+ RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, NULL);
+#endif
if (instance->ops->get_userdata(instance->device, md, &userdata))
return NULL;
<snip>
So as you can see, the checks were already there. They've just been
wrapped up with RTE_DEBUG macro for disabling them in non-debug
compilation mode and the validation of paramter was change to avoid
possible segmentation fault if instance lub ops would be NULL
>> https://protect2.fireeye.com/url?k=e0478418-bdd92a82-e0460f57-0cc47a336fae-55cc35a7b94c97c0&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_ethdev%2Frte_ethdev.h%23L4372
>>
>> Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such checks.
>> https://protect2.fireeye.com/url?k=79d7974a-244939d0-79d61c05-0cc47a336fae-19f540008a9467cf&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_cryptodev%2Frte_cryptodev.h%23L962
> That's a different story:
> rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions that
> have to be implemented by each ethdev/cryptodev API.
>
>>
>> Thanks,
>> Anoob
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
>>> Sent: Thursday, April 23, 2020 5:22 AM
>>> To: dev@dpdk.org
>>> Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin Ananyev
>>> <konstantin.ananyev@intel.com>
>>> Subject: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented
>>> ops
>>>
>>> Valid checks for optional function pointers inside dev-ops were disabled by
>>> undefined macro.
>>>
>>> Fixes: b6ee98547847 ("security: fix verification of parameters")
>>>
>>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>> ---
>>> lib/librte_security/rte_security.c | 4 ----
>>> 1 file changed, 4 deletions(-)
>>>
>>> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
>>> index d475b0977..b65430ce2 100644
>>> --- a/lib/librte_security/rte_security.c
>>> +++ b/lib/librte_security/rte_security.c
>>> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
>>> *instance,
>>> struct rte_security_session *sess,
>>> struct rte_mbuf *m, void *params) { -#ifdef
>>> RTE_DEBUG
>>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
>>> EINVAL,
>>> -ENOTSUP);
>>> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
>>> -#endif
>>> return instance->ops->set_pkt_metadata(instance->device,
>>> sess, m, params);
>>> }
>>> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx
>>> *instance, uint64_t md) {
>>> void *userdata = NULL;
>>>
>>> -#ifdef RTE_DEBUG
>>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
>>> NULL); -#endif
>>> if (instance->ops->get_userdata(instance->device, md, &userdata))
>>> return NULL;
>>>
>>> --
>>> 2.17.1
>
> -----Original Message-----
> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Sent: Thursday, April 23, 2020 9:06 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>; techboard@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops
>
>
> W dniu 23.04.2020 o 09:54, Ananyev, Konstantin pisze:
> >> Hi Konstantin,
> >>
> >> These are data path ops and so it will be better if we can avoid such checks in the datapath. The same is done in ethdev also.
> > AFAIK, get_userdata is an *optional* dev-ops function that can be used by data-path.
> > So far there was no strict requirement for the rte_security PMDs to *always* implement it.
> > So what you guys did is a silent change of public API behaviour.
> > As result ixgbe, (and probably some others rte_security PMDs) stopped working properly.
> > I don't see any point in these changes, but if you'd like to do that, at
> > least our usual procedure has to be followed:
> > 1. Send and RFC to get an agreement with rte_security PMDs maintainers (one release ahead)
> > 2. send a deprecation note (one release ahead)
> > 3. change the behaviour of the public API
> > 4. update release notes
> >
> > AFAIK 1), 2), 4) wasn't done.
> > So I think right now we need to revert original behaviour.
> The current changes were made in patch: b6ee9854784 security: fix
> verification of parameters
>
> <snip>
> @@ -91,7 +119,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);
> +#ifdef RTE_DEBUG
> + RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, NULL);
> +#endif
> if (instance->ops->get_userdata(instance->device, md, &userdata))
> return NULL;
> <snip>
>
>
> So as you can see, the checks were already there. They've just been
> wrapped up with RTE_DEBUG macro for disabling them in non-debug
> compilation mode and the validation of paramter was change to avoid
> possible segmentation fault if instance lub ops would be NULL
>
> >> https://protect2.fireeye.com/url?k=e0478418-bdd92a82-e0460f57-0cc47a336fae-
> 55cc35a7b94c97c0&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_ethdev%2Frte_ethdev.h%23L43
> 72
> >>
> >> Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such checks.
> >> https://protect2.fireeye.com/url?k=79d7974a-244939d0-79d61c05-0cc47a336fae-
> 19f540008a9467cf&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_cryptodev%2Frte_cryptodev.h%
> 23L962
> > That's a different story:
> > rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions that
> > have to be implemented by each ethdev/cryptodev API.
> >
> >>
> >> Thanks,
> >> Anoob
> >>
> >>> -----Original Message-----
> >>> From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> >>> Sent: Thursday, April 23, 2020 5:22 AM
> >>> To: dev@dpdk.org
> >>> Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin Ananyev
> >>> <konstantin.ananyev@intel.com>
> >>> Subject: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented
> >>> ops
> >>>
> >>> Valid checks for optional function pointers inside dev-ops were disabled by
> >>> undefined macro.
> >>>
> >>> Fixes: b6ee98547847 ("security: fix verification of parameters")
> >>>
> >>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >>> ---
> >>> lib/librte_security/rte_security.c | 4 ----
> >>> 1 file changed, 4 deletions(-)
> >>>
> >>> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> >>> index d475b0977..b65430ce2 100644
> >>> --- a/lib/librte_security/rte_security.c
> >>> +++ b/lib/librte_security/rte_security.c
> >>> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
> >>> *instance,
> >>> struct rte_security_session *sess,
> >>> struct rte_mbuf *m, void *params) { -#ifdef
> >>> RTE_DEBUG
> >>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> >>> EINVAL,
> >>> -ENOTSUP);
> >>> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> >>> -#endif
> >>> return instance->ops->set_pkt_metadata(instance->device,
> >>> sess, m, params);
> >>> }
> >>> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx
> >>> *instance, uint64_t md) {
> >>> void *userdata = NULL;
> >>>
> >>> -#ifdef RTE_DEBUG
> >>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> >>> NULL); -#endif
> >>> if (instance->ops->get_userdata(instance->device, md, &userdata))
> >>> return NULL;
> >>>
> >>> --
> >>> 2.17.1
> >
> --
>
> Lukasz Wojciechowski
> Principal Software Engineer
>
> Samsung R&D Institute Poland
> Samsung Electronics
> Office +48 22 377 88 25
> l.wojciechow@partner.samsung.com
> >> Hi Konstantin,
> >>
> >> These are data path ops and so it will be better if we can avoid such checks in the datapath. The same is done in ethdev also.
> > AFAIK, get_userdata is an *optional* dev-ops function that can be used by data-path.
> > So far there was no strict requirement for the rte_security PMDs to *always* implement it.
> > So what you guys did is a silent change of public API behaviour.
> > As result ixgbe, (and probably some others rte_security PMDs) stopped working properly.
> > I don't see any point in these changes, but if you'd like to do that, at
> > least our usual procedure has to be followed:
> > 1. Send and RFC to get an agreement with rte_security PMDs maintainers (one release ahead)
> > 2. send a deprecation note (one release ahead)
> > 3. change the behaviour of the public API
> > 4. update release notes
> >
> > AFAIK 1), 2), 4) wasn't done.
> > So I think right now we need to revert original behaviour.
> The current changes were made in patch: b6ee9854784 security: fix
> verification of parameters
>
> <snip>
> @@ -91,7 +119,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);
> +#ifdef RTE_DEBUG
> + RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, NULL);
> +#endif
> if (instance->ops->get_userdata(instance->device, md, &userdata))
> return NULL;
> <snip>
>
>
> So as you can see, the checks were already there.
>They've just been
> wrapped up with RTE_DEBUG macro for disabling them in non-debug
> compilation mode and the validation of paramter was change to avoid
> possible segmentation fault if instance lub ops would be NULL
Sigh, that's what I am talking about:
you effectively complied out valid checks for non-debug mode.
Yes, these checks have been there and they *should* stay there
for *ANY* mode (both debug and non-debug).
This is an *optional* dev-ops function.
PMD has a freedom not to implement optional function.
It is a rte_security framework responsibility to check that these functions
are implemented or not.
If you like to change that - the procedure described above has to be followed.
Konstantin
>
> >> https://protect2.fireeye.com/url?k=e0478418-bdd92a82-e0460f57-0cc47a336fae-
> 55cc35a7b94c97c0&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_ethdev%2Frte_ethdev.h%23L43
> 72
> >>
> >> Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such checks.
> >> https://protect2.fireeye.com/url?k=79d7974a-244939d0-79d61c05-0cc47a336fae-
> 19f540008a9467cf&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_cryptodev%2Frte_cryptodev.h%
> 23L962
> > That's a different story:
> > rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions that
> > have to be implemented by each ethdev/cryptodev API.
> >
> >>
> >> Thanks,
> >> Anoob
> >>
> >>> -----Original Message-----
> >>> From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> >>> Sent: Thursday, April 23, 2020 5:22 AM
> >>> To: dev@dpdk.org
> >>> Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin Ananyev
> >>> <konstantin.ananyev@intel.com>
> >>> Subject: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented
> >>> ops
> >>>
> >>> Valid checks for optional function pointers inside dev-ops were disabled by
> >>> undefined macro.
> >>>
> >>> Fixes: b6ee98547847 ("security: fix verification of parameters")
> >>>
> >>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >>> ---
> >>> lib/librte_security/rte_security.c | 4 ----
> >>> 1 file changed, 4 deletions(-)
> >>>
> >>> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> >>> index d475b0977..b65430ce2 100644
> >>> --- a/lib/librte_security/rte_security.c
> >>> +++ b/lib/librte_security/rte_security.c
> >>> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
> >>> *instance,
> >>> struct rte_security_session *sess,
> >>> struct rte_mbuf *m, void *params) { -#ifdef
> >>> RTE_DEBUG
> >>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> >>> EINVAL,
> >>> -ENOTSUP);
> >>> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> >>> -#endif
> >>> return instance->ops->set_pkt_metadata(instance->device,
> >>> sess, m, params);
> >>> }
> >>> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx
> >>> *instance, uint64_t md) {
> >>> void *userdata = NULL;
> >>>
> >>> -#ifdef RTE_DEBUG
> >>> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> >>> NULL); -#endif
> >>> if (instance->ops->get_userdata(instance->device, md, &userdata))
> >>> return NULL;
> >>>
> >>> --
> >>> 2.17.1
> >
> --
>
> Lukasz Wojciechowski
> Principal Software Engineer
>
> Samsung R&D Institute Poland
> Samsung Electronics
> Office +48 22 377 88 25
> l.wojciechow@partner.samsung.com
> W dniu 23.04.2020 o 06:07, Anoob Joseph pisze:
> > Hi Konstantin,
> >
> > These are data path ops and so it will be better if we can avoid such checks in the datapath. The same is done in ethdev also.
> >
> > https://protect2.fireeye.com/url?k=d44931cf-89d2cdac-d448ba80-0cc47a31cdbc-
> 8281a62b4c91d848&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_ethdev%2Frte_ethdev.h%23L43
> 72
> >
> > Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such checks.
> > https://protect2.fireeye.com/url?k=51324200-0ca9be63-5133c94f-0cc47a31cdbc-
> 11f88758fc12c996&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_cryptodev%2Frte_cryptodev.h%
> 23L962
> >
> >
> > Thanks,
> > Anoob
>
> Hi Konstantine,
>
> It's my fault. Sorry.
>
> These checks need to be disabled in non-debug code, so they should be
> wrapped in a macro. It's just not the valid macro.
> The discussion about rte_debug mode is ongoing
> (https://patchwork.dpdk.org/patch/68815/)
> and currently the v2 version of patches is prepared to gather
> maintainers opinion.
>
> After the rte_debug is introduced the proper macro to use will be
> RTE_DEBUG_SECURITY.
>
> Until then, the RTE_DEBUG macro can stay as like Anoob mentioned the
> checks will have impact on dataplane performance.
>
> If you want to enable this code, please use CFLAGS="-DRTE_DEBUG"
Really? So what we have to tell now to our customers?
"Yes, rte_security is broken and can easily crash your app.
But we might fix it in future versions... or maybe not.
For now just recompile our source with that flag enabled?"
Obviously this is not an option.
It is a bug and it is a stopper for 20.05 release.
It has to be fixed asap.
>
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> >> Sent: Thursday, April 23, 2020 5:22 AM
> >> To: dev@dpdk.org
> >> Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin Ananyev
> >> <konstantin.ananyev@intel.com>
> >> Subject: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented
> >> ops
> >>
> >> Valid checks for optional function pointers inside dev-ops were disabled by
> >> undefined macro.
> >>
> >> Fixes: b6ee98547847 ("security: fix verification of parameters")
> >>
> >> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >> ---
> >> lib/librte_security/rte_security.c | 4 ----
> >> 1 file changed, 4 deletions(-)
> >>
> >> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> >> index d475b0977..b65430ce2 100644
> >> --- a/lib/librte_security/rte_security.c
> >> +++ b/lib/librte_security/rte_security.c
> >> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
> >> *instance,
> >> struct rte_security_session *sess,
> >> struct rte_mbuf *m, void *params) { -#ifdef
> >> RTE_DEBUG
> >> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> >> EINVAL,
> >> -ENOTSUP);
> >> RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> >> -#endif
> >> return instance->ops->set_pkt_metadata(instance->device,
> >> sess, m, params);
> >> }
> >> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx
> >> *instance, uint64_t md) {
> >> void *userdata = NULL;
> >>
> >> -#ifdef RTE_DEBUG
> >> RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> >> NULL); -#endif
> >> if (instance->ops->get_userdata(instance->device, md, &userdata))
> >> return NULL;
> >>
> >> --
> >> 2.17.1
> >
> --
>
> Lukasz Wojciechowski
> Principal Software Engineer
>
> Samsung R&D Institute Poland
> Samsung Electronics
> Office +48 22 377 88 25
> l.wojciechow@partner.samsung.com
Hi Konstantin,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, April 23, 2020 1:24 PM
> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>;
> techboard@dpdk.org
> Subject: [EXT] RE: [dpdk-dev] [PATCH] security: fix crash at accessing non-
> implemented ops
>
> External Email
>
> ----------------------------------------------------------------------
> >
> > Hi Konstantin,
> >
> > These are data path ops and so it will be better if we can avoid such checks in
> the datapath. The same is done in ethdev also.
>
> AFAIK, get_userdata is an *optional* dev-ops function that can be used by data-
> path.
> So far there was no strict requirement for the rte_security PMDs to *always*
> implement it.
[Anoob] I don't think DPDK categorizes dev-ops as *optional* and *always*. If yes, can you point me?
My understanding is, all ops are optional. For example, I could implement a crypto PMD which is doing packet delivery only via event device (using crypto adapter). So dequeue op will not be implemented in that case and DPDK spec allows it.
> So what you guys did is a silent change of public API behaviour.
[Anoob] I believe Lukasz had submitted 3 or 4 revisions and it was all in the ML. RTE_DEBUG was suggested by Thomas I guess.
> As result ixgbe, (and probably some others rte_security PMDs) stopped working
> properly.
[Anoob] set_pkt_metadata() is the only one of interest to IXGBE. And I believe the function is implemented as well. So what exactly is the concern?
> I don't see any point in these changes, but if you'd like to do that, at least our
> usual procedure has to be followed:
> 1. Send and RFC to get an agreement with rte_security PMDs maintainers (one
> release ahead) 2. send a deprecation note (one release ahead) 3. change the
> behaviour of the public API 4. update release notes
>
> AFAIK 1), 2), 4) wasn't done.
> So I think right now we need to revert original behaviour.
>
> >
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_dpdk
> > _v20.02_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4372&d=DwIFAg&c=n
> > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> WYLn1v9SyTMrT5EQqh2TU&m=
> > 6ObfSanVVuHOsiqVlWxXsFWi-
> 2XNp76HCOX0vbUfma4&s=jDVyDDEILmgY1Yb9ZBswBVbn
> > 8FpZuQI5ukH_osmtUiI&e=
> >
> > Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such
> checks.
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_dpdk
> > _v20.02_source_lib_librte-5Fcryptodev_rte-5Fcryptodev.h-23L962&d=DwIFA
> > g&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> WYLn1v9SyTMrT5EQqh2
> > TU&m=6ObfSanVVuHOsiqVlWxXsFWi-
> 2XNp76HCOX0vbUfma4&s=LEWQOKs0r2Im_zL95VI
> > df4kQ2Pu0iRHV9Co2J1gsNBE&e=
>
> That's a different story:
> rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions that have
> to be implemented by each ethdev/cryptodev API.
[Anoob] I couldn't find any reference stating that way. If you can point me, I can update that to include datapath ops required for inline protocol processing.
>
> >
> >
> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> > > Sent: Thursday, April 23, 2020 5:22 AM
> > > To: dev@dpdk.org
> > > Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin
> > > Ananyev <konstantin.ananyev@intel.com>
> > > Subject: [dpdk-dev] [PATCH] security: fix crash at accessing
> > > non-implemented ops
> > >
> > > Valid checks for optional function pointers inside dev-ops were
> > > disabled by undefined macro.
> > >
> > > Fixes: b6ee98547847 ("security: fix verification of parameters")
> > >
> > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > ---
> > > lib/librte_security/rte_security.c | 4 ----
> > > 1 file changed, 4 deletions(-)
> > >
> > > diff --git a/lib/librte_security/rte_security.c
> > > b/lib/librte_security/rte_security.c
> > > index d475b0977..b65430ce2 100644
> > > --- a/lib/librte_security/rte_security.c
> > > +++ b/lib/librte_security/rte_security.c
> > > @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct
> > > rte_security_ctx *instance,
> > > struct rte_security_session *sess,
> > > struct rte_mbuf *m, void *params) { -#ifdef
> RTE_DEBUG
> > > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> > > EINVAL,
> > > -ENOTSUP);
> > > RTE_PTR_OR_ERR_RET(sess, -EINVAL); -#endif
> > > return instance->ops->set_pkt_metadata(instance->device,
> > > sess, m, params);
> > > }
> > > @@ -121,9 +119,7 @@ rte_security_get_userdata(struct
> > > rte_security_ctx *instance, uint64_t md) {
> > > void *userdata = NULL;
> > >
> > > -#ifdef RTE_DEBUG
> > > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> > > NULL); -#endif
> > > if (instance->ops->get_userdata(instance->device, md, &userdata))
> > > return NULL;
> > >
> > > --
> > > 2.17.1
> > >
> > > These are data path ops and so it will be better if we can avoid such checks in
> > the datapath. The same is done in ethdev also.
> >
> > AFAIK, get_userdata is an *optional* dev-ops function that can be used by data-
> > path.
> > So far there was no strict requirement for the rte_security PMDs to *always*
> > implement it.
>
> [Anoob] I don't think DPDK categorizes dev-ops as *optional* and *always*. If yes, can you point me?
> My understanding is, all ops are optional. For example, I could implement a crypto PMD which is doing packet delivery only via event device
> (using crypto adapter). So dequeue op will not be implemented in that case and DPDK spec allows it.
Your PMD can have enqueue_burst/dequeue_burst as NOP,
but you still have to provide valid function pointers:
they are stored inside crypto_dev structure itself and will be called
unconditionally (without any extra checking) by
rte_cryptodev_enqueue_burst/rte_cryptodev_dequeue_burst.
For all other calls (both data and control path) there is a check
that actual function pointer is a valid one.
Same story for eth dev: pkt_rx_burst/pkt_tx_burst and rest of dev-ops.
> > So what you guys did is a silent change of public API behaviour.
>
> [Anoob] I believe Lukasz had submitted 3 or 4 revisions and it was all in the ML. RTE_DEBUG was suggested by Thomas I guess.
I believe it is not a right procedure to change existing behaviour of rte_security framework.
I think you have to communicate clear and loudly in advance (at least one release in advance).
Plus RTE_DEBUG has nothing to do with changing non-debug behaviour.
> > As result ixgbe, (and probably some others rte_security PMDs) stopped working
> > properly.
>
> [Anoob] set_pkt_metadata() is the only one of interest to IXGBE. And I believe the function is implemented as well. So what exactly is the
> concern?
Check that ops->get_userdata is a valid function pointer will be compiled out.
So PMDs that don't implement this function will crash in rte_security_get_userdata().
In our particular case - ixgbe.
Same story with rte_security_set_pkt_metadata() - see the patch.
>
> > I don't see any point in these changes, but if you'd like to do that, at least our
> > usual procedure has to be followed:
> > 1. Send and RFC to get an agreement with rte_security PMDs maintainers (one
> > release ahead) 2. send a deprecation note (one release ahead) 3. change the
> > behaviour of the public API 4. update release notes
> >
> > AFAIK 1), 2), 4) wasn't done.
> > So I think right now we need to revert original behaviour.
> >
> > >
> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_dpdk
> > > _v20.02_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4372&d=DwIFAg&c=n
> > > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > WYLn1v9SyTMrT5EQqh2TU&m=
> > > 6ObfSanVVuHOsiqVlWxXsFWi-
> > 2XNp76HCOX0vbUfma4&s=jDVyDDEILmgY1Yb9ZBswBVbn
> > > 8FpZuQI5ukH_osmtUiI&e=
> > >
> > > Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such
> > checks.
> > > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_dpdk
> > > _v20.02_source_lib_librte-5Fcryptodev_rte-5Fcryptodev.h-23L962&d=DwIFA
> > > g&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > WYLn1v9SyTMrT5EQqh2
> > > TU&m=6ObfSanVVuHOsiqVlWxXsFWi-
> > 2XNp76HCOX0vbUfma4&s=LEWQOKs0r2Im_zL95VI
> > > df4kQ2Pu0iRHV9Co2J1gsNBE&e=
> >
> > That's a different story:
> > rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions that have
> > to be implemented by each ethdev/cryptodev API.
>
> [Anoob] I couldn't find any reference stating that way. If you can point me, I can update that to include datapath ops required for inline
> protocol processing.
Look at the code.
>
> >
> > >
> > >
> > > Thanks,
> > > Anoob
> > >
> > > > -----Original Message-----
> > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> > > > Sent: Thursday, April 23, 2020 5:22 AM
> > > > To: dev@dpdk.org
> > > > Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin
> > > > Ananyev <konstantin.ananyev@intel.com>
> > > > Subject: [dpdk-dev] [PATCH] security: fix crash at accessing
> > > > non-implemented ops
> > > >
> > > > Valid checks for optional function pointers inside dev-ops were
> > > > disabled by undefined macro.
> > > >
> > > > Fixes: b6ee98547847 ("security: fix verification of parameters")
> > > >
> > > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > > ---
> > > > lib/librte_security/rte_security.c | 4 ----
> > > > 1 file changed, 4 deletions(-)
> > > >
> > > > diff --git a/lib/librte_security/rte_security.c
> > > > b/lib/librte_security/rte_security.c
> > > > index d475b0977..b65430ce2 100644
> > > > --- a/lib/librte_security/rte_security.c
> > > > +++ b/lib/librte_security/rte_security.c
> > > > @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct
> > > > rte_security_ctx *instance,
> > > > struct rte_security_session *sess,
> > > > struct rte_mbuf *m, void *params) { -#ifdef
> > RTE_DEBUG
> > > > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> > > > EINVAL,
> > > > -ENOTSUP);
> > > > RTE_PTR_OR_ERR_RET(sess, -EINVAL); -#endif
> > > > return instance->ops->set_pkt_metadata(instance->device,
> > > > sess, m, params);
> > > > }
> > > > @@ -121,9 +119,7 @@ rte_security_get_userdata(struct
> > > > rte_security_ctx *instance, uint64_t md) {
> > > > void *userdata = NULL;
> > > >
> > > > -#ifdef RTE_DEBUG
> > > > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> > > > NULL); -#endif
> > > > if (instance->ops->get_userdata(instance->device, md, &userdata))
> > > > return NULL;
> > > >
> > > > --
> > > > 2.17.1
Hi Konstantin,
Please see inline.
Thanks,
Anoob
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Thursday, April 23, 2020 4:25 PM
> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org; Lukasz
> Wojciechowski <l.wojciechow@partner.samsung.com>
> Cc: akhil.goyal@nxp.com; Doherty, Declan <declan.doherty@intel.com>
> Subject: [EXT] RE: [dpdk-dev] [PATCH] security: fix crash at accessing non-
> implemented ops
>
> External Email
>
> ----------------------------------------------------------------------
>
> > > >
> > > > These are data path ops and so it will be better if we can avoid
> > > > such checks in
> > > the datapath. The same is done in ethdev also.
> > >
> > > AFAIK, get_userdata is an *optional* dev-ops function that can be
> > > used by data- path.
> > > So far there was no strict requirement for the rte_security PMDs to
> > > *always* implement it.
> >
> > [Anoob] I don't think DPDK categorizes dev-ops as *optional* and *always*. If
> yes, can you point me?
>
> > My understanding is, all ops are optional. For example, I could
> > implement a crypto PMD which is doing packet delivery only via event device
> (using crypto adapter). So dequeue op will not be implemented in that case and
> DPDK spec allows it.
>
> Your PMD can have enqueue_burst/dequeue_burst as NOP, but you still have to
> provide valid function pointers:
> they are stored inside crypto_dev structure itself and will be called
> unconditionally (without any extra checking) by
> rte_cryptodev_enqueue_burst/rte_cryptodev_dequeue_burst.
[Anoob] I think there is a basic misunderstanding here. You are treating unconditional calls as mandatory implementations. If that is the case rte_eth_tx_burst() & rte_eth_rx_burst() shouldn't check for function pointers even when DEBUG is enabled.
static inline uint16_t
rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
struct rte_mbuf **rx_pkts, const uint16_t nb_pkts)
{
struct rte_eth_dev *dev = &rte_eth_devices[port_id];
uint16_t nb_rx;
#ifdef RTE_LIBRTE_ETHDEV_DEBUG
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, 0);
RTE_FUNC_PTR_OR_ERR_RET(*dev->rx_pkt_burst, 0);
if (queue_id >= dev->data->nb_rx_queues) {
RTE_ETHDEV_LOG(ERR, "Invalid RX queue_id=%u\n", queue_id);
return 0;
}
#endif
nb_rx = (*dev->rx_pkt_burst)(dev->data->rx_queues[queue_id],
rx_pkts, nb_pkts);
From my view point, function pointer checks and argument checks are required in every API for stability. But having such checks in the datapath adversely affects the performance. And for cases where function pointers are not set, application would get one crash in the first run. And that can be debugged after having the required options enabled.
> For all other calls (both data and control path) there is a check that actual
> function pointer is a valid one.
> Same story for eth dev: pkt_rx_burst/pkt_tx_burst and rest of dev-ops.
>
> > > So what you guys did is a silent change of public API behaviour.
> >
> > [Anoob] I believe Lukasz had submitted 3 or 4 revisions and it was all in the ML.
> RTE_DEBUG was suggested by Thomas I guess.
>
> I believe it is not a right procedure to change existing behaviour of rte_security
> framework.
> I think you have to communicate clear and loudly in advance (at least one
> release in advance).
> Plus RTE_DEBUG has nothing to do with changing non-debug behaviour.
>
> > > As result ixgbe, (and probably some others rte_security PMDs)
> > > stopped working properly.
> >
> > [Anoob] set_pkt_metadata() is the only one of interest to IXGBE. And I
> > believe the function is implemented as well. So what exactly is the concern?
>
> Check that ops->get_userdata is a valid function pointer will be compiled out.
> So PMDs that don't implement this function will crash in
> rte_security_get_userdata().
> In our particular case - ixgbe.
> Same story with rte_security_set_pkt_metadata() - see the patch.
[Anoob] But ixgbe doesn't implement inline protocol which is the primary consumer of this API (rte_security_get_userdata()). So what is the trouble?
Also, application is expected to call rte_security_set_pkt_metadata() only on devices with offload flag RTE_SECURITY_TX_OLOAD_NEED_MDATA. If a PMD states it needs MDATA but fails to register a function pointer for doing the same, it is a control path problem. Checking for that in the datapath is an overkill.
>
> >
> > > I don't see any point in these changes, but if you'd like to do
> > > that, at least our usual procedure has to be followed:
> > > 1. Send and RFC to get an agreement with rte_security PMDs
> > > maintainers (one release ahead) 2. send a deprecation note (one
> > > release ahead) 3. change the behaviour of the public API 4. update
> > > release notes
> > >
> > > AFAIK 1), 2), 4) wasn't done.
> > > So I think right now we need to revert original behaviour.
> > >
> > > >
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_
> > > > dpdk
> > > > _v20.02_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4372&d=DwIFAg
> > > > &c=n
> > > > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > > WYLn1v9SyTMrT5EQqh2TU&m=
> > > > 6ObfSanVVuHOsiqVlWxXsFWi-
> > > 2XNp76HCOX0vbUfma4&s=jDVyDDEILmgY1Yb9ZBswBVbn
> > > > 8FpZuQI5ukH_osmtUiI&e=
> > > >
> > > > Datapath functions in cryptodev (enqueue/dequeue) doesn't even
> > > > have such
> > > checks.
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_
> > > > dpdk
> > > > _v20.02_source_lib_librte-5Fcryptodev_rte-5Fcryptodev.h-23L962&d=D
> > > > wIFA
> > > > g&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> > > WYLn1v9SyTMrT5EQqh2
> > > > TU&m=6ObfSanVVuHOsiqVlWxXsFWi-
> > > 2XNp76HCOX0vbUfma4&s=LEWQOKs0r2Im_zL95VI
> > > > df4kQ2Pu0iRHV9Co2J1gsNBE&e=
> > >
> > > That's a different story:
> > > rx_burst/tx_burst, enqueue/dequeue are mandatory dev-ops functions
> > > that have to be implemented by each ethdev/cryptodev API.
> >
> > [Anoob] I couldn't find any reference stating that way. If you can
> > point me, I can update that to include datapath ops required for inline protocol
> processing.
>
> Look at the code.
[Anoob] Code is not conclusive for this as I've explained above for rte_eth_rx/tx_burst() case.
>
> >
> > >
> > > >
> > > >
> > > > Thanks,
> > > > Anoob
> > > >
> > > > > -----Original Message-----
> > > > > From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> > > > > Sent: Thursday, April 23, 2020 5:22 AM
> > > > > To: dev@dpdk.org
> > > > > Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin
> > > > > Ananyev <konstantin.ananyev@intel.com>
> > > > > Subject: [dpdk-dev] [PATCH] security: fix crash at accessing
> > > > > non-implemented ops
> > > > >
> > > > > Valid checks for optional function pointers inside dev-ops were
> > > > > disabled by undefined macro.
> > > > >
> > > > > Fixes: b6ee98547847 ("security: fix verification of parameters")
> > > > >
> > > > > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > > > ---
> > > > > lib/librte_security/rte_security.c | 4 ----
> > > > > 1 file changed, 4 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_security/rte_security.c
> > > > > b/lib/librte_security/rte_security.c
> > > > > index d475b0977..b65430ce2 100644
> > > > > --- a/lib/librte_security/rte_security.c
> > > > > +++ b/lib/librte_security/rte_security.c
> > > > > @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct
> > > > > rte_security_ctx *instance,
> > > > > struct rte_security_session *sess,
> > > > > struct rte_mbuf *m, void *params) { -#ifdef
> > > RTE_DEBUG
> > > > > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
> > > > > EINVAL,
> > > > > -ENOTSUP);
> > > > > RTE_PTR_OR_ERR_RET(sess, -EINVAL); -#endif
> > > > > return instance->ops->set_pkt_metadata(instance->device,
> > > > > sess, m, params);
> > > > > }
> > > > > @@ -121,9 +119,7 @@ rte_security_get_userdata(struct
> > > > > rte_security_ctx *instance, uint64_t md) {
> > > > > void *userdata = NULL;
> > > > >
> > > > > -#ifdef RTE_DEBUG
> > > > > RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> > > > > NULL); -#endif
> > > > > if (instance->ops->get_userdata(instance->device, md, &userdata))
> > > > > return NULL;
> > > > >
> > > > > --
> > > > > 2.17.1
Hi Anoob/Konstantin,
> >
> > Check that ops->get_userdata is a valid function pointer will be compiled out.
> > So PMDs that don't implement this function will crash in
> > rte_security_get_userdata().
> > In our particular case - ixgbe.
> > Same story with rte_security_set_pkt_metadata() - see the patch.
>
> [Anoob] But ixgbe doesn't implement inline protocol which is the primary
> consumer of this API (rte_security_get_userdata()). So what is the trouble?
>
> Also, application is expected to call rte_security_set_pkt_metadata() only on
> devices with offload flag RTE_SECURITY_TX_OLOAD_NEED_MDATA. If a PMD
> states it needs MDATA but fails to register a function pointer for doing the same,
> it is a control path problem. Checking for that in the datapath is an overkill.
>
Whatever your concern is, we can resolve it later, but for now we should have the same
Unconditional checks that were there earlier. We need to make RC1 today/tomorrow.
And this cannot go as an issue.
These are optional APIs and every PMD may not have supported that.
Konstantin,
Please send an update to your patch reverting the original patch for these 2 functions.
Currently it is adding 2 extra checks.
Regards,
Akhil
W dniu 23.04.2020 o 14:55, Akhil Goyal pisze:
> Hi Anoob/Konstantin,
>>> Check that ops->get_userdata is a valid function pointer will be compiled out.
>>> So PMDs that don't implement this function will crash in
>>> rte_security_get_userdata().
>>> In our particular case - ixgbe.
>>> Same story with rte_security_set_pkt_metadata() - see the patch.
>> [Anoob] But ixgbe doesn't implement inline protocol which is the primary
>> consumer of this API (rte_security_get_userdata()). So what is the trouble?
>>
>> Also, application is expected to call rte_security_set_pkt_metadata() only on
>> devices with offload flag RTE_SECURITY_TX_OLOAD_NEED_MDATA. If a PMD
>> states it needs MDATA but fails to register a function pointer for doing the same,
>> it is a control path problem. Checking for that in the datapath is an overkill.
>>
> Whatever your concern is, we can resolve it later, but for now we should have the same
> Unconditional checks that were there earlier. We need to make RC1 today/tomorrow.
> And this cannot go as an issue.
>
> These are optional APIs and every PMD may not have supported that.
>
> Konstantin,
> Please send an update to your patch reverting the original patch for these 2 functions.
> Currently it is adding 2 extra checks.
>
> Regards,
> Akhil
>
Please remember also about updating app/test.
I will be glad to help with this matter.
Hi Akhil,
>
> Hi Anoob/Konstantin,
> > >
> > > Check that ops->get_userdata is a valid function pointer will be compiled out.
> > > So PMDs that don't implement this function will crash in
> > > rte_security_get_userdata().
> > > In our particular case - ixgbe.
> > > Same story with rte_security_set_pkt_metadata() - see the patch.
> >
> > [Anoob] But ixgbe doesn't implement inline protocol which is the primary
> > consumer of this API (rte_security_get_userdata()). So what is the trouble?
> >
> > Also, application is expected to call rte_security_set_pkt_metadata() only on
> > devices with offload flag RTE_SECURITY_TX_OLOAD_NEED_MDATA. If a PMD
> > states it needs MDATA but fails to register a function pointer for doing the same,
> > it is a control path problem. Checking for that in the datapath is an overkill.
> >
> Whatever your concern is, we can resolve it later, but for now we should have the same
> Unconditional checks that were there earlier. We need to make RC1 today/tomorrow.
> And this cannot go as an issue.
>
> These are optional APIs and every PMD may not have supported that.
>
> Konstantin,
> Please send an update to your patch reverting the original patch for these 2 functions.
> Currently it is adding 2 extra checks.
>
I am afraid we can't do just that.
As in that case /app/test/test_security.c build wih -DRE_DEBUG will start crashing.
I think we have 3 alternative how to fix it:
1. Keep all these 3 checks for debug and non-debug mode (that what my current patch does).
2. Have both: existed 1 check in non-debug mode, plus new checks in debug mode, i.e.:
rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
{
void *userdata = NULL;
+#ifdef RTE_DEBUG
+ RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, NULL);
+#else
RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL);
+#endif
....
3. Keep only 1 existed check in non-debug mode and remove cases in app/test/test_security.c
that would crash with -DRTE_DEBUG.
My preference is 1), I don't think these 2 extra checks will affect performance greatly.
Also with 1) we can make these new test-case to be executed for non-debug mode too.
2) is probably also ok - but I think RTE_DEBUG concept should be a separate patch series,
and I don't want to mix things.
What is your opinion here?
Konstantin
>
> Hi Akhil,
>
> >
> > Hi Anoob/Konstantin,
> > > >
> > > > Check that ops->get_userdata is a valid function pointer will be compiled
> out.
> > > > So PMDs that don't implement this function will crash in
> > > > rte_security_get_userdata().
> > > > In our particular case - ixgbe.
> > > > Same story with rte_security_set_pkt_metadata() - see the patch.
> > >
> > > [Anoob] But ixgbe doesn't implement inline protocol which is the primary
> > > consumer of this API (rte_security_get_userdata()). So what is the trouble?
> > >
> > > Also, application is expected to call rte_security_set_pkt_metadata() only on
> > > devices with offload flag RTE_SECURITY_TX_OLOAD_NEED_MDATA. If a
> PMD
> > > states it needs MDATA but fails to register a function pointer for doing the
> same,
> > > it is a control path problem. Checking for that in the datapath is an overkill.
> > >
> > Whatever your concern is, we can resolve it later, but for now we should have
> the same
> > Unconditional checks that were there earlier. We need to make RC1
> today/tomorrow.
> > And this cannot go as an issue.
> >
> > These are optional APIs and every PMD may not have supported that.
> >
> > Konstantin,
> > Please send an update to your patch reverting the original patch for these 2
> functions.
> > Currently it is adding 2 extra checks.
> >
>
> I am afraid we can't do just that.
> As in that case /app/test/test_security.c build wih -DRE_DEBUG will start
> crashing.
>
> I think we have 3 alternative how to fix it:
>
> 1. Keep all these 3 checks for debug and non-debug mode (that what my current
> patch does).
> 2. Have both: existed 1 check in non-debug mode, plus new checks in debug
> mode, i.e.:
> rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
> {
> void *userdata = NULL;
>
> +#ifdef RTE_DEBUG
> + RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> NULL);
> +#else
> RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL);
> +#endif
>
> ....
>
> 3. Keep only 1 existed check in non-debug mode and remove cases in
> app/test/test_security.c
> that would crash with -DRTE_DEBUG.
>
> My preference is 1), I don't think these 2 extra checks will affect performance
> greatly.
> Also with 1) we can make these new test-case to be executed for non-debug
> mode too.
> 2) is probably also ok - but I think RTE_DEBUG concept should be a separate
> patch series,
> and I don't want to mix things.
> What is your opinion here?
>
I am OK with both 1 and 2.
Anoob may be concerned about the performance.
But if we go with 2, it would be better to have
rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
{
void *userdata = NULL;
+#ifdef RTE_DEBUG
+ RTE_PTR_OR_ERR_RET(instance, NULL);
+ RTE_PTR_OR_ERR_RET(instance->ops, NULL);
+#endif
RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL);
....
}
And for security test, we can have a separate patch. Lukasz or you can send that later if not now.
> >
> > Hi Akhil,
> >
> > >
> > > Hi Anoob/Konstantin,
> > > > >
> > > > > Check that ops->get_userdata is a valid function pointer will be compiled
> > out.
> > > > > So PMDs that don't implement this function will crash in
> > > > > rte_security_get_userdata().
> > > > > In our particular case - ixgbe.
> > > > > Same story with rte_security_set_pkt_metadata() - see the patch.
> > > >
> > > > [Anoob] But ixgbe doesn't implement inline protocol which is the primary
> > > > consumer of this API (rte_security_get_userdata()). So what is the trouble?
> > > >
> > > > Also, application is expected to call rte_security_set_pkt_metadata() only on
> > > > devices with offload flag RTE_SECURITY_TX_OLOAD_NEED_MDATA. If a
> > PMD
> > > > states it needs MDATA but fails to register a function pointer for doing the
> > same,
> > > > it is a control path problem. Checking for that in the datapath is an overkill.
> > > >
> > > Whatever your concern is, we can resolve it later, but for now we should have
> > the same
> > > Unconditional checks that were there earlier. We need to make RC1
> > today/tomorrow.
> > > And this cannot go as an issue.
> > >
> > > These are optional APIs and every PMD may not have supported that.
> > >
> > > Konstantin,
> > > Please send an update to your patch reverting the original patch for these 2
> > functions.
> > > Currently it is adding 2 extra checks.
> > >
> >
> > I am afraid we can't do just that.
> > As in that case /app/test/test_security.c build wih -DRE_DEBUG will start
> > crashing.
> >
> > I think we have 3 alternative how to fix it:
> >
> > 1. Keep all these 3 checks for debug and non-debug mode (that what my current
> > patch does).
> > 2. Have both: existed 1 check in non-debug mode, plus new checks in debug
> > mode, i.e.:
> > rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
> > {
> > void *userdata = NULL;
> >
> > +#ifdef RTE_DEBUG
> > + RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
> > NULL);
> > +#else
> > RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL);
> > +#endif
> >
> > ....
> >
> > 3. Keep only 1 existed check in non-debug mode and remove cases in
> > app/test/test_security.c
> > that would crash with -DRTE_DEBUG.
> >
> > My preference is 1), I don't think these 2 extra checks will affect performance
> > greatly.
> > Also with 1) we can make these new test-case to be executed for non-debug
> > mode too.
> > 2) is probably also ok - but I think RTE_DEBUG concept should be a separate
> > patch series,
> > and I don't want to mix things.
> > What is your opinion here?
> >
> I am OK with both 1 and 2.
> Anoob may be concerned about the performance.
> But if we go with 2, it would be better to have
> rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
> {
> void *userdata = NULL;
>
> +#ifdef RTE_DEBUG
> + RTE_PTR_OR_ERR_RET(instance, NULL);
> + RTE_PTR_OR_ERR_RET(instance->ops, NULL);
> +#endif
> RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL);
> ....
> }
>
> And for security test, we can have a separate patch. Lukasz or you can send that later if not now.
Ok, then to keep everyone happy will go with your code snippet above.
@@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
struct rte_security_session *sess,
struct rte_mbuf *m, void *params)
{
-#ifdef RTE_DEBUG
RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -EINVAL,
-ENOTSUP);
RTE_PTR_OR_ERR_RET(sess, -EINVAL);
-#endif
return instance->ops->set_pkt_metadata(instance->device,
sess, m, params);
}
@@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
{
void *userdata = NULL;
-#ifdef RTE_DEBUG
RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL, NULL);
-#endif
if (instance->ops->get_userdata(instance->device, md, &userdata))
return NULL;