Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] The error path in `panfrost_perfcnt_enable_locked` incorrectly sets `perfcnt->user = user` instead of `NULL`, leading to a corrupt state and Use-After-Free. - [Medium] Hardware reset notification is permanently lost if the ioctl wait is interrupted by a signal. --
--- Patch [13]: [PATCH v4 13/13] drm/panfrost: Bump driver minor to reflect new DUMP IOCTL req field --- commit 33e476c0f2af99299bbeb687048c33539616ce05 Author: Adrián Larumbe <[email protected]> drm/panfrost: Fix races between perfcnt and reset sequence This commit fixes races between the reset sequence and perfcnt session teardown. It uses a read/write semaphore to serialize access to the hardware during resets and updates the DRM uAPI to notify userspace of resets during a perfcnt DUMP operation. > 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 [ ... ] > @@ -55,38 +59,97 @@ void panfrost_perfcnt_sample_done(struct panfrost_device > *pfdev) > gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES); > } > > -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] Does this code permanently lose the hardware reset notification if the ioctl wait is interrupted by a signal? Since the internal state is cleared prior to the interruptible wait and not restored on the -ERESTARTSYS error path, could a restarted syscall overwrite req->hw_reset with the cleared value, causing userspace to accumulate counters that were actually reset? > + 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)); [ ... ] > @@ -127,60 +192,46 @@ static int panfrost_perfcnt_enable_locked(struct > panfrost_device *pfdev, [ ... ] > /* 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 it possible this error path incorrectly reassigns perfcnt->user to user instead of setting it to NULL? If panfrost_perfcnt_hw_enable() fails (e.g. when panfrost_mmu_as_get fails because address spaces are exhausted), could this leave perfcnt->user set while the backing resources are freed, possibly leading to a use-after-free on subsequent ioctls? > drm_gem_vunmap(&bo->base, &map); > err_put_mapping: > panfrost_gem_mapping_put(perfcnt->mapping); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
