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 <[email protected]> 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/[email protected]?part=15
