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

[Alice Ryhl]

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@xxxxxxxxxx>
---
 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);
+
         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