Hi Janusz,
On 2026-07-15 at 13:18:21 +0200, Janusz Krzysztofik wrote:
> 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 ...
Hmm, very well, sounds reasonable.
>
> > 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]>
> > ---
[...]
> > + 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.
I do not recall why exactly I added that check here. I think
validating max_segment earlier is more visible, so I'll do as
you suggest.
>
> > }), 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.
Yup, got it.
>
> >
> > 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.
I think what we are missing here is the assumption that folios
are placed one after another, on which we base next_pfn
calculations. If that assumption is wrong and next_pfn is not
the address of the next folio, then we land in the "if" instead
of "else" block.
In any case, I'll add this info in the next version, either via
modifying the if conditions or adding a comment here.
Thansk for looking at this!
--
Best Regards,
Krzysztof
