Re: [PATCH 2/2] rust_binder: account for deferred work size in Thread::read()

[Carlos Llamas <cmllamas@google.com>]

On Sun, Nov 08, 2026 at 03:11:32PM +0000, Chris Wiseman wrote:
> When a looper thread replies with TF_DEFER_COMPLETE, its 4-byte
> BR_TRANSACTION_COMPLETE work item is queued via push_work_deferred() in
> inner.work_list without setting inner.process_work_list. On the next
> blocking BINDER_WRITE_READ ioctl, inner.should_use_process_work_queue()
> returns true and Thread::read() checks only whether the remaining output
> buffer can fit a single transaction (4-byte command code plus
> size_of::<uapi::binder_transaction_data_secctx>()).
> 
> If userspace provides a read buffer with 76 bytes remaining after the
> initial 4-byte BR_NOOP placeholder (read_size = 80), Thread::read()
> calls Self::get_work(), sleeps in Process.ready_threads, and receives an
> incoming synchronous transaction queued behind the deferred
> BR_TRANSACTION_COMPLETE in inner.work_list. Iteration 1 of the loop
> writes the 4-byte BR_TRANSACTION_COMPLETE, reducing writer.len() to 72
> bytes, and iteration 2 exits the loop without delivering the incoming
> transaction, leaving it stranded in the thread-local work_list.

ok, so if userspace is setting TF_DEFER_COMPLETE, the read buffer size
should reflect the possiblity of a BR_TRANSACTION_COMPLETE no? I'm not
sure this is a driver issue. Maybe I'm missing something?

I guess we are trying to match the behavior we have without the
TF_DEFER_COMPLETE and using a smaller read buffer e.g. 65-bytes. I
suppose this makes sense.

> 
> Add a size(&self) -> usize method to DeliverToRead returning the maximum
> number of bytes that do_work() may write to the user read buffer, and
> sum the sizes of all deferred items in inner.work_list when
> use_proc_queue is true. Include deferred_size on the right-hand side of
> the while condition in Thread::read() so the thread does not wait for
> global events when the output buffer lacks room for both its deferred
> work items and an incoming transaction, and subtract work.size() as each
> item is popped so subsequent loop iterations do not double-count items
> already written to writer.
> 
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Fixes: 8eb05e8e47f1 ("rust_binder: add TF_DEFER_COMPLETE flag for avoiding userspace roundtrip")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chris Wiseman <chriswiseman@google.com>
> ---
>  drivers/android/binder/freeze.rs           |  4 +++
>  drivers/android/binder/node.rs             |  8 +++++
>  drivers/android/binder/node/wrapper.rs     |  4 +++
>  drivers/android/binder/rust_binder_main.rs |  8 +++++
>  drivers/android/binder/thread.rs           | 34 ++++++++++++++++------
>  drivers/android/binder/transaction.rs      |  4 +++
>  6 files changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/android/binder/freeze.rs b/drivers/android/binder/freeze.rs
> index 69e058e6d818..5103bfac781f 100644
> --- a/drivers/android/binder/freeze.rs
> +++ b/drivers/android/binder/freeze.rs
> @@ -152,6 +152,10 @@ fn should_sync_wakeup(&self) -> bool {
>          false
>      }
>  
> +    fn size(&self) -> usize {
> +        size_of::<u32>() + size_of::<BinderFrozenStateInfo>()
> +    }
> +
>      #[inline(never)]
>      fn debug_print(&self, m: &SeqFile, prefix: &str, _tprefix: &str) -> Result {
>          seq_print!(m, "{}has frozen binder\n", prefix);
> diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs
> index efdafc202e27..4f4c892cba58 100644
> --- a/drivers/android/binder/node.rs
> +++ b/drivers/android/binder/node.rs
> @@ -754,6 +754,10 @@ fn should_sync_wakeup(&self) -> bool {
>          false
>      }
>  
> +    fn size(&self) -> usize {
> +        2 * (size_of::<u32>() + 2 * size_of::<u64>())
> +    }
> +
>      #[inline(never)]
>      fn debug_print(&self, m: &SeqFile, prefix: &str, _tprefix: &str) -> Result {
>          seq_print!(
> @@ -1149,6 +1153,10 @@ fn should_sync_wakeup(&self) -> bool {
>          false
>      }
>  
> +    fn size(&self) -> usize {
> +        size_of::<u32>() + size_of::<u64>()
> +    }
> +
>      #[inline(never)]
>      fn debug_print(&self, m: &SeqFile, prefix: &str, _tprefix: &str) -> Result {
>          let inner = self.inner.lock();
> diff --git a/drivers/android/binder/node/wrapper.rs b/drivers/android/binder/node/wrapper.rs
> index edfbccf6e6da..37826349eafb 100644
> --- a/drivers/android/binder/node/wrapper.rs
> +++ b/drivers/android/binder/node/wrapper.rs
> @@ -93,6 +93,10 @@ fn should_sync_wakeup(&self) -> bool {
>          false
>      }
>  
> +    fn size(&self) -> usize {
> +        2 * (size_of::<u32>() + 2 * size_of::<u64>())
> +    }
> +
>      #[inline(never)]
>      fn debug_print(&self, m: &SeqFile, prefix: &str, _tprefix: &str) -> Result {
>          seq_print!(
> diff --git a/drivers/android/binder/rust_binder_main.rs b/drivers/android/binder/rust_binder_main.rs
> index 56c786475237..361f3a987235 100644
> --- a/drivers/android/binder/rust_binder_main.rs
> +++ b/drivers/android/binder/rust_binder_main.rs
> @@ -163,6 +163,10 @@ fn do_work(
>      /// Generally only set to true for non-oneway transactions.
>      fn should_sync_wakeup(&self) -> bool;
>  
> +    /// Returns the maximum size in bytes that [`DeliverToRead::do_work`] may write to the read
> +    /// buffer.
> +    fn size(&self) -> usize;
> +
>      fn debug_print(&self, m: &SeqFile, prefix: &str, transaction_prefix: &str) -> Result;
>  }
>  
> @@ -279,6 +283,10 @@ fn should_sync_wakeup(&self) -> bool {
>          false
>      }
>  
> +    fn size(&self) -> usize {
> +        size_of::<u32>()
> +    }
> +
>      fn debug_print(&self, m: &SeqFile, prefix: &str, _tprefix: &str) -> Result {
>          seq_print!(m, "{}", prefix);
>          if self.skip.load(Relaxed) {
> diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
> index 3ca5c95695e4..af7a340adcb3 100644
> --- a/drivers/android/binder/thread.rs
> +++ b/drivers/android/binder/thread.rs
> @@ -1599,13 +1599,20 @@ fn read(self: &Arc<Self>, req: &mut BinderWriteRead, wait: bool) -> Result {
>              UserSlice::new(UserPtr::from_addr(read_start as usize), read_len as usize).writer(),
>              self,
>          );
> -        let (in_pool, has_transaction, thread_todo, use_proc_queue) = {
> +        let (in_pool, has_transaction, thread_todo, use_proc_queue, mut deferred_size) = {
>              let inner = self.inner.lock();
> +            let use_proc_queue = inner.should_use_process_work_queue();
> +            let deferred_size = if use_proc_queue {
> +                inner.work_list.iter().map(|w| w.size()).sum()
> +            } else {
> +                0
> +            };
>              (
>                  inner.is_looper(),
>                  inner.current_transaction.is_some(),
>                  !inner.work_list.is_empty(),
> -                inner.should_use_process_work_queue(),
> +                use_proc_queue,
> +                deferred_size,
>              )
>          };
>  
> @@ -1630,15 +1637,20 @@ fn read(self: &Arc<Self>, req: &mut BinderWriteRead, wait: bool) -> Result {
>  
>          // Loop doing work while there is room in the buffer.
>          let initial_len = writer.len();
> -        while writer.len() >= size_of::<uapi::binder_transaction_data_secctx>() + 4 {
> +        while writer.len() >= size_of::<uapi::binder_transaction_data_secctx>() + 4 + deferred_size
> +        {
>              match getter(self, wait && initial_len == writer.len()) {
> -                Ok(Some(work)) => match work.into_arc().do_work(self, &mut writer) {
> -                    Ok(true) => {}
> -                    Ok(false) => break,
> -                    Err(err) => {
> -                        return Err(err);
> +                Ok(Some(work)) => {
> +                    let work = work.into_arc();
> +                    deferred_size = deferred_size.saturating_sub(work.size());
> +                    match work.do_work(self, &mut writer) {
> +                        Ok(true) => {}
> +                        Ok(false) => break,
> +                        Err(err) => {
> +                            return Err(err);
> +                        }
>                      }
> -                },
> +                }
>                  Ok(None) => {
>                      break;
>                  }
> @@ -1798,6 +1810,10 @@ fn should_sync_wakeup(&self) -> bool {
>          false
>      }
>  
> +    fn size(&self) -> usize {
> +        size_of::<u32>()
> +    }
> +
>      fn debug_print(&self, m: &SeqFile, prefix: &str, _tprefix: &str) -> Result {
>          seq_print!(
>              m,
> diff --git a/drivers/android/binder/transaction.rs b/drivers/android/binder/transaction.rs
> index 48ac942bb75a..6909d7a8a219 100644
> --- a/drivers/android/binder/transaction.rs
> +++ b/drivers/android/binder/transaction.rs
> @@ -762,6 +762,10 @@ fn should_sync_wakeup(&self) -> bool {
>          !self.flags.is_oneway()
>      }
>  
> +    fn size(&self) -> usize {
> +        size_of::<u32>() + size_of::<BinderTransactionDataSecctx>()
> +    }
> +
>      fn debug_print(&self, m: &SeqFile, _prefix: &str, tprefix: &str) -> Result {
>          self.debug_print_inner(m, tprefix);
>          Ok(())
> -- 
> 2.56.0.317.g454f8302e1-goog
> 

It seems sashiko has found a legit issue, see:
https://sashiko.dev/#/patchset/18e61a85b0d13e9c.3137322e32332e3235332e313736.9c152.0000sendgmr%40google.com

--
Carlos Llamas