Re: [PATCH v3 3/3] drm/xe: Synchronously wait for queue kill on FD close

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/xe/guc: async reset on kill timeout causes page table UAF
- [High] drm/xe: missing queue kill_sync in xe_exec_queue_destroy_ioctl
--

commit 50edeb4bbee349d41ce073d20f8d55dc6152900b
Author: Daniel Charles <daniel.charles@intel.com>

    drm/xe: Synchronously wait for queue kill on FD close
    
    The early KILL message disables GuC scheduling outside the serialized
    TDR path, but xe_file_close() does not wait for it to complete before
    dropping queue references. GuC may still schedule contexts whose page
    tables are being torn down, producing CAT errors and engine resets.

> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 7ade915c59a85..af5d9e8f0b16e 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -1623,6 +1623,19 @@ void xe_exec_queue_kill(struct xe_exec_queue *q)
>  	xe_vm_remove_compute_exec_queue(q->vm, q);
>  }
>  
> +/**
> + * xe_exec_queue_kill_sync() - Wait for queue scheduling to stop
> + * @q: The exec queue
> + *
> + * This function waits for the backend to finish stopping the queue after
> + * xe_exec_queue_kill(). It is safe to call multiple times.
> + */
> +void xe_exec_queue_kill_sync(struct xe_exec_queue *q)
> +{
> +	if (q->ops->kill_sync)
> +		q->ops->kill_sync(q);
> +}

[Severity: High]
Does this same fix need to be applied to xe_exec_queue_destroy_ioctl()?

If userspace calls the queue destruction ioctl to destroy an exec queue,
followed immediately by the VM destruction ioctl to destroy the associated
VM, xe_exec_queue_destroy_ioctl() calls xe_exec_queue_kill() which
decrements vm->preempt.num_exec_queues to 0, but it returns without waiting.

The subsequent VM destruction ioctl then succeeds and synchronously frees the
VM's page tables while GuC may still be actively scheduling the queue on
those tables. Will this lead to the same CAT errors and engine resets this
patch is trying to fix?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 609af66b343d5..559937e584aa3 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
[ ... ]
> @@ -2267,6 +2289,28 @@ static void guc_exec_queue_kill(struct xe_exec_queue *q)
>  	xe_guc_exec_queue_trigger_cleanup(q);
>  }
>  
> +static void guc_exec_queue_kill_sync(struct xe_exec_queue *q)
> +{
> +	struct xe_exec_queue *primary = xe_exec_queue_multi_queue_primary(q);
> +	struct xe_guc *guc = exec_queue_to_guc(primary);
> +	long ret;
> +
> +	xe_assert(guc_to_xe(guc), exec_queue_killed(q));
> +
> +	ret = wait_event_timeout(guc->submission_state.kill_wq,
> +				 (!exec_queue_enabled(primary) &&
> +				  !exec_queue_pending_disable(primary) &&
> +				  !exec_queue_pending_enable(primary)) ||
> +				 xe_guc_read_stopped(guc) ||
> +				 vf_recovery(guc) ||
> +				 exec_queue_wedged(primary), HZ * 5);
> +	if (!ret) {
> +		xe_gt_warn(q->gt, "Queue kill failed to respond, guc_id=%d\n",
> +			   primary->guc->id);
> +		xe_gt_reset_async(q->gt);
> +	}

[Severity: High]
Can this timeout handling cause a use-after-free of page tables?

If wait_event_timeout() expires because GuC fails to respond, we queue an
asynchronous GT reset via xe_gt_reset_async() and return immediately.

Because the reset is processed asynchronously by a worker, the GuC hardware
remains active. By returning immediately, the caller (xe_file_close()) will
proceed to drop queue references and synchronously destroy the VM's page
tables via xe_vm_close_and_put().

Since the GuC hardware is not halted before the page tables are freed, could
the hardware page walker access the freed page table memory? Should this
path either halt the hardware synchronously or wait for the reset to complete
before returning?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015050136.1069815-1-daniel.charles@intel.com?part=3