[dpdk-dev] [PATCH] net/mlx5: fix parsing all-multicast from flow item

[dpdk-dev] [PATCH] net/mlx5: fix parsing all-multicast from flow item

[Yongseok Koh]

As the dst_mac of allmulti is already masked with the mask, it has 0x01 in
the first octet. Checking the least significant bit only can't distinguish
it from broadcast or IPv6 multicast.

Fixes: bb47fb6e6067 ("net/mlx5: fix flow type for allmulti rules")
Cc: stable@dpdk.org

Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index 305b2ec01..d01c8069b 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -1281,7 +1281,7 @@ mlx5_flow_create_eth(const struct rte_flow_item *item,
 		eth.val.ether_type &= eth.mask.ether_type;
 	}
 	mlx5_flow_create_copy(parser, &eth, eth_size);
-	parser->allmulti = eth.val.dst_mac[0] & 1;
+	parser->allmulti = eth.val.dst_mac[0] == 0x01;
 	return 0;
 }
 
-- 
2.11.0

Re: [dpdk-dev] [PATCH] net/mlx5: fix parsing all-multicast from flow item

[Nélio Laranjeiro]

On Wed, Jan 10, 2018 at 11:51:53PM -0800, Yongseok Koh wrote:
> As the dst_mac of allmulti is already masked with the mask, it has 0x01 in
> the first octet. Checking the least significant bit only can't distinguish
> it from broadcast or IPv6 multicast.
>
> Fixes: bb47fb6e6067 ("net/mlx5: fix flow type for allmulti rules")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_flow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
> index 305b2ec01..d01c8069b 100644
> --- a/drivers/net/mlx5/mlx5_flow.c
> +++ b/drivers/net/mlx5/mlx5_flow.c
> @@ -1281,7 +1281,7 @@ mlx5_flow_create_eth(const struct rte_flow_item *item,
>  		eth.val.ether_type &= eth.mask.ether_type;
>  	}
>  	mlx5_flow_create_copy(parser, &eth, eth_size);
> -	parser->allmulti = eth.val.dst_mac[0] & 1;
> +	parser->allmulti = eth.val.dst_mac[0] == 0x01;
>  	return 0;
>  }
>  
> -- 
> 2.11.0
> 

Seems you are introducing a bug, for broadcast Mac addresses, this will
not work i.e. 0xff != 0x01 but it as the multicast bit set.  From my
understanding, Verbs flow attribute must also be modified in such
situation.

Are you sure about this change?

Thanks,

-- 
Nélio Laranjeiro
6WIND

Re: [dpdk-dev] [PATCH] net/mlx5: fix parsing all-multicast from flow item

[Yongseok Koh]

> On Jan 11, 2018, at 12:20 AM, Nélio Laranjeiro <nelio.laranjeiro@6wind.com> wrote:
> 
> On Wed, Jan 10, 2018 at 11:51:53PM -0800, Yongseok Koh wrote:
>> As the dst_mac of allmulti is already masked with the mask, it has 0x01 in
>> the first octet. Checking the least significant bit only can't distinguish
>> it from broadcast or IPv6 multicast.
>> 
>> Fixes: bb47fb6e6067 ("net/mlx5: fix flow type for allmulti rules")
>> Cc: stable@dpdk.org
>> 
>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
>> ---
>> drivers/net/mlx5/mlx5_flow.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
>> index 305b2ec01..d01c8069b 100644
>> --- a/drivers/net/mlx5/mlx5_flow.c
>> +++ b/drivers/net/mlx5/mlx5_flow.c
>> @@ -1281,7 +1281,7 @@ mlx5_flow_create_eth(const struct rte_flow_item *item,
>> 		eth.val.ether_type &= eth.mask.ether_type;
>> 	}
>> 	mlx5_flow_create_copy(parser, &eth, eth_size);
>> -	parser->allmulti = eth.val.dst_mac[0] & 1;
>> +	parser->allmulti = eth.val.dst_mac[0] == 0x01;
>> 	return 0;
>> }
>> 
>> -- 
>> 2.11.0
>> 
> 
> Seems you are introducing a bug, for broadcast Mac addresses, this will
> not work i.e. 0xff != 0x01 but it as the multicast bit set.  From my
> understanding, Verbs flow attribute must also be modified in such
> situation.
> 
> Are you sure about this change?

Indeed. I was trying to fix a bug about the control flow. In
priv_dev_traffic_enable(), if VLAN filter is configured, it isn't properly set
for broadcast. Looks like code should be changed a lot regarding allmulti. And I
heard Raslan is preparing it. I'm taking back this patch so that Raslan include
the fix in his patch set.

Thanks,
Yongseok