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