Re: [PATCH v4 1/5] drm/i915/gem: Count mapped pages in a folio

[Sebastian Brzezinka <sebastian.brzezinka@intel.com>]

Hi Krzysztof,

On Thu Jul 23, 2026 at 12:25 PM CEST, Krzysztof Karas wrote:
> Before addition of commit 029ae067431a
> ("drm/i915: Fix potential overflow of shmem scatterlist length")
> and after folios were introduced complete folios were always
> allocated, possibly overloading the scatterlist capacity which
> was never truly limited to PAGE_SIZE when requested via
> max_segment. The above commit addressed scatterlist overloading,
> but unintentionally disabled PAGE_SIZE as a valid max_segment
> value and failed to take care of remaining pages from folios
> above max_segment boundary.
>
> This created a state, where multitude of scatterlists were used
> for the same folio, but never counting enough of its pages to
> jump to the next 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 <krzysztof.karas@intel.com>
> ---
> v4:
>  * Rewrote commit message to be more explicit about the problem
>   (Janusz);
>  * Moved max_segment validation to the beginning of the function
>   (Janusz);
>
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 123 ++++++++++++++--------
>  1 file changed, 77 insertions(+), 46 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..af195db63038 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))
> @@ -88,6 +91,9 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  	if (sg_alloc_table(st, page_count, GFP_KERNEL | __GFP_NOWARN))
>  		return -ENOMEM;
>  
> +	if (max_segment < PAGE_SIZE)
> +		return -EINVAL;
> +
Move this validation before sg_alloc_table,right now you're leaking st.

>  	/*
>  	 * Get the list of pages out of our struct file.  They'll be pinned
>  	 * at this point until we release them.
> @@ -101,7 +107,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 +115,95 @@ 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;
> +		/* 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;
> -			}
> +				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);
> +				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.
> +				*/
Looks like this comments are missing space in aligment.

-- 
Best regards,
Sebastian