Re: [PATCH v5 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] drm/xe: NULL pointer dereference due to incomplete locking in hw_engine_fini
--

commit 888f9d1be166404e1216f8d0d23505412e283b23
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 fixes a NULL pointer dereference by invalidating the engine
name and GT pointer during teardown in hw_engine_fini() under the
irq.lock, preventing the VF memory IRQ handler from scanning it.

> 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]
Can this still cause a NULL pointer dereference for lockless IOCTL callers?

Unprivileged userspace can call the DRM_IOCTL_XE_DEVICE_QUERY ioctl
(specifically query_engine_cycles) concurrently with device unbind or unplug.
Since the ioctl is not guarded by drm_dev_enter(), it can proceed
concurrently with devres_release_all() and hw_engine_fini().

The ioctl looks up the hardware engine via xe_gt_hw_engine() in
query_engine_cycles():

drivers/gpu/drm/xe/xe_query.c:query_engine_cycles() {
    ...
    hwe = xe_gt_hw_engine(gt, user_to_xe_engine_class[eci->engine_class],
                          eci->engine_instance, true);
    if (!hwe)
        return -EINVAL;
    ...
}

Immediately after this check, hw_engine_fini() can execute and clear
hwe->gt = NULL (guarded only by xe->irq.lock, which the ioctl does not hold).

The ioctl then calls hwe_read_timestamp(), which dereferences hwe->gt:

drivers/gpu/drm/xe/xe_query.c:hwe_read_timestamp() {
    struct xe_mmio *mmio = &hwe->gt->mmio;
    ...
}

Does this lockless path need additional protection against concurrent
teardown?

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

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