From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm: Add PERFCNTR_CONFIG ioctl Date: Tue, 05 May 2026 08:06:04 +1000 Message-ID: In-Reply-To: <20260504190751.61052-14-robin.clark@oss.qualcomm.com> References: <20260504190751.61052-1-robin.clark@oss.qualcomm.com> <20260504190751.61052-14-robin.clark@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review This is the core patch. Several observations: **Issue: No upper bound on `bufsz_shift`.** ```c stream->fifo_size =3D 1 << args->bufsz_shift; ... void *buf =3D kmalloc(1 << args->bufsz_shift, GFP_KERNEL); ``` A malicious/buggy user could pass `bufsz_shift =3D 30` and try to allocate = 1 GB. The ioctl requires `perfmon_capable()` for STREAM mode so this is som= ewhat mitigated, but a bounds check would be prudent. Also, if `bufsz_shift= ` is 0 and STREAM is not set, `fifo_size` would be 1 which is odd but harml= ess since the FIFO is never used. **Issue: `guard(pm_runtime_active_auto)` does not check return value.** ```c guard(pm_runtime_active_auto)(&gpu->pdev->dev); ``` This expands to `pm_runtime_get_sync()` which can fail. The guard pattern d= oesn't allow checking the return value. If the device fails to resume, the = ioctl will proceed with a powered-off GPU. Consider using an explicit `pm_r= untime_get_sync()` with error checking, or at minimum documenting why this = is acceptable. **Good: UAPI design.** The `DRM_IOW` direction is correctly chosen =E2=80= =94 the comment explains that returning the fd as a return value (rather th= an via an output parameter) avoids the `copy_to_user` fault-after-fd-creati= on problem. The `MSM_PERFCNTR_UPDATE` flag for returning available counter = counts on `E2BIG` is a nice touch for discoverability. **Good: Locking discipline.** The `perfcntr_lock` protects counter allocati= on state, `read_lock` protects FIFO consumer side, and the single-producer = pattern (kthread worker) avoids needing locks on the producer side. The `se= l_work` is correctly dispatched on the scheduler's submit_wq to serialize w= ith GEM_SUBMITs. **Minor: `UERR(ETOOSMALL, ...)`.** Is `ETOOSMALL` a standard errno? I belie= ve the correct name is `ETOOMANYREFS` or similar =E2=80=94 this may be a cu= stom MSM driver macro. If `UERR` handles non-standard names, fine; otherwis= e this should be `EINVAL` or a more standard error. **Minor: `group_stride` validation.** The stride is validated to be non-zer= o when `nr_groups > 0`, but there's no check that `group_stride >=3D sizeof= (struct drm_msm_perfcntr_group)`. A too-small stride would cause `copy_from= _user` to read partial data. However, since `copy_from_user` uses `args->gr= oup_stride` as the size, a smaller stride would just read fewer fields =E2= =80=94 the zero-initialization of `g` mitigates this. **Minor: `scoped_guard` with `break`.** In `sel_worker`: ```c scoped_guard (mutex, &gpu->lock) { guard(mutex)(&gpu->perfcntr_lock); if (stream !=3D gpu->perfcntrs->stream) break; ... } ``` Using `break` to exit a `scoped_guard` block is valid but unusual =E2=80=94= it works because `scoped_guard` expands to a loop-like construct. This is = fine but may surprise readers. --- Generated by Claude Code Patch Reviewer