Re: [PATCH] rust: irq: make Registration compatible with lifetime-bound drivers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] The doctests for `Registration` and `ThreadedRegistration` demonstrate wrapping the `unsafe` constructors in safe functions, violating soundness requirements.
- [Low] Missing `// INVARIANT:` comments when constructing structs with `# Invariants` documentation.

Pre-existing issues:
- [High] Creating a Rust reference to `Registration` or `ThreadedRegistration` before its `request` field is fully initialized causes Undefined Behavior during IRQ registration.
- [Low] Missing `#[inline]` annotations on small/forwarding functions.
--

commit 8e263235d0a19c8fc9b9f8c688ce7dc1f63f4165
Author: Danilo Krummrich <dakr@kernel.org>

rust: irq: make Registration compatible with lifetime-bound drivers

This commit adapts the IRQ registration to support Higher-Ranked Lifetime
Types (HRT) device drivers by removing the static bound from IRQ handlers.
It replaces Devres indirection with direct request_irq() and free_irq()
calls during initialization and drop, tying the registration directly to
the device binding scope.

> diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
> --- a/rust/kernel/irq/request.rs
> +++ b/rust/kernel/irq/request.rs
[ ... ]
> @@ -163,12 +126,21 @@ pub fn irq(&self) -> u32 {
>  /// //
>  /// // This runs in process context and assumes `request` was previously acquired from a device.
>  /// fn register_irq(
> -///     handler: impl PinInit<Data, Error>,
>  ///     request: IrqRequest<'_>,
> -/// ) -> Result<Arc<Registration<Data>>> {
> -///     let registration = Registration::new(request, Flags::SHARED, c"my_device", handler);
> +/// ) -> Result<Pin<KBox<Registration<'_, Data>>>> {
> +///     // SAFETY: The returned Registration is not leaked.
> +///     let registration = unsafe {
> +///         Registration::new(
> +///             request,
> +///             Flags::SHARED,
> +///             c"my_device",
> +///             try_pin_init!(Data {
> +///                 completion <- Completion::new(),
> +///             }? Error),
> +///         )
> +///     };

[Severity: Medium]
Is it safe to expose this leak-sensitive object from a safe function?

The Registration::new() constructor is unsafe because callers must not
leak the returned registration to guarantee proper drop semantics.
Since register_irq() is a safe function returning a Pin<KBox<...>>,
could a safe caller leak the registration and potentially trigger a 
use-after-free after the driver unbinds?

Should this example function be marked unsafe fn?

[ ... ]
> @@ -194,43 +165,44 @@ pub struct Registration<T: Handler> {
[ ... ]
> +    pub unsafe fn new(
[ ... ]
> +        try_pin_init!(&this in Self {
> +            handler <- handler,
> +            request: {
> +                // SAFETY:
> +                // - The callbacks are valid for use with request_irq.
> +                // - If this succeeds, the slot is guaranteed to be valid until the destructor of
> +                //   Self runs, which will deregister the callbacks before the memory location
> +                //   becomes invalid.
> +                // - When request_irq is called, everything that handle_irq_callback will touch
> +                //   has already been initialized, so it's safe for the callback to be called
> +                //   immediately.
> +                to_result(unsafe {
> +                    bindings::request_irq(
> +                        request.irq,
> +                        Some(handle_irq_callback::<T>),
> +                        flags.into_inner(),
> +                        name.as_char_ptr(),
> +                        this.as_ptr().cast::<c_void>(),
> +                    )
> +                })?;
> +                request
> +            },

[Severity: Low]
This isn't a bug, but shouldn't there be an // INVARIANT: comment inside
this initialization block to explain why the invariants for Registration
are satisfied?

[Severity: High]
This is a pre-existing issue, but does this pattern trigger undefined 
behavior if the interrupt fires immediately?

During try_pin_init!, bindings::request_irq() is called while the request 
field is still being initialized. If the IRQ handler is invoked before 
request_irq() returns:

handle_irq_callback()
    let ptr = ptr.cast_const().cast::<Registration<'_, T>>();
    let registration = unsafe { &*ptr };

Could this create a Rust reference to the Registration struct while the
request field is not fully initialized?

Could the request_irq call be moved into a _: { ... } block to ensure
the struct is fully initialized before the interrupt can fire?

[ ... ]
> @@ -241,19 +213,24 @@ pub fn handler(&self) -> &T {
>      }
>  
>      /// Wait for pending IRQ handlers on other CPUs.
> -    ///
> -    /// This will attempt to access the inner [`Devres`] container.
> -    pub fn try_synchronize(&self) -> Result {
> -        let inner = self.inner.try_access().ok_or(ENODEV)?;
> -        inner.synchronize();
> -        Ok(())
> +    pub fn synchronize(&self) {
> +        // SAFETY: `self.request.irq` is a valid registered IRQ number (type invariant).
> +        unsafe { bindings::synchronize_irq(self.request.irq) };
>      }

[Severity: Low]
This is a pre-existing issue and not a bug, but should this forwarding 
function (and the small accessor IrqRequest::irq() in the same file) be 
annotated with #[inline] according to the Rust subsystem guidelines?

[ ... ]
> @@ -375,13 +336,21 @@ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {
>  /// // This is executing in process context and assumes that `request` was
>  /// // previously acquired from a device.
>  /// fn register_threaded_irq(
> -///     handler: impl PinInit<Data, Error>,
>  ///     request: IrqRequest<'_>,
> -/// ) -> Result<Arc<ThreadedRegistration<Data>>> {
[ ... ]
> +/// ) -> Result<Pin<KBox<ThreadedRegistration<'_, Data>>>> {
> +///     // SAFETY: The returned Registration is not leaked.
> +///     let registration = unsafe {
> +///         ThreadedRegistration::new(

[Severity: Medium]
Is it safe to expose this leak-sensitive object from a safe function?

Similar to register_irq(), the ThreadedRegistration::new() constructor
is unsafe to guarantee proper drop semantics. Does returning a Pin<KBox>
from a safe function allow a caller to trivially leak the registration?

[ ... ]
> @@ -411,44 +380,45 @@ pub struct ThreadedRegistration<T: ThreadedHandler> {
[ ... ]
> +    pub unsafe fn new(
[ ... ]
> +        try_pin_init!(&this in Self {
> +            handler <- handler,
> +            request: {
> +                // SAFETY:
> +                // - The callbacks are valid for use with request_threaded_irq.
[ ... ]
> +                to_result(unsafe {
> +                    bindings::request_threaded_irq(
> +                        request.irq,
> +                        Some(handle_threaded_irq_callback::<T>),
> +                        Some(thread_fn_callback::<T>),
> +                        flags.into_inner(),
> +                        name.as_char_ptr(),
> +                        this.as_ptr().cast::<c_void>(),
> +                    )
> +                })?;
> +                request
> +            },

[Severity: Low]
This isn't a bug, but shouldn't there be an // INVARIANT: comment here 
explaining why the invariants are satisfied?

[Severity: High]
This is a pre-existing issue, but does this pattern also trigger undefined 
behavior if the interrupt fires immediately, similar to Registration::new()?

Could this request_threaded_irq call be moved into a _: { ... } block
to avoid creating a reference to an uninitialized request field?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703210936.1128698-1-dakr@kernel.org?part=1