Re: [PATCH v6 1/3] drm/xe: Prevent VF memory IRQ from accessing a torn-down engine

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] timer: UAF and NULL pointer dereference due to un-synchronized timer_delete() on self-rearming timer
--

commit 6990383ed0fa291cb7a9714db126aaef3e32f00c
Author: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>

    drm/xe: Prevent VF memory IRQ from accessing a torn-down engine

The commit invalidates the engine name during teardown so subsequent scans
skip it, preventing VF memory IRQs from accessing a torn-down engine. It
modifies hw_engine_fini to clear the name and gt pointers under the irq
lock.

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 265f95c0c8993..55ee0c892c1e8 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -275,11 +275,22 @@ static const struct engine_info engine_infos[] = {
>  static void hw_engine_fini(void *arg)
>  {
>  	struct xe_hw_engine *hwe = arg;
> +	struct xe_gt *gt = hwe->gt;
> +
> +	if (gt) {
> +		struct xe_device *xe = gt_to_xe(gt);
> +		unsigned long flags;
>  
> -	if (hwe->exl_port)
> -		xe_execlist_port_destroy(hwe->exl_port);
> +		if (hwe->exl_port)
> +			xe_execlist_port_destroy(hwe->exl_port);
>  
> -	hwe->gt = NULL;
> +		spin_lock_irqsave(&xe->irq.lock, flags);
> +		hwe->name = NULL;
> +		hwe->gt = NULL;

[Severity: High]
Does this code introduce a race condition with a self-rearming timer that can
lead to a NULL pointer dereference or use-after-free?

The xe_execlist_port_destroy() function, called just before this block, tears
down the execlist port but deletes the timer without synchronization:

drivers/gpu/drm/xe/xe_execlist.c:xe_execlist_port_destroy() {
	...
	timer_delete(&port->irq_fail);
	...
}

However, the timer handler unconditionally rearms itself:

drivers/gpu/drm/xe/xe_execlist.c:xe_execlist_port_irq_fail_timer() {
	...
	port->irq_fail.expires = jiffies + msecs_to_jiffies(1000);
	add_timer(&port->irq_fail);
}

If the timer is executing concurrently on another CPU during teardown,
timer_delete() returns immediately without waiting for the handler to finish.
The handler will then execute add_timer() at the end, defeating the deletion. 

When the rearmed timer fires, it will eventually dereference port->hwe->gt.
Since hw_engine_fini() clears hwe->gt to NULL immediately after calling
xe_execlist_port_destroy(), could this result in a NULL pointer dereference?
Furthermore, since the port structure itself will eventually be freed when
the DRM device unbinds, could this lead to a use-after-free if the timer
persists?

Would it be safer to use a state flag, or replace timer_delete() with
timer_delete_sync() or timer_shutdown_sync() to safely tear down the
self-rearming timer?

> +		spin_unlock_irqrestore(&xe->irq.lock, flags);
> +	} else {
> +		hwe->name = NULL;
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012093053.2103743-5-satyanarayana.k.v.p@intel.com?part=1