Re: [PATCH v2] drm/i915/selftests: Fix unlocked mm.stolen in reset selftest

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915/selftests: Hardware-reserved stolen memory is poisoned, causing crash
- [Low] drm/i915/selftests: TOCTOU race in GPU reset stolen memory test
--

commit 855f7a70fdf31c1c3dd9401d2d9ed883c5184a38
Author: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
Subject: drm/i915/selftests: Fix unlocked mm.stolen in reset selftest

This patch extends the stolen_lock coverage in the reset selftest to include
the poison and verify writes, not just the lookup. It also introduces a
stolen_page_unused() helper to perform the lookup.

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

> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index 2cfc23c58e909..77af788314ffd 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
[ ... ]
> @@ -95,10 +105,10 @@ __igt_reset_stolen(struct intel_gt *gt,
>  				      ggtt->error_capture.start,
>  				      PAGE_SIZE);
>  
> -		if (!__drm_mm_interval_first(&gt->i915->mm.stolen,
> -					     page << PAGE_SHIFT,
> -					     ((page + 1) << PAGE_SHIFT) - 1))
> +		mutex_lock(&gt->i915->mm.stolen_lock);
> +		if (stolen_page_unused(gt, page))
>  			memset_io(s, STACK_MAGIC, PAGE_SIZE);
> +		mutex_unlock(&gt->i915->mm.stolen_lock);

[Severity: High]
Since __igt_reset_stolen() loops over the full resource_size(&gt->i915->dsm.stolen):

	num_pages = resource_size(dsm) >> PAGE_SHIFT;
	...
	for (page = 0; page < num_pages; page++) {

will this code overwrite hardware-reserved memory regions (like WOPCM or GuC
firmware) at the top of stolen memory?

The drm_mm allocator only covers the unreserved usable_size, so queries to the
reserved region using stolen_page_unused() will return true. Writing STACK_MAGIC
to these hardware-reserved pages could destroy hardware state before triggering
a GPU reset, potentially leading to a GPU hang or system crash.

This regression appears to have been unmasked by fixing the previously
always-false !__drm_mm_interval_first() check.

[Severity: Low]
Does dropping the lock here and re-acquiring it later for the verification pass
create a race condition?

If a concurrent thread allocates, writes to, and frees a page in stolen memory
between this baseline CRC pass and the later verification pass, could the test
incorrectly attribute the modification to the GPU reset and fail spuriously?

>  
>  		in = (void __force *)s;
>  		if (i915_memcpy_from_wc(tmp, in, PAGE_SIZE))
[ ... ]
> @@ -143,16 +153,15 @@ __igt_reset_stolen(struct intel_gt *gt,
>  			in = tmp;
>  		x = crc32_le(0, in, PAGE_SIZE);
>  
> -		if (x != crc[page] &&
> -		    !__drm_mm_interval_first(&gt->i915->mm.stolen,
> -					     page << PAGE_SHIFT,
> -					     ((page + 1) << PAGE_SHIFT) - 1)) {
> +		mutex_lock(&gt->i915->mm.stolen_lock);
> +		if (x != crc[page] && stolen_page_unused(gt, page)) {

[Severity: Low]
Because the lock was dropped during the reset window, can this check guarantee
that the page remained unused throughout the entire reset window?

It appears this only checks if the page is currently unused and modified at
this specific moment, missing any concurrent use while the lock was not held.

>  			pr_debug("unused stolen page %pa modified by GPU reset\n",
>  				 &page);
>  			if (count++ == 0)
>  				igt_hexdump(in, PAGE_SIZE);
>  			max = page;
>  		}
> +		mutex_unlock(&gt->i915->mm.stolen_lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917113717.2852-1-sebastian.brzezinka@intel.com?part=1