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.
Furthermore, current do-while loop goes over the same folio
multiple times, increasing the refcount each time, and breaks
get/put symmetry: many shmem_read_folio_gfp() gets and only one
put in i915_gem_object_put_pages_shmem() per 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")
Cc: [email protected]
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/15816
Assisted-by: GitHub Copilot:claude-opus-5
Signed-off-by: Krzysztof Karas <[email protected]>
---
v6:
* moved max_segment check into its own patch;
* moved comment about gcc warnings above block of
initializations;
* removed
WARN_ON_ONCE(folio_page_index >= folio_nr_pages(folio))
block (Andi);
* restored original condition (folio_pfn(folio) != next_pfn)
(Janusz);
* dropped folio_page_index and folio initializations;
* expanded the commit message to contain information about
previously unexplained folio reference leaks;
* ran final checks with Claude Opus and added a tag.
drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 112 +++++++++++++---------
1 file changed, 67 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
index 4b5ce9a2f74f..8e5f2a7d5f6b 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
@@ -70,7 +70,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 i;
struct scatterlist *sg;
- unsigned long next_pfn = 0; /* suppress gcc warning */
+ /* suppress gcc warnings */
+ unsigned long next_pfn = 0;
+ unsigned long folio_start = 0;
+ unsigned long folio_end = 0;
+ struct folio *folio;
gfp_t noreclaim;
int ret;
@@ -104,7 +108,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;
unsigned long nr_pages;
const unsigned int shrink[] = {
I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
@@ -112,51 +116,58 @@ 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);
-
- nr_pages = min3(folio_nr_pages(folio),
+ * 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;
+ nr_pages = min3(folio_nr_pages(folio) - folio_page_index,
page_count - i, max_segment / PAGE_SIZE);
if (!i ||
@@ -166,13 +177,24 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
struct sg_table *st,
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(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. */
--
2.34.1
