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

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Satyanarayana K V P <satyanarayana.k.v.p@intel.com> wrote on pon [2026-paź-12 05:50:55 +0000]:
> The VF memory IRQ handler iterates over HW engines whose names are set.
> Engine teardown clears hwe->gt but leaves hwe->name set, so the handler
> can select an engine whose GT has already been invalidated. This can
> cause a NULL pointer dereference in xe_hw_engine_handle_irq().
> 
> Invalidate the engine name during teardown so subsequent engine scans
> skip it.
> 
> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Fixes: 6eb2aad402cc ("drm/xe: Move hw_engine_fini to devm managed")
> ---
> V3 -> V4:
> - Fix issue with xe_execlist_port_destroy() with V3 version (Sashiko).
> 
> V2 -> V3:
> - Updated spin_lock_irq() to spin_lock_irqsave().
> 
> V1 -> V2:
> - Fix race with xe_memirq_handler() and hw_engine_fini() (Sashiko & Piotr).
> ---
>  drivers/gpu/drm/xe/xe_hw_engine.c | 17 ++++++++++++++---
>  drivers/gpu/drm/xe/xe_irq.c       |  3 +++
>  drivers/gpu/drm/xe/xe_memirq.c    |  3 +++
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> index 265f95c0c899..55ee0c892c1e 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;
> +		spin_unlock_irqrestore(&xe->irq.lock, flags);
> +	} else {
> +		hwe->name = NULL;
> +	}

NIT: I don't think the !gt branch can ever be taken. hwe->gt is set in
hw_engine_init_early(), long before hw_engine_init() registers
hw_engine_fini() via devm_add_action_or_reset(). Both the normal devm
release and the _or_reset error path therefore always see a valid
hwe->gt, and the original code relied on that as well
(xe_execlist_port_destroy() dereferences port->hwe->gt unconditionally).

>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
> index 9e49e2241da4..b394c8edf89c 100644
> --- a/drivers/gpu/drm/xe/xe_irq.c
> +++ b/drivers/gpu/drm/xe/xe_irq.c
> @@ -908,6 +908,7 @@ static irqreturn_t xe_irq_msix_default_hwe_handler(int irq, void *arg)
>  	struct xe_hw_engine *hwe;
>  	enum xe_hw_engine_id id;
>  	struct xe_tile *tile;
> +	unsigned long flags;
>  	struct xe_gt *gt;
>  
>  	if (!atomic_read(&xe->irq.enabled))
> @@ -922,8 +923,10 @@ static irqreturn_t xe_irq_msix_default_hwe_handler(int irq, void *arg)
>  			if (gt->tile != tile)
>  				continue;
>  
> +			spin_lock_irqsave(&xe->irq.lock, flags);
>  			for_each_hw_engine(hwe, gt, id)
>  				xe_memirq_hwe_handler(memirq, hwe);
> +			spin_unlock_irqrestore(&xe->irq.lock, flags);
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/xe/xe_memirq.c b/drivers/gpu/drm/xe/xe_memirq.c
> index 9dfe965cb46e..1073132f2ded 100644
> --- a/drivers/gpu/drm/xe/xe_memirq.c
> +++ b/drivers/gpu/drm/xe/xe_memirq.c
> @@ -555,6 +555,7 @@ void xe_memirq_handler(struct xe_memirq *memirq)
>  	struct xe_hw_engine *hwe;
>  	enum xe_hw_engine_id id;
>  	struct iosys_map map;
> +	unsigned long flags;
>  	unsigned int gtid;
>  	struct xe_gt *gt;
>  
> @@ -568,8 +569,10 @@ void xe_memirq_handler(struct xe_memirq *memirq)
>  		if (gt->tile != tile)
>  			continue;
>  
> +		spin_lock_irqsave(&xe->irq.lock, flags);
>  		for_each_hw_engine(hwe, gt, id)
>  			xe_memirq_hwe_handler(memirq, hwe);
> +		spin_unlock_irqrestore(&xe->irq.lock, flags);
>  	}
>  
>  	/* GuC and media GuC (if present) must be checked separately */
> -- 
> 2.43.0
> 

LGTM, one NIT above:
Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

--