Re: [PATCH 1/2] drm/xe/guc: Reject a duplicate deregister-done G2H

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Preexisting TOCTOU Use-After-Free race in G2H handlers due to lockless XArray lookup and immediate kfree() of xe_exec_queue.
--

commit 7071b18636a4e1a16b055aa587e7944a18af7e7e
Author: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

drm/xe/guc: Reject a duplicate deregister-done G2H

This commit adds a check to reject duplicate deregister-done G2H messages
during the window where the registered bit is cleared but the guc_id is not
yet freed.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 99d8c807ff056..e90304e5b9554 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -3323,8 +3323,15 @@ int xe_guc_deregister_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
>  	if (unlikely(!q))
>  		return -EPROTO;
>  
> -	if (!exec_queue_destroyed(q) || exec_queue_pending_disable(q) ||
> -	    exec_queue_pending_enable(q) || exec_queue_enabled(q)) {
> +	/*
> +	 * The registered bit is cleared by the first deregister-done, but the
> +	 * guc_id is only freed once the deferred destroy work has run, so the
> +	 * lookup above still resolves in between. Checking it here makes a
> +	 * duplicate notification an error rather than a second destroy.
> +	 */
> +	if (!exec_queue_registered(q) || !exec_queue_destroyed(q) ||

[Severity: High]
This isn't a bug introduced by this patch, but this added check may trigger
a use-after-free in certain timing conditions.

The queue is loaded from the XArray locklessly and without taking a reference
count in drivers/gpu/drm/xe/xe_guc_submit.c:g2h_exec_queue_lookup:

drivers/gpu/drm/xe/xe_guc_submit.c:g2h_exec_queue_lookup() {
    ...
    q = xa_load(&guc->submission_state.exec_queue_lookup, guc_id);
    ...
}

If a duplicate DEREGISTER_DONE G2H message arrives just as the asynchronous
destroy worker (__guc_exec_queue_destroy_async) is tearing down the
xe_exec_queue, the G2H worker can grab a pointer to q just before the destroy
worker calls xa_erase.

Because the lookup does not increment a reference count, and the object is
freed via kfree in __xe_exec_queue_free without waiting for RCU grace periods,
could exec_queue_registered(q) end up dereferencing a freed pointer?

> +	    exec_queue_pending_disable(q) || exec_queue_pending_enable(q) ||
> +	    exec_queue_enabled(q)) {
>  		xe_gt_err(guc_to_gt(guc),
>  			  "DEREGISTER_DONE: Unexpected engine state 0x%04x, guc_id=%d",
>  			  atomic_read(&q->guc->state), q->guc->id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908231854.1218934-4-umesh.nerlige.ramappa@intel.com?part=1