Re: [PATCH v10 01/27] drm/xe/eudebug: Introduce eudebug interface

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Pending events are inaccessible to the debugger after the target is detached, violating the UAPI contract because the event queue is not drained.
--

commit 048c5c077c45df4be0f59a215d2b70b35b5c5301
Author: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Subject: drm/xe/eudebug: Introduce eudebug interface

This commit introduces the eudebug interface for the Xe driver. It allows 
user-space debuggers to connect, track GPU resources, and listen for resource 
creation and destruction events via a file descriptor.

> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
> new file mode 100644
> index 0000000000000..9fe073f606801
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_eudebug.c
[ ... ]
> +static int event_fifo_pending(struct xe_eudebug *d,
> +			      struct drm_xe_eudebug_event **pending)
> +{
> +	struct drm_xe_eudebug_event *e = d->events.pending;
> +	unsigned int len, copied;
> +
> +	lockdep_assert_held(&d->events.lock);
> +
> +	*pending = NULL;
> +
> +	if (xe_eudebug_detached(d))
> +		return -ENOTCONN;

[Severity: High]
If the target DRM client closes its file descriptor and detaches the debugger 
while there are still unread events in the kfifo (such as VM destruction 
events), does this check prevent the debugger from reading them?

The UAPI documentation explicitly guarantees that -ENOTCONN is returned if the 
debug target is gone and the queue has been drained. Should we allow the queue 
to be drained before returning -ENOTCONN?

> +
> +	if (d->events.pending_occupied) {
> +		*pending = e;
> +		return 0;
> +	}
[ ... ]
> +static long xe_eudebug_read_event(struct xe_eudebug *d,
> +				  const u64 arg,
> +				  const bool wait)
> +{
> +	struct xe_device *xe = d->xe;
> +	struct drm_xe_eudebug_event __user * const user_orig =
> +		u64_to_user_ptr(arg);
> +	struct xe_eudebug *reader __free(reader_active) = NULL;
> +	struct drm_xe_eudebug_event *event_out __free(kvfree) = NULL;
> +	struct drm_xe_eudebug_event user_event;
> +	struct drm_xe_eudebug_event *pending;
> +	long ret = 0;
> +	int pending_len = 0;
> +	int fifo_ret;
> +
> +	if (XE_IOCTL_DBG(xe, copy_from_user(&user_event, user_orig, sizeof(user_event))))
> +		return -EFAULT;
> +
> +	if (XE_IOCTL_DBG(xe, user_event.type != DRM_XE_EUDEBUG_EVENT_READ))
> +		return -EINVAL;
> +
> +	if (XE_IOCTL_DBG(xe, user_event.len < sizeof(*user_orig)))
> +		return -EINVAL;
> +
> +	if (XE_IOCTL_DBG(xe, user_event.flags))
> +		return -EINVAL;
> +
> +	if (XE_IOCTL_DBG(xe, user_event.seqno))
> +		return -EINVAL;
> +
> +	if (XE_IOCTL_DBG(xe, user_event.reserved))
> +		return -EINVAL;
> +
> +	if (XE_IOCTL_DBG(xe, xe_eudebug_detached(d)))
> +		return -ENOTCONN;

[Severity: High]
Similarly, this short-circuits the read ioctl entirely if the target is 
detached. Can this be modified to check if the queue is empty first, ensuring 
the debugger can pull any pending events leading up to the target's termination?

> +
> +	if (test_and_set_bit_lock(XE_EUDEBUG_READER_ACTIVE, &d->flags))
> +		return -EBUSY;
> +
> +	reader = d;
[ ... ]

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