Re: [PATCH v4 7/7] drm/i915/gt: Use signalers_lock to prevent starvation of irq_work.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Dropping `ce->signal_lock` to avoid lock inversion in `i915_request_enable_breadcrumb` allows concurrent threads to double-add a context to the `b->signalers` list, corrupting the list and leaking `intel_context` references.
--

commit 51101492996df475bcd91b7f36bd0af9680441f1
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

drm/i915/gt: Use signalers_lock to prevent starvation of irq_work.

This patch adds a global spinlock to prevent starvation of irq_work by the
threaded interrupt and removes the existing RCU handling.

> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index c10ac0ab3bfa8..0ae7759dc74a3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> @@ -88,24 +88,25 @@ static void add_signaling_context(struct intel_breadcrumbs *b,
>  				  struct intel_context *ce)
>  {
>  	lockdep_assert_held(&ce->signal_lock);
> +	lockdep_assert_held(&b->signalers_lock);
>  
> -	spin_lock(&b->signalers_lock);
> -	list_add_rcu(&ce->signal_link, &b->signalers);
> -	spin_unlock(&b->signalers_lock);
> +	if (list_empty(&ce->signals))
> +		return;
> +
> +	intel_context_get(ce);
> +	list_add(&ce->signal_link, &b->signalers);
>  }

[Severity: Critical]
Could this unconditionally corrupt the signalers list? 

If add_signaling_context() is called concurrently for the same context, it
will call list_add() without verifying whether ce->signal_link is already
linked, leading to list corruption and a leak of intel_context objects.

[ ... ]

> @@ -427,30 +424,87 @@ bool i915_request_enable_breadcrumb(struct i915_request *rq)
>  		return true;
>  
>  	spin_lock(&ce->signal_lock);
> +	b = READ_ONCE(rq->engine)->breadcrumbs;
> +
>  	if (test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags))
> -		insert_breadcrumb(rq);
> +		add_context = insert_breadcrumb(rq, b);
> +
> +	if (add_context && spin_trylock(&b->signalers_lock)) {
> +		add_signaling_context(b, ce);
> +		spin_unlock(&b->signalers_lock);
> +		add_context = false;
> +	}
>  	spin_unlock(&ce->signal_lock);
>  
> +	if (add_context) {
> +		/*
> +		 * Fast trylock didn't work, use slow locking.
> +		 *
> +		 * Dropping the lock to solve the inversion is safe, since
> +		 * no race is possible against remove_signaling_context()
> +		 * without being added as signaling context.
> +		 */
> +		spin_lock(&b->signalers_lock);
> +		spin_lock(&ce->signal_lock);
> +		add_signaling_context(b, ce);
> +		spin_unlock(&ce->signal_lock);
> +		spin_unlock(&b->signalers_lock);
> +	}

[Severity: Critical]
Does dropping ce->signal_lock here create a race condition that leads to the
double-add in add_signaling_context()?

If multiple threads attempt to enable breadcrumbs for the same context and
fail the fast trylock, they will drop ce->signal_lock and enter the slow path.
This can allow a sequence where:

Thread 1 calls i915_request_enable_breadcrumb() for rq1, gets
add_context = true, fails the trylock, drops ce->signal_lock, and waits
on the slow path.

Thread 2 cancels rq1, making ce->signals empty again.

Thread 3 calls i915_request_enable_breadcrumb() for rq3. Because ce->signals
is empty, it also gets add_context = true. It fails the trylock, drops the
lock, and waits on the slow path.

Thread 1 and Thread 3 sequentially acquire the locks in the slow path and both
call add_signaling_context(). Since Thread 3 added rq3, ce->signals is not
empty. Both threads will then execute intel_context_get() and list_add()
unconditionally, permanently corrupting the doubly-linked list.

Should add_signaling_context() verify that list_empty(&ce->signal_link) is
true before calling list_add() to prevent this regression?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706114823.752313-1-dev@lankhorst.se?part=7