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/vkms: sanitize display mode to prevent hrtimer livelock
Date: Wed, 27 May 2026 13:52:39 +1000
Message-ID: <review-patch1-20260527034733.701705-1-w15303746062@163.com>
In-Reply-To: <20260527034733.701705-1-w15303746062@163.com>
References: <20260527034733.701705-1-w15303746062@163.com> <20260527034733.701705-1-w15303746062@163.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

**Positive aspects:**
- The commit message is well-written, clearly describes the bug, and includ=
es syzbot logs.
- The `Fixes:` tag and `Cc: stable` are appropriate.
- The overall approach (reject in `atomic_check`) is the right place for th=
is validation.
- The 1000 Hz threshold is reasonable for a virtual display.

**Issue 1: Wrong mode fields for frame duration calculation (bug)**

The vblank timer period is computed from `crtc_clock`/`crtc_htotal`/`crtc_v=
total` in `drm_calc_timestamping_constants()` (`drm_vblank.c:652-660`), but=
 the patch validates `htotal`/`vtotal`:

```c
if (!mode->crtc_clock || !mode->htotal || !mode->vtotal)
    return -EINVAL;
frame_ns =3D div_u64((u64)mode->htotal * mode->vtotal * 1000000ULL, mode->c=
rtc_clock);
```

This should use `mode->crtc_htotal` and `mode->crtc_vtotal` to match the ac=
tual timer computation. While the `adjusted_mode` often has `crtc_` fields =
matching non-prefixed fields, they can differ after mode fixup, and the `cr=
tc_` fields are what determine the actual timer period.

**Issue 2: `drm_mode_vrefresh()` uses `clock`, not `crtc_clock` (bug)**

`drm_mode_vrefresh()` computes refresh from `mode->clock` / `mode->htotal` =
/ `mode->vtotal`, but the timer period depends on `mode->crtc_clock` / `mod=
e->crtc_htotal` / `mode->crtc_vtotal`. A mode could have a sane `clock`/`ht=
otal`/`vtotal` but a crafted `crtc_clock`/`crtc_htotal`/`crtc_vtotal` that =
still produces a near-zero frame duration. The `drm_mode_vrefresh()` check =
alone is not sufficient =E2=80=94 the manual frame_ns check (once fixed to =
use `crtc_` fields) is the one that actually matters.

**Issue 3: Check ordering =E2=80=94 zero fields before vrefresh (minor)**

```c
vrefresh =3D drm_mode_vrefresh(mode);
if (vrefresh > 1000)
    return -EINVAL;
if (!mode->crtc_clock || !mode->htotal || !mode->vtotal)
    return -EINVAL;
```

The zero-field check should come before calling `drm_mode_vrefresh()`, not =
after. When `htotal` or `vtotal` is zero, `drm_mode_vrefresh()` returns 0 (=
safe), so this doesn't cause a bug, but it's cleaner to validate basic fiel=
d sanity before computing derived values. It also means the `drm_mode_vrefr=
esh()` call is unnecessary overhead for degenerate modes.

**Issue 4: Interlace modes not accounted for (bug)**

`drm_calc_timestamping_constants()` halves `framedur_ns` for interlaced mod=
es:

```c
if (mode->flags & DRM_MODE_FLAG_INTERLACE)
    framedur_ns /=3D 2;
```

The patch's manual calculation does not account for this:

```c
frame_ns =3D div_u64((u64)mode->htotal * mode->vtotal * 1000000ULL, mode->c=
rtc_clock);
if (frame_ns < 1000000)
    return -EINVAL;
```

An interlaced mode with a frame duration of 1.5 ms would pass this check, b=
ut after halving in `drm_calc_timestamping_constants()`, the actual timer p=
eriod would be 0.75 ms =E2=80=94 still fast enough to cause problems.

**Issue 5: Comment is verbose (style, minor)**

The 8-line block comment is longer than typical kernel style for a simple v=
alidation. A 2-3 line comment would suffice, e.g.:

```c
/* Reject modes with frame duration < 1 ms to prevent hrtimer storms. */
```

**Suggestion:** Consider computing `framedur_ns` exactly as `drm_calc_times=
tamping_constants()` does (using `crtc_` fields, accounting for interlace),=
 then rejecting if it's below the threshold. Alternatively, consider whethe=
r the validation belongs in `drm_calc_timestamping_constants()` itself or i=
n the DRM core's mode validation, which would protect all drivers using vbl=
ank timers, not just vkms.

---
Generated by Claude Code Patch Reviewer