[Qemu-devel] [PATCH] vhost: Release memory references on cleanup

[Qemu-devel] [PATCH] vhost: Release memory references on cleanup

[Alex Williamson]

vhost registers a MemoryListener where it adds and removes references
to MemoryRegions as the MemoryRegionSections pass through.  The
region_add callback is invoked for each existing section when the
MemoryListener is registered, but unregistering the MemoryListener
performs no reciprocal region_del callback.  It's therefore the
owner of the MemoryListener's responsibility to cleanup any persistent
changes, such as these memory references, after unregistering.

The consequence of this bug is that if we have both a vhost device
and a vfio device, the vhost device will reference any mmap'd MMIO of
the vfio device via this MemoryListener.  If the vhost device is then
removed, those references remain outstanding.  If we then attempt to
remove the vfio device, it never gets finalized and the only way to
release the kernel file descriptors is to terminate the QEMU process.

Fixes: dfde4e6e1a86 ("memory: add ref/unref calls")
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-stable@nongnu.org # v1.6.0+
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/virtio/vhost.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6eddb099b02f..b737ca915b06 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1356,6 +1356,10 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     if (hdev->mem) {
         /* those are only safe after successful init */
         memory_listener_unregister(&hdev->memory_listener);
+        for (i = 0; i < hdev->n_mem_sections; ++i) {
+            MemoryRegionSection *section = &hdev->mem_sections[i];
+            memory_region_unref(section->mr);
+        }
         QLIST_REMOVE(hdev, entry);
     }
     if (hdev->migration_blocker) {

Re: [Qemu-devel] [PATCH] vhost: Release memory references on cleanup

[Paolo Bonzini]

On 07/09/2017 22:27, Alex Williamson wrote:
> vhost registers a MemoryListener where it adds and removes references
> to MemoryRegions as the MemoryRegionSections pass through.  The
> region_add callback is invoked for each existing section when the
> MemoryListener is registered, but unregistering the MemoryListener
> performs no reciprocal region_del callback.  It's therefore the
> owner of the MemoryListener's responsibility to cleanup any persistent
> changes, such as these memory references, after unregistering.
> 
> The consequence of this bug is that if we have both a vhost device
> and a vfio device, the vhost device will reference any mmap'd MMIO of
> the vfio device via this MemoryListener.  If the vhost device is then
> removed, those references remain outstanding.  If we then attempt to
> remove the vfio device, it never gets finalized and the only way to
> release the kernel file descriptors is to terminate the QEMU process.
> 
> Fixes: dfde4e6e1a86 ("memory: add ref/unref calls")
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: qemu-stable@nongnu.org # v1.6.0+
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

This fix is correct.  It's surprising that it was only found now!

Thanks,

Paolo

> ---
>  hw/virtio/vhost.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 6eddb099b02f..b737ca915b06 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1356,6 +1356,10 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
>      if (hdev->mem) {
>          /* those are only safe after successful init */
>          memory_listener_unregister(&hdev->memory_listener);
> +        for (i = 0; i < hdev->n_mem_sections; ++i) {
> +            MemoryRegionSection *section = &hdev->mem_sections[i];
> +            memory_region_unref(section->mr);
> +        }
>          QLIST_REMOVE(hdev, entry);
>      }
>      if (hdev->migration_blocker) {
>