Re: [PATCH v2 7/7] drm/i915/gt: Use a spinlock 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:
- [High] Concurrent modification and lockless traversal of the `b->signalers` list.
- [High] Failure to restore interrupt state on early return in `i915_request_cancel_breadcrumb()`.
- [High] Use-after-free of `ce->signal_lock` in `signal_irq_work()` due to early `intel_context_put()`.
- [High] Double `list_add()` causing list corruption due to dropped lock in `unlock_and_maybe_remove_signaling_context()`.
- [High] Double `list_del()` and double `intel_context_put()` on already removed contexts in `intel_context_remove_breadcrumbs()`.
- [Medium] Incorrect locking used for iterating `ce->signals` in `print_signals()`.
--

commit e3123b26bfbdb1e44033e9e8c3f7ae5f8b76d837
Author: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
drm/i915/gt: Use a spinlock to prevent starvation of irq_work.

This patch aims to prevent irq_work starvation on PREEMPT_RT by removing
RCU and introducing a global spinlock to force completion. It replaces
list_add_rcu/list_del_rcu with standard list operations and adds a
signaler_active spinlock.

> diff --git a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> --- a/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_breadcrumbs.c
[ ... ]
> @@ -208,12 +208,12 @@ static void signal_irq_work(struct irq_work *work)
>  	if (!signal && READ_ONCE(b->irq_armed) && list_empty(&b->signalers))
>  		intel_breadcrumbs_disarm_irq(b);
>  
> -	rcu_read_lock();
> -	atomic_inc(&b->signaler_active);
> -	list_for_each_entry_rcu(ce, &b->signalers, signal_link) {
> -		struct i915_request *rq;
> +	spin_lock(&b->signaler_active);
> +	list_for_each_entry_safe(ce, next, &b->signalers, signal_link) {

[Severity: High]
Is the b->signalers list properly protected against concurrent
modifications during this traversal?

This loop traverses b->signalers holding only b->signaler_active. However,
add_signaling_context() and remove_signaling_context() modify this same
list while holding b->signalers_lock, without acquiring b->signaler_active.

Any thread queuing a request could trigger an insertion while this
traversal is happening, which might cause data races or pointer corruption
since they are in different lock domains.

> +		spin_lock(&ce->signal_lock);
> +		list_for_each_entry_safe(rq, rq_next, &ce->signals, signal_link) {
[ ... ]
> -			spin_lock(&ce->signal_lock);
> -			list_del_rcu(&rq->signal_link);
> +			list_del(&rq->signal_link);
>  			release = remove_signaling_context(b, ce);
> -			spin_unlock(&ce->signal_lock);
>  			if (release) {
>  				if (intel_timeline_is_last(ce->timeline, rq))
>  					add_retire(b, ce->timeline);
> @@ -244,9 +242,9 @@ static void signal_irq_work(struct irq_work *work)
>  			else
>  				i915_request_put(rq);
>  		}
> +		spin_unlock(&ce->signal_lock);

[Severity: High]
Could this lead to a use-after-free of ce->signal_lock?

If intel_context_put(ce) drops the final reference to the context, the
memory backing ce (including the lock itself) could be freed immediately.

Since RCU read-side protection was removed, the memory might be scheduled
for freeing before the loop concludes and calls
spin_unlock(&ce->signal_lock).

[ ... ]
> +static void unlock_and_maybe_remove_signaling_context(struct intel_context *ce,
> +						      struct intel_breadcrumbs *b,
> +						      unsigned long flags)
> +{
> +	bool release = false, retry = false;
> +
> +	if (list_empty(&ce->signals)) {
> +		if (spin_trylock(&b->signaler_active)) {
> +			release = remove_signaling_context(b, ce);
> +			spin_unlock(&b->signaler_active);
> +		} else {
> +			retry = true;
> +		}
> +	}
> +	spin_unlock_irqrestore(&ce->signal_lock, flags);
> +
> +	if (retry) {

[Severity: High]
Could dropping the lock here result in double list additions?

If spin_trylock(&b->signaler_active) fails, this function drops
ce->signal_lock and retries to avoid an AB-BA deadlock. During this
lockless window, ce->signals is empty but ce->signal_link is still linked
in the global b->signalers list.

If a concurrent thread calls insert_breadcrumb(), it will observe an empty
ce->signals list and call add_signaling_context():

insert_breadcrumb() {
    if (list_empty(&ce->signals)) {
        intel_context_get(ce);
        add_signaling_context(b, ce);
}

This would blindly execute list_add(&ce->signal_link, &b->signalers) while
it is already linked, which might corrupt the pointers.

[ ... ]
>  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);
>  		return;
>  	}

[Severity: High]
Will this early return leave local interrupts permanently disabled?

The lock acquisition is upgraded to spin_lock_irqsave(), which disables
local interrupts and saves the state in the flags variable. However, this
early return path uses plain spin_unlock(&ce->signal_lock) instead of
spin_unlock_irqrestore(&ce->signal_lock, flags).

If a request is cancelled that has already been signaled, the CPU will
return from this function with IRQs disabled, which might cause a hard
lockup.

[ ... ]
>  void intel_context_remove_breadcrumbs(struct intel_context *ce,
>  				      struct intel_breadcrumbs *b)
>  {
[ ... ]
> -	release = remove_signaling_context(b, ce);
>  
>  unlock:
> -	spin_unlock_irqrestore(&ce->signal_lock, flags);
> -	if (release)
> -		intel_context_put(ce);
> -
> -	while (atomic_read(&b->signaler_active))
> -		cpu_relax();
> +	unlock_and_maybe_remove_signaling_context(ce, b, flags);
>  }

[Severity: High]
Does this cause a double list_del() on contexts that are already removed?

If ce->signals is already empty when intel_context_remove_breadcrumbs()
is called (which happens for cleanly destroyed contexts), it will jump
to the unlock label and call unlock_and_maybe_remove_signaling_context().

That helper checks if ce->signals is empty, sees that it is, and calls
remove_signaling_context(), which blindly executes
list_del(&ce->signal_link). Since the link was already deleted when the
list originally became empty, this executes list_del() on LIST_POISON.
It could also cause a double intel_context_put() from the refcount
underflow.

[ ... ]
>  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->signaler_active, flags);
> +	list_for_each_entry(ce, &b->signalers, signal_link) {
> +		spin_lock(&b->signalers_lock);
> +		list_for_each_entry(rq, &ce->signals, signal_link)

[Severity: Medium]
Is b->signalers_lock the correct lock to protect the ce->signals list?

This acquires b->signalers_lock instead of ce->signal_lock to protect
the traversal of the ce->signals list. Since insert_breadcrumb()
modifies ce->signals under ce->signal_lock, traversing it here with the
wrong lock allows concurrent mutations during iteration.

When i915_engine_info is read via debugfs, this could lead to list
corruption or an oops.

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