From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Revert "accel/amdxdna: Support read-only user-pointer BO mappings" Date: Mon, 25 May 2026 19:46:43 +1000 Message-ID: In-Reply-To: <20260521162930.1451042-1-lizhi.hou@amd.com> References: <20260521162930.1451042-1-lizhi.hou@amd.com> <20260521162930.1451042-1-lizhi.hou@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 **Commit message issues:** The message "The read-only feature requires further consideration" is insuf= ficiently detailed for a revert. It should explain the specific problems. T= he original `readonly_va_entry()` function had at least two notable issues: 1. **Race condition / TOCTOU:** The function checks VMA flags (`vm_flags & = VM_WRITE`) under `mmap_read_lock`, but by the time `pin_user_pages_fast()` = is called (which does its own VMA lookup internally), the VMA could have ch= anged =E2=80=94 it could have been split, unmapped, or had its permissions = changed via `mprotect()`. This is a classic time-of-check-to-time-of-use ra= ce. The `pin_user_pages_fast` GUP path already handles write permission che= cking internally when `FOLL_WRITE` is (or isn't) passed, so duplicating tha= t logic at a different point in time is fragile. 2. **Spanning multiple VMAs:** The check `vma->vm_end - va_ent->vaddr < va_= ent->len` assumes the entire range is covered by a single VMA, but a user-p= ointer range could span multiple VMAs with different permissions. `find_vma= ()` only returns the first one. This means the read-only detection could be= incorrect for multi-VMA ranges. These would be good things to mention in the commit message. **Code changes =E2=80=94 correctness:** The diff itself is a clean revert: - Removes `readonly_va_entry()` entirely =E2=80=94 correct. - Removes `bool readonly =3D true;` declaration =E2=80=94 correct. - Removes the `readonly_va_entry()` call in the validation loop =E2=80=94 c= orrect. - Changes `(readonly ? 0 : FOLL_WRITE) | FOLL_LONGTERM` to `FOLL_WRITE | FO= LL_LONGTERM` =E2=80=94 correct, always pins as writable. - Changes `(readonly ? O_RDONLY : O_RDWR) | O_CLOEXEC` to `O_RDWR | O_CLOEX= EC` =E2=80=94 correct, always exports as read-write. The post-revert state matches what existed before `f649e63d4a64` was applie= d. **Minor nit:** The revert message doesn't include a `Cc: stable@` tag. If t= he original commit went into a released kernel and the TOCTOU race is consi= dered a security-relevant bug, a stable backport annotation might be approp= riate. But this is at the maintainer's discretion. **Recommendation:** Request an improved commit message that explains the sp= ecific technical concerns motivating the revert (TOCTOU race, single-VMA as= sumption, or whatever the actual driver is). The code change itself is corr= ect and can be accepted as-is once the message is improved. --- Generated by Claude Code Patch Reviewer