Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] State Loss: The `reset_happened` flag is cleared prematurely on error paths - [Medium] API Flaw: Uninitialized userspace memory leaked back as flags and unvalidated padding - [Low] Cleanup Rule Violation: Mixing goto-based error handling and scoped_guard in the same function Pre-existing issues: - [High] Race Condition / GPU Fault: Pending asynchronous GPU sample operation is not cancelled when wait is interrupted -- commit 9121f328e8a02b83046e96373b7d6c617fd603c1 Author: Adrián Larumbe <[email protected]> drm/panfrost: Fix races between perfcnt and reset sequence This commit uses the reset read/write semaphore to manage access to hardware during reset time. It expands the DRM uAPI for the perfcnt dump operation so userspace can be aware of hardware resets. It also attempts to reestablish the perfcnt configuration if a session was enabled during a reset. > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > @@ -47,11 +52,15 @@ static void panfrost_perfcnt_hw_disable(struct > panfrost_device *pfdev) > void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev) > { > - if (pfdev->features.selected_coherency != COHERENCY_ACE) > + if (pfdev->features.selected_coherency != COHERENCY_ACE) { > gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES); > - else > + } else { > + pfdev->perfcnt->dump_finished = true; > complete(&pfdev->perfcnt->dump_comp); > + } > } [Severity: High] This is a pre-existing issue, but does this IRQ handler complete the dump_comp unconditionally without verifying if it corresponds to the currently active dump request? If the wait in panfrost_perfcnt_dump_locked() is interrupted by a signal, the hardware GPU_CMD_PERFCNT_SAMPLE command could still be running. If userspace immediately closes the file descriptor, the GEM object and MMU mappings could be freed while the GPU is still actively writing to memory, potentially causing a GPU page fault. [ ... ] > @@ -60,25 +105,43 @@ static int panfrost_perfcnt_dump_locked(struct > panfrost_device *pfdev) > -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev) > +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 > *state) > { > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + if (!perfcnt->owns_as_ref) { > + *state = PANFROST_PERFCNT_SESSION_DEAD; > + return -EIO; > + } > + > + if (perfcnt->reset_happened) { > + *state = PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET; > + perfcnt->reset_happened = false; > + } [Severity: High] If wait_for_completion_interruptible_timeout() gets interrupted (e.g., by a signal) or times out, it will return an error like -ERESTARTSYS. Since perfcnt->reset_happened is cleared here at the start of the function, won't the reset condition be permanently lost? If the ioctl is retried by VFS or userspace, userspace might incorrectly accumulate counter values across a GPU reset without knowing it happened. [ ... ] > @@ -122,54 +200,31 @@ static int panfrost_perfcnt_enable_locked(struct > panfrost_device *pfdev, > ret = drm_gem_vmap(&bo->base, &map); > if (ret) > goto err_put_mapping; > + > perfcnt->buf = map.vaddr; > + perfcnt->counterset = counterset; > > panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer"); > > + scoped_guard(rwsem_read, &pfdev->reset.lock) { > + /* > + * Clear the counters to start from a fresh state. > + */ > + gpu_write(pfdev, GPU_INT_CLEAR, > GPU_IRQ_PERFCNT_SAMPLE_COMPLETED); > + gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR); > > + ret = panfrost_perfcnt_hw_enable(pfdev); > + if (ret) > + goto err_vunmap; [Severity: Low] Does this goto statement jump out of the scoped_guard() block directly into legacy error handling labels? The subsystem coding guidelines specify that goto-based cleanup and scoped_guard() / __free() shouldn't be mixed in the same function to avoid confusing ownership semantics. [ ... ] > @@ -255,7 +313,7 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, > void *data, > goto out; > } > > - ret = panfrost_perfcnt_dump_locked(pfdev); > + ret = panfrost_perfcnt_dump_locked(pfdev, &req->state); [Severity: Medium] Since DRM_IOCTL_PANFROST_PERFCNT_DUMP uses DRM_IOWR, is it possible that req->state and req->pad contain uninitialized user stack memory here? It appears req->state is passed directly into panfrost_perfcnt_dump_locked() where it is conditionally updated using a bitwise OR, without ever being unconditionally zero-initialized first. Also, req->pad doesn't seem to be validated to be zero, which could complicate extending this struct in the future. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=15
