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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Pinned folios are permanently leaked in the error path if an allocation fails before the scatterlist pointer `sg` is advanced past the first entry.
--

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

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

This commit changes the main "for" loop into a "while" loop to get rid of
the obscure iterator "i" and use a more descriptive name to indicate how
many pages were already covered.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 7c8de8fe0a225..66d0f8f6ffccb 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c

[ ... ]

> @@ -212,7 +211,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  		}
>  
>  		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 isn't a bug introduced by this patch, but looking at the error handling
in shmem_sg_alloc_table() in drivers/gpu/drm/i915/gem/i915_gem_shmem.c (around
line 228), is it possible to leak pinned folios?

If an allocation fails after successfully allocating the first folio(s) into
the first scatterlist entry, st->nents will be 1 but sg will still equal
st->sgl. The error path jumps to err_sg:

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);
	}

Since sg == st->sgl, this takes the else branch and bypasses
shmem_sg_free_table(), leaving the folios in the first scatterlist entry
pinned. Would checking if (st->nents > 0) be more appropriate here?

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