Message-ID: <aW4DWebqCg1c8Fgb@google.com>
Date: Mon, 19 Jan 2026 10:11:37 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Jason Hall <jason.kei.hall@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	"Arve Hjønnevåg" <arve@...roid.com>, Todd Kjos <tkjos@...gle.com>, Carlos Llamas <cmllamas@...gle.com>, 
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rust_binder: refactor context management to use KVVec

On Sun, Jan 18, 2026 at 07:26:23AM -0700, Jason Hall wrote:
> Replace the linked list management in context.rs with KVVec.
> This simplifies the ownership model by using standard
> Arc-based tracking and moves away from manual unsafe list removals.
> 
> The refactor improves memory safety by leveraging Rust's contiguous
> collection types while maintaining proper error propagation for
> allocation failures during process registration.
> 
> Suggested-by: Alice Ryhl <aliceryhl@...gle.com>
> Link: https://github.com/rust-for-linux/linux/issues/1215
> Signed-off-by: Jason Hall <jason.kei.hall@...il.com>

> -    pub(crate) fn register_process(self: &Arc<Self>, proc: ListArc<Process>) {
> +    pub(crate) fn register_process(self: &Arc<Self>, proc: Arc<Process>) -> Result {
>          if !Arc::ptr_eq(self, &proc.ctx) {
>              pr_err!("Context::register_process called on the wrong context.");
> -            return;
> +            return Err(EINVAL);
>          }
> -        self.manager.lock().all_procs.push_back(proc);
> +        self.manager
> +            .lock()
> +            .all_procs
> +            .push(proc, GFP_KERNEL)
> +            .map_err(Error::from)

This can be simplified to:

self.manager
    .lock()
    .all_procs
    .push(proc, GFP_KERNEL)?;
Ok(())

>      }
>  
>      pub(crate) fn deregister_process(self: &Arc<Self>, proc: &Process) {
> @@ -108,8 +99,12 @@ pub(crate) fn deregister_process(self: &Arc<Self>, proc: &Process) {
>              pr_err!("Context::deregister_process called on the wrong context.");
>              return;
>          }
> -        // SAFETY: We just checked that this is the right list.
> -        unsafe { self.manager.lock().all_procs.remove(proc) };
> +        // Safe removal using retain
> +        self.manager.lock().all_procs.retain(|p| {
> +            let p1 = Arc::as_ptr(p);
> +            let p2 = proc as *const Process;
> +            p1 != p2
> +        });

Please use Arc::ptr_eq() for this comparison (in both places). You may
change deregister_process to take an &Arc<Process> as argument to make
this work.

Also, I think we should shrink the `all_procs` vector if it becomes too
much larger than the number of procs. Let's say ... after removing a
process, if the capacity() is less than half of the length and less than
128, then we shrink the capacity.

>      }
>  
>      pub(crate) fn set_manager_node(&self, node_ref: NodeRef) -> Result {
> @@ -158,13 +153,11 @@ pub(crate) fn for_each_proc<F>(&self, mut func: F)
>          }
>      }
>  
> -    pub(crate) fn get_all_procs(&self) -> Result<KVec<Arc<Process>>> {
> +    pub(crate) fn get_all_procs(&self) -> Result<KVVec<Arc<Process>>> {
>          let lock = self.manager.lock();
> -        let count = lock.all_procs.iter().count();
> -
> -        let mut procs = KVec::with_capacity(count, GFP_KERNEL)?;
> -        for proc in &lock.all_procs {
> -            procs.push(Arc::from(proc), GFP_KERNEL)?;
> +        let mut procs = KVVec::with_capacity(lock.all_procs.len(), GFP_KERNEL)?;
> +        for proc in lock.all_procs.iter() {
> +            procs.push(Arc::clone(proc), GFP_KERNEL)?;
>          }

Let's change get_procs_with_pid() too. Both to use KVVec, and also let's
have it iterate all_procs() directly.

> diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
> index 132055b4790f..c3676fc7785d 100644
> --- a/drivers/android/binder/process.rs
> +++ b/drivers/android/binder/process.rs
> @@ -513,7 +513,9 @@ fn new(ctx: Arc<Context>, cred: ARef<Credential>) -> Result<Arc<Self>> {
>          )?;
>  
>          let process = list_process.clone_arc();
> -        process.ctx.register_process(list_process);
> +        process
> +            .ctx
> +            .register_process(Arc::from(list_process.as_arc_borrow()))?;

You do not need to create a ListArc<Process> in the first place.

 impl Process {
     fn new(ctx: Arc<Context>, cred: ARef<Credential>) -> Result<Arc<Self>> {
         let current = kernel::current!();
-        let list_process = ListArc::pin_init::<Error>(
+        let process = Arc::pin_init::<Error>(
             try_pin_init!(Process {
                 ctx,
                 cred,
                 inner <- kernel::new_spinlock!(ProcessInner::new(), "Process::inner"),
                 pages <- ShrinkablePageRange::new(&super::BINDER_SHRINKER),
                 node_refs <- kernel::new_mutex!(ProcessNodeRefs::new(), "Process::node_refs"),
                 freeze_wait <- kernel::new_condvar!("Process::freeze_wait"),
                 task: current.group_leader().into(),
                 defer_work <- kernel::new_work!("Process::defer_work"),
                 links <- ListLinks::new(),
                 stats: BinderStats::new(),
             }),
             GFP_KERNEL,
         )?;
 
-        let process = list_process.clone_arc();
-        process
-            .ctx
-            .register_process(Arc::from(list_process.as_arc_borrow()))?;
+        process.ctx.register_process(process.clone())?;
 
         Ok(process)
     }

Alice

Powered by blists - more mailing lists