Re: [PATCH 6.18.y 2/6] rust: Introduce interrupt module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] rust: docs falsely claim local_interrupt_disable is no-op on !PREEMPT kernels
- [Medium] rust: interrupt: missing #[must_use] on LocalInterruptDisabled guard
- [Low] rust: missing // INVARIANT: comment for LocalInterruptDisabled
- [Low] rust: redundant FFI helper for out-of-line local_interrupt_disable()
--

commit d5f70a73a2c90b91e9114a20f641c084d5e70a96
Author: Lyude Paul <lyude@redhat.com>
rust: Introduce interrupt module

This introduces a module for dealing with interrupt-disabled contexts,
including the ability to enable and disable interrupts along with the
ability to annotate functions as expecting that IRQs are already
disabled on the local CPU.

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

> diff --git a/rust/helpers/interrupt.c b/rust/helpers/interrupt.c
> new file mode 100644
> index 0000000000000..69595498620f5
> --- /dev/null
> +++ b/rust/helpers/interrupt.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/spinlock.h>
> +
> +__rust_helper void rust_helper_local_interrupt_disable(void)
> +{
> +	local_interrupt_disable();
> +}
> +
> +__rust_helper void rust_helper_local_interrupt_enable(void)
> +{
> +	local_interrupt_enable();
> +}

[Severity: Low]
This isn't a bug, but are these C FFI wrappers necessary? According to
the subsystem rules, rust/helpers/ should only be used to export inline
functions or function macros.

Looking at kernel/softirq.c, local_interrupt_disable() and
local_interrupt_enable() are unconditionally implemented as out-of-line
functions across all configurations. Could we expose them directly via
bindgen by including <linux/interrupt_rc.h> in
rust/bindings/bindings_helper.h instead of adding redundant boilerplate?

[ ... ]

> diff --git a/rust/kernel/interrupt.rs b/rust/kernel/interrupt.rs
> new file mode 100644
> index 0000000000000..a880ec3b8538a
> --- /dev/null
> +++ b/rust/kernel/interrupt.rs

[ ... ]

> +/// # Invariants
> +///
> +/// Local processor interrupts are disabled on preemptible kernels for as long as an object of this
> +/// type exists.
> +pub struct LocalInterruptDisabled(NotThreadSafe);

[Severity: Medium]
Should this struct be marked with #[must_use]?

Because LocalInterruptDisabled implements Drop to call
local_interrupt_enable(), if a caller forgets to bind the returned guard to
a variable:

    local_interrupt_disable();

the guard will be dropped instantly at the semicolon. This would
unintentionally re-enable interrupts immediately, leaving the intended
critical section unprotected against interrupts.

> +
> +/// Disable local processor interrupts on a preemptible kernel.
> +///
> +/// This function disables local processor interrupts on a preemptible kernel, and returns a
> +/// [`LocalInterruptDisabled`] token as proof of this. On non-preemptible kernels, this function is
> +/// a no-op.

[Severity: Medium]
Does this accurately describe the behavior on non-preemptible kernels?

Looking at the C implementation in kernel/softirq.c:local_interrupt_disable(),
it unconditionally calls local_irq_save(flags). This always disables
hardware interrupts regardless of CONFIG_PREEMPT or CONFIG_PREEMPT_RT.

Could this documentation mislead developers into assuming the function
has zero cost on non-preemptible kernels, while it actually degrades
interrupt latency?

> +///
> +/// **Usage of this function is discouraged** unless you are absolutely sure you know what you are
> +/// doing, as kernel interfaces for Rust that deal with interrupt state will typically handle local
> +/// processor interrupt state management on their own and managing this by hand is quite error
> +/// prone.
> +#[inline]
> +pub fn local_interrupt_disable() -> LocalInterruptDisabled {
> +    // SAFETY: It's always safe to call `local_interrupt_disable()`.
> +    unsafe { bindings::local_interrupt_disable() };
> +
> +    LocalInterruptDisabled(NotThreadSafe)

[Severity: Low]
This isn't a bug, but should this construction have an // INVARIANT: comment?

The subsystem guidelines mandate that when a struct with an # Invariants
documentation section is constructed, the code should have an // INVARIANT:
comment explaining why the invariants are satisfied.

[ ... ]

> +impl LocalInterruptDisabled {
> +    /// Assume that local processor interrupts are disabled on preemptible kernels.
> +    ///
> +    /// This can be used for annotating code that is known to be run in contexts where local
> +    /// processor interrupts are disabled on preemptible kernels. It makes no changes to the local
> +    /// interrupt state on its own.
> +    ///
> +    /// # Safety
> +    ///
> +    /// For the whole life `'a`, local interrupts must be disabled on preemptible kernels. This
> +    /// could be a context like, for example, an interrupt handler.
> +    #[inline]
> +    pub unsafe fn assume_disabled<'a>() -> &'a LocalInterruptDisabled {
> +        const ASSUME_DISABLED: &LocalInterruptDisabled = &LocalInterruptDisabled(NotThreadSafe);

[Severity: Low]
Similarly, should this construction also include an // INVARIANT: comment?

> +
> +        // Confirm they're actually disabled if lockdep is available
> +        // SAFETY: It's always safe to call `lockdep_assert_irqs_disabled()`.
> +        unsafe { bindings::lockdep_assert_irqs_disabled() };
> +
> +        ASSUME_DISABLED
> +    }
> +}

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