From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Re: [PATCH] drm/syncobj: Fix handle <-> fd ioctls with dirty stack Date: Tue, 03 Mar 2026 13:58:23 +1000 Message-ID: In-Reply-To: <93f2c5b5-4f5e-46f1-a7f3-c388ab762592@amd.com> References: <93f2c5b5-4f5e-46f1-a7f3-c388ab762592@amd.com> <93f2c5b5-4f5e-46f1-a7f3-c388ab762592@amd.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 **Analysis of the fix**: The code in both handlers follows the same pattern: ```c u64 point =3D 0; // ... if (args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) point =3D args->point; // ... export_sync_file early return ... if (args->point) // <-- BUG: checks raw field even when TIMELINE not set return -EINVAL; return drm_syncobj_handle_to_fd(file_private, args->handle, &args->fd); ``` The local variable `point` already captures the correct semantic intent: it= 's `0` unless the `TIMELINE` flag is set, in which case it copies `args->po= int`. The change from `if (args->point)` to `if (point)` means: - **TIMELINE not set**: `point` is always `0`, check passes regardless of s= tack garbage in `args->point`. This is correct =E2=80=94 when TIMELINE is n= ot requested, the `point` field is meaningless and should be ignored. - **TIMELINE set**: `point =3D=3D args->point`, so the check is equivalent = to the old behavior. No security concern arises because the `point` value is not used in the sub= sequent `drm_syncobj_handle_to_fd()` / `drm_syncobj_fd_to_handle()` calls. **Comments**: 1. **Correct and minimal fix**. The change is the smallest possible fix tha= t addresses the regression. Using the already-existing local `point` variab= le is elegant =E2=80=94 no new logic is needed. 2. **Commit message is well-written**. The example program clearly demonstr= ates the regression, the explanation of why the "correct" fix (`if (args->f= lags & TIMELINE) return -EINVAL`) can't be used is sound =E2=80=94 it could= break userspace that passes TIMELINE + point=3D=3D0 and expects success. 3. **Missing `Cc: stable`**. Given this is a regression fix with a `Fixes:`= tag, it should include `Cc: stable@vger.kernel.org` to ensure it gets back= ported to stable kernels that carry commit c2d3a7300695. 4. **Both ioctls fixed symmetrically**. The fix correctly applies to both `= drm_syncobj_handle_to_fd_ioctl` and `drm_syncobj_fd_to_handle_ioctl`, which= have the same pattern and the same bug. Good. 5. **Minor observation on the commit message**: The comment says "userspace= is required to set point to 0" but with this fix, userspace is no longer r= equired to set `point` to 0 when not using TIMELINE =E2=80=94 that's the wh= ole point of the fix. The commit message is clear about this being the inte= nt, so this is just an observation, not a problem. Overall: **Reviewed-by worthy**. Clean, correct, minimal regression fix. --- Generated by Claude Code Patch Reviewer