[PATCH] vhost: fix crash caused by accessing a freed vsocket
Maxime Coquelin
maxime.coquelin at redhat.com
Wed Apr 3 11:39:58 CEST 2024
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 at 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 at dpdk.org
>
> Signed-off-by: Gongming Chen <chengm11 at 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
More information about the stable
mailing list