On Fri, Jun 05, 2026 at 01:49:35PM +0000, [email protected] wrote: > 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. > -- > [...] > > + > > + 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`? >
There is a "TODO" here to describe why currently kvfree_call_rcu() is fine, is this a real issue in the current code base? Regards, Boqun > > + } > > + } > > +} > > -- > Sashiko AI review ยท > https://sashiko.dev/#/patchset/[email protected]?part=1
