Re: [PATCH 1/1] 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] xe: TOCTOU race in hw_engine_fini() allows NULL pointer dereference despite hwe->name invalidation
--

commit 582041e3254f04e73ce6e909225432f284799ee4
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 attempts to fix a NULL pointer dereference in the VF memory IRQ
handler by clearing hwe->name before hwe->gt during engine teardown, aiming
to have the handler skip the torn-down engine.

> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 9680d2a5adec7..2267249c26894 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -279,6 +279,7 @@ static void hw_engine_fini(void *arg)
>  	if (hwe->exl_port)
>  		xe_execlist_port_destroy(hwe->exl_port);
>  
> +	hwe->name = NULL;
>  	hwe->gt = NULL;

[Severity: High]
Does this leave a Time-Of-Check to Time-Of-Use (TOCTOU) race window that
still allows a NULL pointer dereference?

Because xe_irq_install() is called before xe_gt_init() in xe_device_probe(),
and devm actions execute in LIFO order, hw_engine_fini() can run while
hardware interrupts are still active.

If an interrupt fires concurrently with teardown, the VF memory IRQ handler
xe_memirq_handler() iterates locklessly and can observe a valid hwe->name
via xe_hw_engine_is_valid():

drivers/gpu/drm/xe/xe_memirq.c:xe_memirq_handler() {
    ...
    for_each_hw_engine(hwe, gt, id)
        xe_memirq_hwe_handler(memirq, hwe);
    ...
}

Concurrently, hw_engine_fini() clears both hwe->name and hwe->gt without any
locks or memory barriers. The IRQ handler then proceeds to dereference the
now-NULL hwe->gt:

drivers/gpu/drm/xe/xe_hw_engine.c:xe_hw_engine_handle_irq() {
    ...
    wake_up_all(&gt_to_xe(hwe->gt)->ufence_wq);
    ...
}

Furthermore, the lack of memory barriers (such as WRITE_ONCE and
smp_store_release) allows the CPU or compiler to reorder the nullification
of name and gt, widening the race window.

Is proper synchronization (such as synchronize_irq(), locking, or disabling
IRQs prior to engine teardown) required here to safely close the race?

>  }
>

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