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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Pointer cast bypasses struct field offset, causing `drop_before_gp` to operate on `rcu_head` instead of `value`.
- [High] `RcuFreeBox::drop` creates a mutable reference (`&mut T`) before the RCU grace period elapses, violating Rust's strict aliasing rules.
- [High] `RcuFreeBox` wraps `T` in `ManuallyDrop<T>`, bypassing its destructor and creating an unresolvable leak or Use-After-Free dilemma.
- [Low] Missing `#[inline]` annotations on small/forwarding functions in the new abstraction.
- [Low] Multiple spelling errors in the code comments.
--

commit 78e35ac6ad00fb0c6ab7802573453596f9ee6134
Author: Boqun Feng <boqun@kernel.org>

rust: rcu: Introduce RcuFreeBox

The current RcuBox calls the drop function after a grace period inside an RCU
callback. To support a pattern where an object is cleaned up, unshared, and
freed after a grace period without readers, RcuFreeBox is introduced.

> diff --git a/rust/kernel/sync/rcu/rcu_box.rs b/rust/kernel/sync/rcu/rcu_box.rs
> index 943fe3e8974e0..8f52bb472daf9 100644
> --- a/rust/kernel/sync/rcu/rcu_box.rs
> +++ b/rust/kernel/sync/rcu/rcu_box.rs

[ ... ]

> @@ -205,6 +207,50 @@ 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>);
> +
> +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]
Does this function need an inline annotation? The Rust subsystem guidelines
mention that small or forwarding functions in abstractions should be
annotated with #[inline].

> +        Ok(Self(RcuBox::new(ManuallyDrop::new(x), flags)?))
> +    }
> +
> +    /// Access the value for a grace period.
> +    pub fn with_rcu<'rcu>(&self, read_guard: &'rcu Guard) -> &'rcu T {

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

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

[Severity: Low]
Would an #[inline] annotation be appropriate for this Deref implementation
as well, since it is a simple forwarding call?

> +        self.0.deref()
> +    }
> +}
> +
> +impl<T: Send + RcuFreeSafe, A: Allocator> Drop for RcuFreeBox<T, A> {
> +    fn drop(&mut self) {
> +        // CAST: ManuallyDrop<T> is transparet to T, adn RcuBox owns the object per type

[Severity: Low]
There are a couple of typos here: transparet instead of transparent, and adn
instead of and.

> +        // invariants.
> +        let ptr = self.0 .0.as_ptr().cast::<T>();

[Severity: Critical]
Is this pointer cast skipping the struct field offset?

self.0.0.as_ptr() returns a pointer to RcuBoxInner<ManuallyDrop<T>>. The
RcuBoxInner struct starts with an rcu_head field, meaning the actual value
field is located at a non-zero offset.

Casting this pointer directly to *mut T will point to the rcu_head memory
instead of the value.

Could calling drop_before_gp() on this pointer corrupt memory by treating
the internal rcu_head structure bytes as if they were of type T?

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

[Severity: High]
Does creating a mutable reference here violate Rust's strict aliasing rules?

When an RcuFreeBox is dropped, it is typically removed from the shared data
structure, but the RCU grace period has not yet elapsed. Concurrent readers
might still hold shared references (&T) obtained via with_rcu.

Creating an exclusive mutable reference (&mut T) using Pin::new_unchecked
while shared references may still exist is immediate undefined behavior in
Rust.

> +
> +        pin.drop_before_gp();
> +
> +        // needs_drop::<ManuallyDrop>() returns false, hence kvfree_call_rcu() will be called

[Severity: High]
Could bypassing the destructor using ManuallyDrop<T> create an unresolvable
leak or use-after-free regression?

Since needs_drop::<ManuallyDrop<T>>() evaluates to false, it skips dropping
the internals of T. If T holds owned resources like a Box or Vec, they are
never freed after the grace period, which leaks memory.

Conversely, if a user tries to work around this by manually freeing
resources inside drop_before_gp(), could that trigger a use-after-free
since drop_before_gp() runs before the RCU grace period ends while
concurrent readers might still be accessing those resources?

> +        // and free the underlying data after a gracer period.

[Severity: Low]
There is a typo here: gracer instead of grace.

> +    }
> +}

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