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

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

[Sebastian Brzezinka]

_igt_reset_stolen() use via __drm_mm_interval_first(), but
does so without holding i915->mm.stolen_lock. Every writer of this
(i915_gem_stolen.c: drm_mm_insert_node_in_range(), drm_mm_reserve_node(),
drm_mm_remove_node()) takes stolen_lock, so the selftest's unlocked
reader can race.

This was observed as a NULL pointer dereference in
__drm_mm_interval_first() during igt_reset_device_stolen():

BUG: kernel NULL pointer dereference, address: 0000000000000010
...
RIP: 0010:__drm_mm_interval_first+0x2c/0x90
...
Call Trace:
  __igt_reset_stolen+0x565/0x6a0 [i915]
  igt_reset_device_stolen+0x1a/0x30 [i915]
  __i915_subtests+0xb8/0x250 [i915]
...

Fixes: 3da3c5c1c982 ("drm/i915: Exclude low pages (128KiB) of stolen from use")
Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16841
Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
---
v1 -> v2:
- Fix always false !__drm_mm_interval_first() check (Andi)
- Extend stolen_lock over the poison/verify writes, not just
  the lookup (Andi)
- Move the lookup into stolen_page_unused() helper
---
 drivers/gpu/drm/i915/gt/selftest_reset.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
index 2cfc23c58e90..77af788314ff 100644
--- a/drivers/gpu/drm/i915/gt/selftest_reset.c
+++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
@@ -14,6 +14,16 @@
 #include "selftests/igt_atomic.h"
 #include "selftests/igt_spinner.h"
 
+static bool stolen_page_unused(struct intel_gt *gt, resource_size_t page)
+{
+	lockdep_assert_held(&gt->i915->mm.stolen_lock);
+
+	return __drm_mm_interval_first(&gt->i915->mm.stolen,
+					page << PAGE_SHIFT,
+					((page + 1) << PAGE_SHIFT) - 1) ==
+	       &gt->i915->mm.stolen.head_node;
+}
+
 static int
 __igt_reset_stolen(struct intel_gt *gt,
 		   intel_engine_mask_t mask,
@@ -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);
 
 		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)) {
 			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);
 	}
-- 
2.55.0

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

[sashiko-bot]

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

✗ i915.CI.BAT: failure for drm/i915/selftests: Fix unlocked mm.stolen in reset selftest (rev4)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 3103 bytes --]

== Series Details ==

Series: drm/i915/selftests: Fix unlocked mm.stolen in reset selftest (rev4)
URL   : https://patchwork.freedesktop.org/series/172573/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_19160 -> Patchwork_172573v4
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_172573v4 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_172573v4, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172573v4/index.html

Participating hosts (39 -> 38)
------------------------------

  Missing    (1): bat-dg2-13 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_172573v4:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live:
    - fi-bsw-n3050:       [PASS][1] -> [DMESG-FAIL][2] +1 other test dmesg-fail
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19160/fi-bsw-n3050/igt@i915_selftest@live.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172573v4/fi-bsw-n3050/igt@i915_selftest@live.html
    - fi-glk-j4005:       [PASS][3] -> [DMESG-FAIL][4] +1 other test dmesg-fail
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19160/fi-glk-j4005/igt@i915_selftest@live.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172573v4/fi-glk-j4005/igt@i915_selftest@live.html
    - bat-apl-1:          [PASS][5] -> [DMESG-FAIL][6] +1 other test dmesg-fail
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19160/bat-apl-1/igt@i915_selftest@live.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172573v4/bat-apl-1/igt@i915_selftest@live.html

  * igt@i915_selftest@live@reset:
    - fi-bsw-nick:        [PASS][7] -> [DMESG-FAIL][8] +1 other test dmesg-fail
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19160/fi-bsw-nick/igt@i915_selftest@live@reset.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172573v4/fi-bsw-nick/igt@i915_selftest@live@reset.html

  * igt@kms_flip@basic-flip-vs-dpms@c-hdmi-a1:
    - bat-jsl-5:          [PASS][9] -> [ABORT][10] +1 other test abort
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_19160/bat-jsl-5/igt@kms_flip@basic-flip-vs-dpms@c-hdmi-a1.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172573v4/bat-jsl-5/igt@kms_flip@basic-flip-vs-dpms@c-hdmi-a1.html

  


Build changes
-------------

  * Linux: CI_DRM_19160 -> Patchwork_172573v4

  CI-20190529: 20190529
  CI_DRM_19160: d709485448984efaa7ca726c5340ef088f930cac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_9099: 9099
  Patchwork_172573v4: d709485448984efaa7ca726c5340ef088f930cac @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_172573v4/index.html

[-- Attachment #2: Type: text/html, Size: 3792 bytes --]