[dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Konstantin Ananyev]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Ananyev, Konstantin]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Anoob Joseph]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Ananyev, Konstantin]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Lukasz Wojciechowski]

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

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Lukasz Wojciechowski]

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

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Lukasz Wojciechowski]

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

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Ananyev, Konstantin]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Ananyev, Konstantin]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Ananyev, Konstantin]

> 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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Anoob Joseph]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Ananyev, 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.

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Anoob Joseph]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Akhil Goyal]

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

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Lukasz Wojciechowski]

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.

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Ananyev, 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?

Konstantin

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Akhil Goyal]

> 
> 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.

Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Ananyev, 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.

Ok, then to keep everyone happy will go with your code snippet above.

[dpdk-dev] [PATCH v2] security: fix crash at accessing non-implemented ops

[Konstantin Ananyev]

Valid checks for optional function pointers inside dev-ops
were disabled by undefined macro.

Fixes: b6ee98547847 ("security: fix verification of parameters")
Cc: stable@dpdk.org

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 lib/librte_security/rte_security.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
index d475b0977..dc9a3e89c 100644
--- a/lib/librte_security/rte_security.c
+++ b/lib/librte_security/rte_security.c
@@ -108,10 +108,11 @@ rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
 			      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);
+	RTE_PTR_OR_ERR_RET(instance, -EINVAL);
+	RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL);
 #endif
+	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);
 	return instance->ops->set_pkt_metadata(instance->device,
 					       sess, m, params);
 }
@@ -122,8 +123,10 @@ 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);
+	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);
 	if (instance->ops->get_userdata(instance->device, md, &userdata))
 		return NULL;
 
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] security: fix crash at accessing non-implemented ops

[Akhil Goyal]

> Valid checks for optional function pointers inside dev-ops
> were disabled by undefined macro.
> 
> Fixes: b6ee98547847 ("security: fix verification of parameters")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---

Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Anoob,

Do you have any concerns over this patch?

Regards,
Akhil

Re: [dpdk-dev] [PATCH v2] security: fix crash at accessing non-implemented ops

[Anoob Joseph]

Hi Akhil,

I have my concerns over unwanted checks in the datapath. Something that crypto enqueue/dequeue APIs are not doing is being enforced on other APIs. As Konstantin had suggested, PMDs (IXGBE here) could define a function which returns -ENOTSUP and it would have been win-win for everyone.

Anyway, I don't have any objections to this.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, April 23, 2020 9:22 PM
> To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org;
> Anoob Joseph <anoobj@marvell.com>
> Cc: declan.doherty@intel.com; stable@dpdk.org
> Subject: [EXT] RE: [PATCH v2] security: fix crash at accessing non-implemented
> ops
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> > Valid checks for optional function pointers inside dev-ops were
> > disabled by undefined macro.
> >
> > Fixes: b6ee98547847 ("security: fix verification of parameters")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> 
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> 
> Anoob,
> 
> Do you have any concerns over this patch?
> 
> Regards,
> Akhil

Re: [dpdk-dev] [PATCH v2] security: fix crash at accessing non-implemented ops

[Akhil Goyal]

Hi Anoob,
> 
> Hi Akhil,
> 
> I have my concerns over unwanted checks in the datapath. Something that
> crypto enqueue/dequeue APIs are not doing is being enforced on other APIs. As
> Konstantin had suggested, PMDs (IXGBE here) could define a function which
> returns -ENOTSUP and it would have been win-win for everyone.
> 
> Anyway, I don't have any objections to this.
> 
Your concerns are valid and those can be handled separately.
We will apply this patch.

[dpdk-dev] [PATCH] test/security: enable tests for non-implemented ops

[Lukasz Wojciechowski]

After re-enabling checks for non-implemneted ops in non-debug mode
in librte_security set_pkt_metadata and get_userdata functions,
tests verifying proper work of tests can be enabled also.

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test_security.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/app/test/test_security.c b/app/test/test_security.c
index 724ce56f4..3076a4c5a 100644
--- a/app/test/test_security.c
+++ b/app/test/test_security.c
@@ -1474,7 +1474,6 @@ test_set_pkt_metadata_inv_context_ops(void)
 static int
 test_set_pkt_metadata_inv_context_ops_fun(void)
 {
-#ifdef RTE_DEBUG
 	struct security_unittest_params *ut_params = &unittest_params;
 	struct rte_mbuf m;
 	int params;
@@ -1487,9 +1486,6 @@ test_set_pkt_metadata_inv_context_ops_fun(void)
 	TEST_ASSERT_MOCK_CALLS(mock_set_pkt_metadata_exp, 0);
 
 	return TEST_SUCCESS;
-#else
-	return TEST_SKIPPED;
-#endif
 }
 
 /**
@@ -1621,7 +1617,6 @@ test_get_userdata_inv_context_ops(void)
 static int
 test_get_userdata_inv_context_ops_fun(void)
 {
-#ifdef RTE_DEBUG
 	struct security_unittest_params *ut_params = &unittest_params;
 	uint64_t md = 0xDEADBEEF;
 	ut_params->ctx.ops = &empty_ops;
@@ -1632,9 +1627,6 @@ test_get_userdata_inv_context_ops_fun(void)
 	TEST_ASSERT_MOCK_CALLS(mock_get_userdata_exp, 0);
 
 	return TEST_SUCCESS;
-#else
-	return TEST_SKIPPED;
-#endif
 }
 
 /**
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] security: fix crash at accessing non-implemented ops

[Lukasz Wojciechowski]

W dniu 23.04.2020 o 18:14, Akhil Goyal pisze:
> Hi Anoob,
>> Hi Akhil,
>>
>> I have my concerns over unwanted checks in the datapath. Something that
>> crypto enqueue/dequeue APIs are not doing is being enforced on other APIs. As
>> Konstantin had suggested, PMDs (IXGBE here) could define a function which
>> returns -ENOTSUP and it would have been win-win for everyone.
>>
>> Anyway, I don't have any objections to this.
>>
> Your concerns are valid and those can be handled separately.
> We will apply this patch.

I just pushed a patch enabling 2 tests in non-debug build mode.

Sorry for the trouble I caused


Best regards

Lukasz

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com

Re: [dpdk-dev] [PATCH] test/security: enable tests for non-implemented ops

[Akhil Goyal]

> 
> After re-enabling checks for non-implemneted ops in non-debug mode
> in librte_security set_pkt_metadata and get_userdata functions,
> tests verifying proper work of tests can be enabled also.
> 
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> ---
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

Applied to dpdk-next-crypto

Thanks.

Re: [dpdk-dev] [PATCH] test/security: enable tests for non-implemented ops

[Lukasz Wojciechowski]

W dniu 09.05.2020 o 23:47, Akhil Goyal pisze:
>> After re-enabling checks for non-implemneted ops in non-debug mode
>> in librte_security set_pkt_metadata and get_userdata functions,
>> tests verifying proper work of tests can be enabled also.
>>
>> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>> ---
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
>
> Applied to dpdk-next-crypto
>
> Thanks.
Thank you

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow@partner.samsung.com