Re: [PATCH v2 5/6] rust_binder: avoid destructors in insert_or_update_handle()

[Matthew Maurer]

On Tue, Jun 9, 2026 at 2:34 AM Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote:
>
> The insert_or_update_handle() function currently has two places where it
> drops objects under the node_refs lock. In preparation for changing
> node_refs into a spinlock, update the code to either entirely remove the
> codepath or drop the node_refs lock first before running the destructor.
>
> This also has the side-benefit that we avoid traversing the by_node
> rbtree twice. Currently it's first traversed to see if the new node is
> present, and then traversed again to insert it. By saving the
> VacantEntry from the first lookup, we can perform the insertion without
> traversing the tree again.
>
> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> ---
>  drivers/android/binder/process.rs | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
> index c8e9fd0d0472..268d78f8cfb3 100644
> --- a/drivers/android/binder/process.rs
> +++ b/drivers/android/binder/process.rs
> @@ -861,14 +861,17 @@ pub(crate) fn insert_or_update_handle(
>          let handle = unused_id.as_u32();
>
>          // Do a lookup again as node may have been inserted before the lock was reacquired.
> -        if let Some(handle_ref) = refs.by_node.get(&node_ref.node.global_id()) {
> -            let handle = *handle_ref;
> -            let info = refs.by_handle.get_mut(&handle).unwrap();
> -            info.node_ref().absorb(node_ref);
> -            return Ok(handle);
> -        }
> +        let by_node_slot = match refs.by_node.entry(node_ref.node.global_id()) {
> +            rbtree::Entry::Vacant(by_node_slot) => by_node_slot,
> +            rbtree::Entry::Occupied(handle_ref) => {
> +                // The node was inserted by another thread while we didn't hold the lock.
> +                let handle = handle_ref.get();
> +                let info = refs.by_handle.get_mut(handle).unwrap();
> +                info.node_ref().absorb(node_ref);
> +                return Ok(*handle);
> +            }
> +        };
>
> -        let gid = node_ref.node.global_id();
>          let (info_proc, info_node) = {
>              let info_init = NodeRefInfo::new(node_ref, handle, self.into());
>              match info.pin_init_with(info_init) {
> @@ -884,6 +887,9 @@ pub(crate) fn insert_or_update_handle(
>          // first thing in `deferred_release`, process cleanup will not miss the items inserted into
>          // `refs` below.
>          if self.inner.lock().is_dead {
> +            // Explicitly drop the lock so that `info_proc` and `info_node` are dropped outside of
> +            // the lock.
> +            drop(refs_lock);
>              return Err(ESRCH);
>          }
>
> @@ -891,7 +897,7 @@ pub(crate) fn insert_or_update_handle(
>          // `info_node` into the right node's `refs` list.
>          unsafe { info_proc.node_ref2().node.insert_node_info(info_node) };
>
> -        refs.by_node.insert(reserve1.into_node(gid, handle));
> +        by_node_slot.insert(handle, reserve1);
>          by_handle_slot.insert(info_proc, reserve2);
>          unused_id.acquire();
>          Ok(handle)
>
> --
> 2.54.0.1064.gd145956f57-goog
>
>

Reviewed-by: Matthew Maurer <mmaurer@xxxxxxxxxx>