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/syncobj: Enforce strict checking of timeline syncobj struct Date: Thu, 12 Mar 2026 06:50:35 +1000 Message-ID: In-Reply-To: <20260311144524.3046352-2-dev@lankhorst.se> References: <20260311144524.3046352-2-dev@lankhorst.se> <20260311144524.3046352-2-dev@lankhorst.se> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review **Approach:** The patch makes two logical changes in each of the two ioctl = handlers (`handle_to_fd` and `fd_to_handle`): 1. **Reject `point` without `TIMELINE` flag** =E2=80=94 Previously, if user= space passed a non-zero `args->point` without the `TIMELINE` flag, the `poi= nt` local variable was simply left at 0 and the non-zero `args->point` was = silently ignored. Now it returns `-EINVAL`. This is the fix for the describ= ed userspace bug. 2. **Reject `TIMELINE` flag without sync file flag** =E2=80=94 Previously, = after the sync file export/import path, the code checked `if (args->point)`= to reject timeline usage on the non-sync-file path. Now it checks `if (arg= s->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE)` instead. This is more = correct since after change #1, `point` could legitimately be 0 with the `TI= MELINE` flag set. **Positive aspects:** - The commit message clearly explains the motivation (userspace bug with un= initialized `args->point`). - Removing the `point` local variable and using `args->point` directly simp= lifies the code. - The `Fixes:` tag is appropriate. **Concern =E2=80=94 ABI break for existing userspace:** The new check at line 870/901: ```c if (!(args->flags & DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_TIMELINE) && args->point) return -EINVAL; ``` This will **break existing userspace** that has the exact bug described in = the commit message =E2=80=94 passing garbage in `args->point` without the `= TIMELINE` flag. The old code silently ignored this; the new code fails with= `-EINVAL`. While the intent is to force userspace to fix their bugs, this = is technically a UAPI regression. Any deployed userspace with the uninitial= ized `args->point` bug that previously worked will now fail. The kernel's usual stance is "don't break userspace." The commit message ac= knowledges this was "discovered when userspace increased the size of the io= ctl to include args->point, but never cleared args->point," which suggests = a real application is affected. The question is: should the kernel reject t= his (forcing a userspace fix) or continue to be permissive (as it was befor= e)? Given that the `TIMELINE` extensions are relatively new and the affected us= erspace is presumably being fixed concurrently, this is probably acceptable= , but it would be worth confirming that the affected userspace fix is coord= inated. A `Cc: stable` tag might also be appropriate if backporting is desi= red to prevent the buggy userspace pattern from becoming more entrenched. **Minor nit =E2=80=94 TIMELINE + EXPORT_SYNC_FILE with point=3D0:** With this patch, `TIMELINE` flag + `EXPORT_SYNC_FILE` flag + `point=3D0` is= now accepted and passed through to `drm_syncobj_export_sync_file()`. Previ= ously this also worked (point would be set to 0 from `args->point`). This i= s fine =E2=80=94 timeline point 0 typically means "current fence" =E2=80=94= but worth noting that the semantics are preserved. **Overall:** The patch is correct and well-motivated. The only substantive = concern is the potential for breaking existing (buggy) userspace, which the= commit message should address more explicitly =E2=80=94 perhaps noting tha= t the broken userspace in question is being fixed in tandem, or adding a co= mment about the deliberate ABI tightening. --- Generated by Claude Code Patch Reviewer