Re: [PATCH] drm/xe: fix UAF race between exec-queue teardown and GuC CT pause-abort

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/xe: stalled CLEANUP and queue leak in
  xe_guc_submit_pause_abort()
--

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

drm/xe: fix UAF race between exec-queue teardown and GuC CT pause-abort

This commit uses kref_get_unless_zero() in xe_guc_submit_pause_abort() to
skip exec queues whose refcount has already dropped to zero during
concurrent teardown.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 9e7c2778029ca..92290b8be24aa 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -3297,12 +3297,18 @@ void xe_guc_submit_pause_abort(struct xe_guc *guc)
>  	xa_for_each(&guc->submission_state.exec_queue_lookup, index, q) {
>  		struct xe_gpu_scheduler *sched = &q->guc->sched;
>  
> -		/* Prevent redundant attempts to stop parallel queues */
> -		if (q->guc->id != index)
> +		/*
> +		 * Prevent redundant attempts to stop parallel queues, and skip
> +		 * queues already dropped to a zero refcount via their own
> +		 * independent teardown (guc_exec_queue_fini(), xarray entry
> +		 * not yet erased)
> +		 */
> +		if (q->guc->id != index || !kref_get_unless_zero(&q->refcount))
>  			continue;
>  
>  		xe_sched_submission_start(sched);
>  		guc_exec_queue_kill(q);
> +		xe_exec_queue_put(q);
>  	}
>  	mutex_unlock(&guc->submission_state.lock);
>  }

[Severity: High]
Does skipping xe_sched_submission_start(sched) when
kref_get_unless_zero(&q->refcount) fails leave sched permanently stopped
with an unprocessed CLEANUP message, leaking q?

When the final reference to q is dropped via xe_exec_queue_put(),
q->refcount is already zero when xe_exec_queue_destroy() ->
guc_exec_queue_destroy() queues STATIC_MSG_CLEANUP onto sched->msgs to run
teardown asynchronously on sched->work_process_msg:

drivers/gpu/drm/xe/xe_guc_submit.c:guc_exec_queue_destroy() {
	struct xe_sched_msg *msg = q->guc->static_msgs + STATIC_MSG_CLEANUP;

	guc_exec_queue_add_msg(q, msg, CLEANUP);
}

Callers of xe_guc_submit_pause_abort() first stop every scheduler in
exec_queue_lookup via xe_guc_submit_stop() -> xe_sched_submission_stop(),
which stops the workqueue and cancels sched->work_process_msg.  During
driver unbind or SR-IOV VF removal, guc_submit_fini() drops references on
wedged queues (and earlier devm release actions such as xe_migrate_fini()
drop kernel queues) right before calling xe_guc_submit_stop().  If
xe_sched_submission_stop() runs before work_process_msg pops
STATIC_MSG_CLEANUP, exec_queue_destroyed(q) is still false, so
guc_exec_queue_stop() does not destroy the queue either.

When xe_guc_submit_pause_abort() then skips xe_sched_submission_start(sched)
because q->refcount is zero, STATIC_MSG_CLEANUP remains stranded on the
stopped scheduler.  guc_exec_queue_destroy_async() and guc_exec_queue_fini()
never run, leaking q along with its runtime PM and drm_device references,
leaving its entry in exec_queue_lookup (which trips the xa_empty() assert in
guc_submit_sw_fini()), and hanging xe_file_close() on
xef->exec_queue.pending_removal for user queues.

Should xe_sched_submission_start(sched) be called before checking
kref_get_unless_zero(&q->refcount) so pending CLEANUP messages can still
be processed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113132149.2989667-2-tejas.upadhyay@intel.com?part=1