[PATCH v5 0/2] drm/i915/selftests: fix stolen reset test races

[PATCH v5 0/2] drm/i915/selftests: fix stolen reset test races

[Sebastian Brzezinka]

The stolen reset selftests (igt_reset_device_stolen and
igt_reset_engines_stolen) poison the unused stolen pages, reset the GPU
and check that those pages did not change. This series fixes two races
in the test itself. Neither one is a GPU reset problem.

v4 -> v5:
- Hold stolen_lock across the reset too (patch 1)
- Add patch 2, which takes ggtt->error_mutex in the test

Sebastian Brzezinka (2):
  drm/i915/selftests: Fix unlocked mm.stolen in reset selftest
  drm/i915/selftests: take error_mutex in stolen reset test

 drivers/gpu/drm/i915/gt/selftest_reset.c | 47 +++++++++++++++++++-----
 1 file changed, 38 insertions(+), 9 deletions(-)

-- 
2.55.0

[PATCH v5 1/2] 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]
...

Also cap num_pages to i915->dsm.usable_size, which already excludes the
hardware reserved region from drm_mm.stolen's range, so poison/verify
writes never touch reserved stolen pages.

Hold stolen_lock across the reset too, so nothing can use stolen memory
between the poison pass and the verify pass.

Fixing the always false "page unused" check exposed false positives on
non-LLC platforms, where ring buffers are allocated from stolen. The
spinner rings are in use during the poison pass, but are filled with
POISON_FREE and released while the reset retires the requests. In the
verify pass they look unused but have a changed CRC. Track which pages
were actually poisoned and only verify those.

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

v2 -> v3:
- Cap num_pages to i915->dsm.usable_size to avoid poisoning
  hardware reserved stolen pages (Shashiko)

v3 -> v4:
- Only verify pages poisoned before the reset; pages in use (e.g.
  rings) may be rewritten and freed during reset (CI)
---
 drivers/gpu/drm/i915/gt/selftest_reset.c | 45 +++++++++++++++++++-----
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
index 2cfc23c58e90..aae9b55ee023 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,
@@ -26,6 +36,7 @@ __igt_reset_stolen(struct intel_gt *gt,
 	intel_wakeref_t wakeref;
 	enum intel_engine_id id;
 	struct igt_spinner spin;
+	unsigned long *poisoned;
 	long max, count;
 	void *tmp;
 	u32 *crc;
@@ -34,7 +45,8 @@ __igt_reset_stolen(struct intel_gt *gt,
 	if (!drm_mm_node_allocated(&ggtt->error_capture))
 		return 0;
 
-	num_pages = resource_size(dsm) >> PAGE_SHIFT;
+	num_pages = min_t(resource_size_t, resource_size(dsm),
+			  gt->i915->dsm.usable_size) >> PAGE_SHIFT;
 	if (!num_pages)
 		return 0;
 
@@ -42,10 +54,16 @@ __igt_reset_stolen(struct intel_gt *gt,
 	if (!crc)
 		return -ENOMEM;
 
+	poisoned = bitmap_zalloc(num_pages, GFP_KERNEL);
+	if (!poisoned) {
+		err = -ENOMEM;
+		goto err_crc;
+	}
+
 	tmp = kmalloc(PAGE_SIZE, GFP_KERNEL);
 	if (!tmp) {
 		err = -ENOMEM;
-		goto err_crc;
+		goto err_poisoned;
 	}
 
 	igt_global_reset_lock(gt);
@@ -79,6 +97,8 @@ __igt_reset_stolen(struct intel_gt *gt,
 		i915_request_add(rq);
 	}
 
+	mutex_lock(&gt->i915->mm.stolen_lock);
+
 	for (page = 0; page < num_pages; page++) {
 		dma_addr_t dma = (dma_addr_t)dsm->start + (page << PAGE_SHIFT);
 		void __iomem *s;
@@ -95,10 +115,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))
+		if (stolen_page_unused(gt, page)) {
 			memset_io(s, STACK_MAGIC, PAGE_SIZE);
+			__set_bit(page, poisoned);
+		}
 
 		in = (void __force *)s;
 		if (i915_memcpy_from_wc(tmp, in, PAGE_SIZE))
@@ -143,10 +163,13 @@ __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)) {
+		/*
+		 * Only check pages we poisoned: pages in use before the
+		 * reset (e.g. ring buffers) may legitimately be rewritten
+		 * and released during the reset.
+		 */
+		if (x != crc[page] && test_bit(page, poisoned) &&
+		    stolen_page_unused(gt, page)) {
 			pr_debug("unused stolen page %pa modified by GPU reset\n",
 				 &page);
 			if (count++ == 0)
@@ -159,6 +182,8 @@ __igt_reset_stolen(struct intel_gt *gt,
 	mb();
 	ggtt->vm.clear_range(&ggtt->vm, ggtt->error_capture.start, PAGE_SIZE);
 
+	mutex_unlock(&gt->i915->mm.stolen_lock);
+
 	if (count > 0) {
 		pr_info("%s reset clobbered %ld pages of stolen, last clobber at page %ld\n",
 			msg, count, max);
@@ -177,6 +202,8 @@ __igt_reset_stolen(struct intel_gt *gt,
 	igt_global_reset_unlock(gt);
 
 	kfree(tmp);
+err_poisoned:
+	bitmap_free(poisoned);
 err_crc:
 	kfree(crc);
 	return err;
-- 
2.55.0

[PATCH v5 2/2] drm/i915/selftests: take error_mutex in stolen reset test

[Sebastian Brzezinka]

The stolen reset test reads and writes stolen pages through the ggtt
error_capture slot, but it does not take ggtt->error_mutex. The DEBUG_GEM
poisoning of stolen pages, which runs from the object free worker, and
the GPU error capture use the same slot, and both take that mutex.

So the free worker can change the slot while the test is using it, and
part of its poison ends up in the page under test. The page then looks
partly like STACK_MAGIC and partly like POISON_INUSE/POISON_FREE, and the
test wrongly reports that the GPU reset modified stolen memory.

Take ggtt->error_mutex, nested inside stolen_lock, for the whole
sampling and reset.

Signed-off-by: Sebastian Brzezinka <sebastian.brzezinka@intel.com>
---
 drivers/gpu/drm/i915/gt/selftest_reset.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
index aae9b55ee023..f295edbb3feb 100644
--- a/drivers/gpu/drm/i915/gt/selftest_reset.c
+++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
@@ -98,6 +98,7 @@ __igt_reset_stolen(struct intel_gt *gt,
 	}
 
 	mutex_lock(&gt->i915->mm.stolen_lock);
+	mutex_lock(&ggtt->error_mutex);
 
 	for (page = 0; page < num_pages; page++) {
 		dma_addr_t dma = (dma_addr_t)dsm->start + (page << PAGE_SHIFT);
@@ -182,6 +183,7 @@ __igt_reset_stolen(struct intel_gt *gt,
 	mb();
 	ggtt->vm.clear_range(&ggtt->vm, ggtt->error_capture.start, PAGE_SIZE);
 
+	mutex_unlock(&ggtt->error_mutex);
 	mutex_unlock(&gt->i915->mm.stolen_lock);
 
 	if (count > 0) {
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] drm/i915/selftests: redundant poisoned bitmap in
  __igt_reset_stolen()
--

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

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

This commit fixes the stolen memory reset selftest to hold
i915->mm.stolen_lock across the poison and verify passes, check against
mm.stolen.head_node, cap the tested range to dsm.usable_size, and track
poisoned pages in a bitmap.

> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index 3599c9f101e14..a419dfe9a34fb 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
[ ... ]
> @@ -79,6 +97,8 @@ __igt_reset_stolen(struct intel_gt *gt,
>  		i915_request_add(rq);
>  	}
>  
> +	mutex_lock(&gt->i915->mm.stolen_lock);
> +
>  	for (page = 0; page < num_pages; page++) {
>  		dma_addr_t dma = (dma_addr_t)dsm->start + (page << PAGE_SHIFT);
>  		void __iomem *s;
> @@ -95,10 +115,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))
> +		if (stolen_page_unused(gt, page)) {
>  			memset_io(s, STACK_MAGIC, PAGE_SIZE);
> +			__set_bit(page, poisoned);
> +		}
[ ... ]
> @@ -143,10 +163,13 @@ __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)) {
> +		/*
> +		 * Only check pages we poisoned: pages in use before the
> +		 * reset (e.g. ring buffers) may legitimately be rewritten
> +		 * and released during the reset.
> +		 */
> +		if (x != crc[page] && test_bit(page, poisoned) &&
> +		    stolen_page_unused(gt, page)) {
>  			pr_debug("unused stolen page %pa modified by GPU reset\n",
>  				 &page);
>  			if (count++ == 0)

[Severity: Low]
Is the poisoned bitmap needed when gt->i915->mm.stolen_lock is held
continuously across the poison pass, the reset, and the verify pass?

Every insertion and removal in gt->i915->mm.stolen (including
i915_gem_object_release_stolen() -> __i915_gem_stolen_remove_node()) takes
stolen_lock.  Because __igt_reset_stolen() never drops stolen_lock between
the two loops, no stolen node can be released during the reset, so
stolen_page_unused(gt, page) in the verify pass always returns the same
value as in the poison pass and matches test_bit(page, poisoned).

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

Re: [PATCH v5 2/2] drm/i915/selftests: take error_mutex in stolen reset test

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/i915/selftests: lock inversion holding error_mutex in
  __igt_reset_stolen()
- [Medium] drm/i915/selftests: lockdep inversion in __igt_reset_stolen()
  insert_page()
--

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

drm/i915/selftests: take error_mutex in stolen reset test

This commit acquires ggtt->error_mutex around the stolen memory page
sampling and GPU reset in __igt_reset_stolen() to serialize access to the
ggtt->error_capture slot.

> diff --git a/drivers/gpu/drm/i915/gt/selftest_reset.c b/drivers/gpu/drm/i915/gt/selftest_reset.c
> index a419dfe9a34fb..386741637e45d 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_reset.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_reset.c
> @@ -98,6 +98,7 @@ __igt_reset_stolen(struct intel_gt *gt,
>  	}
>  
>  	mutex_lock(&gt->i915->mm.stolen_lock);
> +	mutex_lock(&ggtt->error_mutex);
>  
>  	for (page = 0; page < num_pages; page++) {
>  		dma_addr_t dma = (dma_addr_t)dsm->start + (page << PAGE_SHIFT);

[Severity: Medium]
Does calling ggtt->vm.insert_page() in the two sampling loops while
holding ggtt->error_mutex introduce a lockdep circular dependency on
Cherryview and Broxton with VT-d?

On platforms where intel_vm_no_concurrent_access_wa() is true,
gen8_gmch_probe() sets ggtt->vm.insert_page to
bxt_vtd_ggtt_insert_page__BKL(), which calls stop_machine() and acquires
cpu_hotplug_lock.  Because i915_vma_coredump_create() acquires
ggtt->error_mutex inside a dma_fence_begin_signalling() section (after
i915_vma_resource_hold()), lockdep already tracks cpu_hotplug_lock ->
dma_fence_map -> ggtt->error_mutex.  Calling ggtt->vm.insert_page() under
ggtt->error_mutex inverts that order.

Using ggtt->vm.raw_insert_page when non-NULL, the same way
i915_vma_coredump_create() does under ggtt->error_mutex, avoids
stop_machine().

> @@ -182,6 +183,7 @@ __igt_reset_stolen(struct intel_gt *gt,
>  	mb();
>  	ggtt->vm.clear_range(&ggtt->vm, ggtt->error_capture.start, PAGE_SIZE);
>  
> +	mutex_unlock(&ggtt->error_mutex);
>  	mutex_unlock(&gt->i915->mm.stolen_lock);
>  
>  	if (count > 0) {

[Severity: Medium]
Can keeping ggtt->error_mutex held across intel_gt_reset() cause an ABBA
deadlock with GuC error capture?

CPU0                                  kworker (guc->ct.requests.worker)
----                                  ---------------------------------
__igt_reset_stolen()
  mutex_lock(&ggtt->error_mutex)
  intel_gt_reset()
    mutex_lock(&gt->reset.mutex)
    ...
    flush_work(&guc->ct.requests.worker)
                                      ct_incoming_request_worker_func()
                                        ...
                                        i915_vma_coredump_create()
                                          mutex_lock(&ggtt->error_mutex)

 *** DEADLOCK ***

Holding ggtt->error_mutex across intel_gt_reset() also inverts the lock
order against i_mmap_rwsem (acquired in gt_revoke() ->
unmap_mapping_range()) and dma_fence_map, since i915_vma_coredump_create()
takes ggtt->error_mutex inside a dma_fence_begin_signalling() section.

Since ggtt->error_capture is cleared before the reset and
gt->i915->mm.stolen_lock stays held across it, ggtt->error_mutex could be
unlocked before the reset and re-locked for the post-reset loop.

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

✓ i915.CI.BAT: success for drm/i915/selftests: fix stolen reset test races

[Patchwork]

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

== Series Details ==

Series: drm/i915/selftests: fix stolen reset test races
URL   : https://patchwork.freedesktop.org/series/178393/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_19506 -> Patchwork_178393v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

  Additional (1): bat-adls-6 
  Missing    (1): bat-dg2-13 

Known issues
------------

  Here are the changes found in Patchwork_178393v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-adls-6:         NOTRUN -> [SKIP][1] ([i915#4613]) +3 other tests skip
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@gem_tiled_pread_basic@basic:
    - bat-adls-6:         NOTRUN -> [SKIP][2] ([i915#15656])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@gem_tiled_pread_basic@basic.html

  * igt@intel_hwmon@hwmon-read:
    - bat-adls-6:         NOTRUN -> [SKIP][3] ([i915#7707]) +1 other test skip
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@intel_hwmon@hwmon-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-adls-6:         NOTRUN -> [SKIP][4] ([i915#4103]) +1 other test skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_dsc@dsc-basic:
    - bat-adls-6:         NOTRUN -> [SKIP][5] ([i915#16361])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@kms_dsc@dsc-basic.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-adls-6:         NOTRUN -> [SKIP][6] ([i915#17452])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_pm_backlight@basic-brightness:
    - bat-adls-6:         NOTRUN -> [SKIP][7] ([i915#5354])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@kms_pm_backlight@basic-brightness.html

  * igt@kms_psr@psr-primary-mmap-gtt:
    - bat-adls-6:         NOTRUN -> [SKIP][8] ([i915#1072] / [i915#9732]) +3 other tests skip
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@kms_psr@psr-primary-mmap-gtt.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-adls-6:         NOTRUN -> [SKIP][9] ([i915#3555])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-read:
    - bat-adls-6:         NOTRUN -> [SKIP][10] ([i915#17448] / [i915#3291]) +2 other tests skip
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_178393v1/bat-adls-6/igt@prime_vgem@basic-fence-read.html

  
  [i915#1072]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/1072
  [i915#15656]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/15656
  [i915#16361]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/16361
  [i915#17448]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17448
  [i915#17452]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/17452
  [i915#3291]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/3555
  [i915#4103]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4103
  [i915#4613]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4613
  [i915#5354]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/5354
  [i915#7707]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7707
  [i915#9732]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/9732


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

  * Linux: CI_DRM_19506 -> Patchwork_178393v1

  CI-20190529: 20190529
  CI_DRM_19506: 8182671511e86bff6780533bce20c209a13dc8bb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_9187: 9187
  Patchwork_178393v1: 8182671511e86bff6780533bce20c209a13dc8bb @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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