* [dpdk-stable] [PATCH] net/mlx4: fix minor resource leak during init
@ 2018-08-15 17:04 Moti Haimovsky
2018-08-15 17:27 ` Yongseok Koh
2018-08-16 17:56 ` Yongseok Koh
0 siblings, 2 replies; 3+ messages in thread
From: Moti Haimovsky @ 2018-08-15 17:04 UTC (permalink / raw)
To: yskoh; +Cc: Adrien Mazarguil, stable, Moti Haimovsky
From: Adrien Mazarguil <adrien.mazarguil@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@dpdk.org
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
Signed-off-by: Moti Haimovsky <motih@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",
@@ -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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-stable] [PATCH] net/mlx4: fix minor resource leak during init
2018-08-15 17:04 [dpdk-stable] [PATCH] net/mlx4: fix minor resource leak during init Moti Haimovsky
@ 2018-08-15 17:27 ` Yongseok Koh
2018-08-16 17:56 ` Yongseok Koh
1 sibling, 0 replies; 3+ messages in thread
From: Yongseok Koh @ 2018-08-15 17:27 UTC (permalink / raw)
To: Mordechay Haimovsky; +Cc: Adrien Mazarguil, stable
> On Aug 15, 2018, at 10:04 AM, Mordechay Haimovsky <motih@mellanox.com> wrote:
>
> From: Adrien Mazarguil <adrien.mazarguil@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@dpdk.org
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Signed-off-by: Moti Haimovsky <motih@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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-stable] [PATCH] net/mlx4: fix minor resource leak during init
2018-08-15 17:04 [dpdk-stable] [PATCH] net/mlx4: fix minor resource leak during init Moti Haimovsky
2018-08-15 17:27 ` Yongseok Koh
@ 2018-08-16 17:56 ` Yongseok Koh
1 sibling, 0 replies; 3+ messages in thread
From: Yongseok Koh @ 2018-08-16 17:56 UTC (permalink / raw)
To: Mordechay Haimovsky; +Cc: Adrien Mazarguil, stable
> On Aug 15, 2018, at 10:04 AM, Mordechay Haimovsky <motih@mellanox.com> wrote:
>
> From: Adrien Mazarguil <adrien.mazarguil@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@dpdk.org
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> ---
Applied to stable/17.11
Thanks,
Yongseok
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-16 17:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-15 17:04 [dpdk-stable] [PATCH] net/mlx4: fix minor resource leak during init Moti Haimovsky
2018-08-15 17:27 ` Yongseok Koh
2018-08-16 17:56 ` Yongseok Koh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).