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

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

"Kim, Dongwon" <dongwon.kim@intel.com> writes:

> Hi Marc-André,
>
>> -----Original Message-----
>> From: Marc-André Lureau <marcandre.lureau@gmail.com>
>> Sent: Tuesday, March 3, 2026 8:28 AM
>> To: Kim, Dongwon <dongwon.kim@intel.com>
>> Cc: qemu-devel@nongnu.org
>> Subject: Re: [PATCH] virtio-gpu: Fix scanout dmabuf cleanup during resource
>> destruction
>> 
>> Hi
>> 
>> On Tue, Mar 3, 2026 at 2:06 AM <dongwon.kim@intel.com> wrote:
>> >
>> > 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.
>> >
>> > Cc: Gerd Hoffmann <kraxel@redhat.com>
>> > Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>> > Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
>> 
>> Acked-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> 
>> > ---
>> >  include/hw/virtio/virtio-gpu.h  |  3 ++-
>> > hw/display/virtio-gpu-udmabuf.c | 25 ++++++++++++++++++-------
>> >  hw/display/virtio-gpu.c         |  2 +-
>> >  3 files changed, 21 insertions(+), 9 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.c
>> > b/hw/display/virtio-gpu-udmabuf.c index d804f321aa..bd5b44f5fb 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,24 @@ 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 && (res->dmabuf_fd != -1) &&
>> 
>> Maybe add qemu_dmabuf_get_numplanes() > 0 ?
>
> Do you want me to add this condition and resubmit v2 of this patch? I saw
> this patch has already been in the queue.

If you send v2 I can swap it out.
>
>> 
>> > +            qemu_dmabuf_get_fds(dmabuf->buf, NULL)[0] == res->dmabuf_fd) {
>> > +            qemu_dmabuf_close(dmabuf->buf);
>> > +            res->dmabuf_fd = -1;
>> 
>> I am not really happy about that we close the underlying fd here before the
>> next destroy, but I don't have an immediate solution
>
> Yeah, I just thought this would be the best for now.
>
>> 
>> > +        }
>> > +    }
>> > +
>> > +    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);
>> >      }
>> >  }
>> >
>> > --
>> > 2.43.0
>> >
>> >
>> 
>> 
>> --
>> Marc-André Lureau
>
> Thanks,

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro