Re: [PATCH v4 12/13] drm/panfrost: Fix races between perfcnt and reset sequence

[sashiko-bot@kernel.org]

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

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/20260729-claude-fixes-v4-0-01968f2ec77a@collabora.com?part=12