[PATCH] drm/vmwgfx: uninstall the IRQ before freeing the command buffer manager on unload

[PATCH] drm/vmwgfx: uninstall the IRQ before freeing the command buffer manager on unload

[Fan Wu]

vmw_driver_unload() frees the command buffer manager before uninstalling
the device IRQ. vmw_release_device_late() -> vmw_cmdbuf_man_destroy()
frees the manager, while the threaded handler vmw_thread_fn(), released
only later by vmw_irq_uninstall() -> free_irq(), dereferences
dev_priv->cman without a NULL guard and is the sole producer of
schedule_work(&man->work), whose worker vmw_cmdbuf_work_func() recovers
the manager via container_of(). dev_priv->cman is never NULLed on the
unload path, so a handler woken in the window between kfree(man) and
free_irq() runs against freed memory and can re-arm man->work after the
manager's cancel_work_sync() has already returned.

Reorder the unload path to drain pending fences, then uninstall the IRQ,
and only then free the manager: free_irq() guarantees the threaded
handler has exited before kfree(man). vmw_fence_fifo_down() is called
explicitly before the IRQ uninstall so its dma_fence waits are signalled
by the still-live threaded handler (vmw_fences_update() in
vmw_thread_fn()); uninstalling the IRQ first could force pending fence
waits to time out at VMW_FENCE_WAIT_TIMEOUT. vmw_release_device_late()
is left unchanged because it is shared with the hibernation path; its
existing call to vmw_fence_fifo_down() runs against an empty fence list
after the explicit drain above.

Fixes: ef369904aaf7 ("drm/vmwgfx: Move irq bottom half processing to threads")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.5
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 599052d07ae88af2f5775f968fe1e7046a3efe4e..dbe02fe8d8571587733d6f002436bf18dac1887a 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1181,10 +1181,11 @@
 	vmw_devcaps_destroy(dev_priv);
 	vmw_vram_manager_fini(dev_priv);
 	ttm_device_fini(&dev_priv->bdev);
-	vmw_release_device_late(dev_priv);
-	vmw_fence_manager_takedown(dev_priv->fman);
+	vmw_fence_fifo_down(dev_priv->fman);
 	if (dev_priv->capabilities & SVGA_CAP_IRQMASK)
 		vmw_irq_uninstall(&dev_priv->drm);
+	vmw_release_device_late(dev_priv);
+	vmw_fence_manager_takedown(dev_priv->fman);

 	ttm_object_device_release(&dev_priv->tdev);

--
2.39.5

Re: [PATCH] drm/vmwgfx: uninstall the IRQ before freeing the command buffer manager on unload

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free of `cman` and `fman` in hibernation and module load error paths.
- [High] List corruption in `vmw_fence_fifo_down` due to missing lock.
--

--- Patch [1]: [PATCH] drm/vmwgfx: uninstall the IRQ before freeing the command buffer manager on unload ---
commit b967a1f434a9288823516c01c5ade490f5c844f2
Author: Fan Wu <fanwu01@zju.edu.cn>

drm/vmwgfx: uninstall the IRQ before freeing the command buffer manager on unload

This commit reorders the unload path to drain pending fences, uninstall
the IRQ, and then free the command buffer manager. This prevents a
use-after-free where the threaded handler dereferences the manager after
it has been freed.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 0f101aedb49a0..a35b44dd3aa46 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1181,10 +1181,11 @@ static void vmw_driver_unload(struct drm_device *dev)
>  	vmw_devcaps_destroy(dev_priv);
>  	vmw_vram_manager_fini(dev_priv);
>  	ttm_device_fini(&dev_priv->bdev);
> -	vmw_release_device_late(dev_priv);
> -	vmw_fence_manager_takedown(dev_priv->fman);
> +	vmw_fence_fifo_down(dev_priv->fman);

[Severity: High]
This isn't a bug introduced by this patch, but since vmw_fence_fifo_down()
is now called while the IRQ is still active, it exposes a potential list
corruption.

In drivers/gpu/drm/vmwgfx/vmwgfx_fence.c:vmw_fence_fifo_down():

	spin_lock(&fman->lock);
	fman->fifo_down = true;
	while (!list_empty(&fman->fence_list)) {
		...
		spin_unlock(&fman->lock);

		ret = vmw_fence_obj_wait(fence, false, false,
					 VMW_FENCE_WAIT_TIMEOUT);

		if (unlikely(ret != 0)) {
			list_del_init(&fence->head);
			dma_fence_signal(&fence->base);
		}

If the wait times out, list_del_init() modifies the list without holding
fman->lock. Since vmw_fences_update() in the active IRQ thread could be
modifying or traversing the same fman->fence_list concurrently, could this
cause list corruption?

>  	if (dev_priv->capabilities & SVGA_CAP_IRQMASK)
>  		vmw_irq_uninstall(&dev_priv->drm);
> +	vmw_release_device_late(dev_priv);
> +	vmw_fence_manager_takedown(dev_priv->fman);

[Severity: High]
This is a pre-existing issue, but while this patch fixes the unload path,
does the same use-after-free still exist in the hibernation and module load
error paths?

In drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:vmw_pm_freeze(),
vmw_release_device_late() frees dev_priv->cman while the device IRQ is
still active. If an interrupt triggers vmw_thread_fn(), it will
dereference the freed cman.

Similarly, in vmw_driver_load(), the error path out_no_bdev calls
vmw_fence_manager_takedown() to free dev_priv->fman before
vmw_irq_uninstall() is called under out_no_fman. This leaves a window
where vmw_thread_fn() could access the freed fman if an IRQ fires.

>  
>  	ttm_object_device_release(&dev_priv->tdev);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714094511.2960308-1-fanwu01@zju.edu.cn?part=1

Re: [PATCH] drm/vmwgfx: uninstall the IRQ before freeing the command buffer manager on unload

[Maaz Mombasawala]

On 7/14/26 2:45 AM, Fan Wu wrote:
> vmw_driver_unload() frees the command buffer manager before uninstalling
> the device IRQ. vmw_release_device_late() -> vmw_cmdbuf_man_destroy()
> frees the manager, while the threaded handler vmw_thread_fn(), released
> only later by vmw_irq_uninstall() -> free_irq(), dereferences
> dev_priv->cman without a NULL guard and is the sole producer of
> schedule_work(&man->work), whose worker vmw_cmdbuf_work_func() recovers
> the manager via container_of(). dev_priv->cman is never NULLed on the
> unload path, so a handler woken in the window between kfree(man) and
> free_irq() runs against freed memory and can re-arm man->work after the
> manager's cancel_work_sync() has already returned.
> 
> Reorder the unload path to drain pending fences, then uninstall the IRQ,
> and only then free the manager: free_irq() guarantees the threaded
> handler has exited before kfree(man). vmw_fence_fifo_down() is called
> explicitly before the IRQ uninstall so its dma_fence waits are signalled
> by the still-live threaded handler (vmw_fences_update() in
> vmw_thread_fn()); uninstalling the IRQ first could force pending fence
> waits to time out at VMW_FENCE_WAIT_TIMEOUT. vmw_release_device_late()
> is left unchanged because it is shared with the hibernation path; its
> existing call to vmw_fence_fifo_down() runs against an empty fence list
> after the explicit drain above.
> 
> Fixes: ef369904aaf7 ("drm/vmwgfx: Move irq bottom half processing to threads")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.5
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index 599052d07ae88af2f5775f968fe1e7046a3efe4e..dbe02fe8d8571587733d6f002436bf18dac1887a 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1181,10 +1181,11 @@
>  	vmw_devcaps_destroy(dev_priv);
>  	vmw_vram_manager_fini(dev_priv);
>  	ttm_device_fini(&dev_priv->bdev);
> -	vmw_release_device_late(dev_priv);
> -	vmw_fence_manager_takedown(dev_priv->fman);
> +	vmw_fence_fifo_down(dev_priv->fman);
>  	if (dev_priv->capabilities & SVGA_CAP_IRQMASK)
>  		vmw_irq_uninstall(&dev_priv->drm);
> +	vmw_release_device_late(dev_priv);
> +	vmw_fence_manager_takedown(dev_priv->fman);
> 
>  	ttm_object_device_release(&dev_priv->tdev);
> 
> --
> 2.39.5
> 

With this patch we would miss any command buffer processing done on the svga
device in the window between irq_uninstall and vmw_cmdbuf_man_destroy(), so it's
better to leave the takedown order as is.
If you want to address the case where a thread_fn awoken by an earlier irq is
scheduled before irq_uinstall but after vmw_cmdbuf_man_destroy(), then just add checks
to vmw_thread_fn(), though I'm not sure if that case is even possible since the
command buffer contexts should already be taken down at that point.


-- 
Maaz Mombasawala <maaz.mombasawala@broadcom.com>

Re: [PATCH] drm/vmwgfx: uninstall the IRQ before freeing the command buffer manager on unload

[Maaz Mombasawala]

On 7/16/26 7:24 PM, Fan Wu wrote:
> Hi Maaz,
> 
> Thanks for the review. I'll drop this reordering — keeping the IRQ alive
> for command-buffer processing during teardown is the right call.
> 
> One question before I investigate further: is there an existing teardown
> invariant that guarantees an already-woken, but not yet run, CMDBUF or
> FENCE threaded handler (vmw_thread_fn) cannot remain pending once cman or
> fman teardown begins? If there is, I may well be chasing a non-issue, and
> I'd rather not propose an alternative that fights an invariant I'm missing.
> 
> I'll re-check whether such a handler can outlive the current destroy
> sequence before suggesting anything further.
> 
> Best,
> Fan Wu
> 

You can drop this, the current teardown sequence processes things in
the correct order workqueue -> thread_fn -> irq.

-- 
Maaz Mombasawala <maaz.mombasawala@broadcom.com>