Re: [PATCH 12/20] drm/xe/eudebug: Add ufence events with acks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] drm/xe/eudebug: use of destroyed eudebug.lock in
  xe_eudebug_detach()
- [Low] drm/xe/eudebug: TOCTOU race in xe_eudebug_ack_event_ioctl() on
  detach
--

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

drm/xe/eudebug: Add ufence events with acks

This commit adds DRM_XE_EUDEBUG_EVENT_VM_BIND_UFENCE events and the
DRM_XE_EUDEBUG_IOCTL_ACK_EVENT ioctl so that a debugger can stall user
fence signalling on a VM bind with debug data until the event is
acknowledged.

> diff --git a/drivers/gpu/drm/xe/xe_eudebug.c b/drivers/gpu/drm/xe/xe_eudebug.c
> index c6d08b27042d2..934dc116120e6 100644
> --- a/drivers/gpu/drm/xe/xe_eudebug.c
> +++ b/drivers/gpu/drm/xe/xe_eudebug.c

[ ... ]

> @@ -1199,6 +1330,191 @@ static int vm_bind_op_event(struct xe_eudebug *d,
>  	return err;
>  }

[ ... ]

> +static int track_ufence(struct xe_eudebug *d,
> +			struct xe_user_fence *ufence,
> +			u64 bind_ref_seqno,
> +			bool *tracked)
> +{

[ ... ]

> +	/*
> +	 * The ack and the event asking for it go in under one lock, so the
> +	 * debugger cannot ack a fence it has not been told about. Once the
> +	 * lock is dropped the ack may already be gone, so it is not touched
> +	 * after that.
> +	 */
> +	mutex_lock(&d->lock);
> +	old = rb_find_add(&ack->rb_node, &d->acks.tree, ack_insert_cmp);
> +	if (old) {
> +		mutex_unlock(&d->lock);
> +		xe_sync_ufence_put(ufence);
> +		kfree(ack);
> +		return -EEXIST;
> +	}
> +
> +	*tracked = true;
> +
> +	event = xe_eudebug_prepare_event(d, DRM_XE_EUDEBUG_EVENT_VM_BIND_UFENCE,
> +					 seqno, flags, sz);
> +
> +	e = cast_event(e, event);
> +	e->vm_bind_ref_seqno = bind_ref_seqno;
> +
> +	ret = xe_eudebug_queue_event(d, event);
> +	mutex_unlock(&d->lock);
> +
> +	return ret;
> +}

[ ... ]

> +bool xe_eudebug_ufence_track(struct xe_user_fence *ufence)
> +{
> +	struct xe_eudebug *d;
> +	u64 bind_ref_seqno;
> +	bool tracked = false;
> +	int ret;

[ ... ]

> +	if (xe_eudebug_is_detached(d)) {
> +		/* Nothing will ack this fence any more */
> +		ufence_drop_debugger(ufence);
> +		xe_eudebug_put(d);
> +		return false;
> +	}
> +
> +	ret = track_ufence(d, ufence, bind_ref_seqno, &tracked);
> +	if (ret) {
> +		/*
> +		 * If we failed before the ack made it into the tree, the
> +		 * disconnect below has nothing to release for this fence.
> +		 */
> +		if (!tracked)
> +			ufence_drop_debugger(ufence);
> +
> +		xe_eudebug_disconnect(d, ret);
> +	}
> +
> +	xe_eudebug_put(d);
> +
> +	return tracked;
> +}

[Severity: Medium]
Can xe_eudebug_ufence_track() lock a destroyed d->xe->eudebug.lock in
xe_eudebug_detach()?

As noted above kick_ufence_worker(), user_fence_worker() runs on
xe->ordered_wq, which xe_device_destroy() drains only after the drmm
actions registered in xe_device_probe() have already run.  One of those
actions is xe_eudebug_lock_fini(), which destroys xe->eudebug.lock:

drivers/gpu/drm/xe/xe_eudebug.c:xe_eudebug_lock_fini() {
	mutex_destroy(&to_xe_device(dev)->eudebug.lock);
}

A user fence holds a reference to struct xe_eudebug in
ufence->eudebug.debugger, but not a drm_device reference:

CPU0 (user_fence_worker)          CPU1
xe_eudebug_ufence_track()
  xe_eudebug_is_detached(d)
    -> false
                                  xe_eudebug_release()
                                    xe_eudebug_disconnect(d, 0)
                                    drm_dev_put(&d->xe->drm)
                                      ...
                                      xe_eudebug_lock_fini()
                                        mutex_destroy(&xe->eudebug.lock)
                                      xe_device_destroy()
                                        destroy_workqueue(xe->ordered_wq)
  track_ufence()
    xe_eudebug_queue_event()
      -> -ENOTCONN
  xe_eudebug_disconnect(d, ret)
    xe_eudebug_detach()
      mutex_lock(&d->xe->eudebug.lock)  <-- destroyed mutex

With lock debugging enabled, mutex_lock() warns on the destroyed mutex.
If track_ufence() checked __xe_eudebug_is_detached(d) under d->lock
before inserting ack into d->acks.tree, a racing detach would not need a
second xe_eudebug_disconnect() call.

[ ... ]

> @@ -1691,6 +2032,42 @@ static long xe_eudebug_read_event(struct xe_eudebug *d,
>  	return ret;
>  }
>  
> +static long
> +xe_eudebug_ack_event_ioctl(struct xe_eudebug *d,
> +			   const u64 arg)
> +{

[ ... ]

> +	if (XE_IOCTL_DBG(xe, xe_eudebug_is_detached(d)))
> +		return -ENOTCONN;
> +
> +	ack = remove_ack(d, user_arg.seqno);
> +	if (XE_IOCTL_DBG(xe, !ack))
> +		return -EINVAL;
> +
> +	handle_ack(d, ack, false);
> +
> +	return 0;
> +}

[Severity: Low]
Can xe_eudebug_ack_event_ioctl() return -EINVAL instead of -ENOTCONN when
racing with xe_eudebug_detach()?

xe_eudebug_is_detached() and remove_ack() take d->lock in separate
critical sections.  If xe_eudebug_detach() runs in between, it clears
d->target.xef and empties d->acks.tree under d->lock, so remove_ack()
finds nothing and the ioctl returns -EINVAL for a valid seqno instead of
the documented -ENOTCONN.  Checking __xe_eudebug_is_detached(d) inside
remove_ack() under d->lock would avoid the race.

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