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 <[email protected]> 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: [email protected] > 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-674b907d0...@gmail.com?part=3
