[PATCH 1/3] vhost: fix deadlock due to vhostuser socket and fdset

[PATCH 1/3] vhost: fix deadlock due to vhostuser socket and fdset

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

When qemu close the unix socket fd of the vhostuser as a
server, and then immediately delete the vhostuser port on
openvswitch. There will be a deadlock.

A thread (fdset event thread):       B thread:
1. fdset_event_dispatch              rte_vhost_driver_unregister
2. set the fd busy to 1.             lock vsocket->conn_mutex
3. vhost_user_read_cb                fdset_del waits busy changed to 0.
4. vhost peer closed, remove the
   conn from vsocket->conn_list:
   lock vsocket->conn_mutex

5. set the fd busy to 0

Fixes: 65388b43 ("vhost: fix fd leaks for vhost-user server mode")
Cc: stable@dpdk.org
Cc: Yuanhan Liu <yliu@fridaylinux.org>
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 lib/librte_vhost/fd_man.c | 32 ++++++++++++++++++++++++++++++++
 lib/librte_vhost/fd_man.h |  1 +
 lib/librte_vhost/socket.c | 13 ++++++++++++-
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index 8590ee5..b24c27d 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -174,6 +174,38 @@
 	return dat;
 }
 
+/**
+ *  Unregister the fd from the fdset.
+ *
+ *  If parameters are invalid, return directly -2.
+ *  And check whether fd is busy, if yes, return -1.
+ *  Otherwise, try to delete the fd from fdset and
+ *  return true.
+ */
+int
+fdset_try_del(struct fdset *pfdset, int fd)
+{
+	int i;
+
+	if (pfdset == NULL || fd == -1)
+		return -2;
+
+	pthread_mutex_lock(&pfdset->fd_mutex);
+	i = fdset_find_fd(pfdset, fd);
+	if (i != -1 && pfdset->fd[i].busy) {
+		pthread_mutex_unlock(&pfdset->fd_mutex);
+		return -1;
+	}
+
+	if (i != -1) {
+		pfdset->fd[i].fd = -1;
+		pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
+		pfdset->fd[i].dat = NULL;
+	}
+
+	pthread_mutex_unlock(&pfdset->fd_mutex);
+	return 0;
+}
 
 /**
  * This functions runs in infinite blocking loop until there is no fd in
diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
index 76a42fb..3331bcd 100644
--- a/lib/librte_vhost/fd_man.h
+++ b/lib/librte_vhost/fd_man.h
@@ -44,6 +44,7 @@ int fdset_add(struct fdset *pfdset, int fd,
 	fd_cb rcb, fd_cb wcb, void *dat);
 
 void *fdset_del(struct fdset *pfdset, int fd);
+int fdset_try_del(struct fdset *pfdset, int fd);
 
 void *fdset_event_dispatch(void *arg);
 
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 4a561ad..822db41 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -922,13 +922,24 @@ struct vhost_user_reconnect_list {
 				vhost_user_remove_reconnect(vsocket);
 			}
 
+again:
 			pthread_mutex_lock(&vsocket->conn_mutex);
 			for (conn = TAILQ_FIRST(&vsocket->conn_list);
 			     conn != NULL;
 			     conn = next) {
 				next = TAILQ_NEXT(conn, next);
 
-				fdset_del(&vhost_user.fdset, conn->connfd);
+				/*
+				 * If r/wcb is executing, release the
+				 * conn_mutex lock, and try again since
+				 * the r/wcb may use the conn_mutex lock.
+				 */
+				if (fdset_try_del(&vhost_user.fdset,
+						  conn->connfd) == -1) {
+					pthread_mutex_unlock(&vsocket->conn_mutex);
+					goto again;
+				}
+
 				RTE_LOG(INFO, VHOST_CONFIG,
 					"free connfd = %d for device '%s'\n",
 					conn->connfd, path);
-- 
1.8.3.1

[PATCH 2/3] vhost: fix crash and fd leak due to vhostuser destroyed

[xiangxia.m.yue]

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

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];
-- 
1.8.3.1

[PATCH 3/3] vhost: fix typo in fdset_event_dispatch comment

[xiangxia.m.yue]

From: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 lib/librte_vhost/fd_man.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
index b24c27d..38347ab 100644
--- a/lib/librte_vhost/fd_man.c
+++ b/lib/librte_vhost/fd_man.c
@@ -293,7 +293,7 @@
 			 * because the fd is closed in the cb,
 			 * the old fd val could be reused by when creates new
 			 * listen fd in another thread, we couldn't call
-			 * fd_set_del.
+			 * fdset_del.
 			 */
 			if (remove1 || remove2) {
 				pfdentry->fd = -1;
-- 
1.8.3.1

Re: [PATCH 1/3] vhost: fix deadlock due to vhostuser socket and fdset

[Maxime Coquelin]

On 04/27/2018 05:19 PM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> When qemu close the unix socket fd of the vhostuser as a
> server, and then immediately delete the vhostuser port on
> openvswitch. There will be a deadlock.
> 
> A thread (fdset event thread):       B thread:
> 1. fdset_event_dispatch              rte_vhost_driver_unregister
> 2. set the fd busy to 1.             lock vsocket->conn_mutex
> 3. vhost_user_read_cb                fdset_del waits busy changed to 0.
> 4. vhost peer closed, remove the
>     conn from vsocket->conn_list:
>     lock vsocket->conn_mutex
> 
> 5. set the fd busy to 0
> 
> Fixes: 65388b43 ("vhost: fix fd leaks for vhost-user server mode")
> Cc: stable@dpdk.org
> Cc: Yuanhan Liu <yliu@fridaylinux.org>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>   lib/librte_vhost/fd_man.c | 32 ++++++++++++++++++++++++++++++++
>   lib/librte_vhost/fd_man.h |  1 +
>   lib/librte_vhost/socket.c | 13 ++++++++++++-
>   3 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
> index 8590ee5..b24c27d 100644
> --- a/lib/librte_vhost/fd_man.c
> +++ b/lib/librte_vhost/fd_man.c
> @@ -174,6 +174,38 @@
>   	return dat;
>   }
>   
> +/**
> + *  Unregister the fd from the fdset.
> + *
> + *  If parameters are invalid, return directly -2.
> + *  And check whether fd is busy, if yes, return -1.
> + *  Otherwise, try to delete the fd from fdset and
> + *  return true.
> + */
> +int
> +fdset_try_del(struct fdset *pfdset, int fd)
> +{
> +	int i;
> +
> +	if (pfdset == NULL || fd == -1)
> +		return -2;
> +
> +	pthread_mutex_lock(&pfdset->fd_mutex);
> +	i = fdset_find_fd(pfdset, fd);
> +	if (i != -1 && pfdset->fd[i].busy) {
> +		pthread_mutex_unlock(&pfdset->fd_mutex);
> +		return -1;
> +	}
> +
> +	if (i != -1) {
> +		pfdset->fd[i].fd = -1;
> +		pfdset->fd[i].rcb = pfdset->fd[i].wcb = NULL;
> +		pfdset->fd[i].dat = NULL;
> +	}
> +
> +	pthread_mutex_unlock(&pfdset->fd_mutex);
> +	return 0;
> +}
>   
>   /**
>    * This functions runs in infinite blocking loop until there is no fd in
> diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
> index 76a42fb..3331bcd 100644
> --- a/lib/librte_vhost/fd_man.h
> +++ b/lib/librte_vhost/fd_man.h
> @@ -44,6 +44,7 @@ int fdset_add(struct fdset *pfdset, int fd,
>   	fd_cb rcb, fd_cb wcb, void *dat);
>   
>   void *fdset_del(struct fdset *pfdset, int fd);
> +int fdset_try_del(struct fdset *pfdset, int fd);
>   
>   void *fdset_event_dispatch(void *arg);
>   
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index 4a561ad..822db41 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -922,13 +922,24 @@ struct vhost_user_reconnect_list {
>   				vhost_user_remove_reconnect(vsocket);
>   			}
>   
> +again:
>   			pthread_mutex_lock(&vsocket->conn_mutex);
>   			for (conn = TAILQ_FIRST(&vsocket->conn_list);
>   			     conn != NULL;
>   			     conn = next) {
>   				next = TAILQ_NEXT(conn, next);
>   
> -				fdset_del(&vhost_user.fdset, conn->connfd);
> +				/*
> +				 * If r/wcb is executing, release the
> +				 * conn_mutex lock, and try again since
> +				 * the r/wcb may use the conn_mutex lock.
> +				 */
> +				if (fdset_try_del(&vhost_user.fdset,
> +						  conn->connfd) == -1) {
> +					pthread_mutex_unlock(&vsocket->conn_mutex);
> +					goto again;
> +				}
> +
>   				RTE_LOG(INFO, VHOST_CONFIG,
>   					"free connfd = %d for device '%s'\n",
>   					conn->connfd, path);
> 

It looks a bit fragile at first, but I don't have a better alternative
in mind, so:

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

Thanks,
Maxime

Re: [PATCH 2/3] vhost: fix crash and fd leak due to vhostuser destroyed

[Maxime Coquelin]

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

Re: [PATCH 3/3] vhost: fix typo in fdset_event_dispatch comment

[Maxime Coquelin]

On 04/27/2018 05:19 PM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>   lib/librte_vhost/fd_man.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

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

Thanks,
Maxime

Re: [PATCH 1/3] vhost: fix deadlock due to vhostuser socket and fdset

[Maxime Coquelin]

On 04/27/2018 05:19 PM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> 
> When qemu close the unix socket fd of the vhostuser as a
> server, and then immediately delete the vhostuser port on
> openvswitch. There will be a deadlock.
> 
> A thread (fdset event thread):       B thread:
> 1. fdset_event_dispatch              rte_vhost_driver_unregister
> 2. set the fd busy to 1.             lock vsocket->conn_mutex
> 3. vhost_user_read_cb                fdset_del waits busy changed to 0.
> 4. vhost peer closed, remove the
>     conn from vsocket->conn_list:
>     lock vsocket->conn_mutex
> 
> 5. set the fd busy to 0
> 
> Fixes: 65388b43 ("vhost: fix fd leaks for vhost-user server mode")
> Cc: stable@dpdk.org
> Cc: Yuanhan Liu <yliu@fridaylinux.org>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
>   lib/librte_vhost/fd_man.c | 32 ++++++++++++++++++++++++++++++++
>   lib/librte_vhost/fd_man.h |  1 +
>   lib/librte_vhost/socket.c | 13 ++++++++++++-
>   3 files changed, 45 insertions(+), 1 deletion(-)
> 

Applied to dpdk-next-virtio/master.

Please next time add a cover-letter and run check-git-log.sh and
checkpatch.sh scripts before submitting.

Thanks!
Maxime

Re: [PATCH 2/3] vhost: fix crash and fd leak due to vhostuser destroyed

[Maxime Coquelin]

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

Re: [PATCH 3/3] vhost: fix typo in fdset_event_dispatch comment

[Maxime Coquelin]

On 04/27/2018 05:19 PM, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang<xiangxia.m.yue@gmail.com>
> 
> Signed-off-by: Tonghao Zhang<xiangxia.m.yue@gmail.com>
> ---
>   lib/librte_vhost/fd_man.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Applied to dpdk-next-virtio/master.

Thanks,
Maxime