From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: dma-buf: Fix silent overflow for phys vec to sgt Date: Thu, 28 May 2026 12:05:56 +1000 Message-ID: In-Reply-To: <20260527205048.2168808-1-xuehaohu@google.com> References: <20260527205048.2168808-1-xuehaohu@google.com> <20260527205048.2168808-1-xuehaohu@google.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 nit:** There's a dangling fragment in the commit message b= ody: ``` to mapped_len, and leading to a silent overflow ``` This line (line 103 of the mbox) looks like leftover text from an earlier d= raft that wasn't cleaned up. It should be removed before merging. **`mapped_len` promotion (main fix) =E2=80=94 correct:** ```c - unsigned int nents, mapped_len =3D 0; ... + size_t mapped_len =3D 0; + unsigned int nents; ``` This is the core fix. `mapped_len` accumulates `phys_vec[i].len` values (wh= ich are `size_t`) at line 154 of the source: ```c mapped_len +=3D phys_vec[i].len; ``` With >4GB total MMIO, the old `unsigned int` wraps silently. Then `dma_iova= _sync()` and `fill_sg_entry()` receive a truncated length, which is a data-= corruption-grade bug. The fix is correct and minimal. **`fill_sg_entry` loop iterator =E2=80=94 correct but cosmetic:** ```c - int i; + unsigned int i; ``` The loop compares `i < nents` where `nents` is `unsigned int`. Using a sign= ed `int` for the iterator means the comparison is well-defined (the signed = value gets implicitly converted), so this is not a functional bug, but matc= hing signedness is cleaner and avoids compiler warnings with `-Wsign-compar= e`. Fine to include. **`calc_sg_nents` overflow guard =E2=80=94 needs attention:** ```c - unsigned int nents =3D 0; + size_t nents =3D 0; ... + if (nents > UINT_MAX) + return 0; ``` The function signature still returns `unsigned int`: ```c static unsigned int calc_sg_nents(struct dma_iova_state *state, ...) ``` Promoting `nents` to `size_t` locally and clamping to `UINT_MAX` before ret= urning is logically correct =E2=80=94 it prevents the intermediate accumula= tion from overflowing before the check. However, there are concerns: 1. **Returning 0 on overflow is a silent failure.** The caller at line 135-= 137 does: ```c nents =3D calc_sg_nents(dma->state, phys_vec, nr_ranges, size); ret =3D sg_alloc_table(&dma->sgt, nents, GFP_KERNEL | __GFP_ZERO); ``` Passing `nents =3D 0` to `sg_alloc_table()` =E2=80=94 which takes `unsig= ned int nents` =E2=80=94 will cause `sg_alloc_table` to fail (it returns `-= EINVAL` for 0 entries per the kernel implementation). So the error *will* p= ropagate, but through `sg_alloc_table` failing rather than `calc_sg_nents` = explicitly signaling the problem. This works but is fragile =E2=80=94 it re= lies on `sg_alloc_table`'s behavior for a degenerate input rather than the = caller explicitly handling the overflow. A `WARN_ON_ONCE` or explicit error= return (e.g., returning `UINT_MAX` with a warning, or having the caller ch= eck for 0) would make the failure mode more obvious and debuggable. 2. **The non-IOVA path can also overflow.** In the loop: ```c for (i =3D 0; i < nr_ranges; i++) nents +=3D DIV_ROUND_UP(phys_vec[i].len, UINT_MAX); ``` Each `phys_vec[i].len` produces at least 1 entry, and `nr_ranges` is `si= ze_t`. If `nr_ranges` is extremely large (though unlikely in practice for M= MIO regions), `nents` could still overflow even as `size_t` on 64-bit. The = `> UINT_MAX` check after the loop catches this, so it's fine. But the IOVA = path with `DIV_ROUND_UP(size, UINT_MAX)` will never exceed `UINT_MAX` on 64= -bit since `size` is `size_t` (at most 2^64-1 / (2^32-1) =E2=89=88 2^32), s= o the check is mostly relevant for the non-IOVA accumulation path. **Reverse Christmas tree ordering =E2=80=94 correct:** The variable declarations in `dma_buf_phys_vec_to_sgt` are properly reorder= ed by line length: ```c struct dma_buf_dma *dma; struct scatterlist *sgl; size_t mapped_len =3D 0; unsigned int nents; dma_addr_t addr; size_t i; int ret; ``` This follows the kernel coding style convention. **Summary:** The core `mapped_len` overflow fix is correct and important. T= he `calc_sg_nents` overflow guard works but returning 0 silently is not ide= al =E2=80=94 consider adding a `WARN_ON_ONCE(nents > UINT_MAX)` before the = return so the failure is visible in dmesg rather than silently propagating = through `sg_alloc_table`. The dangling text fragment in the commit message = ("to mapped_len, and leading to a silent overflow") must be removed. --- Generated by Claude Code Patch Reviewer