net/mlx4_core: Avoid impossible mlx4_db_alloc() order value

PATCH Applied Kees Cook 5 participants 6 messages Feb 10, 2025 – Feb 15, 2025

Subsystems (5)

Networking Network drivers InfiniBand Mellanox MLX4 core VPI driver +1 more subsystems

lore

Share & tools

lore

Kees Cook

author

2025-02-10 17:45 UTC #0 lore raw

PATCH 1/1 net/mlx4_core: Avoid impossible mlx4_db_alloc() order value

GCC can see that the value range for "order" is capped, but this leads it to consider that it might be negative, leading to a false positive warning (with GCC 15 with -Warray-bounds -fdiagnostics-details):

../drivers/net/ethernet/mellanox/mlx4/alloc.c:691:47: error: array subscript -1 is below array bounds of 'long unsigned int *[2]' [-Werror=array-bounds=]

  691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);
      |                                    ~~~~~~~~~~~^~~
  'mlx4_alloc_db_from_pgdir': events 1-2
  691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);                        |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                     |                         |                                                   |                     |                         (2) out of array bounds here
      |                     (1) when the condition is evaluated to true                             In file included from ../drivers/net/ethernet/mellanox/mlx4/mlx4.h:53,
                 from ../drivers/net/ethernet/mellanox/mlx4/alloc.c:42:

../include/linux/mlx4/device.h:664:33: note: while referencing 'bits'
664 | unsigned long *bits[2];
| ^~~~

Switch the argument to unsigned int, which removes the compiler needing to consider negative values.

Signed-off-by Kees Cook <[email protected]>

---
Cc: Tariq Toukan <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Yishai Hadas <[email protected]>
Cc: [email protected]
Cc: [email protected]

---
 drivers/net/ethernet/mellanox/mlx4/alloc.c | 6 +++---
 include/linux/mlx4/device.h                | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

drivers/net/ethernet/mellanox/mlx4/alloc.c +3 -3

index b330020dc0d6..f2bded847e61 100644
@@ -682,9 +682,9 @@ static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device)
 }
 
 static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
-				    struct mlx4_db *db, int order)
+				    struct mlx4_db *db, unsigned int order)
 {
-	int o;
+	unsigned int o;
 	int i;
 
 	for (o = order; o <= 1; ++o) {

@@ -712,7 +712,7 @@ static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
 	return 0;
 }
 
-int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order)
+int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, unsigned int order)
 {
 	struct mlx4_priv *priv = mlx4_priv(dev);
 	struct mlx4_db_pgdir *pgdir;

include/linux/mlx4/device.h +1 -1

index 27f42f713c89..86f0f2a25a3d 100644
@@ -1135,7 +1135,7 @@ int mlx4_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
 int mlx4_buf_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
 		       struct mlx4_buf *buf);
 
-int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order);
+int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, unsigned int order);
 void mlx4_db_free(struct mlx4_dev *dev, struct mlx4_db *db);
 
 int mlx4_alloc_hwq_res(struct mlx4_dev *dev, struct mlx4_hwq_resources *wqres,

Signature · 1 line

2.34.1

Justin Stitt

2025-02-11 00:01 UTC #1 lore raw

Quoted 41 lines · Kees Cook

GCC can see that the value range for "order" is capped, but this leads it to consider that it might be negative, leading to a false positive warning (with GCC 15 with -Warray-bounds -fdiagnostics-details):
../drivers/net/ethernet/mellanox/mlx4/alloc.c:691:47: error: array subscript -1 is below array bounds of 'long unsigned int *[2]' [-Werror=array-bounds=]
  691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);
      |                                    ~~~~~~~~~~~^~~
  'mlx4_alloc_db_from_pgdir': events 1-2
  691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);                        |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                     |                         |                                                   |                     |                         (2) out of array bounds here
      |                     (1) when the condition is evaluated to true                             In file included from ../drivers/net/ethernet/mellanox/mlx4/mlx4.h:53,
                 from ../drivers/net/ethernet/mellanox/mlx4/alloc.c:42:
../include/linux/mlx4/device.h:664:33: note: while referencing 'bits'
664 | unsigned long *bits[2];
| ^~~~
Switch the argument to unsigned int, which removes the compiler needing to consider negative values.
Signed-off-by Kees Cook <[email protected]>
---
Cc: Tariq Toukan <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Yishai Hadas <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
 drivers/net/ethernet/mellanox/mlx4/alloc.c | 6 +++---
 include/linux/mlx4/device.h                | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx4/alloc.c +0 -0
index b330020dc0d6..f2bded847e61 100644
@@ -682,9 +682,9 @@ static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device)
 }

drivers/net/ethernet/mellanox/mlx4/alloc.c static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device) · line 690

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx4/alloc.c +2 -2

 
 static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
-				    struct mlx4_db *db, int order)
+				    struct mlx4_db *db, unsigned int order)
 {
-	int o;
+	unsigned int o;
 	int i;
 
 	for (o = order; o <= 1; ++o) {

^ Knowing now that @order can only be 0 or 1 can this for loop (and goto) be dropped entirely?

The code is already short and sweet so I don't feel strongly either way.

Quoted 25 lines

drivers/net/ethernet/mellanox/mlx4/alloc.c +1 -1

@@ -712,7 +712,7 @@ static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
 	return 0;
 }
 
-int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order)
+int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, unsigned int order)
 {
 	struct mlx4_priv *priv = mlx4_priv(dev);
 	struct mlx4_db_pgdir *pgdir;

include/linux/mlx4/device.h +1 -1

index 27f42f713c89..86f0f2a25a3d 100644
@@ -1135,7 +1135,7 @@ int mlx4_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
 int mlx4_buf_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
 		       struct mlx4_buf *buf);
 
-int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order);
+int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, unsigned int order);
 void mlx4_db_free(struct mlx4_dev *dev, struct mlx4_db *db);
 
 int mlx4_alloc_hwq_res(struct mlx4_dev *dev, struct mlx4_hwq_resources *wqres,

Signature · 1 line

2.34.1

Justin

Tariq Toukan

2025-02-11 14:22 UTC #2 lore raw

Quoted 51 lines · Justin Stitt

Quoted 50 lines · Kees Cook
GCC can see that the value range for "order" is capped, but this leads it to consider that it might be negative, leading to a false positive warning (with GCC 15 with -Warray-bounds -fdiagnostics-details):
../drivers/net/ethernet/mellanox/mlx4/alloc.c:691:47: error: array subscript -1 is below array bounds of 'long unsigned int *[2]' [-Werror=array-bounds=]
   691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);
       |                                    ~~~~~~~~~~~^~~
   'mlx4_alloc_db_from_pgdir': events 1-2
   691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);                        |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       |                     |                         |                                                   |                     |                         (2) out of array bounds here
       |                     (1) when the condition is evaluated to true                             In file included from ../drivers/net/ethernet/mellanox/mlx4/mlx4.h:53,
                  from ../drivers/net/ethernet/mellanox/mlx4/alloc.c:42:
../include/linux/mlx4/device.h:664:33: note: while referencing 'bits'
664 | unsigned long *bits[2];
| ^~~~
Switch the argument to unsigned int, which removes the compiler needing to consider negative values.
Signed-off-by Kees Cook <[email protected]>
---
Cc: Tariq Toukan <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Yishai Hadas <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
  drivers/net/ethernet/mellanox/mlx4/alloc.c | 6 +++---
  include/linux/mlx4/device.h                | 2 +-
  2 files changed, 4 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx4/alloc.c +2 -2
index b330020dc0d6..f2bded847e61 100644
@@ -682,9 +682,9 @@ static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device)
  }
 
  static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
-				    struct mlx4_db *db, int order)
+				    struct mlx4_db *db, unsigned int order)
  {
-	int o;
+	unsigned int o;
  	int i;
 

Quoted 5 lines

drivers/net/ethernet/mellanox/mlx4/alloc.c static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device) · line 690
Quoted 1 line
drivers/net/ethernet/mellanox/mlx4/alloc.c +0 -0
  	for (o = order; o <= 1; ++o) {
^ Knowing now that @order can only be 0 or 1 can this for loop (and goto) be dropped entirely?

Maybe I'm missing something...
Can you please explain why you think this can be dropped?

Quoted 31 lines

The code is already short and sweet so I don't feel strongly either way.

Quoted 25 lines

drivers/net/ethernet/mellanox/mlx4/alloc.c +1 -1

@@ -712,7 +712,7 @@ static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
  	return 0;
  }
 
-int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order)
+int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, unsigned int order)
  {
  	struct mlx4_priv *priv = mlx4_priv(dev);
  	struct mlx4_db_pgdir *pgdir;

include/linux/mlx4/device.h +1 -1

index 27f42f713c89..86f0f2a25a3d 100644
@@ -1135,7 +1135,7 @@ int mlx4_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
  int mlx4_buf_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
  		       struct mlx4_buf *buf);
 
-int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order);
+int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, unsigned int order);
  void mlx4_db_free(struct mlx4_dev *dev, struct mlx4_db *db);
 
  int mlx4_alloc_hwq_res(struct mlx4_dev *dev, struct mlx4_hwq_resources *wqres,

Signature · 1 line

2.34.1

Justin

Justin Stitt

2025-02-13 00:10 UTC #3 lore raw

Quoted 55 lines · Tariq Toukan

Quoted 51 lines · Justin Stitt
Quoted 50 lines · Kees Cook
GCC can see that the value range for "order" is capped, but this leads it to consider that it might be negative, leading to a false positive warning (with GCC 15 with -Warray-bounds -fdiagnostics-details):
../drivers/net/ethernet/mellanox/mlx4/alloc.c:691:47: error: array subscript -1 is below array bounds of 'long unsigned int *[2]' [-Werror=array-bounds=]
   691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);
       |                                    ~~~~~~~~~~~^~~
   'mlx4_alloc_db_from_pgdir': events 1-2
   691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);                        |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       |                     |                         |                                                   |                     |                         (2) out of array bounds here
       |                     (1) when the condition is evaluated to true                             In file included from ../drivers/net/ethernet/mellanox/mlx4/mlx4.h:53,
                  from ../drivers/net/ethernet/mellanox/mlx4/alloc.c:42:
../include/linux/mlx4/device.h:664:33: note: while referencing 'bits'
664 | unsigned long *bits[2];
| ^~~~
Switch the argument to unsigned int, which removes the compiler needing to consider negative values.
Signed-off-by Kees Cook <[email protected]>
---
Cc: Tariq Toukan <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Yishai Hadas <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
  drivers/net/ethernet/mellanox/mlx4/alloc.c | 6 +++---
  include/linux/mlx4/device.h                | 2 +-
  2 files changed, 4 insertions(+), 4 deletions(-)
drivers/net/ethernet/mellanox/mlx4/alloc.c +2 -2
index b330020dc0d6..f2bded847e61 100644
@@ -682,9 +682,9 @@ static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device)
  }
 
  static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
-                                struct mlx4_db *db, int order)
+                                struct mlx4_db *db, unsigned int order)
  {
-    int o;
+    unsigned int o;
     int i;
 

Quoted 8 lines

Quoted 5 lines
drivers/net/ethernet/mellanox/mlx4/alloc.c static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device) · line 690
Quoted 1 line
drivers/net/ethernet/mellanox/mlx4/alloc.c +0 -0
     for (o = order; o <= 1; ++o) {
^ Knowing now that @order can only be 0 or 1 can this for loop (and goto) be dropped entirely?
Maybe I'm missing something...
Can you please explain why you think this can be dropped?

I meant "rewritten to use two if statements" instead of "dropped". I think "replaced" or "refactored" was the word I wanted.

Quoted 34 lines

Quoted 33 lines

The code is already short and sweet so I don't feel strongly either way.

Quoted 23 lines

drivers/net/ethernet/mellanox/mlx4/alloc.c +1 -1

@@ -712,7 +712,7 @@ static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
     return 0;
  }
 
-int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order)
+int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, unsigned int order)
  {
     struct mlx4_priv *priv = mlx4_priv(dev);
     struct mlx4_db_pgdir *pgdir;

include/linux/mlx4/device.h +1 -2

index 27f42f713c89..86f0f2a25a3d 100644
@@ -1135,7 +1135,7 @@ int mlx4_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
  int mlx4_buf_write_mtt(struct mlx4_dev *dev, struct mlx4_mtt *mtt,
                    struct mlx4_buf *buf);
 
-int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, int order);
+int mlx4_db_alloc(struct mlx4_dev *dev, struct mlx4_db *db, unsigned int order);
  void mlx4_db_free(struct mlx4_dev *dev, struct mlx4_db *db);
 
  int mlx4_alloc_hwq_res(struct mlx4_dev *dev, struct mlx4_hwq_resources *wqres,
--

Quoted 2 lines

2.34.1

Justin

Paolo Abeni

maintainer

2025-02-13 10:13 UTC #4 lore raw

Quoted 23 lines · Justin Stitt

Quoted 22 lines · Tariq Toukan
Quoted 20 lines · Justin Stitt
Quoted 6 lines
drivers/net/ethernet/mellanox/mlx4/alloc.c +0 -0
index b330020dc0d6..f2bded847e61 100644
@@ -682,9 +682,9 @@ static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device)
  }
drivers/net/ethernet/mellanox/mlx4/alloc.c static struct mlx4_db_pgdir *mlx4_alloc_db_pgdir(struct device *dma_device) · line 690
Quoted 10 lines
drivers/net/ethernet/mellanox/mlx4/alloc.c +2 -2
 
  static int mlx4_alloc_db_from_pgdir(struct mlx4_db_pgdir *pgdir,
-                                struct mlx4_db *db, int order)
+                                struct mlx4_db *db, unsigned int order)
  {
-    int o;
+    unsigned int o;
     int i;
 
     for (o = order; o <= 1; ++o) {
^ Knowing now that @order can only be 0 or 1 can this for loop (and goto) be dropped entirely?

Quoted 5 lines

Quoted 2 lines
Maybe I'm missing something...
Can you please explain why you think this can be dropped?
I meant "rewritten to use two if statements" instead of "dropped". I think "replaced" or "refactored" was the word I wanted.

IMHO that would be a significant uglification, not worthy to address an issue that could be solved with the patch proposed here.

@Tariq: are you ok with this patch?

Thanks,

Paolo

[email protected]

CI Applied

2025-02-15 03:50 UTC #5 lore raw

In reply to Kees Cook

This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 12 lines · On Mon, 10 Feb 2025 09:45:05 -0800 you wrote:

GCC can see that the value range for "order" is capped, but this leads it to consider that it might be negative, leading to a false positive warning (with GCC 15 with -Warray-bounds -fdiagnostics-details):
../drivers/net/ethernet/mellanox/mlx4/alloc.c:691:47: error: array subscript -1 is below array bounds of 'long unsigned int *[2]' [-Werror=array-bounds=]
  691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);
      |                                    ~~~~~~~~~~~^~~
  'mlx4_alloc_db_from_pgdir': events 1-2
  691 |                 i = find_first_bit(pgdir->bits[o], MLX4_DB_PER_PAGE >> o);                        |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                     |                         |                                                   |                     |                         (2) out of array bounds here
      |                     (1) when the condition is evaluated to true                             In file included from ../drivers/net/ethernet/mellanox/mlx4/mlx4.h:53,
                 from ../drivers/net/ethernet/mellanox/mlx4/alloc.c:42:

Quoted 5 lines

../include/linux/mlx4/device.h:664:33: note: while referencing 'bits'
664 | unsigned long *bits[2];
| ^~~~
[...]

Here is the summary with links:
- net/mlx4_core: Avoid impossible mlx4_db_alloc() order value

    https://git.kernel.org/netdev/net-next/c/4a6f18f28627

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html