patches for DPDK stable branches
 help / color / Atom feed
* [dpdk-stable] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow
@ 2020-05-27  8:37 Michael Baum
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 2/6] net/mlx5: fix hairpin Rx " Michael Baum
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Michael Baum @ 2020-05-27  8:37 UTC (permalink / raw)
  To: dev; +Cc: matan, viacheslavo, stable

The mlx5_txq_obj_hairpin_new function defines a pointer named tmpl and
allocates memory for it using the rte_zmalloc_socket function.
Later, this function allocates memory to a variable inside tmpl using
the mlx5_devx_cmd_create_sq function.

In both cases, if the allocation fails, the code jumps to the error
label and frees allocated resources. However, in the first jump there
are still no resources to free and the jump only for the line return
NULL is unnecessary. Even worse, when it jumps to error label with
invalid tmpl it actually does dereference to a null pointer.
In contrast, the second jump needs to free the tmpl variable but the
function instead of freeing, tries to free the variable that it just
failed to allocate, and another variable that has never been allocated.
In addition, for another error, the function returns NULL without
freeing the tmpl variable before, causing a memory leak.

Delete the error label and replace each jump with local return NULL and
free tmpl variable if needed.

Fixes: ae18a1ae9692 ("net/mlx5: support Tx hairpin queues")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5_txq.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
index a211fa9..7cc620a 100644
--- a/drivers/net/mlx5/mlx5_txq.c
+++ b/drivers/net/mlx5/mlx5_txq.c
@@ -493,7 +493,6 @@
 		container_of(txq_data, struct mlx5_txq_ctrl, txq);
 	struct mlx5_devx_create_sq_attr attr = { 0 };
 	struct mlx5_txq_obj *tmpl = NULL;
-	int ret = 0;
 	uint32_t max_wq_data;
 
 	MLX5_ASSERT(txq_data);
@@ -505,7 +504,7 @@
 			"port %u Tx queue %u cannot allocate memory resources",
 			dev->data->port_id, txq_data->idx);
 		rte_errno = ENOMEM;
-		goto error;
+		return NULL;
 	}
 	tmpl->type = MLX5_TXQ_OBJ_TYPE_DEVX_HAIRPIN;
 	tmpl->txq_ctrl = txq_ctrl;
@@ -518,6 +517,7 @@
 			DRV_LOG(ERR, "total data size %u power of 2 is "
 				"too large for hairpin",
 				priv->config.log_hp_size);
+			rte_free(tmpl);
 			rte_errno = ERANGE;
 			return NULL;
 		}
@@ -537,22 +537,15 @@
 		DRV_LOG(ERR,
 			"port %u tx hairpin queue %u can't create sq object",
 			dev->data->port_id, idx);
+		rte_free(tmpl);
 		rte_errno = errno;
-		goto error;
+		return NULL;
 	}
 	DRV_LOG(DEBUG, "port %u sxq %u updated with %p", dev->data->port_id,
 		idx, (void *)&tmpl);
 	rte_atomic32_inc(&tmpl->refcnt);
 	LIST_INSERT_HEAD(&priv->txqsobj, tmpl, next);
 	return tmpl;
-error:
-	ret = rte_errno; /* Save rte_errno before cleanup. */
-	if (tmpl->tis)
-		mlx5_devx_cmd_destroy(tmpl->tis);
-	if (tmpl->sq)
-		mlx5_devx_cmd_destroy(tmpl->sq);
-	rte_errno = ret; /* Restore rte_errno. */
-	return NULL;
 }
 
 /**
-- 
1.8.3.1


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

* [dpdk-stable] [PATCH 2/6] net/mlx5: fix hairpin Rx queue creation error flow
  2020-05-27  8:37 [dpdk-stable] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Michael Baum
@ 2020-05-27  8:37 ` " Michael Baum
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 3/6] net/mlx5: fix unnecessary init in socket handle Michael Baum
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Michael Baum @ 2020-05-27  8:37 UTC (permalink / raw)
  To: dev; +Cc: matan, viacheslavo, stable

The mlx5_rxq_obj_hairpin_new function defines a pointer named tmpl and
allocates memory for it using the rte_zmalloc_socket function.
Later, this function allocates memory to a variable inside tmpl using
the mlx5_devx_cmd_create_rq function.

In both cases, if the allocation fails, the code jumps to the error
label and frees allocated resources. However, in the first jump there
are still no resources to free and the jump only for the line return
NULL is unnecessary. Even worse, when it jumps to error label with
invalid tmpl it actually does dereference to a null pointer.
In contrast, the second jump needs to free the tmpl variable but the
function instead of freeing, tries to free the variable that it just
failed to allocate.
In addition, for another error, the function returns NULL without
freeing the tmpl variable before, causing a memory leak.

Delete the error label and replace each jump with local return NULL and
free tmpl variable if needed.

Fixes: e79c9be91515 ("net/mlx5: support Rx hairpin queues")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5_rxq.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index 7a50ec6..0b0abe1 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1267,7 +1267,6 @@
 		container_of(rxq_data, struct mlx5_rxq_ctrl, rxq);
 	struct mlx5_devx_create_rq_attr attr = { 0 };
 	struct mlx5_rxq_obj *tmpl = NULL;
-	int ret = 0;
 	uint32_t max_wq_data;
 
 	MLX5_ASSERT(rxq_data);
@@ -1279,7 +1278,7 @@
 			"port %u Rx queue %u cannot allocate verbs resources",
 			dev->data->port_id, rxq_data->idx);
 		rte_errno = ENOMEM;
-		goto error;
+		return NULL;
 	}
 	tmpl->type = MLX5_RXQ_OBJ_TYPE_DEVX_HAIRPIN;
 	tmpl->rxq_ctrl = rxq_ctrl;
@@ -1291,6 +1290,7 @@
 			DRV_LOG(ERR, "total data size %u power of 2 is "
 				"too large for hairpin",
 				priv->config.log_hp_size);
+			rte_free(tmpl);
 			rte_errno = ERANGE;
 			return NULL;
 		}
@@ -1310,8 +1310,9 @@
 		DRV_LOG(ERR,
 			"port %u Rx hairpin queue %u can't create rq object",
 			dev->data->port_id, idx);
+		rte_free(tmpl);
 		rte_errno = errno;
-		goto error;
+		return NULL;
 	}
 	DRV_LOG(DEBUG, "port %u rxq %u updated with %p", dev->data->port_id,
 		idx, (void *)&tmpl);
@@ -1319,12 +1320,6 @@
 	LIST_INSERT_HEAD(&priv->rxqsobj, tmpl, next);
 	priv->verbs_alloc_ctx.type = MLX5_VERBS_ALLOC_TYPE_NONE;
 	return tmpl;
-error:
-	ret = rte_errno; /* Save rte_errno before cleanup. */
-	if (tmpl->rq)
-		mlx5_devx_cmd_destroy(tmpl->rq);
-	rte_errno = ret; /* Restore rte_errno. */
-	return NULL;
 }
 
 /**
-- 
1.8.3.1


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

* [dpdk-stable] [PATCH 3/6] net/mlx5: fix unnecessary init in socket handle
  2020-05-27  8:37 [dpdk-stable] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Michael Baum
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 2/6] net/mlx5: fix hairpin Rx " Michael Baum
@ 2020-05-27  8:37 ` Michael Baum
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 4/6] net/mlx5: fix socket handle closing Michael Baum
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Michael Baum @ 2020-05-27  8:37 UTC (permalink / raw)
  To: dev; +Cc: matan, viacheslavo, stable

In the mlx5_pmd_socket_handle function it calls the recvmsg function
which returns the number of bytes read. The function assigns this return
value into a ret variable defined at the beginning of the function.
Similarly in the mlx5_pmd_socket_init function the it calls the socket
function which returns a file descriptor for the new socket. The
function also assigns this return value into a ret variable defined at
the beginning of the function.

In both functions they initialize the variable when defining it,
however, in both cases they do not use any ret variable before assigning
the return value from the function, so the initialization is
unnecessary.

Clean the aforementioned unnecessary initializations.

Fixes: e6cdc54cc0ef ("net/mlx5: add socket server for external tools")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5_socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/mlx5/mlx5_socket.c b/drivers/net/mlx5/mlx5_socket.c
index a79896c..f473795 100644
--- a/drivers/net/mlx5/mlx5_socket.c
+++ b/drivers/net/mlx5/mlx5_socket.c
@@ -37,7 +37,7 @@
 mlx5_pmd_socket_handle(void *cb __rte_unused)
 {
 	int conn_sock;
-	int ret = -1;
+	int ret;
 	struct cmsghdr *cmsg = NULL;
 	int data;
 	char buf[CMSG_SPACE(sizeof(int))] = { 0 };
@@ -163,7 +163,7 @@
 	struct sockaddr_un sun = {
 		.sun_family = AF_UNIX,
 	};
-	int ret = -1;
+	int ret;
 	int flags;
 
 	MLX5_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
-- 
1.8.3.1


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

* [dpdk-stable] [PATCH 4/6] net/mlx5: fix socket handle closing
  2020-05-27  8:37 [dpdk-stable] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Michael Baum
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 2/6] net/mlx5: fix hairpin Rx " Michael Baum
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 3/6] net/mlx5: fix unnecessary init in socket handle Michael Baum
@ 2020-05-27  8:37 ` Michael Baum
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 5/6] net/mlx5: fix needless txq initialization check Michael Baum
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Michael Baum @ 2020-05-27  8:37 UTC (permalink / raw)
  To: dev; +Cc: matan, viacheslavo, stable

The mlx5_pmd_socket_handle function calls the accept function that
returns the socket descriptor into the conn_sock variable. The socket
descriptor value can be 0 (according to accept API) or positive and so
immediately after calling the function it checks whether conn_sock < 0.
Later in the function when other things fail it jumps to the error label
and release previously allocated resources (such as socket or file).

During the resource release, it checks whether the variable conn_sock
containing the socket descriptor is positive and if it is, it releases
it. However, in this check it misses the case where conn_sock == 0, in
this case the socket will not be released and there will be a Resource
leak.

Extend the close condition for 0 value too.

Fixes: e6cdc54cc0ef ("net/mlx5: add socket server for external tools")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5_socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_socket.c b/drivers/net/mlx5/mlx5_socket.c
index f473795..08af905 100644
--- a/drivers/net/mlx5/mlx5_socket.c
+++ b/drivers/net/mlx5/mlx5_socket.c
@@ -109,7 +109,7 @@
 		DRV_LOG(WARNING, "failed to send response %s",
 			strerror(errno));
 error:
-	if (conn_sock > 0)
+	if (conn_sock >= 0)
 		close(conn_sock);
 	if (file)
 		fclose(file);
-- 
1.8.3.1


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

* [dpdk-stable] [PATCH 5/6] net/mlx5: fix needless txq initialization check
  2020-05-27  8:37 [dpdk-stable] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Michael Baum
                   ` (2 preceding siblings ...)
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 4/6] net/mlx5: fix socket handle closing Michael Baum
@ 2020-05-27  8:37 ` Michael Baum
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 6/6] net/mlx5: fix compiler code arrangement in MPLS support Michael Baum
  2020-05-31 11:26 ` [dpdk-stable] [dpdk-dev] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Raslan Darawsheh
  5 siblings, 0 replies; 7+ messages in thread
From: Michael Baum @ 2020-05-27  8:37 UTC (permalink / raw)
  To: dev; +Cc: matan, viacheslavo, stable

The mlx5_txq_obj_new function defines a pointer named txq_data and
assign value into it. After assigning, the code writer is sure that the
variable does not point to NULL and even express it using assertion.

During the function, the function does dereferencing to the pointer
several times and at no point change its value. However, at the end of
the function at the error label when it wants to free one of the fields
of the structure that txq_data points to, it checks again whether
txq_data is invalid.
This check is unnecessary since it knows for sure that txq_data is
valid.

Remove the aforementioned needless check.

Fixes: 644906881881 ("net/mlx5: add free on completion queue")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5_txq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_txq.c b/drivers/net/mlx5/mlx5_txq.c
index 7cc620a..80d99ff 100644
--- a/drivers/net/mlx5/mlx5_txq.c
+++ b/drivers/net/mlx5/mlx5_txq.c
@@ -793,7 +793,7 @@ struct mlx5_txq_obj *
 		claim_zero(mlx5_glue->destroy_cq(tmpl.cq));
 	if (tmpl.qp)
 		claim_zero(mlx5_glue->destroy_qp(tmpl.qp));
-	if (txq_data && txq_data->fcqs)
+	if (txq_data->fcqs)
 		rte_free(txq_data->fcqs);
 	if (txq_obj)
 		rte_free(txq_obj);
-- 
1.8.3.1


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

* [dpdk-stable] [PATCH 6/6] net/mlx5: fix compiler code arrangement in MPLS support
  2020-05-27  8:37 [dpdk-stable] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Michael Baum
                   ` (3 preceding siblings ...)
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 5/6] net/mlx5: fix needless txq initialization check Michael Baum
@ 2020-05-27  8:37 ` Michael Baum
  2020-05-31 11:26 ` [dpdk-stable] [dpdk-dev] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Raslan Darawsheh
  5 siblings, 0 replies; 7+ messages in thread
From: Michael Baum @ 2020-05-27  8:37 UTC (permalink / raw)
  To: dev; +Cc: matan, viacheslavo, stable

The mlx5_flow_validate_item_mpls function checks MPLS item validation.
It first checks if the device supports MPLS, it is done using the ifdef
condition that if it fails to skip to endif and return the appropriate
error.

When MPLS is supported, the preprocessor will copy the body of the
function ending with return 0 followed by the lines that report MPLS
support.
In fact, these lines are unreachable because before them the function
returns 0 and in any case they are unnecessary.

Replace the endif by else and move endif to the end of the
function.

Fixes: 23c1d42c7138 ("net/mlx5: split flow validation to dedicated function")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
 drivers/net/mlx5/mlx5_flow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_flow.c b/drivers/net/mlx5/mlx5_flow.c
index ae478a5..f2c3cf9 100644
--- a/drivers/net/mlx5/mlx5_flow.c
+++ b/drivers/net/mlx5/mlx5_flow.c
@@ -2269,11 +2269,12 @@ uint32_t mlx5_flow_adjust_priority(struct rte_eth_dev *dev, int32_t priority,
 	if (ret < 0)
 		return ret;
 	return 0;
-#endif
+#else
 	return rte_flow_error_set(error, ENOTSUP,
 				  RTE_FLOW_ERROR_TYPE_ITEM, item,
 				  "MPLS is not supported by Verbs, please"
 				  " update.");
+#endif
 }
 
 /**
-- 
1.8.3.1


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

* Re: [dpdk-stable] [dpdk-dev] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow
  2020-05-27  8:37 [dpdk-stable] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Michael Baum
                   ` (4 preceding siblings ...)
  2020-05-27  8:37 ` [dpdk-stable] [PATCH 6/6] net/mlx5: fix compiler code arrangement in MPLS support Michael Baum
@ 2020-05-31 11:26 ` Raslan Darawsheh
  5 siblings, 0 replies; 7+ messages in thread
From: Raslan Darawsheh @ 2020-05-31 11:26 UTC (permalink / raw)
  To: Michael Baum, dev; +Cc: Matan Azrad, Slava Ovsiienko, stable

Hi,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Michael Baum
> Sent: Wednesday, May 27, 2020 11:38 AM
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Slava Ovsiienko
> <viacheslavo@mellanox.com>; stable@dpdk.org
> Subject: [dpdk-dev] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation
> error flow
> 
> The mlx5_txq_obj_hairpin_new function defines a pointer named tmpl and
> allocates memory for it using the rte_zmalloc_socket function.
> Later, this function allocates memory to a variable inside tmpl using
> the mlx5_devx_cmd_create_sq function.
> 
> In both cases, if the allocation fails, the code jumps to the error
> label and frees allocated resources. However, in the first jump there
> are still no resources to free and the jump only for the line return
> NULL is unnecessary. Even worse, when it jumps to error label with
> invalid tmpl it actually does dereference to a null pointer.
> In contrast, the second jump needs to free the tmpl variable but the
> function instead of freeing, tries to free the variable that it just
> failed to allocate, and another variable that has never been allocated.
> In addition, for another error, the function returns NULL without
> freeing the tmpl variable before, causing a memory leak.
> 
> Delete the error label and replace each jump with local return NULL and
> free tmpl variable if needed.
> 
> Fixes: ae18a1ae9692 ("net/mlx5: support Tx hairpin queues")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Michael Baum <michaelba@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/mlx5/mlx5_txq.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 

Series applied to next-net-mlx,

Kindest regards,
Raslan Darawsheh

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

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27  8:37 [dpdk-stable] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Michael Baum
2020-05-27  8:37 ` [dpdk-stable] [PATCH 2/6] net/mlx5: fix hairpin Rx " Michael Baum
2020-05-27  8:37 ` [dpdk-stable] [PATCH 3/6] net/mlx5: fix unnecessary init in socket handle Michael Baum
2020-05-27  8:37 ` [dpdk-stable] [PATCH 4/6] net/mlx5: fix socket handle closing Michael Baum
2020-05-27  8:37 ` [dpdk-stable] [PATCH 5/6] net/mlx5: fix needless txq initialization check Michael Baum
2020-05-27  8:37 ` [dpdk-stable] [PATCH 6/6] net/mlx5: fix compiler code arrangement in MPLS support Michael Baum
2020-05-31 11:26 ` [dpdk-stable] [dpdk-dev] [PATCH 1/6] net/mlx5: fix hairpin Tx queue creation error flow Raslan Darawsheh

patches for DPDK stable branches

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/stable/0 stable/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 stable stable/ http://inbox.dpdk.org/stable \
		stable@dpdk.org
	public-inbox-index stable


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.stable


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