Re: [PATCH] vhost: fix crash caused by accessing a freed vsocket

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Gongming,

It's the 9th time the patch has been sent.
I'm not sure whether there are changes between them or these are just
re-sends, but that's something to avoid.

If there are differences, you should use versionning to highlight it.
If unsure, please check the contributions guidelines first.

Regarding the patch itself, I don't know if this is avoidable, but I
would prefer we do not introduce yet another lock in there.

Thanks,
Maxime

On 4/3/24 08:31, Gongming Chen wrote:
> From: Gongming Chen <chengm11@chinatelecom.cn>
> 
> When a vhost user message handling error in the event dispatch thread,
> vsocket reconn is added to the reconnection list of the reconnection
> thread.
> Since the reconnection, event dispatching and app configuration thread
> do not have common thread protection restrictions, the app config
> thread freed vsocket in the rte_vhost_driver_unregister process,
> but vsocket reconn can still exist in the reconn_list through this
> mechanism.
> Then in the reconnection thread, the vsocket is connected again and
> conn is added to the dispatch thread.
> Finally, the vsocket is accessed again in the event dispatch thread,
> resulting in a use-after-free error.
> 
> This patch adds a vhost threads read-write lock to restrict
> reconnection, event dispatching and app configuration threads.
> When the vhost driver unregisters, it exclusively holds the lock to
> safely free the vsocket.
> 
> #0  0x0000000000000025 in ?? ()
> #1  0x0000000003ed7ca0 in vhost_user_read_cb at lib/vhost/socket.c:330
> #2  0x0000000003ed625f in fdset_event_dispatch at lib/vhost/fd_man.c:283
> 
> Fixes: e623e0c6d8a5 ("vhost: add vhost-user client mode")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Gongming Chen <chengm11@chinatelecom.cn>
> ---
>   lib/vhost/fd_man.c       |  3 +++
>   lib/vhost/meson.build    |  1 +
>   lib/vhost/socket.c       | 30 ++++++++++++------------------
>   lib/vhost/vhost_thread.c | 37 +++++++++++++++++++++++++++++++++++++
>   lib/vhost/vhost_thread.h | 16 ++++++++++++++++
>   5 files changed, 69 insertions(+), 18 deletions(-)
>   create mode 100644 lib/vhost/vhost_thread.c
>   create mode 100644 lib/vhost/vhost_thread.h
> 
> diff --git a/lib/vhost/fd_man.c b/lib/vhost/fd_man.c
> index 481e6b900a..b0e0aa2640 100644
> --- a/lib/vhost/fd_man.c
> +++ b/lib/vhost/fd_man.c
> @@ -9,6 +9,7 @@
>   #include <rte_log.h>
>   
>   #include "fd_man.h"
> +#include "vhost_thread.h"
>   
>   RTE_LOG_REGISTER_SUFFIX(vhost_fdset_logtype, fdset, INFO);
>   #define RTE_LOGTYPE_VHOST_FDMAN vhost_fdset_logtype
> @@ -250,6 +251,7 @@ fdset_event_dispatch(void *arg)
>   		if (val < 0)
>   			continue;
>   
> +		vhost_thread_read_lock();
>   		need_shrink = 0;
>   		for (i = 0; i < numfds; i++) {
>   			pthread_mutex_lock(&pfdset->fd_mutex);
> @@ -303,6 +305,7 @@ fdset_event_dispatch(void *arg)
>   
>   		if (need_shrink)
>   			fdset_shrink(pfdset);
> +		vhost_thread_read_unlock();
>   	}
>   
>   	return 0;
> diff --git a/lib/vhost/meson.build b/lib/vhost/meson.build
> index 41b622a9be..7bc1840ed0 100644
> --- a/lib/vhost/meson.build
> +++ b/lib/vhost/meson.build
> @@ -25,6 +25,7 @@ sources = files(
>           'vdpa.c',
>           'vhost.c',
>           'vhost_crypto.c',
> +        'vhost_thread.c',
>           'vhost_user.c',
>           'virtio_net.c',
>           'virtio_net_ctrl.c',
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index 96b3ab5595..e05d36f549 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -20,6 +20,7 @@
>   #include "fd_man.h"
>   #include "vduse.h"
>   #include "vhost.h"
> +#include "vhost_thread.h"
>   #include "vhost_user.h"
>   
>   
> @@ -463,6 +464,7 @@ vhost_user_client_reconnect(void *arg __rte_unused)
>   	struct vhost_user_reconnect *reconn, *next;
>   
>   	while (1) {
> +		vhost_thread_read_lock();
>   		pthread_mutex_lock(&reconn_list.mutex);
>   
>   		/*
> @@ -494,6 +496,7 @@ vhost_user_client_reconnect(void *arg __rte_unused)
>   		}
>   
>   		pthread_mutex_unlock(&reconn_list.mutex);
> +		vhost_thread_read_unlock();
>   		sleep(1);
>   	}
>   
> @@ -1071,7 +1074,7 @@ rte_vhost_driver_unregister(const char *path)
>   	if (path == NULL)
>   		return -1;
>   
> -again:
> +	vhost_thread_write_lock();
>   	pthread_mutex_lock(&vhost_user.mutex);
>   
>   	for (i = 0; i < vhost_user.vsocket_cnt; i++) {
> @@ -1083,14 +1086,10 @@ rte_vhost_driver_unregister(const char *path)
>   			vduse_device_destroy(path);
>   		} else if (vsocket->is_server) {
>   			/*
> -			 * If r/wcb is executing, release vhost_user's
> -			 * mutex lock, and try again since the r/wcb
> -			 * may use the mutex lock.
> +			 * The vhost thread write lock has been acquired,
> +			 * and fd must be deleted from fdset.
>   			 */
> -			if (fdset_try_del(&vhost_user.fdset, vsocket->socket_fd) == -1) {
> -				pthread_mutex_unlock(&vhost_user.mutex);
> -				goto again;
> -			}
> +			fdset_del(&vhost_user.fdset, vsocket->socket_fd);
>   		} else if (vsocket->reconnect) {
>   			vhost_user_remove_reconnect(vsocket);
>   		}
> @@ -1102,17 +1101,10 @@ rte_vhost_driver_unregister(const char *path)
>   			next = TAILQ_NEXT(conn, next);
>   
>   			/*
> -			 * If r/wcb is executing, release vsocket's
> -			 * conn_mutex and vhost_user's mutex locks, and
> -			 * try again since the r/wcb may use the
> -			 * conn_mutex and mutex locks.
> +			 * The vhost thread write lock has been acquired,
> +			 * and fd must be deleted from fdset.
>   			 */
> -			if (fdset_try_del(&vhost_user.fdset,
> -					  conn->connfd) == -1) {
> -				pthread_mutex_unlock(&vsocket->conn_mutex);
> -				pthread_mutex_unlock(&vhost_user.mutex);
> -				goto again;
> -			}
> +			fdset_del(&vhost_user.fdset, conn->connfd);
>   
>   			VHOST_CONFIG_LOG(path, INFO, "free connfd %d", conn->connfd);
>   			close(conn->connfd);
> @@ -1134,9 +1126,11 @@ rte_vhost_driver_unregister(const char *path)
>   		vhost_user.vsockets[i] = vhost_user.vsockets[count];
>   		vhost_user.vsockets[count] = NULL;
>   		pthread_mutex_unlock(&vhost_user.mutex);
> +		vhost_thread_write_unlock();
>   		return 0;
>   	}
>   	pthread_mutex_unlock(&vhost_user.mutex);
> +	vhost_thread_write_unlock();
>   
>   	return -1;
>   }
> diff --git a/lib/vhost/vhost_thread.c b/lib/vhost/vhost_thread.c
> new file mode 100644
> index 0000000000..6b5dc22042
> --- /dev/null
> +++ b/lib/vhost/vhost_thread.c
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2024 China Telecom Cloud Technology Co., Ltd
> + */
> +
> +#include <rte_rwlock.h>
> +
> +#include "vhost_thread.h"
> +
> +static rte_rwlock_t vhost_thread_lock = RTE_RWLOCK_INITIALIZER;
> +
> +void
> +vhost_thread_read_lock(void)
> +	__rte_no_thread_safety_analysis
> +{
> +	rte_rwlock_read_lock(&vhost_thread_lock);
> +}
> +
> +void
> +vhost_thread_read_unlock(void)
> +	__rte_no_thread_safety_analysis
> +{
> +	rte_rwlock_read_unlock(&vhost_thread_lock);
> +}
> +
> +void
> +vhost_thread_write_lock(void)
> +	__rte_no_thread_safety_analysis
> +{
> +	rte_rwlock_write_lock(&vhost_thread_lock);
> +}
> +
> +void
> +vhost_thread_write_unlock(void)
> +	__rte_no_thread_safety_analysis
> +{
> +	rte_rwlock_write_unlock(&vhost_thread_lock);
> +}
> diff --git a/lib/vhost/vhost_thread.h b/lib/vhost/vhost_thread.h
> new file mode 100644
> index 0000000000..61679172af
> --- /dev/null
> +++ b/lib/vhost/vhost_thread.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright (c) 2024 China Telecom Cloud Technology Co., Ltd
> + */
> +
> +#ifndef _VHOST_THREAD_H_
> +#define _VHOST_THREAD_H_
> +
> +void vhost_thread_read_lock(void);
> +
> +void vhost_thread_read_unlock(void);
> +
> +void vhost_thread_write_lock(void);
> +
> +void vhost_thread_write_unlock(void);
> +
> +#endif