[dpdk-dev] [PATCH] common/mlx5: fix bogus assert

[dpdk-dev] [PATCH] common/mlx5: fix bogus assert

[Stephen Hemminger]

The MLX5 device supports up to 256 MAC addresses.
The code flushes all MAC devices.

If DPDK is compiled with MLX5_DEBUG this would an assert.
PANIC in mlx5_nl_mac_addr_flush():
line 775	assert "(size_t)(i) < sizeof(mac_own) * 8" failed

The root cause is that mac_own is a pointer and is being used as
a bitmap array. The sizeof(mac_own) would therfore be 64 but the
number of entries to be flushed would be 256.

There is a whole set of asserts in MLX5 netlink code with
the same bug; that should just be changed into proper error checks.

Fixes: 8e46d4e18f09 ("common/mlx5: improve assert control")
Cc: akozyrev@mellanox.com

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/common/mlx5/mlx5_nl.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_nl.c b/drivers/common/mlx5/mlx5_nl.c
index 549e787b04bf..69f5efa50aa8 100644
--- a/drivers/common/mlx5/mlx5_nl.c
+++ b/drivers/common/mlx5/mlx5_nl.c
@@ -671,7 +671,9 @@ mlx5_nl_mac_addr_add(int nlsk_fd, unsigned int iface_idx,
 
 	ret = mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 1);
 	if (!ret) {
-		MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
+		if (index >= MLX5_MAX_MAC_ADDRESSES)
+			return -EINVAL;
+
 		BITFIELD_SET(mac_own, index);
 	}
 	if (ret == -EEXIST)
@@ -700,7 +702,9 @@ int
 mlx5_nl_mac_addr_remove(int nlsk_fd, unsigned int iface_idx, uint64_t *mac_own,
 			struct rte_ether_addr *mac, uint32_t index)
 {
-	MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
+	if (index >= MLX5_MAX_MAC_ADDRESSES)
+		return -EINVAL;
+
 	BITFIELD_RESET(mac_own, index);
 	return mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 0);
 }
@@ -769,10 +773,12 @@ mlx5_nl_mac_addr_flush(int nlsk_fd, unsigned int iface_idx,
 {
 	int i;
 
+	if (n <= 0 || n >= MLX5_MAX_MAC_ADDRESSES)
+		return;
+
 	for (i = n - 1; i >= 0; --i) {
 		struct rte_ether_addr *m = &mac_addrs[i];
 
-		MLX5_ASSERT((size_t)(i) < sizeof(mac_own) * CHAR_BIT);
 		if (BITFIELD_ISSET(mac_own, i))
 			mlx5_nl_mac_addr_remove(nlsk_fd, iface_idx, mac_own, m,
 						i);
-- 
2.20.1

Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert

[Slava Ovsiienko]

Hi, Stephen

Thank you for the fix.

The exposed API to set MAC addresses:
- mlx5_mac_addr_set (invoked by rte_mac_addr_set ())
- mlx5_set_mc_addr_list (invoked by rte_eth_dev_set_mc_addr_list())

Both routines call mlx5_internal_mac_addr_add(), it in its turn calls
mlx5_nl_mac_addr_add() (that is subject of the patch).

mlx5_nl_mac_addr_add is internal function, not exposed external API,
the wrong parameter means the critical internal bug, so assert looks to be relevant here.
I would not remove MLX5_ASSERT at all but fix just it. 
Adding the parameter check and return an error is nice.
What do you think?

With best regards, Slava

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, March 31, 2020 9:03
> To: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Stephen Hemminger <stephen@networkplumber.org>;
> Alexander Kozyrev <akozyrev@mellanox.com>
> Subject: [PATCH] common/mlx5: fix bogus assert
> 
> The MLX5 device supports up to 256 MAC addresses.
> The code flushes all MAC devices.
> 
> If DPDK is compiled with MLX5_DEBUG this would an assert.
> PANIC in mlx5_nl_mac_addr_flush():
> line 775	assert "(size_t)(i) < sizeof(mac_own) * 8" failed
> 
> The root cause is that mac_own is a pointer and is being used as a bitmap
> array. The sizeof(mac_own) would therfore be 64 but the number of entries
> to be flushed would be 256.
> 
> There is a whole set of asserts in MLX5 netlink code with the same bug; that
> should just be changed into proper error checks.
> 
> Fixes: 8e46d4e18f09 ("common/mlx5: improve assert control")
> Cc: akozyrev@mellanox.com
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/common/mlx5/mlx5_nl.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_nl.c
> b/drivers/common/mlx5/mlx5_nl.c index 549e787b04bf..69f5efa50aa8 100644
> --- a/drivers/common/mlx5/mlx5_nl.c
> +++ b/drivers/common/mlx5/mlx5_nl.c
> @@ -671,7 +671,9 @@ mlx5_nl_mac_addr_add(int nlsk_fd, unsigned int
> iface_idx,
> 
>  	ret = mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 1);
>  	if (!ret) {
> -		MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
> +		if (index >= MLX5_MAX_MAC_ADDRESSES)
> +			return -EINVAL;
> +
>  		BITFIELD_SET(mac_own, index);
>  	}
>  	if (ret == -EEXIST)
> @@ -700,7 +702,9 @@ int
>  mlx5_nl_mac_addr_remove(int nlsk_fd, unsigned int iface_idx, uint64_t
> *mac_own,
>  			struct rte_ether_addr *mac, uint32_t index)  {
> -	MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
> +	if (index >= MLX5_MAX_MAC_ADDRESSES)
> +		return -EINVAL;
> +
>  	BITFIELD_RESET(mac_own, index);
>  	return mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 0);  } @@ -
> 769,10 +773,12 @@ mlx5_nl_mac_addr_flush(int nlsk_fd, unsigned int
> iface_idx,  {
>  	int i;
> 
> +	if (n <= 0 || n >= MLX5_MAX_MAC_ADDRESSES)
> +		return;
> +
>  	for (i = n - 1; i >= 0; --i) {
>  		struct rte_ether_addr *m = &mac_addrs[i];
> 
> -		MLX5_ASSERT((size_t)(i) < sizeof(mac_own) * CHAR_BIT);
>  		if (BITFIELD_ISSET(mac_own, i))
>  			mlx5_nl_mac_addr_remove(nlsk_fd, iface_idx,
> mac_own, m,
>  						i);
> --
> 2.20.1

Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert

[Stephen Hemminger]

On Tue, 31 Mar 2020 07:31:48 +0000
Slava Ovsiienko <viacheslavo@mellanox.com> wrote:

> Hi, Stephen
> 
> Thank you for the fix.
> 
> The exposed API to set MAC addresses:
> - mlx5_mac_addr_set (invoked by rte_mac_addr_set ())
> - mlx5_set_mc_addr_list (invoked by rte_eth_dev_set_mc_addr_list())
> 
> Both routines call mlx5_internal_mac_addr_add(), it in its turn calls
> mlx5_nl_mac_addr_add() (that is subject of the patch).
> 
> mlx5_nl_mac_addr_add is internal function, not exposed external API,
> the wrong parameter means the critical internal bug, so assert looks to be relevant here.
> I would not remove MLX5_ASSERT at all but fix just it. 
> Adding the parameter check and return an error is nice.
> What do you think?
> 
> With best regards, Slava

The real root cause is that sizeof(mac_own) is the wrong thing
to do. The error handling is up to you.

Since ASSERT's are compiled out they are never tested and are actually
making code less safe.

Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert

[Slava Ovsiienko]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, March 31, 2020 17:55
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; dev@dpdk.org; Alexander Kozyrev
> <akozyrev@mellanox.com>
> Subject: Re: [PATCH] common/mlx5: fix bogus assert
> 
> On Tue, 31 Mar 2020 07:31:48 +0000
> Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > Hi, Stephen
> >
> > Thank you for the fix.
> >
> > The exposed API to set MAC addresses:
> > - mlx5_mac_addr_set (invoked by rte_mac_addr_set ())
> > - mlx5_set_mc_addr_list (invoked by rte_eth_dev_set_mc_addr_list())
> >
> > Both routines call mlx5_internal_mac_addr_add(), it in its turn calls
> > mlx5_nl_mac_addr_add() (that is subject of the patch).
> >
> > mlx5_nl_mac_addr_add is internal function, not exposed external API,
> > the wrong parameter means the critical internal bug, so assert looks to be
> relevant here.
> > I would not remove MLX5_ASSERT at all but fix just it.
> > Adding the parameter check and return an error is nice.
> > What do you think?
> >
> > With best regards, Slava
> 
> The real root cause is that sizeof(mac_own) is the wrong thing to do. The
> error handling is up to you.
> 
> Since ASSERT's are compiled out they are never tested and are actually
> making code less safe.

Generally speaking assert is not subject to test - I would consider it as a part of debug means.
Yes, this assert was with wrong condition and was not tested, but once enabled and a lot of MACs
came into game - we got an issue and your patch is here 😊. 

>> making code less safe.
The debug version of code is usually less safe and has no performance.
Adding the check and error return is OK, it works  always and improves the code, we do not expect engaging of it here, though.
Removing assert (instead of fixing one) reduces our debugging capabilities, so it is not OK, as for me.

With best regards, Slava

Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert

[Stephen Hemminger]

On Tue, 31 Mar 2020 15:09:43 +0000
Slava Ovsiienko <viacheslavo@mellanox.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, March 31, 2020 17:55
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> > <shahafs@mellanox.com>; dev@dpdk.org; Alexander Kozyrev
> > <akozyrev@mellanox.com>
> > Subject: Re: [PATCH] common/mlx5: fix bogus assert
> > 
> > On Tue, 31 Mar 2020 07:31:48 +0000
> > Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> >   
> > > Hi, Stephen
> > >
> > > Thank you for the fix.
> > >
> > > The exposed API to set MAC addresses:
> > > - mlx5_mac_addr_set (invoked by rte_mac_addr_set ())
> > > - mlx5_set_mc_addr_list (invoked by rte_eth_dev_set_mc_addr_list())
> > >
> > > Both routines call mlx5_internal_mac_addr_add(), it in its turn calls
> > > mlx5_nl_mac_addr_add() (that is subject of the patch).
> > >
> > > mlx5_nl_mac_addr_add is internal function, not exposed external API,
> > > the wrong parameter means the critical internal bug, so assert looks to be  
> > relevant here.  
> > > I would not remove MLX5_ASSERT at all but fix just it.
> > > Adding the parameter check and return an error is nice.
> > > What do you think?
> > >
> > > With best regards, Slava  
> > 
> > The real root cause is that sizeof(mac_own) is the wrong thing to do. The
> > error handling is up to you.
> > 
> > Since ASSERT's are compiled out they are never tested and are actually
> > making code less safe.  
> 
> Generally speaking assert is not subject to test - I would consider it as a part of debug means.
> Yes, this assert was with wrong condition and was not tested, but once enabled and a lot of MACs
> came into game - we got an issue and your patch is here 😊. 
> 
> >> making code less safe.  
> The debug version of code is usually less safe and has no performance.
> Adding the check and error return is OK, it works  always and improves the code, we do not expect engaging of it here, though.
>

I am done being diplomatic.
You have repeatedly ignored the fact that doing sizeof a pointer is not
correct here. mac_own is a pointer so doing sizeof(mac_own) will not give what
you want.  You probably thought mac_own was an array, or that compiler would
know that the pointer was an array.

Any visible config option should work correctly. The code should not break.
Any not visible config option #ifdefs should be expunged from the upstream
code.

Either take the patch, or fix your code please

Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert

[Slava Ovsiienko]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Friday, April 10, 2020 20:15
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> <shahafs@mellanox.com>; dev@dpdk.org; Alexander Kozyrev
> <akozyrev@mellanox.com>
> Subject: Re: [PATCH] common/mlx5: fix bogus assert
> 
> On Tue, 31 Mar 2020 15:09:43 +0000
> Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Tuesday, March 31, 2020 17:55
> > > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > Cc: Matan Azrad <matan@mellanox.com>; Shahaf Shuler
> > > <shahafs@mellanox.com>; dev@dpdk.org; Alexander Kozyrev
> > > <akozyrev@mellanox.com>
> > > Subject: Re: [PATCH] common/mlx5: fix bogus assert
> > >
> > > On Tue, 31 Mar 2020 07:31:48 +0000
> > > Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> > >
> > > > Hi, Stephen
> > > >
> > > > Thank you for the fix.
> > > >
> > > > The exposed API to set MAC addresses:
> > > > - mlx5_mac_addr_set (invoked by rte_mac_addr_set ())
> > > > - mlx5_set_mc_addr_list (invoked by
> > > > rte_eth_dev_set_mc_addr_list())
> > > >
> > > > Both routines call mlx5_internal_mac_addr_add(), it in its turn
> > > > calls
> > > > mlx5_nl_mac_addr_add() (that is subject of the patch).
> > > >
> > > > mlx5_nl_mac_addr_add is internal function, not exposed external
> > > > API, the wrong parameter means the critical internal bug, so
> > > > assert looks to be
> > > relevant here.
> > > > I would not remove MLX5_ASSERT at all but fix just it.
> > > > Adding the parameter check and return an error is nice.
> > > > What do you think?
> > > >
> > > > With best regards, Slava
> > >
> > > The real root cause is that sizeof(mac_own) is the wrong thing to
> > > do. The error handling is up to you.
> > >
> > > Since ASSERT's are compiled out they are never tested and are
> > > actually making code less safe.
> >
> > Generally speaking assert is not subject to test - I would consider it as a part
> of debug means.
> > Yes, this assert was with wrong condition and was not tested, but once
> > enabled and a lot of MACs came into game - we got an issue and your patch
> is here 😊.
> >
> > >> making code less safe.
> > The debug version of code is usually less safe and has no performance.
> > Adding the check and error return is OK, it works  always and improves the
> code, we do not expect engaging of it here, though.
> >
> 
> I am done being diplomatic.
> You have repeatedly ignored the fact that doing sizeof a pointer is not correct
> here.
You are quite right. It is obvious bug and must be fixed, thank you for the patch.
And let me make you sure I did not mind fixing in anyway. 
My only proposal was to fix ASSERT as well instead of dropping one,
sorry if I did not express it in clear way.
Something like this:
MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);

> mac_own is a pointer so doing sizeof(mac_own) will not give what you
> want.  You probably thought mac_own was an array, or that compiler would
> know that the pointer was an array.
> 
> Any visible config option should work correctly. The code should not break.
> Any not visible config option #ifdefs should be expunged from the upstream
> code.
> 
> Either take the patch, or fix your code please
Whatever you'd prefer - please, fix ASSERT, or let me know if I should.

Thanks in advance,
Slava

[dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert

[Viacheslav Ovsiienko]

The MLX5 device supports up to MLX5_MAX_MAC_ADDRESSES (256) MAC addresses.
The code flushes all MAC devices.

If DPDK is compiled with MLX5_DEBUG this would an assert.
PANIC in mlx5_nl_mac_addr_flush():
line 775	assert "(size_t)(i) < sizeof(mac_own) * 8" failed

The root cause is that mac_own is a pointer and is being used as
a bitmap array. The sizeof(mac_own) would therefore be 64 but the
number of entries to be flushed would be 256.

There is a whole set of asserts in MLX5 netlink code with
the same bug; that should just be changed into proper error checks.

Fixes: 8e46d4e18f09 ("common/mlx5: improve assert control")
Cc: akozyrev@mellanox.com
Cc: stable@dpdk.org

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

---
v2: fix asserts
v1: http://patches.dpdk.org/patch/67453/
---
 drivers/common/mlx5/mlx5_nl.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/common/mlx5/mlx5_nl.c b/drivers/common/mlx5/mlx5_nl.c
index c144223..65efcd3 100644
--- a/drivers/common/mlx5/mlx5_nl.c
+++ b/drivers/common/mlx5/mlx5_nl.c
@@ -671,7 +671,10 @@ struct mlx5_nl_ifindex_data {
 
 	ret = mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 1);
 	if (!ret) {
-		MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
+		MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
+		if (index >= MLX5_MAX_MAC_ADDRESSES)
+			return -EINVAL;
+
 		BITFIELD_SET(mac_own, index);
 	}
 	if (ret == -EEXIST)
@@ -700,7 +703,10 @@ struct mlx5_nl_ifindex_data {
 mlx5_nl_mac_addr_remove(int nlsk_fd, unsigned int iface_idx, uint64_t *mac_own,
 			struct rte_ether_addr *mac, uint32_t index)
 {
-	MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
+	MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
+	if (index >= MLX5_MAX_MAC_ADDRESSES)
+		return -EINVAL;
+
 	BITFIELD_RESET(mac_own, index);
 	return mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 0);
 }
@@ -769,10 +775,12 @@ struct mlx5_nl_ifindex_data {
 {
 	int i;
 
+	if (n <= 0 || n >= MLX5_MAX_MAC_ADDRESSES)
+		return;
+
 	for (i = n - 1; i >= 0; --i) {
 		struct rte_ether_addr *m = &mac_addrs[i];
 
-		MLX5_ASSERT((size_t)(i) < sizeof(mac_own) * CHAR_BIT);
 		if (BITFIELD_ISSET(mac_own, i))
 			mlx5_nl_mac_addr_remove(nlsk_fd, iface_idx, mac_own, m,
 						i);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert

[Alexander Kozyrev]

These asserts seem redundant for me. Don't you think?
EINVAL is returned, why bother to assert the same condition?

Regards,
Alex  

> -----Original Message-----
> From: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Sent: Thursday, May 14, 2020 3:09
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; stephen@networkplumber.org; Alexander Kozyrev
> <akozyrev@mellanox.com>; stable@dpdk.org
> Subject: [PATCH v2] common/mlx5: fix bogus assert
> 
> The MLX5 device supports up to MLX5_MAX_MAC_ADDRESSES (256) MAC
> addresses.
> The code flushes all MAC devices.
> 
> If DPDK is compiled with MLX5_DEBUG this would an assert.
> PANIC in mlx5_nl_mac_addr_flush():
> line 775	assert "(size_t)(i) < sizeof(mac_own) * 8" failed
> 
> The root cause is that mac_own is a pointer and is being used as a bitmap array.
> The sizeof(mac_own) would therefore be 64 but the number of entries to be
> flushed would be 256.
> 
> There is a whole set of asserts in MLX5 netlink code with the same bug; that
> should just be changed into proper error checks.
> 
> Fixes: 8e46d4e18f09 ("common/mlx5: improve assert control")
> Cc: akozyrev@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> ---
> v2: fix asserts
> v1:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatches.d
> pdk.org%2Fpatch%2F67453%2F&amp;data=02%7C01%7Cakozyrev%40mellanox
> .com%7C4f8e2cb5aacd4a33e22a08d7f7d5c7c4%7Ca652971c7d2e4d9ba6a4d14
> 9256f461b%7C0%7C0%7C637250369858023357&amp;sdata=ZI7CTCQDnnmr6n
> pYXTOxOf4%2BBktSgmE%2F3rC4NG3QXxc%3D&amp;reserved=0
> ---
>  drivers/common/mlx5/mlx5_nl.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_nl.c b/drivers/common/mlx5/mlx5_nl.c
> index c144223..65efcd3 100644
> --- a/drivers/common/mlx5/mlx5_nl.c
> +++ b/drivers/common/mlx5/mlx5_nl.c
> @@ -671,7 +671,10 @@ struct mlx5_nl_ifindex_data {
> 
>  	ret = mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 1);
>  	if (!ret) {
> -		MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
> +		MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
> +		if (index >= MLX5_MAX_MAC_ADDRESSES)
> +			return -EINVAL;
> +
>  		BITFIELD_SET(mac_own, index);
>  	}
>  	if (ret == -EEXIST)
> @@ -700,7 +703,10 @@ struct mlx5_nl_ifindex_data {
> mlx5_nl_mac_addr_remove(int nlsk_fd, unsigned int iface_idx, uint64_t
> *mac_own,
>  			struct rte_ether_addr *mac, uint32_t index)  {
> -	MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
> +	MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
> +	if (index >= MLX5_MAX_MAC_ADDRESSES)
> +		return -EINVAL;
> +
>  	BITFIELD_RESET(mac_own, index);
>  	return mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 0);  } @@ -
> 769,10 +775,12 @@ struct mlx5_nl_ifindex_data {  {
>  	int i;
> 
> +	if (n <= 0 || n >= MLX5_MAX_MAC_ADDRESSES)
> +		return;
> +
>  	for (i = n - 1; i >= 0; --i) {
>  		struct rte_ether_addr *m = &mac_addrs[i];
> 
> -		MLX5_ASSERT((size_t)(i) < sizeof(mac_own) * CHAR_BIT);
>  		if (BITFIELD_ISSET(mac_own, i))
>  			mlx5_nl_mac_addr_remove(nlsk_fd, iface_idx,
> mac_own, m,
>  						i);
> --
> 1.8.3.1

Re: [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert

[Slava Ovsiienko]

> -----Original Message-----
> From: Alexander Kozyrev <akozyrev@mellanox.com>
> Sent: Thursday, May 14, 2020 18:11
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; stephen@networkplumber.org; stable@dpdk.org
> Subject: RE: [PATCH v2] common/mlx5: fix bogus assert
> 
> These asserts seem redundant for me. Don't you think?
> EINVAL is returned, why bother to assert the same condition?
To stop the wrong conditions evolving on debug ?

> 
> Regards,
> Alex
> 
> > -----Original Message-----
> > From: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > Sent: Thursday, May 14, 2020 3:09
> > To: dev@dpdk.org
> > Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > <rasland@mellanox.com>; stephen@networkplumber.org; Alexander
> Kozyrev
> > <akozyrev@mellanox.com>; stable@dpdk.org
> > Subject: [PATCH v2] common/mlx5: fix bogus assert
> >
> > The MLX5 device supports up to MLX5_MAX_MAC_ADDRESSES (256) MAC
> > addresses.
> > The code flushes all MAC devices.
> >
> > If DPDK is compiled with MLX5_DEBUG this would an assert.
> > PANIC in mlx5_nl_mac_addr_flush():
> > line 775	assert "(size_t)(i) < sizeof(mac_own) * 8" failed
> >
> > The root cause is that mac_own is a pointer and is being used as a bitmap
> array.
> > The sizeof(mac_own) would therefore be 64 but the number of entries to
> > be flushed would be 256.
> >
> > There is a whole set of asserts in MLX5 netlink code with the same
> > bug; that should just be changed into proper error checks.
> >
> > Fixes: 8e46d4e18f09 ("common/mlx5: improve assert control")
> > Cc: akozyrev@mellanox.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> >
> > ---
> > v2: fix asserts
> > v1:
> >
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> > es.d
> pdk.org%2Fpatch%2F67453%2F&amp;data=02%7C01%7Cakozyrev%40mella
> nox
> >
> .com%7C4f8e2cb5aacd4a33e22a08d7f7d5c7c4%7Ca652971c7d2e4d9ba6a4
> d14
> >
> 9256f461b%7C0%7C0%7C637250369858023357&amp;sdata=ZI7CTCQDnnm
> r6n
> > pYXTOxOf4%2BBktSgmE%2F3rC4NG3QXxc%3D&amp;reserved=0
> > ---
> >  drivers/common/mlx5/mlx5_nl.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/common/mlx5/mlx5_nl.c
> > b/drivers/common/mlx5/mlx5_nl.c index c144223..65efcd3 100644
> > --- a/drivers/common/mlx5/mlx5_nl.c
> > +++ b/drivers/common/mlx5/mlx5_nl.c
> > @@ -671,7 +671,10 @@ struct mlx5_nl_ifindex_data {
> >
> >  	ret = mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 1);
> >  	if (!ret) {
> > -		MLX5_ASSERT((size_t)(index) < sizeof(mac_own) *
> CHAR_BIT);
> > +		MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
> > +		if (index >= MLX5_MAX_MAC_ADDRESSES)
> > +			return -EINVAL;
> > +
> >  		BITFIELD_SET(mac_own, index);
> >  	}
> >  	if (ret == -EEXIST)
> > @@ -700,7 +703,10 @@ struct mlx5_nl_ifindex_data {
> > mlx5_nl_mac_addr_remove(int nlsk_fd, unsigned int iface_idx, uint64_t
> > *mac_own,
> >  			struct rte_ether_addr *mac, uint32_t index)  {
> > -	MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
> > +	MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
> > +	if (index >= MLX5_MAX_MAC_ADDRESSES)
> > +		return -EINVAL;
> > +
> >  	BITFIELD_RESET(mac_own, index);
> >  	return mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 0);  } @@
> -
> > 769,10 +775,12 @@ struct mlx5_nl_ifindex_data {  {
> >  	int i;
> >
> > +	if (n <= 0 || n >= MLX5_MAX_MAC_ADDRESSES)
> > +		return;
> > +
> >  	for (i = n - 1; i >= 0; --i) {
> >  		struct rte_ether_addr *m = &mac_addrs[i];
> >
> > -		MLX5_ASSERT((size_t)(i) < sizeof(mac_own) * CHAR_BIT);
> >  		if (BITFIELD_ISSET(mac_own, i))
> >  			mlx5_nl_mac_addr_remove(nlsk_fd, iface_idx,
> mac_own, m,
> >  						i);
> > --
> > 1.8.3.1

Re: [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert

[Matan Azrad]

From: Viacheslav Ovsiienko
> The MLX5 device supports up to MLX5_MAX_MAC_ADDRESSES (256) MAC
> addresses.
> The code flushes all MAC devices.
> 
> If DPDK is compiled with MLX5_DEBUG this would an assert.
> PANIC in mlx5_nl_mac_addr_flush():
> line 775	assert "(size_t)(i) < sizeof(mac_own) * 8" failed
> 
> The root cause is that mac_own is a pointer and is being used as a bitmap
> array. The sizeof(mac_own) would therefore be 64 but the number of
> entries to be flushed would be 256.
> 
> There is a whole set of asserts in MLX5 netlink code with the same bug; that
> should just be changed into proper error checks.
> 
> Fixes: 8e46d4e18f09 ("common/mlx5: improve assert control")
> Cc: akozyrev@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>

Re: [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert

[Raslan Darawsheh]

Hi,

> -----Original Message-----
> From: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Sent: Thursday, May 14, 2020 10:09 AM
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; stephen@networkplumber.org; Alexander
> Kozyrev <akozyrev@mellanox.com>; stable@dpdk.org
> Subject: [PATCH v2] common/mlx5: fix bogus assert
> 
> The MLX5 device supports up to MLX5_MAX_MAC_ADDRESSES (256) MAC
> addresses.
> The code flushes all MAC devices.
> 
> If DPDK is compiled with MLX5_DEBUG this would an assert.
> PANIC in mlx5_nl_mac_addr_flush():
> line 775	assert "(size_t)(i) < sizeof(mac_own) * 8" failed
> 
> The root cause is that mac_own is a pointer and is being used as
> a bitmap array. The sizeof(mac_own) would therefore be 64 but the
> number of entries to be flushed would be 256.
> 
> There is a whole set of asserts in MLX5 netlink code with
> the same bug; that should just be changed into proper error checks.
> 
> Fixes: 8e46d4e18f09 ("common/mlx5: improve assert control")
> Cc: akozyrev@mellanox.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> ---
> v2: fix asserts
> v1:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> es.dpdk.org%2Fpatch%2F67453%2F&amp;data=02%7C01%7Crasland%40mell
> anox.com%7C71b3b1e727b8493f98fe08d7f7d5ce48%7Ca652971c7d2e4d9ba6
> a4d149256f461b%7C0%7C0%7C637250369975781842&amp;sdata=RI5eahOH
> MTb7khPqPkYe0AD3bRB6vPtuRh9INSJa8N0%3D&amp;reserved=0
> ---
>  drivers/common/mlx5/mlx5_nl.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/common/mlx5/mlx5_nl.c
> b/drivers/common/mlx5/mlx5_nl.c
> index c144223..65efcd3 100644
> --- a/drivers/common/mlx5/mlx5_nl.c
> +++ b/drivers/common/mlx5/mlx5_nl.c
> @@ -671,7 +671,10 @@ struct mlx5_nl_ifindex_data {
> 
>  	ret = mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 1);
>  	if (!ret) {
> -		MLX5_ASSERT((size_t)(index) < sizeof(mac_own) *
> CHAR_BIT);
> +		MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
> +		if (index >= MLX5_MAX_MAC_ADDRESSES)
> +			return -EINVAL;
> +
>  		BITFIELD_SET(mac_own, index);
>  	}
>  	if (ret == -EEXIST)
> @@ -700,7 +703,10 @@ struct mlx5_nl_ifindex_data {
>  mlx5_nl_mac_addr_remove(int nlsk_fd, unsigned int iface_idx, uint64_t
> *mac_own,
>  			struct rte_ether_addr *mac, uint32_t index)
>  {
> -	MLX5_ASSERT((size_t)(index) < sizeof(mac_own) * CHAR_BIT);
> +	MLX5_ASSERT(index < MLX5_MAX_MAC_ADDRESSES);
> +	if (index >= MLX5_MAX_MAC_ADDRESSES)
> +		return -EINVAL;
> +
>  	BITFIELD_RESET(mac_own, index);
>  	return mlx5_nl_mac_addr_modify(nlsk_fd, iface_idx, mac, 0);
>  }
> @@ -769,10 +775,12 @@ struct mlx5_nl_ifindex_data {
>  {
>  	int i;
> 
> +	if (n <= 0 || n >= MLX5_MAX_MAC_ADDRESSES)
> +		return;
> +
>  	for (i = n - 1; i >= 0; --i) {
>  		struct rte_ether_addr *m = &mac_addrs[i];
> 
> -		MLX5_ASSERT((size_t)(i) < sizeof(mac_own) * CHAR_BIT);
>  		if (BITFIELD_ISSET(mac_own, i))
>  			mlx5_nl_mac_addr_remove(nlsk_fd, iface_idx,
> mac_own, m,
>  						i);
> --
> 1.8.3.1


Patch applied to next-net-mlx,

Kindest regards
Raslan Darawsheh