Re: [PATCH 2/4] rust: rcu: add RcuBox type

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] `RcuBox::into_foreign` consumes `self` by value without preventing its destructor from running, causing immediate RCU freeing of the active allocation.
- [High] `RcuBox` defers the execution of the inner value's destructor using RCU but fails to restrict the value to the `'static` lifetime, allowing use-after-free on stack references.
- [High] `ForeignOwnableRcu` implementation for `RcuBox<T>` does not require `T: Sync`, allowing data races on `!Sync` types.
--

commit 81791c9c93af881931b7e9c9ed0d885ba31c73cb
Author: Alice Ryhl <aliceryhl@google.com>

rust: rcu: add RcuBox type

This adds an RcuBox container, which is like KBox except that the value
is freed with kfree_rcu. To allow containers to rely on the rcu properties
of RcuBox, an extension of ForeignOwnable is added.

> diff --git a/rust/kernel/sync/rcu/rcu_box.rs b/rust/kernel/sync/rcu/rcu_box.rs
> new file mode 100644
> index 0000000000000..2508fdb609ecc
> --- /dev/null
> +++ b/rust/kernel/sync/rcu/rcu_box.rs

[ ... ]

> +/// A box that is freed with rcu.
> +///
> +/// The value must be `Send`, as rcu may drop it on another thread.
> +///
> +/// # Invariants
> +///
> +/// * The pointer is valid and references a pinned `RcuBoxInner<T>` allocated with `kmalloc`.
> +/// * This `RcuBox` holds exclusive permissions to rcu free the allocation.
> +pub struct RcuBox<T: Send>(NonNull<RcuBoxInner<T>>);

[Severity: High]
Does RcuBox need a 'static bound on T here?

RcuBox defers the execution of the inner value's destructor using RCU via
call_rcu, but the definition lacks a 'static bound on T. This means the Rust
compiler would permit short-lived local references (e.g., &'a mut i32) to be
stored inside types wrapped by RcuBox.

Since the actual destructor of T is executed asynchronously in the RCU
callback, it may occur after the local variables it references have been
popped from the stack. If T's destructor accesses those references, it will
read or write to freed stack memory.

[ ... ]

> +// SAFETY:
> +// * The `RcuBoxInner<T>` was allocated with `kmalloc`.
> +// * `NonNull::as_ptr` returns a non-null pointer.
> +unsafe impl<T: Send + 'static> ForeignOwnable for RcuBox<T> {
> +    const FOREIGN_ALIGN: usize = <KBox<RcuBoxInner<T>> as ForeignOwnable>::FOREIGN_ALIGN;
> +
> +    type Borrowed<'a> = &'a T;
> +    type BorrowedMut<'a> = &'a T;
> +
> +    fn into_foreign(self) -> *mut c_void {
> +        self.0.as_ptr().cast()
> +    }

[Severity: Critical]
Does this code inadvertently run the destructor of RcuBox during
into_foreign()?

Since this method takes self by value and doesn't explicitly wrap it in
core::mem::ManuallyDrop or call core::mem::forget(self), the object will
be dropped at the end of the method's scope.

This would invoke RcuBox::drop immediately, enqueuing a call to
bindings::call_rcu or bindings::kvfree_call_rcu before returning the
pointer. Any subsequent access by C code reading the published pointer
would result in a use-after-free, and calling from_foreign() and dropping
it again would cause a double-free.

> +
> +    unsafe fn from_foreign(ptr: *mut c_void) -> Self {

[ ... ]

> +impl<T: Send + 'static> ForeignOwnableRcu for RcuBox<T> {

[Severity: High]
Should this trait implementation also require T: Sync?

The ForeignOwnableRcu trait is designed to allow concurrent RCU readers to
safely borrow the underlying data, with rcu_borrow() returning &'a T.

Because RCU allows multiple threads to execute read-side critical sections
concurrently, multiple CPUs can concurrently call rcu_borrow on the same
pointer. If T is not Sync (e.g., Cell<i32>), this could result in multiple
&T references to a !Sync type across different threads, allowing concurrent
mutation through shared references.

> +    type RcuBorrowed<'a> = &'a T;
> +
> +    unsafe fn rcu_borrow<'a>(ptr: *mut c_void) -> &'a T {
> +        // SAFETY: `RcuBox::drop` can only run after `from_foreign` is called, and the value is
> +        // valid until `RcuBox::drop` plus one grace period.
> +        unsafe { &(*ptr.cast::<RcuBoxInner<T>>()).value }
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260530143541.229628-2-phasta@kernel.org?part=2