* [PATCH v2] rust_binder: fix BINDER_GET_EXTENDED_ERROR
@ 2026-06-04 11:37 Alice Ryhl
2026-06-04 12:58 ` Alice Ryhl
0 siblings, 1 reply; 2+ messages in thread
From: Alice Ryhl @ 2026-06-04 11:37 UTC (permalink / raw)
To: Greg Kroah-Hartman, Carlos Llamas
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Danilo Krummrich, rust-for-linux, linux-kernel, stable
This code currently copies the ExtendedError struct to the stack,
modifies the copy, and then doesn't modify the original. Thus, fix it.
Furthermore, errors when replying must be delivered directly to the
remote thread, so update deliver_reply() to take an extended error
argument.
Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v2:
- Also handle extended error for replies.
- Link to v1: https://lore.kernel.org/r/20260527-set-extended-error-v1-1-407b4b466035@google.com
---
drivers/android/binder/error.rs | 13 +++----
drivers/android/binder/thread.rs | 65 +++++++++++++++++++++++++----------
drivers/android/binder/transaction.rs | 15 ++++----
3 files changed, 58 insertions(+), 35 deletions(-)
diff --git a/drivers/android/binder/error.rs b/drivers/android/binder/error.rs
index 45d85d4c2815..63c8d90c409a 100644
--- a/drivers/android/binder/error.rs
+++ b/drivers/android/binder/error.rs
@@ -73,20 +73,17 @@ impl fmt::Debug for BinderError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self.reply {
BR_FAILED_REPLY => match self.source.as_ref() {
- Some(source) => f
- .debug_struct("BR_FAILED_REPLY")
- .field("source", source)
- .finish(),
+ Some(source) => source.fmt(f),
None => f.pad("BR_FAILED_REPLY"),
},
BR_DEAD_REPLY => f.pad("BR_DEAD_REPLY"),
BR_FROZEN_REPLY => f.pad("BR_FROZEN_REPLY"),
BR_TRANSACTION_PENDING_FROZEN => f.pad("BR_TRANSACTION_PENDING_FROZEN"),
BR_TRANSACTION_COMPLETE => f.pad("BR_TRANSACTION_COMPLETE"),
- _ => f
- .debug_struct("BinderError")
- .field("reply", &self.reply)
- .finish(),
+ _ => match self.source.as_ref() {
+ Some(source) => source.fmt(f),
+ None => f.pad("OTHER_ERROR"),
+ },
}
}
}
diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs
index 97d5f31e8fe3..334f4b49511d 100644
--- a/drivers/android/binder/thread.rs
+++ b/drivers/android/binder/thread.rs
@@ -495,9 +495,16 @@ pub(crate) fn debug_print(self: &Arc<Self>, m: &SeqFile, print_all: bool) -> Res
Ok(())
}
+ pub(crate) fn clear_extended_error(&self, debug_id: usize) {
+ self.inner.lock().extended_error = ExtendedError::new(debug_id as u32, BR_OK, 0);
+ }
+
pub(crate) fn get_extended_error(&self, data: UserSlice) -> Result {
let mut writer = data.writer();
- let ee = self.inner.lock().extended_error;
+ let mut inner = self.inner.lock();
+ let ee = inner.extended_error;
+ inner.extended_error = ExtendedError::new(0, BR_OK, 0);
+ drop(inner);
writer.write(&ee)?;
Ok(())
}
@@ -1109,7 +1116,10 @@ fn unwind_transaction_stack(self: &Arc<Self>) {
inner.pop_transaction_to_reply(thread.as_ref())
} {
let reply = Err(BR_DEAD_REPLY);
- if !transaction.from.deliver_single_reply(reply, &transaction) {
+ if !transaction
+ .from
+ .deliver_single_reply(reply, &transaction, None)
+ {
break;
}
@@ -1121,8 +1131,9 @@ pub(crate) fn deliver_reply(
&self,
reply: Result<DLArc<Transaction>, u32>,
transaction: &DArc<Transaction>,
+ extended_error: Option<ExtendedError>,
) {
- if self.deliver_single_reply(reply, transaction) {
+ if self.deliver_single_reply(reply, transaction, extended_error) {
transaction.from.unwind_transaction_stack();
}
}
@@ -1136,6 +1147,7 @@ fn deliver_single_reply(
&self,
reply: Result<DLArc<Transaction>, u32>,
transaction: &DArc<Transaction>,
+ extended_error: Option<ExtendedError>,
) -> bool {
if let Ok(transaction) = &reply {
crate::trace::trace_transaction(true, transaction, Some(&self.task));
@@ -1152,6 +1164,12 @@ fn deliver_single_reply(
return true;
}
+ if let Some(ee) = extended_error {
+ if inner.extended_error.command == BR_OK {
+ inner.extended_error = ee;
+ }
+ }
+
match reply {
Ok(work) => {
inner.push_work(work);
@@ -1222,6 +1240,9 @@ fn read_transaction_info(
info.buffers_size = td.buffers_size as usize;
// SAFETY: Above `read` call initializes all bytes, so this union read is ok.
info.target_handle = unsafe { td.transaction_data.target.handle };
+
+ info.debug_id = super::next_debug_id();
+
Ok(())
}
@@ -1230,6 +1251,8 @@ fn transaction(self: &Arc<Self>, cmd: u32, reader: &mut UserSliceReader) -> Resu
let mut info = TransactionInfo::zeroed();
self.read_transaction_info(cmd, reader, &mut info)?;
+ self.clear_extended_error(info.debug_id);
+
let ret = if info.is_reply {
self.reply_inner(&mut info)
} else if info.is_oneway() {
@@ -1239,23 +1262,21 @@ fn transaction(self: &Arc<Self>, cmd: u32, reader: &mut UserSliceReader) -> Resu
};
if let Err(err) = ret {
- if err.reply != BR_TRANSACTION_COMPLETE {
- info.reply = err.reply;
- }
-
self.push_return_work(err.reply);
- if let Some(source) = &err.source {
- info.errno = source.to_errno();
+ if err.reply != BR_TRANSACTION_COMPLETE {
info.reply = err.reply;
+ if let Some(source) = &err.source {
+ info.errno = source.to_errno();
- {
- let mut ee = self.inner.lock().extended_error;
- ee.command = err.reply;
- ee.param = source.to_errno();
+ {
+ let mut inner = self.inner.lock();
+ inner.extended_error =
+ ExtendedError::new(info.debug_id as u32, err.reply, source.to_errno());
+ }
}
pr_warn!(
- "{}:{} transaction to {} failed: {source:?}",
+ "{}:{} transaction to {} failed: {err:?}",
info.from_pid,
info.from_tid,
info.to_pid
@@ -1320,18 +1341,24 @@ fn reply_inner(self: &Arc<Self>, info: &mut TransactionInfo) -> BinderResult {
let allow_fds = orig.flags & TF_ACCEPT_FDS != 0;
let reply = Transaction::new_reply(self, process, info, allow_fds)?;
self.inner.lock().push_work(completion);
- orig.from.deliver_reply(Ok(reply), &orig);
+ orig.from.deliver_reply(Ok(reply), &orig, None);
Ok(())
})()
.map_err(|mut err| {
// 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!(
- "Failure {:?} during reply - delivering BR_FAILED_REPLY to sender.",
- err
+ "{}:{} reply to {} failed: {err:?}",
+ info.from_pid,
+ info.from_tid,
+ info.to_pid
);
- let reply = Err(BR_FAILED_REPLY);
- orig.from.deliver_reply(reply, &orig);
+
+ let param = err.source.as_ref().map_or(0, |e| e.to_errno());
+ let ee = ExtendedError::new(orig.debug_id as u32, err.reply, param);
+ orig.from
+ .deliver_reply(Err(BR_FAILED_REPLY), &orig, Some(ee));
err.reply = BR_TRANSACTION_COMPLETE;
err
});
diff --git a/drivers/android/binder/transaction.rs b/drivers/android/binder/transaction.rs
index 1d9b66920a21..0e5d07b7e6f0 100644
--- a/drivers/android/binder/transaction.rs
+++ b/drivers/android/binder/transaction.rs
@@ -42,6 +42,7 @@ pub(crate) struct TransactionInfo {
pub(crate) reply: u32,
pub(crate) oneway_spam_suspect: bool,
pub(crate) is_reply: bool,
+ pub(crate) debug_id: usize,
}
impl TransactionInfo {
@@ -93,7 +94,6 @@ pub(crate) fn new(
from: &Arc<Thread>,
info: &mut TransactionInfo,
) -> BinderResult<DLArc<Self>> {
- let debug_id = super::next_debug_id();
let allow_fds = node_ref.node.flags & FLAT_BINDER_FLAG_ACCEPTS_FDS != 0;
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 };
@@ -101,7 +101,7 @@ pub(crate) fn new(
let mut alloc = match from.copy_transaction_data(
to.clone(),
info,
- debug_id,
+ info.debug_id,
allow_fds,
txn_security_ctx_off.as_mut(),
) {
@@ -128,7 +128,7 @@ pub(crate) fn new(
let data_address = alloc.ptr;
Ok(DTRWrap::arc_pin_init(pin_init!(Transaction {
- debug_id,
+ debug_id: info.debug_id,
target_node: Some(target_node),
from_parent,
sender_euid: Kuid::current_euid(),
@@ -152,9 +152,8 @@ pub(crate) fn new_reply(
info: &mut TransactionInfo,
allow_fds: bool,
) -> BinderResult<DLArc<Self>> {
- let debug_id = super::next_debug_id();
let mut alloc =
- match from.copy_transaction_data(to.clone(), info, debug_id, allow_fds, None) {
+ 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);
@@ -165,7 +164,7 @@ pub(crate) fn new_reply(
alloc.set_info_clear_on_drop();
}
Ok(DTRWrap::arc_pin_init(pin_init!(Transaction {
- debug_id,
+ debug_id: info.debug_id,
target_node: None,
from_parent: None,
sender_euid: Kuid::current_euid(),
@@ -394,7 +393,7 @@ fn do_work(
let send_failed_reply = ScopeGuard::new(|| {
if self.target_node.is_some() && self.flags & TF_ONE_WAY == 0 {
let reply = Err(BR_FAILED_REPLY);
- self.from.deliver_reply(reply, &self);
+ self.from.deliver_reply(reply, &self, None);
}
self.drop_outstanding_txn();
});
@@ -478,7 +477,7 @@ fn cancel(self: DArc<Self>) {
// If this is not a reply or oneway transaction, then send a dead reply.
if self.target_node.is_some() && self.flags & TF_ONE_WAY == 0 {
let reply = Err(BR_DEAD_REPLY);
- self.from.deliver_reply(reply, &self);
+ self.from.deliver_reply(reply, &self, None);
}
self.drop_outstanding_txn();
---
base-commit: da61573f783897ae5a96c8f1c71aad6242344feb
change-id: 20260527-set-extended-error-e2ca37e0696d
Best regards,
--
Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH v2] rust_binder: fix BINDER_GET_EXTENDED_ERROR
2026-06-04 11:37 [PATCH v2] rust_binder: fix BINDER_GET_EXTENDED_ERROR Alice Ryhl
@ 2026-06-04 12:58 ` Alice Ryhl
0 siblings, 0 replies; 2+ messages in thread
From: Alice Ryhl @ 2026-06-04 12:58 UTC (permalink / raw)
To: Greg Kroah-Hartman, Carlos Llamas
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
rust-for-linux, linux-kernel, stable
On Thu, Jun 04, 2026 at 11:37:07AM +0000, Alice Ryhl wrote:
> This code currently copies the ExtendedError struct to the stack,
> modifies the copy, and then doesn't modify the original. Thus, fix it.
>
> Furthermore, errors when replying must be delivered directly to the
> remote thread, so update deliver_reply() to take an extended error
> argument.
>
> Cc: stable@vger.kernel.org
> Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> BR_DEAD_REPLY => f.pad("BR_DEAD_REPLY"),
> BR_FROZEN_REPLY => f.pad("BR_FROZEN_REPLY"),
> BR_TRANSACTION_PENDING_FROZEN => f.pad("BR_TRANSACTION_PENDING_FROZEN"),
> BR_TRANSACTION_COMPLETE => f.pad("BR_TRANSACTION_COMPLETE"),
> - _ => f
> - .debug_struct("BinderError")
> - .field("reply", &self.reply)
> - .finish(),
> + _ => match self.source.as_ref() {
> + Some(source) => source.fmt(f),
> + None => f.pad("OTHER_ERROR"),
As sashiko points out, this is probably better as:
self.reply.fmt(f)
to just print the raw integer.
> - let reply = Err(BR_FAILED_REPLY);
> - orig.from.deliver_reply(reply, &orig);
> +
> + let param = err.source.as_ref().map_or(0, |e| e.to_errno());
> + let ee = ExtendedError::new(orig.debug_id as u32, err.reply, param);
> + orig.from
> + .deliver_reply(Err(BR_FAILED_REPLY), &orig, Some(ee));
As sashiko points out it should be info.debug_id instead of
orig.debug_id here.
Alice
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-06-04 12:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-04 11:37 [PATCH v2] rust_binder: fix BINDER_GET_EXTENDED_ERROR Alice Ryhl
2026-06-04 12:58 ` Alice Ryhl
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox