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 <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: address@hidden # v1.6.0+
> Signed-off-by: Alex Williamson <address@hidden>

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) {
>