Re: [PATCH 17/20] drm/xe/eudebug: Introduce EU control interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] drm/xe/eudebug: missing wedged and unplug guards in
  do_eu_control()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] drm/xe/oa: unbalanced de-whitelisting in
  xe_oa_stream_open_ioctl_locked() [1]
--

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

drm/xe/eudebug: Introduce EU control interface

This commit adds the DRM_XE_EUDEBUG_IOCTL_EU_CONTROL ioctl to let an EU
debugger interrupt, query, and resume EU threads on debuggable execution
queues, and makes eudebug and OA streams mutually exclusive.

> diff --git a/drivers/gpu/drm/xe/xe_eudebug_hw.c b/drivers/gpu/drm/xe/xe_eudebug_hw.c
> index 7388facfe0a5d..3c025f279488f 100644
> --- a/drivers/gpu/drm/xe/xe_eudebug_hw.c
> +++ b/drivers/gpu/drm/xe/xe_eudebug_hw.c
[ ... ]
> +static int do_eu_control(struct xe_eudebug *d,
> +			 const struct drm_xe_eudebug_eu_control * const arg,
> +			 struct drm_xe_eudebug_eu_control __user * const user_ptr)
> +{
[ ... ]
> +	lrc = xe_eudebug_find_lrc(d, arg->lrc_handle);
> +	if (XE_IOCTL_DBG(xe, !lrc)) {
> +		ret = -EINVAL;
> +		goto queue_put;
> +	}
> +
> +	if (!xe_pm_runtime_get_if_active(xe)) {
> +		ret = -EIO;
> +		goto queue_put;
> +	}
> +
> +	active = xe_gt_runalone_active_queue_get(q->gt, &active_hwe, &lrc_idx);
> +	if (XE_IOCTL_DBG(xe, IS_ERR(active))) {
> +		ret = PTR_ERR(active);
> +		goto out_pm;
> +	}

[Severity: Medium]
Can do_eu_control() race with PCI device unbind or run on a wedged device
to dereference a NULL hwe->gt or access unmapped MMIO registers?

Unlike DRM character device ioctls, DRM_XE_EUDEBUG_IOCTL_EU_CONTROL is
dispatched directly on the anonymous [xe_eudebug] file, which holds a
drm_dev_get() reference that keeps struct xe_device alive after PCI
unbind.  xe_pm_runtime_get_if_active(xe) does not stop callers during or
after unbind because xe_pm_runtime_fini() calls pm_runtime_get_sync() and
pm_runtime_forbid(), leaving runtime PM active while devres tears down
the device:

CPU0                                  CPU1
do_eu_control()
  xe_pm_runtime_get_if_active(xe)
  ...
  memdup_user() / usleep_range()
                                      xe_pci_remove()
                                        drm_dev_unplug(&xe->drm)
                                        ...
                                        devres_release_all()
                                          hw_engine_fini()
                                            hwe->gt = NULL
                                          pcim_iomap_release()
  check_lrc_active(active_hwe, lrc)
    read_hw_lrca(hwe, &lrc_hw)
      gt_to_fw(hwe->gt)  <-- NULL pointer dereference

If hwe->gt is instead cleared during get_runalone_active_hw_engine(),
engine_status() dereferences hwe->gt as well, and after mmio_fini() clears
xe->mmio.regs, MMIO accesses trigger device warnings.  Sibling anonymous
fd callbacks in drivers/gpu/drm/xe/xe_eudebug_vm.c guard against both
cases via vm_file_enter(), which checks xe_wedged(xe) and wraps hardware
accesses in drm_dev_enter() / drm_dev_exit().

> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> index fe91abaccb14a..52b3f342344a3 100644
> --- a/drivers/gpu/drm/xe/xe_oa.c
> +++ b/drivers/gpu/drm/xe/xe_oa.c
[ ... ]
> @@ -1880,6 +1914,21 @@ static int xe_oa_stream_open_ioctl_locked(struct xe_oa *oa,
>  		goto exit;
>  	}
>  
> +	/*
> +	 * eudebug and OA both change EU and engine state that the other
> +	 * relies on, such as the debug registers and workarounds eudebug
> +	 * keeps in hwe->reg_sr and the settings OA changes and restores
> +	 * around a stream. They do not have to run at the same time, so
> +	 * refuse rather than interleave. xe_eudebug_enable() checks the
> +	 * other direction with xe_oa_stream_active(), under the same
> +	 * gt_lock held here.
> +	 */
> +	if (xe_eudebug_is_enabled_oa(param->hwe->gt)) {
> +		drm_dbg(&oa->xe->drm, "OA not available while eudebug is enabled\n");
> +		ret = -EBUSY;
> +		goto exit;
> +	}

[Severity: High]
This problem wasn't introduced by this patch, but can
xe_oa_stream_open_ioctl_locked() cause an unbalanced
xe_reg_dewhitelist_oa_regs() call [1] when opening a stream with
DRM_XE_OA_PROPERTY_SAMPLE_OA fails?

xe_oa_stream_init() sets stream->sample = param->sample, but
xe_reg_whitelist_oa_regs() is only called after xe_oa_enable_locked() and
anon_inode_getfd() both succeed:

drivers/gpu/drm/xe/xe_oa.c:xe_oa_stream_open_ioctl_locked() {
    ...
	if (!param->disabled) {
		ret = xe_oa_enable_locked(stream);
		if (ret)
			goto err_destroy;
	}

	stream_fd = anon_inode_getfd("[xe_oa]", &xe_oa_fops, stream, 0);
	if (stream_fd < 0) {
		ret = stream_fd;
		goto err_disable;
	}

	if (stream->sample)
		xe_reg_whitelist_oa_regs(stream->gt);
    ...
err_disable:
	if (!param->disabled)
		xe_oa_disable_locked(stream);
err_destroy:
	xe_oa_stream_destroy(stream);
    ...
}

If anon_inode_getfd() fails (for example with -EMFILE) or
xe_oa_enable_locked() fails, xe_oa_stream_destroy() sees stream->sample
set and calls xe_reg_dewhitelist_oa_regs(stream->gt) without a matching
xe_reg_whitelist_oa_regs().  That triggers xe_assert() and underflows
gt->oa.whitelist_count from 0 to U32_MAX (or drops another active OA
unit's whitelist reference on the same GT).

[1] https://sashiko.dev/#/bug/linux-44e618e3-ff9f-4811-9ea1-86af519616cc

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