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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Time-of-check to time-of-use (TOCTOU) race and unconditional forceful unpinning of BO corrupts pin accounting and leads to panic.
--

commit c3198ab922e6897a8d11667a30d137d679162800
Author: Tejas Upadhyay <tejas.upadhyay@intel.com>

drm/xe/vram: Add VRAM page offline fault handler

Add the core VRAM page offlining logic to handle HW-reported faulty
physical addresses.

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index d97739a84a2d1..aea4cb0b66fd0 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c

[ ... ]

> +static int xe_ttm_vram_purge_page(struct xe_device *xe, struct xe_bo *bo)
> +{
> +	struct ttm_operation_ctx ctx = {};

[ ... ]

> +	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 code forcefully unpin the BO without owning the pin reference?

When xe_ttm_vram_purge_page() gets called, it unconditionally drops the
pin count if the BO is pinned. Since the pin is owned by an external subsystem
(like display or dma-buf), this steals their reference. 

When the legitimate owner later calls xe_bo_unpin_external(), the pin count
will underflow or fail an assertion (xe_assert(xe, xe_bo_is_pinned(bo))),
leading to a kernel panic because xe_bo_unpin_external() lacks the
xe_bo_is_purged() short-circuit found in xe_bo_unpin().

Is there a way to handle this without corrupting the pin accounting?

> +	ret = xe_ttm_bo_purge(&bo->ttm, &ctx);
> +	xe_bo_unlock(bo);
> +
> +out:
> +	if (q_to_put) {
> +		xe_exec_queue_kill(q_to_put);
> +		xe_exec_queue_put(q_to_put);
> +	}
> +
> +	return ret;
> +}

[ ... ]

> +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(xe, 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]
Can a concurrent pin on a user BO bypass this check?

This check in xe_ttm_vram_reserve_page_at_addr() reads xe_bo_is_pinned(pbo)
without holding the BO's dma_resv lock. If a user BO is concurrently pinned via
dma-buf or display immediately after this check, it will fall through to
xe_ttm_vram_purge_page(). 

Because xe_ttm_vram_purge_page() acquires the lock and calls xe_bo_unpin()
unconditionally on pinned BOs, the newly pinned BO will have its pin reference
stolen, eventually causing a panic when the true owner tries to unpin it. 

Furthermore, kernel BOs with XE_BO_FLAG_PINNED_LATE_RESTORE intentionally
bypass this check and are also forcefully unpinned. If the subsequent
xe_ttm_bo_purge() fails, the BO isn't marked as purged, and a later legitimate
unpin will hit the same assertion panic.

Could this race be resolved by checking the pin status after acquiring the
dma_resv lock?

> +				kfree(nentry);
> +				pbo_to_put = pbo;
> +				drm_err(&xe->drm,
> +					"%s: addr: 0x%llx is critical kernel bo, requesting SBR\n",
> +					__func__, addr);
> +				break;
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903161553.528932-17-tejas.upadhyay@intel.com?part=9