Re: [PATCH V20 09/15] drm/xe/vram: Add VRAM page offline fault handler

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Sep 02, 2026 at 04:25:40PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Lockless read of `res->bo` in `xe_ttm_vram_block_owner_get` leads to a Use-After-Free when racing with a TTM ghost object transfer.
> - [High] Race condition when offlining a free VRAM page allows the faulty page to be allocated and never purged.
> - [High] Forcefully unpinning BOs in `xe_ttm_vram_purge_page` violates pin counting semantics and creates TOCTOU vulnerabilities.
> - [High] `xe_ttm_vram_addr_to_region` ignores the `kernel_vram` region, causing hardware faults in critical kernel memory to be silently ignored.
> - [Medium] A race condition between two-phase page reservation and synchronous BO eviction falsely marks successfully offlined pages as failed.
> --
> 
> commit 23a2bf9a5461265765230ffd0e7a9d24c27ca918
> Author: Tejas Upadhyay <tejas.upadhyay@intel.com>
> Subject: drm/xe/vram: Add VRAM page offline fault handler
> 
> This commit introduces a hardware VRAM page offline fault handler for the xe
> driver. It adds functionality to purge the buffer object containing the faulty
> address, ban the associated VM or exec queue, and reserve the hardware page in
> the buddy allocator to prevent future use.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> > index d97739a84a2d1..0ab92bc3c6998 100644
> > --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> > +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> [ ... ]
> > @@ -580,3 +618,295 @@ u64 xe_ttm_vram_get_avail(struct ttm_resource_manager *man)
> [ ... ]
> > +static int xe_ttm_vram_purge_page(struct xe_device *xe, struct xe_bo *bo)
> > +{
> [ ... ]
> > +	xe_bo_set_purgeable_state(bo, XE_MADV_PURGEABLE_DONTNEED);
> > +	ttm_bo_unmap_virtual(&bo->ttm);   /* nuke CPU mmap + VRAM IO mappings */
> > +	if (xe_bo_is_pinned(bo))
> > +		xe_bo_unpin(bo);
> 
> [Severity: High]
> Does this forcefully break the pin counting contract by unpinning a BO on
> behalf of an unknown owner?
> 
> In xe_ttm_vram_purge_page(), this unconditionally unpins the BO if it is
> pinned, stealing a pin reference belonging to another subsystem (such as the
> display controller). When the true owner later unpins the BO, could this
> trigger an underflow crash?
> 
> > +	ret = xe_ttm_bo_purge(&bo->ttm, &ctx);
> > +	xe_bo_unlock(bo);
> > +
> > +out:
> [ ... ]
> > +static struct xe_bo *xe_ttm_vram_block_owner_get(struct gpu_buddy_block *block)
> > +{
> > +	struct ttm_resource *res = block->private;
> > +	struct ttm_buffer_object *tbo;
> > +	struct xe_bo *bo;
> > +
> > +	if (!res)
> > +		return NULL;
> > +
> > +	/*
> > +	 * res->bo is updated under bdev->lru_lock by ttm_resource_set_bo().
> > +	 * Racing with a ghost transfer here is benign: we either see the old
> > +	 * owner (whose purge is a no-op and the retry path recovers) or the
> > +	 * ghost (rejected below).
> > +	 *
> > +	 * A ghost is a bare ttm_transfer_obj, not an xe_bo, so ttm_to_xe_bo()
> > +	 * on one would be out of bounds. xe_bo_is_xe_bo() rejects it since
> > +	 * only our own BOs carry xe_ttm_bo_destroy().
> > +	 */
> > +	tbo = READ_ONCE(res->bo);
> > +	if (!tbo || !xe_bo_is_xe_bo(tbo))
> > +		return NULL;
> > +
> > +	bo = ttm_to_xe_bo(tbo);
> 
> [Severity: High]
> Is it safe to read res->bo locklessly and pass it to xe_bo_is_xe_bo()?
> 
> In xe_ttm_vram_block_owner_get(), tbo is read locklessly. A concurrent
> accelerated move or pipelined gutting can transfer the resource to a TTM
> ghost object under bdev->lru_lock. This drops the last reference to the
> original BO, freeing it via kfree(). 
> 
> If the thread is preempted and resumes after the original BO is freed,
> passing the stale tbo pointer to xe_bo_is_xe_bo(tbo) will dereference
> tbo->destroy on freed memory, causing a use-after-free regression.
> 

This one is correct and this memory unsafe here.

Luckily, this fix is easy...

This one will need to be fixed, snippet below:

diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
index 6a954018012a..ff937ab3837a 100644
--- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
+++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
@@ -733,7 +733,8 @@ static bool xe_ttm_vram_page_already_processed(struct xe_ttm_vram_mgr *mgr,
  * blocks will hit xe_ttm_vram_mgr_del() on their own and the pending
  * reservation is retried from there.
  */
-static struct xe_bo *xe_ttm_vram_block_owner_get(struct gpu_buddy_block *block)
+static struct xe_bo *xe_ttm_vram_block_owner_get(struct xe_device *xe,
+                                                struct gpu_buddy_block *block)
 {
        struct ttm_resource *res = block->private;
        struct ttm_buffer_object *tbo;
@@ -742,6 +743,8 @@ static struct xe_bo *xe_ttm_vram_block_owner_get(struct gpu_buddy_block *block)
        if (!res)
                return NULL;

+       guard(spinlock)(&xe->ttm.lru_lock);
+
        /*
         * res->bo is updated under bdev->lru_lock by ttm_resource_set_bo().
         * Racing with a ghost transfer here is benign: we either see the old
@@ -788,7 +791,7 @@ static int xe_ttm_vram_reserve_page_at_addr(struct xe_device *xe, u64 addr,
                nentry->addr = addr;

                if (block) {
-                       pbo = xe_ttm_vram_block_owner_get(block);
+                       pbo = xe_ttm_vram_block_owner_get(xe, block);

                        /*
                         * Critical kernel BO? Best-effort check without resv lock;


> > +
> > +	/* The BO may already be in teardown with a zero refcount */
> > +	return xe_bo_get_unless_zero(bo) ? bo : NULL;
> > +}
> > +
> > +static int xe_ttm_vram_reserve_page_at_addr(struct xe_device *xe, u64 addr,
> > +					    struct xe_ttm_vram_mgr *vram_mgr, struct gpu_buddy *mm)
> > +{
> [ ... ]
> > +		if (block) {
> > +			pbo = xe_ttm_vram_block_owner_get(block);
> > +
> > +			/*
> > +			 * Critical kernel BO? Best-effort check without resv lock;
> > +			 * worst case a concurrent pin causes reset path unnecessarily.
> > +			 */
> > +			if (pbo && ((pbo->ttm.type == ttm_bo_type_kernel &&
> > +				     !(pbo->flags & XE_BO_FLAG_PINNED_LATE_RESTORE)) ||
> > +				    (xe_bo_is_user(pbo) && xe_bo_is_pinned(pbo)))) {
> 
> [Severity: High]
> Does this lockless pin check create a time-of-check to time-of-use race condition?
> 
> In xe_ttm_vram_reserve_page_at_addr(), xe_bo_is_pinned(pbo) is checked
> without holding the appropriate locks. A user BO could be pinned right after
> this check (e.g., for display scanout), and then forcefully unpinned and
> purged. 
> 
> The display hardware could then continue scanning out the old physical
> address, potentially leaking data of whatever new BO is allocated there.
>

We should probably file a Jira for this, as I agree this possible with
the right race + NIC trying to pin the BO, but unsure how to handle one
properly.
 
> > +				kfree(nentry);
> > +				pbo_to_put = pbo;
> > +				drm_err(&xe->drm,
> > +					"%s: addr: 0x%llx is critical kernel bo, requesting SBR\n",
> > +					__func__, addr);
> > +				break;
> > +			}
> > +		}
> > +		/* Queue both free and occupied (to-be-purged) pages */
> > +		++vram_mgr->n_queued_pages;
> > +		list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
> > +	}
> > +
> > +	/* Deferred put outside lock to avoid recursive deadlock */
> 
> [Severity: High]
> Can a concurrent allocation claim a faulty free page when the lock is dropped?
> 
> In xe_ttm_vram_reserve_page_at_addr(), when a hardware fault occurs at a free
> address, gpu_buddy_allocated_addr_to_block() returns NULL, meaning pbo is
> NULL. The page is queued and vram_mgr->lock is dropped to prevent deadlocks.
> 
> During this lockless window, a concurrent TTM allocation can claim this
> exact free buddy block. When the fault handler later re-acquires the lock
> and attempts xe_ttm_vram_buddy_alloc(), it will fail, returning 0 without
> purging (since pbo was NULL). This leaves the newly allocated BO using a
> faulty hardware page, which will cause persistent hardware errors.
> 

This also looks right. So I think:

@@ -804,10 +807,17 @@ static int xe_ttm_vram_reserve_page_at_addr(struct xe_device *xe, u64 addr,
                                        __func__, addr);
                                break;
                        }
+
+                       /* Queue both free and occupied (to-be-purged) pages */
+                       ++vram_mgr->n_queued_pages;
+                       list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
+               } else {
+                       /* Immediately offline */
+                       return xe_ttm_vram_buddy_alloc(vram_mgr, addr, addr + size,
+                                                      size, size, &nentry->blocks,
+                                                      GPU_BUDDY_RANGE_ALLOCATION,
+                                                      NULL, &nentry->used_visible_size);
                }
-               /* Queue both free and occupied (to-be-purged) pages */
-               ++vram_mgr->n_queued_pages;
-               list_add_rcu(&nentry->queued_link, &vram_mgr->queued_pages);
        }


> > +	if (pbo_to_put) {
> > +		xe_bo_put(pbo_to_put);
> > +		/* Hint System controller driver for reset with -EIO  */
> > +		return -EIO;
> > +	}
> > +
> > +	if (pbo) {
> > +		/*
> > +		 * Purge BO containing address - reference held from above.
> > +		 * This does not necessarily free the blocks synchronously: if
> > +		 * the BO is not idle, ttm_bo_pipeline_gutting() hands the
> > +		 * resource to a ghost object and it is released only once the
> > +		 * move fences signal. The reservation below then fails and is
> > +		 * retried from xe_ttm_vram_mgr_del().
> > +		 */
> > +		ret = xe_ttm_vram_purge_page(xe, pbo);
> > +		xe_bo_put(pbo);
> > +		if (ret)
> > +			drm_warn(&xe->drm, "Purge failed at addr:0x%llx, ret:%d\n", addr, ret);
> > +	}
> > +
> > +	scoped_guard(mutex, &vram_mgr->lock) {
> > +		ret = xe_ttm_vram_buddy_alloc(vram_mgr, addr, addr + size,
> > +					      size, size, &nentry->blocks,
> > +					      GPU_BUDDY_RANGE_ALLOCATION,
> > +					      NULL, &nentry->used_visible_size);
> > +		if (ret) {
> > +			nentry->status = XE_PAGE_RESERVE_FAIL;
> > +			drm_dbg(&xe->drm,
> > +				"Page at addr:0x%llx still busy (%d), deferring reservation\n",
> > +				addr, ret);
> > +			return 0;
> > +		}
> 
> [Severity: Medium]
> Does this unconditionally attempt buddy allocation without checking if the
> retry path already succeeded?
> 
> In xe_ttm_vram_reserve_page_at_addr(), if xe_ttm_vram_purge_page() frees the
> BO's VRAM resource synchronously, it invokes xe_ttm_vram_mgr_del() which
> triggers xe_ttm_vram_retry_queued_pages(). The retry logic will successfully
> allocate the buddy block.
> 
> When the original thread re-acquires the lock here and blindly calls
> xe_ttm_vram_buddy_alloc(), it will fail with -ENOSPC because the block is
> already reserved. The page's status is then falsely marked as
> XE_PAGE_RESERVE_FAIL, leading to misleading internal state.
> 
> > +
> > +		list_for_each_entry_safe(pos, n, &vram_mgr->queued_pages, queued_link) {
> [ ... ]
> > +static struct xe_vram_region *xe_ttm_vram_addr_to_region(struct xe_device *xe, u64 addr)
> > +{
> > +	struct xe_tile *tile;
> > +	u8 id;
> > +
> > +	for_each_tile(tile, xe, id) {
> > +		struct xe_vram_region *vr = tile->mem.vram;
> > +
> > +		if (!vr)
> > +			continue;
> 
> [Severity: High]
> Does this skip checking the kernel-dedicated VRAM region?
> 
> In xe_ttm_vram_addr_to_region(), the address lookup loop only checks
> tile->mem.vram and neglects tile->mem.kernel_vram. If a hardware fault occurs
> within kernel_vram, the address mapping fails and returns
> ERR_PTR(-EOPNOTSUPP). This causes faults in kernel memory to be silently
> ignored instead of triggering a system reset.
> 

tile->mem.kernel_vram = tile->mem.vram in the current code but for
completeness, this likely right too. I'd check the kernel_vram if
tile->mem.kernel_vram != tile->mem.vram.

Matt

> > +
> > +		if (addr >= vr->dpa_base && addr < (vr->dpa_base + vr->usable_size))
> > +			return vr;
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260902145343.465686-17-tejas.upadhyay@intel.com?part=9