[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup |
Date: |
Mon, 1 Nov 2021 17:00:48 -0400 |
On Mon, Nov 01, 2021 at 04:38:13PM +0800, Xueming Li wrote:
> When vhost-user device cleanup is executed and un-mmaps notifier
> address, VM cpu thread writing the notifier fails by accessing invalid
> address error.
>
> To avoid this concurrent issue, call RCU and wait for a memory flatview
> update, then un-mmap notifiers in callback.
>
> Fixes: 44866521bd6e ("vhost-user: support registering external host
> notifiers")
> Cc: qemu-stable@nongnu.org
> Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> ---
> hw/virtio/vhost-user.c | 50 +++++++++++++++++++++-------------
> include/hw/virtio/vhost-user.h | 2 ++
> 2 files changed, 33 insertions(+), 19 deletions(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index c671719e9b..5adad4d029 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -25,6 +25,7 @@
> #include "migration/migration.h"
> #include "migration/postcopy-ram.h"
> #include "trace.h"
> +#include "exec/ramblock.h"
>
> #include <sys/ioctl.h>
> #include <sys/socket.h>
> @@ -1143,15 +1144,27 @@ static int vhost_user_set_vring_num(struct vhost_dev
> *dev,
> return vhost_set_vring(dev, VHOST_USER_SET_VRING_NUM, ring);
> }
>
> -static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> - int queue_idx)
> +static void vhost_user_host_notifier_free(VhostUserHostNotifier *n)
> {
> - struct vhost_user *u = dev->opaque;
> - VhostUserHostNotifier *n = &u->user->notifier[queue_idx];
> - VirtIODevice *vdev = dev->vdev;
> + assert(n && n->old_addr);
> + munmap(n->old_addr, qemu_real_host_page_size);
> + n->old_addr = NULL;
> +}
> +
> +static void vhost_user_host_notifier_remove(VhostUserState *user,
> + VirtIODevice *vdev, int
> queue_idx)
> +{
> + VhostUserHostNotifier *n = &user->notifier[queue_idx];
>
> if (n->addr) {
> - virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> + if (vdev) {
> + virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr,
> false);
> + }
> + assert(n->addr);
> + assert(!n->old_addr);
> + n->old_addr = n->addr;
> + n->addr = NULL;
> + call_rcu(n, vhost_user_host_notifier_free, rcu);
> }
> }
>
> @@ -1190,8 +1203,9 @@ static int vhost_user_get_vring_base(struct vhost_dev
> *dev,
> .payload.state = *ring,
> .hdr.size = sizeof(msg.payload.state),
> };
> + struct vhost_user *u = dev->opaque;
>
> - vhost_user_host_notifier_remove(dev, ring->index);
> + vhost_user_host_notifier_remove(u->user, dev->vdev, ring->index);
>
> if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> return -1;
> @@ -1486,12 +1500,7 @@ static int
> vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
>
> n = &user->notifier[queue_idx];
>
> - if (n->addr) {
> - virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> - object_unparent(OBJECT(&n->mr));
> - munmap(n->addr, page_size);
> - n->addr = NULL;
> - }
> + vhost_user_host_notifier_remove(user, vdev, queue_idx);
>
> if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
> return 0;
> @@ -1510,9 +1519,12 @@ static int
> vhost_user_slave_handle_vring_host_notifier(struct vhost_dev *dev,
>
> name = g_strdup_printf("vhost-user/host-notifier@%p mmaps[%d]",
> user, queue_idx);
> - if (!n->mr.ram) /* Don't init again after suspend. */
> + if (!n->mr.ram) { /* Don't init again after suspend. */
> memory_region_init_ram_device_ptr(&n->mr, OBJECT(vdev), name,
> page_size, addr);
> + } else {
> + n->mr.ram_block->host = addr;
> + }
> g_free(name);
>
> if (virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, true)) {
> @@ -2460,17 +2472,17 @@ bool vhost_user_init(VhostUserState *user,
> CharBackend *chr, Error **errp)
> void vhost_user_cleanup(VhostUserState *user)
> {
> int i;
> + VhostUserHostNotifier *n;
>
> if (!user->chr) {
> return;
> }
> memory_region_transaction_begin();
> for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> - if (user->notifier[i].addr) {
> - object_unparent(OBJECT(&user->notifier[i].mr));
> - munmap(user->notifier[i].addr, qemu_real_host_page_size);
> - user->notifier[i].addr = NULL;
> - }
> + n = &user->notifier[i];
> + assert(!n->addr);
I'm pretty confused as to why this assert holds.
Add a comment?
> + vhost_user_host_notifier_remove(user, NULL, i);
> + object_unparent(OBJECT(&n->mr));
> }
> memory_region_transaction_commit();
> user->chr = NULL;
I'm also confused on why we can do unparent for notifiers which have
never been set up. Won't n->mr be invalid then?
> diff --git a/include/hw/virtio/vhost-user.h b/include/hw/virtio/vhost-user.h
> index f6012b2078..03aa22d450 100644
> --- a/include/hw/virtio/vhost-user.h
> +++ b/include/hw/virtio/vhost-user.h
> @@ -12,8 +12,10 @@
> #include "hw/virtio/virtio.h"
>
> typedef struct VhostUserHostNotifier {
> + struct rcu_head rcu;
> MemoryRegion mr;
> void *addr;
> + void *old_addr;
That's not a very clear name. Is this literally just
"address for the rcu callback to unmap"?
Maybe unmap_addr then?
> } VhostUserHostNotifier;
>
> typedef struct VhostUserState {
> --
> 2.33.0
[PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup, Xueming Li, 2021/11/01
- Re: [PATCH v6 2/2] vhost-user: fix VirtQ notifier cleanup,
Michael S. Tsirkin <=