From 5a41bb32dd788cc03c4687dba13ca830afc8afa5 Mon Sep 17 00:00:00 2001 From: Alice Ryhl Date: Tue, 5 Aug 2025 11:59:13 +0000 Subject: [PATCH] ANDROID: rust_binder: rework process cleanup Rust Binder cleanup is reworked to match the order in which C Binder cleans up things. A few notes on the changes: * Actually dropping thread objects is done at the very end because they contain a call to synchronize_rcu() which is slow. This ensures that death notifications are sent without waiting for those calls. This avoids failures in rustBinderTest. (The test is already flaky, but this extra sleep makes the flake much more likely to happen.) * We now free refs on remote nodes in release explicitly. Previously that only happened implicitly when everything keeping the ref alive has been dropped. To avoid spurious warnings from this, the warning print about dropping a ref that doesn't exist is only printed if the process is alive. Bug: 431914626 Change-Id: I3d1f4f15ffac7587d1bb0113a41330b2aea69b3d Signed-off-by: Alice Ryhl --- drivers/android/binder/node.rs | 33 +++++------- drivers/android/binder/process.rs | 83 ++++++++++++++++--------------- drivers/android/binder/thread.rs | 5 +- rust/kernel/sync/poll.rs | 12 +++-- 4 files changed, 67 insertions(+), 66 deletions(-) diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs index b3f93616ba61..f3db36e1514b 100644 --- a/drivers/android/binder/node.rs +++ b/drivers/android/binder/node.rs @@ -351,17 +351,6 @@ impl Node { (self.ptr, self.cookie) } - pub(crate) fn next_death( - &self, - guard: &mut Guard<'_, ProcessInner, SpinLockBackend>, - ) -> Option> { - self.inner - .access_mut(guard) - .death_list - .pop_front() - .map(|larc| larc.into_arc()) - } - pub(crate) fn add_death( &self, death: ListArc, 1>, @@ -575,16 +564,18 @@ impl Node { Ok(()) } - pub(crate) fn release(&self, guard: &mut Guard<'_, ProcessInner, SpinLockBackend>) { - // Move every pending oneshot message to the process todolist. The process - // will cancel it later. - // - // New items can't be pushed after this call, since `submit_oneway` fails when the process - // is dead, which is set before `Node::release` is called. - // - // TODO: Give our linked list implementation the ability to move everything in one go. - while let Some(work) = self.inner.access_mut(guard).oneway_todo.pop_front() { - guard.push_work_for_release(work); + pub(crate) fn release(&self) { + let mut guard = self.owner.inner.lock(); + while let Some(work) = self.inner.access_mut(&mut guard).oneway_todo.pop_front() { + drop(guard); + work.into_arc().cancel(); + guard = self.owner.inner.lock(); + } + + let death_list = core::mem::take(&mut self.inner.access_mut(&mut guard).death_list); + drop(guard); + for death in death_list { + death.into_arc().set_dead(); } } diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs index 23d0f2af426c..927a7c085b1b 100644 --- a/drivers/android/binder/process.rs +++ b/drivers/android/binder/process.rs @@ -184,11 +184,6 @@ impl ProcessInner { } } - /// Push work to be cancelled. Only used during process teardown. - pub(crate) fn push_work_for_release(&mut self, work: DLArc) { - self.work.push_back(work); - } - pub(crate) fn remove_node(&mut self, ptr: u64) { self.nodes.remove(&ptr); } @@ -931,7 +926,10 @@ impl Process { refs.by_node.remove(&id); } } else { - pr_warn!("{}: no such ref {handle}\n", self.pid_in_current_ns()); + // All refs are cleared in process exit, so this warning is expected in that case. + if !self.inner.lock().is_dead { + pr_warn!("{}: no such ref {handle}\n", self.pid_in_current_ns()); + } } Ok(()) } @@ -1297,21 +1295,31 @@ impl Process { let binderfs_file = self.inner.lock().binderfs_file.take(); drop(binderfs_file); - // Move oneway_todo into the process todolist. + // Release threads. + let threads = { + let mut inner = self.inner.lock(); + let threads = take(&mut inner.threads); + let ready = take(&mut inner.ready_threads); + drop(inner); + drop(ready); + + for thread in threads.values() { + thread.release(); + } + threads + }; + + // Release nodes. { - let mut inner = self.lock_with_nodes(); - for node in inner.nodes.values() { - node.release(&mut inner.inner); + while let Some(node) = { + let mut lock = self.inner.lock(); + lock.nodes.cursor_front().map(|c| c.remove_current().1) + } { + node.to_key_value().1.release(); } } - // Cancel all pending work items. - while let Some(work) = self.get_work() { - work.into_arc().cancel(); - } - - // Drop all references. We do this dance with `swap` to avoid destroying the references - // while holding the lock. + // Clean up death listeners and remove nodes from external node info lists. for info in self.node_refs.lock().by_handle.values_mut() { // SAFETY: We are removing the `NodeRefInfo` from the right node. unsafe { info.node_ref2().node.remove_node_info(&info) }; @@ -1324,38 +1332,32 @@ impl Process { }; death.set_cleared(false); } + + // Clean up freeze listeners. let freeze_listeners = take(&mut self.node_refs.lock().freeze_listeners); for listener in freeze_listeners.values() { listener.on_process_exit(&self); } drop(freeze_listeners); - // Do similar dance for the state lock. - let mut inner = self.inner.lock(); - let threads = take(&mut inner.threads); - let nodes = take(&mut inner.nodes); - drop(inner); - - // Release all threads. - for thread in threads.values() { - thread.release(); + // Release refs on foreign nodes. + { + let mut refs = self.node_refs.lock(); + let by_handle = take(&mut refs.by_handle); + let by_node = take(&mut refs.by_node); + drop(refs); + drop(by_node); + drop(by_handle); } - // Deliver death notifications. - for node in nodes.values() { - loop { - let death = { - let mut inner = self.inner.lock(); - if let Some(death) = node.next_death(&mut inner) { - death - } else { - break; - } - }; - death.set_dead(); - } + // Cancel all pending work items. + while let Some(work) = self.get_work() { + work.into_arc().cancel(); } + let delivered_deaths = take(&mut self.inner.lock().delivered_deaths); + drop(delivered_deaths); + // Free any resources kept alive by allocated buffers. let omapping = self.inner.lock().mapping.take(); if let Some(mut mapping) = omapping { @@ -1376,6 +1378,9 @@ impl Process { drop(alloc) }); } + + // calls to synchronize_rcu() in thread drop will happen here + drop(threads); } pub(crate) fn drop_outstanding_txn(&self) { diff --git a/drivers/android/binder/thread.rs b/drivers/android/binder/thread.rs index 362d72052d60..12cd119f0ab0 100644 --- a/drivers/android/binder/thread.rs +++ b/drivers/android/binder/thread.rs @@ -1653,12 +1653,13 @@ impl Thread { pub(crate) fn release(self: &Arc) { self.inner.lock().is_dead = true; + self.work_condvar.clear(); + self.unwind_transaction_stack(); + // Cancel all pending work items. while let Ok(Some(work)) = self.get_work_local(false) { work.into_arc().cancel(); } - - self.unwind_transaction_stack(); } } diff --git a/rust/kernel/sync/poll.rs b/rust/kernel/sync/poll.rs index 06db8dee3f01..5a03f293a3d2 100644 --- a/rust/kernel/sync/poll.rs +++ b/rust/kernel/sync/poll.rs @@ -78,6 +78,13 @@ impl PollCondVar { inner <- CondVar::new(name, key), }) } + + /// Clear anything registered using `register_wait`. + pub fn clear(&self) { + // SAFETY: The pointer points at a valid `wait_queue_head`. The destructor waits an rcu + // grace period before the wait queue is freed. + unsafe { bindings::__wake_up_pollfree(self.inner.wait_queue_head.get()) }; + } } // Make the `CondVar` methods callable on `PollCondVar`. @@ -92,10 +99,7 @@ impl Deref for PollCondVar { #[pinned_drop] impl PinnedDrop for PollCondVar { fn drop(self: Pin<&mut Self>) { - // Clear anything registered using `register_wait`. - // - // SAFETY: The pointer points at a valid `wait_queue_head`. - unsafe { bindings::__wake_up_pollfree(self.inner.wait_queue_head.get()) }; + self.clear(); // Wait for epoll items to be properly removed. //