Re: [PATCH v3 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:
- [Critical] drm/xe: NULL Pointer Dereference in xe_execlist_port_destroy and timer due to early hwe->gt clearing
--

commit 0f76e8a7471aad3de71482c37a1644b32699ef4e
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 fixes a NULL pointer dereference by invalidating the engine name
and clearing hwe->gt earlier in the teardown process. However, this
reordering appears to introduce a new issue where xe_execlist_port_destroy()
encounters a NULL pointer.

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 9680d2a5adec7..3a6b9a4fe63ff 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;
> +
> +		spin_lock_irqsave(&xe->irq.lock, flags);
> +		hwe->name = NULL;
> +		hwe->gt = NULL;
> +		spin_unlock_irqrestore(&xe->irq.lock, flags);
> +	} else {
> +		hwe->name = NULL;
> +	}
>  
>  	if (hwe->exl_port)
>  		xe_execlist_port_destroy(hwe->exl_port);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Critical]
Will this call to xe_execlist_port_destroy() cause a kernel panic now that
hwe->gt has been cleared to NULL?

Looking at xe_execlist_port_destroy() in drivers/gpu/drm/xe/xe_execlist.c, it
dereferences port->hwe->gt:

void xe_execlist_port_destroy(struct xe_execlist_port *port)
{
	timer_delete(&port->irq_fail);

	/* Prevent an interrupt while we're destroying */
	spin_lock_irq(&gt_to_xe(port->hwe->gt)->irq.lock);
	...
}

Because hwe->gt is set to NULL right before this call, gt_to_xe(NULL) will
dereference a NULL pointer (specifically gt_to_tile(NULL)->xe) when acquiring
the spinlock.

This problem wasn't introduced by this patch, but doesn't calling
timer_delete() instead of timer_delete_sync() on port->irq_fail allow the
timer to execute concurrently during or after destruction? If the timer
fires and calls read_execlist_status(port->hwe) which also accesses hwe->gt,
would it hit the same NULL pointer dereference now that the pointer is cleared
earlier?

> -
> -	hwe->gt = NULL;
>  }

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