Re: [PATCH v2 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] drm/xe: TOCTOU NULL pointer dereference in DRM scheduler timeout handler during engine teardown
--

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

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

This commit clears the engine name during teardown to ensure that subsequent
hardware engine scans skip the invalidated engine. This prevents a NULL pointer
dereference in the VF memory IRQ handler when accessing torn-down engines.

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

[Severity: High]
While the new lock protects the IRQ handler, can a lockless reader like the DRM
scheduler timeout handler still encounter a time-of-check to time-of-use race?

If the timeout handler fires after a device unplug, it locklessly iterates over
the engines:

drivers/gpu/drm/xe/xe_gt.c:xe_gt_hw_engine() {
    ...
    for_each_hw_engine(hwe, gt, id)
        if (hwe->class == class && ...
    ...
}

The for_each_hw_engine iteration evaluates xe_hw_engine_is_valid, which checks
hwe->name. If this lockless check occurs right before hw_engine_fini clears
hwe->name, the timeout handler will proceed with the engine pointer.

Later in hw_engine_fini, hwe->gt is set to NULL. The timeout handler could
then dereference the null pointer when it calls into the LRC code:

drivers/gpu/drm/xe/xe_lrc.c:xe_lrc_get_multi_queue_active_queue_id() {
    ...
    u32 val = xe_mmio_read32(&hwe->gt->mmio,
                             RING_CSMQDEBUG(hwe->mmio_base));
    ...
}

Does the lockless reader need additional synchronization to ensure hwe->gt
is safely accessed?

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