[dpdk-stable] [PATCH v2 01/13] security: fix verification of parameters

[dpdk-stable] [PATCH v2 01/13] security: fix verification of parameters

[Lukasz Wojciechowski]

This patch adds verification of the parameters to the ret_security API
functions. All required parameters are checked if they are not NULL.

Checks verify full chain of pointers, e.g. in case of verification of
"instance->ops->session_XXX", they check also "instance"
and "instance->ops".

Fixes: c261d1431bd8 ("security: introduce security API and framework")
Cc: akhil.goyal@nxp.com

Fixes: 1a08c379b9b5 ("security: support user data retrieval")
Cc: anoob.joseph@caviumnetworks.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 config/common_base                 |  1 +
 lib/librte_security/rte_security.c | 59 +++++++++++++++++++++++-------
 2 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/config/common_base b/config/common_base
index c31175f9d..ef1cdbb62 100644
--- a/config/common_base
+++ b/config/common_base
@@ -695,6 +695,7 @@ CONFIG_RTE_LIBRTE_PMD_NITROX=y
 # Compile generic security library
 #
 CONFIG_RTE_LIBRTE_SECURITY=y
+CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
 
 #
 # Compile generic compression device library
diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
index bc81ce15d..f1b4a894e 100644
--- a/lib/librte_security/rte_security.c
+++ b/lib/librte_security/rte_security.c
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright 2017 NXP.
  * Copyright(c) 2017 Intel Corporation.
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd All Rights Reserved
  */
 
 #include <rte_malloc.h>
@@ -9,6 +10,19 @@
 #include "rte_security.h"
 #include "rte_security_driver.h"
 
+/* Macro to check for invalid pointers */
+#define RTE_PTR_OR_ERR_RET(ptr, retval) do {	\
+	if ((ptr) == NULL)			\
+		return retval;			\
+} while (0)
+
+/* Macro to check for invalid pointers chains */
+#define RTE_PTR_CHAIN3_OR_ERR_RET(p1, p2, p3, retval, last_retval) do {	\
+	RTE_PTR_OR_ERR_RET(p1, retval);					\
+	RTE_PTR_OR_ERR_RET(p1->p2, retval);				\
+	RTE_PTR_OR_ERR_RET(p1->p2->p3, last_retval);			\
+} while (0)
+
 struct rte_security_session *
 rte_security_session_create(struct rte_security_ctx *instance,
 			    struct rte_security_session_conf *conf,
@@ -16,10 +30,9 @@ rte_security_session_create(struct rte_security_ctx *instance,
 {
 	struct rte_security_session *sess = NULL;
 
-	if (conf == NULL)
-		return NULL;
-
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_create, NULL, NULL);
+	RTE_PTR_OR_ERR_RET(conf, NULL);
+	RTE_PTR_OR_ERR_RET(mp, NULL);
 
 	if (rte_mempool_get(mp, (void **)&sess))
 		return NULL;
@@ -38,14 +51,19 @@ rte_security_session_update(struct rte_security_ctx *instance,
 			    struct rte_security_session *sess,
 			    struct rte_security_session_conf *conf)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_update, -ENOTSUP);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_update, -EINVAL,
+			-ENOTSUP);
+	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
+	RTE_PTR_OR_ERR_RET(conf, -EINVAL);
+
 	return instance->ops->session_update(instance->device, sess, conf);
 }
 
 unsigned int
 rte_security_session_get_size(struct rte_security_ctx *instance)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_get_size, 0);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_get_size, 0, 0);
+
 	return instance->ops->session_get_size(instance->device);
 }
 
@@ -54,7 +72,11 @@ rte_security_session_stats_get(struct rte_security_ctx *instance,
 			       struct rte_security_session *sess,
 			       struct rte_security_stats *stats)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_stats_get, -ENOTSUP);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_stats_get, -EINVAL,
+			-ENOTSUP);
+	/* Parameter sess can be NULL in case of getting global statistics. */
+	RTE_PTR_OR_ERR_RET(stats, -EINVAL);
+
 	return instance->ops->session_stats_get(instance->device, sess, stats);
 }
 
@@ -64,7 +86,9 @@ rte_security_session_destroy(struct rte_security_ctx *instance,
 {
 	int ret;
 
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, -ENOTSUP);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_destroy, -EINVAL,
+			-ENOTSUP);
+	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
 
 	if (instance->sess_cnt)
 		instance->sess_cnt--;
@@ -81,7 +105,11 @@ rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
 			      struct rte_security_session *sess,
 			      struct rte_mbuf *m, void *params)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);
+#ifdef RTE_LIBRTE_SECURITY_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);
 }
@@ -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_LIBRTE_SECURITY_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;
 
@@ -101,7 +131,8 @@ rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
 const struct rte_security_capability *
 rte_security_capabilities_get(struct rte_security_ctx *instance)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, capabilities_get, NULL, NULL);
+
 	return instance->ops->capabilities_get(instance->device);
 }
 
@@ -113,7 +144,9 @@ rte_security_capability_get(struct rte_security_ctx *instance,
 	const struct rte_security_capability *capability;
 	uint16_t i = 0;
 
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, capabilities_get, NULL, NULL);
+	RTE_PTR_OR_ERR_RET(idx, NULL);
+
 	capabilities = instance->ops->capabilities_get(instance->device);
 
 	if (capabilities == NULL)
@@ -121,7 +154,7 @@ rte_security_capability_get(struct rte_security_ctx *instance,
 
 	while ((capability = &capabilities[i++])->action
 			!= RTE_SECURITY_ACTION_TYPE_NONE) {
-		if (capability->action  == idx->action &&
+		if (capability->action == idx->action &&
 				capability->protocol == idx->protocol) {
 			if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) {
 				if (capability->ipsec.proto ==
-- 
2.17.1

Re: [dpdk-stable] [PATCH v2 01/13] security: fix verification of parameters

[Thomas Monjalon]

08/04/2020 05:13, Lukasz Wojciechowski:
> This patch adds verification of the parameters to the ret_security API
> functions. All required parameters are checked if they are not NULL.
[...]
> --- a/config/common_base
> +++ b/config/common_base
>  CONFIG_RTE_LIBRTE_SECURITY=y
> +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n

Is it a leftover?

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Anoob Joseph]

Hi Thomas,

> 08/04/2020 05:13, Lukasz Wojciechowski:
> > This patch adds verification of the parameters to the ret_security API
> > functions. All required parameters are checked if they are not NULL.
> [...]
> > --- a/config/common_base
> > +++ b/config/common_base
> >  CONFIG_RTE_LIBRTE_SECURITY=y
> > +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
> 
> Is it a leftover?
> 

[Anoob]  It is similar to 'RTE_LIBRTE_ETHDEV_DEBUG' for usage in datapath. Like in, http://code.dpdk.org/dpdk/latest/source/lib/librte_ethdev/rte_ethdev.h#L4378

Thanks,
Anoob

Re: [dpdk-stable] [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Thomas Monjalon]

08/04/2020 15:02, Anoob Joseph:
> Hi Thomas,
> 
> > 08/04/2020 05:13, Lukasz Wojciechowski:
> > > This patch adds verification of the parameters to the ret_security API
> > > functions. All required parameters are checked if they are not NULL.
> > [...]
> > > --- a/config/common_base
> > > +++ b/config/common_base
> > >  CONFIG_RTE_LIBRTE_SECURITY=y
> > > +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
> > 
> > Is it a leftover?
> > 
> 
> [Anoob]  It is similar to 'RTE_LIBRTE_ETHDEV_DEBUG' for usage in datapath. Like in, http://code.dpdk.org/dpdk/latest/source/lib/librte_ethdev/rte_ethdev.h#L4378

1/ I don't see it used in this patch
2/ Adding makefile-only option is weird
3/ Adding new compile-time options is discouraged

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Anoob Joseph]

Hi Thomas,

> 
> ----------------------------------------------------------------------
> 08/04/2020 15:02, Anoob Joseph:
> > Hi Thomas,
> >
> > > 08/04/2020 05:13, Lukasz Wojciechowski:
> > > > This patch adds verification of the parameters to the ret_security
> > > > API functions. All required parameters are checked if they are not NULL.
> > > [...]
> > > > --- a/config/common_base
> > > > +++ b/config/common_base
> > > >  CONFIG_RTE_LIBRTE_SECURITY=y
> > > > +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
> > >
> > > Is it a leftover?
> > >
> >
> > [Anoob]  It is similar to 'RTE_LIBRTE_ETHDEV_DEBUG' for usage in
> > datapath. Like in,
> > https://urldefense.proofpoint.com/v2/url?u=http-3A__code.dpdk.org_dpdk
> > _latest_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4378&d=DwICAg&c=n
> > KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> WYLn1v9SyTMrT5EQqh2TU&m=
> >
> STCBgRhcnCb9M6MWQL9CUszLwy2r0NJ_3m93_D5UX3g&s=HVsD0LKZ2Q6UCW
> BSRvbw9beD
> > 7OtuQyWPrRrx9eofnz8&e=
> 
> 1/ I don't see it used in this patch

[Anoob] Following snippet uses.

+#ifdef RTE_LIBRTE_SECURITY_DEBUG
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -EINVAL,
+			-ENOTSUP);
+	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
+#endif

> 2/ Adding makefile-only option is weird
> 3/ Adding new compile-time options is discouraged

[Anoob] This is only introduced for data path APIs. And the same approach is followed in eth dev as well.

Thanks,
Anoob

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Lukasz Wojciechowski]

Hi guys,

I don't know what is the current status of "legacy" build using 
gnumakes, so I added the new DEBUG flag to config just as it was done in 
other libs like eventdev.
Many guides still point config files as the one that should be changed 
in order to enable some features, so I thought I should add it there.

If I understand well the official build system now is the one based on 
using meson and ninja, however it hasn't got anything similar to the 
gnamakefiles system, e.g.
in the meson.build file for libraries all the libraries have build 
variable set to true and there are few ifs that check it, but as it's 
set to true all libraries build always.
And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
It's kind of weird.

    foreach l:libraries
    *    build = true**
    *    reason = '<unknown reason>' # set if build == false to explain why
         ...
    *    if not build*
             dpdk_libs_disabled += name
             set_variable(name.underscorify() + '_disable_reason', reason)
         else
             enabled_libs += name
    *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
         ...

Have you think about reusing config files in meson configuration and 
have a single point of configuration? Of course all meson flags can 
overwrite the default config.

Best regards
Lukasz


BTW
I got still some warnings from linters when processing Fixes flags. I 
used the fixline alias recommended on dpdk build manual, but the 
automated linter does not find the commits I mention there. What have I 
done wrong?


W dniu 08.04.2020 o 16:44, Anoob Joseph pisze:
> Hi Thomas,
>
>> ----------------------------------------------------------------------
>> 08/04/2020 15:02, Anoob Joseph:
>>> Hi Thomas,
>>>
>>>> 08/04/2020 05:13, Lukasz Wojciechowski:
>>>>> This patch adds verification of the parameters to the ret_security
>>>>> API functions. All required parameters are checked if they are not NULL.
>>>> [...]
>>>>> --- a/config/common_base
>>>>> +++ b/config/common_base
>>>>>   CONFIG_RTE_LIBRTE_SECURITY=y
>>>>> +CONFIG_RTE_LIBRTE_SECURITY_DEBUG=n
>>>> Is it a leftover?
>>>>
>>> [Anoob]  It is similar to 'RTE_LIBRTE_ETHDEV_DEBUG' for usage in
>>> datapath. Like in,
>>> https://protect2.fireeye.com/url?k=ebdcc0dd-b6100959-ebdd4b92-0cc47aa8f5ba-33f3beec7b92faf2&q=1&u=https%3A%2F%2Furldefense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttp-3A__code.dpdk.org_dpdk
>>> _latest_source_lib_librte-5Fethdev_rte-5Fethdev.h-23L4378&d=DwICAg&c=n
>>> KjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
>> WYLn1v9SyTMrT5EQqh2TU&m=
>> STCBgRhcnCb9M6MWQL9CUszLwy2r0NJ_3m93_D5UX3g&s=HVsD0LKZ2Q6UCW
>> BSRvbw9beD
>>> 7OtuQyWPrRrx9eofnz8&e=
>> 1/ I don't see it used in this patch
> [Anoob] Following snippet uses.
>
> +#ifdef RTE_LIBRTE_SECURITY_DEBUG
> +	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -EINVAL,
> +			-ENOTSUP);
> +	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> +#endif
>
>> 2/ Adding makefile-only option is weird
>> 3/ Adding new compile-time options is discouraged
> [Anoob] This is only introduced for data path APIs. And the same approach is followed in eth dev as well.
>
> Thanks,
> Anoob
>
-- 

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-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Thomas Monjalon]

08/04/2020 17:49, Lukasz Wojciechowski:
> Hi guys,
> 
> I don't know what is the current status of "legacy" build using 
> gnumakes, so I added the new DEBUG flag to config just as it was done in 
> other libs like eventdev.
> Many guides still point config files as the one that should be changed 
> in order to enable some features, so I thought I should add it there.
> 
> If I understand well the official build system now is the one based on 
> using meson and ninja, however it hasn't got anything similar to the 
> gnamakefiles system, e.g.
> in the meson.build file for libraries all the libraries have build 
> variable set to true and there are few ifs that check it, but as it's 
> set to true all libraries build always.
> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
> It's kind of weird.
> 
>     foreach l:libraries
>     *    build = true**
>     *    reason = '<unknown reason>' # set if build == false to explain why
>          ...
>     *    if not build*
>              dpdk_libs_disabled += name
>              set_variable(name.underscorify() + '_disable_reason', reason)
>          else
>              enabled_libs += name
>     *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
>          ...
> 
> Have you think about reusing config files in meson configuration and 
> have a single point of configuration? Of course all meson flags can 
> overwrite the default config.

This is on purpose.
We are removing most of compile-time options with meson.

I think we can use a global option for debug-specific code.
Bruce, what do you recommend?

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Bruce Richardson]

On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> 08/04/2020 17:49, Lukasz Wojciechowski:
> > Hi guys,
> > 
> > I don't know what is the current status of "legacy" build using 
> > gnumakes, so I added the new DEBUG flag to config just as it was done in 
> > other libs like eventdev.
> > Many guides still point config files as the one that should be changed 
> > in order to enable some features, so I thought I should add it there.
> > 
> > If I understand well the official build system now is the one based on 
> > using meson and ninja, however it hasn't got anything similar to the 
> > gnamakefiles system, e.g.
> > in the meson.build file for libraries all the libraries have build 
> > variable set to true and there are few ifs that check it, but as it's 
> > set to true all libraries build always.
> > And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
> > It's kind of weird.
> > 
> >     foreach l:libraries
> >     *    build = true**
> >     *    reason = '<unknown reason>' # set if build == false to explain why
> >          ...
> >     *    if not build*
> >              dpdk_libs_disabled += name
> >              set_variable(name.underscorify() + '_disable_reason', reason)
> >          else
> >              enabled_libs += name
> >     *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> >          ...
> > 
> > Have you think about reusing config files in meson configuration and 
> > have a single point of configuration? Of course all meson flags can 
> > overwrite the default config.
> 
> This is on purpose.
> We are removing most of compile-time options with meson.
> 
> I think we can use a global option for debug-specific code.
> Bruce, what do you recommend?
> 
Meson has a built-in global debug setting which could be used. However,
that may be too course-grained. If that is the case there are a couple of
options:

1 Each library can have it's own debug flag defined, which is set on
  the commandline in CFLAGS. Can be done right now - just reuse any of the
  debug variables in the existing make config files (stripping off the
  CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
2 Since that is perhaps not the most usable - though easiest to implement -
  we can look to add a general debug option (or couple of options) in
  meson, e.g. debug_libs=, debug_drivers=, where each option takes a list of
  libs or drivers to pass the debug flags to. This will require a little
  work in the meson build infrastructure, but is not that hard. The harder
  part is standardizing the debug flags across all components.

The advantage of #1 is that it works today and just needs some
documentation for each lib/driver what it's debug flags are. The advantage
of #2 is more usability, but it requires a lot more work to standardize
IMHO.

/Bruce

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Thomas Monjalon]

09/04/2020 12:14, Bruce Richardson:
> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> > 08/04/2020 17:49, Lukasz Wojciechowski:
> > > Hi guys,
> > > 
> > > I don't know what is the current status of "legacy" build using 
> > > gnumakes, so I added the new DEBUG flag to config just as it was done in 
> > > other libs like eventdev.
> > > Many guides still point config files as the one that should be changed 
> > > in order to enable some features, so I thought I should add it there.
> > > 
> > > If I understand well the official build system now is the one based on 
> > > using meson and ninja, however it hasn't got anything similar to the 
> > > gnamakefiles system, e.g.
> > > in the meson.build file for libraries all the libraries have build 
> > > variable set to true and there are few ifs that check it, but as it's 
> > > set to true all libraries build always.
> > > And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
> > > It's kind of weird.
> > > 
> > >     foreach l:libraries
> > >     *    build = true**
> > >     *    reason = '<unknown reason>' # set if build == false to explain why
> > >          ...
> > >     *    if not build*
> > >              dpdk_libs_disabled += name
> > >              set_variable(name.underscorify() + '_disable_reason', reason)
> > >          else
> > >              enabled_libs += name
> > >     *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> > >          ...
> > > 
> > > Have you think about reusing config files in meson configuration and 
> > > have a single point of configuration? Of course all meson flags can 
> > > overwrite the default config.
> > 
> > This is on purpose.
> > We are removing most of compile-time options with meson.
> > 
> > I think we can use a global option for debug-specific code.
> > Bruce, what do you recommend?
> > 
> Meson has a built-in global debug setting which could be used. However,
> that may be too course-grained. If that is the case there are a couple of
> options:
> 
> 1 Each library can have it's own debug flag defined, which is set on
>   the commandline in CFLAGS. Can be done right now - just reuse any of the
>   debug variables in the existing make config files (stripping off the
>   CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
> 2 Since that is perhaps not the most usable - though easiest to implement -
>   we can look to add a general debug option (or couple of options) in
>   meson, e.g. debug_libs=, debug_drivers=, where each option takes a list of
>   libs or drivers to pass the debug flags to. This will require a little
>   work in the meson build infrastructure, but is not that hard. The harder
>   part is standardizing the debug flags across all components.
> 
> The advantage of #1 is that it works today and just needs some
> documentation for each lib/driver what it's debug flags are. The advantage
> of #2 is more usability, but it requires a lot more work to standardize
> IMHO.

In this case, we need a general option as the one already provided by meson.
It means: "I am not in production, I want to see anything behaving wrong
in the datapath."
"Anything" means we don't need a per-library switch.
And for the other needs (out of fast path), we have a new function:
	rte_log_can_log(mylogtype, RTE_LOG_DEBUG)

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Bruce Richardson]

On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
> 09/04/2020 12:14, Bruce Richardson:
> > On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> > > 08/04/2020 17:49, Lukasz Wojciechowski:
> > > > Hi guys,
> > > > 
> > > > I don't know what is the current status of "legacy" build using 
> > > > gnumakes, so I added the new DEBUG flag to config just as it was done in 
> > > > other libs like eventdev.
> > > > Many guides still point config files as the one that should be changed 
> > > > in order to enable some features, so I thought I should add it there.
> > > > 
> > > > If I understand well the official build system now is the one based on 
> > > > using meson and ninja, however it hasn't got anything similar to the 
> > > > gnamakefiles system, e.g.
> > > > in the meson.build file for libraries all the libraries have build 
> > > > variable set to true and there are few ifs that check it, but as it's 
> > > > set to true all libraries build always.
> > > > And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME]. 
> > > > It's kind of weird.
> > > > 
> > > >     foreach l:libraries
> > > >     *    build = true**
> > > >     *    reason = '<unknown reason>' # set if build == false to explain why
> > > >          ...
> > > >     *    if not build*
> > > >              dpdk_libs_disabled += name
> > > >              set_variable(name.underscorify() + '_disable_reason', reason)
> > > >          else
> > > >              enabled_libs += name
> > > >     *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> > > >          ...
> > > > 
> > > > Have you think about reusing config files in meson configuration and 
> > > > have a single point of configuration? Of course all meson flags can 
> > > > overwrite the default config.
> > > 
> > > This is on purpose.
> > > We are removing most of compile-time options with meson.
> > > 
> > > I think we can use a global option for debug-specific code.
> > > Bruce, what do you recommend?
> > > 
> > Meson has a built-in global debug setting which could be used. However,
> > that may be too course-grained. If that is the case there are a couple of
> > options:
> > 
> > 1 Each library can have it's own debug flag defined, which is set on
> >   the commandline in CFLAGS. Can be done right now - just reuse any of the
> >   debug variables in the existing make config files (stripping off the
> >   CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
> > 2 Since that is perhaps not the most usable - though easiest to implement -
> >   we can look to add a general debug option (or couple of options) in
> >   meson, e.g. debug_libs=, debug_drivers=, where each option takes a list of
> >   libs or drivers to pass the debug flags to. This will require a little
> >   work in the meson build infrastructure, but is not that hard. The harder
> >   part is standardizing the debug flags across all components.
> > 
> > The advantage of #1 is that it works today and just needs some
> > documentation for each lib/driver what it's debug flags are. The advantage
> > of #2 is more usability, but it requires a lot more work to standardize
> > IMHO.
> 
> In this case, we need a general option as the one already provided by meson.
> It means: "I am not in production, I want to see anything behaving wrong
> in the datapath."
> "Anything" means we don't need a per-library switch.
> And for the other needs (out of fast path), we have a new function:
> 	rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
> 
To use the general option in meson something like below is probably all
that is needed to flag the debug build to all components:

diff --git a/config/meson.build b/config/meson.build
index 49482091d..b01cd1251 100644
--- a/config/meson.build
+++ b/config/meson.build
@@ -176,6 +176,10 @@ endif
 # add -include rte_config to cflags
 add_project_arguments('-include', 'rte_config.h', language: 'c')

+if get_option('debug')
+       add_project_arguments('-DDEBUG', language: 'c')
+endif
+
 # enable extra warnings and disable any unwanted warnings
 warning_flags = [
        # -Wall is added by meson by default, so add -Wextra only
 

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Lukasz Wojciechowski]

W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
>> 09/04/2020 12:14, Bruce Richardson:
>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
>>>>> Hi guys,
>>>>>
>>>>> I don't know what is the current status of "legacy" build using
>>>>> gnumakes, so I added the new DEBUG flag to config just as it was done in
>>>>> other libs like eventdev.
>>>>> Many guides still point config files as the one that should be changed
>>>>> in order to enable some features, so I thought I should add it there.
>>>>>
>>>>> If I understand well the official build system now is the one based on
>>>>> using meson and ninja, however it hasn't got anything similar to the
>>>>> gnamakefiles system, e.g.
>>>>> in the meson.build file for libraries all the libraries have build
>>>>> variable set to true and there are few ifs that check it, but as it's
>>>>> set to true all libraries build always.
>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
>>>>> It's kind of weird.
>>>>>
>>>>>      foreach l:libraries
>>>>>      *    build = true**
>>>>>      *    reason = '<unknown reason>' # set if build == false to explain why
>>>>>           ...
>>>>>      *    if not build*
>>>>>               dpdk_libs_disabled += name
>>>>>               set_variable(name.underscorify() + '_disable_reason', reason)
>>>>>           else
>>>>>               enabled_libs += name
>>>>>      *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
>>>>>           ...
>>>>>
>>>>> Have you think about reusing config files in meson configuration and
>>>>> have a single point of configuration? Of course all meson flags can
>>>>> overwrite the default config.
>>>> This is on purpose.
>>>> We are removing most of compile-time options with meson.
>>>>
>>>> I think we can use a global option for debug-specific code.
>>>> Bruce, what do you recommend?
>>>>
>>> Meson has a built-in global debug setting which could be used. However,
>>> that may be too course-grained. If that is the case there are a couple of
>>> options:
>>>
>>> 1 Each library can have it's own debug flag defined, which is set on
>>>    the commandline in CFLAGS. Can be done right now - just reuse any of the
>>>    debug variables in the existing make config files (stripping off the
>>>    CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
>>> 2 Since that is perhaps not the most usable - though easiest to implement -
>>>    we can look to add a general debug option (or couple of options) in
>>>    meson, e.g. debug_libs=, debug_drivers=, where each option takes a list of
>>>    libs or drivers to pass the debug flags to. This will require a little
>>>    work in the meson build infrastructure, but is not that hard. The harder
>>>    part is standardizing the debug flags across all components.
>>>
>>> The advantage of #1 is that it works today and just needs some
>>> documentation for each lib/driver what it's debug flags are. The advantage
>>> of #2 is more usability, but it requires a lot more work to standardize
>>> IMHO.
>> In this case, we need a general option as the one already provided by meson.
>> It means: "I am not in production, I want to see anything behaving wrong
>> in the datapath."
>> "Anything" means we don't need a per-library switch.
>> And for the other needs (out of fast path), we have a new function:
>> 	rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
>>
> To use the general option in meson something like below is probably all
> that is needed to flag the debug build to all components:
>
> diff --git a/config/meson.build b/config/meson.build
> index 49482091d..b01cd1251 100644
> --- a/config/meson.build
> +++ b/config/meson.build
> @@ -176,6 +176,10 @@ endif
>   # add -include rte_config to cflags
>   add_project_arguments('-include', 'rte_config.h', language: 'c')
>
> +if get_option('debug')
> +       add_project_arguments('-DDEBUG', language: 'c')
> +endif
> +

This will conflict with DEBUG define for log level.

How about adding similar define in library meson.build file? , e.g

diff --git a/lib/librte_security/meson.build 
b/lib/librte_security/meson.build
index 5679c8b5c..ee92483c5 100644
--- a/lib/librte_security/meson.build
+++ b/lib/librte_security/meson.build
@@ -4,3 +4,7 @@
  sources = files('rte_security.c')
  headers = files('rte_security.h', 'rte_security_driver.h')
  deps += ['mempool', 'cryptodev']
+
+if get_option('debug')
+ add_project_arguments('-DRTE_LIBRTE_SECURITY_DEBUG', language: 'c')
+endif


>   # enable extra warnings and disable any unwanted warnings
>   warning_flags = [
>          # -Wall is added by meson by default, so add -Wextra only
>   
>
-- 

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-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Lukasz Wojciechowski]

W dniu 09.04.2020 o 16:07, Lukasz Wojciechowski pisze:
>
> W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
>> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
>>> 09/04/2020 12:14, Bruce Richardson:
>>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
>>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
>>>>>> Hi guys,
>>>>>>
>>>>>> I don't know what is the current status of "legacy" build using
>>>>>> gnumakes, so I added the new DEBUG flag to config just as it was 
>>>>>> done in
>>>>>> other libs like eventdev.
>>>>>> Many guides still point config files as the one that should be 
>>>>>> changed
>>>>>> in order to enable some features, so I thought I should add it 
>>>>>> there.
>>>>>>
>>>>>> If I understand well the official build system now is the one 
>>>>>> based on
>>>>>> using meson and ninja, however it hasn't got anything similar to the
>>>>>> gnamakefiles system, e.g.
>>>>>> in the meson.build file for libraries all the libraries have build
>>>>>> variable set to true and there are few ifs that check it, but as 
>>>>>> it's
>>>>>> set to true all libraries build always.
>>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
>>>>>> It's kind of weird.
>>>>>>
>>>>>>      foreach l:libraries
>>>>>>      *    build = true**
>>>>>>      *    reason = '<unknown reason>' # set if build == false to 
>>>>>> explain why
>>>>>>           ...
>>>>>>      *    if not build*
>>>>>>               dpdk_libs_disabled += name
>>>>>>               set_variable(name.underscorify() + 
>>>>>> '_disable_reason', reason)
>>>>>>           else
>>>>>>               enabled_libs += name
>>>>>>      *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
>>>>>>           ...
>>>>>>
>>>>>> Have you think about reusing config files in meson configuration and
>>>>>> have a single point of configuration? Of course all meson flags can
>>>>>> overwrite the default config.
>>>>> This is on purpose.
>>>>> We are removing most of compile-time options with meson.
>>>>>
>>>>> I think we can use a global option for debug-specific code.
>>>>> Bruce, what do you recommend?
>>>>>
>>>> Meson has a built-in global debug setting which could be used. 
>>>> However,
>>>> that may be too course-grained. If that is the case there are a 
>>>> couple of
>>>> options:
>>>>
>>>> 1 Each library can have it's own debug flag defined, which is set on
>>>>    the commandline in CFLAGS. Can be done right now - just reuse 
>>>> any of the
>>>>    debug variables in the existing make config files (stripping off 
>>>> the
>>>>    CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
>>>> 2 Since that is perhaps not the most usable - though easiest to 
>>>> implement -
>>>>    we can look to add a general debug option (or couple of options) in
>>>>    meson, e.g. debug_libs=, debug_drivers=, where each option takes 
>>>> a list of
>>>>    libs or drivers to pass the debug flags to. This will require a 
>>>> little
>>>>    work in the meson build infrastructure, but is not that hard. 
>>>> The harder
>>>>    part is standardizing the debug flags across all components.
>>>>
>>>> The advantage of #1 is that it works today and just needs some
>>>> documentation for each lib/driver what it's debug flags are. The 
>>>> advantage
>>>> of #2 is more usability, but it requires a lot more work to 
>>>> standardize
>>>> IMHO.
>>> In this case, we need a general option as the one already provided 
>>> by meson.
>>> It means: "I am not in production, I want to see anything behaving 
>>> wrong
>>> in the datapath."
>>> "Anything" means we don't need a per-library switch.
>>> And for the other needs (out of fast path), we have a new function:
>>>     rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
>>>
>> To use the general option in meson something like below is probably all
>> that is needed to flag the debug build to all components:
>>
>> diff --git a/config/meson.build b/config/meson.build
>> index 49482091d..b01cd1251 100644
>> --- a/config/meson.build
>> +++ b/config/meson.build
>> @@ -176,6 +176,10 @@ endif
>>   # add -include rte_config to cflags
>>   add_project_arguments('-include', 'rte_config.h', language: 'c')
>>
>> +if get_option('debug')
>> +       add_project_arguments('-DDEBUG', language: 'c')
>> +endif
>> +
>
> This will conflict with DEBUG define for log level.
Just to be more precise, the log level is defined as RTE_LOG_DEBUG, but 
in few places you can find something like:
#define NTB_LOG(level, fmt, args...) \
     rte_log(RTE_LOG_ ## level, ntb_logtype,    "%s(): " fmt "\n", \

         __func__, ##args)

and usage like this:
NTB_LOG(DEBUG, "Link is not up.");

>
> How about adding similar define in library meson.build file? , e.g
>
> diff --git a/lib/librte_security/meson.build 
> b/lib/librte_security/meson.build
> index 5679c8b5c..ee92483c5 100644
> --- a/lib/librte_security/meson.build
> +++ b/lib/librte_security/meson.build
> @@ -4,3 +4,7 @@
>  sources = files('rte_security.c')
>  headers = files('rte_security.h', 'rte_security_driver.h')
>  deps += ['mempool', 'cryptodev']
> +
> +if get_option('debug')
> + add_project_arguments('-DRTE_LIBRTE_SECURITY_DEBUG', language: 'c')
> +endif
>
>
>>   # enable extra warnings and disable any unwanted warnings
>>   warning_flags = [
>>          # -Wall is added by meson by default, so add -Wextra only
>>
-- 

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-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Thomas Monjalon]

09/04/2020 16:21, Lukasz Wojciechowski:
> W dniu 09.04.2020 o 16:07, Lukasz Wojciechowski pisze:
> > W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
> >> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
> >>> 09/04/2020 12:14, Bruce Richardson:
> >>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> >>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
> >>>>>> Hi guys,
> >>>>>>
> >>>>>> I don't know what is the current status of "legacy" build using
> >>>>>> gnumakes, so I added the new DEBUG flag to config just as it was 
> >>>>>> done in
> >>>>>> other libs like eventdev.
> >>>>>> Many guides still point config files as the one that should be 
> >>>>>> changed
> >>>>>> in order to enable some features, so I thought I should add it 
> >>>>>> there.
> >>>>>>
> >>>>>> If I understand well the official build system now is the one 
> >>>>>> based on
> >>>>>> using meson and ninja, however it hasn't got anything similar to the
> >>>>>> gnamakefiles system, e.g.
> >>>>>> in the meson.build file for libraries all the libraries have build
> >>>>>> variable set to true and there are few ifs that check it, but as 
> >>>>>> it's
> >>>>>> set to true all libraries build always.
> >>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
> >>>>>> It's kind of weird.
> >>>>>>
> >>>>>>      foreach l:libraries
> >>>>>>      *    build = true**
> >>>>>>      *    reason = '<unknown reason>' # set if build == false to 
> >>>>>> explain why
> >>>>>>           ...
> >>>>>>      *    if not build*
> >>>>>>               dpdk_libs_disabled += name
> >>>>>>               set_variable(name.underscorify() + 
> >>>>>> '_disable_reason', reason)
> >>>>>>           else
> >>>>>>               enabled_libs += name
> >>>>>>      *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> >>>>>>           ...
> >>>>>>
> >>>>>> Have you think about reusing config files in meson configuration and
> >>>>>> have a single point of configuration? Of course all meson flags can
> >>>>>> overwrite the default config.
> >>>>> This is on purpose.
> >>>>> We are removing most of compile-time options with meson.
> >>>>>
> >>>>> I think we can use a global option for debug-specific code.
> >>>>> Bruce, what do you recommend?
> >>>>>
> >>>> Meson has a built-in global debug setting which could be used. 
> >>>> However,
> >>>> that may be too course-grained. If that is the case there are a 
> >>>> couple of
> >>>> options:
> >>>>
> >>>> 1 Each library can have it's own debug flag defined, which is set on
> >>>>    the commandline in CFLAGS. Can be done right now - just reuse 
> >>>> any of the
> >>>>    debug variables in the existing make config files (stripping off 
> >>>> the
> >>>>    CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
> >>>> 2 Since that is perhaps not the most usable - though easiest to 
> >>>> implement -
> >>>>    we can look to add a general debug option (or couple of options) in
> >>>>    meson, e.g. debug_libs=, debug_drivers=, where each option takes 
> >>>> a list of
> >>>>    libs or drivers to pass the debug flags to. This will require a 
> >>>> little
> >>>>    work in the meson build infrastructure, but is not that hard. 
> >>>> The harder
> >>>>    part is standardizing the debug flags across all components.
> >>>>
> >>>> The advantage of #1 is that it works today and just needs some
> >>>> documentation for each lib/driver what it's debug flags are. The 
> >>>> advantage
> >>>> of #2 is more usability, but it requires a lot more work to 
> >>>> standardize
> >>>> IMHO.
> >>> In this case, we need a general option as the one already provided 
> >>> by meson.
> >>> It means: "I am not in production, I want to see anything behaving 
> >>> wrong
> >>> in the datapath."
> >>> "Anything" means we don't need a per-library switch.
> >>> And for the other needs (out of fast path), we have a new function:
> >>>     rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
> >>>
> >> To use the general option in meson something like below is probably all
> >> that is needed to flag the debug build to all components:
> >>
> >> diff --git a/config/meson.build b/config/meson.build
> >> index 49482091d..b01cd1251 100644
> >> --- a/config/meson.build
> >> +++ b/config/meson.build
> >> @@ -176,6 +176,10 @@ endif
> >>   # add -include rte_config to cflags
> >>   add_project_arguments('-include', 'rte_config.h', language: 'c')
> >>
> >> +if get_option('debug')
> >> +       add_project_arguments('-DDEBUG', language: 'c')
> >> +endif
> >> +
> >
> > This will conflict with DEBUG define for log level.
> Just to be more precise, the log level is defined as RTE_LOG_DEBUG, but 
> in few places you can find something like:
> #define NTB_LOG(level, fmt, args...) \
>      rte_log(RTE_LOG_ ## level, ntb_logtype,    "%s(): " fmt "\n", \
> 
>          __func__, ##args)
> 
> and usage like this:
> NTB_LOG(DEBUG, "Link is not up.");

This is not a conflict.
The compiler sees only RTE_LOG_DEBUG.

Anyway the right name for the general flag should be RTE_DEBUG.


> > How about adding similar define in library meson.build file? , e.g
> >
> > diff --git a/lib/librte_security/meson.build 
> > b/lib/librte_security/meson.build
> > index 5679c8b5c..ee92483c5 100644
> > --- a/lib/librte_security/meson.build
> > +++ b/lib/librte_security/meson.build
> > @@ -4,3 +4,7 @@
> >  sources = files('rte_security.c')
> >  headers = files('rte_security.h', 'rte_security_driver.h')
> >  deps += ['mempool', 'cryptodev']
> > +
> > +if get_option('debug')
> > + add_project_arguments('-DRTE_LIBRTE_SECURITY_DEBUG', language: 'c')
> > +endif

It can work yes.
But it would be simpler to align on single debug flag.

Re: [dpdk-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Lukasz Wojciechowski]

W dniu 09.04.2020 o 17:22, Thomas Monjalon pisze:
> 09/04/2020 16:21, Lukasz Wojciechowski:
>> W dniu 09.04.2020 o 16:07, Lukasz Wojciechowski pisze:
>>> W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
>>>> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
>>>>> 09/04/2020 12:14, Bruce Richardson:
>>>>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
>>>>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
>>>>>>>> Hi guys,
>>>>>>>>
>>>>>>>> I don't know what is the current status of "legacy" build using
>>>>>>>> gnumakes, so I added the new DEBUG flag to config just as it was
>>>>>>>> done in
>>>>>>>> other libs like eventdev.
>>>>>>>> Many guides still point config files as the one that should be
>>>>>>>> changed
>>>>>>>> in order to enable some features, so I thought I should add it
>>>>>>>> there.
>>>>>>>>
>>>>>>>> If I understand well the official build system now is the one
>>>>>>>> based on
>>>>>>>> using meson and ninja, however it hasn't got anything similar to the
>>>>>>>> gnamakefiles system, e.g.
>>>>>>>> in the meson.build file for libraries all the libraries have build
>>>>>>>> variable set to true and there are few ifs that check it, but as
>>>>>>>> it's
>>>>>>>> set to true all libraries build always.
>>>>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
>>>>>>>> It's kind of weird.
>>>>>>>>
>>>>>>>>       foreach l:libraries
>>>>>>>>       *    build = true**
>>>>>>>>       *    reason = '<unknown reason>' # set if build == false to
>>>>>>>> explain why
>>>>>>>>            ...
>>>>>>>>       *    if not build*
>>>>>>>>                dpdk_libs_disabled += name
>>>>>>>>                set_variable(name.underscorify() +
>>>>>>>> '_disable_reason', reason)
>>>>>>>>            else
>>>>>>>>                enabled_libs += name
>>>>>>>>       *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
>>>>>>>>            ...
>>>>>>>>
>>>>>>>> Have you think about reusing config files in meson configuration and
>>>>>>>> have a single point of configuration? Of course all meson flags can
>>>>>>>> overwrite the default config.
>>>>>>> This is on purpose.
>>>>>>> We are removing most of compile-time options with meson.
>>>>>>>
>>>>>>> I think we can use a global option for debug-specific code.
>>>>>>> Bruce, what do you recommend?
>>>>>>>
>>>>>> Meson has a built-in global debug setting which could be used.
>>>>>> However,
>>>>>> that may be too course-grained. If that is the case there are a
>>>>>> couple of
>>>>>> options:
>>>>>>
>>>>>> 1 Each library can have it's own debug flag defined, which is set on
>>>>>>     the commandline in CFLAGS. Can be done right now - just reuse
>>>>>> any of the
>>>>>>     debug variables in the existing make config files (stripping off
>>>>>> the
>>>>>>     CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
>>>>>> 2 Since that is perhaps not the most usable - though easiest to
>>>>>> implement -
>>>>>>     we can look to add a general debug option (or couple of options) in
>>>>>>     meson, e.g. debug_libs=, debug_drivers=, where each option takes
>>>>>> a list of
>>>>>>     libs or drivers to pass the debug flags to. This will require a
>>>>>> little
>>>>>>     work in the meson build infrastructure, but is not that hard.
>>>>>> The harder
>>>>>>     part is standardizing the debug flags across all components.
>>>>>>
>>>>>> The advantage of #1 is that it works today and just needs some
>>>>>> documentation for each lib/driver what it's debug flags are. The
>>>>>> advantage
>>>>>> of #2 is more usability, but it requires a lot more work to
>>>>>> standardize
>>>>>> IMHO.
>>>>> In this case, we need a general option as the one already provided
>>>>> by meson.
>>>>> It means: "I am not in production, I want to see anything behaving
>>>>> wrong
>>>>> in the datapath."
>>>>> "Anything" means we don't need a per-library switch.
>>>>> And for the other needs (out of fast path), we have a new function:
>>>>>      rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
>>>>>
>>>> To use the general option in meson something like below is probably all
>>>> that is needed to flag the debug build to all components:
>>>>
>>>> diff --git a/config/meson.build b/config/meson.build
>>>> index 49482091d..b01cd1251 100644
>>>> --- a/config/meson.build
>>>> +++ b/config/meson.build
>>>> @@ -176,6 +176,10 @@ endif
>>>>    # add -include rte_config to cflags
>>>>    add_project_arguments('-include', 'rte_config.h', language: 'c')
>>>>
>>>> +if get_option('debug')
>>>> +       add_project_arguments('-DDEBUG', language: 'c')
>>>> +endif
>>>> +
>>> This will conflict with DEBUG define for log level.
>> Just to be more precise, the log level is defined as RTE_LOG_DEBUG, but
>> in few places you can find something like:
>> #define NTB_LOG(level, fmt, args...) \
>>       rte_log(RTE_LOG_ ## level, ntb_logtype,    "%s(): " fmt "\n", \
>>
>>           __func__, ##args)
>>
>> and usage like this:
>> NTB_LOG(DEBUG, "Link is not up.");
> This is not a conflict.
> The compiler sees only RTE_LOG_DEBUG.
>
> Anyway the right name for the general flag should be RTE_DEBUG.
>
Ok, I'll use RTE_DEBUG.

@Bruce Is it ok, so that I would create a patch for meson.build?
>>> How about adding similar define in library meson.build file? , e.g
>>>
>>> diff --git a/lib/librte_security/meson.build
>>> b/lib/librte_security/meson.build
>>> index 5679c8b5c..ee92483c5 100644
>>> --- a/lib/librte_security/meson.build
>>> +++ b/lib/librte_security/meson.build
>>> @@ -4,3 +4,7 @@
>>>   sources = files('rte_security.c')
>>>   headers = files('rte_security.h', 'rte_security_driver.h')
>>>   deps += ['mempool', 'cryptodev']
>>> +
>>> +if get_option('debug')
>>> + add_project_arguments('-DRTE_LIBRTE_SECURITY_DEBUG', language: 'c')
>>> +endif
> It can work yes.
> But it would be simpler to align on single debug flag.
>
In this set of patches I would only align the security and tests code to 
RTE_DEBUG.
Changes in global configuration to use a single debug flag should be 
done in separate set of patches.
>
-- 

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-stable] [EXT] Re: [dpdk-dev] [PATCH v2 01/13] security: fix verification of parameters

[Bruce Richardson]

On Thu, Apr 09, 2020 at 06:10:11PM +0200, Lukasz Wojciechowski wrote:
> 
> W dniu 09.04.2020 o 17:22, Thomas Monjalon pisze:
> > 09/04/2020 16:21, Lukasz Wojciechowski:
> >> W dniu 09.04.2020 o 16:07, Lukasz Wojciechowski pisze:
> >>> W dniu 09.04.2020 o 13:13, Bruce Richardson pisze:
> >>>> On Thu, Apr 09, 2020 at 12:54:10PM +0200, Thomas Monjalon wrote:
> >>>>> 09/04/2020 12:14, Bruce Richardson:
> >>>>>> On Wed, Apr 08, 2020 at 07:51:35PM +0200, Thomas Monjalon wrote:
> >>>>>>> 08/04/2020 17:49, Lukasz Wojciechowski:
> >>>>>>>> Hi guys,
> >>>>>>>>
> >>>>>>>> I don't know what is the current status of "legacy" build using
> >>>>>>>> gnumakes, so I added the new DEBUG flag to config just as it was
> >>>>>>>> done in
> >>>>>>>> other libs like eventdev.
> >>>>>>>> Many guides still point config files as the one that should be
> >>>>>>>> changed
> >>>>>>>> in order to enable some features, so I thought I should add it
> >>>>>>>> there.
> >>>>>>>>
> >>>>>>>> If I understand well the official build system now is the one
> >>>>>>>> based on
> >>>>>>>> using meson and ninja, however it hasn't got anything similar to the
> >>>>>>>> gnamakefiles system, e.g.
> >>>>>>>> in the meson.build file for libraries all the libraries have build
> >>>>>>>> variable set to true and there are few ifs that check it, but as
> >>>>>>>> it's
> >>>>>>>> set to true all libraries build always.
> >>>>>>>> And each library considered there defines RTE_LIBRTE_[LIBRARY_NAME].
> >>>>>>>> It's kind of weird.
> >>>>>>>>
> >>>>>>>>       foreach l:libraries
> >>>>>>>>       *    build = true**
> >>>>>>>>       *    reason = '<unknown reason>' # set if build == false to
> >>>>>>>> explain why
> >>>>>>>>            ...
> >>>>>>>>       *    if not build*
> >>>>>>>>                dpdk_libs_disabled += name
> >>>>>>>>                set_variable(name.underscorify() +
> >>>>>>>> '_disable_reason', reason)
> >>>>>>>>            else
> >>>>>>>>                enabled_libs += name
> >>>>>>>>       *dpdk_conf.set('RTE_LIBRTE_' + name.to_upper(), 1)*
> >>>>>>>>            ...
> >>>>>>>>
> >>>>>>>> Have you think about reusing config files in meson configuration and
> >>>>>>>> have a single point of configuration? Of course all meson flags can
> >>>>>>>> overwrite the default config.
> >>>>>>> This is on purpose.
> >>>>>>> We are removing most of compile-time options with meson.
> >>>>>>>
> >>>>>>> I think we can use a global option for debug-specific code.
> >>>>>>> Bruce, what do you recommend?
> >>>>>>>
> >>>>>> Meson has a built-in global debug setting which could be used.
> >>>>>> However,
> >>>>>> that may be too course-grained. If that is the case there are a
> >>>>>> couple of
> >>>>>> options:
> >>>>>>
> >>>>>> 1 Each library can have it's own debug flag defined, which is set on
> >>>>>>     the commandline in CFLAGS. Can be done right now - just reuse
> >>>>>> any of the
> >>>>>>     debug variables in the existing make config files (stripping off
> >>>>>> the
> >>>>>>     CONFIG_), e.g. CFLAGS=-DRTE_MALLOC_DEBUG
> >>>>>> 2 Since that is perhaps not the most usable - though easiest to
> >>>>>> implement -
> >>>>>>     we can look to add a general debug option (or couple of options) in
> >>>>>>     meson, e.g. debug_libs=, debug_drivers=, where each option takes
> >>>>>> a list of
> >>>>>>     libs or drivers to pass the debug flags to. This will require a
> >>>>>> little
> >>>>>>     work in the meson build infrastructure, but is not that hard.
> >>>>>> The harder
> >>>>>>     part is standardizing the debug flags across all components.
> >>>>>>
> >>>>>> The advantage of #1 is that it works today and just needs some
> >>>>>> documentation for each lib/driver what it's debug flags are. The
> >>>>>> advantage
> >>>>>> of #2 is more usability, but it requires a lot more work to
> >>>>>> standardize
> >>>>>> IMHO.
> >>>>> In this case, we need a general option as the one already provided
> >>>>> by meson.
> >>>>> It means: "I am not in production, I want to see anything behaving
> >>>>> wrong
> >>>>> in the datapath."
> >>>>> "Anything" means we don't need a per-library switch.
> >>>>> And for the other needs (out of fast path), we have a new function:
> >>>>>      rte_log_can_log(mylogtype, RTE_LOG_DEBUG)
> >>>>>
> >>>> To use the general option in meson something like below is probably all
> >>>> that is needed to flag the debug build to all components:
> >>>>
> >>>> diff --git a/config/meson.build b/config/meson.build
> >>>> index 49482091d..b01cd1251 100644
> >>>> --- a/config/meson.build
> >>>> +++ b/config/meson.build
> >>>> @@ -176,6 +176,10 @@ endif
> >>>>    # add -include rte_config to cflags
> >>>>    add_project_arguments('-include', 'rte_config.h', language: 'c')
> >>>>
> >>>> +if get_option('debug')
> >>>> +       add_project_arguments('-DDEBUG', language: 'c')
> >>>> +endif
> >>>> +
> >>> This will conflict with DEBUG define for log level.
> >> Just to be more precise, the log level is defined as RTE_LOG_DEBUG, but
> >> in few places you can find something like:
> >> #define NTB_LOG(level, fmt, args...) \
> >>       rte_log(RTE_LOG_ ## level, ntb_logtype,    "%s(): " fmt "\n", \
> >>
> >>           __func__, ##args)
> >>
> >> and usage like this:
> >> NTB_LOG(DEBUG, "Link is not up.");
> > This is not a conflict.
> > The compiler sees only RTE_LOG_DEBUG.
> >
> > Anyway the right name for the general flag should be RTE_DEBUG.
> >
> Ok, I'll use RTE_DEBUG.
> 
> @Bruce Is it ok, so that I would create a patch for meson.build?

Sure. Setting RTE_DEBUG at top level when a debug build is set is probably
a good idea.

/Bruce

[dpdk-stable] [PATCH v2 02/13] security: fix return types in documentation

[Lukasz Wojciechowski]

Enhance returned values description for rte_security_session_destroy
and some other minor description changes.

Fixes: c261d1431bd8 ("security: introduce security API and framework")
Cc: akhil.goyal@nxp.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_security/rte_security.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
index ef47118fa..747830d67 100644
--- a/lib/librte_security/rte_security.h
+++ b/lib/librte_security/rte_security.h
@@ -378,7 +378,7 @@ rte_security_session_create(struct rte_security_ctx *instance,
  * @param   conf	update configuration parameters
  * @return
  *  - On success returns 0
- *  - On failure return errno
+ *  - On failure returns a negative errno value.
  */
 __rte_experimental
 int
@@ -403,12 +403,14 @@ rte_security_session_get_size(struct rte_security_ctx *instance);
  * return it to its original mempool.
  *
  * @param   instance	security instance
- * @param   sess	security session to freed
+ * @param   sess	security session to be freed
  *
  * @return
  *  - 0 if successful.
- *  - -EINVAL if session is NULL.
+ *  - -EINVAL if session or context instance is NULL.
  *  - -EBUSY if not all device private data has been freed.
+ *  - -ENOTSUP if destroying private data is not supported.
+ *  - other negative values in case of freeing private data errors.
  */
 int
 rte_security_session_destroy(struct rte_security_ctx *instance,
-- 
2.17.1

[dpdk-stable] [PATCH v2 03/13] security: fix session counter

[Lukasz Wojciechowski]

Fix session counter to be decreased in rte_security_session_destroy
only when session was successfully destroyed.

Formerly session counter was decreased prior session destroying
and returning session object to mempool. It remained decreased even
if session was not destroyed and mempool object released making counter
invalid.

Fixes: c261d1431bd8 ("security: introduce security API and framework")
Cc: akhil.goyal@nxp.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_security/rte_security.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
index f1b4a894e..ae57d3421 100644
--- a/lib/librte_security/rte_security.c
+++ b/lib/librte_security/rte_security.c
@@ -90,14 +90,16 @@ rte_security_session_destroy(struct rte_security_ctx *instance,
 			-ENOTSUP);
 	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
 
+	ret = instance->ops->session_destroy(instance->device, sess);
+	if (ret != 0)
+		return ret;
+
+	rte_mempool_put(rte_mempool_from_obj(sess), (void *)sess);
+
 	if (instance->sess_cnt)
 		instance->sess_cnt--;
 
-	ret = instance->ops->session_destroy(instance->device, sess);
-	if (!ret)
-		rte_mempool_put(rte_mempool_from_obj(sess), (void *)sess);
-
-	return ret;
+	return 0;
 }
 
 int
-- 
2.17.1

[dpdk-stable] [PATCH v2 04/13] app/test: fix macro definition

[Lukasz Wojciechowski]

Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
as it might be already defined.

Fixes: 5afc521eac6a ("eal: add test assert macros")
Cc: pbhagavatula@caviumnetworks.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 app/test/test.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/test/test.h b/app/test/test.h
index ac0c50616..8ac581cbc 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -22,7 +22,9 @@
 # define TEST_TRACE_FAILURE(_file, _line, _func)
 #endif
 
-#define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
+#ifndef RTE_TEST_TRACE_FAILURE
+# define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
+#endif
 
 #include <rte_test.h>
 
-- 
2.17.1

Re: [dpdk-stable] [PATCH v2 04/13] app/test: fix macro definition

[Thomas Monjalon]

08/04/2020 05:13, Lukasz Wojciechowski:
> Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
> as it might be already defined.

I think it should not be defined at all.
Why not including rte_test.h?

Re: [dpdk-stable] [PATCH v2 04/13] app/test: fix macro definition

[Lukasz Wojciechowski]

Hi Thomas,

Before my patch there was just a definition:
#define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
without #ifndef condition.

It caused a build problem to me when working on security test, which 
uses both rte_test.h and test.h
As libraries should go first on the include list before local files I used:

#include <rte_test.h>
#include "test.h"

sequence, which cause obvious build error as RTE_TEST_TRACE_FAILURE was 
first defined as an empty macro inside rte_test.h, and redefinition in 
test.h caused a problem.


So I had two ways to solve the issue:
1) to wrap it with #ifndef condition and leave the definition there
2) to remove the redefinition from test.h

I've chosen the 1) solution because:
* Author of the former patch had placed the definition there for some 
purpose
* In my opinion it is better to have the definition present and pointing 
to the same macro for both RTE_TEST_TRACE_FAILURE and TEST_TRACE_FAILURE 
as it would make logs look more consistent when printing information the 
same way.

Best regards
Lukasz


W dniu 08.04.2020 o 14:53, Thomas Monjalon pisze:
> 08/04/2020 05:13, Lukasz Wojciechowski:
>> Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
>> as it might be already defined.
> I think it should not be defined at all.
> Why not including rte_test.h?
>
>
>
-- 

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-stable] [PATCH v2 04/13] app/test: fix macro definition

[Thomas Monjalon]

08/04/2020 18:15, Lukasz Wojciechowski:
> Hi Thomas,
> 
> Before my patch there was just a definition:
> #define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
> without #ifndef condition.
> 
> It caused a build problem to me when working on security test, which 
> uses both rte_test.h and test.h
> As libraries should go first on the include list before local files I used:
> 
> #include <rte_test.h>
> #include "test.h"
> 
> sequence, which cause obvious build error as RTE_TEST_TRACE_FAILURE was 
> first defined as an empty macro inside rte_test.h, and redefinition in 
> test.h caused a problem.
> 
> 
> So I had two ways to solve the issue:
> 1) to wrap it with #ifndef condition and leave the definition there
> 2) to remove the redefinition from test.h
> 
> I've chosen the 1) solution because:
> * Author of the former patch had placed the definition there for some 
> purpose

Because rte_test.h is more recent and its addition was not complete enough.
rte_test.h should be included in test.h, and overlaps removed.

> * In my opinion it is better to have the definition present and pointing 
> to the same macro for both RTE_TEST_TRACE_FAILURE and TEST_TRACE_FAILURE 
> as it would make logs look more consistent when printing information the 
> same way.

I think solution 2 is better.


PS: please avoid top-posting


> W dniu 08.04.2020 o 14:53, Thomas Monjalon pisze:
> > 08/04/2020 05:13, Lukasz Wojciechowski:
> >> Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
> >> as it might be already defined.
> > I think it should not be defined at all.
> > Why not including rte_test.h?
> >
> >
> >
> 

Re: [dpdk-stable] [PATCH v2 04/13] app/test: fix macro definition

[Lukasz Wojciechowski]

W dniu 08.04.2020 o 19:47, Thomas Monjalon pisze:
> 08/04/2020 18:15, Lukasz Wojciechowski:
>> Hi Thomas,
>>
>> Before my patch there was just a definition:
>> #define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
>> without #ifndef condition.
>>
>> It caused a build problem to me when working on security test, which
>> uses both rte_test.h and test.h
>> As libraries should go first on the include list before local files I used:
>>
>> #include <rte_test.h>
>> #include "test.h"
>>
>> sequence, which cause obvious build error as RTE_TEST_TRACE_FAILURE was
>> first defined as an empty macro inside rte_test.h, and redefinition in
>> test.h caused a problem.
>>
>>
>> So I had two ways to solve the issue:
>> 1) to wrap it with #ifndef condition and leave the definition there
>> 2) to remove the redefinition from test.h
>>
>> I've chosen the 1) solution because:
>> * Author of the former patch had placed the definition there for some
>> purpose
> Because rte_test.h is more recent and its addition was not complete enough.
> rte_test.h should be included in test.h, and overlaps removed.
>
>> * In my opinion it is better to have the definition present and pointing
>> to the same macro for both RTE_TEST_TRACE_FAILURE and TEST_TRACE_FAILURE
>> as it would make logs look more consistent when printing information the
>> same way.
> I think solution 2 is better.
Ok I'll change this patch and remove the macro definition at all from 
app/test/test.h
I'll publish it with version 3, but I'll wait a bit more for getting 
more comments on version 2
>
>
> PS: please avoid top-posting
Sorry
>
>
>> W dniu 08.04.2020 o 14:53, Thomas Monjalon pisze:
>>> 08/04/2020 05:13, Lukasz Wojciechowski:
>>>> Wrap RTE_TEST_TRACE_FAILURE macro definition into #ifndef clause
>>>> as it might be already defined.
>>> I think it should not be defined at all.
>>> Why not including rte_test.h?
>>>
>>>
>>>
>
>
>
>
>
-- 

Lukasz Wojciechowski
Principal Software Engineer

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

[dpdk-stable] [PATCH v3 01/13] security: fix verification of parameters

[Lukasz Wojciechowski]

This patch adds verification of the parameters to the ret_security API
functions. All required parameters are checked if they are not NULL.

Checks verify full chain of pointers, e.g. in case of verification of
"instance->ops->session_XXX", they check also "instance"
and "instance->ops".

Fixes: c261d1431bd8 ("security: introduce security API and framework")
Cc: akhil.goyal@nxp.com

Fixes: 1a08c379b9b5 ("security: support user data retrieval")
Cc: anoob.joseph@caviumnetworks.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_security/rte_security.c | 59 +++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 13 deletions(-)

diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
index bc81ce15d..38ccc2ea9 100644
--- a/lib/librte_security/rte_security.c
+++ b/lib/librte_security/rte_security.c
@@ -1,6 +1,7 @@
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright 2017 NXP.
  * Copyright(c) 2017 Intel Corporation.
+ * Copyright (c) 2020 Samsung Electronics Co., Ltd All Rights Reserved
  */
 
 #include <rte_malloc.h>
@@ -9,6 +10,19 @@
 #include "rte_security.h"
 #include "rte_security_driver.h"
 
+/* Macro to check for invalid pointers */
+#define RTE_PTR_OR_ERR_RET(ptr, retval) do {	\
+	if ((ptr) == NULL)			\
+		return retval;			\
+} while (0)
+
+/* Macro to check for invalid pointers chains */
+#define RTE_PTR_CHAIN3_OR_ERR_RET(p1, p2, p3, retval, last_retval) do {	\
+	RTE_PTR_OR_ERR_RET(p1, retval);					\
+	RTE_PTR_OR_ERR_RET(p1->p2, retval);				\
+	RTE_PTR_OR_ERR_RET(p1->p2->p3, last_retval);			\
+} while (0)
+
 struct rte_security_session *
 rte_security_session_create(struct rte_security_ctx *instance,
 			    struct rte_security_session_conf *conf,
@@ -16,10 +30,9 @@ rte_security_session_create(struct rte_security_ctx *instance,
 {
 	struct rte_security_session *sess = NULL;
 
-	if (conf == NULL)
-		return NULL;
-
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_create, NULL, NULL);
+	RTE_PTR_OR_ERR_RET(conf, NULL);
+	RTE_PTR_OR_ERR_RET(mp, NULL);
 
 	if (rte_mempool_get(mp, (void **)&sess))
 		return NULL;
@@ -38,14 +51,19 @@ rte_security_session_update(struct rte_security_ctx *instance,
 			    struct rte_security_session *sess,
 			    struct rte_security_session_conf *conf)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_update, -ENOTSUP);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_update, -EINVAL,
+			-ENOTSUP);
+	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
+	RTE_PTR_OR_ERR_RET(conf, -EINVAL);
+
 	return instance->ops->session_update(instance->device, sess, conf);
 }
 
 unsigned int
 rte_security_session_get_size(struct rte_security_ctx *instance)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_get_size, 0);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_get_size, 0, 0);
+
 	return instance->ops->session_get_size(instance->device);
 }
 
@@ -54,7 +72,11 @@ rte_security_session_stats_get(struct rte_security_ctx *instance,
 			       struct rte_security_session *sess,
 			       struct rte_security_stats *stats)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_stats_get, -ENOTSUP);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_stats_get, -EINVAL,
+			-ENOTSUP);
+	/* Parameter sess can be NULL in case of getting global statistics. */
+	RTE_PTR_OR_ERR_RET(stats, -EINVAL);
+
 	return instance->ops->session_stats_get(instance->device, sess, stats);
 }
 
@@ -64,7 +86,9 @@ rte_security_session_destroy(struct rte_security_ctx *instance,
 {
 	int ret;
 
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, -ENOTSUP);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, session_destroy, -EINVAL,
+			-ENOTSUP);
+	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
 
 	if (instance->sess_cnt)
 		instance->sess_cnt--;
@@ -81,7 +105,11 @@ rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
 			      struct rte_security_session *sess,
 			      struct rte_mbuf *m, void *params)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -ENOTSUP);
+#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);
 }
@@ -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;
 
@@ -101,7 +131,8 @@ rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md)
 const struct rte_security_capability *
 rte_security_capabilities_get(struct rte_security_ctx *instance)
 {
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, capabilities_get, NULL, NULL);
+
 	return instance->ops->capabilities_get(instance->device);
 }
 
@@ -113,7 +144,9 @@ rte_security_capability_get(struct rte_security_ctx *instance,
 	const struct rte_security_capability *capability;
 	uint16_t i = 0;
 
-	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
+	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, capabilities_get, NULL, NULL);
+	RTE_PTR_OR_ERR_RET(idx, NULL);
+
 	capabilities = instance->ops->capabilities_get(instance->device);
 
 	if (capabilities == NULL)
@@ -121,7 +154,7 @@ rte_security_capability_get(struct rte_security_ctx *instance,
 
 	while ((capability = &capabilities[i++])->action
 			!= RTE_SECURITY_ACTION_TYPE_NONE) {
-		if (capability->action  == idx->action &&
+		if (capability->action == idx->action &&
 				capability->protocol == idx->protocol) {
 			if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC) {
 				if (capability->ipsec.proto ==
-- 
2.17.1

Re: [dpdk-stable] [dpdk-dev] [PATCH v3 01/13] security: fix verification of parameters

[Anoob Joseph]

> This patch adds verification of the parameters to the ret_security API functions.
> All required parameters are checked if they are not NULL.
> 
> Checks verify full chain of pointers, e.g. in case of verification of "instance->ops-
> >session_XXX", they check also "instance"
> and "instance->ops".
> 
> Fixes: c261d1431bd8 ("security: introduce security API and framework")
> Cc: akhil.goyal@nxp.com
> 
> Fixes: 1a08c379b9b5 ("security: support user data retrieval")
> Cc: anoob.joseph@caviumnetworks.com
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

Acked-by: Anoob Joseph <anoobj@marvell.com>

[dpdk-stable] [PATCH v3 02/13] security: fix return types in documentation

[Lukasz Wojciechowski]

Enhance returned values description for rte_security_session_destroy
and some other minor description changes.

Fixes: c261d1431bd8 ("security: introduce security API and framework")
Cc: akhil.goyal@nxp.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_security/rte_security.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/librte_security/rte_security.h b/lib/librte_security/rte_security.h
index ef47118fa..747830d67 100644
--- a/lib/librte_security/rte_security.h
+++ b/lib/librte_security/rte_security.h
@@ -378,7 +378,7 @@ rte_security_session_create(struct rte_security_ctx *instance,
  * @param   conf	update configuration parameters
  * @return
  *  - On success returns 0
- *  - On failure return errno
+ *  - On failure returns a negative errno value.
  */
 __rte_experimental
 int
@@ -403,12 +403,14 @@ rte_security_session_get_size(struct rte_security_ctx *instance);
  * return it to its original mempool.
  *
  * @param   instance	security instance
- * @param   sess	security session to freed
+ * @param   sess	security session to be freed
  *
  * @return
  *  - 0 if successful.
- *  - -EINVAL if session is NULL.
+ *  - -EINVAL if session or context instance is NULL.
  *  - -EBUSY if not all device private data has been freed.
+ *  - -ENOTSUP if destroying private data is not supported.
+ *  - other negative values in case of freeing private data errors.
  */
 int
 rte_security_session_destroy(struct rte_security_ctx *instance,
-- 
2.17.1

Re: [dpdk-stable] [dpdk-dev] [PATCH v3 02/13] security: fix return types in documentation

[Anoob Joseph]

> Enhance returned values description for rte_security_session_destroy and some
> other minor description changes.
> 
> Fixes: c261d1431bd8 ("security: introduce security API and framework")
> Cc: akhil.goyal@nxp.com
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

Acked-by: Anoob Joseph <anoobj@marvell.com>

[dpdk-stable] [PATCH v3 03/13] security: fix session counter

[Lukasz Wojciechowski]

Fix session counter to be decreased in rte_security_session_destroy
only when session was successfully destroyed.

Formerly session counter was decreased prior session destroying
and returning session object to mempool. It remained decreased even
if session was not destroyed and mempool object released making counter
invalid.

Fixes: c261d1431bd8 ("security: introduce security API and framework")
Cc: akhil.goyal@nxp.com

Cc: stable@dpdk.org

Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
---
 lib/librte_security/rte_security.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
index 38ccc2ea9..d475b0977 100644
--- a/lib/librte_security/rte_security.c
+++ b/lib/librte_security/rte_security.c
@@ -90,14 +90,16 @@ rte_security_session_destroy(struct rte_security_ctx *instance,
 			-ENOTSUP);
 	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
 
+	ret = instance->ops->session_destroy(instance->device, sess);
+	if (ret != 0)
+		return ret;
+
+	rte_mempool_put(rte_mempool_from_obj(sess), (void *)sess);
+
 	if (instance->sess_cnt)
 		instance->sess_cnt--;
 
-	ret = instance->ops->session_destroy(instance->device, sess);
-	if (!ret)
-		rte_mempool_put(rte_mempool_from_obj(sess), (void *)sess);
-
-	return ret;
+	return 0;
 }
 
 int
-- 
2.17.1

Re: [dpdk-stable] [dpdk-dev] [PATCH v3 03/13] security: fix session counter

[Anoob Joseph]

> Fix session counter to be decreased in rte_security_session_destroy only when
> session was successfully destroyed.
> 
> Formerly session counter was decreased prior session destroying and returning
> session object to mempool. It remained decreased even if session was not
> destroyed and mempool object released making counter invalid.
> 
> Fixes: c261d1431bd8 ("security: introduce security API and framework")
> Cc: akhil.goyal@nxp.com
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>

Acked-by: Anoob Joseph <anoobj@marvell.com>

[dpdk-stable] [PATCH v3 04/13] app/test: remove macro definition

[Lukasz Wojciechowski]

Remove RTE_TEST_TRACE_FAILURE macro definition from app/test/test.h
as it might be already defined and cause build problems.

Also it is good to leave the decision of additional logs to the final
user of test.h and rte_test.h

Fixes: 5afc521eac6a ("eal: add test assert macros")
Cc: pbhagavatula@caviumnetworks.com

Cc: stable@dpdk.org

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

diff --git a/app/test/test.h b/app/test/test.h
index ac0c50616..b07f6c1ef 100644
--- a/app/test/test.h
+++ b/app/test/test.h
@@ -22,8 +22,6 @@
 # define TEST_TRACE_FAILURE(_file, _line, _func)
 #endif
 
-#define RTE_TEST_TRACE_FAILURE TEST_TRACE_FAILURE
-
 #include <rte_test.h>
 
 #define TEST_ASSERT RTE_TEST_ASSERT
-- 
2.17.1