Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [Critical] The error path in `panfrost_perfcnt_enable_locked` incorrectly assigns `perfcnt->user = user` instead of clearing it, leading to a dangling session state and severe Use-After-Free vulnerabilities. - [High] In `panfrost_perfcnt_disable_locked`, the MMU Address Space (AS) is released before the GPU performance counters are disabled. - [High] The loop counter `retries` in `panfrost_perfcnt_dump_locked` is typed as an unsigned 64-bit integer, making the loop condition `--retries >= 0` always true. - [High] `panfrost_perfcnt_postreset()` ignores the return value of `panfrost_perfcnt_hw_enable()`, leading to Address Space (AS) reference count underflows and unconfigured hardware. - [Medium] Clearing `perfcnt->reset_happened` before an interruptible wait in `panfrost_perfcnt_dump_locked` causes reset notifications to be lost if the wait is interrupted. --
--- Patch [12]: [PATCH v4 12/13] drm/panfrost: Fix races between perfcnt and reset sequence --- commit 33e476c0f2af99299bbeb687048c33539616ce05 Author: Adrián Larumbe <[email protected]> drm/panfrost: Fix races between perfcnt and reset sequence This commit uses the reset r/w semaphore to govern hardware access during resets. It expands the DRM uAPI for the perfcnt DUMP operation to notify userspace if a reset occurred, and reestablishes perfcnt to its original configuration post-reset. > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > index ad1156678e91c..b92ad90618931 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c [ ... ] > -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev) > +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, > + u64 *reset_happened) > { > - u64 gpuva; > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt; > + u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT; > + u64 retries = PERFCNT_DUMP_MAX_RETRIES; > int ret; > > - reinit_completion(&pfdev->perfcnt->dump_comp); > - gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT; > - gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva)); > - gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva)); > - gpu_write(pfdev, GPU_INT_CLEAR, > - GPU_IRQ_CLEAN_CACHES_COMPLETED | > - GPU_IRQ_PERFCNT_SAMPLE_COMPLETED); > - gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE); > +dump_retry: > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + *reset_happened = perfcnt->reset_happened; > + perfcnt->reset_happened = false; [Severity: Medium] Could clearing perfcnt->reset_happened before the wait cause lost notifications? If wait_for_completion_interruptible_timeout() is interrupted by a signal, it returns a negative error code (like -ERESTARTSYS). The ioctl will fail and skip copying the hw_reset state to userspace. When the kernel transparently restarts the syscall, the flag has already been cleared here. Does this result in userspace successfully getting a dump but missing the reset notification, leading to invalid performance counter deltas? > + reinit_completion(&pfdev->perfcnt->dump_comp); > + gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva)); > + gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva)); > + gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_CLEAN_CACHES_COMPLETED | > + > GPU_IRQ_PERFCNT_SAMPLE_COMPLETED); > + gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE); > + } > + > ret = > wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp, > msecs_to_jiffies(1000)); > - if (!ret) > - ret = -ETIMEDOUT; > - else if (ret > 0) > - ret = 0; > + > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + if (ret > 0) { > + if (perfcnt->reset_happened) { > + if (--retries >= 0) [Severity: High] Can this loop condition ever be false? Since retries is declared as an unsigned u64 at the top of the function, decrementing it past zero wraps around to ULLONG_MAX, which is always >= 0. If a hardware reset is continually detected (for example, a workload repeatedly hanging the GPU), does this trap the ioctl in an infinite loop instead of gracefully returning -EBUSY? > + goto dump_retry; > + else > + ret = -EBUSY; > + } else { > + ret = 0; > + } > + } else if (!ret) { > + ret = -ETIMEDOUT; > + } > + } > > return ret; > } [ ... ] > static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev, > struct drm_file *file_priv, > unsigned int counterset) > { [ ... ] > /* The BO ref is retained by the mapping. */ > drm_gem_object_put(&bo->base); > > - perfcnt->user = user; > - > return 0; > > err_vunmap: > + scoped_guard(rwsem_read, &pfdev->reset.lock) > + perfcnt->user = user; [Severity: Critical] Is this intended to assign 'user', or should it clear the user pointer? If a failure occurs (such as in panfrost_perfcnt_hw_enable()), the error path jumps to err_vunmap where it unmaps the buffers and frees the mappings. By assigning perfcnt->user = user here instead of NULL, the user pointer remains valid while the underlying resources are freed. Can a subsequent call to disable or dump the counters by the same user bypass the active session check and trigger a use-after-free on the freed memory? > drm_gem_vunmap(&bo->base, &map); > err_put_mapping: > panfrost_gem_mapping_put(perfcnt->mapping); > @@ -203,13 +254,15 @@ static int panfrost_perfcnt_disable_locked(struct > panfrost_device *pfdev, > if (user != perfcnt->user) > return -EINVAL; > > - panfrost_perfcnt_gpu_disable(pfdev); > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu); > + panfrost_perfcnt_gpu_disable(pfdev); [Severity: High] Is it safe to release the Address Space (AS) before disabling the hardware that uses it? By calling panfrost_mmu_as_put() first, the AS reference count drops and it becomes immediately available for reallocation to another concurrent context. However, the performance counters are still actively configured to write to this AS index until panfrost_perfcnt_gpu_disable() executes. Does this create a race window where performance counters emit data into a repurposed address space, potentially causing memory corruption or page faults? > + perfcnt->user = NULL; > + } > > - perfcnt->user = NULL; > drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map); > perfcnt->buf = NULL; > panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv); > - panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu); > panfrost_gem_mapping_put(perfcnt->mapping); > perfcnt->mapping = NULL; > pm_runtime_put_autosuspend(pfdev->base.dev); [ ... ] > +void panfrost_perfcnt_postreset(struct panfrost_device *pfdev) > +{ > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt; > + > + if (drm_WARN_ON(&pfdev->base, !perfcnt)) > + return; > + > + lockdep_assert_held(&pfdev->reset.lock); > + > + if (!perfcnt->user) > + return; > + > + panfrost_perfcnt_hw_enable(pfdev); [Severity: High] Should we check the return value of panfrost_perfcnt_hw_enable() here? During a GPU reset, all Address Spaces are reset and their reference counts zeroed. If panfrost_perfcnt_hw_enable() fails (for example, if no AS is available and panfrost_mmu_as_get returns -EBUSY), the hardware remains unconfigured and the AS reference count is not incremented. Since this failure is silently discarded, when the session is eventually disabled, panfrost_perfcnt_disable_locked() will unconditionally call panfrost_mmu_as_put(). Will this decrement the AS count below zero, triggering a warning and corrupting the MMU AS tracking logic? > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12
