1. Patchew
  2. QEMU
  3. [PATCH v2 0/2] Improve vhost-user VQ notifier unmap
  4. View patch

[PATCH v2 1/2] vhost-user: fix VirtQ notifier cleanup

Xueming Li posted 2 patches 4 years, 4 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>
There is a newer version of this series
[PATCH v2 1/2] vhost-user: fix VirtQ notifier cleanup
Posted by Xueming Li 4 years, 4 months ago 
When vhost-user device stop and unmmap notifier address, vCPU thread
that writing the notifier via old flatview failed with accessing invalid
address.

To avoid this concurrent issue, wait memory flatview update by draining
rcu callbacks before unmaping notifiers.

Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
Cc: tiwei.bie@intel.com
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 | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 2c8556237f..08581e6711 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1165,6 +1165,11 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
 
     if (n->addr && n->set) {
         virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
+        if (!qemu_in_vcpu_thread())
+            /* Wait vCPU threads accessing notifier via old flatview. */
+            drain_call_rcu();
+        munmap(n->addr, qemu_real_host_page_size);
+        n->addr = NULL;
         n->set = false;
     }
 }
@@ -1502,12 +1507,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(dev, queue_idx);
 
     if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
         return 0;
@@ -2484,11 +2484,17 @@ void vhost_user_cleanup(VhostUserState *user)
     for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
         if (user->notifier[i].addr) {
             object_unparent(OBJECT(&user->notifier[i].mr));
+        }
+    }
+    memory_region_transaction_commit();
+    /* Wait VM threads accessing old flatview which contains notifier. */
+    drain_call_rcu();
+    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        if (user->notifier[i].addr) {
             munmap(user->notifier[i].addr, qemu_real_host_page_size);
             user->notifier[i].addr = NULL;
         }
     }
-    memory_region_transaction_commit();
     user->chr = NULL;
 }
 
-- 
2.33.0
Re: [PATCH v2 1/2] vhost-user: fix VirtQ notifier cleanup
Posted by Michael S. Tsirkin 4 years, 4 months ago 
On Fri, Sep 17, 2021 at 08:26:15PM +0800, Xueming Li wrote:
> When vhost-user device stop and unmmap notifier address, vCPU thread
> that writing the notifier via old flatview failed with accessing invalid
> address.
> 
> To avoid this concurrent issue, wait memory flatview update by draining
> rcu callbacks before unmaping notifiers.
> 
> Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> Cc: tiwei.bie@intel.com
> Cc: qemu-stable@nongnu.org
> Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> Signed-off-by: Xueming Li <xuemingl@nvidia.com>


Pls post v2 as a new thread, with changelog in the cover letter.

> ---
>  hw/virtio/vhost-user.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 2c8556237f..08581e6711 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1165,6 +1165,11 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
>  
>      if (n->addr && n->set) {
>          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> +        if (!qemu_in_vcpu_thread())
> +            /* Wait vCPU threads accessing notifier via old flatview. */

Wait VM - Wait for VM

> +            drain_call_rcu();

okay.
but this has a coding style violation:
should use {} in if.


> +        munmap(n->addr, qemu_real_host_page_size);
> +        n->addr = NULL;
>          n->set = false;
>      }
>  }
> @@ -1502,12 +1507,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(dev, queue_idx);
>  
>      if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
>          return 0;
> @@ -2484,11 +2484,17 @@ void vhost_user_cleanup(VhostUserState *user)
>      for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
>          if (user->notifier[i].addr) {
>              object_unparent(OBJECT(&user->notifier[i].mr));
> +        }
> +    }
> +    memory_region_transaction_commit();
> +    /* Wait VM threads accessing old flatview which contains notifier. */

Wait VM - Wait for VM

> +    drain_call_rcu();
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        if (user->notifier[i].addr) {
>              munmap(user->notifier[i].addr, qemu_real_host_page_size);
>              user->notifier[i].addr = NULL;
>          }
>      }
> -    memory_region_transaction_commit();
>      user->chr = NULL;
>  }
>  
> -- 
> 2.33.0
Re: [PATCH v2 1/2] vhost-user: fix VirtQ notifier cleanup
Posted by Xueming(Steven) Li 4 years, 4 months ago 
On Tue, 2021-10-05 at 10:40 -0400, Michael S. Tsirkin wrote:
> On Fri, Sep 17, 2021 at 08:26:15PM +0800, Xueming Li wrote:
> > When vhost-user device stop and unmmap notifier address, vCPU thread
> > that writing the notifier via old flatview failed with accessing invalid
> > address.
> > 
> > To avoid this concurrent issue, wait memory flatview update by draining
> > rcu callbacks before unmaping notifiers.
> > 
> > Fixes: 44866521bd6e ("vhost-user: support registering external host notifiers")
> > Cc: tiwei.bie@intel.com
> > Cc: qemu-stable@nongnu.org
> > Cc: Yuwei Zhang <zhangyuwei.9149@bytedance.com>
> > Signed-off-by: Xueming Li <xuemingl@nvidia.com>
> 
> 
> Pls post v2 as a new thread, with changelog in the cover letter.

Thanks, v3 posted with below coding style and comment fixes.

> 
> > ---
> >  hw/virtio/vhost-user.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 2c8556237f..08581e6711 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -1165,6 +1165,11 @@ static void vhost_user_host_notifier_remove(struct vhost_dev *dev,
> >  
> >      if (n->addr && n->set) {
> >          virtio_queue_set_host_notifier_mr(vdev, queue_idx, &n->mr, false);
> > +        if (!qemu_in_vcpu_thread())
> > +            /* Wait vCPU threads accessing notifier via old flatview. */
> 
> Wait VM - Wait for VM
> 
> > +            drain_call_rcu();
> 
> okay.
> but this has a coding style violation:
> should use {} in if.
> 
> 
> > +        munmap(n->addr, qemu_real_host_page_size);
> > +        n->addr = NULL;
> >          n->set = false;
> >      }
> >  }
> > @@ -1502,12 +1507,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(dev, queue_idx);
> >  
> >      if (area->u64 & VHOST_USER_VRING_NOFD_MASK) {
> >          return 0;
> > @@ -2484,11 +2484,17 @@ void vhost_user_cleanup(VhostUserState *user)
> >      for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> >          if (user->notifier[i].addr) {
> >              object_unparent(OBJECT(&user->notifier[i].mr));
> > +        }
> > +    }
> > +    memory_region_transaction_commit();
> > +    /* Wait VM threads accessing old flatview which contains notifier. */
> 
> Wait VM - Wait for VM
> 
> > +    drain_call_rcu();
> > +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> > +        if (user->notifier[i].addr) {
> >              munmap(user->notifier[i].addr, qemu_real_host_page_size);
> >              user->notifier[i].addr = NULL;
> >          }
> >      }
> > -    memory_region_transaction_commit();
> >      user->chr = NULL;
> >  }
> >  
> > -- 
> > 2.33.0
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.