drivers/android/binder/freeze.rs | 67 +++++++++++++++++++++++++++------------ drivers/android/binder/node.rs | 55 ++++++++++++++++++-------------- drivers/android/binder/process.rs | 6 ++-- 3 files changed, 80 insertions(+), 48 deletions(-)
Unfortunately the current use of a mutex for this lock leads to priority
inversion. Traces have been observed where a process is trying to obtain
this mutex for 22ms, but it's unable to do so because the thread holding
the lock is scheduled out. Since this occurred on a UI thread, that is
an extremely long delay.
To support this change, logic in freeze.rs is updated to avoid
reallocating under the lock by adding a retry loop to
request_freeze_notif(). The remove_freeze_listener() function is also
updated to avoid calling kvfree() under the lock.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
drivers/android/binder/freeze.rs | 67 +++++++++++++++++++++++++++------------
drivers/android/binder/node.rs | 55 ++++++++++++++++++--------------
drivers/android/binder/process.rs | 6 ++--
3 files changed, 80 insertions(+), 48 deletions(-)
diff --git a/drivers/android/binder/freeze.rs b/drivers/android/binder/freeze.rs
index 53b60035639a..20041689e98d 100644
--- a/drivers/android/binder/freeze.rs
+++ b/drivers/android/binder/freeze.rs
@@ -173,36 +173,60 @@ pub(crate) fn request_freeze_notif(
let msg = FreezeMessage::new(GFP_KERNEL)?;
let alloc = RBTreeNodeReservation::new(GFP_KERNEL)?;
+ let mut afl_vec_alloc = KVVec::new();
+ let mut info;
+ let mut node;
+ let mut freeze_entry;
let mut node_refs_guard = self.node_refs.lock();
- let node_refs = &mut *node_refs_guard;
- let Some(info) = node_refs.by_handle.get_mut(&handle) else {
- pr_warn!("BC_REQUEST_FREEZE_NOTIFICATION invalid ref {}\n", handle);
- return Err(EINVAL);
- };
- if info.freeze().is_some() {
- pr_warn!("BC_REQUEST_FREEZE_NOTIFICATION already set\n");
- return Err(EINVAL);
- }
- let node_ref = info.node_ref();
- let freeze_entry = node_refs.freeze_listeners.entry(cookie);
-
- if let rbtree::Entry::Occupied(ref dupe) = freeze_entry {
- if !dupe.get().allow_duplicate(&node_ref.node) {
- pr_warn!("BC_REQUEST_FREEZE_NOTIFICATION duplicate cookie\n");
+ loop {
+ let node_refs = &mut *node_refs_guard;
+ info = match node_refs.by_handle.get_mut(&handle) {
+ Some(info) => info,
+ None => {
+ pr_warn!("BC_REQUEST_FREEZE_NOTIFICATION invalid ref {}\n", handle);
+ return Err(EINVAL);
+ }
+ };
+ if info.freeze().is_some() {
+ pr_warn!("BC_REQUEST_FREEZE_NOTIFICATION already set\n");
return Err(EINVAL);
}
- }
+ let node_ref = info.node_ref();
+ node = node_ref.node.clone();
+ freeze_entry = node_refs.freeze_listeners.entry(cookie);
+
+ if let rbtree::Entry::Occupied(ref dupe) = freeze_entry {
+ if !dupe.get().allow_duplicate(&node_ref.node) {
+ pr_warn!("BC_REQUEST_FREEZE_NOTIFICATION duplicate cookie\n");
+ return Err(EINVAL);
+ }
+ }
- // All failure paths must come before this call, and all modifications must come after this
- // call.
- node_ref.node.add_freeze_listener(self, GFP_KERNEL)?;
+ // Now we add to the node's freeze listener list, with retry and re-allocate if the
+ // vector is full.
+ //
+ // To ensure that the node is added atomically, this is the first time we modify any
+ // state. When this call succeeds, all other modifications must occur without the
+ // possibility for any failure paths.
+ match node_ref
+ .node
+ .add_freeze_listener(self, &mut afl_vec_alloc)?
+ {
+ Ok(()) => break,
+ Err(resize_target) => {
+ drop(node_refs_guard);
+ Node::resize_for_add_freeze_listener(&mut afl_vec_alloc, resize_target)?;
+ node_refs_guard = self.node_refs.lock();
+ }
+ }
+ }
match freeze_entry {
rbtree::Entry::Vacant(entry) => {
entry.insert(
FreezeListener {
cookie,
- node: node_ref.node.clone(),
+ node,
last_is_frozen: None,
is_pending: false,
is_clearing: false,
@@ -273,6 +297,7 @@ pub(crate) fn clear_freeze_notif(self: &Arc<Self>, reader: &mut UserSliceReader)
let handle = hc.handle;
let cookie = FreezeCookie(hc.cookie);
+ let _to_free_fl;
let alloc = FreezeMessage::new(GFP_KERNEL)?;
let mut node_refs_guard = self.node_refs.lock();
let node_refs = &mut *node_refs_guard;
@@ -293,7 +318,7 @@ pub(crate) fn clear_freeze_notif(self: &Arc<Self>, reader: &mut UserSliceReader)
return Err(EINVAL);
};
listener.is_clearing = true;
- listener.node.remove_freeze_listener(self);
+ _to_free_fl = listener.node.remove_freeze_listener(self);
*info.freeze() = None;
let mut msg = None;
if !listener.is_pending {
diff --git a/drivers/android/binder/node.rs b/drivers/android/binder/node.rs
index 69f757ff7461..fb27674a8c94 100644
--- a/drivers/android/binder/node.rs
+++ b/drivers/android/binder/node.rs
@@ -657,33 +657,37 @@ fn do_work_locked(
pub(crate) fn add_freeze_listener(
&self,
process: &Arc<Process>,
- flags: kernel::alloc::Flags,
- ) -> Result {
- let mut vec_alloc = KVVec::<Arc<Process>>::new();
- loop {
- let mut guard = self.owner.inner.lock();
- // Do not check for `guard.dead`. The `dead` flag that matters here is the owner of the
- // listener, no the target.
- let inner = self.inner.access_mut(&mut guard);
- let len = inner.freeze_list.len();
- if len >= inner.freeze_list.capacity() {
- if len >= vec_alloc.capacity() {
- drop(guard);
- vec_alloc = KVVec::with_capacity((1 + len).next_power_of_two(), flags)?;
- continue;
- }
- mem::swap(&mut inner.freeze_list, &mut vec_alloc);
- for elem in vec_alloc.drain_all() {
- inner.freeze_list.push_within_capacity(elem)?;
- }
+ // If the vector needs to be resized, it's done via this argument.
+ vec_alloc: &mut KVVec<Arc<Process>>,
+ ) -> Result<Result<(), usize>> {
+ let mut guard = self.owner.inner.lock();
+ // Do not check for `guard.dead`. The `dead` flag that matters here is the owner of the
+ // listener, not the target.
+ let inner = self.inner.access_mut(&mut guard);
+ let len = inner.freeze_list.len();
+ if len == inner.freeze_list.capacity() {
+ if len >= vec_alloc.capacity() {
+ // Request the caller to reallocate.
+ return Ok(Err(1 + len));
+ }
+ mem::swap(&mut inner.freeze_list, vec_alloc);
+ for elem in vec_alloc.drain_all() {
+ inner.freeze_list.push_within_capacity(elem)?;
}
- inner.freeze_list.push_within_capacity(process.clone())?;
- return Ok(());
}
+ inner.freeze_list.push_within_capacity(process.clone())?;
+ Ok(Ok(()))
+ }
+
+ pub(crate) fn resize_for_add_freeze_listener(
+ vec_alloc: &mut KVVec<Arc<Process>>,
+ target_size: usize,
+ ) -> Result {
+ *vec_alloc = KVVec::with_capacity(target_size.next_power_of_two(), GFP_KERNEL)?;
+ Ok(())
}
- pub(crate) fn remove_freeze_listener(&self, p: &Arc<Process>) {
- let _unused_capacity;
+ pub(crate) fn remove_freeze_listener(&self, p: &Arc<Process>) -> KVVec<Arc<Process>> {
let mut guard = self.owner.inner.lock();
let inner = self.inner.access_mut(&mut guard);
let len = inner.freeze_list.len();
@@ -694,9 +698,12 @@ pub(crate) fn remove_freeze_listener(&self, p: &Arc<Process>) {
p.pid_in_current_ns()
);
}
+ // If the vector is empty it needs to be freed. However, we can't free it here because that
+ // might sleep, so return it to the caller.
if inner.freeze_list.is_empty() {
- _unused_capacity = mem::take(&mut inner.freeze_list);
+ return mem::take(&mut inner.freeze_list);
}
+ KVVec::new()
}
pub(crate) fn freeze_list<'a>(&'a self, guard: &'a ProcessInner) -> &'a [Arc<Process>] {
diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index 96b8440ceac6..e3c6eccce730 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -30,7 +30,7 @@
sync::{
aref::ARef,
lock::{spinlock::SpinLockBackend, Guard},
- Arc, ArcBorrow, CondVar, CondVarTimeoutResult, Mutex, SpinLock, UniqueArc,
+ Arc, ArcBorrow, CondVar, CondVarTimeoutResult, SpinLock, UniqueArc,
},
task::Task,
uaccess::{UserSlice, UserSliceReader},
@@ -455,7 +455,7 @@ pub(crate) struct Process {
// Node references are in a different lock to avoid recursive acquisition when
// incrementing/decrementing a node in another process.
#[pin]
- node_refs: Mutex<ProcessNodeRefs>,
+ node_refs: SpinLock<ProcessNodeRefs>,
// Work node for deferred work item.
#[pin]
@@ -510,7 +510,7 @@ fn new(ctx: Arc<Context>, cred: ARef<Credential>) -> Result<Arc<Self>> {
cred,
inner <- kernel::new_spinlock!(ProcessInner::new(), "Process::inner"),
pages <- ShrinkablePageRange::new(&super::BINDER_SHRINKER),
- node_refs <- kernel::new_mutex!(ProcessNodeRefs::new(), "Process::node_refs"),
+ node_refs <- kernel::new_spinlock!(ProcessNodeRefs::new(), "Process::node_refs"),
freeze_wait <- kernel::new_condvar!("Process::freeze_wait"),
task: current.group_leader().into(),
defer_work <- kernel::new_work!("Process::defer_work"),
---
base-commit: da61573f783897ae5a96c8f1c71aad6242344feb
change-id: 20260608-binder-noderefs-spin-3a0ec0589043
Best regards,
--
Alice Ryhl <aliceryhl@google.com>© 2016 - 2026 Red Hat, Inc.