Re: [PATCH v3 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 6 potential issue(s) to consider:
- [Critical] Missing `spin_unlock_irqrestore` in the early return path leaves local interrupts disabled.
- [High] Uninitialized variable `add_context` may be evaluated and lead to undefined behavior.
- [High] Data race / list corruption when iterating `ce->signals` without holding `ce->signal_lock`.
- [High] Double `list_add` in `add_signaling_context` causes list corruption due to race condition.
- [High] Queueing `irq_work` before adding the context to the signalers list causes missed wakeups.
- [High] Unconditional call to `list_empty` and `list_del_init` on a potentially uninitialized list node causes NULL pointer dereference.
--

commit 84930541bab063f0b3f4b8114a287062a6526adc
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

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

This commit replaces RCU with a global spinlock to manage the signalers list
in order to prevent IRQ-Work starvation and support PREEMPT_RT.

> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> index c10ac0ab3bfa8..be87c8d0ae922 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]
Could this unconditionally add a node that is already linked?

In unlock_context_remove_signaling(), if spin_trylock() fails,
ce->signal_lock is dropped while the context remains linked in b->signalers:

    if (list_empty(&ce->signals)) {
        if (spin_trylock(&b->signalers_lock)) {
            // ...
        } else {
            retry = true;
        }
    }
    spin_unlock_irqrestore(&ce->signal_lock, flags);

If another thread concurrently submits a new request to this context via
i915_request_enable_breadcrumb(), it sees an empty ce->signals list, inserts
the request, and calls add_signaling_context().

Because ce->signals is now non-empty, add_signaling_context() unconditionally
executes list_add(&ce->signal_link, &b->signalers). Since the node is already
linked, this would corrupt the doubly-linked list.

>  }
>  
>  static bool remove_signaling_context(struct intel_breadcrumbs *b,
>  				     struct intel_context *ce)
>  {
>  	lockdep_assert_held(&ce->signal_lock);
> +	lockdep_assert_held(&b->signalers_lock);
>  
> -	if (!list_empty(&ce->signals))
> +	if (!list_empty(&ce->signals) || list_empty(&ce->signal_link))
>  		return false;
>  
> -	spin_lock(&b->signalers_lock);
> -	list_del_rcu(&ce->signal_link);
> -	spin_unlock(&b->signalers_lock);
> -
> +	list_del_init(&ce->signal_link);

[Severity: High]
Can remove_signaling_context() dereference a NULL pointer if the list node
is zero-initialized?

The patch introduces logic in unlock_context_remove_signaling() that calls
remove_signaling_context() even if ce->signals was empty and the context
was never added to b->signalers.

If ce->signal_link is zero-initialized rather than initialized via
INIT_LIST_HEAD, list_empty(&ce->signal_link) evaluates to
NULL == &ce->signal_link (returning false). It then proceeds to call
list_del_init(&ce->signal_link), which dereferences the NULL pointers inside
the zeroed list head.

>  	return true;
>  }

[ ... ]

> @@ -427,30 +424,88 @@ 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)) {

[Severity: High]
Can add_context be used uninitialized here?

If test_bit(I915_FENCE_FLAG_ACTIVE, &rq->fence.flags) evaluates to false 
(e.g., if the bit was cleared concurrently after the lockless check above),
add_context is never assigned.

It is then evaluated in the condition:

    if (add_context && spin_trylock(&b->signalers_lock)) {

If the uninitialized stack variable evaluates to true, it could lead to
incorrectly calling add_signaling_context() and list corruption.

> +		add_signaling_context(b, ce);
> +		spin_unlock(&b->signalers_lock);
> +		add_context = false;
> +	}
> +
> +	/*
> +	 * Defer enabling the interrupt to after HW submission and recheck
> +	 * the request as it may have completed and raised the interrupt as
> +	 * we were attaching it into the lists.
> +	 */
> +	if (!READ_ONCE(b->irq_armed) || __i915_request_is_complete(rq))
> +		irq_work_queue(&b->irq_work);
> +
>  	spin_unlock(&ce->signal_lock);
>  
> +	if (add_context) {

[Severity: High]
Does queuing irq_work before the context is added to the signalers list
create a TOCTOU race?

If the fast path spin_trylock() fails in i915_request_enable_breadcrumb(),
adding the context to b->signalers is deferred to the slow path here. However,
irq_work_queue() is executed immediately before the slow path block.

If the irq_work executes before the slow path adds the context, it sees an
empty list and finishes without arming the hardware interrupt. Once the
context is subsequently added, no further IRQ work is queued, and the
interrupt remains disarmed forever, causing system hangs.

> +		/*
> +		 * 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);
> +	}
> +
>  	return true;
>  }

[ ... ]

>  void i915_request_cancel_breadcrumb(struct i915_request *rq)
>  {
>  	struct intel_breadcrumbs *b = READ_ONCE(rq->engine)->breadcrumbs;
>  	struct intel_context *ce = rq->context;
> -	bool release;
> +	unsigned long flags;
>  
> -	spin_lock(&ce->signal_lock);
> +	spin_lock_irqsave(&ce->signal_lock, flags);
>  	if (!test_and_clear_bit(I915_FENCE_FLAG_SIGNAL, &rq->fence.flags)) {
>  		spin_unlock(&ce->signal_lock);

[Severity: Critical]
Are local interrupts left disabled in this early return path?

The lock acquisition was changed to spin_lock_irqsave(&ce->signal_lock, flags),
but the early return still uses spin_unlock(&ce->signal_lock), which fails to
restore the CPU's interrupt state.

>  		return;
>  	}

[ ... ]

> @@ -508,7 +557,7 @@ static void print_signals(struct intel_breadcrumbs *b, struct drm_printer *p)
>  {
>  	struct intel_context *ce;
>  	struct i915_request *rq;
> +	unsigned long flags;
>  
>  	drm_printf(p, "Signals:\n");
>  
> -	rcu_read_lock();
> -	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
> -		list_for_each_entry_rcu(rq, &ce->signals, signal_link)
> +	spin_lock_irqsave(&b->signalers_lock, flags);
> +	list_for_each_entry(ce, &b->signalers, signal_link) {
> +		list_for_each_entry(rq, &ce->signals, signal_link)

[Severity: High]
Is it safe to iterate over ce->signals without holding ce->signal_lock?

Since RCU is removed, ce->signals is a standard doubly-linked list. Writers
(e.g., i915_request_cancel_breadcrumb) modify ce->signals holding only
ce->signal_lock.

Iterating over it here while only holding b->signalers_lock exposes a data
race where concurrent modifications could cause the iteration to dereference
LIST_POISON1.

>  			drm_printf(p, "\t[%llx:%llx%s] @ %dms\n",

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