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 <[email protected]> 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/[email protected]?part=7
