virtio-gpu/virgl: free BHs and timers on renderer reset

[PATCH] virtio-gpu/virgl: free BHs and timers on renderer reset

Posted by marcandre.lureau@redhat.com 1 month ago

From: Marc-André Lureau <marcandre.lureau@redhat.com>

virtio_gpu_virgl_reset() does not free the bottom halves or timers
allocated by the previous virtio_gpu_virgl_init() call. A subsequent
virtio_gpu_virgl_init() overwrites the old pointers, leaking the
previous allocations.

Fixes: bd9258917fbf ("virtio-gpu: Destroy virgl resources on virtio-gpu reset")
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/display/virtio-gpu-virgl.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
index 0b79aef8d1f5..8bf6417c6514 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -1425,6 +1425,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
 
 static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
 {
+    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
     struct virtio_gpu_simple_resource *res, *tmp;
 
     /*
@@ -1443,6 +1444,11 @@ static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
 
     virtio_gpu_virgl_reset_async_fences(g);
 
+    g_clear_pointer(&gl->cmdq_resume_bh, qemu_bh_delete);
+    g_clear_pointer(&gl->async_fence_bh, qemu_bh_delete);
+    g_clear_pointer(&gl->print_stats, timer_free);
+    g_clear_pointer(&gl->fence_poll, timer_free);
+
     return true;
 }
 
-- 
2.55.0.543.g5ebe2ebe4ea8

Re: [PATCH] virtio-gpu/virgl: free BHs and timers on renderer reset

Posted by Akihiko Odaki 4 weeks, 1 day ago

On 2026/08/28 0:43, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> virtio_gpu_virgl_reset() does not free the bottom halves or timers
> allocated by the previous virtio_gpu_virgl_init() call. A subsequent
> virtio_gpu_virgl_init() overwrites the old pointers, leaking the
> previous allocations.
> 
> Fixes: bd9258917fbf ("virtio-gpu: Destroy virgl resources on virtio-gpu reset")

This Fixes: tag needs to be corrected. bd9258917fbf^ already reset the
renderer and re-entered virtio_gpu_virgl_init(), overwriting the 
allocations. That behavior began in 7e688d1bf515, so the current tag 
misses older affected releases. Please use:

Fixes: 7e688d1bf515 ("virtio-gpu: Handle virtio_gpu_virgl_init() failure")

With the Fixes tag corrected:

Reviewed-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

Regards,
Akihiko Odaki

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   hw/display/virtio-gpu-virgl.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 0b79aef8d1f5..8bf6417c6514 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -1425,6 +1425,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
>   
>   static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
>   {
> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>       struct virtio_gpu_simple_resource *res, *tmp;
>   
>       /*
> @@ -1443,6 +1444,11 @@ static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
>   
>       virtio_gpu_virgl_reset_async_fences(g);
>   
> +    g_clear_pointer(&gl->cmdq_resume_bh, qemu_bh_delete);
> +    g_clear_pointer(&gl->async_fence_bh, qemu_bh_delete);
> +    g_clear_pointer(&gl->print_stats, timer_free);
> +    g_clear_pointer(&gl->fence_poll, timer_free);
> +
>       return true;
>   }
>

Re: [PATCH] virtio-gpu/virgl: free BHs and timers on renderer reset

Posted by Alex Bennée 1 month ago

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> virtio_gpu_virgl_reset() does not free the bottom halves or timers
> allocated by the previous virtio_gpu_virgl_init() call. A subsequent
> virtio_gpu_virgl_init() overwrites the old pointers, leaking the
> previous allocations.
>
> Fixes: bd9258917fbf ("virtio-gpu: Destroy virgl resources on virtio-gpu reset")
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/display/virtio-gpu-virgl.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> index 0b79aef8d1f5..8bf6417c6514 100644
> --- a/hw/display/virtio-gpu-virgl.c
> +++ b/hw/display/virtio-gpu-virgl.c
> @@ -1425,6 +1425,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
>  
>  static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
>  {
> +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>      struct virtio_gpu_simple_resource *res, *tmp;
>  
>      /*
> @@ -1443,6 +1444,11 @@ static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
>  
>      virtio_gpu_virgl_reset_async_fences(g);
>  
> +    g_clear_pointer(&gl->cmdq_resume_bh, qemu_bh_delete);
> +    g_clear_pointer(&gl->async_fence_bh, qemu_bh_delete);
> +    g_clear_pointer(&gl->print_stats, timer_free);
> +    g_clear_pointer(&gl->fence_poll, timer_free);
> +

Don't we need some ptr checks in virtio_gpu_gl_device_unrealize so a
reset followed by an unplug doesn't causes segs chasing now NULL
pointers?


>      return true;
>  }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] virtio-gpu/virgl: free BHs and timers on renderer reset

Posted by Marc-André Lureau 1 month ago

Hi

On Thu, Aug 27, 2026 at 8:11 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> marcandre.lureau@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > virtio_gpu_virgl_reset() does not free the bottom halves or timers
> > allocated by the previous virtio_gpu_virgl_init() call. A subsequent
> > virtio_gpu_virgl_init() overwrites the old pointers, leaking the
> > previous allocations.
> >
> > Fixes: bd9258917fbf ("virtio-gpu: Destroy virgl resources on virtio-gpu reset")
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/display/virtio-gpu-virgl.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
> > index 0b79aef8d1f5..8bf6417c6514 100644
> > --- a/hw/display/virtio-gpu-virgl.c
> > +++ b/hw/display/virtio-gpu-virgl.c
> > @@ -1425,6 +1425,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
> >
> >  static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
> >  {
> > +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
> >      struct virtio_gpu_simple_resource *res, *tmp;
> >
> >      /*
> > @@ -1443,6 +1444,11 @@ static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
> >
> >      virtio_gpu_virgl_reset_async_fences(g);
> >
> > +    g_clear_pointer(&gl->cmdq_resume_bh, qemu_bh_delete);
> > +    g_clear_pointer(&gl->async_fence_bh, qemu_bh_delete);
> > +    g_clear_pointer(&gl->print_stats, timer_free);
> > +    g_clear_pointer(&gl->fence_poll, timer_free);
> > +
>
> Don't we need some ptr checks in virtio_gpu_gl_device_unrealize so a
> reset followed by an unplug doesn't causes segs chasing now NULL
> pointers?

Good point, actually we should be safe, since render_state < RS_INITED.

But those qemu_bh_delete().. can we make it safe to call with NULL?
timer_free() was fixed already.

>
>
> >      return true;
> >  }
>
> --
> Alex Bennée
> Virtualisation Tech Lead @ Linaro
>

Re: [PATCH] virtio-gpu/virgl: free BHs and timers on renderer reset

Posted by Alex Bennée 1 month ago

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Thu, Aug 27, 2026 at 8:11 PM Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > virtio_gpu_virgl_reset() does not free the bottom halves or timers
>> > allocated by the previous virtio_gpu_virgl_init() call. A subsequent
>> > virtio_gpu_virgl_init() overwrites the old pointers, leaking the
>> > previous allocations.
>> >
>> > Fixes: bd9258917fbf ("virtio-gpu: Destroy virgl resources on virtio-gpu reset")
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  hw/display/virtio-gpu-virgl.c | 6 ++++++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c
>> > index 0b79aef8d1f5..8bf6417c6514 100644
>> > --- a/hw/display/virtio-gpu-virgl.c
>> > +++ b/hw/display/virtio-gpu-virgl.c
>> > @@ -1425,6 +1425,7 @@ void virtio_gpu_virgl_reset_scanout(VirtIOGPU *g)
>> >
>> >  static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
>> >  {
>> > +    VirtIOGPUGL *gl = VIRTIO_GPU_GL(g);
>> >      struct virtio_gpu_simple_resource *res, *tmp;
>> >
>> >      /*
>> > @@ -1443,6 +1444,11 @@ static bool virtio_gpu_virgl_reset(VirtIOGPU *g)
>> >
>> >      virtio_gpu_virgl_reset_async_fences(g);
>> >
>> > +    g_clear_pointer(&gl->cmdq_resume_bh, qemu_bh_delete);
>> > +    g_clear_pointer(&gl->async_fence_bh, qemu_bh_delete);
>> > +    g_clear_pointer(&gl->print_stats, timer_free);
>> > +    g_clear_pointer(&gl->fence_poll, timer_free);
>> > +
>>
>> Don't we need some ptr checks in virtio_gpu_gl_device_unrealize so a
>> reset followed by an unplug doesn't causes segs chasing now NULL
>> pointers?
>
> Good point, actually we should be safe, since render_state < RS_INITED.
>
> But those qemu_bh_delete().. can we make it safe to call with NULL?

That seems sane as it will be on the exit path. I wouldn't tolerate
NULL for any of the other qemu_bh functions though.

> timer_free() was fixed already.
>
>>
>>
>> >      return true;
>> >  }
>>
>> --
>> Alex Bennée
>> Virtualisation Tech Lead @ Linaro
>>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro