Re: [PATCH v10 04/27] drm/xe/eudebug: Introduce discovery for resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Global ordered workqueue stall via copy_from_user block while holding ioctl_lock.
- [Medium] Event sequence numbers (seqno) can be queued out-of-order in the event stream due to a race between seqno generation and event queuing.
--

commit e2061e72d8e954a17bde35e9c970ee41bc2db0ad
Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>

    drm/xe/eudebug: Introduce discovery for resources

This commit introduces a resource discovery mechanism for debugger connections.
When a debugger connects, all existing resources for the target client are
relayed as events via an ordered workqueue, and DRM ioctls are blocked using
an rw_semaphore until discovery is complete.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 032a7ec5fbc8d..f10959b898297 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
[ ... ]
> @@ -238,8 +239,12 @@ static long xe_drm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  
>  	ACQUIRE(xe_pm_runtime_ioctl, pm)(xe);
>  	ret = ACQUIRE_ERR(xe_pm_runtime_ioctl, &pm);
> -	if (ret >= 0)
> +	if (ret >= 0) {
> +		bool lock = xe_eudebug_discovery_lock(file, cmd);
>  		ret = drm_ioctl(file, cmd, arg);

[Severity: High]
Does holding the discovery lock across drm_ioctl() create a vector for a
workqueue stall? 

If an unprivileged user maps the ioctl argument pointer to a blocking FUSE
or userfaultfd region, drm_ioctl() can be stalled indefinitely during
copy_from_user() while holding xef->eudebug.ioctl_lock for read.

> +		if (lock)
> +			xe_eudebug_discovery_unlock(file, cmd);
> +	}
>  
>  	return ret;
>  }
[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
> index 835dbd7309af5..7a5b14346e059 100644
> --- a/drivers/gpu/drm/xe/xe_eudebug.c
> +++ b/drivers/gpu/drm/xe/xe_eudebug.c
[ ... ]
> @@ -659,6 +705,87 @@ void xe_eudebug_vm_destroy(struct xe_file *xef, struct xe_vm *vm)
>  	xe_eudebug_put(d);
>  }
>  
> +static struct xe_file *xe_eudebug_target_get(struct xe_eudebug *d)
> +{
[ ... ]
> +static void discover_client(struct xe_eudebug *d)
> +{
> +	struct xe_file *xef;
> +	struct xe_vm *vm;
> +	unsigned long i;
> +	unsigned int vm_count = 0;
> +	int err = 0;
> +
> +	xef = xe_eudebug_target_get(d);
> +	if (!xef) {
> +		complete_all(&d->discovery);
> +		return;
> +	}
> +
> +	/*
> +	 * An xe_eudebug ref is taken for the discovery worker, which holds a
> +	 * ref to the target xe_file; the xe_file in turn holds the vm and
> +	 * exec_queue refs.
> +	 *
> +	 * All the relevant ioctls that manipulate the xe_file resources eudebug
> +	 * cares about are gated behind down_read(&xef->eudebug.ioctl_lock).
> +	 * Thus taking the write lock excludes all those ioctls, which lets us
> +	 * peek into the resource lists without taking their individual locks.
> +	 */
> +	down_write(&xef->eudebug.ioctl_lock);

[Severity: High]
If the target process is blocked in copy_from_user() inside drm_ioctl()
while holding the read lock, will this down_write() block indefinitely?

Because discovery runs on a global ordered workqueue (configured below in
xe_eudebug_init), this single blocked work item could stall all eudebug
discovery operations system-wide, causing a Denial of Service for all
debugger connections.

> +
> +	if (xe_eudebug_detached(d))
> +		goto out;
[ ... ]
> @@ -1126,12 +1266,20 @@ void xe_eudebug_init_early(struct xe_device *xe)
>  void xe_eudebug_init(struct xe_device *xe)
>  {
>  	struct drm_device *dev = &xe->drm;
> +	struct workqueue_struct *wq;
>  	int err;
>  
>  	/* early init failed */
>  	if (xe->eudebug.cap_state == XE_EUDEBUG_CAP_NOT_SUPPORTED)
>  		return;
>  
> +	wq = drmm_alloc_ordered_workqueue(dev, "xe-eudebug-discovery", 0);

[Severity: High]
This allocates the workqueue as ordered (max_active=1), which means a single
stalled work item blocks all other discovery work.

Can the workqueue be allocated differently, or can the ioctl_lock be managed
to avoid wrapping the copy_from_user() operations in drm_ioctl()?

> +	if (IS_ERR(wq)) {
> +		err = PTR_ERR(wq);
> +		goto out_err;
> +	}
> +	xe->eudebug.discovery_wq = wq;
> +
>  	err = sysfs_create_file(&dev->dev->kobj,
>  				&dev_attr_enable_eudebug.attr);
>  	if (err)
[ ... ]

[Severity: Medium]
Can sequence numbers be queued out-of-order in the event stream?

Looking at send_vm_bind_event() in xe_eudebug.c:

	*seqno = atomic_long_inc_return(&d->events.seqno);

	spin_lock(&d->events.lock);
	event = xe_eudebug_prepare_event(d, DRM_XE_EUDEBUG_EVENT_VM_BIND,
					 *seqno, base_flags, sz);

Since the sequence number is incremented atomically outside of the FIFO
spinlock, could two threads acquire sequence numbers N and N+1, but acquire
the FIFO lock in reverse order?

This same pattern appears in _xe_eudebug_add_handle(). Pushing N+1 before
N would break the expected monotonicity of the event stream and could cause
the debugger to drop events.

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