| From 2a74ccd1bcc170e66525f34e9de8652b57e2cf3d Mon Sep 17 00:00:00 2001 | |
| From: Alice Ryhl <aliceryhl@google.com> | |
| Date: Thu, 3 Sep 2026 11:36:03 +0000 | |
| Subject: rust_binder: reschedule node refcount update on thread exit | |
| From: Alice Ryhl <aliceryhl@google.com> | |
| commit 2a74ccd1bcc170e66525f34e9de8652b57e2cf3d upstream. | |
| When a thread exits via BINDER_THREAD_EXIT, its pending work items are | |
| cancelled. If a thread exits while holding a pending node refcount | |
| increment (e.g. pushed as deferred work to that thread), the refcount | |
| increment was previously dropped because Node::cancel() and | |
| NodeWrapper::cancel() were no-ops. | |
| Dropping the refcount update leaves the node's delivery state and count | |
| state desynchronized, and userspace will not receive the notification, | |
| which can cause the node to never be freed from the process's nodes tree | |
| when all external references are dropped. | |
| Fix this by implementing DeliverToRead::cancel() for Node and NodeWrapper | |
| to move the pending refcount update to the process's work queue on thread | |
| exit so another thread can deliver it to userspace. | |
| Cc: stable <stable@kernel.org> | |
| Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver") | |
| Signed-off-by: Alice Ryhl <aliceryhl@google.com> | |
| Link: https://patch.msgid.link/20260903-binder-thread-exit-node-v1-1-be09ff14f6a4@google.com | |
| [ Dropped first argument in calls to `push_work()` to adjust to the | |
| existing form without mainline commit dbb17c9ea756 ("rust_binder: move | |
| (e)poll wait queue to Process"). - Miguel ] | |
| Signed-off-by: Miguel Ojeda <ojeda@kernel.org> | |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
| --- | |
| drivers/android/binder/node.rs | 20 ++++++++++++++++--- | |
| drivers/android/binder/node/wrapper.rs | 34 ++++++++++++++++++++++++++++++++- | |
| 2 files changed, 50 insertions(+), 4 deletions(-) | |
| --- a/drivers/android/binder/node.rs | |
| +++ b/drivers/android/binder/node.rs | |
| @@ -49,9 +49,9 @@ pub(crate) struct CouldNotDeliverCritica | |
| /// about to drop the weak reference, then the strong increment could be processed after the | |
| /// other thread has already exited, which would be too late. | |
| /// | |
| -/// Note that trying to create a `ListArc` to the node can succeed even if `has_normal_push` is | |
| +/// Note that trying to create a `ListArc` to the node can succeed even if `has_pushed_node` is | |
| /// set. This is because another thread might just have popped the node from a todo list, but not | |
| -/// yet called `do_work`. However, if `has_normal_push` is false, then creating a `ListArc` should | |
| +/// yet called `do_work`. However, if `has_pushed_node` is false, then creating a `ListArc` should | |
| /// always succeed. | |
| /// | |
| /// Like the other fields in `NodeInner`, the delivery state is protected by the process lock. | |
| @@ -733,7 +733,21 @@ impl DeliverToRead for Node { | |
| self.do_work_locked(writer, owner_inner) | |
| } | |
| - fn cancel(self: DArc<Self>) {} | |
| + fn cancel(self: DArc<Self>) { | |
| + let _drop_outside_lock; | |
| + let mut owner_inner = self.owner.inner.lock(); | |
| + | |
| + // We only do something on BINDER_THREAD_EXIT, not process exit. | |
| + if owner_inner.is_dead { | |
| + return; | |
| + } | |
| + | |
| + // If BINDER_THREAD_EXIT is invoked on a thread with a pending node refcount update, we | |
| + // should move ourselves to ensure the refcount update is still delivered. | |
| + if let Some(node) = ListArc::try_from_arc_borrow(self.as_arc_borrow()) { | |
| + _drop_outside_lock = owner_inner.push_work(node); | |
| + } | |
| + } | |
| fn should_sync_wakeup(&self) -> bool { | |
| false | |
| --- a/drivers/android/binder/node/wrapper.rs | |
| +++ b/drivers/android/binder/node/wrapper.rs | |
| @@ -57,7 +57,39 @@ impl DeliverToRead for NodeWrapper { | |
| node.do_work_locked(writer, owner_inner) | |
| } | |
| - fn cancel(self: DArc<Self>) {} | |
| + fn cancel(self: DArc<Self>) { | |
| + let _drop_outside_lock; | |
| + let node = &self.node; | |
| + let mut owner_inner = node.owner.inner.lock(); | |
| + | |
| + // We only do something on BINDER_THREAD_EXIT, not process exit. | |
| + if owner_inner.is_dead { | |
| + return; | |
| + } | |
| + | |
| + // We transfer the responsibility of the node refcount update to the scheduled Node because | |
| + // NodeWrapper has no way to re-create the ListArc. | |
| + let inner = node.inner.access_mut(&mut owner_inner); | |
| + | |
| + let ds = &mut inner.delivery_state; | |
| + assert!(ds.has_pushed_wrapper); | |
| + assert!(ds.has_strong_zero2one); | |
| + ds.has_pushed_wrapper = false; | |
| + | |
| + // We are changing the state to one where the Node is the strong zero2one update instead of | |
| + // the wrapper. | |
| + ds.has_weak_zero2one = false; | |
| + | |
| + if !ds.has_pushed_node { | |
| + if let Some(node2) = ListArc::try_from_arc_borrow(node.as_arc_borrow()) { | |
| + ds.has_pushed_node = true; | |
| + _drop_outside_lock = owner_inner.push_work(node2); | |
| + } else { | |
| + // This can't actually happen. | |
| + ds.has_strong_zero2one = false; | |
| + } | |
| + } | |
| + } | |
| fn should_sync_wakeup(&self) -> bool { | |
| false |