Re: [PATCH v3 3/4] drm/imagination: stop the watchdog before tearing the device down

[sashiko-bot@kernel.org]

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

New issues:
- [High] Moving pvr_watchdog_fini() before drm_dev_unplug() creates a race where concurrent IOCTLs can trigger a PM resume and re-queue the watchdog work after it has been cancelled.

Pre-existing issues:
- [Critical] pvr_remove() ignores the return value of pm_runtime_suspend() and frees firmware memory while the GPU hardware may still be running.
- [Critical] Inverted teardown order in pvr_remove() destroys the scheduler workqueue after firmware structures are freed, causing pending jobs to execute against freed memory.
- [High] Use-after-free of pvr_dev and sched_wq when a dma_fence outlives the DRM device.
- [High] Concurrent calls to drm_dev_unplug() due to TOCTOU race on the non-atomic pvr_dev->lost flag.
--

commit 34ac43451671ac25507b846dc7e38412b5318f2e
Author: Ryan Brue <ryanbrue.dev@gmail.com>
drm/imagination: stop the watchdog before tearing the device down

This commit moves the watchdog finalization to the beginning of device
removal, aiming to prevent the worker from reading freed memory if it
fires during teardown.

Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index fc92a82a72084..20b27a468327b 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1469,6 +1469,9 @@ static void pvr_remove(struct platform_device *plat_dev)
>  	struct drm_device *drm_dev = platform_get_drvdata(plat_dev);
>  	struct pvr_device *pvr_dev = to_pvr_device(drm_dev);
>  
> +	/* Stop the watchdog before anything it reads is freed. */
> +	pvr_watchdog_fini(pvr_dev);
> +

[Severity: High]
Can a concurrent ioctl re-arm the watchdog right after it is cancelled?

Since drm_dev_unplug() has not blocked ioctls yet, an incoming job
submission could call pm_runtime_resume_and_get(), triggering
pvr_power_fw_enable().

Looking at drivers/gpu/drm/imagination/pvr_power.c:pvr_power_fw_enable():

    queue_delayed_work(pvr_dev->sched_wq, &pvr_dev->watchdog.work,
                       msecs_to_jiffies(WATCHDOG_TIME_MS));

If this occurs, the work is re-queued. Later in pvr_remove(), if
pm_runtime_suspend() fails (returning -EAGAIN) due to the elevated usage
count, the watchdog is never cancelled again and could fire after
pvr_device_fini() frees the firmware memory.

>  	/*
>  	 * Unplug before freeing anything, so no ioctl is still inside
>  	 * drm_dev_enter(). pvr_device_lost() may already have done it, and
>  	 * drm_dev_unplug() is not idempotent.
>  	 */
>  	if (!pvr_dev->lost)
>  		drm_dev_unplug(drm_dev);

[Severity: High]
This is a pre-existing issue, but does checking this non-atomic flag
without locking create a race condition with pvr_device_lost()?

If a hardware timeout (e.g., from an MMU cache flush via
pvr_mmu_flush_exec) concurrently triggers pvr_device_lost() in pvr_power.c,
both threads might check !pvr_dev->lost at the exact same time, evaluate it
as true, and both call drm_dev_unplug().

Since drm_dev_unplug() is not idempotent, couldn't this result in a double
unregister kernel panic?

>  
>  	WARN_ON(!xa_empty(&pvr_dev->job_ids));
>  	WARN_ON(!xa_empty(&pvr_dev->free_list_ids));
>  
>  	pm_runtime_suspend(drm_dev->dev);

[Severity: Critical]
This is a pre-existing issue, but what happens if the runtime PM usage
count is elevated (e.g., due to an active job or a prior sysfs write to
power/control)?

If the usage count is elevated, pm_runtime_suspend() will return -EAGAIN
and skip the suspend callback. The hardware will remain physically powered
on and the firmware will continue to run.

Because pvr_remove() ignores this return value, it will proceed to unmap
MMIO and free firmware OS data unconditionally in pvr_device_fini(), which
might lead to IOMMU faults or memory corruption when the actively running
firmware accesses those freed structures.

>  
>  	xa_destroy(&pvr_dev->job_ids);
>  	xa_destroy(&pvr_dev->free_list_ids);
>  
>  	pvr_device_fini(pvr_dev);
> -	pvr_watchdog_fini(pvr_dev);
>  	pvr_queue_device_fini(pvr_dev);

[Severity: Critical]
This is a pre-existing issue, but does this teardown order cause pending
jobs to execute against freed memory?

pvr_device_fini() unmaps and frees firmware memory. Right after it,
pvr_queue_device_fini() destroys the scheduler workqueue:

drivers/gpu/drm/imagination/pvr_queue.c:pvr_queue_device_fini() {
    destroy_workqueue(pvr_dev->sched_wq);
}

Calling destroy_workqueue() flushes and drains the workqueue, which would
force pending DRM scheduler jobs to execute synchronously via
pvr_queue_run_job().

Since the firmware memory was already freed by pvr_device_fini(), won't
these executed jobs access freed memory?

[Severity: High]
This is a pre-existing issue, but is it possible for a dma_fence to outlive
the DRM device if it is exported to userspace as a sync_file FD?

If a sync_file FD outlives device unbind, closing it later triggers the
fence release callback. In drivers/gpu/drm/imagination/pvr_queue.c:

pvr_queue_fence_release() {
    queue_work(pvr_dev->sched_wq, &fence->release_work);
}

Since pvr_remove() destroys pvr_dev->sched_wq, calling queue_work() on the
destroyed workqueue (and accessing potentially freed pvr_dev) would cause a
crash.

Does pvr_queue_fence_init() need to take a reference to the drm_device
instead of just the pvr_context?

>  	pvr_context_device_fini(pvr_dev);
>  	pvr_power_domains_fini(pvr_dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913-rbrue-suez-upstreaming-pvr-suspend-gpu-system-sleep-v3-0-674b907d0652@gmail.com?part=3