[dpdk-stable] [PATCH] net/mlx4: fix minor resource leak during init

[Yongseok Koh]

> On Aug 15, 2018, at 10:04 AM, Mordechay Haimovsky <motih at mellanox.com> wrote:
> 
> From: Adrien Mazarguil <adrien.mazarguil at 6wind.com>
> 
> Temporary IB device context and list are not freed in case of a successful
> initialization of the device.
> 
> This issue is caused by the two following commits, the first of which
> causes initialization to return early, while the second one goes a bit
> overboard while switching to negative errno values; an internal variable
> (err) is needed to tell success from failure at the end of the function
> since rte_errno is not reliable enough.
> 
> Fixes: f2318196c71a ("net/mlx4: remove limitation on number of instances")
> Fixes: 9d14b27308a0 ("net/mlx4: standardize on negative errno values")
> Cc: stable at dpdk.org
> 
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil at 6wind.com>
> Signed-off-by: Moti Haimovsky <motih at mellanox.com>
> ---
> drivers/net/mlx4/mlx4.c | 35 ++++++++++++++++++-----------------
> 1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/mlx4/mlx4.c b/drivers/net/mlx4/mlx4.c
> index f5c450d..118edd9 100644
> --- a/drivers/net/mlx4/mlx4.c
> +++ b/drivers/net/mlx4/mlx4.c
> @@ -494,14 +494,14 @@ struct mlx4_conf {
> 	ibv_dev = list[i];
> 	DEBUG("device opened");
> 	if (ibv_query_device(attr_ctx, &device_attr)) {
> -		rte_errno = ENODEV;
> +		err = ENODEV;
> 		goto error;
> 	}
> 	INFO("%u port(s) detected", device_attr.phys_port_cnt);
> 	conf.ports.present |= (UINT64_C(1) << device_attr.phys_port_cnt) - 1;
> 	if (mlx4_args(pci_dev->device.devargs, &conf)) {
> 		ERROR("failed to process device arguments");
> -		rte_errno = EINVAL;
> +		err = EINVAL;
> 		goto error;
> 	}
> 	/* Use all ports when none are defined */
> @@ -522,18 +522,18 @@ struct mlx4_conf {
> 		DEBUG("using port %u", port);
> 		ctx = ibv_open_device(ibv_dev);
> 		if (ctx == NULL) {
> -			rte_errno = ENODEV;
> +			err = ENODEV;
> 			goto port_error;
> 		}
> 		/* Check port status. */
> 		err = ibv_query_port(ctx, port, &port_attr);
> 		if (err) {
> -			rte_errno = err;
> -			ERROR("port query failed: %s", strerror(rte_errno));
> +			err = ENODEV;
> +			ERROR("port query failed: %s", strerror(err));
> 			goto port_error;
> 		}
> 		if (port_attr.link_layer != IBV_LINK_LAYER_ETHERNET) {
> -			rte_errno = ENOTSUP;
> +			err = ENOTSUP;
> 			ERROR("port %d is not configured in Ethernet mode",
> 			      port);
> 			goto port_error;
> @@ -543,15 +543,16 @@ struct mlx4_conf {
> 			      port, ibv_port_state_str(port_attr.state),
> 			      port_attr.state);
> 		/* Make asynchronous FD non-blocking to handle interrupts. */
> -		if (mlx4_fd_set_non_blocking(ctx->async_fd) < 0) {
> +		err = mlx4_fd_set_non_blocking(ctx->async_fd);
> +		if (err) {
> 			ERROR("cannot make asynchronous FD non-blocking: %s",
> -			      strerror(rte_errno));
> +			      strerror(err));
> 			goto port_error;
> 		}
> 		/* Allocate protection domain. */
> 		pd = ibv_alloc_pd(ctx);
> 		if (pd == NULL) {
> -			rte_errno = ENOMEM;
> +			err = ENOMEM;
> 			ERROR("PD allocation failure");
> 			goto port_error;
> 		}
> @@ -560,7 +561,7 @@ struct mlx4_conf {
> 				   sizeof(*priv),
> 				   RTE_CACHE_LINE_SIZE);
> 		if (priv == NULL) {
> -			rte_errno = ENOMEM;
> +			err = ENOMEM;
> 			ERROR("priv allocation failure");
> 			goto port_error;
> 		}
> @@ -585,9 +586,10 @@ struct mlx4_conf {
> 			device_attr_ex.rss_caps.max_rwq_indirection_table_size;
> 		DEBUG("MAX RSS queues %d", priv->hw_rss_max_qps);
> 		/* Configure the first MAC address by default. */
> -		if (mlx4_get_mac(priv, &mac.addr_bytes)) {
> +		err = mlx4_get_mac(priv, &mac.addr_bytes);
> +		if (err) {
> 			ERROR("cannot get MAC address, is mlx4_en loaded?"
> -			      " (rte_errno: %s)", strerror(rte_errno));
> +			      " (error: %s)", strerror(err));
> 			goto port_error;
> 		}
> 		INFO("port %u MAC address is %02x:%02x:%02x:%02x:%02x:%02x",

Thanks for your volunteer.
But, this hunk isn't applied cleanly.

Please rebase it on top of the latest stable/17.11.

Thanks,
Yongseok

> @@ -620,8 +622,8 @@ struct mlx4_conf {
> 			eth_dev = rte_eth_dev_allocate(name);
> 		}
> 		if (eth_dev == NULL) {
> +			err = ENOMEM;
> 			ERROR("can not allocate rte ethdev");
> -			rte_errno = ENOMEM;
> 			goto port_error;
> 		}
> 		eth_dev->data->dev_private = priv;
> @@ -666,8 +668,6 @@ struct mlx4_conf {
> 			rte_eth_dev_release_port(eth_dev);
> 		break;
> 	}
> -	if (i == device_attr.phys_port_cnt)
> -		return 0;
> 	/*
> 	 * XXX if something went wrong in the loop above, there is a resource
> 	 * leak (ctx, pd, priv, dpdk ethdev) but we can do nothing about it as
> @@ -679,8 +679,9 @@ struct mlx4_conf {
> 		claim_zero(ibv_close_device(attr_ctx));
> 	if (list)
> 		ibv_free_device_list(list);
> -	assert(rte_errno >= 0);
> -	return -rte_errno;
> +	if (err)
> +		rte_errno = err;
> +	return -err;
> }
> 
> static const struct rte_pci_id mlx4_pci_id_map[] = {
> -- 
> 1.8.3.1
>