diff mbox series

[v1] ethdev: fix flow API item/action name conversion

Message ID 1538929311-31815-1-git-send-email-motih@mellanox.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers
Series [v1] ethdev: fix flow API item/action name conversion |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK

Commit Message

Moti Haimovsky Oct. 7, 2018, 4:22 p.m. UTC 
  This patch fixes a typecast bug found in rte_flow_conv_name routine
used in rte_flow item/action name conversion.

Fixes: 0c2640cbfa7a ("ethdev: add flow API item/action name conversion")

Signed-off-by: Moti Haimovsky <motih@mellanox.com>
---
v1:
 Fixed wrong hash number in "Fixes" message.
--- 
 lib/librte_ethdev/rte_flow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ori Kam Oct. 7, 2018, 4:31 p.m. UTC | #1 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Mordechay Haimovsky
> Sent: Sunday, October 7, 2018 7:22 PM
> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> <shahafs@mellanox.com>; orika@contextream.com
> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>
> Subject: [dpdk-dev] [PATCH v1] ethdev: fix flow API item/action name
> conversion
> 
> This patch fixes a typecast bug found in rte_flow_conv_name routine
> used in rte_flow item/action name conversion.
> 
> Fixes: 0c2640cbfa7a ("ethdev: add flow API item/action name conversion")
> 
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> ---
> v1:
>  Fixed wrong hash number in "Fixes" message.
> ---
>  lib/librte_ethdev/rte_flow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index 9c56a97..21a4286 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -767,7 +767,7 @@ enum rte_flow_conv_item_spec_type {
>  		{ rte_flow_desc_action, RTE_DIM(rte_flow_desc_action), },
>  	};
>  	const struct desc_info *const info = &info_rep[!!is_action];
> -	unsigned int type = (uintptr_t)src;
> +	unsigned int type = *(const unsigned int *)src;
> 
>  	if (type >= info->num)
>  		return rte_flow_error_set
> --
> 1.8.3.1
Acked-by: Ori Kam <orika@mellanox.com>

Thanks,
Ori
Ferruh Yigit Oct. 9, 2018, 1:21 p.m. UTC | #2 
On 10/7/2018 5:31 PM, Ori Kam wrote:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Mordechay Haimovsky
>> Sent: Sunday, October 7, 2018 7:22 PM
>> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
>> <shahafs@mellanox.com>; orika@contextream.com
>> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>
>> Subject: [dpdk-dev] [PATCH v1] ethdev: fix flow API item/action name
>> conversion
>>
>> This patch fixes a typecast bug found in rte_flow_conv_name routine
>> used in rte_flow item/action name conversion.
>>
>> Fixes: 0c2640cbfa7a ("ethdev: add flow API item/action name conversion")
>>
>> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
<...>
> Acked-by: Ori Kam <orika@mellanox.com>

Series applied to dpdk-next-net/master, thanks.

(please confirm latest next-net head)
Ferruh Yigit Oct. 9, 2018, 1:38 p.m. UTC | #3 
On 10/9/2018 2:21 PM, Ferruh Yigit wrote:
> On 10/7/2018 5:31 PM, Ori Kam wrote:
>>
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Mordechay Haimovsky
>>> Sent: Sunday, October 7, 2018 7:22 PM
>>> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
>>> <shahafs@mellanox.com>; orika@contextream.com
>>> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>
>>> Subject: [dpdk-dev] [PATCH v1] ethdev: fix flow API item/action name
>>> conversion
>>>
>>> This patch fixes a typecast bug found in rte_flow_conv_name routine
>>> used in rte_flow item/action name conversion.
>>>
>>> Fixes: 0c2640cbfa7a ("ethdev: add flow API item/action name conversion")
>>>
>>> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> <...>
>> Acked-by: Ori Kam <orika@mellanox.com>
> 
> Series applied to dpdk-next-net/master, thanks.
> 

Squashed into relevant commit in next-net, thanks.

> (please confirm latest next-net head)
>
Adrien Mazarguil Oct. 9, 2018, 1:54 p.m. UTC | #4 
Hi,

Jumping in although I cannot spend much time on rte_flow at the moment,
please see below.

On Tue, Oct 09, 2018 at 02:21:23PM +0100, Ferruh Yigit wrote:
> On 10/7/2018 5:31 PM, Ori Kam wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Mordechay Haimovsky
> >> Sent: Sunday, October 7, 2018 7:22 PM
> >> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> >> <shahafs@mellanox.com>; orika@contextream.com
> >> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>
> >> Subject: [dpdk-dev] [PATCH v1] ethdev: fix flow API item/action name
> >> conversion
> >>
> >> This patch fixes a typecast bug found in rte_flow_conv_name routine
> >> used in rte_flow item/action name conversion.
> >>
> >> Fixes: 0c2640cbfa7a ("ethdev: add flow API item/action name conversion")
> >>
> >> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> <...>
> > Acked-by: Ori Kam <orika@mellanox.com>
> 
> Series applied to dpdk-next-net/master, thanks.
> 
> (please confirm latest next-net head)

Please revert, it breaks something that didn't need to be fixed. I don't
think this patch was validated properly.

As documented in RTE_FLOW_CONV_OP_ITEM_NAME, RTE_FLOW_CONV_OP_ACTION_NAME,
RTE_FLOW_CONV_OP_ITEM_NAME_PTR and RTE_FLOW_CONV_OP_ACTION_NAME_PTR:

 @p src type:
   @code (const void *)enum rte_flow_item_type @endcode

With the following reminder in rte_flow_conv_name()'s Doxygen documentation:

 @param[in] src
   Depending on @p is_action, source pattern item or action type cast as a
   pointer.

Hence the original conversion results in the expected behavior while this
one is almost guaranteed to trigger a segfault:

 -       unsigned int type = (uintptr_t)src;
 +       unsigned int type = *(const unsigned int *)src;

This can be validated with testpmd. See what happens with "flow list".
Ferruh Yigit Oct. 11, 2018, 10:14 a.m. UTC | #5 
On 10/9/2018 2:54 PM, Adrien Mazarguil wrote:
> Hi,
> 
> Jumping in although I cannot spend much time on rte_flow at the moment,
> please see below.
> 
> On Tue, Oct 09, 2018 at 02:21:23PM +0100, Ferruh Yigit wrote:
>> On 10/7/2018 5:31 PM, Ori Kam wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Mordechay Haimovsky
>>>> Sent: Sunday, October 7, 2018 7:22 PM
>>>> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
>>>> <shahafs@mellanox.com>; orika@contextream.com
>>>> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>
>>>> Subject: [dpdk-dev] [PATCH v1] ethdev: fix flow API item/action name
>>>> conversion
>>>>
>>>> This patch fixes a typecast bug found in rte_flow_conv_name routine
>>>> used in rte_flow item/action name conversion.
>>>>
>>>> Fixes: 0c2640cbfa7a ("ethdev: add flow API item/action name conversion")
>>>>
>>>> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
>> <...>
>>> Acked-by: Ori Kam <orika@mellanox.com>
>>
>> Series applied to dpdk-next-net/master, thanks.
>>
>> (please confirm latest next-net head)
> 
> Please revert, it breaks something that didn't need to be fixed. I don't
> think this patch was validated properly.
> 
> As documented in RTE_FLOW_CONV_OP_ITEM_NAME, RTE_FLOW_CONV_OP_ACTION_NAME,
> RTE_FLOW_CONV_OP_ITEM_NAME_PTR and RTE_FLOW_CONV_OP_ACTION_NAME_PTR:
> 
>  @p src type:
>    @code (const void *)enum rte_flow_item_type @endcode
> 
> With the following reminder in rte_flow_conv_name()'s Doxygen documentation:
> 
>  @param[in] src
>    Depending on @p is_action, source pattern item or action type cast as a
>    pointer.
> 
> Hence the original conversion results in the expected behavior while this
> one is almost guaranteed to trigger a segfault:
> 
>  -       unsigned int type = (uintptr_t)src;
>  +       unsigned int type = *(const unsigned int *)src;
> 
> This can be validated with testpmd. See what happens with "flow list".

Thanks Adrien, patch has been reverted on next-net.
Moti Haimovsky Oct. 11, 2018, 10:36 a.m. UTC | #6 
Hi,
 Thanks for reverting https://patches.dpdk.org/patch/46221/ , It was a bad fix in the wrong place. 

Moti H.

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Thursday, October 11, 2018 1:14 PM
> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Cc: Ori Kam <orika@mellanox.com>; Mordechay Haimovsky
> <motih@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>;
> orika@contextream.com; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1] ethdev: fix flow API item/action name
> conversion
> 
> On 10/9/2018 2:54 PM, Adrien Mazarguil wrote:
> > Hi,
> >
> > Jumping in although I cannot spend much time on rte_flow at the
> > moment, please see below.
> >
> > On Tue, Oct 09, 2018 at 02:21:23PM +0100, Ferruh Yigit wrote:
> >> On 10/7/2018 5:31 PM, Ori Kam wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Mordechay
> Haimovsky
> >>>> Sent: Sunday, October 7, 2018 7:22 PM
> >>>> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Shahaf Shuler
> >>>> <shahafs@mellanox.com>; orika@contextream.com
> >>>> Cc: dev@dpdk.org; Mordechay Haimovsky <motih@mellanox.com>
> >>>> Subject: [dpdk-dev] [PATCH v1] ethdev: fix flow API item/action
> >>>> name conversion
> >>>>
> >>>> This patch fixes a typecast bug found in rte_flow_conv_name routine
> >>>> used in rte_flow item/action name conversion.
> >>>>
> >>>> Fixes: 0c2640cbfa7a ("ethdev: add flow API item/action name
> >>>> conversion")
> >>>>
> >>>> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> >> <...>
> >>> Acked-by: Ori Kam <orika@mellanox.com>
> >>
> >> Series applied to dpdk-next-net/master, thanks.
> >>
> >> (please confirm latest next-net head)
> >
> > Please revert, it breaks something that didn't need to be fixed. I
> > don't think this patch was validated properly.
> >
> > As documented in RTE_FLOW_CONV_OP_ITEM_NAME,
> > RTE_FLOW_CONV_OP_ACTION_NAME,
> RTE_FLOW_CONV_OP_ITEM_NAME_PTR and
> RTE_FLOW_CONV_OP_ACTION_NAME_PTR:
> >
> >  @p src type:
> >    @code (const void *)enum rte_flow_item_type @endcode
> >
> > With the following reminder in rte_flow_conv_name()'s Doxygen
> documentation:
> >
> >  @param[in] src
> >    Depending on @p is_action, source pattern item or action type cast as a
> >    pointer.
> >
> > Hence the original conversion results in the expected behavior while
> > this one is almost guaranteed to trigger a segfault:
> >
> >  -       unsigned int type = (uintptr_t)src;
> >  +       unsigned int type = *(const unsigned int *)src;
> >
> > This can be validated with testpmd. See what happens with "flow list".
> 
> Thanks Adrien, patch has been reverted on next-net.
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
index 9c56a97..21a4286 100644
--- a/lib/librte_ethdev/rte_flow.c
+++ b/lib/librte_ethdev/rte_flow.c
@@ -767,7 +767,7 @@  enum rte_flow_conv_item_spec_type {
 		{ rte_flow_desc_action, RTE_DIM(rte_flow_desc_action), },
 	};
 	const struct desc_info *const info = &info_rep[!!is_action];
-	unsigned int type = (uintptr_t)src;
+	unsigned int type = *(const unsigned int *)src;
 
 	if (type >= info->num)
 		return rte_flow_error_set