Hi Krzysztof,
Since reading your commit description I was not really able to understand
what the issue you were trying to fix was about, I'm providing some
comments based on my understanding of the issue.
On Mon, 2026-07-13 at 09:58 +0000, Krzysztof Karas wrote:
> With addition of commit 029ae067431a
> ("drm/i915: Fix potential overflow of shmem scatterlist length")
> max_segment size was included in calculating a number of pages
> for the scatterlist. This meant that segment sizes considerably
> smaller than number of pages in a folio (see shmem_get_pages(),
> rebuild_st label for context),
Two values of max_segment only could and still can be expected, either
maximum capacity of a scatterlist, or PAGE_SIZE. Before the blamed
commit, that argument was mostly (unintentionally?) not respected when
allocating pages from a folio to a scatterlist. IOW, complete folios were
always allocated, possibly overloading the scatterlist capacity,
virtually never limited to PAGE_SIZE when requested via max_segment.
The blamed commit took care of scatterlist overloading, and
unintentionally also of respecting the max_segment == PAGE_SIZE case, by
limiting the number of folio pages allocated to a single scatterlist not
to exceed max_segment, but failed to take care of correctly allocating
remaining pages from folios larger than max_segment. That's what this
commit description should tell us about, I believe.
> were not enough to jump to the
> next folio, which has never been a problem before folios have
> been intoduced. In result, sg_set_folio() was called multiple
> times with nr_pages smaller than folio size, using multitude of
> scatterlists,
I don't think it matters how many times sg_set_folio() was called or how
many scatterlists were used, since we may expect them to be huge when
shmem_sg_alloc_table() is called with max_segment == PAGE_SIZE.
> all pointing to the beginning pages of the folio
> and never fully covering its range of pages.
Yeah, that's the real issue. An offset within a folio should be tracked
and used when allocating pages still left in the folio.
> Track how many pages have already been counted in a folio
... and use that number as an offset on consecutive allocations from the
same folio ...
> to ensure it is fully covered before reading next folio.
> Fixes: 029ae067431a ("drm/i915: Fix potential overflow of shmem scatterlist
> length")
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/15816
> Signed-off-by: Krzysztof Karas <[email protected]>
> ---
> v3:
> * Fixed a bug that caused first folio to never be considered.
>
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 120 +++++++++++++---------
> 1 file changed, 70 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 06543ae60706..0011d76f5b8c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -68,10 +68,13 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> struct sg_table *st,
> unsigned int max_segment)
> {
> unsigned int page_count; /* restricted by sg_alloc_table */
> - unsigned long i;
> + unsigned long next_pfn = 0; /* suppress gcc warning */
> + unsigned long folio_start = 0;
> + unsigned long folio_end = 0;
> + struct folio *folio = NULL;
> struct scatterlist *sg;
> - unsigned long next_pfn = 0; /* suppress gcc warning */
> gfp_t noreclaim;
> + unsigned long i;
> int ret;
>
> if (overflows_type(size / PAGE_SIZE, page_count))
> @@ -101,7 +104,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> struct sg_table *st,
> sg = st->sgl;
> st->nents = 0;
> for (i = 0; i < page_count; i++) {
> - struct folio *folio;
> + unsigned long folio_page_index = 0;
> unsigned long nr_pages;
> const unsigned int shrink[] = {
> I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
> @@ -109,71 +112,87 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> struct sg_table *st,
> }, *s = shrink;
> gfp_t gfp = noreclaim;
>
> - do {
> - cond_resched();
> - folio = shmem_read_folio_gfp(mapping, i, gfp);
> - if (!IS_ERR(folio))
> - break;
> -
> - if (!*s) {
> - ret = PTR_ERR(folio);
> - goto err_sg;
> - }
> -
> - i915_gem_shrink(NULL, i915, 2 * page_count, NULL, *s++);
> -
> - /*
> - * We've tried hard to allocate the memory by reaping
> - * our own buffer, now let the real VM do its job and
> - * go down in flames if truly OOM.
> - *
> - * However, since graphics tend to be disposable,
> - * defer the oom here by reporting the ENOMEM back
> - * to userspace.
> - */
> - if (!*s) {
> - /* reclaim and warn, but no oom */
> - gfp = mapping_gfp_mask(mapping);
> + /* Grab the next folio if we exhausted the current one. */
> + if (!i || i > folio_end) {
> + do {
> + cond_resched();
> + folio = shmem_read_folio_gfp(mapping, i, gfp);
> + if (!IS_ERR(folio))
> + break;
> +
> + if (!*s) {
> + ret = PTR_ERR(folio);
> + goto err_sg;
> + }
> +
> + i915_gem_shrink(NULL, i915, 2 * page_count,
> NULL, *s++);
>
> /*
> - * Our bo are always dirty and so we require
> - * kswapd to reclaim our pages (direct reclaim
> - * does not effectively begin pageout of our
> - * buffers on its own). However, direct reclaim
> - * only waits for kswapd when under allocation
> - * congestion. So as a result __GFP_RECLAIM is
> - * unreliable and fails to actually reclaim our
> - * dirty pages -- unless you try over and over
> - * again with !__GFP_NORETRY. However, we still
> - * want to fail this allocation rather than
> - * trigger the out-of-memory killer and for
> - * this we want __GFP_RETRY_MAYFAIL.
> - */
> - gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> - }
> - } while (1);
> + * We've tried hard to allocate the memory by
> reaping
> + * our own buffer, now let the real VM do its
> job and
> + * go down in flames if truly OOM.
> + *
> + * However, since graphics tend to be disposable,
> + * defer the oom here by reporting the ENOMEM
> back
> + * to userspace.
> + */
> + if (!*s) {
> + /* reclaim and warn, but no oom */
> + gfp = mapping_gfp_mask(mapping);
> +
> + /*
> + * Our bo are always dirty and so we
> require
> + * kswapd to reclaim our pages (direct
> reclaim
> + * does not effectively begin pageout
> of our
> + * buffers on its own). However, direct
> reclaim
> + * only waits for kswapd when under
> allocation
> + * congestion. So as a result
> __GFP_RECLAIM is
> + * unreliable and fails to actually
> reclaim our
> + * dirty pages -- unless you try over
> and over
> + * again with !__GFP_NORETRY. However,
> we still
> + * want to fail this allocation rather
> than
> + * trigger the out-of-memory killer and
> for
> + * this we want __GFP_RETRY_MAYFAIL.
> + */
> + gfp |= __GFP_RETRY_MAYFAIL |
> __GFP_NOWARN;
> + }
> + } while (1);
> +
> + folio_start = folio_pgoff(folio);
> + folio_end = folio_start + folio_nr_pages(folio) - 1;
> + }
> +
> + folio_page_index = i - folio_start;
> + if (WARN_ON_ONCE(folio_page_index >= folio_nr_pages(folio))) {
> + ret = -EINVAL;
> + folio_put(folio);
> + goto err_sg;
> + }
>
> nr_pages = min_array(((unsigned long[]) {
> - folio_nr_pages(folio),
> + folio_nr_pages(folio) -
> folio_page_index,
> page_count - i,
> - max_segment / PAGE_SIZE,
> + max_t(unsigned int, 1, max_segment /
> PAGE_SIZE),
Why? This function is never called with max_segment less than PAGE_SIZE,
then max_segment / PAGE_SIZE can't drop to 0. If you want to address the
unexpected 0 case then I think we should just validate max_segment before
using it.
> }), 3);
>
> if (!i ||
> sg->length >= max_segment ||
> - folio_pfn(folio) != next_pfn) {
> + folio_pfn(folio) + folio_page_index != next_pfn) {
> if (i)
> sg = sg_next(sg);
>
> st->nents++;
> - sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, 0);
> + sg_set_page(sg, folio_page(folio, folio_page_index),
> + nr_pages * PAGE_SIZE, 0);
> } else {
> nr_pages = min_t(unsigned long, nr_pages,
> - (max_segment - sg->length) /
> PAGE_SIZE);
> + max_t(unsigned long, 1,
> + (max_segment - sg->length) /
> PAGE_SIZE));
The same as above, we may exepect max_segment not less than PAGE_SIZE.
>
> sg->length += nr_pages * PAGE_SIZE;
The only case for appending more pages to a not yet full scatterlist I can
imagine is when first page of next folio is next to the last page of a
previous folio assigned here before so contiguity of pages can be
preserved. Then, I think the above if conditions should better reflect
that only case for clarity.
Thanks,
Janusz
> }
> - next_pfn = folio_pfn(folio) + nr_pages;
> +
> + next_pfn = folio_pfn(folio) + folio_page_index + nr_pages;
> i += nr_pages - 1;
>
> /* Check that the i965g/gm workaround works. */
> @@ -186,6 +205,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> struct sg_table *st,
> i915_sg_trim(st);
>
> return 0;
> +
> err_sg:
> sg_mark_end(sg);
> if (sg != st->sgl) {
