From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm: Add PERFCNTR_CONFIG ioctl
Date: Thu, 23 Apr 2026 09:13:16 +1000
Message-ID: <review-patch13-20260420222621.417276-14-robin.clark@oss.qualcomm.com>
In-Reply-To: <20260420222621.417276-14-robin.clark@oss.qualcomm.com>
References: <20260420222621.417276-1-robin.clark@oss.qualcomm.com> <20260420222621.417276-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

The main ioctl implementation. Well-structured with clear separation between validation, allocation, and setup phases. The fd-based stream with `DRM_IOW` (returning fd via ioctl return value) is a nice design choice, well-documented in the UAPI header.

**Bug -- stack buffer overflow via `group_stride`**:
```c
struct drm_msm_perfcntr_group g = {0};
void __user *userptr =
    u64_to_user_ptr(args->groups + (i * args->group_stride));

if (copy_from_user(&g, userptr, args->group_stride))
    return -EFAULT;
```
If `args->group_stride > sizeof(struct drm_msm_perfcntr_group)`, this `copy_from_user` writes past the end of the stack variable `g`. The `group_stride` field exists for future extensibility (to allow adding fields to the struct), but there's no check that it doesn't exceed `sizeof(g)`. Fix:
```c
if (copy_from_user(&g, userptr, min((size_t)args->group_stride, sizeof(g))))
```

**Missing validation on `bufsz_shift`**: There's no upper bound check on `args->bufsz_shift`. A malicious user could pass `bufsz_shift = 30` to try to allocate 1GB of kernel memory:
```c
void *buf __free(kfree) =
    kmalloc(1 << args->bufsz_shift, GFP_KERNEL);
```
Add a reasonable upper bound (e.g., `if (args->bufsz_shift > 20)` for a 1MB cap), or at least use `kmalloc()` with `__GFP_NOWARN` to avoid log spam from failed huge allocations.

**Typo in commit message**: "on exist of IFPC" should be "on exit of IFPC".

**Typo in error message** (line 7030):
```c
return UERR(EBUSY, dev, "groups[%d]: to few counters available", i);
```
Should be "too few".

**`strncmp` for group name matching** in `get_group_idx()`:
```c
if (!strncmp(group->name, name, len))
```
where `len = sizeof(g.group_name)` which is 16. This compares at most 16 bytes, but if a group name is exactly 16 bytes (no null terminator), `strncmp` would match a prefix. Since group names are likely short (e.g., "CP", "SP"), this is unlikely to be a practical issue, but `strnlen` + exact match would be more robust.

**Lock ordering**: `get_available_counters()` acquires `gpu->dev->filelist_mutex` while `gpu->perfcntr_lock` is already held. Verify this doesn't conflict with any path that takes these locks in the opposite order (e.g., `drm_file` open/close paths that might touch perfcntr state).

**Race on stream teardown**: In `msm_perfcntrs_stream_release()`, between dropping `perfcntr_lock` and calling `cancel_work_sync(&stream->sel_work)`, `sel_worker` could be scheduled and run. The worker checks `stream != gpu->perfcntrs->stream` under lock, which would be true (since we set it to NULL), so it would bail out via `break`. This is safe -- good design with the documented comment.

**Missing `EPOLLERR`/`EPOLLHUP`**: The `poll` implementation only returns `EPOLLIN`. It might be useful to signal `EPOLLHUP` when the stream is being torn down, but this is a minor enhancement, not a bug.

---
Generated by Claude Code Patch Reviewer