DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v3 0/3] vhost: fix fd an memory leaks
@ 2020-11-09 12:16 Maxime Coquelin
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory tables Maxime Coquelin
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Maxime Coquelin @ 2020-11-09 12:16 UTC (permalink / raw)
  To: dev, xuan.ding, stephen, thomas, stable, chenbo.xia; +Cc: Maxime Coquelin

This series fixes several leaks in Vhost-user requests
handling.

Thanks to Xuan Ding from Intel for reporting these
issues.


Changes in v3:
- Remove the right closing of fds. (Chenbo)
Changes in v2:
- Fix typos in commit messages. (Chenbo)
- Remove useless closing of fds in set_log_base. (Chenbo)

Maxime Coquelin (3):
  vhost: fix error path when setting memory tables
  vhost: fix fd leak in dirty logging setup
  vhost: fix fd leak in kick setup

 lib/librte_vhost/vhost_user.c | 79 ++++++++++++++++++++++-------------
 1 file changed, 50 insertions(+), 29 deletions(-)

-- 
2.26.2


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

* [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory tables
  2020-11-09 12:16 [dpdk-dev] [PATCH v3 0/3] vhost: fix fd an memory leaks Maxime Coquelin
@ 2020-11-09 12:16 ` Maxime Coquelin
  2020-11-11  6:06   ` Xueming(Steven) Li
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup Maxime Coquelin
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup Maxime Coquelin
  2 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2020-11-09 12:16 UTC (permalink / raw)
  To: dev, xuan.ding, stephen, thomas, stable, chenbo.xia; +Cc: Maxime Coquelin

If an error is encountered before the memory regions are
parsed, the file descriptors for these shared buffers are
leaked.

This patch fixes this by closing the message file descriptors
on error, taking care of avoiding double closing of the file
descriptors. guest_pages is also freed, even though it was not
leaked as its pointer was not overridden on subsequent function
calls.

Fixes: 8f972312b8f4 ("vhost: support vhost-user")
Cc: stable@dpdk.org

Reported-by: Xuan Ding <xuan.ding@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
 lib/librte_vhost/vhost_user.c | 65 +++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 8a8726f8b8..473fd778ca 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -99,8 +99,15 @@ close_msg_fds(struct VhostUserMsg *msg)
 {
 	int i;
 
-	for (i = 0; i < msg->fd_num; i++)
-		close(msg->fds[i]);
+	for (i = 0; i < msg->fd_num; i++) {
+		int fd = msg->fds[i];
+
+		if (fd == -1)
+			continue;
+
+		msg->fds[i] = -1;
+		close(fd);
+	}
 }
 
 /*
@@ -1004,7 +1011,6 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	uint64_t alignment;
 	uint32_t i;
 	int populate;
-	int fd;
 
 	if (validate_msg_fds(msg, memory->nregions) != 0)
 		return RTE_VHOST_MSG_RESULT_ERR;
@@ -1012,16 +1018,13 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	if (memory->nregions > VHOST_MEMORY_MAX_NREGIONS) {
 		VHOST_LOG_CONFIG(ERR,
 			"too many memory regions (%u)\n", memory->nregions);
-		return RTE_VHOST_MSG_RESULT_ERR;
+		goto close_msg_fds;
 	}
 
 	if (dev->mem && !vhost_memory_changed(memory, dev->mem)) {
 		VHOST_LOG_CONFIG(INFO,
 			"(%d) memory regions not changed\n", dev->vid);
-
-		close_msg_fds(msg);
-
-		return RTE_VHOST_MSG_RESULT_OK;
+		goto close_msg_fds;
 	}
 
 	if (dev->mem) {
@@ -1054,7 +1057,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 				"(%d) failed to allocate memory "
 				"for dev->guest_pages\n",
 				dev->vid);
-			return RTE_VHOST_MSG_RESULT_ERR;
+			goto close_msg_fds;
 		}
 	}
 
@@ -1064,18 +1067,23 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 		VHOST_LOG_CONFIG(ERR,
 			"(%d) failed to allocate memory for dev->mem\n",
 			dev->vid);
-		return RTE_VHOST_MSG_RESULT_ERR;
+		goto free_guest_pages;
 	}
 	dev->mem->nregions = memory->nregions;
 
 	for (i = 0; i < memory->nregions; i++) {
-		fd  = msg->fds[i];
 		reg = &dev->mem->regions[i];
 
 		reg->guest_phys_addr = memory->regions[i].guest_phys_addr;
 		reg->guest_user_addr = memory->regions[i].userspace_addr;
 		reg->size            = memory->regions[i].memory_size;
-		reg->fd              = fd;
+		reg->fd              = msg->fds[i];
+
+		/*
+		 * Assign invalid file descriptor value to avoid double
+		 * closing on error path.
+		 */
+		msg->fds[i] = -1;
 
 		mmap_offset = memory->regions[i].mmap_offset;
 
@@ -1085,7 +1093,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 				"mmap_offset (%#"PRIx64") and memory_size "
 				"(%#"PRIx64") overflow\n",
 				mmap_offset, reg->size);
-			goto err_mmap;
+			goto free_mem_table;
 		}
 
 		mmap_size = reg->size + mmap_offset;
@@ -1098,11 +1106,11 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 		 * to avoid failure, make sure in caller to keep length
 		 * aligned.
 		 */
-		alignment = get_blk_size(fd);
+		alignment = get_blk_size(reg->fd);
 		if (alignment == (uint64_t)-1) {
 			VHOST_LOG_CONFIG(ERR,
 				"couldn't get hugepage size through fstat\n");
-			goto err_mmap;
+			goto free_mem_table;
 		}
 		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
 		if (mmap_size == 0) {
@@ -1118,17 +1126,17 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 			VHOST_LOG_CONFIG(ERR, "mmap size (0x%" PRIx64 ") "
 					"or alignment (0x%" PRIx64 ") is invalid\n",
 					reg->size + mmap_offset, alignment);
-			goto err_mmap;
+			goto free_mem_table;
 		}
 
 		populate = dev->async_copy ? MAP_POPULATE : 0;
 		mmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
-				 MAP_SHARED | populate, fd, 0);
+				 MAP_SHARED | populate, reg->fd, 0);
 
 		if (mmap_addr == MAP_FAILED) {
 			VHOST_LOG_CONFIG(ERR,
 				"mmap region %u failed.\n", i);
-			goto err_mmap;
+			goto free_mem_table;
 		}
 
 		reg->mmap_addr = mmap_addr;
@@ -1141,7 +1149,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 				VHOST_LOG_CONFIG(ERR,
 					"adding guest pages to region %u failed.\n",
 					i);
-				goto err_mmap;
+				goto free_mem_table;
 			}
 
 		VHOST_LOG_CONFIG(INFO,
@@ -1184,17 +1192,17 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 		if (read_vhost_message(main_fd, &ack_msg) <= 0) {
 			VHOST_LOG_CONFIG(ERR,
 				"Failed to read qemu ack on postcopy set-mem-table\n");
-			goto err_mmap;
+			goto free_mem_table;
 		}
 
 		if (validate_msg_fds(&ack_msg, 0) != 0)
-			goto err_mmap;
+			goto free_mem_table;
 
 		if (ack_msg.request.master != VHOST_USER_SET_MEM_TABLE) {
 			VHOST_LOG_CONFIG(ERR,
 				"Bad qemu ack on postcopy set-mem-table (%d)\n",
 				ack_msg.request.master);
-			goto err_mmap;
+			goto free_mem_table;
 		}
 
 		/* Now userfault register and we can use the memory */
@@ -1218,7 +1226,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 					"Failed to register ufd for region %d: (ufd = %d) %s\n",
 					i, dev->postcopy_ufd,
 					strerror(errno));
-				goto err_mmap;
+				goto free_mem_table;
 			}
 			VHOST_LOG_CONFIG(INFO,
 				"\t userfaultfd registered for range : "
@@ -1227,7 +1235,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 				(uint64_t)reg_struct.range.start +
 				(uint64_t)reg_struct.range.len - 1);
 #else
-			goto err_mmap;
+			goto free_mem_table;
 #endif
 		}
 	}
@@ -1249,7 +1257,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 			dev = translate_ring_addresses(dev, i);
 			if (!dev) {
 				dev = *pdev;
-				goto err_mmap;
+				goto free_mem_table;
 			}
 
 			*pdev = dev;
@@ -1260,10 +1268,15 @@ vhost_user_set_mem_table(struct virtio_net **pdev, struct VhostUserMsg *msg,
 
 	return RTE_VHOST_MSG_RESULT_OK;
 
-err_mmap:
+free_mem_table:
 	free_mem_region(dev);
 	rte_free(dev->mem);
 	dev->mem = NULL;
+free_guest_pages:
+	rte_free(dev->guest_pages);
+	dev->guest_pages = NULL;
+close_msg_fds:
+	close_msg_fds(msg);
 	return RTE_VHOST_MSG_RESULT_ERR;
 }
 
-- 
2.26.2


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

* [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup
  2020-11-09 12:16 [dpdk-dev] [PATCH v3 0/3] vhost: fix fd an memory leaks Maxime Coquelin
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory tables Maxime Coquelin
@ 2020-11-09 12:16 ` Maxime Coquelin
  2020-11-10  1:44   ` Xia, Chenbo
  2020-11-11  6:13   ` Xueming(Steven) Li
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup Maxime Coquelin
  2 siblings, 2 replies; 11+ messages in thread
From: Maxime Coquelin @ 2020-11-09 12:16 UTC (permalink / raw)
  To: dev, xuan.ding, stephen, thomas, stable, chenbo.xia; +Cc: Maxime Coquelin

This patch fixes a file descriptor leak which happens
in the error path of vhost_user_set_log_base().

Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
Cc: stable@dpdk.org

Reported-by: Xuan Ding <xuan.ding@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 473fd778ca..94b066f0b9 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2083,7 +2083,7 @@ vhost_user_set_log_base(struct virtio_net **pdev, struct VhostUserMsg *msg,
 		VHOST_LOG_CONFIG(ERR,
 			"invalid log base msg size: %"PRId32" != %d\n",
 			msg->size, (int)sizeof(VhostUserLog));
-		return RTE_VHOST_MSG_RESULT_ERR;
+		goto close_msg_fds;
 	}
 
 	size = msg->payload.log.mmap_size;
@@ -2094,7 +2094,7 @@ vhost_user_set_log_base(struct virtio_net **pdev, struct VhostUserMsg *msg,
 		VHOST_LOG_CONFIG(ERR,
 			"log offset %#"PRIx64" and log size %#"PRIx64" overflow\n",
 			off, size);
-		return RTE_VHOST_MSG_RESULT_ERR;
+		goto close_msg_fds;
 	}
 
 	VHOST_LOG_CONFIG(INFO,
@@ -2131,6 +2131,10 @@ vhost_user_set_log_base(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	msg->fd_num = 0;
 
 	return RTE_VHOST_MSG_RESULT_REPLY;
+
+close_msg_fds:
+	close_msg_fds(msg);
+	return RTE_VHOST_MSG_RESULT_ERR;
 }
 
 static int vhost_user_set_log_fd(struct virtio_net **pdev __rte_unused,
-- 
2.26.2


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

* [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
  2020-11-09 12:16 [dpdk-dev] [PATCH v3 0/3] vhost: fix fd an memory leaks Maxime Coquelin
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory tables Maxime Coquelin
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup Maxime Coquelin
@ 2020-11-09 12:16 ` Maxime Coquelin
  2020-11-11  6:01   ` Xueming(Steven) Li
  2 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2020-11-09 12:16 UTC (permalink / raw)
  To: dev, xuan.ding, stephen, thomas, stable, chenbo.xia; +Cc: Maxime Coquelin

This patch fixes a file descriptor leak which happens
in the error path of vhost_user_set_vring_kick().

Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
---
 lib/librte_vhost/vhost_user.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 94b066f0b9..f3b2adabac 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1855,8 +1855,12 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *msg,
 
 	/* Interpret ring addresses only when ring is started. */
 	dev = translate_ring_addresses(dev, file.index);
-	if (!dev)
+	if (!dev) {
+		if (file.fd != VIRTIO_INVALID_EVENTFD)
+			close(file.fd);
+
 		return RTE_VHOST_MSG_RESULT_ERR;
+	}
 
 	*pdev = dev;
 
-- 
2.26.2


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

* Re: [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup Maxime Coquelin
@ 2020-11-10  1:44   ` Xia, Chenbo
  2020-11-11  6:13   ` Xueming(Steven) Li
  1 sibling, 0 replies; 11+ messages in thread
From: Xia, Chenbo @ 2020-11-10  1:44 UTC (permalink / raw)
  To: Maxime Coquelin, dev, Ding, Xuan, stephen, thomas, stable

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, November 9, 2020 8:16 PM
> To: dev@dpdk.org; Ding, Xuan <xuan.ding@intel.com>;
> stephen@networkplumber.org; thomas@monjalon.net; stable@dpdk.org; Xia,
> Chenbo <chenbo.xia@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup
> 
> This patch fixes a file descriptor leak which happens
> in the error path of vhost_user_set_log_base().
> 
> Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
> Cc: stable@dpdk.org
> 
> Reported-by: Xuan Ding <xuan.ding@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost_user.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 473fd778ca..94b066f0b9 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -2083,7 +2083,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
>  		VHOST_LOG_CONFIG(ERR,
>  			"invalid log base msg size: %"PRId32" != %d\n",
>  			msg->size, (int)sizeof(VhostUserLog));
> -		return RTE_VHOST_MSG_RESULT_ERR;
> +		goto close_msg_fds;
>  	}
> 
>  	size = msg->payload.log.mmap_size;
> @@ -2094,7 +2094,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
>  		VHOST_LOG_CONFIG(ERR,
>  			"log offset %#"PRIx64" and log size %#"PRIx64"
> overflow\n",
>  			off, size);
> -		return RTE_VHOST_MSG_RESULT_ERR;
> +		goto close_msg_fds;
>  	}
> 
>  	VHOST_LOG_CONFIG(INFO,
> @@ -2131,6 +2131,10 @@ vhost_user_set_log_base(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
>  	msg->fd_num = 0;
> 
>  	return RTE_VHOST_MSG_RESULT_REPLY;
> +
> +close_msg_fds:
> +	close_msg_fds(msg);
> +	return RTE_VHOST_MSG_RESULT_ERR;
>  }
> 
>  static int vhost_user_set_log_fd(struct virtio_net **pdev __rte_unused,
> --
> 2.26.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

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

* Re: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup Maxime Coquelin
@ 2020-11-11  6:01   ` Xueming(Steven) Li
  2020-11-11  7:57     ` Xia, Chenbo
  0 siblings, 1 reply; 11+ messages in thread
From: Xueming(Steven) Li @ 2020-11-11  6:01 UTC (permalink / raw)
  To: Maxime Coquelin, dev, xuan.ding, stephen,
	NBU-Contact-Thomas Monjalon, stable, chenbo.xia

Hi Maxime,

Near end of this function, if vhost_check_queue_inflights_packed() and
vhost_check_queue_inflights_split() return with error, is the fd expected to be
closed by closing vq?

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>Sent: Monday, November 9, 2020 8:17 PM
>To: dev@dpdk.org; xuan.ding@intel.com; stephen@networkplumber.org;
>NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org;
>chenbo.xia@intel.com
>Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>Subject: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
>
>This patch fixes a file descriptor leak which happens in the error path of
>vhost_user_set_vring_kick().
>
>Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
>Cc: stable@dpdk.org
>
>Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
>---
> lib/librte_vhost/vhost_user.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
>94b066f0b9..f3b2adabac 100644
>--- a/lib/librte_vhost/vhost_user.c
>+++ b/lib/librte_vhost/vhost_user.c
>@@ -1855,8 +1855,12 @@ vhost_user_set_vring_kick(struct virtio_net **pdev,
>struct VhostUserMsg *msg,
>
> 	/* Interpret ring addresses only when ring is started. */
> 	dev = translate_ring_addresses(dev, file.index);
>-	if (!dev)
>+	if (!dev) {
>+		if (file.fd != VIRTIO_INVALID_EVENTFD)
>+			close(file.fd);
>+
> 		return RTE_VHOST_MSG_RESULT_ERR;
>+	}
>
> 	*pdev = dev;
>
>--
>2.26.2


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

* Re: [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory tables
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory tables Maxime Coquelin
@ 2020-11-11  6:06   ` Xueming(Steven) Li
  2020-11-12 17:00     ` Maxime Coquelin
  0 siblings, 1 reply; 11+ messages in thread
From: Xueming(Steven) Li @ 2020-11-11  6:06 UTC (permalink / raw)
  To: Maxime Coquelin, dev, xuan.ding, stephen,
	NBU-Contact-Thomas Monjalon, stable, chenbo.xia

Hi Maxime,

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>Sent: Monday, November 9, 2020 8:16 PM
>To: dev@dpdk.org; xuan.ding@intel.com; stephen@networkplumber.org;
>NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org;
>chenbo.xia@intel.com
>Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>Subject: [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory
>tables
>
>If an error is encountered before the memory regions are parsed, the file
>descriptors for these shared buffers are leaked.
>
>This patch fixes this by closing the message file descriptors on error, taking
>care of avoiding double closing of the file descriptors. guest_pages is also
>freed, even though it was not leaked as its pointer was not overridden on
>subsequent function calls.
>
>Fixes: 8f972312b8f4 ("vhost: support vhost-user")
>Cc: stable@dpdk.org
>
>Reported-by: Xuan Ding <xuan.ding@intel.com>
>Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
>---
> lib/librte_vhost/vhost_user.c | 65 +++++++++++++++++++++--------------
> 1 file changed, 39 insertions(+), 26 deletions(-)
>
>diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
>8a8726f8b8..473fd778ca 100644
>--- a/lib/librte_vhost/vhost_user.c
>+++ b/lib/librte_vhost/vhost_user.c
>@@ -99,8 +99,15 @@ close_msg_fds(struct VhostUserMsg *msg)  {
> 	int i;
>
>-	for (i = 0; i < msg->fd_num; i++)
>-		close(msg->fds[i]);
>+	for (i = 0; i < msg->fd_num; i++) {
>+		int fd = msg->fds[i];
>+
>+		if (fd == -1)
>+			continue;
>+
>+		msg->fds[i] = -1;
>+		close(fd);
>+	}
> }
>
> /*
>@@ -1004,7 +1011,6 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 	uint64_t alignment;
> 	uint32_t i;
> 	int populate;
>-	int fd;
>
> 	if (validate_msg_fds(msg, memory->nregions) != 0)
> 		return RTE_VHOST_MSG_RESULT_ERR;
>@@ -1012,16 +1018,13 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 	if (memory->nregions > VHOST_MEMORY_MAX_NREGIONS) {
> 		VHOST_LOG_CONFIG(ERR,
> 			"too many memory regions (%u)\n", memory-
>>nregions);
>-		return RTE_VHOST_MSG_RESULT_ERR;
>+		goto close_msg_fds;
> 	}
>
> 	if (dev->mem && !vhost_memory_changed(memory, dev->mem)) {
> 		VHOST_LOG_CONFIG(INFO,
> 			"(%d) memory regions not changed\n", dev->vid);
>-
>-		close_msg_fds(msg);
>-
>-		return RTE_VHOST_MSG_RESULT_OK;
>+		goto close_msg_fds;

Return code will be changed to RTE_VHOST_MSG_RESULT_ERR, is this ok?

> 	}
>
> 	if (dev->mem) {
>@@ -1054,7 +1057,7 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 				"(%d) failed to allocate memory "
> 				"for dev->guest_pages\n",
> 				dev->vid);
>-			return RTE_VHOST_MSG_RESULT_ERR;
>+			goto close_msg_fds;
> 		}
> 	}
>
>@@ -1064,18 +1067,23 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 		VHOST_LOG_CONFIG(ERR,
> 			"(%d) failed to allocate memory for dev->mem\n",
> 			dev->vid);
>-		return RTE_VHOST_MSG_RESULT_ERR;
>+		goto free_guest_pages;
> 	}
> 	dev->mem->nregions = memory->nregions;
>
> 	for (i = 0; i < memory->nregions; i++) {
>-		fd  = msg->fds[i];
> 		reg = &dev->mem->regions[i];
>
> 		reg->guest_phys_addr = memory->regions[i].guest_phys_addr;
> 		reg->guest_user_addr = memory->regions[i].userspace_addr;
> 		reg->size            = memory->regions[i].memory_size;
>-		reg->fd              = fd;
>+		reg->fd              = msg->fds[i];
>+
>+		/*
>+		 * Assign invalid file descriptor value to avoid double
>+		 * closing on error path.
>+		 */
>+		msg->fds[i] = -1;
>
> 		mmap_offset = memory->regions[i].mmap_offset;
>
>@@ -1085,7 +1093,7 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 				"mmap_offset (%#"PRIx64") and memory_size
>"
> 				"(%#"PRIx64") overflow\n",
> 				mmap_offset, reg->size);
>-			goto err_mmap;
>+			goto free_mem_table;
> 		}
>
> 		mmap_size = reg->size + mmap_offset;
>@@ -1098,11 +1106,11 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 		 * to avoid failure, make sure in caller to keep length
> 		 * aligned.
> 		 */
>-		alignment = get_blk_size(fd);
>+		alignment = get_blk_size(reg->fd);
> 		if (alignment == (uint64_t)-1) {
> 			VHOST_LOG_CONFIG(ERR,
> 				"couldn't get hugepage size through fstat\n");
>-			goto err_mmap;
>+			goto free_mem_table;
> 		}
> 		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
> 		if (mmap_size == 0) {
>@@ -1118,17 +1126,17 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 			VHOST_LOG_CONFIG(ERR, "mmap size (0x%" PRIx64 ")
>"
> 					"or alignment (0x%" PRIx64 ") is
>invalid\n",
> 					reg->size + mmap_offset, alignment);
>-			goto err_mmap;
>+			goto free_mem_table;
> 		}
>
> 		populate = dev->async_copy ? MAP_POPULATE : 0;
> 		mmap_addr = mmap(NULL, mmap_size, PROT_READ |
>PROT_WRITE,
>-				 MAP_SHARED | populate, fd, 0);
>+				 MAP_SHARED | populate, reg->fd, 0);
>
> 		if (mmap_addr == MAP_FAILED) {
> 			VHOST_LOG_CONFIG(ERR,
> 				"mmap region %u failed.\n", i);
>-			goto err_mmap;
>+			goto free_mem_table;
> 		}
>
> 		reg->mmap_addr = mmap_addr;
>@@ -1141,7 +1149,7 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 				VHOST_LOG_CONFIG(ERR,
> 					"adding guest pages to region %u
>failed.\n",
> 					i);
>-				goto err_mmap;
>+				goto free_mem_table;
> 			}
>
> 		VHOST_LOG_CONFIG(INFO,
>@@ -1184,17 +1192,17 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 		if (read_vhost_message(main_fd, &ack_msg) <= 0) {
> 			VHOST_LOG_CONFIG(ERR,
> 				"Failed to read qemu ack on postcopy set-
>mem-table\n");
>-			goto err_mmap;
>+			goto free_mem_table;
> 		}
>
> 		if (validate_msg_fds(&ack_msg, 0) != 0)
>-			goto err_mmap;
>+			goto free_mem_table;
>
> 		if (ack_msg.request.master != VHOST_USER_SET_MEM_TABLE)
>{
> 			VHOST_LOG_CONFIG(ERR,
> 				"Bad qemu ack on postcopy set-mem-table
>(%d)\n",
> 				ack_msg.request.master);
>-			goto err_mmap;
>+			goto free_mem_table;
> 		}
>
> 		/* Now userfault register and we can use the memory */ @@
>-1218,7 +1226,7 @@ vhost_user_set_mem_table(struct virtio_net **pdev,
>struct VhostUserMsg *msg,
> 					"Failed to register ufd for region %d:
>(ufd = %d) %s\n",
> 					i, dev->postcopy_ufd,
> 					strerror(errno));
>-				goto err_mmap;
>+				goto free_mem_table;
> 			}
> 			VHOST_LOG_CONFIG(INFO,
> 				"\t userfaultfd registered for range : "
>@@ -1227,7 +1235,7 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 				(uint64_t)reg_struct.range.start +
> 				(uint64_t)reg_struct.range.len - 1);  #else
>-			goto err_mmap;
>+			goto free_mem_table;
> #endif
> 		}
> 	}
>@@ -1249,7 +1257,7 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
> 			dev = translate_ring_addresses(dev, i);
> 			if (!dev) {
> 				dev = *pdev;
>-				goto err_mmap;
>+				goto free_mem_table;
> 			}
>
> 			*pdev = dev;
>@@ -1260,10 +1268,15 @@ vhost_user_set_mem_table(struct virtio_net
>**pdev, struct VhostUserMsg *msg,
>
> 	return RTE_VHOST_MSG_RESULT_OK;
>
>-err_mmap:
>+free_mem_table:
> 	free_mem_region(dev);
> 	rte_free(dev->mem);
> 	dev->mem = NULL;
>+free_guest_pages:
>+	rte_free(dev->guest_pages);
>+	dev->guest_pages = NULL;
>+close_msg_fds:
>+	close_msg_fds(msg);
> 	return RTE_VHOST_MSG_RESULT_ERR;
> }
>
>--
>2.26.2


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

* Re: [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup
  2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup Maxime Coquelin
  2020-11-10  1:44   ` Xia, Chenbo
@ 2020-11-11  6:13   ` Xueming(Steven) Li
  1 sibling, 0 replies; 11+ messages in thread
From: Xueming(Steven) Li @ 2020-11-11  6:13 UTC (permalink / raw)
  To: Maxime Coquelin, dev, xuan.ding, stephen,
	NBU-Contact-Thomas Monjalon, stable, chenbo.xia
  Cc: Xueming(Steven) Li



>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>Sent: Monday, November 9, 2020 8:16 PM
>To: dev@dpdk.org; xuan.ding@intel.com; stephen@networkplumber.org;
>NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org;
>chenbo.xia@intel.com
>Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>Subject: [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup
>
>This patch fixes a file descriptor leak which happens in the error path of
>vhost_user_set_log_base().
>
>Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
>Cc: stable@dpdk.org
>
>Reported-by: Xuan Ding <xuan.ding@intel.com>
>Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>---
> lib/librte_vhost/vhost_user.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
>diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
>473fd778ca..94b066f0b9 100644
>--- a/lib/librte_vhost/vhost_user.c
>+++ b/lib/librte_vhost/vhost_user.c
>@@ -2083,7 +2083,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
>struct VhostUserMsg *msg,
> 		VHOST_LOG_CONFIG(ERR,
> 			"invalid log base msg size: %"PRId32" != %d\n",
> 			msg->size, (int)sizeof(VhostUserLog));
>-		return RTE_VHOST_MSG_RESULT_ERR;
>+		goto close_msg_fds;
> 	}
>
> 	size = msg->payload.log.mmap_size;
>@@ -2094,7 +2094,7 @@ vhost_user_set_log_base(struct virtio_net **pdev,
>struct VhostUserMsg *msg,
> 		VHOST_LOG_CONFIG(ERR,
> 			"log offset %#"PRIx64" and log size %#"PRIx64"
>overflow\n",
> 			off, size);
>-		return RTE_VHOST_MSG_RESULT_ERR;
>+		goto close_msg_fds;
> 	}
>
> 	VHOST_LOG_CONFIG(INFO,
>@@ -2131,6 +2131,10 @@ vhost_user_set_log_base(struct virtio_net **pdev,
>struct VhostUserMsg *msg,
> 	msg->fd_num = 0;
>
> 	return RTE_VHOST_MSG_RESULT_REPLY;
>+
>+close_msg_fds:
>+	close_msg_fds(msg);
>+	return RTE_VHOST_MSG_RESULT_ERR;
> }
>
> static int vhost_user_set_log_fd(struct virtio_net **pdev __rte_unused,
>--
>2.26.2

Reviewed-by: Xueming(Steven) Li <xuemingl@nvidia.com>

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

* Re: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
  2020-11-11  6:01   ` Xueming(Steven) Li
@ 2020-11-11  7:57     ` Xia, Chenbo
  2020-11-12 17:06       ` Maxime Coquelin
  0 siblings, 1 reply; 11+ messages in thread
From: Xia, Chenbo @ 2020-11-11  7:57 UTC (permalink / raw)
  To: Xueming(Steven) Li, Maxime Coquelin, dev, Ding, Xuan, stephen,
	NBU-Contact-Thomas Monjalon, stable

Hi Xueming & Maxime,

> -----Original Message-----
> From: Xueming(Steven) Li <xuemingl@nvidia.com>
> Sent: Wednesday, November 11, 2020 2:02 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org; Ding, Xuan
> <xuan.ding@intel.com>; stephen@networkplumber.org; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; stable@dpdk.org; Xia, Chenbo
> <chenbo.xia@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
> 
> Hi Maxime,
> 
> Near end of this function, if vhost_check_queue_inflights_packed() and
> vhost_check_queue_inflights_split() return with error, is the fd expected
> to be
> closed by closing vq?

I thought about this before. In theory, it will not cause fd leak because the fd
is saved in vq. It will be closed upon next kick msg or vhost device destroy. But
thinking it again, maybe it's better to close it now since anyway it's useless now😊

What do you think?

Thanks,
Chenbo

> 
> >-----Original Message-----
> >From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
> >Sent: Monday, November 9, 2020 8:17 PM
> >To: dev@dpdk.org; xuan.ding@intel.com; stephen@networkplumber.org;
> >NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org;
> >chenbo.xia@intel.com
> >Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> >Subject: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
> >
> >This patch fixes a file descriptor leak which happens in the error path
> of
> >vhost_user_set_vring_kick().
> >
> >Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
> >Cc: stable@dpdk.org
> >
> >Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> >---
> > lib/librte_vhost/vhost_user.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> >diff --git a/lib/librte_vhost/vhost_user.c
> b/lib/librte_vhost/vhost_user.c index
> >94b066f0b9..f3b2adabac 100644
> >--- a/lib/librte_vhost/vhost_user.c
> >+++ b/lib/librte_vhost/vhost_user.c
> >@@ -1855,8 +1855,12 @@ vhost_user_set_vring_kick(struct virtio_net **pdev,
> >struct VhostUserMsg *msg,
> >
> > 	/* Interpret ring addresses only when ring is started. */
> > 	dev = translate_ring_addresses(dev, file.index);
> >-	if (!dev)
> >+	if (!dev) {
> >+		if (file.fd != VIRTIO_INVALID_EVENTFD)
> >+			close(file.fd);
> >+
> > 		return RTE_VHOST_MSG_RESULT_ERR;
> >+	}
> >
> > 	*pdev = dev;
> >
> >--
> >2.26.2


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

* Re: [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory tables
  2020-11-11  6:06   ` Xueming(Steven) Li
@ 2020-11-12 17:00     ` Maxime Coquelin
  0 siblings, 0 replies; 11+ messages in thread
From: Maxime Coquelin @ 2020-11-12 17:00 UTC (permalink / raw)
  To: Xueming(Steven) Li, dev, xuan.ding, stephen,
	NBU-Contact-Thomas Monjalon, stable, chenbo.xia



On 11/11/20 7:06 AM, Xueming(Steven) Li wrote:
>> @@ -1012,16 +1018,13 @@ vhost_user_set_mem_table(struct virtio_net
>> **pdev, struct VhostUserMsg *msg,
>> 	if (memory->nregions > VHOST_MEMORY_MAX_NREGIONS) {
>> 		VHOST_LOG_CONFIG(ERR,
>> 			"too many memory regions (%u)\n", memory-
>>> nregions);
>> -		return RTE_VHOST_MSG_RESULT_ERR;
>> +		goto close_msg_fds;
>> 	}
>>
>> 	if (dev->mem && !vhost_memory_changed(memory, dev->mem)) {
>> 		VHOST_LOG_CONFIG(INFO,
>> 			"(%d) memory regions not changed\n", dev->vid);
>> -
>> -		close_msg_fds(msg);
>> -
>> -		return RTE_VHOST_MSG_RESULT_OK;
>> +		goto close_msg_fds;
> Return code will be changed to RTE_VHOST_MSG_RESULT_ERR, is this ok?
> 

Good catch, that is not OK to return RTE_VHOST_MSG_RESULT_ERR here.

Will fix in v3.

Thanks!
Maxime


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

* Re: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
  2020-11-11  7:57     ` Xia, Chenbo
@ 2020-11-12 17:06       ` Maxime Coquelin
  0 siblings, 0 replies; 11+ messages in thread
From: Maxime Coquelin @ 2020-11-12 17:06 UTC (permalink / raw)
  To: Xia, Chenbo, Xueming(Steven) Li, dev, Ding, Xuan, stephen,
	NBU-Contact-Thomas Monjalon, stable



On 11/11/20 8:57 AM, Xia, Chenbo wrote:
> Hi Xueming & Maxime,
> 
>> -----Original Message-----
>> From: Xueming(Steven) Li <xuemingl@nvidia.com>
>> Sent: Wednesday, November 11, 2020 2:02 PM
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org; Ding, Xuan
>> <xuan.ding@intel.com>; stephen@networkplumber.org; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; stable@dpdk.org; Xia, Chenbo
>> <chenbo.xia@intel.com>
>> Subject: RE: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
>>
>> Hi Maxime,
>>
>> Near end of this function, if vhost_check_queue_inflights_packed() and
>> vhost_check_queue_inflights_split() return with error, is the fd expected
>> to be
>> closed by closing vq?
> 
> I thought about this before. In theory, it will not cause fd leak because the fd
> is saved in vq. It will be closed upon next kick msg or vhost device destroy. But
> thinking it again, maybe it's better to close it now since anyway it's useless now😊
> 
> What do you think?

I did it on purpose, as indeed it is saved in the vq metadata at that
stage.

The goal of the series being to avoid leaks, I think the patch does what
is necessary.

There is a function to cleanup the FDs and memory saved in the metadata,
so let it be done there.

Thanks,
Maxime

> Thanks,
> Chenbo
> 
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>>> Sent: Monday, November 9, 2020 8:17 PM
>>> To: dev@dpdk.org; xuan.ding@intel.com; stephen@networkplumber.org;
>>> NBU-Contact-Thomas Monjalon <thomas@monjalon.net>; stable@dpdk.org;
>>> chenbo.xia@intel.com
>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Subject: [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup
>>>
>>> This patch fixes a file descriptor leak which happens in the error path
>> of
>>> vhost_user_set_vring_kick().
>>>
>>> Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
>>> ---
>>> lib/librte_vhost/vhost_user.c | 6 +++++-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c
>> b/lib/librte_vhost/vhost_user.c index
>>> 94b066f0b9..f3b2adabac 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1855,8 +1855,12 @@ vhost_user_set_vring_kick(struct virtio_net **pdev,
>>> struct VhostUserMsg *msg,
>>>
>>> 	/* Interpret ring addresses only when ring is started. */
>>> 	dev = translate_ring_addresses(dev, file.index);
>>> -	if (!dev)
>>> +	if (!dev) {
>>> +		if (file.fd != VIRTIO_INVALID_EVENTFD)
>>> +			close(file.fd);
>>> +
>>> 		return RTE_VHOST_MSG_RESULT_ERR;
>>> +	}
>>>
>>> 	*pdev = dev;
>>>
>>> --
>>> 2.26.2
> 


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

end of thread, other threads:[~2020-11-12 17:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-09 12:16 [dpdk-dev] [PATCH v3 0/3] vhost: fix fd an memory leaks Maxime Coquelin
2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 1/3] vhost: fix error path when setting memory tables Maxime Coquelin
2020-11-11  6:06   ` Xueming(Steven) Li
2020-11-12 17:00     ` Maxime Coquelin
2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 2/3] vhost: fix fd leak in dirty logging setup Maxime Coquelin
2020-11-10  1:44   ` Xia, Chenbo
2020-11-11  6:13   ` Xueming(Steven) Li
2020-11-09 12:16 ` [dpdk-dev] [PATCH v3 3/3] vhost: fix fd leak in kick setup Maxime Coquelin
2020-11-11  6:01   ` Xueming(Steven) Li
2020-11-11  7:57     ` Xia, Chenbo
2020-11-12 17:06       ` Maxime Coquelin

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