* [PATCH] app/testpmd: fix flow list command
@ 2018-10-09 22:51 John Daley
2018-10-10 8:37 ` Adrien Mazarguil
0 siblings, 1 reply; 4+ messages in thread
From: John Daley @ 2018-10-09 22:51 UTC (permalink / raw)
To: adrien.mazarguil; +Cc: dev, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] app/testpmd: fix flow list command 2018-10-09 22:51 [PATCH] app/testpmd: fix flow list command John Daley @ 2018-10-10 8:37 ` Adrien Mazarguil 2018-10-10 16:27 ` Mordechay Haimovsky 0 siblings, 1 reply; 4+ messages in thread From: Adrien Mazarguil @ 2018-10-10 8:37 UTC (permalink / raw) To: John Daley; +Cc: dev, Ferruh Yigit 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] app/testpmd: fix flow list command 2018-10-10 8:37 ` Adrien Mazarguil @ 2018-10-10 16:27 ` Mordechay Haimovsky 2018-10-10 16:35 ` Adrien Mazarguil 0 siblings, 1 reply; 4+ messages in thread From: Mordechay Haimovsky @ 2018-10-10 16:27 UTC (permalink / raw) To: Adrien Mazarguil, John Daley; +Cc: dev, Ferruh Yigit 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&data=02%7C01%7Cmotih%40mellanox.com%7 > C76b627327b134664eabe08d62e8bba68%7Ca652971c7d2e4d9ba6a4d149256f > 461b%7C0%7C0%7C636747575020110564&sdata=nRUfdIhoS%2F2Rm76y > osDvBlttJlvcGJDB1egot8pql%2FY%3D&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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] app/testpmd: fix flow list command 2018-10-10 16:27 ` Mordechay Haimovsky @ 2018-10-10 16:35 ` Adrien Mazarguil 0 siblings, 0 replies; 4+ messages in thread From: Adrien Mazarguil @ 2018-10-10 16:35 UTC (permalink / raw) To: Mordechay Haimovsky; +Cc: John Daley, dev, Ferruh Yigit 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-10-10 16:36 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-09 22:51 [PATCH] app/testpmd: fix flow list command John Daley 2018-10-10 8:37 ` Adrien Mazarguil 2018-10-10 16:27 ` Mordechay Haimovsky 2018-10-10 16:35 ` Adrien Mazarguil
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.