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: Sat, 16 May 2026 14:37:12 +1000 Message-ID: In-Reply-To: <20260511214243.349487-1-xuehaohu@google.com> References: <20260511214243.349487-1-xuehaohu@google.com> <20260511214243.349487-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 **The bug:** In `drivers/dma-buf/dma-buf-mapping.c:97-98` (pre-patch), both= `nents` and `mapped_len` were declared together as `unsigned int`. The acc= umulation at line 155: ```c mapped_len +=3D phys_vec[i].len; ``` adds `size_t` values (since `struct phys_vec` in `include/linux/types.h:173= -176` defines `len` as `size_t`) into a 32-bit variable. When total MMIO si= ze exceeds 4 GiB this wraps silently, causing the subsequent `WARN_ON_ONCE(= mapped_len !=3D size)` check at line 170 to fire, and more critically, `dma= _iova_sync()` and `fill_sg_entry()` to receive a truncated length. **The fix is correct:** ```c - unsigned int nents, mapped_len =3D 0; + unsigned int nents =3D 0; + size_t mapped_len =3D 0; ``` - `size_t` for `mapped_len` matches the types it interacts with: `phys_vec[= i].len` (`size_t`), the `size` function parameter (`size_t`), `dma_iova_syn= c()` size argument (`size_t`), `dma_iova_destroy()` size argument, and `fil= l_sg_entry()` length argument (`size_t`). - `nents` correctly remains `unsigned int`, matching the return type of `ca= lc_sg_nents()` and the `sg_alloc_table()` nents parameter. - Separating the declarations onto two lines is the right call since the ty= pes now differ. **Minor observations:** 1. **Commit message quality:** The commit message is a bit terse and could = be clearer. "total linked IVOA" appears to be a typo for "total linked IOVA= ". The description would benefit from explicitly stating that `mapped_len` = was `unsigned int` (32 bits) and is being changed to `size_t` (64 bits), an= d clarifying the specific consequence (truncated length passed to `dma_iova= _sync`/`dma_iova_destroy`). 2. **The `nents =3D 0` initialization change:** The original code had `nent= s` implicitly initialized alongside `mapped_len` in the combined declaratio= n `unsigned int nents, mapped_len =3D 0;` =E2=80=94 but note that **only** = `mapped_len` was initialized to 0 in the original code; `nents` was left un= initialized. The patch adds `=3D 0` to `nents`, which is a minor improvemen= t for safety, though `nents` is always assigned from `calc_sg_nents()` befo= re use at line 136, so the initialization is technically unnecessary. This = is fine but could be mentioned as a separate cleanup. 3. **No Cc: stable tag:** Given this is a `Fixes:` patch for a potential da= ta corruption / incorrect DMA mapping on >4 GiB MMIO BARs, a `Cc: stable@vg= er.kernel.org` tag would be appropriate if the fix should be backported. **No functional issues found.** The patch is minimal, correct, and addresse= s a real overflow bug on large MMIO regions. --- Generated by Claude Code Patch Reviewer