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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Double list_add() on ce->signal_link causing list corruption and a memory leak due to missing state checks during lock reacquisition.
--

--- Patch [1]: [PATCH] drm/i915/gt: Use signalers_lock to prevent starvation of irq_work. ---
commit c467298effeb39850e67f70de02e5adc9ad9e9fb
Author: Maarten Lankhorst <[email protected]>

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

This commit introduces a global spinlock (signalers_lock) to prevent irq_work
starvation and removes RCU handling to avoid PREEMPT_RT issues and request
lifetime UAFs.

> 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: High]
Can this cause a double list_add() on ce->signal_link, leading to list
corruption and a leaked intel_context reference?

If add_signaling_context() is called without verifying if ce->signal_link is
already linked (e.g., by checking !list_empty(&ce->signal_link)), it might act
as a nexus for list corruption.

>  }

[ ... ]

> @@ -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);

[Severity: High]
Does dropping ce->signal_lock here open a race window that could lead to the
list corruption mentioned above?

Since the lock is dropped before reacquisition on the slow path, other threads
can modify ce->signals and add the context before lock reacquisition. For
example:

Thread A enables breadcrumbs on rq1, fails the fast trylock, and drops
ce->signal_lock to retry on the slow path.

Thread B cancels rq1, emptying ce->signals.

Thread C enables breadcrumbs on rq2, sees an empty ce->signals, succeeds with
the fast trylock, and adds ce->signal_link to b->signalers.

Thread A resumes, reacquires the locks, and unconditionally calls
add_signaling_context(). Since ce->signals now holds rq2, it isn't empty,
causing Thread A to execute a second list_add() on ce->signal_link.

> +		spin_unlock(&ce->signal_lock);
> +		spin_unlock(&b->signalers_lock);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1