Re: [PATCH V18 13/14] drm/xe/uapi: Expose ban reason in EXEC_QUEUE_GET_PROPERTY_BAN

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Concurrent iteration and mutation of `vm->exec_queues.list` using only `down_read` causes list and kernel stack corruption.
- [Medium] The PAGE_OFFLINE ban reason is hidden for non-LR queues until TDR fires, defeating the logic to prevent GPU_HANG masking.
- [Medium] Missing read memory barrier before checking `ban_reason` allows speculative out-of-order execution, causing the UAPI to report a stale status.
--

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

drm/xe/uapi: Expose ban reason in EXEC_QUEUE_GET_PROPERTY_BAN

Extend DRM_XE_EXEC_QUEUE_GET_PROPERTY_BAN to return a bitmask indicating
the reason for the ban, rather than a simple boolean. This allows
userspace to distinguish between different ban causes.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 99d8c807ff056..0a6e2b81b5a53 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -1599,6 +1600,12 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>  	else
>  		wedged = xe_device_wedged(xe);
>  
> +	/*
> +	 * Only tag as GPU hang if this is the original timeout, not a
> +	 * consequence of a prior kill (e.g., page-offline).
> +	 */
> +	if (!exec_queue_killed(q))
> +		atomic_or(DRM_XE_EXEC_QUEUE_BAN_REASON_GPU_HANG, &q->ban_reason);

[Severity: Medium]
Will this properly mask the GPU_HANG bit for non-LR (legacy) queues?

When xe_ttm_vram_purge_page() sets PAGE_OFFLINE on non-LR queues, it calls
xe_vm_kill(). However, xe_vm_kill() only explicitly iterates and kills LR
queues (those in &vm->preempt.exec_queues). 

Since non-LR queues are not explicitly marked as killed, wouldn't this check
evaluate to true and still incorrectly append the GPU_HANG reason when the
doomed non-LR queue eventually times out?

>  	set_exec_queue_banned(q);
>  
>  	/* Kick job / queue off hardware */
[ ... ]
> @@ -2580,13 +2590,29 @@ static void guc_exec_queue_multi_queue_drop_suspend(struct xe_exec_queue *q)
[ ... ]
> -static bool guc_exec_queue_reset_status(struct xe_exec_queue *q)
> +static u64 guc_exec_queue_reset_status(struct xe_exec_queue *q)
>  {
[ ... ]
> -	return exec_queue_reset(q) || exec_queue_killed_or_banned_or_wedged(q);
> +	if (exec_queue_reset(q) || exec_queue_killed_or_banned_or_wedged(q)) {
> +		u64 reason = atomic_read_acquire(&q->ban_reason);
> +
> +		/* If no specific reason was recorded, default to GPU hang */
> +		if (!reason)
> +			reason = DRM_XE_EXEC_QUEUE_BAN_REASON_GPU_HANG;
> +		return reason;
> +	}

[Severity: Medium]
Does this sequence allow for a stale or speculative status read?

Because the condition (exec_queue_killed_or_banned_or_wedged) is evaluated
using a relaxed atomic read, there is only a control dependency before
atomic_read_acquire() on q->ban_reason.

On weakly-ordered architectures, the CPU can speculatively load q->ban_reason
before the state check. If the speculative read happens right as
xe_ttm_vram_purge_page() is updating the status, couldn't reason evaluate to 0
and spuriously default to GPU_HANG instead of PAGE_OFFLINE? Would an smp_rmb()
be required here to properly order the reads?

> diff --git a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> index af9e1fa868d7f..8f583f1631bf4 100644
> --- a/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> +++ b/drivers/gpu/drm/xe/xe_ttm_vram_mgr.c
> @@ -596,7 +598,22 @@ static int xe_ttm_vram_purge_page(struct xe_device *xe, struct xe_bo *bo)
>  	xe_bo_unlock(bo);
>  	/*  Ban VM if BO is PPGTT */
>  	if (vm && (flags & XE_BO_FLAG_PAGETABLE)) {
> +		struct xe_exec_queue *eq;
> +		int id;
> +
>  		down_write(&vm->lock);
> +		if (xe->info.has_ctx_tlb_inval) {
> +			down_read(&vm->exec_queues.lock);
> +			for (id = 0; id < ARRAY_SIZE(vm->exec_queues.list); id++)
> +				list_for_each_entry(eq, &vm->exec_queues.list[id],
> +						    vm_exec_queue_link)
> +					atomic_or(q_flag, &eq->ban_reason);
> +			up_read(&vm->exec_queues.lock);

[Severity: Critical]
Is it safe to iterate vm->exec_queues.list while holding only the lock in
read mode?

Currently, the MMU notifier paths like xe_svm_invalidate() submit TLB
invalidations by calling send_tlb_inval_ctx_ppgtt(). That function also takes
down_read(&vm->exec_queues.lock) but then it mutates the list, moving items
using list_move_tail(&q->vm_exec_queue_link, &tlb_inval_list) onto a local
stack variable.

If the shrinker triggers this purge_page path concurrently with the MMU
notifier, both will execute concurrently because they both hold the lock using
down_read().

Could the list_for_each_entry() iterator here follow pointers into the stack
frame of send_tlb_inval_ctx_ppgtt(), resulting in list/kernel stack
corruption and arbitrary memory writes when atomic_or() is called?

> +		} else {
> +			list_for_each_entry(eq, &vm->preempt.exec_queues, lr.link)
> +				atomic_or(q_flag, &eq->ban_reason);
> +		}
> +		smp_wmb(); /* Force all queue bits to be visible before killing the VM */
>  		xe_vm_kill(vm, true);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826135136.204044-16-tejas.upadhyay@intel.com?part=13