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 <[email protected]> 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/[email protected]?part=1
