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/i915: Fix potential overflow of shmem scatterlist length Date: Fri, 27 Feb 2026 15:21:32 +1000 Message-ID: In-Reply-To: <20260224094944.2447913-2-janusz.krzysztofik@linux.intel.com> References: <20260224094944.2447913-2-janusz.krzysztofik@linux.intel.com> <20260224094944.2447913-2-janusz.krzysztofik@linux.intel.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**: Well-written. The problem description, stack trace, Fix= es tag (`0b62af28f249b`), Closes link, and `Cc: stable` are all appropriate= . Good that it references the original `/* XXX: could overflow? */` comment. **Hunk 1 =E2=80=94 Clamping `nr_pages` at allocation (lines 153=E2=80=93142= of the diff)**: ```c - nr_pages =3D min_t(unsigned long, - folio_nr_pages(folio), page_count - i); + nr_pages =3D min_array(((unsigned long[]) { + folio_nr_pages(folio), + page_count - i, + max_segment / PAGE_SIZE, + }), 3); ``` **Style concern**: Using `min_array` with a compound literal is unusual for= kernel code. The kernel provides `min3()` (defined at `include/linux/minma= x.h:143`) which is the conventional way to take the minimum of three values= . Alternatively, chaining two `min_t()` calls would be clearer: ```c nr_pages =3D min_t(unsigned long, folio_nr_pages(folio), page_count - i); nr_pages =3D min_t(unsigned long, nr_pages, max_segment / PAGE_SIZE); ``` **Correctness concern**: Adding `max_segment / PAGE_SIZE` as a limit here m= eans that if a single folio is larger than `max_segment`, `nr_pages` will b= e clamped and `i` will advance only partway through the folio (`i +=3D nr_p= ages - 1`). The next iteration calls `shmem_read_folio_gfp(mapping, i, gfp)= ` which returns the **same folio**, but `folio_nr_pages(folio)` still retur= ns the full folio size, and `sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, = 0)` maps from **offset 0** =E2=80=94 re-mapping pages already covered. In p= ractice this is unlikely since shmem folios don't typically exceed `max_seg= ment` (~4GB), but it's a latent bug. **Hunk 2 =E2=80=94 Clamping in the coalescing branch (lines 150=E2=80=93155= of the diff)**: ```c - /* XXX: could overflow? */ + nr_pages =3D min_t(unsigned long, nr_pages, + (max_segment - sg->length) / PAGE_SIZE); + sg->length +=3D nr_pages * PAGE_SIZE; ``` **This is the core of the fix** and addresses the overflow directly. Howeve= r: **Bug introduced**: When `nr_pages` is clamped below the folio's actual rem= aining pages, the loop advances `i` by only the clamped count. The next ite= ration re-fetches the same folio (since `i` still falls within it). Then: 1. `folio_pfn(folio)` returns the **head page PFN**, not the PFN at index `= i` within the folio 2. `folio_pfn(folio) !=3D next_pfn` evaluates true =E2=86=92 new sg entry i= s started 3. `sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, 0)` maps from **offset 0*= * in the folio This means the first N pages of the folio are double-mapped (once in the ol= d sg entry, once in the new), while the remaining pages at the tail of the = folio that should have been mapped in the new entry are lost. The backing s= tore of the GEM object would have incorrect physical pages. **Concrete example**: Suppose `sg->length` is at `max_segment - 1MB`, and a= 2MB folio arrives for coalescing. `nr_pages` is clamped to 256 (1MB worth)= . The remaining 256 pages of the folio should be mapped starting at offset = 1MB within the folio, but instead they'd be mapped from offset 0, duplicati= ng the first 256 pages. **Suggested approach**: Track the offset within the folio when splitting is= needed. Something like: ```c unsigned long folio_ofs =3D i - folio->index; nr_pages =3D min_t(unsigned long, folio_nr_pages(folio) - folio_ofs, page_count - i); nr_pages =3D min_t(unsigned long, nr_pages, max_segment / PAGE_SIZE); /* ... */ sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, folio_ofs * PAGE_SIZE); ``` And in the coalescing branch, the remaining-space clamping should account f= or the fact that unclaimed folio pages need proper offset tracking in the s= ubsequent iteration. **Edge case**: If `max_segment` is not page-aligned and `sg->length` equals= `max_segment` rounded down to the nearest page, then `(max_segment - sg->l= ength) / PAGE_SIZE` could be 0, making `nr_pages =3D 0` and causing `i +=3D= nr_pages - 1` to underflow (since `i` is `unsigned long`). This is guarded= by the `sg->length >=3D max_segment` check in the preceding `if`, but only= if `max_segment` is page-aligned. In practice `i915_sg_segment_size()` ret= urns page-aligned values, so this is safe but fragile. **Summary**: The patch correctly identifies and addresses a real overflow b= ug, but the implementation introduces a folio-splitting correctness issue w= hen a folio straddles the `max_segment` boundary. I'd recommend a respin th= at properly tracks the offset within the folio. The `min_array` usage shoul= d also be replaced with `min3` or chained `min_t` calls for conventional st= yle. --- Generated by Claude Code Patch Reviewer