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 <[email protected]> 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(>->i915->mm.stolen, > - page << PAGE_SHIFT, > - ((page + 1) << PAGE_SHIFT) - 1)) > + mutex_lock(>->i915->mm.stolen_lock); > + if (stolen_page_unused(gt, page)) > memset_io(s, STACK_MAGIC, PAGE_SIZE); > + mutex_unlock(>->i915->mm.stolen_lock); [Severity: High] Since __igt_reset_stolen() loops over the full resource_size(>->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(>->i915->mm.stolen, > - page << PAGE_SHIFT, > - ((page + 1) << PAGE_SHIFT) - 1)) { > + mutex_lock(>->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(>->i915->mm.stolen_lock); -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
