Re: [PATCH v1 1/1] drm/xe/guc: Fix wake_up race in handle_sched_done

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Use-After-Free in handle_sched_done due to dereferencing q after destruction

Pre-existing issues:
- [High] Missing Wakeup / Wait Timeout causing spurious GT resets
--

commit ab601decc69aeaee9b9cfcd355026b894c210277
Author: Jakub Legowski <jakub.legowski@intel.com>

drm/xe/guc: Fix wake_up race in handle_sched_done

When a SCHED_DONE G2H arrives for a banned exec queue, handle_sched_done()
calls wake_up_all() before clear_exec_queue_pending_disable(). This commit
moves the wake_up_all() after clear_exec_queue_pending_disable() to prevent
a race with guc_exec_queue_timedout_job().

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 864859bd0ccc9..dca3ad7b4c149 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -3105,10 +3105,6 @@ static void handle_sched_done(struct xe_guc *guc, struct xe_exec_queue *q,
>  			clear_exec_queue_pending_disable(q);
>  			suspend_fence_signal(q);
>  		} else {

[Severity: High]
This isn't a bug introduced by this patch, but does this skip waking up
waiters on guc->ct.wq if a suspend is pending?

If a suspend operation completes here in handle_sched_done(), the code
clears pending_disable but calls suspend_fence_signal(q). On non-VF
platforms, this wakes up q->guc->suspend_wait rather than guc->ct.wq.

Because wake_up_all(&guc->ct.wq) is skipped in this suspend_pending block,
any thread waiting on guc->ct.wq for !pending_disable will time out. Waiters
in guc_exec_queue_timedout_job() or disable_scheduling_deregister() will
hang for 5 seconds and then trigger an unnecessary GT reset.

This can occur under normal operation if a TDR or deregister races with a
queue suspend operation.

While this commit moves the wake_up_all() to ensure waiters observe the
updated state, is it also necessary to wake up guc->ct.wq here?

> -			if (exec_queue_banned(q)) {
> -				smp_wmb();
> -				wake_up_all(&guc->ct.wq);
> -			}
>  			if (exec_queue_destroyed(q)) {
>  				/*
>  				 * Make sure to clear the pending_disable only
[ ... ]
>  				clear_exec_queue_pending_disable(q);
>  				deregister_exec_queue(guc, q);
>  			} else {
>  				clear_exec_queue_pending_disable(q);
>  			}
> +			if (exec_queue_banned(q)) {

[Severity: High]
Can deregister_exec_queue() free the xe_exec_queue q before it is accessed
by exec_queue_banned(q) here in handle_sched_done()?

If q is a multi-queue secondary, deregister_exec_queue() invokes
handle_deregister_done(), which calls __guc_exec_queue_destroy() to queue
the destruction of the queue.

If the queue is wedged, destruction happens synchronously, immediately calling
xe_exec_queue_fini() and freeing q. If not wedged, the destruction occurs
asynchronously on a workqueue, which can execute concurrently and free q.

Either way, q can be freed before handle_sched_done() completes. The
subsequent evaluation of exec_queue_banned(q) accesses
atomic_read(&q->guc->state), resulting in a use-after-free.

Can q be pinned, or could the banned check be moved before deregistering?

> +				/* Pairs with the smp_rmb() in guc_exec_queue_timedout_job() */
> +				smp_wmb();
> +				wake_up_all(&guc->ct.wq);
> +			}
>  		}
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826093325.3233742-1-jakub.legowski@intel.com?part=1