Skip to contentSkip to conversation

regex/mlx5: fix size of setup constants

PATCH Superseded Michael Baum 3 participants 15 messages Jun 1, 2021 – Jul 22, 2021

Subsystems (3)
Network drivers virtio core Mellanox MLX5 vdpa driver
lore
Share & tools
lore

Michael Baum

author
#0 lore raw

PATCH 1/4 regex/mlx5: fix size of setup constants

The constant representing the size of the metadata is defined as a unsigned int variable with 32-bit.
Similarly the constant representing the maximal output is also defined as a unsigned int variable with 32-bit.

There is potentially overflowing expression when those constants are evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type size_t (64 bits, unsigned).

Change the size of the above constants to 64-bit.

Fixes 30d604bb1504 ("regex/mlx5: fix type of setup constants")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
---
 drivers/regex/mlx5/mlx5_regex_fastpath.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/regex/mlx5/mlx5_regex_fastpath.c +2 -2
index b57e7d7794..3ef5e6c1eb 100644
@@ -25,8 +25,8 @@
 #include "mlx5_regex.h"
 
 #define MLX5_REGEX_MAX_WQE_INDEX 0xffff
-#define MLX5_REGEX_METADATA_SIZE UINT32_C(64)
-#define MLX5_REGEX_MAX_OUTPUT RTE_BIT32(11)
+#define MLX5_REGEX_METADATA_SIZE UINT64_C(64)
+#define MLX5_REGEX_MAX_OUTPUT RTE_BIT64(11)
 #define MLX5_REGEX_WQE_CTRL_OFFSET 12
 #define MLX5_REGEX_WQE_METADATA_OFFSET 16
 #define MLX5_REGEX_WQE_GATHER_OFFSET 32
Signature · 1 line
2.25.1
author
#1 lore raw

PATCH 2/4 compress/mlx5: fix constant size in QP creation

The mlx5_compress_qp_setup function makes shifting to the numeric constant 1, then sends it as a parameter to rte_calloc function.

The rte_calloc function expects to get size_t (64 bits, unsigned) and instead gets a 32-bit variable, because the numeric constant size is a 32-bit.
In case the shift is greater than 32 the variable will lose its value even though the function can get 64-bit argument.

Change the size of the numeric constant 1 to 64-bit.

Fixes 8619fcd5161b ("compress/mlx5: support queue pair operations")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
---
 drivers/compress/mlx5/mlx5_compress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/compress/mlx5/mlx5_compress.c +1 -1
index 80c564f10b..90d009c56b 100644
@@ -209,7 +209,7 @@ mlx5_compress_qp_setup(struct rte_compressdev *dev, uint16_t qp_id,
 		return -rte_errno;
 	}
 	dev->data->queue_pairs[qp_id] = qp;
-	opaq_buf = rte_calloc(__func__, 1u << log_ops_n,
+	opaq_buf = rte_calloc(__func__, RTE_BIT64(log_ops_n),
 			      sizeof(struct mlx5_gga_compress_opaque),
 			      sizeof(struct mlx5_gga_compress_opaque));
 	if (opaq_buf == NULL) {
Signature · 1 line
2.25.1
Acked-by on v1 2/4
#2 lore raw

Re: [dpdk-dev] [PATCH 2/4] compress/mlx5: fix constant size in QP creation

From: Michael Baum:

Quoted 8 lines

The mlx5_compress_qp_setup function makes shifting to the numeric constant 1, then sends it as a parameter to rte_calloc function.

The rte_calloc function expects to get size_t (64 bits, unsigned) and instead gets a 32-bit variable, because the numeric constant size is a 32-bit. In case the shift is greater than 32 the variable will lose its value even though the function can get 64-bit argument.

Quoted 6 lines

Change the size of the numeric constant 1 to 64-bit.

Fixes 8619fcd5161b ("compress/mlx5: support queue pair operations")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>
#3 lore raw

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/4] compress/mlx5: fix constant size in QP creation

In reply to Michael Baum

01/06/2021 09:11, Michael Baum:

Quoted 4 lines

The mlx5_compress_qp_setup function makes shifting to the numeric constant 1, then sends it as a parameter to rte_calloc function.

The rte_calloc function expects to get size_t (64 bits, unsigned) and

No on 32-bit systems, size_t is 32 bits.

Quoted 2 lines

instead gets a 32-bit variable, because the numeric constant size is a 32-bit.

Most of the patches of this series say "constant" where it is a variable.

Quoted 4 lines

In case the shift is greater than 32 the variable will lose its value even though the function can get 64-bit argument.

Change the size of the numeric constant 1 to 64-bit.

[...]

Quoted 2 lines
drivers/compress/mlx5/mlx5_compress.c +1 -1
-	opaq_buf = rte_calloc(__func__, 1u << log_ops_n,
+	opaq_buf = rte_calloc(__func__, RTE_BIT64(log_ops_n),
author
#4 lore raw

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/4] compress/mlx5: fix constant size in QP creation

External email: Use caution opening links or attachments

Quoted 13 lines

-----Original Message-----
From: Thomas Monjalon <[email protected]>
Sent: Wednesday, June 23, 2021 9:48 AM
To: Michael Baum <[email protected]>
Cc: [email protected]; [email protected]; Matan Azrad <[email protected]>; Raslan Darawsheh <[email protected]>; Slava Ovsiienko <[email protected]>
Subject: Re: [dpdk-stable] [PATCH 2/4] compress/mlx5: fix constant size in QP creation

External email: Use caution opening links or attachments

Quoted 7 lines

01/06/2021 09:11, Michael Baum:

Quoted 4 lines

The mlx5_compress_qp_setup function makes shifting to the numeric constant 1, then sends it as a parameter to rte_calloc function.

The rte_calloc function expects to get size_t (64 bits, unsigned) and

No on 32-bit systems, size_t is 32 bits.

Thanks for the comment, I'll send v2.

Quoted 14 lines
Quoted 2 lines

instead gets a 32-bit variable, because the numeric constant size is a 32-bit.

Most of the patches of this series say "constant" where it is a variable.

Quoted 4 lines

In case the shift is greater than 32 the variable will lose its value even though the function can get 64-bit argument.

Change the size of the numeric constant 1 to 64-bit.

[...]

Quoted 2 lines
drivers/compress/mlx5/mlx5_compress.c +1 -1
-     opaq_buf = rte_calloc(__func__, 1u << log_ops_n,
+     opaq_buf = rte_calloc(__func__, RTE_BIT64(log_ops_n),
author
#5 lore raw

PATCH 3/4 vdpa/mlx5: fix constant type in QP creation

In reply to Michael Baum

The mlx5_vdpa_event_qp_create function makes shifting to the numeric constant 1, then multiplies it by another constant and finally assigns it into a uint64_t variable.

The numeric constant type is an int with a 32-bit sign. if after shifting , its MSB (bit of sign) will change, the uint64 variable will get into it a different value than what the function intended it to get.

Set the numeric constant 1 to be uint64_t in the first place.

Fixes 8395927cdfaf ("vdpa/mlx5: prepare HW queues")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
---
 drivers/vdpa/mlx5/mlx5_vdpa_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/vdpa/mlx5/mlx5_vdpa_event.c +1 -1
index 88f6a4256d..3541c652ce 100644
@@ -629,8 +629,8 @@ mlx5_vdpa_event_qp_create(struct mlx5_vdpa_priv *priv, uint16_t desc_n,
 	attr.wq_umem_id = eqp->umem_obj->umem_id;
 	attr.wq_umem_offset = 0;
 	attr.dbr_umem_id = eqp->umem_obj->umem_id;
-	attr.dbr_address = (1 << log_desc_n) * MLX5_WSEG_SIZE;
 	attr.ts_format = mlx5_ts_format_conv(priv->qp_ts_format);
+	attr.dbr_address = RTE_BIT64(log_desc_n) * MLX5_WSEG_SIZE;
 	eqp->sw_qp = mlx5_devx_cmd_create_qp(priv->ctx, &attr);
 	if (!eqp->sw_qp) {
 		DRV_LOG(ERR, "Failed to create SW QP(%u).", rte_errno);
Signature · 1 line
2.25.1
Acked-by on v1 3/4
#6 lore raw

Re: [dpdk-dev] [PATCH 3/4] vdpa/mlx5: fix constant type in QP creation

From: Michael Baum:

Quoted 8 lines

The mlx5_vdpa_event_qp_create function makes shifting to the numeric constant 1, then multiplies it by another constant and finally assigns it into a uint64_t variable.

The numeric constant type is an int with a 32-bit sign. if after shifting , its MSB (bit of sign) will change, the uint64 variable will get into it a different value than what the function intended it to get.

Quoted 6 lines

Set the numeric constant 1 to be uint64_t in the first place.

Fixes 8395927cdfaf ("vdpa/mlx5: prepare HW queues")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>
author
#7 lore raw

PATCH 4/4 net/mlx5: fix constant type in MP allocation

In reply to Michael Baum

The mlx5_mprq_alloc_mp function makes shifting to the numeric constant 1, for sending it as a parameter to rte_mempool_create function.

The rte_mempool_create function expects to get void pointer (64 bits, uintptr_t) and instead gets a 32-bit variable, because the numeric constant size is a 32-bit.
In case the shift is greater than 32 the variable will lose its value even though the function can get 64-bit argument.

Change the size of the numeric constant 1 to 64-bit.

Fixes 3a22f3877c9d ("net/mlx5: replace external mbuf shared memory")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
---
 drivers/net/mlx5/mlx5_rxq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/mlx5/mlx5_rxq.c +1 -1
index bb9a908087..950f327f03 100644
@@ -1240,7 +1240,7 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
 	snprintf(name, sizeof(name), "port-%u-mprq", dev->data->port_id);
 	mp = rte_mempool_create(name, obj_num, obj_size, MLX5_MPRQ_MP_CACHE_SZ,
 				0, NULL, NULL, mlx5_mprq_buf_init,
-				(void *)(uintptr_t)(1 << strd_num_n),
+				(void *)(uintptr_t)RTE_BIT64(strd_num_n),
 				dev->device->numa_node, 0);
 	if (mp == NULL) {
 		DRV_LOG(ERR,
Signature · 1 line
2.25.1
Acked-by on v1 4/4
#8 lore raw

Re: [dpdk-dev] [PATCH 4/4] net/mlx5: fix constant type in MP allocation

From: Michael Baum:

Quoted 9 lines

The mlx5_mprq_alloc_mp function makes shifting to the numeric constant 1, for sending it as a parameter to rte_mempool_create function.

The rte_mempool_create function expects to get void pointer (64 bits, uintptr_t) and instead gets a 32-bit variable, because the numeric constant size is a 32-bit.
In case the shift is greater than 32 the variable will lose its value even though the function can get 64-bit argument.

Quoted 6 lines

Change the size of the numeric constant 1 to 64-bit.

Fixes 3a22f3877c9d ("net/mlx5: replace external mbuf shared memory")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>
Acked-by on v1 1/4
#9 lore raw
In reply to Michael Baum

From: Michael Baum

Quoted 16 lines

Sent: Tuesday, June 1, 2021 10:11 AM
To: [email protected]
Cc: Matan Azrad <[email protected]>; Raslan Darawsheh <[email protected]>; Slava Ovsiienko <[email protected]>; [email protected]
Subject: [PATCH 1/4] regex/mlx5: fix size of setup constants

The constant representing the size of the metadata is defined as a unsigned int variable with 32-bit.
Similarly the constant representing the maximal output is also defined as a unsigned int variable with 32-bit.

There is potentially overflowing expression when those constants are evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type size_t (64 bits, unsigned).

Quoted 6 lines

Change the size of the above constants to 64-bit.

Fixes 30d604bb1504 ("regex/mlx5: fix type of setup constants")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>
author
#10 lore raw

PATCH v2 1/4 regex/mlx5: fix size of setup constants

In reply to Michael Baum

The constant representing the size of the metadata is defined as a unsigned int variable with 32-bit.
Similarly the constant representing the maximal output is also defined as a unsigned int variable with 32-bit.

There is potentially overflowing expression when those constants are evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type size_t that might be 64 bit.

Change the size of the above constants to size_t.

Fixes 30d604bb1504 ("regex/mlx5: fix type of setup constants")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>

---
v2: use size_t and uintptr_t instead of uint64_t.

 drivers/regex/mlx5/mlx5_regex_fastpath.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/regex/mlx5/mlx5_regex_fastpath.c +2 -2
index b57e7d7794..910bc845f3 100644
@@ -25,8 +25,8 @@
 #include "mlx5_regex.h"
 
 #define MLX5_REGEX_MAX_WQE_INDEX 0xffff
-#define MLX5_REGEX_METADATA_SIZE UINT32_C(64)
-#define MLX5_REGEX_MAX_OUTPUT RTE_BIT32(11)
+#define MLX5_REGEX_METADATA_SIZE ((size_t)64)
+#define MLX5_REGEX_MAX_OUTPUT (((size_t)1) << 11)
 #define MLX5_REGEX_WQE_CTRL_OFFSET 12
 #define MLX5_REGEX_WQE_METADATA_OFFSET 16
 #define MLX5_REGEX_WQE_GATHER_OFFSET 32
Signature · 1 line
2.25.1
author
#11 lore raw

PATCH v2 2/4 compress/mlx5: fix constant size in QP creation

The mlx5_compress_qp_setup function makes shifting to the numeric constant 1, then sends it as a parameter to rte_calloc function.

The rte_calloc function expects to get size_t (might be 64 bit) and instead gets a 32-bit variable, because the numeric constant size is a 32-bit.
In case the shift is greater than 32 bit and it 64-system, the variable will lose its value even though the function can get 64-bit argument.

Change the size of the numeric constant 1 to size_t.

Fixes 8619fcd5161b ("compress/mlx5: support queue pair operations")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>
---
 drivers/compress/mlx5/mlx5_compress.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/compress/mlx5/mlx5_compress.c +1 -1
index 80c564f10b..f09f71ecd6 100644
@@ -209,7 +209,7 @@ mlx5_compress_qp_setup(struct rte_compressdev *dev, uint16_t qp_id,
 		return -rte_errno;
 	}
 	dev->data->queue_pairs[qp_id] = qp;
-	opaq_buf = rte_calloc(__func__, 1u << log_ops_n,
+	opaq_buf = rte_calloc(__func__, (size_t)1 << log_ops_n,
 			      sizeof(struct mlx5_gga_compress_opaque),
 			      sizeof(struct mlx5_gga_compress_opaque));
 	if (opaq_buf == NULL) {
Signature · 1 line
2.25.1
author
#12 lore raw

PATCH v2 3/4 vdpa/mlx5: fix constant type in QP creation

In reply to Michael Baum

The mlx5_vdpa_event_qp_create function makes shifting to the numeric constant 1, then multiplies it by another constant and finally assigns it into a uint64_t variable.

The numeric constant type is an int with a 32-bit sign. if after shifting , its MSB (bit of sign) will change, the uint64 variable will get into it a different value than what the function intended it to get.

Set the numeric constant 1 to be uint64_t in the first place.

Fixes 8395927cdfaf ("vdpa/mlx5: prepare HW queues")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>
---
 drivers/vdpa/mlx5/mlx5_vdpa_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/vdpa/mlx5/mlx5_vdpa_event.c +1 -1
index 88f6a4256d..3541c652ce 100644
@@ -629,8 +629,8 @@ mlx5_vdpa_event_qp_create(struct mlx5_vdpa_priv *priv, uint16_t desc_n,
 	attr.wq_umem_id = eqp->umem_obj->umem_id;
 	attr.wq_umem_offset = 0;
 	attr.dbr_umem_id = eqp->umem_obj->umem_id;
-	attr.dbr_address = (1 << log_desc_n) * MLX5_WSEG_SIZE;
 	attr.ts_format = mlx5_ts_format_conv(priv->qp_ts_format);
+	attr.dbr_address = RTE_BIT64(log_desc_n) * MLX5_WSEG_SIZE;
 	eqp->sw_qp = mlx5_devx_cmd_create_qp(priv->ctx, &attr);
 	if (!eqp->sw_qp) {
 		DRV_LOG(ERR, "Failed to create SW QP(%u).", rte_errno);
Signature · 1 line
2.25.1
author
#13 lore raw

PATCH v2 4/4 net/mlx5: fix constant type in MP allocation

In reply to Michael Baum

The mlx5_mprq_alloc_mp function makes shifting to the numeric constant 1, for sending it as a parameter to rte_mempool_create function.

The rte_mempool_create function expects to get void pointer (uintptr_t, might be 64-bit) and instead gets a 32-bit variable, because the numeric constant size is a 32-bit.
In case the shift is greater than 32 the variable might lose its value even though the function might get 64-bit argument.

Change the size of the numeric constant 1 to uintptr_t.

Fixes 3a22f3877c9d ("net/mlx5: replace external mbuf shared memory")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>
---
 drivers/net/mlx5/mlx5_rxq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/mlx5/mlx5_rxq.c +1 -1
index bb9a908087..83ee8ddb9c 100644
@@ -1240,7 +1240,7 @@ mlx5_mprq_alloc_mp(struct rte_eth_dev *dev)
 	snprintf(name, sizeof(name), "port-%u-mprq", dev->data->port_id);
 	mp = rte_mempool_create(name, obj_num, obj_size, MLX5_MPRQ_MP_CACHE_SZ,
 				0, NULL, NULL, mlx5_mprq_buf_init,
-				(void *)(uintptr_t)(1 << strd_num_n),
+				(void *)((uintptr_t)1 << strd_num_n),
 				dev->device->numa_node, 0);
 	if (mp == NULL) {
 		DRV_LOG(ERR,
Signature · 1 line
2.25.1
#14 lore raw
In reply to Michael Baum

01/07/2021 08:39, Michael Baum:

Quoted 9 lines

The constant representing the size of the metadata is defined as a unsigned int variable with 32-bit.
Similarly the constant representing the maximal output is also defined as a unsigned int variable with 32-bit.

There is potentially overflowing expression when those constants are evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type size_t that might be 64 bit.

Quoted 7 lines

Change the size of the above constants to size_t.

Fixes 30d604bb1504 ("regex/mlx5: fix type of setup constants")
Cc [email protected]
Signed-off-by Michael Baum <[email protected]>
Acked-by Matan Azrad <[email protected]>

Series applied, thanks.