_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 <[email protected]>
---
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

Reply via email to