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