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