Re: [PATCH v8 15/16] drm/panfrost: Fix races between perfcnt and reset sequence

[sashiko-bot@kernel.org]

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 <adrian.larumbe@collabora.com>

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/20260907-claude-fixes-v8-0-c2bcb5e82184@collabora.com?part=15