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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: TOCTOU race in unused stolen memory verification
--

commit c306e84112aa79cbd3009b777df94536525c83b4
Author: Sebastian Brzezinka <sebastian.brzezinka@intel.com>

drm/i915/selftests: Fix unlocked mm.stolen in reset selftest

This commit fixes an unlocked mm.stolen read by introducing the
stolen_page_unused() helper and wrapping accesses with stolen_lock. It also
caps num_pages to avoid the hardware-reserved region.

> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index 2cfc23c58e909..811ddf991cb27 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c

[ ... ]

> @@ -95,10 +106,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);
>  
>  		in = (void __force *)s;
>  		if (i915_memcpy_from_wc(tmp, in, PAGE_SIZE))
>  			in = tmp;
>  		crc[page] = crc32_le(0, in, PAGE_SIZE);

[Severity: Medium]
Since stolen_lock is dropped before computing the CRC, does this introduce a
race window where a concurrent thread can allocate the page and modify it
before the initial CRC is calculated?

[ ... ]

> @@ -143,16 +154,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: Medium]
Similarly, can a concurrent thread legitimately allocate, modify, and free the
page while the lock is dropped here?

If this happens, when stolen_lock is re-acquired, stolen_page_unused() will
return true, but the CRC will mismatch. Could this lead the test to falsely
report that the GPU reset clobbered unused stolen memory?

>  			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);
>  
>  		io_mapping_unmap(s);
>  	}

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