DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] common/mlx5: fix bogus assert
@ 2020-03-31  6:02 Stephen Hemminger
  2020-03-31  7:31 ` Slava Ovsiienko
  2020-05-14  7:09 ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
  0 siblings, 2 replies; 11+ messages in thread
From: Stephen Hemminger @ 2020-03-31  6:02 UTC (permalink / raw)
  To: matan, shahafs, viacheslavo; +Cc: dev, Stephen Hemminger, akozyrev

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert
  2020-03-31  6:02 [dpdk-dev] [PATCH] common/mlx5: fix bogus assert Stephen Hemminger
@ 2020-03-31  7:31 ` Slava Ovsiienko
  2020-03-31 14:55   ` Stephen Hemminger
  2020-05-14  7:09 ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
  1 sibling, 1 reply; 11+ messages in thread
From: Slava Ovsiienko @ 2020-03-31  7:31 UTC (permalink / raw)
  To: Stephen Hemminger, Matan Azrad, Shahaf Shuler; +Cc: dev, Alexander Kozyrev

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert
  2020-03-31  7:31 ` Slava Ovsiienko
@ 2020-03-31 14:55   ` Stephen Hemminger
  2020-03-31 15:09     ` Slava Ovsiienko
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2020-03-31 14:55 UTC (permalink / raw)
  To: Slava Ovsiienko; +Cc: Matan Azrad, Shahaf Shuler, dev, Alexander Kozyrev

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert
  2020-03-31 14:55   ` Stephen Hemminger
@ 2020-03-31 15:09     ` Slava Ovsiienko
  2020-04-10 17:14       ` Stephen Hemminger
  0 siblings, 1 reply; 11+ messages in thread
From: Slava Ovsiienko @ 2020-03-31 15:09 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Matan Azrad, Shahaf Shuler, dev, Alexander Kozyrev

> -----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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert
  2020-03-31 15:09     ` Slava Ovsiienko
@ 2020-04-10 17:14       ` Stephen Hemminger
  2020-04-13  9:51         ` Slava Ovsiienko
  0 siblings, 1 reply; 11+ messages in thread
From: Stephen Hemminger @ 2020-04-10 17:14 UTC (permalink / raw)
  To: Slava Ovsiienko; +Cc: Matan Azrad, Shahaf Shuler, dev, Alexander Kozyrev

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] common/mlx5: fix bogus assert
  2020-04-10 17:14       ` Stephen Hemminger
@ 2020-04-13  9:51         ` Slava Ovsiienko
  0 siblings, 0 replies; 11+ messages in thread
From: Slava Ovsiienko @ 2020-04-13  9:51 UTC (permalink / raw)
  To: Stephen Hemminger; +Cc: Matan Azrad, Shahaf Shuler, dev, Alexander Kozyrev



> -----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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert
  2020-03-31  6:02 [dpdk-dev] [PATCH] common/mlx5: fix bogus assert Stephen Hemminger
  2020-03-31  7:31 ` Slava Ovsiienko
@ 2020-05-14  7:09 ` Viacheslav Ovsiienko
  2020-05-14 15:11   ` Alexander Kozyrev
                     ` (2 more replies)
  1 sibling, 3 replies; 11+ messages in thread
From: Viacheslav Ovsiienko @ 2020-05-14  7:09 UTC (permalink / raw)
  To: dev; +Cc: matan, rasland, stephen, akozyrev, stable

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert
  2020-05-14  7:09 ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
@ 2020-05-14 15:11   ` Alexander Kozyrev
  2020-05-14 20:38     ` Slava Ovsiienko
  2020-05-17 12:02   ` Matan Azrad
  2020-05-17 12:39   ` Raslan Darawsheh
  2 siblings, 1 reply; 11+ messages in thread
From: Alexander Kozyrev @ 2020-05-14 15:11 UTC (permalink / raw)
  To: Slava Ovsiienko, dev; +Cc: Matan Azrad, Raslan Darawsheh, stephen, stable

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert
  2020-05-14 15:11   ` Alexander Kozyrev
@ 2020-05-14 20:38     ` Slava Ovsiienko
  0 siblings, 0 replies; 11+ messages in thread
From: Slava Ovsiienko @ 2020-05-14 20:38 UTC (permalink / raw)
  To: Alexander Kozyrev, dev; +Cc: Matan Azrad, Raslan Darawsheh, stephen, stable

> -----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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert
  2020-05-14  7:09 ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
  2020-05-14 15:11   ` Alexander Kozyrev
@ 2020-05-17 12:02   ` Matan Azrad
  2020-05-17 12:39   ` Raslan Darawsheh
  2 siblings, 0 replies; 11+ messages in thread
From: Matan Azrad @ 2020-05-17 12:02 UTC (permalink / raw)
  To: Slava Ovsiienko, dev; +Cc: Raslan Darawsheh, stephen, Alexander Kozyrev, stable



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>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH v2] common/mlx5: fix bogus assert
  2020-05-14  7:09 ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
  2020-05-14 15:11   ` Alexander Kozyrev
  2020-05-17 12:02   ` Matan Azrad
@ 2020-05-17 12:39   ` Raslan Darawsheh
  2 siblings, 0 replies; 11+ messages in thread
From: Raslan Darawsheh @ 2020-05-17 12:39 UTC (permalink / raw)
  To: Slava Ovsiienko, dev; +Cc: Matan Azrad, stephen, Alexander Kozyrev, stable

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-05-17 12:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-31  6:02 [dpdk-dev] [PATCH] common/mlx5: fix bogus assert Stephen Hemminger
2020-03-31  7:31 ` Slava Ovsiienko
2020-03-31 14:55   ` Stephen Hemminger
2020-03-31 15:09     ` Slava Ovsiienko
2020-04-10 17:14       ` Stephen Hemminger
2020-04-13  9:51         ` Slava Ovsiienko
2020-05-14  7:09 ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
2020-05-14 15:11   ` Alexander Kozyrev
2020-05-14 20:38     ` Slava Ovsiienko
2020-05-17 12:02   ` Matan Azrad
2020-05-17 12:39   ` Raslan Darawsheh

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git