Re: [PATCH 4/5] rust_binder: consolidate transaction failure prints

[Carlos Llamas <cmllamas@google.com>]

On Tue, Jun 23, 2026 at 03:38:07PM +0000, Alice Ryhl wrote:
> When a transaction fails, it currently hits multiple pr_warn!
> invocations meaning that a single failure can result in several lines in
> the kernel log. This is unnecessary, so consolidate them into one print
> used for all transaction failures.
> 
> For ENOSPC errors we want to know the transaction size, so a field is
> added to TransactionInfo that bubbles up this information to the new
> print location.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  drivers/android/binder/error.rs       |  4 ---
>  drivers/android/binder/thread.rs      | 59 ++++++++++++++++++++---------------
>  drivers/android/binder/transaction.rs | 21 +++----------
>  rust/kernel/error.rs                  |  2 +-
>  4 files changed, 39 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/android/binder/error.rs b/drivers/android/binder/error.rs
> index 1296072c35d9..aed1c747640b 100644
> --- a/drivers/android/binder/error.rs
> +++ b/drivers/android/binder/error.rs
> @@ -37,10 +37,6 @@ pub(crate) fn new_frozen_oneway() -> Self {
>              source: None,
>          }
>      }
> -
> -    pub(crate) fn is_dead(&self) -> bool {
> -        self.reply == BR_DEAD_REPLY
> -    }
>  }
>  
>  /// Convert an errno into a `BinderError` and store the errno used to construct it. The errno
> diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
> index def00edce9c4..a5b038877747 100644
> --- a/drivers/android/binder/thread.rs
> +++ b/drivers/android/binder/thread.rs
> @@ -25,7 +25,7 @@
>  use crate::{
>      allocation::{Allocation, AllocationView, BinderObject, BinderObjectRef, NewAllocation},
>      defs::*,
> -    error::BinderResult,
> +    error::{BinderError, BinderResult},
>      process::{GetWorkOrRegister, Process},
>      ptr_align,
>      stats::GLOBAL_STATS,
> @@ -1018,18 +1018,9 @@ pub(crate) fn copy_transaction_data(
>                  .ok_or(ENOMEM)?,
>              size_of::<u64>(),
>          );
> +        info.debug_total_size = len;
>          let secctx_off = aligned_data_size + offsets_size + buffers_size;
> -        let mut alloc = match to_process.buffer_alloc(debug_id, len, info) {
> -            Ok(alloc) => alloc,
> -            Err(err) => {
> -                pr_warn!(
> -                    "Failed to allocate buffer. len:{}, is_oneway:{}",
> -                    len,
> -                    info.is_oneway(),
> -                );
> -                return Err(err);
> -            }
> -        };
> +        let mut alloc = to_process.buffer_alloc(debug_id, len, info)?;
>  
>          let mut buffer_reader = UserSlice::new(info.data_ptr, data_size).reader();
>          let mut end_of_previous_object = 0;
> @@ -1259,6 +1250,15 @@ fn read_transaction_info(
>  
>          info.debug_id = super::next_debug_id();
>  
> +        // We don't yet know the real message size because it depends on the secctx size, so for
> +        // now write an estimate. When the real value is computed, this estimate is replaced.
> +        // Writing an estimate here ensures that the approx message size can still be printed if
> +        // the transaction fails before it is computed exactly.
> +        info.debug_total_size = info
> +            .data_size
> +            .saturating_add(info.offsets_size)
> +            .saturating_add(info.buffers_size);

hmm, I'm not so sure about this "estimation". I can see how it might be
confusing against the "final" size, after secctx and alignment, bla bla.
I wonder if it would be best to leave this as zero (until this is known)
or maybe have the "raw" individual values from userspace?

> +
>          Ok(())
>      }
>  
> @@ -1277,6 +1277,9 @@ fn transaction(self: &Arc<Self>, cmd: u32, reader: &mut UserSliceReader) -> Resu
>              self.transaction_inner(&mut info)
>          };
>  
> +        // This runs when return work is passed to the caller. This is not
> +        // always the same as the transaction failing, as reply errors are
> +        // delivered to the remote process.
>          if let Err(err) = ret {
>              self.push_return_work(err.reply);
>              if err.reply != BR_TRANSACTION_COMPLETE {
> @@ -1290,13 +1293,6 @@ fn transaction(self: &Arc<Self>, cmd: u32, reader: &mut UserSliceReader) -> Resu
>                              ExtendedError::new(info.debug_id as u32, err.reply, source.to_errno());
>                      }
>                  }
> -
> -                pr_warn!(
> -                    "{}:{} transaction to {} failed: {err:?}",
> -                    info.from_pid,
> -                    info.from_tid,
> -                    info.to_pid
> -                );
>              }
>          }
>  
> @@ -1305,8 +1301,27 @@ fn transaction(self: &Arc<Self>, cmd: u32, reader: &mut UserSliceReader) -> Resu
>              // useful in case the transaction failed with BR_TRANSACTION_PENDING_FROZEN.
>              info.report_netlink(BR_ONEWAY_SPAM_SUSPECT, &self.process.ctx);
>          }
> +        // This runs when the transaction failed.
>          if info.reply != 0 {
>              info.report_netlink(info.reply, &self.process.ctx);
> +            let err = BinderError {
> +                reply: info.reply,
> +                source: Error::try_from_errno(info.errno),
> +            };
> +            pr_warn!(
> +                "{}:{} {} to {} failed: {err:?}, {} bytes\n",
> +                info.from_pid,
> +                info.from_tid,
> +                if info.is_reply {
> +                    "reply"
> +                } else if info.is_oneway() {
> +                    "oneway"
> +                } else {
> +                    "transaction"
> +                },
> +                info.to_pid,
> +                info.debug_total_size
> +            );
>          }
>  
>          Ok(())
> @@ -1374,12 +1389,6 @@ fn reply_inner(self: &Arc<Self>, info: &mut TransactionInfo) -> BinderResult {
>              // At this point we only return `BR_TRANSACTION_COMPLETE` to the caller, and we must let
>              // the sender know that the transaction has completed (with an error in this case).
>  
> -            pr_warn!(
> -                "{}:{} reply to {} failed: {err:?}",
> -                info.from_pid,
> -                info.from_tid,
> -                info.to_pid
> -            );
>              let param = err.source.as_ref().map_or(0, |e| e.to_errno());
>              let ee = ExtendedError::new(info.debug_id as u32, err.reply, param);
>              orig.from
> diff --git a/drivers/android/binder/transaction.rs b/drivers/android/binder/transaction.rs
> index 9aefa01599fb..316627e5f9ac 100644
> --- a/drivers/android/binder/transaction.rs
> +++ b/drivers/android/binder/transaction.rs
> @@ -39,6 +39,7 @@ pub(crate) struct TransactionInfo {
>      pub(crate) offsets_ptr: UserPtr,
>      pub(crate) offsets_size: usize,
>      pub(crate) buffers_size: usize,
> +    pub(crate) debug_total_size: usize,
>      pub(crate) target_handle: u32,
>      pub(crate) errno: i32,
>      pub(crate) reply: u32,
> @@ -138,21 +139,13 @@ pub(crate) fn new(
>          let txn_security_ctx = node_ref.node.flags & FLAT_BINDER_FLAG_TXN_SECURITY_CTX != 0;
>          let mut txn_security_ctx_off = if txn_security_ctx { Some(0) } else { None };
>          let to = node_ref.node.owner.clone();
> -        let mut alloc = match from.copy_transaction_data(
> +        let mut alloc = from.copy_transaction_data(
>              to.clone(),
>              info,
>              info.debug_id,
>              allow_fds,
>              txn_security_ctx_off.as_mut(),
> -        ) {
> -            Ok(alloc) => alloc,
> -            Err(err) => {
> -                if !err.is_dead() {
> -                    pr_warn!("Failure in copy_transaction_data: {:?}", err);
> -                }
> -                return Err(err);
> -            }
> -        };
> +        )?;
>          if info.is_oneway() {
>              if from_parent.is_some() {
>                  pr_warn!("Oneway transaction should not be in a transaction stack.");
> @@ -193,13 +186,7 @@ pub(crate) fn new_reply(
>          allow_fds: bool,
>      ) -> BinderResult<DLArc<Self>> {
>          let mut alloc =
> -            match from.copy_transaction_data(to.clone(), info, info.debug_id, allow_fds, None) {
> -                Ok(alloc) => alloc,
> -                Err(err) => {
> -                    pr_warn!("Failure in copy_transaction_data: {:?}", err);
> -                    return Err(err);
> -                }
> -            };
> +            from.copy_transaction_data(to.clone(), info, info.debug_id, allow_fds, None)?;
>          if info.flags & TF_CLEAR_BUF != 0 {
>              alloc.set_info_clear_on_drop();
>          }
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index 05cf869ac090..89c247f150b3 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -137,7 +137,7 @@ pub fn from_errno(errno: crate::ffi::c_int) -> Error {
>      /// Creates an [`Error`] from a kernel error code.
>      ///
>      /// Returns [`None`] if `errno` is out-of-range.
> -    const fn try_from_errno(errno: crate::ffi::c_int) -> Option<Error> {
> +    pub const fn try_from_errno(errno: crate::ffi::c_int) -> Option<Error> {
>          if errno < -(bindings::MAX_ERRNO as i32) || errno >= 0 {
>              return None;
>          }
> 
> -- 
> 2.55.0.rc0.799.gd6f94ed593-goog
>