Re: [PATCH v10 15/27] drm/xe/eudebug: Introduce EU control interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect hardware engine (hwe) used to read RING_CURRENT_LRCA for virtual exec queues, causing valid EU interruption to fail with -EBUSY.
- [High] Calling `xe_eudebug_exec_queue_get` triggers a use-after-free because the debugger handle map stores raw pointers to queues that can be freed when the target process exits.
--

commit ae12753c8db49bf5356301911889cee3d0b8fe63
Author: Dominik Grzegorzek <dominik.grzegorzek@intel.com>

    drm/xe/eudebug: Introduce EU control interface

This commit introduces the EU control interface, allowing the EU debugger
to interrupt, resume, and query the state of EU threads. It provides an
abstraction layer for the GuC to hook into in the future.

> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
> index 188a4c60513c5..ecd6d4c5d63ca 100644
> --- a/drivers/gpu/drm/xe/xe_eudebug.c
> +++ b/drivers/gpu/drm/xe/xe_eudebug.c
[ ... ]
> @@ -738,6 +741,33 @@ struct xe_vm *xe_eudebug_vm_get(struct xe_eudebug *d, u32 id)
>  	return vm;
>  }
>  
> +struct xe_exec_queue *xe_eudebug_exec_queue_get(struct xe_eudebug *d, u32 id)
> +{
> +	struct xe_eudebug_resource *r =
> +		resource_from_type(d, XE_EUDEBUG_RES_TYPE_EXEC_QUEUE);
> +	struct xe_exec_queue *eq;
> +
> +	mutex_lock(&r->lock);
> +	eq = find_resource__unlocked(r, id);
> +	if (eq)
> +		xe_exec_queue_get(eq);
> +	mutex_unlock(&r->lock);
> +
> +	return eq;
> +}

[Severity: High]
Does this code introduce a use-after-free risk during target process exit?

The debugger handle map (d->target.res) stores raw pointers to target
resources like exec queues. When the target process closes its DRM fd,
xe_file_close() sets d->target.xef = NULL via xe_eudebug_file_close(), and
proceeds to drop references to all target exec queues, freeing them.

However, the raw pointers remain in the debugger's resource map until the
debugger itself is closed. If a debugger races with the target process
exit and invokes the DRM_XE_EUDEBUG_IOCTL_EU_CONTROL ioctl, it can pass
the xe_eudebug_detached() check before the target clears d->target.xef.
Then do_eu_control() calls xe_eudebug_exec_queue_get(), which retrieves the
freed pointer and calls xe_exec_queue_get(eq), performing a kref_get() on
freed memory.

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_eudebug_hw.c b/drivers/gpu/drm/xe/xe_eudebug_hw.c
> index aa31b4c917133..d14e4e4775f2f 100644
> --- a/drivers/gpu/drm/xe/xe_eudebug_hw.c
> +++ b/drivers/gpu/drm/xe/xe_eudebug_hw.c
[ ... ]
> +struct xe_exec_queue *
> +xe_gt_runalone_active_queue_get(struct xe_gt *gt, int *lrc_idx)
> +{
> +	struct xe_hw_engine *active;
> +
> +	active = get_runalone_active_hw_engine(gt);
> +	if (IS_ERR(active))
> +		return ERR_CAST(active);
> +
> +	return active_hwe_to_exec_queue(active, lrc_idx);
> +}

[Severity: High]
Does returning only the exec queue here discard the specific active
physical engine context? 

This discovers the correct active hardware engine but discards it by
returning only the queue, removing context necessary for subsequent hw
access. For virtual queues, the physical engine where the queue is actively
running might not be the primary assigned engine. When
active_hwe_to_exec_queue() returns the queue, the knowledge of which
physical engine it was found on is lost.

[ ... ]
> +static int xe_eu_control_interrupt_all(struct xe_eudebug *d,
> +				       struct xe_exec_queue *active,
> +				       struct xe_lrc *lrc)
> +{
> +	unsigned int fw_ref = 0;
> +	u32 lrc_hw, lrc_hw_post;
> +	u32 td_ctl;
> +	int err = -EINVAL;
> +
> +	fw_ref = xe_force_wake_get(gt_to_fw(active->gt), active->hwe->domain);
> +	if (!fw_ref) {
> +		err = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	/* Additional check just before issuing MMIO writes */
> +	err = read_hw_lrca_fw(active->hwe, &lrc_hw);
> +	if (err)
> +		goto out;
> +
> +	if (!lrca_equals(lower_32_bits(xe_lrc_descriptor(lrc)), lrc_hw)) {
> +		err = -EBUSY;
> +		goto out;
> +	}

[Severity: High]
Is active->hwe always the correct physical engine to read
RING_CURRENT_LRCA from?

When do_eu_control() passes a virtual exec queue into this function,
active->hwe represents the primary assigned engine rather than the actual
physical engine where the runalone workload is currently executing. This
can cause read_hw_lrca_fw(active->hwe, ...) to target the wrong physical
engine, making the LRCA check fail and erroneously return -EBUSY, preventing
the EU interruption from succeeding on virtual queues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903145952.848051-1-mika.kuoppala@linux.intel.com?part=15