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

[sashiko-bot@kernel.org]

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