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

[sashiko-bot@kernel.org]

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