Re: [PATCH 2/3] rust: maple_tree: add load_rcu()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Lockless traversal in `load_rcu()` causes a use-after-free of internal mapletree nodes because the tree is not initialized with `MT_FLAGS_USE_RCU`.
- [High] Unsound API: `RcuBox::new` safely allows types with blocking destructors to execute their `Drop` implementation in RCU softirq context, leading to atomic context violations.
--

commit b7609fa7d8f12dc31f5e195b6f4d6155f2a20574
Author: Alice Ryhl <aliceryhl@google.com>

rust: maple_tree: add load_rcu()

This commit adds a load_rcu() method to MapleTree, allowing lookups under an
RCU read lock without taking the spinlock.

> diff --git a/rust/kernel/maple_tree.rs b/rust/kernel/maple_tree.rs
> index 265d6396a78a1..1499191b8935a 100644
> --- a/rust/kernel/maple_tree.rs
> +++ b/rust/kernel/maple_tree.rs

[ ... ]

> @@ -233,6 +237,54 @@ pub fn erase(&self, index: usize) -> Option<T> {
>          unsafe { T::try_from_foreign(ret) }
>      }
>  
> +    /// Load the value at the given index with rcu.
> +    ///
> +    /// # Examples
> +    ///
> +    /// Read the value under an rcu read lock. Even if the value is removed, it remains accessible
> +    /// for one rcu grace period.
> +    ///
> +    /// ```ignore
> +    /// use kernel::{
> +    ///     maple_tree::MapleTree,
> +    ///     sync::rcu::{self, RcuBox},
> +    /// };
> +    ///
> +    /// let tree = KBox::pin_init(MapleTree::<RcuBox<i32>>::new(), GFP_KERNEL)?;
> +    ///
> +    /// let ten = RcuBox::new(10, GFP_KERNEL)?;

[Severity: High]
Is it safe to allow arbitrary types to be managed via RcuBox?

RcuBox::new() is a safe function that accepts any T: Send. It lacks trait
bounds to restrict T to types that are safe to drop in atomic contexts.

When an RcuBox is dropped, if T requires dropping, it schedules drop_rcu_box()
using bindings::call_rcu(). The drop_rcu_box() callback executes in an RCU
softirq context, where sleeping or blocking is strictly forbidden.

If a safe T blocks during its Drop implementation (for example, safely
acquiring a kernel::sync::Mutex on drop), will this execute in softirq
context and cause a scheduling-while-atomic panic?

> +    /// tree.insert(100, ten, GFP_KERNEL)?;
> +    ///
> +    /// let rcu_read_lock = rcu::Guard::new();
> +    /// let ten = tree.load_rcu(100, &rcu_read_lock);
> +    /// assert_eq!(ten, Some(&10));
> +    ///
> +    /// // Even if the value gets removed, we may continue to access it for one rcu grace period.
> +    /// tree.erase(100);
> +    /// assert_eq!(ten, Some(&10));
> +    /// # Ok::<_, Error>(())
> +    /// ```
> +    #[inline]
> +    pub fn load_rcu<'rcu>(
> +        &self,
> +        index: usize,
> +        _rcu: &'rcu rcu::Guard,
> +    ) -> Option<T::RcuBorrowed<'rcu>>
> +    where
> +        T: ForeignOwnableRcu,
> +    {
> +        // SAFETY: `self.tree` contains a valid maple tree.
> +        let ret = unsafe { bindings::mtree_load(self.tree.get(), index) };

[Severity: Critical]
Does this lockless traversal risk a use-after-free?

Looking at MapleTree::new() and MapleTreeAlloc::new() in
rust/kernel/maple_tree.rs, the tree is initialized with 0 or
bindings::MT_FLAGS_ALLOC_RANGE respectively, without bindings::MT_FLAGS_USE_RCU.

If MT_FLAGS_USE_RCU is not set, structural modifications to the tree (like
node splits or merges) will synchronously free the replaced internal tree
nodes directly to the slab allocator via kmem_cache_free(), completely
bypassing call_rcu().

Could a concurrent load_rcu() walking the tree dereference a node pointer
that was just synchronously freed by a writer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605133541.22569-1-boqun@kernel.org?part=2