Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] RcuBox::into_foreign drops `self` instead of forgetting it, causing a guaranteed Use-After-Free. - [High] RcuBox::drop bypasses the custom Allocator A and hardcodes kvfree_call_rcu() for types that do not require dropping, causing undefined behavior for non-kvfree compatible allocators. --
commit f69c06884595bb68a76f4f1dd4014ee9e2c9e31c 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..943fe3e8974e0 > --- /dev/null > +++ b/rust/kernel/sync/rcu/rcu_box.rs [ ... ] > +// SAFETY: > +// * The `RcuBoxInner<T>` was allocated with `A`. > +// * `NonNull::as_ptr` returns a non-null pointer. > +unsafe impl<T: Send + 'static, A: Allocator> ForeignOwnable for RcuBox<T, A> > { > + const FOREIGN_ALIGN: usize = <Box<RcuBoxInner<T>, A> as > ForeignOwnable>::FOREIGN_ALIGN; > + > + type Borrowed<'a> = &'a T; > + type BorrowedMut<'a> = &'a T; > + > + fn into_foreign(self) -> *mut c_void { > + self.0.as_ptr().cast() > + } [Severity: Critical] Since `self` is consumed by value here without being wrapped in `ManuallyDrop` or passed to `core::mem::forget()`, won't it be dropped at the end of this method? This seems like it would immediately schedule the memory for RCU free, causing a use-after-free for the foreign code when it later accesses the pointer. > + > + unsafe fn from_foreign(ptr: *mut c_void) -> Self { [ ... ] > +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>)) }; > + } 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] Since `RcuBox` is generic over any custom `A: Allocator`, wouldn't calling `kvfree_call_rcu()` here break for allocators other than kmalloc or vmalloc? Does this need to always route through `A::free()` via `drop_rcu_box`, or should `A` be explicitly bounded to an allocator trait that supports `kvfree_rcu`? > + } > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
