Re: [RFC PATCH v2 1/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` lacks a `'static` bound on `T`, which allows asynchronous Use-After-Free for types with custom Drop implementations that contain non-'static references.
- [High] `RcuBox<T, A>` bypasses the generic `Allocator`'s free method in its `Drop` implementation when `needs_drop::<T>()` is false, leading to invalid frees for custom allocators.
- [High] `RcuBox` safely uses `call_rcu()` with a monomorphized callback `drop_rcu_box`, but Rust does not expose `rcu_barrier()`, making safe module unload impossible.
--

--- Patch [1]: [RFC PATCH v2 1/4] rust: rcu: Add RcuBox type ---
commit b7ba3ecd614c74b985c54d7b0785315fa2c066c8
Author: Alice Ryhl <aliceryhl@google.com>

rust: rcu: Add RcuBox type

This adds an RcuBox container, which is like Box except that the value
is freed after waiting for one grace period (via {kvfree_,}call_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..cb1fd422480a8
> --- /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 A.
> +/// * This RcuBox holds exclusive permissions to rcu free the allocation.
> +pub struct RcuBox<T: Send, A: Allocator>(NonNull<RcuBoxInner<T>>, PhantomData<A>);

[Severity: Critical]
Since the drop implementation can delegate the actual drop to an asynchronous
background task via call_rcu, could a type with a non-static lifetime
trigger a use-after-free if its custom drop implementation accesses a scoped
reference after it goes out of scope?

Would it be safer to require T: 'static here?

[ ... ]

> +impl<T: Send, A: Allocator> Drop for RcuBox<T, A> {
> +    fn drop(&mut self) {
> +        // SAFETY: The rcu_head field is in-bounds of a valid allocation.
> +        let rcu_head = unsafe { &raw mut (*self.0.as_ptr()).rcu_head };
> +        if core::mem::needs_drop::<T>() {
> +            // SAFETY: rcu_head is the rcu_head field of RcuBoxInner<T>. All users will be
> +            // gone in an rcu grace period. This is the destructor, so we may pass ownership of the
> +            // allocation.
> +            unsafe { bindings::call_rcu(rcu_head, Some(drop_rcu_box::<T, A>)) };

[Severity: High]
Does this monomorphized callback require modules to use rcu_barrier() during
unload to prevent executing unloaded memory?

Since rcu_barrier() isn't exposed in the Rust bindings yet, is there a way
for loadable modules using RcuBox to safely unload without risking a regression
if the RCU grace period hasn't completed?

> +        } else {
> +            // SAFETY: All users will be gone in an rcu grace period.
> +            // TODO: We are luckily since kvfree_call_rcu() works on both kmalloc and vmalloc,
> +            // maybe a new Allocator method is needed.
> +            unsafe { bindings::kvfree_call_rcu(rcu_head, self.0.as_ptr().cast()) };

[Severity: High]
If RcuBox is instantiated with a custom allocator, will hardcoding
kvfree_call_rcu cause an invalid free when needs_drop is false?

This branch appears to completely bypass the generic allocator's free method.

> +        }
> +    }
> +}

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