Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Time-of-Check to Time-of-Use (TOCTOU) race condition on timeout in panfrost_perfcnt_enable_locked() causes MMU Address Space leak and use-after-free risk. - [High] Incorrect handling of perfcnt->reset_failed causes MMU AS reference underflows and leaks. - [Medium] State loss of reset notification flag in panfrost_perfcnt_dump_locked() due to premature clearing. --
--- Patch [10]: [PATCH v5 10/11] drm/panfrost: Fix races between perfcnt and reset sequence --- Note: The format of this review has been altered to use a free-form summary rather than inline diff quoting due to recitation restrictions. commit 3ad1a2b0fa637dba7c60b8e41dbe3e3239112852 Author: Adrián Larumbe <[email protected]> Subject: drm/panfrost: Fix perfcnt reset sequence and expand dump uAPI This commit modifies the Panfrost driver to use a reset read/write semaphore to prevent race conditions during performance counter teardowns. It also expands the DRM uAPI for the perfcnt dump operation to notify userspace if a reset occurred, allowing it to handle counter resets properly. [Severity: High] Can this sequence cause an MMU Address Space leak and a use-after-free risk? In panfrost_perfcnt_enable_locked() in drivers/gpu/drm/panfrost/panfrost_perfcnt.c, the code locklessly checks !perfcnt->reset_happened after the wait for completion times out. If a GPU reset occurs between this lockless check and the acquisition of pfdev->reset.lock at the err_vunmap label, the reset worker will see an active session because perfcnt->user is not yet NULL. The reset worker would then call panfrost_perfcnt_postreset(), allocating an AS reference and programming the GPU. When the original thread subsequently acquires the lock, it clears perfcnt->user and frees the buffer without calling panfrost_mmu_as_put(). Would this leave the GPU hardware active and writing to an unmapped buffer? [Severity: Medium] Does clearing the reset flag before the wait completes successfully cause userspace to lose the reset notification? In panfrost_perfcnt_dump_locked() in drivers/gpu/drm/panfrost/panfrost_perfcnt.c, perfcnt->reset_happened is destructively cleared to false before calling wait_for_completion_interruptible_timeout(). If the wait is interrupted by a signal, it returns -ERESTARTSYS. The syscall would be restarted, but since the reset_happened flag was already cleared, the second attempt will falsely report to userspace that no reset occurred, leading to incorrect accumulation of performance counters. [Severity: High] Could the current handling of perfcnt->reset_failed cause MMU AS reference underflows or leaks? In panfrost_perfcnt_postreset() in drivers/gpu/drm/panfrost/panfrost_perfcnt.c, perfcnt->reset_failed is set to true on failure, but it does not appear to be cleared on success. If panfrost_mmu_reset() resets all AS counts to 0 and postreset fails, panfrost_perfcnt_disable_locked() still unconditionally calls panfrost_mmu_as_put() without checking if reset_failed is true. This drops the count to -1 and triggers a WARN_ON. Alternatively, if a subsequent reset occurs and postreset succeeds, the failure to clear reset_failed means the next call to panfrost_perfcnt_dump_locked() will see reset_failed as true and call panfrost_perfcnt_hw_enable() again. This would double-acquire the AS, which is only put once when disabled, leading to a permanent leak. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
