[PATCH 0/6] VDUSE reconnection fixes and cleanup

[PATCH 0/6] VDUSE reconnection fixes and cleanup

[Maxime Coquelin]

This series provides various fixes for VDUSE reconnection
introduced in -rc1, some of them being reported by
Coverity.

The series also takes the opportunity to refactor
reconnection to make it more self-contained to simplify
readability and error path handling.

Maxime Coquelin (6):
  vhost: fix vduse device creation error handling
  vhost: fix possible TOCTOU in VDUSE dev creation
  vhost: fix VDUSE reconnect device start failure
  vhost: refactor VDUSE reconnection log mapping
  vhost: fix and refactor VDUSE reconnect log check
  vhost: move VDUSE reconnection after device is created

 lib/vhost/vduse.c | 322 ++++++++++++++++++++++++++--------------------
 1 file changed, 183 insertions(+), 139 deletions(-)

-- 
2.46.2

[PATCH 1/6] vhost: fix vduse device creation error handling

[Maxime Coquelin]

This patch fixes missing reconnection log unmapping
miss in the error path at VDUSE creation time.

Coverity issue: 445525
Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vduse.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index f9ac317438..89bc29a33f 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -488,7 +488,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	struct vduse_dev_config *dev_config = NULL;
 	const char *name = path + strlen("/dev/vduse/");
 	char reconnect_file[PATH_MAX];
-	struct vhost_reconnect_data *reconnect_log = NULL;
+	struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
 	bool reconnect = false;
 
 	if (vduse.fdset == NULL) {
@@ -530,13 +530,13 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	ret = rte_vhost_driver_get_features(path, &features);
 	if (ret < 0) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to get backend features");
-		goto out_free;
+		goto out_ctrl_close;
 	}
 
 	ret = rte_vhost_driver_get_queue_num(path, &max_queue_pairs);
 	if (ret < 0) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to get max queue pairs");
-		goto out_free;
+		goto out_ctrl_close;
 	}
 
 	VHOST_CONFIG_LOG(path, INFO, "VDUSE max queue pairs: %u", max_queue_pairs);
@@ -584,7 +584,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 					"Features mismatch between backend (0x%" PRIx64 ") & reconnection file (0x%" PRIx64 ")",
 					features, reconnect_log->features);
 			ret = -1;
-			goto out_ctrl_close;
+			goto out_log_unmap;
 		}
 
 		if (reconnect_log->nr_vrings != total_queues) {
@@ -592,7 +592,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 					"Queues number mismatch between backend (%u) and reconnection file (%u)",
 					total_queues, reconnect_log->nr_vrings);
 			ret = -1;
-			goto out_ctrl_close;
+			goto out_log_unmap;
 		}
 	} else {
 		reco_fd = open(reconnect_file, O_CREAT | O_EXCL | O_RDWR, 0600);
@@ -633,7 +633,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 		if (!dev_config) {
 			VHOST_CONFIG_LOG(name, ERR, "Failed to allocate VDUSE config");
 			ret = -1;
-			goto out_ctrl_close;
+			goto out_log_unmap;
 		}
 
 		vnet_config.max_virtqueue_pairs = max_queue_pairs;
@@ -649,16 +649,16 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 		memcpy(dev_config->config, &vnet_config, sizeof(vnet_config));
 
 		ret = ioctl(control_fd, VDUSE_CREATE_DEV, dev_config);
+		free(dev_config);
+		dev_config = NULL;
 		if (ret < 0) {
 			VHOST_CONFIG_LOG(name, ERR, "Failed to create VDUSE device: %s",
 					strerror(errno));
-			goto out_free;
+			goto out_log_unmap;
 		}
 
 		memcpy(&reconnect_log->config, &vnet_config, sizeof(vnet_config));
 		reconnect_log->nr_vrings = total_queues;
-		free(dev_config);
-		dev_config = NULL;
 	}
 
 	dev_fd = open(path, O_RDWR);
@@ -693,6 +693,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	dev->vduse_ctrl_fd = control_fd;
 	dev->vduse_dev_fd = dev_fd;
 	dev->reconnect_log = reconnect_log;
+	reconnect_log = MAP_FAILED;
 	if (reconnect)
 		dev->status = dev->reconnect_log->status;
 
@@ -768,8 +769,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	if (dev_fd >= 0)
 		close(dev_fd);
 	ioctl(control_fd, VDUSE_DESTROY_DEV, name);
-out_free:
-	free(dev_config);
+out_log_unmap:
+	if (reconnect_log != MAP_FAILED)
+		munmap(reconnect_log, sizeof(*reconnect_log));
 out_ctrl_close:
 	close(control_fd);
 
-- 
2.46.2

[PATCH 2/6] vhost: fix possible TOCTOU in VDUSE dev creation

[Maxime Coquelin]

This patch fixes a possible TOCTOU on the VDUSE
device chardev opening at device creation time.

Coverity issue: 445526
Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vduse.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 89bc29a33f..d1373d0549 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -547,7 +547,8 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	else
 		total_queues += 1; /* Includes ctrl queue */
 
-	if (access(path, F_OK) == 0) {
+	dev_fd = open(path, O_RDWR);
+	if (dev_fd >= 0) {
 		VHOST_CONFIG_LOG(name, INFO, "Device already exists, reconnecting...");
 		reconnect = true;
 
@@ -560,7 +561,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 				VHOST_CONFIG_LOG(name, ERR, "Failed to open reconnect file %s (%s)",
 						reconnect_file, strerror(errno));
 			ret = -1;
-			goto out_ctrl_close;
+			goto out_dev_close;
 		}
 
 		reconnect_log = mmap(NULL, sizeof(*reconnect_log), PROT_READ | PROT_WRITE,
@@ -570,7 +571,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 			VHOST_CONFIG_LOG(name, ERR, "Failed to mmap reconnect file %s (%s)",
 					reconnect_file, strerror(errno));
 			ret = -1;
-			goto out_ctrl_close;
+			goto out_dev_close;
 		}
 
 		if (reconnect_log->version != VHOST_RECONNECT_VERSION) {
@@ -594,7 +595,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 			ret = -1;
 			goto out_log_unmap;
 		}
-	} else {
+	} else if (errno == ENOENT) {
 		reco_fd = open(reconnect_file, O_CREAT | O_EXCL | O_RDWR, 0600);
 		if (reco_fd < 0) {
 			if (errno == EEXIST) {
@@ -659,34 +660,39 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 
 		memcpy(&reconnect_log->config, &vnet_config, sizeof(vnet_config));
 		reconnect_log->nr_vrings = total_queues;
-	}
 
-	dev_fd = open(path, O_RDWR);
-	if (dev_fd < 0) {
+		dev_fd = open(path, O_RDWR);
+		if (dev_fd < 0) {
+			VHOST_CONFIG_LOG(name, ERR, "Failed to open newly created device %s: %s",
+					path, strerror(errno));
+			ret = -1;
+			goto out_log_unmap;
+		}
+	} else {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to open device %s: %s",
 				path, strerror(errno));
 		ret = -1;
-		goto out_dev_close;
+		goto out_ctrl_close;
 	}
 
 	ret = fcntl(dev_fd, F_SETFL, O_NONBLOCK);
 	if (ret < 0) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to set chardev as non-blocking: %s",
 				strerror(errno));
-		goto out_dev_close;
+		goto out_log_unmap;
 	}
 
 	vid = vhost_new_device(&vduse_backend_ops);
 	if (vid < 0) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to create new Vhost device");
 		ret = -1;
-		goto out_dev_close;
+		goto out_log_unmap;
 	}
 
 	dev = get_device(vid);
 	if (!dev) {
 		ret = -1;
-		goto out_dev_close;
+		goto out_dev_destroy;
 	}
 
 	strncpy(dev->ifname, path, IF_NAME_SZ - 1);
@@ -765,13 +771,13 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 
 out_dev_destroy:
 	vhost_destroy_device(vid);
+out_log_unmap:
+	if (reconnect_log != MAP_FAILED)
+		munmap(reconnect_log, sizeof(*reconnect_log));
 out_dev_close:
 	if (dev_fd >= 0)
 		close(dev_fd);
 	ioctl(control_fd, VDUSE_DESTROY_DEV, name);
-out_log_unmap:
-	if (reconnect_log != MAP_FAILED)
-		munmap(reconnect_log, sizeof(*reconnect_log));
 out_ctrl_close:
 	close(control_fd);
 
-- 
2.46.2

[PATCH 3/6] vhost: fix VDUSE reconnect device start failure

[Maxime Coquelin]

This patch fixes a FD leak in the VDUSE device reconnect
code fails to start the device.

Also take the opportunity to refactor the related code into
a dedicated function.

Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vduse.c | 63 +++++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 21 deletions(-)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index d1373d0549..c8c4761293 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -476,6 +476,46 @@ vduse_reconnect_handler(int fd, void *arg, int *remove)
 	*remove = 1;
 }
 
+static int
+vduse_reconnect_start_device(struct virtio_net *dev)
+{
+	int fd, ret;
+
+	/*
+	 * Make vduse_device_start() being executed in the same
+	 * context for both reconnection and fresh startup.
+	 */
+	fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
+	if (fd < 0) {
+		VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to create reconnect efd: %s",
+				strerror(errno));
+		ret = -1;
+		goto out_err;
+	}
+
+	ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, dev);
+	if (ret) {
+		VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to add reconnect efd %d to vduse fdset",
+				fd);
+		goto out_err_close;
+	}
+
+	ret = eventfd_write(fd, (eventfd_t)1);
+	if (ret < 0) {
+		VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to write to reconnect eventfd");
+		goto out_err_fdset;
+	}
+
+	return 0;
+
+out_err_fdset:
+	fdset_del(vduse.fdset, fd);
+out_err_close:
+	close(fd);
+out_err:
+	return ret;
+}
+
 int
 vduse_device_create(const char *path, bool compliant_ol_flags)
 {
@@ -741,28 +781,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	}
 
 	if (reconnect && dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK)  {
-		/*
-		 * Make vduse_device_start() being executed in the same
-		 * context for both reconnection and fresh startup.
-		 */
-		reco_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
-		if (reco_fd < 0) {
-			VHOST_CONFIG_LOG(name, ERR, "Failed to create reco_fd: %s",
-					strerror(errno));
-			ret = -1;
-			goto out_dev_destroy;
-		}
-
-		ret = fdset_add(vduse.fdset, reco_fd, vduse_reconnect_handler, NULL, dev);
+		ret = vduse_reconnect_start_device(dev);
 		if (ret) {
-			VHOST_CONFIG_LOG(name, ERR, "Failed to add reconnect fd %d to vduse fdset",
-					reco_fd);
-			goto out_dev_destroy;
-		}
-
-		ret = eventfd_write(reco_fd, (eventfd_t)1);
-		if (ret < 0) {
-			VHOST_CONFIG_LOG(name, ERR, "Failed to write to reconnect eventfd");
+			VHOST_CONFIG_LOG(name, ERR, "Failed to start device at reconnection");
 			goto out_dev_destroy;
 		}
 	}
-- 
2.46.2

[PATCH 4/6] vhost: refactor VDUSE reconnection log mapping

[Maxime Coquelin]

This patch moves the VDUSE reconnection log mapping, as
well as creation if needed, into a dedicated function.

This is a preliminary rework to simplify VDUSE device
creation.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vduse.c | 138 +++++++++++++++++++++++++---------------------
 1 file changed, 75 insertions(+), 63 deletions(-)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index c8c4761293..985d4cc58d 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -431,6 +431,9 @@ vduse_reconnect_path_init(void)
 	const char *directory;
 	int ret;
 
+	if (vduse_reconnect_path_set == true)
+		return 0;
+
 	/* from RuntimeDirectory= see systemd.exec */
 	directory = getenv("RUNTIME_DIRECTORY");
 	if (directory == NULL) {
@@ -462,9 +465,74 @@ vduse_reconnect_path_init(void)
 	VHOST_CONFIG_LOG("vduse", INFO, "Created VDUSE reconnect directory in %s",
 			vduse_reconnect_dir);
 
+	vduse_reconnect_path_set = true;
+
 	return 0;
 }
 
+static int
+vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco_log, bool create)
+{
+	char reco_file[PATH_MAX];
+	int fd, ret;
+
+	if (vduse_reconnect_path_init() < 0) {
+		VHOST_CONFIG_LOG(dev_name, ERR, "Failed to initialize reconnect path");
+			return -1;
+	}
+
+	ret = snprintf(reco_file, sizeof(reco_file), "%s/%s", vduse_reconnect_dir, dev_name);
+	if (ret < 0 || ret == sizeof(reco_file)) {
+		VHOST_CONFIG_LOG(dev_name, ERR, "Failed to create vduse reconnect path name");
+		return -1;
+	}
+
+	if (create) {
+		fd = open(reco_file, O_CREAT | O_EXCL | O_RDWR, 0600);
+		if (fd < 0) {
+			if (errno == EEXIST) {
+				VHOST_CONFIG_LOG(dev_name, ERR, "Reconnect file %s exists but not the device",
+						reco_file);
+			} else {
+				VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
+						reco_file, strerror(errno));
+			}
+			return -1;
+		}
+
+		ret = ftruncate(fd, sizeof(**reco_log));
+		if (ret < 0) {
+			VHOST_CONFIG_LOG(dev_name, ERR, "Failed to truncate reconnect file %s (%s)",
+					reco_file, strerror(errno));
+			goto out_close;
+		}
+	} else {
+		fd = open(reco_file, O_RDWR, 0600);
+		if (fd < 0) {
+			if (errno == ENOENT)
+				VHOST_CONFIG_LOG(dev_name, ERR, "Missing reconnect file (%s)", reco_file);
+			else
+				VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
+						reco_file, strerror(errno));
+			return -1;
+		}
+	}
+
+	*reco_log = mmap(NULL, sizeof(**reco_log), PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+	if (*reco_log == MAP_FAILED) {
+		VHOST_CONFIG_LOG(dev_name, ERR, "Failed to mmap reconnect file %s (%s)",
+				reco_file, strerror(errno));
+		ret = -1;
+		goto out_close;
+	}
+	ret = 0;
+
+out_close:
+	close(fd);
+
+	return ret;
+}
+
 static void
 vduse_reconnect_handler(int fd, void *arg, int *remove)
 {
@@ -519,7 +587,7 @@ vduse_reconnect_start_device(struct virtio_net *dev)
 int
 vduse_device_create(const char *path, bool compliant_ol_flags)
 {
-	int control_fd, dev_fd, vid, ret, reco_fd;
+	int control_fd, dev_fd, vid, ret;
 	uint32_t i, max_queue_pairs, total_queues;
 	struct virtio_net *dev;
 	struct virtio_net_config vnet_config = {{ 0 }};
@@ -527,7 +595,6 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	uint64_t features;
 	struct vduse_dev_config *dev_config = NULL;
 	const char *name = path + strlen("/dev/vduse/");
-	char reconnect_file[PATH_MAX];
 	struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
 	bool reconnect = false;
 
@@ -539,20 +606,6 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 		}
 	}
 
-	if (vduse_reconnect_path_set == false) {
-		if (vduse_reconnect_path_init() < 0) {
-			VHOST_CONFIG_LOG(path, ERR, "failed to initialize reconnect path");
-			return -1;
-		}
-		vduse_reconnect_path_set = true;
-	}
-
-	ret = snprintf(reconnect_file, sizeof(reconnect_file), "%s/%s", vduse_reconnect_dir, name);
-	if (ret < 0 || ret == sizeof(reconnect_file)) {
-		VHOST_CONFIG_LOG(name, ERR, "Failed to create vduse reconnect path name");
-		return -1;
-	}
-
 	control_fd = open(VDUSE_CTRL_PATH, O_RDWR);
 	if (control_fd < 0) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to open %s: %s",
@@ -592,26 +645,10 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 		VHOST_CONFIG_LOG(name, INFO, "Device already exists, reconnecting...");
 		reconnect = true;
 
-		reco_fd = open(reconnect_file, O_RDWR, 0600);
-		if (reco_fd < 0) {
-			if (errno == ENOENT)
-				VHOST_CONFIG_LOG(name, ERR, "Missing reconnect file (%s)",
-						reconnect_file);
-			else
-				VHOST_CONFIG_LOG(name, ERR, "Failed to open reconnect file %s (%s)",
-						reconnect_file, strerror(errno));
-			ret = -1;
-			goto out_dev_close;
-		}
-
-		reconnect_log = mmap(NULL, sizeof(*reconnect_log), PROT_READ | PROT_WRITE,
-				MAP_SHARED, reco_fd, 0);
-		close(reco_fd);
-		if (reconnect_log == MAP_FAILED) {
-			VHOST_CONFIG_LOG(name, ERR, "Failed to mmap reconnect file %s (%s)",
-					reconnect_file, strerror(errno));
-			ret = -1;
-			goto out_dev_close;
+		ret = vduse_reconnect_log_map(name, &reconnect_log, false);
+		if (ret < 0) {
+			VHOST_CONFIG_LOG(name, ERR, "Failed to map reconnect log file");
+			goto out_ctrl_close;
 		}
 
 		if (reconnect_log->version != VHOST_RECONNECT_VERSION) {
@@ -636,34 +673,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 			goto out_log_unmap;
 		}
 	} else if (errno == ENOENT) {
-		reco_fd = open(reconnect_file, O_CREAT | O_EXCL | O_RDWR, 0600);
-		if (reco_fd < 0) {
-			if (errno == EEXIST) {
-				VHOST_CONFIG_LOG(name, ERR, "Reconnect file %s exists but not the device",
-						reconnect_file);
-			} else {
-				VHOST_CONFIG_LOG(name, ERR, "Failed to open reconnect file %s (%s)",
-						reconnect_file, strerror(errno));
-			}
-			ret = -1;
-			goto out_ctrl_close;
-		}
-
-		ret = ftruncate(reco_fd, sizeof(*reconnect_log));
+		ret = vduse_reconnect_log_map(name, &reconnect_log, true);
 		if (ret < 0) {
-			VHOST_CONFIG_LOG(name, ERR, "Failed to truncate reconnect file %s (%s)",
-					reconnect_file, strerror(errno));
-			close(reco_fd);
-			goto out_ctrl_close;
-		}
-
-		reconnect_log = mmap(NULL, sizeof(*reconnect_log), PROT_READ | PROT_WRITE,
-					MAP_SHARED, reco_fd, 0);
-		close(reco_fd);
-		if (reconnect_log == MAP_FAILED) {
-			VHOST_CONFIG_LOG(name, ERR, "Failed to mmap reconnect file %s (%s)",
-					reconnect_file, strerror(errno));
-			ret = -1;
+			VHOST_CONFIG_LOG(name, ERR, "Failed to create reconnect log file");
 			goto out_ctrl_close;
 		}
 
-- 
2.46.2

[PATCH 5/6] vhost: fix and refactor VDUSE reconnect log check

[Maxime Coquelin]

This patch fixes missing error handling in checking the
reconnect log version, and takes the opportunity to move
all the checks into a dedicated function to simply VDUSE
device creation code.

Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vduse.c | 51 +++++++++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 985d4cc58d..0ce17e9690 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -533,6 +533,35 @@ vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco
 	return ret;
 }
 
+static int
+vduse_reconnect_log_check(struct vhost_reconnect_data *reco_log,
+			  const char *dev_name,
+			  uint64_t features, uint32_t total_queues)
+{
+	if (reco_log->version != VHOST_RECONNECT_VERSION) {
+		VHOST_CONFIG_LOG(dev_name, ERR,
+				"Version mismatch between backend (0x%x) & reconnection file (0x%x)",
+				VHOST_RECONNECT_VERSION, reco_log->version);
+		return -1;
+	}
+
+	if ((reco_log->features & features) != reco_log->features) {
+		VHOST_CONFIG_LOG(dev_name, ERR,
+				"Features mismatch between backend (0x%" PRIx64 ") & reconnection file (0x%" PRIx64 ")",
+				features, reco_log->features);
+		return -1;
+	}
+
+	if (reco_log->nr_vrings != total_queues) {
+		VHOST_CONFIG_LOG(dev_name, ERR,
+				"Queues number mismatch between backend (%u) and reconnection file (%u)",
+				total_queues, reco_log->nr_vrings);
+		return -1;
+	}
+
+	return 0;
+}
+
 static void
 vduse_reconnect_handler(int fd, void *arg, int *remove)
 {
@@ -651,25 +680,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 			goto out_ctrl_close;
 		}
 
-		if (reconnect_log->version != VHOST_RECONNECT_VERSION) {
-			VHOST_CONFIG_LOG(name, ERR,
-					"Version mismatch between backend (0x%x) & reconnection file (0x%x)",
-					VHOST_RECONNECT_VERSION, reconnect_log->version);
-		}
-
-		if ((reconnect_log->features & features) != reconnect_log->features) {
-			VHOST_CONFIG_LOG(name, ERR,
-					"Features mismatch between backend (0x%" PRIx64 ") & reconnection file (0x%" PRIx64 ")",
-					features, reconnect_log->features);
-			ret = -1;
-			goto out_log_unmap;
-		}
-
-		if (reconnect_log->nr_vrings != total_queues) {
-			VHOST_CONFIG_LOG(name, ERR,
-					"Queues number mismatch between backend (%u) and reconnection file (%u)",
-					total_queues, reconnect_log->nr_vrings);
-			ret = -1;
+		ret = vduse_reconnect_log_check(reconnect_log, name, features, total_queues);
+		if (ret < 0) {
+			VHOST_CONFIG_LOG(name, ERR, "Reconnection log file check failed");
 			goto out_log_unmap;
 		}
 	} else if (errno == ENOENT) {
-- 
2.46.2

[PATCH 6/6] vhost: move VDUSE reconnection after device is created

[Maxime Coquelin]

This patch moves the VDUSE reconnection log mapping and
setup after the Vhost device is created in order to
simplify the device creation and its error path.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vduse.c | 120 +++++++++++++++++++++-------------------------
 1 file changed, 55 insertions(+), 65 deletions(-)

diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
index 0ce17e9690..68d0e2c5d8 100644
--- a/lib/vhost/vduse.c
+++ b/lib/vhost/vduse.c
@@ -471,19 +471,20 @@ vduse_reconnect_path_init(void)
 }
 
 static int
-vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco_log, bool create)
+vduse_reconnect_log_map(struct virtio_net *dev, bool create)
 {
 	char reco_file[PATH_MAX];
 	int fd, ret;
+	const char *name = dev->ifname + strlen("/dev/vduse/");
 
 	if (vduse_reconnect_path_init() < 0) {
-		VHOST_CONFIG_LOG(dev_name, ERR, "Failed to initialize reconnect path");
+		VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to initialize reconnect path");
 			return -1;
 	}
 
-	ret = snprintf(reco_file, sizeof(reco_file), "%s/%s", vduse_reconnect_dir, dev_name);
+	ret = snprintf(reco_file, sizeof(reco_file), "%s/%s", vduse_reconnect_dir, name);
 	if (ret < 0 || ret == sizeof(reco_file)) {
-		VHOST_CONFIG_LOG(dev_name, ERR, "Failed to create vduse reconnect path name");
+		VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to create vduse reconnect path name");
 		return -1;
 	}
 
@@ -491,18 +492,18 @@ vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco
 		fd = open(reco_file, O_CREAT | O_EXCL | O_RDWR, 0600);
 		if (fd < 0) {
 			if (errno == EEXIST) {
-				VHOST_CONFIG_LOG(dev_name, ERR, "Reconnect file %s exists but not the device",
+				VHOST_CONFIG_LOG(dev->ifname, ERR, "Reconnect file %s exists but not the device",
 						reco_file);
 			} else {
-				VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
+				VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to open reconnect file %s (%s)",
 						reco_file, strerror(errno));
 			}
 			return -1;
 		}
 
-		ret = ftruncate(fd, sizeof(**reco_log));
+		ret = ftruncate(fd, sizeof(*dev->reconnect_log));
 		if (ret < 0) {
-			VHOST_CONFIG_LOG(dev_name, ERR, "Failed to truncate reconnect file %s (%s)",
+			VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to truncate reconnect file %s (%s)",
 					reco_file, strerror(errno));
 			goto out_close;
 		}
@@ -510,17 +511,18 @@ vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco
 		fd = open(reco_file, O_RDWR, 0600);
 		if (fd < 0) {
 			if (errno == ENOENT)
-				VHOST_CONFIG_LOG(dev_name, ERR, "Missing reconnect file (%s)", reco_file);
+				VHOST_CONFIG_LOG(dev->ifname, ERR, "Missing reconnect file (%s)", reco_file);
 			else
-				VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
+				VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to open reconnect file %s (%s)",
 						reco_file, strerror(errno));
 			return -1;
 		}
 	}
 
-	*reco_log = mmap(NULL, sizeof(**reco_log), PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
-	if (*reco_log == MAP_FAILED) {
-		VHOST_CONFIG_LOG(dev_name, ERR, "Failed to mmap reconnect file %s (%s)",
+	dev->reconnect_log = mmap(NULL, sizeof(*dev->reconnect_log), PROT_READ | PROT_WRITE,
+				MAP_SHARED, fd, 0);
+	if (dev->reconnect_log == MAP_FAILED) {
+		VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to mmap reconnect file %s (%s)",
 				reco_file, strerror(errno));
 		ret = -1;
 		goto out_close;
@@ -534,28 +536,26 @@ vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco
 }
 
 static int
-vduse_reconnect_log_check(struct vhost_reconnect_data *reco_log,
-			  const char *dev_name,
-			  uint64_t features, uint32_t total_queues)
+vduse_reconnect_log_check(struct virtio_net *dev, uint64_t features, uint32_t total_queues)
 {
-	if (reco_log->version != VHOST_RECONNECT_VERSION) {
-		VHOST_CONFIG_LOG(dev_name, ERR,
+	if (dev->reconnect_log->version != VHOST_RECONNECT_VERSION) {
+		VHOST_CONFIG_LOG(dev->ifname, ERR,
 				"Version mismatch between backend (0x%x) & reconnection file (0x%x)",
-				VHOST_RECONNECT_VERSION, reco_log->version);
+				VHOST_RECONNECT_VERSION, dev->reconnect_log->version);
 		return -1;
 	}
 
-	if ((reco_log->features & features) != reco_log->features) {
-		VHOST_CONFIG_LOG(dev_name, ERR,
+	if ((dev->reconnect_log->features & features) != dev->reconnect_log->features) {
+		VHOST_CONFIG_LOG(dev->ifname, ERR,
 				"Features mismatch between backend (0x%" PRIx64 ") & reconnection file (0x%" PRIx64 ")",
-				features, reco_log->features);
+				features, dev->reconnect_log->features);
 		return -1;
 	}
 
-	if (reco_log->nr_vrings != total_queues) {
-		VHOST_CONFIG_LOG(dev_name, ERR,
+	if (dev->reconnect_log->nr_vrings != total_queues) {
+		VHOST_CONFIG_LOG(dev->ifname, ERR,
 				"Queues number mismatch between backend (%u) and reconnection file (%u)",
-				total_queues, reco_log->nr_vrings);
+				total_queues, dev->reconnect_log->nr_vrings);
 		return -1;
 	}
 
@@ -624,7 +624,6 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	uint64_t features;
 	struct vduse_dev_config *dev_config = NULL;
 	const char *name = path + strlen("/dev/vduse/");
-	struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
 	bool reconnect = false;
 
 	if (vduse.fdset == NULL) {
@@ -673,33 +672,13 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	if (dev_fd >= 0) {
 		VHOST_CONFIG_LOG(name, INFO, "Device already exists, reconnecting...");
 		reconnect = true;
-
-		ret = vduse_reconnect_log_map(name, &reconnect_log, false);
-		if (ret < 0) {
-			VHOST_CONFIG_LOG(name, ERR, "Failed to map reconnect log file");
-			goto out_ctrl_close;
-		}
-
-		ret = vduse_reconnect_log_check(reconnect_log, name, features, total_queues);
-		if (ret < 0) {
-			VHOST_CONFIG_LOG(name, ERR, "Reconnection log file check failed");
-			goto out_log_unmap;
-		}
 	} else if (errno == ENOENT) {
-		ret = vduse_reconnect_log_map(name, &reconnect_log, true);
-		if (ret < 0) {
-			VHOST_CONFIG_LOG(name, ERR, "Failed to create reconnect log file");
-			goto out_ctrl_close;
-		}
-
-		reconnect_log->version = VHOST_RECONNECT_VERSION;
-
 		dev_config = malloc(offsetof(struct vduse_dev_config, config) +
 				sizeof(vnet_config));
 		if (!dev_config) {
 			VHOST_CONFIG_LOG(name, ERR, "Failed to allocate VDUSE config");
 			ret = -1;
-			goto out_log_unmap;
+			goto out_ctrl_close;
 		}
 
 		vnet_config.max_virtqueue_pairs = max_queue_pairs;
@@ -720,18 +699,15 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 		if (ret < 0) {
 			VHOST_CONFIG_LOG(name, ERR, "Failed to create VDUSE device: %s",
 					strerror(errno));
-			goto out_log_unmap;
+			goto out_ctrl_close;
 		}
 
-		memcpy(&reconnect_log->config, &vnet_config, sizeof(vnet_config));
-		reconnect_log->nr_vrings = total_queues;
-
 		dev_fd = open(path, O_RDWR);
 		if (dev_fd < 0) {
 			VHOST_CONFIG_LOG(name, ERR, "Failed to open newly created device %s: %s",
 					path, strerror(errno));
 			ret = -1;
-			goto out_log_unmap;
+			goto out_ctrl_close;
 		}
 	} else {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to open device %s: %s",
@@ -744,14 +720,14 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	if (ret < 0) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to set chardev as non-blocking: %s",
 				strerror(errno));
-		goto out_log_unmap;
+		goto out_dev_close;
 	}
 
 	vid = vhost_new_device(&vduse_backend_ops);
 	if (vid < 0) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to create new Vhost device");
 		ret = -1;
-		goto out_log_unmap;
+		goto out_dev_close;
 	}
 
 	dev = get_device(vid);
@@ -763,10 +739,25 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	strncpy(dev->ifname, path, IF_NAME_SZ - 1);
 	dev->vduse_ctrl_fd = control_fd;
 	dev->vduse_dev_fd = dev_fd;
-	dev->reconnect_log = reconnect_log;
-	reconnect_log = MAP_FAILED;
-	if (reconnect)
+
+	ret = vduse_reconnect_log_map(dev, !reconnect);
+	if (ret < 0) {
+		VHOST_CONFIG_LOG(name, ERR, "Failed to map reconnect log file");
+		goto out_dev_destroy;
+	}
+
+	if (reconnect) {
+		ret = vduse_reconnect_log_check(dev, features, total_queues);
+		if (ret < 0) {
+			VHOST_CONFIG_LOG(name, ERR, "Reconnection log file check failed");
+			goto out_log_unmap;
+		}
 		dev->status = dev->reconnect_log->status;
+	} else {
+		dev->reconnect_log->version = VHOST_RECONNECT_VERSION;
+		dev->reconnect_log->nr_vrings = total_queues;
+		memcpy(&dev->reconnect_log->config, &vnet_config, sizeof(vnet_config));
+	}
 
 	vhost_setup_virtio_net(dev->vid, true, compliant_ol_flags, true, true);
 
@@ -777,11 +768,11 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 		ret = alloc_vring_queue(dev, i);
 		if (ret) {
 			VHOST_CONFIG_LOG(name, ERR, "Failed to alloc vring %d metadata", i);
-			goto out_dev_destroy;
+			goto out_log_unmap;
 		}
 
 		vq = dev->virtqueue[i];
-		vq->reconnect_log = &reconnect_log->vring[i];
+		vq->reconnect_log = &dev->reconnect_log->vring[i];
 
 		if (reconnect)
 			continue;
@@ -792,7 +783,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 		ret = ioctl(dev->vduse_dev_fd, VDUSE_VQ_SETUP, &vq_cfg);
 		if (ret) {
 			VHOST_CONFIG_LOG(name, ERR, "Failed to set-up VQ %d", i);
-			goto out_dev_destroy;
+			goto out_log_unmap;
 		}
 	}
 
@@ -802,24 +793,23 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
 	if (ret) {
 		VHOST_CONFIG_LOG(name, ERR, "Failed to add fd %d to vduse fdset",
 				dev->vduse_dev_fd);
-		goto out_dev_destroy;
+		goto out_log_unmap;
 	}
 
 	if (reconnect && dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK)  {
 		ret = vduse_reconnect_start_device(dev);
 		if (ret) {
 			VHOST_CONFIG_LOG(name, ERR, "Failed to start device at reconnection");
-			goto out_dev_destroy;
+			goto out_log_unmap;
 		}
 	}
 
 	return 0;
 
+out_log_unmap:
+	munmap(dev->reconnect_log, sizeof(*dev->reconnect_log));
 out_dev_destroy:
 	vhost_destroy_device(vid);
-out_log_unmap:
-	if (reconnect_log != MAP_FAILED)
-		munmap(reconnect_log, sizeof(*reconnect_log));
 out_dev_close:
 	if (dev_fd >= 0)
 		close(dev_fd);
-- 
2.46.2

Re: [PATCH 1/6] vhost: fix vduse device creation error handling

[David Marchand]

On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch fixes missing reconnection log unmapping
> miss in the error path at VDUSE creation time.
>
> Coverity issue: 445525
> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

VDUSE* in the title, for consistency please.

Reviewed-by: David Marchand <david.marchand@redhat.com>

One comment below.

> ---
>  lib/vhost/vduse.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
> index f9ac317438..89bc29a33f 100644
> --- a/lib/vhost/vduse.c
> +++ b/lib/vhost/vduse.c
> @@ -488,7 +488,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>         struct vduse_dev_config *dev_config = NULL;

dev_config could be allocated on the stack, rather than the heap.

But at least, move dev_config declaration in the block where it is
used, this will limit the chance a future change introduces a leak.



>         const char *name = path + strlen("/dev/vduse/");
>         char reconnect_file[PATH_MAX];
> -       struct vhost_reconnect_data *reconnect_log = NULL;
> +       struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
>         bool reconnect = false;
>
>         if (vduse.fdset == NULL) {


-- 
David Marchand

Re: [PATCH 2/6] vhost: fix possible TOCTOU in VDUSE dev creation

[David Marchand]

On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch fixes a possible TOCTOU on the VDUSE
> device chardev opening at device creation time.
>
> Coverity issue: 445526
> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH 3/6] vhost: fix VDUSE reconnect device start failure

[David Marchand]

On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch fixes a FD leak in the VDUSE device reconnect
> code fails to start the device.
>
> Also take the opportunity to refactor the related code into
> a dedicated function.
>
> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/vduse.c | 63 +++++++++++++++++++++++++++++++----------------
>  1 file changed, 42 insertions(+), 21 deletions(-)
>
> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
> index d1373d0549..c8c4761293 100644
> --- a/lib/vhost/vduse.c
> +++ b/lib/vhost/vduse.c
> @@ -476,6 +476,46 @@ vduse_reconnect_handler(int fd, void *arg, int *remove)
>         *remove = 1;
>  }
>
> +static int
> +vduse_reconnect_start_device(struct virtio_net *dev)
> +{
> +       int fd, ret;
> +
> +       /*
> +        * Make vduse_device_start() being executed in the same
> +        * context for both reconnection and fresh startup.
> +        */
> +       fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> +       if (fd < 0) {
> +               VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to create reconnect efd: %s",
> +                               strerror(errno));
> +               ret = -1;
> +               goto out_err;
> +       }
> +
> +       ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, dev);
> +       if (ret) {
> +               VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to add reconnect efd %d to vduse fdset",
> +                               fd);
> +               goto out_err_close;
> +       }
> +
> +       ret = eventfd_write(fd, (eventfd_t)1);
> +       if (ret < 0) {
> +               VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to write to reconnect eventfd");
> +               goto out_err_fdset;
> +       }
> +
> +       return 0;
> +
> +out_err_fdset:
> +       fdset_del(vduse.fdset, fd);
> +out_err_close:
> +       close(fd);
> +out_err:
> +       return ret;
> +}
> +
>  int
>  vduse_device_create(const char *path, bool compliant_ol_flags)
>  {
> @@ -741,28 +781,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>         }
>
>         if (reconnect && dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK)  {
> -               /*
> -                * Make vduse_device_start() being executed in the same
> -                * context for both reconnection and fresh startup.
> -                */
> -               reco_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
> -               if (reco_fd < 0) {
> -                       VHOST_CONFIG_LOG(name, ERR, "Failed to create reco_fd: %s",
> -                                       strerror(errno));
> -                       ret = -1;
> -                       goto out_dev_destroy;
> -               }
> -
> -               ret = fdset_add(vduse.fdset, reco_fd, vduse_reconnect_handler, NULL, dev);
> +               ret = vduse_reconnect_start_device(dev);
>                 if (ret) {
> -                       VHOST_CONFIG_LOG(name, ERR, "Failed to add reconnect fd %d to vduse fdset",
> -                                       reco_fd);
> -                       goto out_dev_destroy;
> -               }
> -
> -               ret = eventfd_write(reco_fd, (eventfd_t)1);
> -               if (ret < 0) {
> -                       VHOST_CONFIG_LOG(name, ERR, "Failed to write to reconnect eventfd");
> +                       VHOST_CONFIG_LOG(name, ERR, "Failed to start device at reconnection");

We don't need a new log, there is already one for each error case in
vduse_reconnect_start_device().


-- 
David Marchand

Re: [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping

[David Marchand]

On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch moves the VDUSE reconnection log mapping, as
> well as creation if needed, into a dedicated function.
>
> This is a preliminary rework to simplify VDUSE device
> creation.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/vduse.c | 138 +++++++++++++++++++++++++---------------------
>  1 file changed, 75 insertions(+), 63 deletions(-)
>
> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
> index c8c4761293..985d4cc58d 100644
> --- a/lib/vhost/vduse.c
> +++ b/lib/vhost/vduse.c
> @@ -431,6 +431,9 @@ vduse_reconnect_path_init(void)
>         const char *directory;
>         int ret;
>
> +       if (vduse_reconnect_path_set == true)
> +               return 0;
> +
>         /* from RuntimeDirectory= see systemd.exec */
>         directory = getenv("RUNTIME_DIRECTORY");
>         if (directory == NULL) {
> @@ -462,9 +465,74 @@ vduse_reconnect_path_init(void)
>         VHOST_CONFIG_LOG("vduse", INFO, "Created VDUSE reconnect directory in %s",
>                         vduse_reconnect_dir);
>
> +       vduse_reconnect_path_set = true;
> +
>         return 0;
>  }
>
> +static int
> +vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco_log, bool create)

You can pass struct virtio_net *dev instead of patch 6.


> +{
> +       char reco_file[PATH_MAX];
> +       int fd, ret;
> +
> +       if (vduse_reconnect_path_init() < 0) {
> +               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to initialize reconnect path");
> +                       return -1;

Weird indent.


> +       }
> +
> +       ret = snprintf(reco_file, sizeof(reco_file), "%s/%s", vduse_reconnect_dir, dev_name);
> +       if (ret < 0 || ret == sizeof(reco_file)) {
> +               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to create vduse reconnect path name");
> +               return -1;
> +       }
> +
> +       if (create) {
> +               fd = open(reco_file, O_CREAT | O_EXCL | O_RDWR, 0600);
> +               if (fd < 0) {
> +                       if (errno == EEXIST) {
> +                               VHOST_CONFIG_LOG(dev_name, ERR, "Reconnect file %s exists but not the device",
> +                                               reco_file);
> +                       } else {
> +                               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
> +                                               reco_file, strerror(errno));
> +                       }
> +                       return -1;
> +               }
> +
> +               ret = ftruncate(fd, sizeof(**reco_log));
> +               if (ret < 0) {
> +                       VHOST_CONFIG_LOG(dev_name, ERR, "Failed to truncate reconnect file %s (%s)",
> +                                       reco_file, strerror(errno));
> +                       goto out_close;
> +               }
> +       } else {
> +               fd = open(reco_file, O_RDWR, 0600);
> +               if (fd < 0) {
> +                       if (errno == ENOENT)
> +                               VHOST_CONFIG_LOG(dev_name, ERR, "Missing reconnect file (%s)", reco_file);
> +                       else
> +                               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
> +                                               reco_file, strerror(errno));
> +                       return -1;
> +               }
> +       }
> +
> +       *reco_log = mmap(NULL, sizeof(**reco_log), PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> +       if (*reco_log == MAP_FAILED) {
> +               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to mmap reconnect file %s (%s)",
> +                               reco_file, strerror(errno));
> +               ret = -1;
> +               goto out_close;
> +       }
> +       ret = 0;
> +
> +out_close:
> +       close(fd);
> +
> +       return ret;
> +}
> +
>  static void
>  vduse_reconnect_handler(int fd, void *arg, int *remove)
>  {
> @@ -519,7 +587,7 @@ vduse_reconnect_start_device(struct virtio_net *dev)
>  int
>  vduse_device_create(const char *path, bool compliant_ol_flags)
>  {
> -       int control_fd, dev_fd, vid, ret, reco_fd;
> +       int control_fd, dev_fd, vid, ret;
>         uint32_t i, max_queue_pairs, total_queues;
>         struct virtio_net *dev;
>         struct virtio_net_config vnet_config = {{ 0 }};
> @@ -527,7 +595,6 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>         uint64_t features;
>         struct vduse_dev_config *dev_config = NULL;
>         const char *name = path + strlen("/dev/vduse/");
> -       char reconnect_file[PATH_MAX];
>         struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
>         bool reconnect = false;
>
> @@ -539,20 +606,6 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>                 }
>         }
>
> -       if (vduse_reconnect_path_set == false) {
> -               if (vduse_reconnect_path_init() < 0) {
> -                       VHOST_CONFIG_LOG(path, ERR, "failed to initialize reconnect path");
> -                       return -1;
> -               }
> -               vduse_reconnect_path_set = true;
> -       }
> -
> -       ret = snprintf(reconnect_file, sizeof(reconnect_file), "%s/%s", vduse_reconnect_dir, name);
> -       if (ret < 0 || ret == sizeof(reconnect_file)) {
> -               VHOST_CONFIG_LOG(name, ERR, "Failed to create vduse reconnect path name");
> -               return -1;
> -       }
> -
>         control_fd = open(VDUSE_CTRL_PATH, O_RDWR);
>         if (control_fd < 0) {
>                 VHOST_CONFIG_LOG(name, ERR, "Failed to open %s: %s",
> @@ -592,26 +645,10 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>                 VHOST_CONFIG_LOG(name, INFO, "Device already exists, reconnecting...");
>                 reconnect = true;
>
> -               reco_fd = open(reconnect_file, O_RDWR, 0600);
> -               if (reco_fd < 0) {
> -                       if (errno == ENOENT)
> -                               VHOST_CONFIG_LOG(name, ERR, "Missing reconnect file (%s)",
> -                                               reconnect_file);
> -                       else
> -                               VHOST_CONFIG_LOG(name, ERR, "Failed to open reconnect file %s (%s)",
> -                                               reconnect_file, strerror(errno));
> -                       ret = -1;
> -                       goto out_dev_close;
> -               }
> -
> -               reconnect_log = mmap(NULL, sizeof(*reconnect_log), PROT_READ | PROT_WRITE,
> -                               MAP_SHARED, reco_fd, 0);
> -               close(reco_fd);
> -               if (reconnect_log == MAP_FAILED) {
> -                       VHOST_CONFIG_LOG(name, ERR, "Failed to mmap reconnect file %s (%s)",
> -                                       reconnect_file, strerror(errno));
> -                       ret = -1;
> -                       goto out_dev_close;
> +               ret = vduse_reconnect_log_map(name, &reconnect_log, false);
> +               if (ret < 0) {
> +                       VHOST_CONFIG_LOG(name, ERR, "Failed to map reconnect log file");

No additional log, all error cases are covered, afaics.



> +                       goto out_ctrl_close;
>                 }
>
>                 if (reconnect_log->version != VHOST_RECONNECT_VERSION) {


-- 
David Marchand

Re: [PATCH 5/6] vhost: fix and refactor VDUSE reconnect log check

[David Marchand]

On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch fixes missing error handling in checking the
> reconnect log version, and takes the opportunity to move
> all the checks into a dedicated function to simply VDUSE
> device creation code.
>
> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Same comment, no additional log.


-- 
David Marchand

Re: [PATCH 1/6] vhost: fix vduse device creation error handling

[Maxime Coquelin]

On 10/23/24 18:06, David Marchand wrote:
> On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch fixes missing reconnection log unmapping
>> miss in the error path at VDUSE creation time.
>>
>> Coverity issue: 445525
>> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> VDUSE* in the title, for consistency please.

Fixed in v2.

> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> One comment below.
> 
>> ---
>>   lib/vhost/vduse.c | 24 +++++++++++++-----------
>>   1 file changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
>> index f9ac317438..89bc29a33f 100644
>> --- a/lib/vhost/vduse.c
>> +++ b/lib/vhost/vduse.c
>> @@ -488,7 +488,7 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>>          struct vduse_dev_config *dev_config = NULL;
> 
> dev_config could be allocated on the stack, rather than the heap.
> 
> But at least, move dev_config declaration in the block where it is
> used, this will limit the chance a future change introduces a leak.
> 

I moved dev_config declaration in the block, having it declared in the
heap makes it less readable IMHO.

> 
>>          const char *name = path + strlen("/dev/vduse/");
>>          char reconnect_file[PATH_MAX];
>> -       struct vhost_reconnect_data *reconnect_log = NULL;
>> +       struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
>>          bool reconnect = false;
>>
>>          if (vduse.fdset == NULL) {
> 
> 

Thanks,
Maxime

Re: [PATCH 3/6] vhost: fix VDUSE reconnect device start failure

[Maxime Coquelin]

On 10/23/24 18:06, David Marchand wrote:
> On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch fixes a FD leak in the VDUSE device reconnect
>> code fails to start the device.
>>
>> Also take the opportunity to refactor the related code into
>> a dedicated function.
>>
>> Fixes: da79cc7fda76 ("vhost: add reconnection support to VDUSE")
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/vhost/vduse.c | 63 +++++++++++++++++++++++++++++++----------------
>>   1 file changed, 42 insertions(+), 21 deletions(-)
>>
>> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
>> index d1373d0549..c8c4761293 100644
>> --- a/lib/vhost/vduse.c
>> +++ b/lib/vhost/vduse.c
>> @@ -476,6 +476,46 @@ vduse_reconnect_handler(int fd, void *arg, int *remove)
>>          *remove = 1;
>>   }
>>
>> +static int
>> +vduse_reconnect_start_device(struct virtio_net *dev)
>> +{
>> +       int fd, ret;
>> +
>> +       /*
>> +        * Make vduse_device_start() being executed in the same
>> +        * context for both reconnection and fresh startup.
>> +        */
>> +       fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
>> +       if (fd < 0) {
>> +               VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to create reconnect efd: %s",
>> +                               strerror(errno));
>> +               ret = -1;
>> +               goto out_err;
>> +       }
>> +
>> +       ret = fdset_add(vduse.fdset, fd, vduse_reconnect_handler, NULL, dev);
>> +       if (ret) {
>> +               VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to add reconnect efd %d to vduse fdset",
>> +                               fd);
>> +               goto out_err_close;
>> +       }
>> +
>> +       ret = eventfd_write(fd, (eventfd_t)1);
>> +       if (ret < 0) {
>> +               VHOST_CONFIG_LOG(dev->ifname, ERR, "Failed to write to reconnect eventfd");
>> +               goto out_err_fdset;
>> +       }
>> +
>> +       return 0;
>> +
>> +out_err_fdset:
>> +       fdset_del(vduse.fdset, fd);
>> +out_err_close:
>> +       close(fd);
>> +out_err:
>> +       return ret;
>> +}
>> +
>>   int
>>   vduse_device_create(const char *path, bool compliant_ol_flags)
>>   {
>> @@ -741,28 +781,9 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>>          }
>>
>>          if (reconnect && dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK)  {
>> -               /*
>> -                * Make vduse_device_start() being executed in the same
>> -                * context for both reconnection and fresh startup.
>> -                */
>> -               reco_fd = eventfd(0, EFD_NONBLOCK | EFD_CLOEXEC);
>> -               if (reco_fd < 0) {
>> -                       VHOST_CONFIG_LOG(name, ERR, "Failed to create reco_fd: %s",
>> -                                       strerror(errno));
>> -                       ret = -1;
>> -                       goto out_dev_destroy;
>> -               }
>> -
>> -               ret = fdset_add(vduse.fdset, reco_fd, vduse_reconnect_handler, NULL, dev);
>> +               ret = vduse_reconnect_start_device(dev);
>>                  if (ret) {
>> -                       VHOST_CONFIG_LOG(name, ERR, "Failed to add reconnect fd %d to vduse fdset",
>> -                                       reco_fd);
>> -                       goto out_dev_destroy;
>> -               }
>> -
>> -               ret = eventfd_write(reco_fd, (eventfd_t)1);
>> -               if (ret < 0) {
>> -                       VHOST_CONFIG_LOG(name, ERR, "Failed to write to reconnect eventfd");
>> +                       VHOST_CONFIG_LOG(name, ERR, "Failed to start device at reconnection");
> 
> We don't need a new log, there is already one for each error case in
> vduse_reconnect_start_device().
> 
> 

Removed in v2.

Thanks,
Maxime

Re: [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping

[Maxime Coquelin]

On 10/23/24 18:07, David Marchand wrote:
> On Wed, Oct 23, 2024 at 5:16 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> This patch moves the VDUSE reconnection log mapping, as
>> well as creation if needed, into a dedicated function.
>>
>> This is a preliminary rework to simplify VDUSE device
>> creation.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/vhost/vduse.c | 138 +++++++++++++++++++++++++---------------------
>>   1 file changed, 75 insertions(+), 63 deletions(-)
>>
>> diff --git a/lib/vhost/vduse.c b/lib/vhost/vduse.c
>> index c8c4761293..985d4cc58d 100644
>> --- a/lib/vhost/vduse.c
>> +++ b/lib/vhost/vduse.c
>> @@ -431,6 +431,9 @@ vduse_reconnect_path_init(void)
>>          const char *directory;
>>          int ret;
>>
>> +       if (vduse_reconnect_path_set == true)
>> +               return 0;
>> +
>>          /* from RuntimeDirectory= see systemd.exec */
>>          directory = getenv("RUNTIME_DIRECTORY");
>>          if (directory == NULL) {
>> @@ -462,9 +465,74 @@ vduse_reconnect_path_init(void)
>>          VHOST_CONFIG_LOG("vduse", INFO, "Created VDUSE reconnect directory in %s",
>>                          vduse_reconnect_dir);
>>
>> +       vduse_reconnect_path_set = true;
>> +
>>          return 0;
>>   }
>>
>> +static int
>> +vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco_log, bool create)
> 
> You can pass struct virtio_net *dev instead of patch 6.

At patch 4, the struct virtio_net *dev has not yet been allocated when
this function is called.

Or, maybe you mean I should squash patch 6 with this one?

> 
> 
>> +{
>> +       char reco_file[PATH_MAX];
>> +       int fd, ret;
>> +
>> +       if (vduse_reconnect_path_init() < 0) {
>> +               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to initialize reconnect path");
>> +                       return -1;
> 
> Weird indent.

Fixed.

> 
>> +       }
>> +
>> +       ret = snprintf(reco_file, sizeof(reco_file), "%s/%s", vduse_reconnect_dir, dev_name);
>> +       if (ret < 0 || ret == sizeof(reco_file)) {
>> +               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to create vduse reconnect path name");
>> +               return -1;
>> +       }
>> +
>> +       if (create) {
>> +               fd = open(reco_file, O_CREAT | O_EXCL | O_RDWR, 0600);
>> +               if (fd < 0) {
>> +                       if (errno == EEXIST) {
>> +                               VHOST_CONFIG_LOG(dev_name, ERR, "Reconnect file %s exists but not the device",
>> +                                               reco_file);
>> +                       } else {
>> +                               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
>> +                                               reco_file, strerror(errno));
>> +                       }
>> +                       return -1;
>> +               }
>> +
>> +               ret = ftruncate(fd, sizeof(**reco_log));
>> +               if (ret < 0) {
>> +                       VHOST_CONFIG_LOG(dev_name, ERR, "Failed to truncate reconnect file %s (%s)",
>> +                                       reco_file, strerror(errno));
>> +                       goto out_close;
>> +               }
>> +       } else {
>> +               fd = open(reco_file, O_RDWR, 0600);
>> +               if (fd < 0) {
>> +                       if (errno == ENOENT)
>> +                               VHOST_CONFIG_LOG(dev_name, ERR, "Missing reconnect file (%s)", reco_file);
>> +                       else
>> +                               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to open reconnect file %s (%s)",
>> +                                               reco_file, strerror(errno));
>> +                       return -1;
>> +               }
>> +       }
>> +
>> +       *reco_log = mmap(NULL, sizeof(**reco_log), PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>> +       if (*reco_log == MAP_FAILED) {
>> +               VHOST_CONFIG_LOG(dev_name, ERR, "Failed to mmap reconnect file %s (%s)",
>> +                               reco_file, strerror(errno));
>> +               ret = -1;
>> +               goto out_close;
>> +       }
>> +       ret = 0;
>> +
>> +out_close:
>> +       close(fd);
>> +
>> +       return ret;
>> +}
>> +
>>   static void
>>   vduse_reconnect_handler(int fd, void *arg, int *remove)
>>   {
>> @@ -519,7 +587,7 @@ vduse_reconnect_start_device(struct virtio_net *dev)
>>   int
>>   vduse_device_create(const char *path, bool compliant_ol_flags)
>>   {
>> -       int control_fd, dev_fd, vid, ret, reco_fd;
>> +       int control_fd, dev_fd, vid, ret;
>>          uint32_t i, max_queue_pairs, total_queues;
>>          struct virtio_net *dev;
>>          struct virtio_net_config vnet_config = {{ 0 }};
>> @@ -527,7 +595,6 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>>          uint64_t features;
>>          struct vduse_dev_config *dev_config = NULL;
>>          const char *name = path + strlen("/dev/vduse/");
>> -       char reconnect_file[PATH_MAX];
>>          struct vhost_reconnect_data *reconnect_log = MAP_FAILED;
>>          bool reconnect = false;
>>
>> @@ -539,20 +606,6 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>>                  }
>>          }
>>
>> -       if (vduse_reconnect_path_set == false) {
>> -               if (vduse_reconnect_path_init() < 0) {
>> -                       VHOST_CONFIG_LOG(path, ERR, "failed to initialize reconnect path");
>> -                       return -1;
>> -               }
>> -               vduse_reconnect_path_set = true;
>> -       }
>> -
>> -       ret = snprintf(reconnect_file, sizeof(reconnect_file), "%s/%s", vduse_reconnect_dir, name);
>> -       if (ret < 0 || ret == sizeof(reconnect_file)) {
>> -               VHOST_CONFIG_LOG(name, ERR, "Failed to create vduse reconnect path name");
>> -               return -1;
>> -       }
>> -
>>          control_fd = open(VDUSE_CTRL_PATH, O_RDWR);
>>          if (control_fd < 0) {
>>                  VHOST_CONFIG_LOG(name, ERR, "Failed to open %s: %s",
>> @@ -592,26 +645,10 @@ vduse_device_create(const char *path, bool compliant_ol_flags)
>>                  VHOST_CONFIG_LOG(name, INFO, "Device already exists, reconnecting...");
>>                  reconnect = true;
>>
>> -               reco_fd = open(reconnect_file, O_RDWR, 0600);
>> -               if (reco_fd < 0) {
>> -                       if (errno == ENOENT)
>> -                               VHOST_CONFIG_LOG(name, ERR, "Missing reconnect file (%s)",
>> -                                               reconnect_file);
>> -                       else
>> -                               VHOST_CONFIG_LOG(name, ERR, "Failed to open reconnect file %s (%s)",
>> -                                               reconnect_file, strerror(errno));
>> -                       ret = -1;
>> -                       goto out_dev_close;
>> -               }
>> -
>> -               reconnect_log = mmap(NULL, sizeof(*reconnect_log), PROT_READ | PROT_WRITE,
>> -                               MAP_SHARED, reco_fd, 0);
>> -               close(reco_fd);
>> -               if (reconnect_log == MAP_FAILED) {
>> -                       VHOST_CONFIG_LOG(name, ERR, "Failed to mmap reconnect file %s (%s)",
>> -                                       reconnect_file, strerror(errno));
>> -                       ret = -1;
>> -                       goto out_dev_close;
>> +               ret = vduse_reconnect_log_map(name, &reconnect_log, false);
>> +               if (ret < 0) {
>> +                       VHOST_CONFIG_LOG(name, ERR, "Failed to map reconnect log file");
> 
> No additional log, all error cases are covered, afaics.

Ack.

> 
> 
>> +                       goto out_ctrl_close;

and it should be out_dev_close here, will change in v2.

Thanks,
Maxime

>>                  }
>>
>>                  if (reconnect_log->version != VHOST_RECONNECT_VERSION) {
> 
> 

Re: [PATCH 4/6] vhost: refactor VDUSE reconnection log mapping

[David Marchand]

On Wed, Oct 23, 2024 at 9:04 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> >> +static int
> >> +vduse_reconnect_log_map(const char *dev_name, struct vhost_reconnect_data **reco_log, bool create)
> >
> > You can pass struct virtio_net *dev instead of patch 6.
>
> At patch 4, the struct virtio_net *dev has not yet been allocated when
> this function is called.

Ah yes, I missed this point.

>
> Or, maybe you mean I should squash patch 6 with this one?

No keep it as is, moving patches or squashing does not improve the diff.


-- 
David Marchand