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 <[email protected]> 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/[email protected]?part=3
