[PATCH v4 0/5] drivers: Improve memory management for large object allocations when i915/shmem is used with iommu

[PATCH v4 0/5] drivers: Improve memory management for large object allocations when i915/shmem is used with iommu

[Krzysztof Karas]

It was observed that allocating large objects via i915 driver
(igt-gpu-tools/tests/gem_exec_big/single) the folios and their
pages were not handled properly leading to buffer corruptions
during relocations.
Furthermore, using iommu driver in this context would leave
residual mappings in memory that could not be released, hogging
available RAM even after the process ended.

Krzysztof Karas (5):
  drm/i915/gem: Count mapped pages in a folio
  iommu/dma: Catch scatterlist length overflows
  drm/i915/gem: Pull out size validation into a separate function
  drm/i915/gem: Read and shrink memory in a separate function
  drm/i915/gem: Remove iterator and use while loop

 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 178 ++++++++++++++--------
 drivers/iommu/dma-iommu.c                 |  20 ++-
 2 files changed, 133 insertions(+), 65 deletions(-)

-- 
2.34.1

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

[Krzysztof Karas]

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;
+
 	/*
 	 * 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.
+				*/
+				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,
 				      }), 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 {
+			/*
+			 * If our prediction about folio placement is true and
+			 * scatterlist still has space left for more pages,
+			 * then we land here.
+			 */
 			nr_pages = min_t(unsigned long, nr_pages,
 					 (max_segment - sg->length) / PAGE_SIZE);
 
 			sg->length += nr_pages * PAGE_SIZE;
 		}
-		next_pfn = folio_pfn(folio) + nr_pages;
+
+		/*
+		 * We assume folios are placed one after the other in memory
+		 * and predict where the next folio begins.
+		 */
+		next_pfn = folio_pfn(folio) + folio_page_index + nr_pages;
 		i += nr_pages - 1;
 
 		/* Check that the i965g/gm workaround works. */
@@ -186,6 +216,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) {
-- 
2.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Memory leak of the scatterlist array when max_segment < PAGE_SIZE.
--

--- Patch [1]: [PATCH v4 1/5] drm/i915/gem: Count mapped pages in a folio ---
commit 3685bb29aed2da7225e7f1cb1cc9319380f378f3
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drm/i915/gem: Count mapped pages in a folio

This commit addresses scatterlist overloading by correctly handling folio
pages above the max_segment boundary, tracking the count to ensure a folio
is fully covered before reading the next.

Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/15816

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 06543ae60706e..af195db63038e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
[ ... ]
> @@ -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;
> +

[Severity: Medium]
Does this code leak the scatterlist array when max_segment < PAGE_SIZE?

Because this validation check is placed in shmem_sg_alloc_table() after
sg_alloc_table() has already successfully allocated memory for st->sgl,
returning directly bypasses the cleanup. It looks like we might need to
call sg_free_table(st) before returning -EINVAL to avoid leaking the array.

>  	/*
>  	 * Get the list of pages out of our struct file.  They'll be pinned
>  	 * at this point until we release them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723102542.3245495-1-krzysztof.karas@intel.com?part=1

[PATCH v4 2/5] iommu/dma: Catch scatterlist length overflows

[Krzysztof Karas]

It is possible, when a very large mapping uses only one
scatterlist, that padding overflows scatterlist's length field.
This results in:
 1) silently wrapping the value
 2) smaller than desired mappings produced by iommu_map_sg
 3) leaving mapped bytes in memory (no iommu_unmap)

Address this issue by adding overflow detection for scatterlist
length field.

Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
Reviewed-by: Robin Murphy <robin.murphy@arm.com>
---
v4:
 * Added value wrap check before s->length assignment (Robin);

 drivers/iommu/dma-iommu.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 381b60d9e7ce..3ca3196a41b0 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -21,6 +21,7 @@
 #include <linux/iommu-dma.h>
 #include <linux/iova.h>
 #include <linux/irq.h>
+#include <linux/limits.h>
 #include <linux/list_sort.h>
 #include <linux/memremap.h>
 #include <linux/mm.h>
@@ -1477,6 +1478,12 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 		sg_dma_len(s) = s_length;
 		s->offset -= s_iova_off;
 		s_length = iova_align(iovad, s_length + s_iova_off);
+		if ((s_length & UINT_MAX) == 0) {
+			/* Prevent possible wrapping upon assignment to s->length. */
+			ret = -EOVERFLOW;
+			goto out_restore_sg;
+		}
+
 		s->length = s_length;
 
 		/*
@@ -1493,7 +1500,18 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 		 *   time through here (i.e. before it has a meaningful value).
 		 */
 		if (pad_len && pad_len < s_length - 1) {
-			prev->length += pad_len;
+			unsigned int new_prev_len;
+			/*
+			 * For large mappings spanning multiple GBs we
+			 * may not be able to fit all needed padding into
+			 * sg->length.
+			 */
+			if (check_add_overflow(prev->length, pad_len, &new_prev_len)) {
+				ret = -EOVERFLOW;
+				goto out_restore_sg;
+			}
+
+			prev->length = new_prev_len;
 			iova_len += pad_len;
 		}
 
-- 
2.34.1

Re: [PATCH v4 2/5] iommu/dma: Catch scatterlist length overflows

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The newly added scatterlist length overflow check `(s_length & UINT_MAX) == 0` is mathematically flawed. It fails to catch integer overflow on both 32-bit and 64-bit systems, leaving the critical truncation bug unpatched, and introduces a regression by falsely rejecting legitimate zero-length mappings.
--

--- Patch [2]: [PATCH v4 2/5] iommu/dma: Catch scatterlist length overflows ---
commit 4c74567e86992a6211d4380e8e5bea4668d3e722
Author: Krzysztof Karas <krzysztof.karas@intel.com>

iommu/dma: Catch scatterlist length overflows

It is possible, when a very large mapping uses only one scatterlist,
that padding overflows scatterlist's length field. This results in
silently wrapping the value, smaller than desired mappings, and leaving
mapped bytes in memory. This commit addresses this issue by adding
overflow detection.

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 9abaec0703efb..5be5d1f3c9f82 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1477,6 +1478,12 @@ int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
>  		sg_dma_len(s) = s_length;
>  		s->offset -= s_iova_off;
>  		s_length = iova_align(iovad, s_length + s_iova_off);
> +		if ((s_length & UINT_MAX) == 0) {

[Severity: Critical]
Does this check actually catch integer overflows?

On 64-bit systems, s_length is a size_t, so this bitwise AND merely checks
if the lowest 32 bits are exactly zero. It will fail to detect an overflow
unless the aligned size is an exact multiple of 4GB.

On 32-bit systems, s_length + s_iova_off wraps before this check, so
s_length would just be a small wrapped value and bypass the check entirely.

Additionally, won't this incorrectly reject legitimate zero-length mappings?

If the overflow check is bypassed on a 64-bit system, s_length is truncated
to 32 bits when assigned to s->length below. iommu_map_sg() will then map
the truncated size while iova_len accumulates the full 64-bit size. The
partial mapping will trigger the error path later in the function:

	ret = iommu_map_sg(domain, iova, sg, nents, prot, GFP_ATOMIC);
	if (ret < 0 || ret < iova_len)
		goto out_free_iova;

	return __finalise_sg(dev, sg, nents, iova);

out_free_iova:
	iommu_dma_free_iova(domain, iova, iova_len, NULL);

This frees the IOVA back to the allocator without calling iommu_unmap(),
leaving the pages mapped and potentially causing device isolation breakdown
when the IOVA is reallocated.

> +			/* Prevent possible wrapping upon assignment to s->length. */
> +			ret = -EOVERFLOW;
> +			goto out_restore_sg;
> +		}
> +
>  		s->length = s_length;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723102542.3245495-1-krzysztof.karas@intel.com?part=2

[PATCH v4 3/5] drm/i915/gem: Pull out size validation into a separate function

[Krzysztof Karas]

shmem_sg_alloc_table is a very large and hard to read function,
so reduce the number of operations it is responsible for by
placing "size" validation ind "page_count" assignment n a new
helper.

Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
v4:
 * Changed helper function to validate size and set page_count
  (Janusz);

 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 30 +++++++++++++++--------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index af195db63038..f35b7efa1fc2 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -62,6 +62,23 @@ void shmem_sg_free_table(struct sg_table *st, struct address_space *mapping,
 	sg_free_table(st);
 }
 
+static int set_page_count_from_size(size_t size, unsigned int *page_count,
+			 struct intel_memory_region *mr)
+{
+	if (overflows_type(size / PAGE_SIZE, *page_count))
+		return -E2BIG;
+
+	/*
+	 * If there's no chance of allocating enough pages for the whole
+	 * object, bail early.
+	 */
+	if (size > resource_size(&mr->region))
+		return -ENOMEM;
+
+	*page_count = size / PAGE_SIZE;
+	return 0;
+}
+
 int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 			 size_t size, struct intel_memory_region *mr,
 			 struct address_space *mapping,
@@ -77,16 +94,9 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 	unsigned long i;
 	int ret;
 
-	if (overflows_type(size / PAGE_SIZE, page_count))
-		return -E2BIG;
-
-	page_count = size / PAGE_SIZE;
-	/*
-	 * If there's no chance of allocating enough pages for the whole
-	 * object, bail early.
-	 */
-	if (size > resource_size(&mr->region))
-		return -ENOMEM;
+	ret = set_page_count_from_size(size, &page_count, mr);
+	if (ret < 0)
+		return ret;
 
 	if (sg_alloc_table(st, page_count, GFP_KERNEL | __GFP_NOWARN))
 		return -ENOMEM;
-- 
2.34.1

Re: [PATCH v4 3/5] drm/i915/gem: Pull out size validation into a separate function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Typos in the commit message making it difficult to read.
--

--- Patch [3]: [PATCH v4 3/5] drm/i915/gem: Pull out size validation into a separate function ---
commit 1c3ac6d93ee48bc676608654e6e56d00b02a3262
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drm/i915/gem: Pull out size validation into a separate function

This commit refactors shmem_sg_alloc_table by extracting the size validation
and page count assignment into a separate helper function to improve
readability.

>     shmem_sg_alloc_table is a very large and hard to read function,
>     so reduce the number of operations it is responsible for by
>     placing "size" validation ind "page_count" assignment n a new
>     helper.

[Severity: Low]
This isn't a bug, but there are a couple of typos here ("ind" instead of "and",
and "n" instead of "in").

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723102542.3245495-1-krzysztof.karas@intel.com?part=3

[PATCH v4 4/5] drm/i915/gem: Read and shrink memory in a separate function

[Krzysztof Karas]

Continue unloading shmem_sg_alloc_table by placing reading
folios and shrink call into a new helper.
Remove the loop, since calling shrink multiple times does not
bring any benefits.

Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
v4:
 * Removed do-while loop;

 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 103 ++++++++++++----------
 1 file changed, 56 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index f35b7efa1fc2..ac8022997553 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -79,6 +79,56 @@ static int set_page_count_from_size(size_t size, unsigned int *page_count,
 	return 0;
 }
 
+static struct folio *shmem_shrink_get_folio(struct address_space *mapping,
+					    unsigned long folio_index,
+					    gfp_t gfp, unsigned int page_count,
+					    struct drm_i915_private *i915)
+{
+	struct folio *folio = NULL;
+
+	cond_resched();
+	folio = shmem_read_folio_gfp(mapping, folio_index, gfp);
+	if (IS_ERR(folio)) {
+		i915_gem_shrink(NULL, i915, 2 * page_count, NULL,
+				I915_SHRINK_BOUND | I915_SHRINK_UNBOUND);
+
+		/*
+		 * 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.
+		 *
+		 * 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;
+
+		/* Retry once after shrinking and gfp modification. */
+		cond_resched();
+		folio = shmem_read_folio_gfp(mapping, folio_index, gfp);
+	}
+
+	return folio;
+}
+
 int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 			 size_t size, struct intel_memory_region *mr,
 			 struct address_space *mapping,
@@ -119,57 +169,16 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 	for (i = 0; i < page_count; i++) {
 		unsigned long folio_page_index = 0;
 		unsigned long nr_pages;
-		const unsigned int shrink[] = {
-			I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
-			0,
-		}, *s = shrink;
 		gfp_t gfp = noreclaim;
 
 		/* 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++);
-
-				/*
-				* 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 = shmem_shrink_get_folio(mapping, i, gfp,
+						       page_count, i915);
+			if (IS_ERR(folio)) {
+				ret = PTR_ERR(folio);
+				goto err_sg;
+			}
 
 			folio_start = folio_pgoff(folio);
 			folio_end = folio_start + folio_nr_pages(folio) - 1;
-- 
2.34.1

Re: [PATCH v4 4/5] drm/i915/gem: Read and shrink memory in a separate function

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Memory leak on error path in `shmem_sg_alloc_table` when failing to allocate after populating exactly one scatterlist entry.
--

--- Patch [4]: [PATCH v4 4/5] drm/i915/gem: Read and shrink memory in a separate function ---
commit 5dd680cf1299c10f0034b5b2b67f54aa39eb638d
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drm/i915/gem: Read and shrink memory in a separate function

This commit extracts the page reading and memory shrinking logic from
shmem_sg_alloc_table() into a new helper function shmem_shrink_get_folio().
It also removes a loop, noting that multiple shrink calls do not provide
additional benefits.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index f35b7efa1fc23..ac8022997553b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c

[ ... ]

> @@ -119,57 +169,16 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  	for (i = 0; i < page_count; i++) {
>  		unsigned long folio_page_index = 0;
>  		unsigned long nr_pages;
> -		const unsigned int shrink[] = {
> -			I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
> -			0,
> -		}, *s = shrink;
>  		gfp_t gfp = noreclaim;
>  
>  		/* Grab the next folio if we exhausted the current one. */
>  		if (!i || i > folio_end) {
> -			do {

[ ... ]

> +			folio = shmem_shrink_get_folio(mapping, i, gfp,
> +						       page_count, i915);
> +			if (IS_ERR(folio)) {
> +				ret = PTR_ERR(folio);
> +				goto err_sg;
> +			}

[Severity: High]
This is a pre-existing issue, but does this error path leak memory if the
allocation fails after exactly one scatterlist entry is populated?

Looking at the error handling for shmem_sg_alloc_table in
drivers/gpu/drm/i915/gem/i915_gem_shmem.c:

err_sg:
	sg_mark_end(sg);
	if (sg != st->sgl) {
		shmem_sg_free_table(st, mapping, false, false);
	} else {
		mapping_clear_unevictable(mapping);
		sg_free_table(st);
	}

If a folio is successfully allocated and added to the first scatterlist
entry, but the loop fails on a subsequent iteration before sg is advanced
with sg_next(), sg will still equal st->sgl.

When we jump to err_sg under this condition, the code skips calling
shmem_sg_free_table and only frees the scatterlist structure via
sg_free_table. Could this bypass the reference count decrements for the
folios already added to the first scatterlist entry, causing a permanent
leak during memory exhaustion?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723102542.3245495-1-krzysztof.karas@intel.com?part=4

[PATCH v4 5/5] drm/i915/gem: Remove iterator and use while loop

[Krzysztof Karas]

Change the main "for" loop into "while" to get rid of obscure
iterator "i" and use more descriptive name to indicate how many
pages were already covered. Detect first loop with st->nents.

Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
v4:
 * Removed "else if" and reverted to having all three conditions
  inside first "if" (Janusz);

 drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index ac8022997553..f8fe5e3a4eb4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -137,11 +137,11 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 	unsigned int page_count; /* restricted by sg_alloc_table */
 	unsigned long next_pfn = 0; /* suppress gcc warning */
 	unsigned long folio_start = 0;
+	unsigned long pages_done = 0;
 	unsigned long folio_end = 0;
 	struct folio *folio = NULL;
 	struct scatterlist *sg;
 	gfp_t noreclaim;
-	unsigned long i;
 	int ret;
 
 	ret = set_page_count_from_size(size, &page_count, mr);
@@ -166,15 +166,15 @@ 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++) {
+	while (pages_done < page_count) {
 		unsigned long folio_page_index = 0;
 		unsigned long nr_pages;
 		gfp_t gfp = noreclaim;
 
 		/* Grab the next folio if we exhausted the current one. */
-		if (!i || i > folio_end) {
-			folio = shmem_shrink_get_folio(mapping, i, gfp,
-						       page_count, i915);
+		if (!pages_done || pages_done > folio_end) {
+			folio = shmem_shrink_get_folio(mapping, pages_done, gfp,
+						       page_count - pages_done, i915);
 			if (IS_ERR(folio)) {
 				ret = PTR_ERR(folio);
 				goto err_sg;
@@ -184,7 +184,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 			folio_end = folio_start + folio_nr_pages(folio) - 1;
 		}
 
-		folio_page_index = i - folio_start;
+		folio_page_index = pages_done - folio_start;
 		if (WARN_ON_ONCE(folio_page_index >= folio_nr_pages(folio))) {
 			ret = -EINVAL;
 			folio_put(folio);
@@ -193,14 +193,14 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 
 		nr_pages = min_array(((unsigned long[]) {
 					folio_nr_pages(folio) - folio_page_index,
-					page_count - i,
+					page_count - pages_done,
 					max_segment / PAGE_SIZE,
 				      }), 3);
 
-		if (!i ||
+		if (!st->nents ||
 		    sg->length >= max_segment ||
 		    folio_pfn(folio) + folio_page_index != next_pfn) {
-			if (i)
+			if (st->nents)
 				sg = sg_next(sg);
 
 			st->nents++;
@@ -223,7 +223,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
 		 * and predict where the next folio begins.
 		 */
 		next_pfn = folio_pfn(folio) + folio_page_index + nr_pages;
-		i += nr_pages - 1;
+		pages_done += nr_pages;
 
 		/* Check that the i965g/gm workaround works. */
 		GEM_BUG_ON(gfp & __GFP_DMA32 && next_pfn >= 0x00100000UL);
-- 
2.34.1

Re: [PATCH v4 5/5] drm/i915/gem: Remove iterator and use while loop

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Flawed scatterlist length overflow check bypasses overflow protection, causing IOVA truncation and IOMMU page table corruption.

Pre-existing issues:
- [High] Pre-existing memory leak in `shmem_sg_alloc_table` error path when scatterlist allocation fails after adding one or more folios to the first segment.
--

--- Patch [5]: [PATCH v4 5/5] drm/i915/gem: Remove iterator and use while loop ---
commit 08123ce95aacd0e0eea7518e3cd7a5c53bd75933
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drm/i915/gem: Remove iterator and use while loop

This commit refactors the main loop in shmem_sg_alloc_table to use a while
loop instead of a for loop. It replaces the iterator with a variable tracking
the completed pages.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index ac8022997553b..f8fe5e3a4eb40 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
[ ... ]
> @@ -223,7 +223,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  		 * and predict where the next folio begins.
>  		 */
>  		next_pfn = folio_pfn(folio) + folio_page_index + nr_pages;
> -		i += nr_pages - 1;
> +		pages_done += nr_pages;
>  
>  		/* Check that the i965g/gm workaround works. */
>  		GEM_BUG_ON(gfp & __GFP_DMA32 && next_pfn >= 0x00100000UL);

[Severity: High]
This is a pre-existing issue, but does this error path leak folios if we fail
while sg == st->sgl?

Looking at shmem_sg_alloc_table() in drivers/gpu/drm/i915/gem/i915_gem_shmem.c:

err_sg:
    sg_mark_end(sg);
    if (sg != st->sgl) {
        shmem_sg_free_table(st, mapping, false, false);
    } else {
        mapping_clear_unevictable(mapping);
        sg_free_table(st);
    }

If the scatterlist allocation fails after adding one or more folios to the
first segment, we jump to err_sg. At this point, no second scatterlist
segment was created, so sg still points to st->sgl.

The condition sg != st->sgl evaluates to false, causing us to skip
shmem_sg_free_table(). Does this mean the folios placed in the first segment
are never released, resulting in a memory leak under memory pressure?


[Severity: High]
Can the overflow check be bypassed in iommu_dma_map_sg() within
drivers/iommu/dma-iommu.c?

iommu_dma_map_sg() {
    ...
    if ((s_length & UINT_MAX) == 0) {
        /* Prevent possible wrapping upon assignment to s->length. */
        ret = -EOVERFLOW;
        goto out_restore_sg;
    }
    ...
}

If a caller maps a buffer larger than 4GB where the alignment calculation
causes s_length + s_iova_off to exceed UINT_MAX, but the lower 32 bits of
the aligned size are non-zero (for example, 0x100010000 with 64KB granules),
the bitwise check evaluates to false.

Does this bypass the intended overflow detection, causing iommu_map_sg() to
map a truncated number of bytes and potentially leading to unmapped IOVAs
remaining accessible via DMA?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723102542.3245495-1-krzysztof.karas@intel.com?part=5

✓ i915.CI.BAT: success for drivers: Improve memory management for large object allocations when i915/shmem is used with iommu

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

== Series Details ==

Series: drivers: Improve memory management for large object allocations when i915/shmem is used with iommu
URL   : https://patchwork.freedesktop.org/series/170988/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_18885 -> Patchwork_170988v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_170988v1/index.html

Participating hosts (41 -> 39)
------------------------------

  Missing    (2): bat-dg2-13 fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_170988v1 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@i915_selftest@live:
    - fi-bsw-nick:        [INCOMPLETE][1] -> [PASS][2] +1 other test pass
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18885/fi-bsw-nick/igt@i915_selftest@live.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_170988v1/fi-bsw-nick/igt@i915_selftest@live.html

  * igt@kms_flip@basic-flip-vs-wf_vblank:
    - fi-bsw-n3050:       [DMESG-WARN][3] ([i915#16057]) -> [PASS][4] +1 other test pass
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18885/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_170988v1/fi-bsw-n3050/igt@kms_flip@basic-flip-vs-wf_vblank.html

  
  [i915#16057]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/16057


Build changes
-------------

  * Linux: CI_DRM_18885 -> Patchwork_170988v1

  CI-20190529: 20190529
  CI_DRM_18885: 2c64192f1ad194a53800f6dd8c779d5b4bbc1831 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_9020: 9020
  Patchwork_170988v1: 2c64192f1ad194a53800f6dd8c779d5b4bbc1831 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_170988v1/index.html

[-- Attachment #2: Type: text/html, Size: 2567 bytes --]