Re: [PATCH 1/3] rust: rcu: add RcuBox type

[sashiko-bot@kernel.org]

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 <aliceryhl@google.com>

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/20260605133541.22569-1-boqun@kernel.org?part=1