Re: [PATCH v13 15/15] 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:
- [High] drm/panfrost: sticky DEAD flag causes AS reference leak on multiple GPU resets
- [High] drm/panfrost: UAF in perfcnt context due to devres lifecycle mismatch
- [Medium] drm/panfrost: perfcnt->state is destroyed on ERESTARTSYS signal interrupt in panfrost_perfcnt_dump_locked()
- [Low] drm/panfrost: cleanup.md violation by mixing goto and scoped_guard in panfrost_perfcnt_enable_locked
--

commit be4c8f35c54f411638ee240c72cfc88e3f4b3c5d
Author: Adrián Larumbe <adrian.larumbe@collabora.com>

drm/panfrost: Fix races between perfcnt and reset sequence

This commit updates the Panfrost driver to use the reset rw_semaphore
when configuring performance counters and during dump ioctls to avoid
races with the reset sequence. It also expands the DUMP uAPI to report
reset states back to userspace and bumps the DRM minor version.

> 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
> @@ -118,12 +118,22 @@ static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 *stat
>  {
[ ... ]
>  	scoped_guard(rwsem_read, &pfdev->reset.lock) {
>  		*state = perfcnt->state;
>  		if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD)
>  			return -EIO;
>  
>  		perfcnt->state = 0;
>  
>  		reinit_completion(&pfdev->perfcnt->dump_comp);
[ ... ]
>  	ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
>  							msecs_to_jiffies(1000));
>  
>  	/* A reset might come through in the gap between the completion returning and the following
>  	 * check, but because no sample was produced, we don't care to relay the state back to UM.
>  	 */
>  	if (!ret)
>  		ret = -ETIMEDOUT;
>  
>  	/* We should not touch perfcnt's state when the completion wait was interrupted by UM */
>  	if (ret < 0)
>  		return ret;

[Severity: Medium]
Could the perfcnt state be permanently lost if the completion wait is
interrupted by a signal like -ERESTARTSYS? 

The state is cleared before the wait, and if ret is less than zero, the
function returns early without restoring it. When the kernel restarts the
ioctl, userspace might miss the reset flag because the state has already
been zeroed out.

[ ... ]
> @@ -182,10 +182,19 @@
>  static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
>  					  struct drm_file *file_priv,
>  					  unsigned int counterset)
>  {
[ ... ]
>  	if (perfcnt->user == user) {
>  		scoped_guard(rwsem_read, &pfdev->reset.lock) {
>  			if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD) {
>  				ret = panfrost_perfcnt_hw_enable(pfdev);
>  				if (!ret)
>  					perfcnt->state = 0;
>  				return ret;
>  			}
>  		}
>  
>  		return 0;
>  	}

[Severity: High]
Does this cause an Address Space reference leak after multiple resets?

If a prior hardware reset fails, the PANFROST_PERFCNT_SESSION_DEAD flag
is set in panfrost_perfcnt_reset(). A subsequent successful reset drops
owns_as_ref but does not clear the dead flag. 

If userspace then issues an enable ioctl, it takes this path and calls
panfrost_perfcnt_hw_enable() which observes owns_as_ref is already true,
and takes a second address space reference without releasing the first.

[ ... ]
> @@ -247,16 +247,15 @@
>  	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]
Is it safe to jump out of a scoped guard using a goto label?

If panfrost_perfcnt_hw_enable() fails, it jumps out of the scoped guard to
the err_vunmap label. The cleanup subsystem guide recommends not mixing
goto-based error handling and scope-based cleanups in the same function
due to the potential for confusing ownership semantics.

[ ... ]
> @@ -326,9 +326,14 @@
>  int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
>  				struct drm_file *file_priv)
>  {
>  	struct panfrost_device *pfdev = to_panfrost_device(dev);
>  	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
>  	struct drm_panfrost_perfcnt_dump *req = data;
>  	void __user *user_ptr = (void __user *)(uintptr_t)req->buf_ptr;
>  	int ret;
>  
>  	ret = panfrost_unstable_ioctl_check();
>  	if (ret)
>  		return ret;
>  
> +	if (req->pad)
> +		return -EINVAL;
> +
>  	mutex_lock(&perfcnt->lock);

[Severity: High]
Might this trigger a memory corruption if the device is unbound while the
DRM file descriptor remains open?

The perfcnt structure is allocated with devm_kzalloc() tied to the platform
device pfdev->base.dev, so it is freed when the device unbinds. However,
the DRM file operations can outlive the platform device. 

When userspace calls this ioctl later, it unconditionally dereferences
pfdev->perfcnt and attempts to acquire perfcnt->lock. Could this be
protected with a drm_dev_enter() unplug check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-claude-fixes-v13-0-d6a493185860@collabora.com?part=15