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