Hi Sebastian,
On Thu, 2026-05-28 at 15:55 +0200, Sebastian Brzezinka wrote:
> Hi Krzysztof,
>
> On Wed May 27, 2026 at 4:08 PM CEST, 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 [1], were not enough to
> > jump to the next folio. In result, sg_set_folio() was called
> > multiple times with nr_pages smaller than folio size, using
> > many scatterlists, all pointing to the beginning pages of the
> > folio and never fully covering its range of pages and corrupting
> > mappings.
> >
> > [1] See shmem_get_pages(), where segment size is set to
> > PAGE_SIZE.
> >
> > Suggested-by: Janusz Krzysztofik <[email protected]>
> > 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]>
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > index 06543ae60706..ac9b263c341a 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > @@ -156,7 +156,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> > struct sg_table *st,
> > nr_pages = min_array(((unsigned long[]) {
> > folio_nr_pages(folio),
> > page_count - i,
> > - max_segment / PAGE_SIZE,
> > + i915_sg_segment_size(i915->drm.dev) /
> > PAGE_SIZE,
> I don't think we can use i915_sg_segment_size() here, please correct me
> if I'm wrong.
Looking at this again, I think you are right to some extent, and
unfortunately my suggestion to use i915_sg_segment_size() here wasn't based
on sufficiently deep analysis of the issue Krzysztof was trying to address.
While my commit 029ae067431a ("drm/i915: Fix potential overflow of shmem
scatterlist length") suggested that clamping nr_pages also to max_segment
was needed only to avoid the overflow, in fact it also changed the way
pages were allocated when current scatterlist length exceeded max_segment
and a new scatterlist was allocated. Assuming the intention of calling
shmem_sg_alloc_table() again with max_segment == PAGE_SIZE once it failed
when called with max_segment == i915_sg_segment_size() was to allocate
pages one per scatterlist, that rule was never followed after commit
0b62af28f249b ("i915: convert shmem_sg_free_table() to use a folio_batch")
and newly allocated scatterlists were still populated with nr_pages not
clamped to max_segment == PAGE_SIZE. With my change addressing the
potential overflow, number of pages allocated to a single scatterlist is
now always clamped to max_segment, and one page per scatterlist mode should
now work as designed.
That said, I think we need to revisit the issue identified by Krzysztof and
try to find its root cause again. Maybe we should also revisit either the
idea or implementation of allocating one page per scatterlist in case when
allocating larger chunks failed, because that seems to be strictly related
to what Krzysztof describes as problematic in his commit description ("...
sg_set_folio() was called multiple times with nr_pages smaller than folio
size, using many scatterlists, all pointing to the beginning pages of the
folio and never fully covering its range of pages ...").
Thanks,
Janusz
>
> When DMA mapping fails, shmem_get_pages() sets max_segment = PAGE_SIZE
> and retries. If we replace max_segment with i915_sg_segment_size() in the
> min_array, nothing really changes between the first and second attempt,
> we end up computing the same nr_pages as before and building the same
> sg table that just failed to map.
>
> It also breaks the sg->length check on line 163. After the first folio,
> sg->length equals i915_sg_segment_size(). With max_segment=PAGE_SIZE the
> check sg->length >= max_segment is always true, never and the PAGE_SIZE
> retry has no effect.
