[dpdk-dev] [PATCH v3] lib/librte_vhost: move fdset_del out of conn_mutex

[dpdk-dev] [PATCH v3] lib/librte_vhost: move fdset_del out of conn_mutex

[zhike wang]

From: wang zhike <wangzhike@jd.com>

v3:
* Fix duplicate variable name, which leads to unexpected memory write.
v2:
* Move fdset_del before conn destroy.
* Fix coding style.

This patch fixes below race condition:
1. one thread calls: rte_vhost_driver_unregister->lock conn_mutex
   ->fdset_del->loop to check fd.busy.
2. another thread calls fdset_event_dispatch, and the busy flag is
   changed AFTER handling on the fd, i.e, rcb(). However, the rcb,
   such as vhost_user_read_cb() would try to retrieve the conn_mutex.

So issue is that the 1st thread will loop check the flag while holding
the mutex, while the 2nd thread would be blocked by mutex and can not
change the flag. Then dead lock is observed.

Signed-off-by: zhike wang <wangzhike@jd.com>
---
 lib/librte_vhost/socket.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 422da00..ea01327 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -749,6 +749,9 @@ struct vhost_user_reconnect_list {
 		struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
 
 		if (!strcmp(vsocket->path, path)) {
+			int del_fds[MAX_FDS];
+			int num_of_fds = 0, fd_index;
+
 			if (vsocket->is_server) {
 				fdset_del(&vhost_user.fdset, vsocket->socket_fd);
 				close(vsocket->socket_fd);
@@ -757,13 +760,26 @@ struct vhost_user_reconnect_list {
 				vhost_user_remove_reconnect(vsocket);
 			}
 
+			/* fdset_del() must be called without conn_mutex. */
+			pthread_mutex_lock(&vsocket->conn_mutex);
+			for (conn = TAILQ_FIRST(&vsocket->conn_list);
+			     conn != NULL;
+			     conn = next) {
+				next = TAILQ_NEXT(conn, next);
+
+				del_fds[num_of_fds++] = conn->connfd;
+			}
+			pthread_mutex_unlock(&vsocket->conn_mutex);
+
+			for (fd_index = 0; fd_index < num_of_fds; fd_index++)
+				fdset_del(&vhost_user.fdset, del_fds[fd_index]);
+
 			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);
 				RTE_LOG(INFO, VHOST_CONFIG,
 					"free connfd = %d for device '%s'\n",
 					conn->connfd, path);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] lib/librte_vhost: move fdset_del out of conn_mutex

[王志克]

Hi Yuanhan, Huawei,

Can you please spare some time for code review? Thanks.

Br,
Zhike

-----Original Message-----
From: 王志克 
Sent: Tuesday, January 02, 2018 6:09 PM
To: dev@dpdk.org
Cc: 王志克
Subject: [PATCH v3] lib/librte_vhost: move fdset_del out of conn_mutex

From: wang zhike <wangzhike@jd.com>

v3:
* Fix duplicate variable name, which leads to unexpected memory write.
v2:
* Move fdset_del before conn destroy.
* Fix coding style.

This patch fixes below race condition:
1. one thread calls: rte_vhost_driver_unregister->lock conn_mutex
   ->fdset_del->loop to check fd.busy.
2. another thread calls fdset_event_dispatch, and the busy flag is
   changed AFTER handling on the fd, i.e, rcb(). However, the rcb,
   such as vhost_user_read_cb() would try to retrieve the conn_mutex.

So issue is that the 1st thread will loop check the flag while holding
the mutex, while the 2nd thread would be blocked by mutex and can not
change the flag. Then dead lock is observed.

Signed-off-by: zhike wang <wangzhike@jd.com>
---
 lib/librte_vhost/socket.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 422da00..ea01327 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -749,6 +749,9 @@ struct vhost_user_reconnect_list {
 		struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
 
 		if (!strcmp(vsocket->path, path)) {
+			int del_fds[MAX_FDS];
+			int num_of_fds = 0, fd_index;
+
 			if (vsocket->is_server) {
 				fdset_del(&vhost_user.fdset, vsocket->socket_fd);
 				close(vsocket->socket_fd);
@@ -757,13 +760,26 @@ struct vhost_user_reconnect_list {
 				vhost_user_remove_reconnect(vsocket);
 			}
 
+			/* fdset_del() must be called without conn_mutex. */
+			pthread_mutex_lock(&vsocket->conn_mutex);
+			for (conn = TAILQ_FIRST(&vsocket->conn_list);
+			     conn != NULL;
+			     conn = next) {
+				next = TAILQ_NEXT(conn, next);
+
+				del_fds[num_of_fds++] = conn->connfd;
+			}
+			pthread_mutex_unlock(&vsocket->conn_mutex);
+
+			for (fd_index = 0; fd_index < num_of_fds; fd_index++)
+				fdset_del(&vhost_user.fdset, del_fds[fd_index]);
+
 			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);
 				RTE_LOG(INFO, VHOST_CONFIG,
 					"free connfd = %d for device '%s'\n",
 					conn->connfd, path);
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH v3] lib/librte_vhost: move fdset_del out of conn_mutex

[Yuanhan Liu]

Hi,

Apologize for late review.

On Tue, Jan 02, 2018 at 02:08:36AM -0800, zhike wang wrote:
> From: wang zhike <wangzhike@jd.com>
> 
> v3:
> * Fix duplicate variable name, which leads to unexpected memory write.
> v2:
> * Move fdset_del before conn destroy.
> * Fix coding style.

Note that we prefer to put the change logs after "---" below Signed-off-by,
so that those change logs won't be tracked in the git log history.

> This patch fixes below race condition:
> 1. one thread calls: rte_vhost_driver_unregister->lock conn_mutex
>    ->fdset_del->loop to check fd.busy.
> 2. another thread calls fdset_event_dispatch, and the busy flag is
>    changed AFTER handling on the fd, i.e, rcb(). However, the rcb,
>    such as vhost_user_read_cb() would try to retrieve the conn_mutex.
> 
> So issue is that the 1st thread will loop check the flag while holding
> the mutex, while the 2nd thread would be blocked by mutex and can not
> change the flag. Then dead lock is observed.

I then would change the title to "vhost: fix deadlock".

I'm also keen to know how do you reproduce this issue with real-life
APP (say ovs) and how easy it is for reproduce.

> Signed-off-by: zhike wang <wangzhike@jd.com>

Again, you need fix your git config file about your name.

> ---
>  lib/librte_vhost/socket.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index 422da00..ea01327 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -749,6 +749,9 @@ struct vhost_user_reconnect_list {
>  		struct vhost_user_socket *vsocket = vhost_user.vsockets[i];
>  
>  		if (!strcmp(vsocket->path, path)) {
> +			int del_fds[MAX_FDS];
> +			int num_of_fds = 0, fd_index;
> +

I think the naming could be a bit shorter, like "fds, nr_fds (or nb_fds),
fd_idx".

>  			if (vsocket->is_server) {
>  				fdset_del(&vhost_user.fdset, vsocket->socket_fd);
>  				close(vsocket->socket_fd);
> @@ -757,13 +760,26 @@ struct vhost_user_reconnect_list {
>  				vhost_user_remove_reconnect(vsocket);
>  			}
>  
> +			/* fdset_del() must be called without conn_mutex. */
> +			pthread_mutex_lock(&vsocket->conn_mutex);
> +			for (conn = TAILQ_FIRST(&vsocket->conn_list);
> +			     conn != NULL;
> +			     conn = next) {
> +				next = TAILQ_NEXT(conn, next);
> +
> +				del_fds[num_of_fds++] = conn->connfd;
> +			}
> +			pthread_mutex_unlock(&vsocket->conn_mutex);
> +
> +			for (fd_index = 0; fd_index < num_of_fds; fd_index++)
> +				fdset_del(&vhost_user.fdset, del_fds[fd_index]);
> +
>  			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 you log the fd here and invoke fdset_del() and close() after the loop,
you then could avoid one extra loop as you did above.

	--yliu
>  				RTE_LOG(INFO, VHOST_CONFIG,
>  					"free connfd = %d for device '%s'\n",
>  					conn->connfd, path);
> -- 
> 1.8.3.1