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: Thu, 07 May 2026 13:19:06 +1000 Message-ID: In-Reply-To: <20260506171127.133572-14-robin.clark@oss.qualcomm.com> References: <20260506171127.133572-1-robin.clark@oss.qualcomm.com> <20260506171127.133572-14-robin.clark@oss.qualcomm.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Status: Several issues** This is the core patch with the UAPI and main ioctl implementation. **Issue 1: Missing bufsz_shift validation** ```c stream->fifo_size = 1 << args->bufsz_shift; ``` If `bufsz_shift` is >= 31 (or >= 63 on 64-bit), this is undefined behavior. The later check `stream->fifo_size > SZ_128M` would catch large values, but by then the shift has already been performed. The v4 changelog says "Add upper limit to userspace controlled FIFO size [Claude]" was added, and the `> SZ_128M` check is there, but the shift itself should be validated first: ```c if (args->bufsz_shift > 27) /* SZ_128M = 1 << 27 */ return -EINVAL; ``` **Issue 2: copy_from_user with args->group_stride could under-read** ```c if (copy_from_user(&g, userptr, args->group_stride)) ``` If `args->group_stride < sizeof(struct drm_msm_perfcntr_group)`, this copies fewer bytes than the struct size, leaving parts of `g` at their zero-initialized values. If `args->group_stride > sizeof(struct drm_msm_perfcntr_group)`, this overflows the stack variable `g`. There's no validation that `group_stride` matches `sizeof(struct drm_msm_perfcntr_group)`. This is presumably intentional for extensibility, but the overflow case needs guarding: ```c if (args->group_stride > sizeof(g)) return -EINVAL; ``` Or use `min(args->group_stride, sizeof(g))` in the copy. **Issue 3: State not rolled back on E2BIG for global stream** When processing groups with `MSM_PERFCNTR_STREAM`, the code writes `allocated_counters` and `countables` into `perfcntrs->groups[idx]` before checking all groups: ```c perfcntrs->groups[idx]->allocated_counters = g.nr_countables; ... if (copy_from_user(perfcntrs->groups[idx]->countables, userptr, sz)) return -EFAULT; ``` If a later group triggers E2BIG, the function returns the error but has already modified `perfcntrs->groups[idx]->allocated_counters` for earlier groups. Since no stream is actually installed (`perfcntrs->stream` remains NULL), this seems harmless in practice -- those `allocated_counters` values are only meaningful when `perfcntrs->stream` is set. But it's messy; a subsequent call with different groups could see stale `allocated_counters` values affecting `get_available_counters()`. Consider clearing the group state on error. **Issue 4: FIFO read may return 0 bytes without -EAGAIN** In `msm_perfcntrs_stream_read()`, after waking from the wait for blocking mode: ```c count = min_t(size_t, count, fifo_count_to_end(stream)); ``` If `fifo_count_to_end()` returns 0 (possible if another reader consumed the data, though `read_lock` prevents that for the single-consumer case), the function returns 0, which means EOF to userspace. For a streaming fd, this would be unexpected. Since `read_lock` serializes consumers, this shouldn't happen in practice, but it's a subtle edge case. **Issue 5: Missing `O_CLOEXEC` on anon_inode_getfd** ```c stream_fd = anon_inode_getfd("[msm_perfcntrs]", &stream_fops, stream, 0); ``` The flags arg is 0. Most modern kernel code passes `O_CLOEXEC` to prevent fd leaks across exec. Consider using `O_CLOEXEC` by default, or accepting flags from userspace. **Issue 6: `guard(pm_runtime_active_auto)` scope** ```c guard(pm_runtime_active_auto)(&gpu->pdev->dev); guard(mutex)(&gpu->perfcntr_lock); ``` The pm_runtime guard holds a reference for the entire ioctl scope, but it's only needed for the stream case (to ensure the GPU is powered for SEL programming). For the local reservation case, pm_runtime is unnecessary. **UAPI comment nit:** ```c * The data read from the has the following format ``` Missing word: "The data read from the **fd** has the following format". --- Generated by Claude Code Patch Reviewer