diff mbox

[dpdk-dev] vhost: fix double free on shutdown

Message ID 20180209171455.2904-1-tomaszx.kulasek@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Maxime Coquelin
Headers

Checks

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

Commit Message

Tomasz Kulasek Feb. 9, 2018, 5:14 p.m. UTC 
  The vhost connection can be closed concurrently from 2 places:
 * the connection thread itself
 * rte_vhost_driver_unregister

The connection thread will terminate the connection if any recv error
occurred. The unregister function will terminate the connection together
with the thread. However, there is no sychronization between those two.
The connection thread runs in the background without any mutex.

The rte_vhost_driver_unregister now signals the connection thread
to terminate itself and waits until it's killed.

Fixes: 65388b43f592 ("vhost: fix fd leaks for vhost-user server mode")
Cc: yuanhan.liu@linux.intel.com
Cc: stable@dpdk.org

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 lib/librte_vhost/socket.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

Comments

Jianfeng Tan Feb. 11, 2018, 1:54 a.m. UTC | #1 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tomasz Kulasek
> Sent: Saturday, February 10, 2018 1:15 AM
> To: yliu@fridaylinux.org
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; stable@dpdk.org; Stojaczyk,
> DariuszX
> Subject: [dpdk-dev] [PATCH] vhost: fix double free on shutdown
> 
> The vhost connection can be closed concurrently from 2 places:
>  * the connection thread itself
>  * rte_vhost_driver_unregister
> 
> The connection thread will terminate the connection if any recv error
> occurred. The unregister function will terminate the connection together
> with the thread. However, there is no sychronization between those two.
> The connection thread runs in the background without any mutex.

Isn't it already protected by vsocket->conn_mutex?

Thanks,
Jianfeng

> 
> The rte_vhost_driver_unregister now signals the connection thread
> to terminate itself and waits until it's killed.
> 
> Fixes: 65388b43f592 ("vhost: fix fd leaks for vhost-user server mode")
> Cc: yuanhan.liu@linux.intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> ---
>  lib/librte_vhost/socket.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index 83befdced..46ac88efd 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -735,7 +735,7 @@ rte_vhost_driver_unregister(const char *path)
>  {
>  	int i;
>  	int count;
> -	struct vhost_user_connection *conn, *next;
> +	struct vhost_user_connection *conn;
> 
>  	pthread_mutex_lock(&vhost_user.mutex);
> 
> @@ -752,22 +752,17 @@ rte_vhost_driver_unregister(const char *path)
>  			}
> 
>  			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);
> +			TAILQ_FOREACH(conn, &vsocket->conn_list, next) {
>  				close(conn->connfd);
> -				vhost_destroy_device(conn->vid);
> -				TAILQ_REMOVE(&vsocket->conn_list, conn,
> next);
> -				free(conn);
>  			}
>  			pthread_mutex_unlock(&vsocket->conn_mutex);
> 
> +			do {
> +				pthread_mutex_lock(&vsocket-
> >conn_mutex);
> +				conn = TAILQ_FIRST(&vsocket->conn_list);
> +				pthread_mutex_unlock(&vsocket-
> >conn_mutex);
> +			} while (conn != NULL);
> +
>  			pthread_mutex_destroy(&vsocket->conn_mutex);
>  			free(vsocket->path);
>  			free(vsocket);
> --
> 2.14.1
diff mbox

Patch

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 83befdced..46ac88efd 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -735,7 +735,7 @@  rte_vhost_driver_unregister(const char *path)
 {
 	int i;
 	int count;
-	struct vhost_user_connection *conn, *next;
+	struct vhost_user_connection *conn;
 
 	pthread_mutex_lock(&vhost_user.mutex);
 
@@ -752,22 +752,17 @@  rte_vhost_driver_unregister(const char *path)
 			}
 
 			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);
+			TAILQ_FOREACH(conn, &vsocket->conn_list, next) {
 				close(conn->connfd);
-				vhost_destroy_device(conn->vid);
-				TAILQ_REMOVE(&vsocket->conn_list, conn, next);
-				free(conn);
 			}
 			pthread_mutex_unlock(&vsocket->conn_mutex);
 
+			do {
+				pthread_mutex_lock(&vsocket->conn_mutex);
+				conn = TAILQ_FIRST(&vsocket->conn_list);
+				pthread_mutex_unlock(&vsocket->conn_mutex);
+			} while (conn != NULL);
+
 			pthread_mutex_destroy(&vsocket->conn_mutex);
 			free(vsocket->path);
 			free(vsocket);