diff mbox

[dpdk-dev,2/3] vhost: fix crash and fd leak due to vhostuser destroyed

Message ID 1524842385-61707-2-git-send-email-xiangxia.m.yue@gmail.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Tonghao Zhang April 27, 2018, 3:19 p.m. UTC 
  From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

when rte_vhost_driver_unregister detstroy the vsocket, we
should set it to NULL after freeing it, because in client mode,
the conn may be added to reconnect thread while vsocket is
destroyed. In one case, if qemu create vhostuser port as a
server with the same unix path, the reconnect thread will
reconnect to it while vsocket is destroyed.

To fix this:
1. set vsocket to NULL after free it.
2. remove the reconnection from reconnection thread in suitable
   position.

Cc: stable@dpdk.org
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 lib/librte_vhost/socket.c | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

Comments

Maxime Coquelin May 4, 2018, 1:02 p.m. UTC | #1 
On 04/27/2018 05:19 PM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> when rte_vhost_driver_unregister detstroy the vsocket, we
> should set it to NULL after freeing it, because in client mode,
> the conn may be added to reconnect thread while vsocket is
> destroyed. In one case, if qemu create vhostuser port as a
> server with the same unix path, the reconnect thread will
> reconnect to it while vsocket is destroyed.
> 
> To fix this:
> 1. set vsocket to NULL after free it.
> 2. remove the reconnection from reconnection thread in suitable
>     position.
> 
> Cc: stable@dpdk.org
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>   lib/librte_vhost/socket.c | 41 ++++++++++++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 13 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
Maxime Coquelin May 4, 2018, 3:11 p.m. UTC | #2 
On 04/27/2018 05:19 PM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> when rte_vhost_driver_unregister detstroy the vsocket, we
> should set it to NULL after freeing it, because in client mode,
> the conn may be added to reconnect thread while vsocket is
> destroyed. In one case, if qemu create vhostuser port as a
> server with the same unix path, the reconnect thread will
> reconnect to it while vsocket is destroyed.
> 
> To fix this:
> 1. set vsocket to NULL after free it.
> 2. remove the reconnection from reconnection thread in suitable
>     position.
> 
> Cc: stable@dpdk.org
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>   lib/librte_vhost/socket.c | 41 ++++++++++++++++++++++++++++-------------
>   1 file changed, 28 insertions(+), 13 deletions(-)
> 

Applied to dpdk-next-virtio/master.

Thanks,
Maxime
diff mbox

Patch

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 822db41..d5a6ac8 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -199,6 +199,9 @@  struct vhost_user {
 	struct vhost_user_connection *conn;
 	int ret;
 
+	if (vsocket == NULL)
+		return;
+
 	conn = malloc(sizeof(*conn));
 	if (conn == NULL) {
 		close(fd);
@@ -778,6 +781,20 @@  struct vhost_user_reconnect_list {
 	return ret;
 }
 
+static void
+vhost_user_socket_mem_free(struct vhost_user_socket *vsocket)
+{
+	if (vsocket && vsocket->path) {
+		free(vsocket->path);
+		vsocket->path = NULL;
+	}
+
+	if (vsocket) {
+		free(vsocket);
+		vsocket = NULL;
+	}
+}
+
 /*
  * Register a new vhost-user socket; here we could act as server
  * (the default case), or client (when RTE_VHOST_USER_CLIENT) flag
@@ -808,7 +825,7 @@  struct vhost_user_reconnect_list {
 	if (vsocket->path == NULL) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"error: failed to copy socket path string\n");
-		free(vsocket);
+		vhost_user_socket_mem_free(vsocket);
 		goto out;
 	}
 	TAILQ_INIT(&vsocket->conn_list);
@@ -866,8 +883,7 @@  struct vhost_user_reconnect_list {
 			"error: failed to destroy connection mutex\n");
 	}
 out_free:
-	free(vsocket->path);
-	free(vsocket);
+	vhost_user_socket_mem_free(vsocket);
 out:
 	pthread_mutex_unlock(&vhost_user.mutex);
 
@@ -914,14 +930,6 @@  struct vhost_user_reconnect_list {
 		struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
 
 		if (!strcmp(vsocket->path, path)) {
-			if (vsocket->is_server) {
-				fdset_del(&vhost_user.fdset, vsocket->socket_fd);
-				close(vsocket->socket_fd);
-				unlink(path);
-			} else if (vsocket->reconnect) {
-				vhost_user_remove_reconnect(vsocket);
-			}
-
 again:
 			pthread_mutex_lock(&vsocket->conn_mutex);
 			for (conn = TAILQ_FIRST(&vsocket->conn_list);
@@ -950,9 +958,16 @@  struct vhost_user_reconnect_list {
 			}
 			pthread_mutex_unlock(&vsocket->conn_mutex);
 
+			if (vsocket->is_server) {
+				fdset_del(&vhost_user.fdset, vsocket->socket_fd);
+				close(vsocket->socket_fd);
+				unlink(path);
+			} else if (vsocket->reconnect) {
+				vhost_user_remove_reconnect(vsocket);
+			}
+
 			pthread_mutex_destroy(&vsocket->conn_mutex);
-			free(vsocket->path);
-			free(vsocket);
+			vhost_user_socket_mem_free(vsocket);
 
 			count = --vhost_user.vsocket_cnt;
 			vhost_user.vsockets[i] = vhost_user.vsockets[count];