Alex Bennée <[email protected]> writes:
> "Kim, Dongwon" <[email protected]> writes:
>
>> Hi Marc-André,
>>
>>> -----Original Message-----
>>> From: Marc-André Lureau <[email protected]>
>>> Sent: Tuesday, March 3, 2026 8:28 AM
>>> To: Kim, Dongwon <[email protected]>
>>> Cc: [email protected]
>>> Subject: Re: [PATCH] virtio-gpu: Fix scanout dmabuf cleanup during resource
>>> destruction
>>>
>>> Hi
>>>
>>> On Tue, Mar 3, 2026 at 2:06 AM <[email protected]> wrote:
>>> >
>>> > From: Dongwon Kim <[email protected]>
>>> >
>>> > 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 <[email protected]>
>>> > Cc: Marc-André Lureau <[email protected]>
>>> > Signed-off-by: Vivek Kasireddy <[email protected]>
>>> > Signed-off-by: Dongwon Kim <[email protected]>
>>>
>>> Acked-by: Marc-André Lureau <[email protected]>
>>>
>>> > ---
>>> > 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.
I also noted you need to fix the stub:
void virtio_gpu_fini_udmabuf(VirtIOGPU *g, struct virtio_gpu_simple_resource
*res)
{
/* nothing (stub) */
}
>>
>>>
>>> > + 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
