Re: [PATCH 6.18.y 6/6] rust: sync: Introduce SpinLockIrq::lock_with() and friends

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] locking: lock_with() panics on PREEMPT_RT and deadlocks with CondVar
- [Medium] rust: sync: lockdep_assert_irqs_disabled() fails on PREEMPT_RT in kunit
--

commit 5ae395d5694f43cafc4f5157f39a1c85552be6b1
Author: Lyude Paul <lyude@redhat.com>

rust: sync: Introduce SpinLockIrq::lock_with() and friends

This commit adds lock_with() and try_lock_with() methods to SpinLockIrq.
These methods allow acquiring a spinlock without changing the interrupt
state by passing a LocalInterruptDisabled token. The goal is to avoid
overhead when it is already known that local interrupts are disabled.

Link: https://patch.msgid.link/20260807070218.27144-19-boqun@kernel.org

> diff --git a/rust/kernel/sync/lock/spinlock.rs b/rust/kernel/sync/lock/spinlock.rs
> index e566f223c6494..b3a0d7a3de04c 100644
> --- a/rust/kernel/sync/lock/spinlock.rs
> +++ b/rust/kernel/sync/lock/spinlock.rs
> @@ -208,7 +221,47 @@ macro_rules! new_spinlock_irq {
>  /// # Ok::<(), Error>(())
>  /// ```
>  ///
> +/// The next example demonstrates locking a [`SpinLockIrq`] using [`lock_with()`] in a function
> +/// which can only be called when local processor interrupts are already disabled.
> +///
> +/// ```
> +/// use kernel::sync::{new_spinlock_irq, SpinLockIrq};
> +/// use kernel::interrupt::*;
> +///
> +/// struct Inner {
> +///     a: u32,
> +/// }
> +///
> +/// #[pin_data]
> +/// struct Example {
> +///     #[pin]
> +///     inner: SpinLockIrq<Inner>,
> +/// }
> +///
> +/// impl Example {
> +///     fn new() -> impl PinInit<Self> {
> +///         pin_init!(Self {
> +///             inner <- new_spinlock_irq!(Inner { a: 20 }),
> +///         })
> +///     }
> +/// }
> +///
> +/// // Accessing an `Example` from a function that can only be called in no-interrupt contexts.
> +/// fn noirq_work(e: &Example, interrupt_disabled: &LocalInterruptDisabled) {
> +///     // Because we know interrupts are disabled from interrupt_disable, we can skip toggling
> +///     // interrupt state using lock_with() and the provided token
> +///     assert_eq!(e.inner.lock_with(interrupt_disabled).a, 20);
> +/// }

[Severity: High]
Does this code demonstrate a pattern that will crash a preempt-rt kernel?

If the guard returned by lock_with() is passed to a CondVar, the condition
variable will drop the spinlock and sleep. Because the LocalInterruptDisabled
token remains in scope, the thread will invoke the scheduler with hard IRQs
physically disabled, causing a deadlock and panic.

[ ... ]

> @@ -273,6 +326,43 @@ unsafe fn assert_is_held(ptr: *mut Self::State) {
>      }
>  }
>  
> +impl<T: ?Sized> Lock<T, SpinLockIrqBackend> {
> +    /// Casts the lock as a `Lock<T, SpinLockBackend>`.
> +    #[inline]
> +    fn as_lock_in_interrupt<'a>(&'a self, _context: &'a LocalInterruptDisabled) -> &'a SpinLock<T> {
> +        // SAFETY:
> +        // - `Lock<T, SpinLockBackend>` and `Lock<T, SpinLockIrqBackend>` both have identical data
> +        //   layouts.
> +        // - As long as local interrupts are disabled (which is proven to be true by _context), it
> +        //   is safe to treat a lock with SpinLockIrqBackend as a SpinLockBackend lock.
> +        unsafe { core::mem::transmute(self) }
> +    }
> +
> +    /// Acquires the lock without modifying local interrupt state.
> +    ///
> +    /// This function should be used in place of the more expensive [`Lock::lock()`] function when
> +    /// possible for [`SpinLockIrq`] locks.
> +    #[inline]
> +    pub fn lock_with<'a>(&'a self, context: &'a LocalInterruptDisabled) -> SpinLockGuard<'a, T> {
> +        self.as_lock_in_interrupt(context).lock()
> +    }

[Severity: High]
Can acquiring a spinlock here while hardware interrupts are explicitly held
disabled by the provided context token violate wait-context rules?

On preempt-rt, spinlock_t is mapped to an rt_mutex which sleeps on contention.
Acquiring a sleeping lock while hardware interrupts are explicitly disabled
triggers a panic. Regular SpinLockIrq::lock() avoids this because
spin_lock_irq() conditionally omits hard IRQ disablement on RT.

> +
> +    /// Tries to acquire the lock without modifying local interrupt state.
> +    ///
> +    /// This function should be used in place of the more expensive [`Lock::try_lock()`] function
> +    /// when possible for [`SpinLockIrq`] locks.
> +    ///
> +    /// Returns a guard that can be used to access the data protected by the lock if successful.
> +    #[must_use = "if unused, the lock will be immediately unlocked"]
> +    #[inline]
> +    pub fn try_lock_with<'a>(
> +        &'a self,
> +        context: &'a LocalInterruptDisabled,
> +    ) -> Option<SpinLockGuard<'a, T>> {
> +        self.as_lock_in_interrupt(context).try_lock()
> +    }

[Severity: High]
Is it safe to try acquiring a sleepable spinlock_t with interrupts disabled,
or does this also trigger a wait-context violation on preempt-rt?


While reviewing the file, I also noticed an issue in the tests:

rust/kernel/sync/lock/spinlock.rs:WorkItem::run() {
    ...
        // Make sure the interrupts actually turned off
        // SAFETY: It's always safe to call `lockdep_assert_irqs_disabled()`
        unsafe { bindings::lockdep_assert_irqs_disabled() };
    ...
}

[Severity: Medium]
Will this lockdep assertion incorrectly assume that SpinLockIrq::lock() disables
hardware interrupts across all configurations?

On preempt-rt it does not, which triggers a lockdep warning when running the
rust_spinlock_irq_condvar kunit test.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-spinlock-irq-6-18-v1-0-72c58975994e@google.com?part=6