From 98d09f446ee41eebf70fea7fd544a84a9908beac Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Tue, 28 Feb 2023 15:08:02 +0100 Subject: [PATCH] rust_binder: keep target_node around until allocation is freed Signed-off-by: Alice Ryhl --- drivers/android/allocation.rs | 8 ++++++- drivers/android/process.rs | 3 +++ drivers/android/transaction.rs | 40 +++++++++++++++++----------------- 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/drivers/android/allocation.rs b/drivers/android/allocation.rs index 706d3647a3d1f2..acf2736b9c2e4a 100644 --- a/drivers/android/allocation.rs +++ b/drivers/android/allocation.rs @@ -147,6 +147,10 @@ impl<'a> Allocation<'a> { pub(crate) fn set_info_clear_on_drop(&mut self) { self.get_or_init_info().clear_on_free = true; } + + pub(crate) fn set_info_target_node(&mut self, target_node: NodeRef) { + self.get_or_init_info().target_node = Some(target_node); + } } impl Drop for Allocation<'_> { @@ -155,11 +159,13 @@ impl Drop for Allocation<'_> { return; } - if let Some(info) = self.allocation_info.take() { + if let Some(mut info) = self.allocation_info.take() { if let Some(oneway_node) = info.oneway_node.as_ref() { oneway_node.pending_oneway_finished(); } + info.target_node = None; + if let Some(offsets) = info.offsets.clone() { let view = AllocationView::new(self, offsets.start); for i in offsets.step_by(size_of::()) { diff --git a/drivers/android/process.rs b/drivers/android/process.rs index 0c6b31e76e2a57..a1d46f31e3b3b5 100644 --- a/drivers/android/process.rs +++ b/drivers/android/process.rs @@ -36,6 +36,9 @@ use crate::{ pub(crate) struct AllocationInfo { /// Range within the allocation where we can find the offsets to the object descriptors. pub(crate) offsets: Option>, + /// The target node of the transaction this allocation is associated to. + /// Not set for replies. + pub(crate) target_node: Option, /// When this allocation is dropped, call `pending_oneway_finished` on the node. /// /// This is used to serialize oneway transaction on the same node. Binder guarantees that diff --git a/drivers/android/transaction.rs b/drivers/android/transaction.rs index 427ffd11109ea6..33ac393822da5b 100644 --- a/drivers/android/transaction.rs +++ b/drivers/android/transaction.rs @@ -16,7 +16,7 @@ use kernel::{ use crate::{ defs::*, - node::NodeRef, + node::{Node, NodeRef}, process::Process, ptr_align, thread::{BinderResult, BinderError, Thread}, @@ -29,8 +29,7 @@ struct TransactionInner { pub(crate) struct Transaction { inner: SpinLock, - // TODO: Node should be released when the buffer is released. - node_ref: Option, + target_node: Option>, stack_next: Option>, pub(crate) from: Ref, to: Ref, @@ -75,6 +74,8 @@ impl Transaction { if trd.flags & TF_CLEAR_BUF != 0 { alloc.set_info_clear_on_drop(); } + let target_node = node_ref.node.clone(); + alloc.set_info_target_node(node_ref); let data_address = alloc.ptr; let file_list = alloc.take_file_list(); alloc.keep_alive(); @@ -83,7 +84,7 @@ impl Transaction { inner: unsafe { SpinLock::new(TransactionInner { file_list }) }, // SAFETY: `PINode::init` is called below. pi_node: unsafe { crate::pi::PINode::new(&from.task) }, - node_ref: Some(node_ref), + target_node: Some(target_node), stack_next, from: from.clone(), to, @@ -134,7 +135,7 @@ impl Transaction { inner: unsafe { SpinLock::new(TransactionInner { file_list }) }, // SAFETY: `PINode::init` is called below. pi_node: unsafe { crate::pi::PINode::new(&from.task) }, - node_ref: None, + target_node: None, stack_next: None, from: from.clone(), to, @@ -179,11 +180,9 @@ impl Transaction { /// useful when finding a target for a new transaction: if the node belongs to a process that /// is already part of the transaction stack, we reuse the thread. fn find_target_thread(&self) -> Option> { - let process = &self.node_ref.as_ref()?.node.owner; - let mut it = &self.stack_next; while let Some(transaction) = it { - if Ref::ptr_eq(&transaction.from.process, process) { + if Ref::ptr_eq(&transaction.from.process, &self.to) { return Some(transaction.from.clone()); } it = &transaction.stack_next; @@ -206,10 +205,12 @@ impl Transaction { /// Submits the transaction to a work queue. Use a thread if there is one in the transaction /// stack, otherwise use the destination process. + /// + /// Not used for replies. pub(crate) fn submit(self: Ref) -> BinderResult { if self.flags & TF_ONE_WAY != 0 { - if let Some(node_ref) = self.node_ref.as_ref() { - node_ref.node.clone().submit_oneway(self)?; + if let Some(target_node) = self.target_node.clone() { + target_node.submit_oneway(self)?; return Ok(()); } else { pr_err!("Failed to submit oneway transaction to node."); @@ -266,8 +267,7 @@ impl Transaction { /// If the target has no available threads, then this is done inside `do_work` when a thread /// picks it up instead. pub(crate) fn set_pi_owner(&self, owner: &Task) { - if self.node_ref.is_some() && self.flags & TF_ONE_WAY == 0 { - // Not a reply and not one-way. + if self.flags & TF_ONE_WAY == 0 { self.pi_node.set_owner(owner); } } @@ -283,13 +283,13 @@ impl Transaction { impl DeliverToRead for Transaction { fn do_work(self: Ref, thread: &Thread, writer: &mut UserSlicePtrWriter) -> Result { let send_failed_reply = ScopeGuard::new(|| { - if self.node_ref.is_some() && self.flags & TF_ONE_WAY == 0 { + if self.target_node.is_some() && self.flags & TF_ONE_WAY == 0 { let reply = Either::Right(BR_FAILED_REPLY); self.from.deliver_reply(reply, &self); } }); - if self.node_ref.is_some() && self.flags & TF_ONE_WAY == 0 { + if self.target_node.is_some() && self.flags & TF_ONE_WAY == 0 { // Not a reply and not one-way. self.pi_node.set_owner(&Task::current()); } @@ -305,8 +305,8 @@ impl DeliverToRead for Transaction { let mut tr_sec = BinderTransactionDataSecctx::default(); let tr = tr_sec.tr_data(); - if let Some(nref) = &self.node_ref { - let (ptr, cookie) = nref.node.get_id(); + if let Some(target_node) = &self.target_node { + let (ptr, cookie) = target_node.get_id(); tr.target.ptr = ptr as _; tr.cookie = cookie as _; }; @@ -323,7 +323,7 @@ impl DeliverToRead for Transaction { tr.sender_euid = self.sender_euid.into_uid_in_current_ns(); tr.sender_pid = 0; - if self.node_ref.is_some() && self.flags & TF_ONE_WAY == 0 { + if self.target_node.is_some() && self.flags & TF_ONE_WAY == 0 { // Not a reply and not one-way. let from_proc = &*self.from.process; if !from_proc.is_dead() { @@ -332,7 +332,7 @@ impl DeliverToRead for Transaction { } } - let code = if self.node_ref.is_none() { + let code = if self.target_node.is_none() { BR_REPLY } else { if self.txn_security_ctx_off.is_some() { @@ -378,7 +378,7 @@ impl DeliverToRead for Transaction { // When this is not a reply and not an async transaction, update `current_transaction`. If // it's a reply, `current_transaction` has already been updated appropriately. - if self.node_ref.is_some() && tr_sec.transaction_data.flags & TF_ONE_WAY == 0 { + if self.target_node.is_some() && tr_sec.transaction_data.flags & TF_ONE_WAY == 0 { thread.set_current_transaction(self); } @@ -387,7 +387,7 @@ impl DeliverToRead for Transaction { fn cancel(self: Ref) { // If this is not a reply or oneway transaction, then send a dead reply. - if self.node_ref.is_some() && self.flags & TF_ONE_WAY == 0 { + if self.target_node.is_some() && self.flags & TF_ONE_WAY == 0 { let reply = Either::Right(BR_DEAD_REPLY); self.from.deliver_reply(reply, &self); }