Claude review: drm/i915: Fix potential overflow of shmem scatterlist length

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Commit message**: Well-written. The problem description, stack trace, Fixes tag (`0b62af28f249b`), Closes link, and `Cc: stable` are all appropriate. Good that it references the original `/* XXX: could overflow? */` comment.

**Hunk 1 — Clamping `nr_pages` at allocation (lines 153–142 of the diff)**:

```c
-		nr_pages = min_t(unsigned long,
-				folio_nr_pages(folio), page_count - i);
+		nr_pages = 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/minmax.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 = min_t(unsigned long, folio_nr_pages(folio), page_count - i);
nr_pages = min_t(unsigned long, nr_pages, max_segment / PAGE_SIZE);
```

**Correctness concern**: Adding `max_segment / PAGE_SIZE` as a limit here means that if a single folio is larger than `max_segment`, `nr_pages` will be clamped and `i` will advance only partway through the folio (`i += nr_pages - 1`). The next iteration calls `shmem_read_folio_gfp(mapping, i, gfp)` which returns the **same folio**, but `folio_nr_pages(folio)` still returns the full folio size, and `sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, 0)` maps from **offset 0** — re-mapping pages already covered. In practice this is unlikely since shmem folios don't typically exceed `max_segment` (~4GB), but it's a latent bug.

**Hunk 2 — Clamping in the coalescing branch (lines 150–155 of the diff)**:

```c
-			/* XXX: could overflow? */
+			nr_pages = min_t(unsigned long, nr_pages,
+					 (max_segment - sg->length) / PAGE_SIZE);
+
 			sg->length += nr_pages * PAGE_SIZE;
```

**This is the core of the fix** and addresses the overflow directly. However:

**Bug introduced**: When `nr_pages` is clamped below the folio's actual remaining pages, the loop advances `i` by only the clamped count. The next iteration 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) != next_pfn` evaluates true → new sg entry is 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 old 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 store 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, duplicating the first 256 pages.

**Suggested approach**: Track the offset within the folio when splitting is needed. Something like:

```c
unsigned long folio_ofs = i - folio->index;

nr_pages = min_t(unsigned long,
                 folio_nr_pages(folio) - folio_ofs, page_count - i);
nr_pages = 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 for the fact that unclaimed folio pages need proper offset tracking in the subsequent 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->length) / PAGE_SIZE` could be 0, making `nr_pages = 0` and causing `i += nr_pages - 1` to underflow (since `i` is `unsigned long`). This is guarded by the `sg->length >= max_segment` check in the preceding `if`, but only if `max_segment` is page-aligned. In practice `i915_sg_segment_size()` returns page-aligned values, so this is safe but fragile.

**Summary**: The patch correctly identifies and addresses a real overflow bug, but the implementation introduces a folio-splitting correctness issue when a folio straddles the `max_segment` boundary. I'd recommend a respin that properly tracks the offset within the folio. The `min_array` usage should also be replaced with `min3` or chained `min_t` calls for conventional style.

---
Generated by Claude Code Patch Reviewer