Re: [PATCH v2] virtio-gpu: Fix scanout dmabuf cleanup during resource destruction

["Alex Bennée" <alex.bennee@linaro.org>]

dongwon.kim@intel.com writes:

> From: Dongwon Kim <dongwon.kim@intel.com>
>
> When a virtio-gpu resource is destroyed, any associated udmabuf must be
> properly torn down. Currently, the code may leave dangling references
> to dmabuf file descriptors in the scanout primary buffers.
>
> This patch updates virtio_gpu_fini_udmabuf to:
> 1. Iterate through all active scanouts.
> 2. Identify dmabufs that match the resource's file descriptor.
> 3. Close the dmabuf and invalidate the resource's FD reference to
>    prevent use-after-free or double-close scenarios.
> 4. Finally, trigger the underlying udmabuf destruction.
>
> This ensures that the display backend does not attempt to access
> memory or FDs that have been released by the guest or the host.

Queued to virtio-gpu/next, thanks.

>
> v2: - Corrected virtio_gpu_fini_udmabuf in stub
>       (Alex Bennée)
>
>     - Make sure that qemu dmabuf has at least one plane before
>       Comparing fds
>       (Marc-André Lureau)

FYI usually we put version information under the --- so it doesn't
pollute the commit message when things are applied.

>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  include/hw/virtio/virtio-gpu.h        |  3 ++-
>  hw/display/virtio-gpu-udmabuf-stubs.c |  2 +-
>  hw/display/virtio-gpu-udmabuf.c       | 27 ++++++++++++++++++++-------
>  hw/display/virtio-gpu.c               |  2 +-
>  4 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
> index 58e0f91fda..65312f869d 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -357,7 +357,8 @@ bool virtio_gpu_scanout_blob_to_fb(struct virtio_gpu_framebuffer *fb,
>  /* virtio-gpu-udmabuf.c */
>  bool virtio_gpu_have_udmabuf(void);
>  void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res);
> -void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource *res);
> +void virtio_gpu_fini_udmabuf(VirtIOGPU *g,
> +                             struct virtio_gpu_simple_resource *res);
>  int virtio_gpu_update_dmabuf(VirtIOGPU *g,
>                               uint32_t scanout_id,
>                               struct virtio_gpu_simple_resource *res,
> diff --git a/hw/display/virtio-gpu-udmabuf-stubs.c b/hw/display/virtio-gpu-udmabuf-stubs.c
> index f692e13510..85d03935a3 100644
> --- a/hw/display/virtio-gpu-udmabuf-stubs.c
> +++ b/hw/display/virtio-gpu-udmabuf-stubs.c
> @@ -12,7 +12,7 @@ void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
>      /* nothing (stub) */
>  }
>  
> -void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource *res)
> +void virtio_gpu_fini_udmabuf(VirtIOGPU *g, struct virtio_gpu_simple_resource *res)
>  {
>      /* nothing (stub) */
>  }
> diff --git a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
> index d804f321aa..74b6a7766a 100644
> --- a/hw/display/virtio-gpu-udmabuf.c
> +++ b/hw/display/virtio-gpu-udmabuf.c
> @@ -151,13 +151,6 @@ void virtio_gpu_init_udmabuf(struct virtio_gpu_simple_resource *res)
>      res->blob = pdata;
>  }
>  
> -void virtio_gpu_fini_udmabuf(struct virtio_gpu_simple_resource *res)
> -{
> -    if (res->remapped) {
> -        virtio_gpu_destroy_udmabuf(res);
> -    }
> -}
> -
>  static void virtio_gpu_free_dmabuf(VirtIOGPU *g, VGPUDMABuf *dmabuf)
>  {
>      struct virtio_gpu_scanout *scanout;
> @@ -169,6 +162,26 @@ static void virtio_gpu_free_dmabuf(VirtIOGPU *g, VGPUDMABuf *dmabuf)
>      g_free(dmabuf);
>  }
>  
> +void virtio_gpu_fini_udmabuf(VirtIOGPU *g, struct virtio_gpu_simple_resource *res)
> +{
> +    int max_outputs = g->parent_obj.conf.max_outputs;
> +    int i;
> +
> +    for (i = 0; i < max_outputs; i++) {
> +        VGPUDMABuf *dmabuf = g->dmabuf.primary[i];
> +
> +        if (dmabuf &&
> +            qemu_dmabuf_get_num_planes(dmabuf->buf) > 0 &&
> +            qemu_dmabuf_get_fds(dmabuf->buf, NULL)[0] == res->dmabuf_fd &&
> +            res->dmabuf_fd != -1) {
> +            qemu_dmabuf_close(dmabuf->buf);
> +            res->dmabuf_fd = -1;
> +        }
> +    }
> +
> +    virtio_gpu_destroy_udmabuf(res);
> +}
> +
>  static VGPUDMABuf
>  *virtio_gpu_create_dmabuf(VirtIOGPU *g,
>                            uint32_t scanout_id,
> diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
> index 643e91ca2a..b2af861f0d 100644
> --- a/hw/display/virtio-gpu.c
> +++ b/hw/display/virtio-gpu.c
> @@ -902,7 +902,7 @@ void virtio_gpu_cleanup_mapping(VirtIOGPU *g,
>      res->addrs = NULL;
>  
>      if (res->blob) {
> -        virtio_gpu_fini_udmabuf(res);
> +        virtio_gpu_fini_udmabuf(g, res);
>      }
>  }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro