[dpdk-dev] [PATCH] app/testpmd: fix flow list command

[dpdk-dev] [PATCH] app/testpmd: fix flow list command

[John Daley]

This patch fixes the 'flow list <port id>' command which caused a
segfault when passing the action or item 'type' field instead
of the action or item struct pointer in the call to rte_flow_conv.

Fixes: 7d94dcedf7ce ("app/testpmd: rely on flow API conversion function")

Signed-off-by: John Daley <johndale@cisco.com>
---
 app/test-pmd/config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 86c205806..2ce40f3e1 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1354,7 +1354,7 @@ port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])
 		while (item->type != RTE_FLOW_ITEM_TYPE_END) {
 			if (rte_flow_conv(RTE_FLOW_CONV_OP_ITEM_NAME_PTR,
 					  &name, sizeof(name),
-					  (void *)(uintptr_t)item->type,
+					  (void *)(uintptr_t)item,
 					  NULL) <= 0)
 				name = "[UNKNOWN]";
 			if (item->type != RTE_FLOW_ITEM_TYPE_VOID)
@@ -1365,7 +1365,7 @@ port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])
 		while (action->type != RTE_FLOW_ACTION_TYPE_END) {
 			if (rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
 					  &name, sizeof(name),
-					  (void *)(uintptr_t)action->type,
+					  (void *)(uintptr_t)action,
 					  NULL) <= 0)
 				name = "[UNKNOWN]";
 			if (action->type != RTE_FLOW_ACTION_TYPE_VOID)
-- 
2.16.2

Re: [dpdk-dev] [PATCH] app/testpmd: fix flow list command

[Adrien Mazarguil]

Hi John

On Tue, Oct 09, 2018 at 03:51:24PM -0700, John Daley wrote:
> This patch fixes the 'flow list <port id>' command which caused a
> segfault when passing the action or item 'type' field instead
> of the action or item struct pointer in the call to rte_flow_conv.
> 
> Fixes: 7d94dcedf7ce ("app/testpmd: rely on flow API conversion function")
> 
> Signed-off-by: John Daley <johndale@cisco.com>

That bug was introduced by a broken fix, it wasn't present in the original
patch, please see yesterday's discussion [1].

RTE_FLOW_CONV_OP_(ITEM|ACTION)_NAME[_PTR] operations are documented as using
an integer type (enum rte_flow_item_type) cast as (void *) for src because
they convert item/action *types* to corresponding strings, i.e. no need to
allocate temporary items/actions just to retrieve their names. I thought it
would be more versatile and efficient that way.

[1] "ethdev: fix flow API item/action name conversion"
    https://mails.dpdk.org/archives/dev/2018-October/115054.html

> ---
>  app/test-pmd/config.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 86c205806..2ce40f3e1 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1354,7 +1354,7 @@ port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])
>  		while (item->type != RTE_FLOW_ITEM_TYPE_END) {
>  			if (rte_flow_conv(RTE_FLOW_CONV_OP_ITEM_NAME_PTR,
>  					  &name, sizeof(name),
> -					  (void *)(uintptr_t)item->type,
> +					  (void *)(uintptr_t)item,

Also, while it does work because type is the first field, it should have
read "&item->type" for correctness. Anyway this patch shouldn't be needed
assuming the broken fix is reverted.

>  					  NULL) <= 0)
>  				name = "[UNKNOWN]";
>  			if (item->type != RTE_FLOW_ITEM_TYPE_VOID)
> @@ -1365,7 +1365,7 @@ port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])
>  		while (action->type != RTE_FLOW_ACTION_TYPE_END) {
>  			if (rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
>  					  &name, sizeof(name),
> -					  (void *)(uintptr_t)action->type,
> +					  (void *)(uintptr_t)action,

Ditto.

>  					  NULL) <= 0)
>  				name = "[UNKNOWN]";
>  			if (action->type != RTE_FLOW_ACTION_TYPE_VOID)
> -- 
> 2.16.2
> 

Thanks.

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: fix flow list command

[Mordechay Haimovsky]

Hi Adrien,
 You are correct, the bug is not where we thought it is, moreover the fix breaks
the CLI and should be rejected.
We investigated more and found that the bug is in the port_flow_query routine
Which passes an incorrect argument to rte_flow_conv as follows:
	     ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
					         &name, sizeof(name), action, &error);
While it should pass onlt the action type as follows:
	ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
			    &name, sizeof(name),
			    (void *)(uintptr_t)action->type, &error);
As done in port_flow_list routine (which works)
	if (rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
						  &name, sizeof(name),
						  (void *)(uintptr_t)action->type,
						  NULL) <= 0)
And according to the  parameters description of rte_flow_conv_name (called by  rte_flow_conv):
	* @param[in] src
 	*   Depending on @p is_action, source pattern item or action type cast as a
	 *   pointer.

Modifying port_flow_query accordingly solves the issue.
I will issue a new patch tonight.

Moti

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrien Mazarguil
> Sent: Wednesday, October 10, 2018 11:38 AM
> To: John Daley <johndale@cisco.com>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix flow list command
> 
> Hi John
> 
> On Tue, Oct 09, 2018 at 03:51:24PM -0700, John Daley wrote:
> > This patch fixes the 'flow list <port id>' command which caused a
> > segfault when passing the action or item 'type' field instead of the
> > action or item struct pointer in the call to rte_flow_conv.
> >
> > Fixes: 7d94dcedf7ce ("app/testpmd: rely on flow API conversion
> > function")
> >
> > Signed-off-by: John Daley <johndale@cisco.com>
> 
> That bug was introduced by a broken fix, it wasn't present in the original
> patch, please see yesterday's discussion [1].
> 
> RTE_FLOW_CONV_OP_(ITEM|ACTION)_NAME[_PTR] operations are
> documented as using an integer type (enum rte_flow_item_type) cast as
> (void *) for src because they convert item/action *types* to corresponding
> strings, i.e. no need to allocate temporary items/actions just to retrieve their
> names. I thought it would be more versatile and efficient that way.
> 
> [1] "ethdev: fix flow API item/action name conversion"
> 
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fma
> ils.dpdk.org%2Farchives%2Fdev%2F2018-
> October%2F115054.html&amp;data=02%7C01%7Cmotih%40mellanox.com%7
> C76b627327b134664eabe08d62e8bba68%7Ca652971c7d2e4d9ba6a4d149256f
> 461b%7C0%7C0%7C636747575020110564&amp;sdata=nRUfdIhoS%2F2Rm76y
> osDvBlttJlvcGJDB1egot8pql%2FY%3D&amp;reserved=0
> 
> > ---
> >  app/test-pmd/config.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> > 86c205806..2ce40f3e1 100644
> > --- a/app/test-pmd/config.c
> > +++ b/app/test-pmd/config.c
> > @@ -1354,7 +1354,7 @@ port_flow_list(portid_t port_id, uint32_t n, const
> uint32_t group[n])
> >  		while (item->type != RTE_FLOW_ITEM_TYPE_END) {
> >  			if
> (rte_flow_conv(RTE_FLOW_CONV_OP_ITEM_NAME_PTR,
> >  					  &name, sizeof(name),
> > -					  (void *)(uintptr_t)item->type,
> > +					  (void *)(uintptr_t)item,
> 
> Also, while it does work because type is the first field, it should have read
> "&item->type" for correctness. Anyway this patch shouldn't be needed
> assuming the broken fix is reverted.
> 
> >  					  NULL) <= 0)
> >  				name = "[UNKNOWN]";
> >  			if (item->type != RTE_FLOW_ITEM_TYPE_VOID) @@
> -1365,7 +1365,7 @@
> > port_flow_list(portid_t port_id, uint32_t n, const uint32_t group[n])
> >  		while (action->type != RTE_FLOW_ACTION_TYPE_END) {
> >  			if
> (rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
> >  					  &name, sizeof(name),
> > -					  (void *)(uintptr_t)action->type,
> > +					  (void *)(uintptr_t)action,
> 
> Ditto.
> 
> >  					  NULL) <= 0)
> >  				name = "[UNKNOWN]";
> >  			if (action->type != RTE_FLOW_ACTION_TYPE_VOID)
> > --
> > 2.16.2
> >
> 
> Thanks.
> 
> --
> Adrien Mazarguil
> 6WIND

Re: [dpdk-dev] [PATCH] app/testpmd: fix flow list command

[Adrien Mazarguil]

On Wed, Oct 10, 2018 at 04:27:59PM +0000, Mordechay Haimovsky wrote:
> Hi Adrien,
>  You are correct, the bug is not where we thought it is, moreover the fix breaks
> the CLI and should be rejected.
> We investigated more and found that the bug is in the port_flow_query routine
> Which passes an incorrect argument to rte_flow_conv as follows:
> 	     ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
> 					         &name, sizeof(name), action, &error);
> While it should pass onlt the action type as follows:
> 	ret = rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
> 			    &name, sizeof(name),
> 			    (void *)(uintptr_t)action->type, &error);
> As done in port_flow_list routine (which works)
> 	if (rte_flow_conv(RTE_FLOW_CONV_OP_ACTION_NAME_PTR,
> 						  &name, sizeof(name),
> 						  (void *)(uintptr_t)action->type,
> 						  NULL) <= 0)
> And according to the  parameters description of rte_flow_conv_name (called by  rte_flow_conv):
> 	* @param[in] src
>  	*   Depending on @p is_action, source pattern item or action type cast as a
> 	 *   pointer.
> 
> Modifying port_flow_query accordingly solves the issue.
> I will issue a new patch tonight.

Indeed, I confirm this is the right bug to address (and seems like I did not
validate rte_flow_query() properly.) Thanks for taking care of it! 

-- 
Adrien Mazarguil
6WIND