Re: [PATCH v3 2/4] drm/imagination: unplug the device before tearing it down in pvr_remove()

[sashiko-bot@kernel.org]

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

New issues:
- [High] TOCTOU race on `pvr_dev->lost` in `pvr_remove()` allows a double `drm_dev_unplug()` execution, causing a kernel oops.

Pre-existing issues:
- [Critical] Deadlock caused by calling `drm_dev_unplug()` from within a `drm_dev_enter()` SRCU read critical section via `pvr_device_lost()`.
- [Critical] Incorrect teardown ordering in `pvr_remove()` destroys firmware memory before stopping asynchronous workers (DRM schedulers), leading to Use-After-Free.
- [High] Missing `drm_dev_enter()` guards in teardown ioctls invalidate the commit's core synchronization assumption.
--

commit 3adf692930871afc287c13c7680a8b09a7c0ad9e
Author: Ryan Brue <ryanbrue.dev@gmail.com>

drm/imagination: unplug the device before tearing it down in pvr_remove()

This commit reorders the teardown sequence in pvr_remove() to call
drm_dev_unplug() before destroying the job and free list xarrays, powering
down the GPU, and running pvr_device_fini(). It skips the unplug if
pvr_device_lost() has already performed it.

Fixes: 1f88f017e649 ("drm/imagination: Get GPU resources")
Fixes: 727538a4bbff ("drm/imagination: Implement power management")
Cc: stable@vger.kernel.org

> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index 5c965ef0274ff..fc92a82a72084 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -1469,15 +1469,23 @@ 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);
>  
> +	/*
> +	 * Unplug before freeing anything, so no ioctl is still inside
> +	 * drm_dev_enter(). pvr_device_lost() may already have done it, and

[Severity: Critical]
This is a pre-existing issue, but does calling drm_dev_unplug() from
pvr_device_lost() introduce a potential self-deadlock when triggered from
an MMU flush?

Looking at pvr_mmu_flush_exec(), it acquires the SRCU read lock via
drm_dev_enter(). If the flush times out, it attempts recovery and calls
pvr_device_lost(), which subsequently triggers drm_dev_unplug() and its
internal synchronize_srcu():

drivers/gpu/drm/imagination/pvr_mmu.c:pvr_mmu_flush_exec() {
        if (!drm_dev_enter(from_pvr_device(pvr_dev), &idx))
                return -EIO;
        ...
        /* On timeout calls pvr_power_reset() or pvr_device_lost() */
}

Since synchronize_srcu() waits indefinitely for all SRCU readers to exit, and
the calling thread itself holds the read lock, won't it hang forever waiting
for its own completion?

> +	 * drm_dev_unplug() is not idempotent.
> +	 */
> +	if (!pvr_dev->lost)
> +		drm_dev_unplug(drm_dev);

[Severity: High]
Is this vulnerable to a time-of-check to time-of-use race with
pvr_power_reset() and pvr_device_lost()?

If pvr_remove() runs on the unbind path and evaluates !pvr_dev->lost as true,
could it be preempted before calling drm_dev_unplug()?

Concurrently, if an asynchronous context like a DRM scheduler timeout triggers
pvr_power_reset(), it would call pvr_device_lost(). Because the flag isn't set
yet, pvr_device_lost() would also evaluate !pvr_dev->lost as true, set the
flag, and execute drm_dev_unplug():

drivers/gpu/drm/imagination/pvr_power.c:pvr_device_lost() {
        if (!pvr_dev->lost) {
                pvr_dev->lost = true;
                drm_dev_unplug(from_pvr_device(pvr_dev));
        }
}

When pvr_remove() resumes, wouldn't it execute drm_dev_unplug() a second time,
causing a kernel oops inside drm_client_sysrq_unregister() since
drm_dev_unplug() is not idempotent?

[Severity: High]
This is a pre-existing issue, but does this unplug actually synchronize with
all ioctls as intended?

While this patch relies on drm_dev_unplug() to wait for in-flight ioctls via
drm_dev_enter() and drm_dev_exit(), some teardown ioctls appear to lack these
guards entirely.

For example, in pvr_ioctl_destroy_context():

drivers/gpu/drm/imagination/pvr_drv.c:pvr_ioctl_destroy_context() {
        ...
        return pvr_context_destroy(pvr_file, args->handle);
}

And in pvr_ioctl_vm_unmap():

drivers/gpu/drm/imagination/pvr_drv.c:pvr_ioctl_vm_unmap() {
        ...
        err = pvr_vm_unmap(vm_ctx, args->device_addr, args->size);
        ...
}

Since these ioctls bypass the drm_dev_enter() check, couldn't userspace
execute them concurrently during or after the unplug, interacting with
hardware queues via pvr_queue_cleanup_fw_context() after pvr_remove() has
already freed them?

[ ... ]

>  	xa_destroy(&pvr_dev->job_ids);
>  	xa_destroy(&pvr_dev->free_list_ids);
>  
> -	pm_runtime_suspend(drm_dev->dev);
>  	pvr_device_fini(pvr_dev);
> -	drm_dev_unplug(drm_dev);
>  	pvr_watchdog_fini(pvr_dev);
>  	pvr_queue_device_fini(pvr_dev);

[Severity: Critical]
This is a pre-existing issue, but does the ordering here tear down firmware
memory before stopping asynchronous workers?

When pvr_device_fini() is called, it tears down hardware components and frees
firmware memory such as pvr_dev->fw_dev. However, the DRM scheduler threads
executing on pvr_dev->sched_wq are not stopped until pvr_queue_device_fini()
is called later.

Couldn't drm_sched concurrently execute pvr_queue_run_job() while or after the
firmware memory is freed, leading to a use-after-free when it accesses that
memory to submit KCCB commands?

Note that while a subsequent patch in the series moves pvr_watchdog_fini(),
the DRM scheduler teardown issue appears to remain unresolved here.

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