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:04:09PM -0700, Matthew Brost wrote:

One mistake in a suggestion. Inline below.

> 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);

I missed adjusting n_offlined_pages and adding nentry to offlined_pages here.

So I think check the return of xe_ttm_vram_buddy_alloc, then adjust
n_offlined_pages + add nentry to offlined_pages.

Matt

>                 }
> -               /* 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