Message-Id: <20260204091147.32511-1-jongan.kim@lge.com>
Date: Wed,  4 Feb 2026 18:11:47 +0900
From: jongan.kim@....com
To: gary@...yguo.net,
	aliceryhl@...gle.com
Cc: a.hindborg@...nel.org,
	arve@...roid.com,
	bjorn3_gh@...tonmail.com,
	boqun.feng@...il.com,
	brauner@...nel.org,
	cmllamas@...gle.com,
	dakr@...nel.org,
	daniel.almeida@...labora.com,
	gregkh@...uxfoundation.org,
	heesu0025.kim@....com,
	ht.hong@....com,
	jongan.kim@....com,
	jungsu.hwang@....com,
	kernel-team@...roid.com,
	linux-kernel@...r.kernel.org,
	lossin@...nel.org,
	ojeda@...nel.org,
	rust-for-linux@...r.kernel.org,
	sanghun.lee@....com,
	seulgi.lee@....com,
	sunghoon.kim@....com,
	tamird@...il.com,
	tkjos@...roid.com,
	tmgross@...ch.edu,
	viresh.kumar@...aro.org,
	vitaly.wool@...sulko.se,
	yury.norov@...il.com
Subject: Re: [PATCH v3 3/3] rust_binder: handle PID namespace conversion for freeze operation

On Tue Feb 3, 2026 at 12:59:45PM +0000, Gary Guo wrote:
> On Tue Feb 3, 2026 at 6:59 AM GMT, jongan.kim wrote:
> > From: HeeSu Kim <heesu0025.kim@....com>
> >
> > Port PID namespace conversion logic from C binder to the Rust
> > implementation.
> >
> > Without namespace conversion, freeze operations from non-init namespaces
> > can match wrong processes due to PID collision. This adds proper
> > conversion to ensure freeze operations target the correct process.
> >
> > Signed-off-by: HeeSu Kim <heesu0025.kim@....com>
> > ---
> > v2 -> v3:
> > - Use task::Pid typedef instead of u32/i32
> > - Use PidNamespace::init_ns() instead of init_pid_ns()
> > - Compare PidNamespace directly with == instead of raw pointers
> > - Use Pid::find_vpid() and pid.pid_task() (dropped _with_guard suffix)
> > - Fix rustfmt import ordering (rcu before Arc)
> > - Rename TaskPid alias to PidT for clearer pid_t type indication
> > - Use task.group_leader().pid() instead of tgid_nr_ns() for consistency with C
> >
> >  drivers/android/binder/process.rs | 37 +++++++++++++++++++++++++++----
> >  1 file changed, 33 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
> > index 132055b4790f..ea30bfac2e0b 100644
> > --- a/drivers/android/binder/process.rs
> > +++ b/drivers/android/binder/process.rs
> > @@ -22,6 +22,8 @@
> >      id_pool::IdPool,
> >      list::{List, ListArc, ListArcField, ListLinks},
> >      mm,
> > +    pid::Pid,
> > +    pid_namespace::PidNamespace,
> >      prelude::*,
> >      rbtree::{self, RBTree, RBTreeNode, RBTreeNodeReservation},
> >      seq_file::SeqFile,
> > @@ -29,9 +31,9 @@
> >      sync::poll::PollTable,
> >      sync::{
> >          lock::{spinlock::SpinLockBackend, Guard},
> > -        Arc, ArcBorrow, CondVar, CondVarTimeoutResult, Mutex, SpinLock, UniqueArc,
> > +        rcu, Arc, ArcBorrow, CondVar, CondVarTimeoutResult, Mutex, SpinLock, UniqueArc,
> >      },
> > -    task::Task,
> > +    task::{Pid as PidT, Task},
> >      types::ARef,
> >      uaccess::{UserSlice, UserSliceReader},
> >      uapi,
> > @@ -1498,17 +1500,42 @@ pub(crate) fn ioctl_freeze(&self, info: &BinderFreezeInfo) -> Result {
> >      }
> >  }
> > 
> > +/// Convert a PID from the current namespace to the global (init) namespace.
> > +fn convert_to_init_ns_tgid(pid: PidT) -> Result<PidT> {
> > +    let current = kernel::current!();
> > +    let init_ns = PidNamespace::init_ns();
> > +
> > +    if current.active_pid_ns() == Some(init_ns) {
> > +        // Already in init namespace.
> > +        return Ok(pid);
> > +    }
> > +
> > +    if pid == 0 {
> > +        return Err(EINVAL);
> > +    }
> > +
> > +    let rcu_guard = rcu::read_lock();
> > +
> > +    let pid_struct = Pid::find_vpid(pid, &rcu_guard).ok_or(ESRCH)?;
> > +    let task = pid_struct.pid_task(&rcu_guard).ok_or(ESRCH)?;
> > +
> > +    Ok(task.group_leader().pid())
> > +}
> > +
> >  fn get_frozen_status(data: UserSlice) -> Result {
> >      let (mut reader, mut writer) = data.reader_writer();
> > 
> >      let mut info = reader.read::<BinderFrozenStatusInfo>()?;
> > +
> > +    let init_ns_pid = convert_to_init_ns_tgid(info.pid as PidT)?;
> > +
> >      info.sync_recv = 0;
> >      info.async_recv = 0;
> >      let mut found = false;
> > 
> >      for ctx in crate::context::get_all_contexts()? {
> >          ctx.for_each_proc(|proc| {
> > -            if proc.task.pid() == info.pid as _ {
> > +            if proc.task.pid() == init_ns_pid as _ {
> >                  found = true;
> >                  let inner = proc.inner.lock();
> >                  let txns_pending = inner.txns_pending_locked();
> > @@ -1530,13 +1557,15 @@ fn get_frozen_status(data: UserSlice) -> Result {
> >  fn ioctl_freeze(reader: &mut UserSliceReader) -> Result {
> >      let info = reader.read::<BinderFreezeInfo>()?;
> > 
> > +    let init_ns_pid = convert_to_init_ns_tgid(info.pid as PidT)?;
> > +
> >      // Very unlikely for there to be more than 3, since a process normally uses at most binder and
> >      // hwbinder.
> >      let mut procs = KVec::with_capacity(3, GFP_KERNEL)?;
> > 
> >      let ctxs = crate::context::get_all_contexts()?;
> >      for ctx in ctxs {
> > -        for proc in ctx.get_procs_with_pid(info.pid as i32)? {
> > +        for proc in ctx.get_procs_with_pid(init_ns_pid)? {
> 
> I think this function already has `ARef<Task>` for all the process, so it feels
> to me that the search should be simply based on task, rather than PID.
> 
> I.e. instead of pid_t -> struct pid -> struct tasks -> pid_t and then search
> with it, we first get `Task` from VPID, and then search directly with it. This
> would also make it no longer necessary to have the init namespace check.
> 
> (BTW, the current impl looks quite inefficient, get_procs_with_pid returns a vec
> which is pushed again to a vec, perhaps using a callback or simply passing in a
> `&mut Vec` is better?)
> 
> Best,
> Gary
> 
> >              procs.push(proc, GFP_KERNEL)?;
> >          }
> >      }

Thank you for the suggestions for more efficient structure.

Your proposal to use task-based search instead of PID-based search
could simplify the logic.

However, I have a few considerations:
1. The current implementation maintains consistency with the existing
C binder implementation. Any structural change here would ideally be
reflected in the  C code as well to keep both implementations aligned.

2. Since the majority of binder usage occurs in the init namespace, the
current early return (checking task_is_in_init_pid_ns) avoids the overhead
of find_vpid() and pid_task() calls in the common case. If we always go
through the task lookup path, this optimization would be lost.

3. Regarding the search mechanism and vec efficiency improvements you
mentioned, these seem like valuable optimizations but would represent
a broader architectural change to the binder driver.

I'd like to get input from the binder driver maintainers on whether:
- This kind of structural change is desired for the binder driver
- If so, whether it should be done as part of this PID namespace fix or
as a separate refactoring effort
- Whether both C and Rust implementations should be updated together

Alice, could you please advise on the preferred approach here?

Thanks,
JongAn Kim

Powered by blists - more mailing lists