Re: [RFC PATCH v2 3/4] rust: rcu: Introduce RcuFreeBox

[sashiko-bot@kernel.org]

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

New issues:
- [High] `RcuFreeBox::drop` creates an exclusive mutable reference (`&mut T`) while concurrent RCU readers may exist, causing Undefined Behavior (Mutable Aliasing).
- [High] `RcuFreeBox` permanently skips `T::drop()`, silently leaking inner resources or forcing developers into Use-After-Free traps.
- [High] Casting between `*mut RcuBoxInner<ManuallyDrop<T>>` and `*mut RcuBoxInner<T>` relies on unguaranteed memory layout, resulting in Undefined Behavior.
- [High] If memory allocation fails in `RcuFreeBox::new()`, the inner object is silently leaked.
- [Medium] `RcuFreeBox` lacks implementations for `ForeignOwnable` and `ForeignOwnableRcu`, making it completely unusable in RCU data structures.
- [Low] Missing `#[inline]` annotations on small forwarding functions in an abstraction.

Pre-existing issues:
- [High] `RcuBox::drop` unconditionally invokes `kvfree_call_rcu`, bypassing custom generic allocators and causing kernel memory corruption.
--

--- Patch [3]: [RFC PATCH v2 3/4] rust: rcu: Introduce RcuFreeBox ---
commit a85d6377ca5df1e0c96d95e2dd50fb0b8a721195
Author: Boqun Feng <boqun@kernel.org>
Subject: rust: rcu: Introduce RcuFreeBox

This commit introduces RcuFreeBox, which extends RcuBox to support an RCU
usage pattern where objects are cleaned up before a grace period and freed
afterwards. It provides methods to unshare an object from future RCU
readers and wait for a grace period before safely freeing the memory.

> diff --git a/rust/kernel/sync/rcu.rs b/rust/kernel/sync/rcu.rs
> index 42f6bbc83f71f..e781a5044de9b 100644
> --- a/rust/kernel/sync/rcu.rs
> +++ b/rust/kernel/sync/rcu.rs
[ ... ]
> @@ -100,3 +104,33 @@ pub trait ForeignOwnableRcu: ForeignOwnable {
>      /// [`from_foreign`]: ForeignOwnable::from_foreign
>      unsafe fn rcu_borrow<'a>(ptr: *mut ffi::c_void) -> Self::RcuBorrowed<'a>;
>  }
> +
> +/// Declares a struct is safe to free after a grace period if all readers are guarded by RCU.
> +///
> +/// # Safety
> +///
> +/// Implementation must guarantee `drop_before_gp()` makes sure no future RCU reader will access
> +/// any part of [`Self`], as a result, after `drop_before_gp()` return + one grace period, no RCU
> +/// reader will be on the object, and it's safe to free it.
> +///
> +/// Notes for implementators: implementing this trait in general requires `Self` being a
> +/// [`UnsafePinned`], i.e. a `&mut Self` is not a noalias reference if `Self` has non-trivial
> +/// `drop()` function.
> +pub unsafe trait RcuFreeSafe {
> +    /// Clean up `Self` and make it ready to be RCU freed.
> +    fn drop_before_gp(self: Pin<&mut Self>);

[Severity: High]
Does this trait signature inherently force a mutable aliasing violation?

By requiring a mutable reference here, does this force the caller to
create an exclusive reference while concurrent RCU readers may still hold
shared references to the object?

> +}
[ ... ]
> diff --git a/rust/kernel/sync/rcu/rcu_box.rs b/rust/kernel/sync/rcu/rcu_box.rs
> index cb1fd422480a8..ab4e9b2a44448 100644
> --- a/rust/kernel/sync/rcu/rcu_box.rs
> +++ b/rust/kernel/sync/rcu/rcu_box.rs
[ ... ]
> @@ -223,6 +225,56 @@ fn drop(&mut self) {
>      drop(unsafe { Box::<_, A>::from_raw(box_inner) });
>  }
>  
> +/// A box that is freed with RCU.
> +///
> +/// Currently we require `T` being `Send` because of an implementation limitation. In theory we can
> +/// support `T` being `!Send`, since the RCU callback is only used to free the memory, not dropping
> +/// `T`.
> +pub struct RcuFreeBox<T: Send + RcuFreeSafe, A: Allocator>(RcuBox<ManuallyDrop<T>, A>);

[Severity: Medium]
How can this struct be used in RCU data structures?

Since it lacks implementations for ForeignOwnable and ForeignOwnableRcu,
and its inner RcuBox field is private, is it structurally possible to
convert this to a foreign pointer or share it with RCU readers?

> +
> +impl<T: Send + RcuFreeSafe, A: Allocator> RcuFreeBox<T, A> {
> +    /// Create a new `RcuFreeBox`.
> +    pub fn new(x: T, flags: alloc::Flags) -> Result<Self, AllocError> {

[Severity: Low]
Should this small forwarding function have an inline annotation?

> +        Ok(Self(RcuBox::new(ManuallyDrop::new(x), flags)?))

[Severity: High]
Does this silently leak the inner object on allocation failure?

If RcuBox::new() returns an AllocError, the ? operator will abort the
function. Because ManuallyDrop intentionally prevents the Drop
implementation of its inner value from running, does this leak the object
and any dynamic resources it holds?

> +    }
> +}
> +
> +impl<T: Send + RcuFreeSafe, A: Allocator> Deref for RcuFreeBox<T, A> {
> +    type Target = T;
> +
> +    fn deref(&self) -> &T {

[Severity: Low]
Should this trivial deref forwarder have an inline annotation?

> +        self.0.deref()
> +    }
> +}
> +
> +impl<T: Send + RcuFreeSafe, A: Allocator> Drop for RcuFreeBox<T, A> {
> +    fn drop(&mut self) {

[Severity: High]
This isn't a bug introduced by this patch, but does the underlying
RcuBox::drop unconditionally bypass custom allocators?

When core::mem::needs_drop::<T>() evaluates to false, RcuBox::drop
bypasses the allocator A and calls bindings::kvfree_call_rcu():

RcuBox::drop() {
    ...
    unsafe { bindings::kvfree_call_rcu(rcu_head, self.0.as_ptr().cast()) };
    ...
}

If a custom allocator (such as a module-specific slab cache) is used,
will passing its pointer to kvfree result in memory corruption?

> +        // CAST: `ManuallyDrop<RcuBoxInner<T>>` is transparent to `RcuBoxInner<T>`, and `RcuBox`
> +        // owns the object per type invariants.
> +        let inner: *mut RcuBoxInner<T> = self.0 .0.as_ptr().cast();

[Severity: High]
Does casting between these types rely on unguaranteed memory layout?

Both RcuBoxInner<ManuallyDrop<T>> and RcuBoxInner<T> use the default
repr(Rust) layout.

Since the Rust compiler provides no layout guarantees for generic
repr(Rust) structs across different type parameters, can fields be
arbitrarily reordered, making this cast undefined behavior?

> +
> +        // SAFETY: Per the invariants of `RcuBox`, `inner` owns the pointed object. And we are not
> +        // going to move it.
> +        let pin = unsafe { Pin::new_unchecked(&mut (*inner).value) };

[Severity: High]
Does creating this exclusive mutable reference cause undefined behavior?

When RcuFreeBox::drop() executes before the RCU grace period,
pre-existing RCU readers may still be traversing the object and holding
shared references.

Does forming a mutable reference while shared references are active
violate Rust aliasing rules?

> +
> +        pin.drop_before_gp();
> +
> +        // `needs_drop::<ManuallyDrop>()` returns `false`, hence `kvfree_call_rcu()` will be called
> +        // and free the underlying data after a grace period.

[Severity: High]
Does permanently skipping T::drop() force a memory leak or a
use-after-free?

Because ManuallyDrop evaluates needs_drop to false, kvfree_call_rcu()
will only free the outermost struct after the grace period, bypassing
T::drop().

If T owns dynamic resources like a Box or Vec, will they permanently leak?

If a developer attempts to fix the leak by manually dropping the inner
fields inside drop_before_gp(), will they trigger a use-after-free
because concurrent RCU readers can still access those fields before the
grace period ends?

> +    }
> +}

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