[PATCH] rust_binder: call set_notification_done() without proc lock

[PATCH] rust_binder: call set_notification_done() without proc lock

[Alice Ryhl]

Consider the following sequence of events on a death listener:
1. The remote process dies and sends a BR_DEAD_BINDER message.
2. The local process invokes the BC_CLEAR_DEATH_NOTIFICATION command.
3. The local process then invokes the BC_DEAD_BINDER_DONE.
Then, the kernel will reply to the BC_DEAD_BINDER_DONE command with a
BR_CLEAR_DEATH_NOTIFICATION_DONE reply using push_work_if_looper().

However, this can result in a deadlock if the current thread is not a
looper. This is because dead_binder_done() still holds the proc lock
during set_notification_done(), which called push_work_if_looper().
Normally, push_work_if_looper() takes the thread lock, which is fine to
take under the proc lock. But if the current thread is not a looper,
then it falls back to delivering the reply to the process work queue,
which involves taking the proc lock. Since the proc lock is already
held, this is a deadlock.

Fix this by releasing the proc lock during set_notification_done(). It
was not intentional that it was held during that function to begin with.

I don't think this ever happens in Android because BC_DEAD_BINDER_DONE
is only invoked in response to BR_DEAD_BINDER messages, and the kernel
always delivers BR_DEAD_BINDER to a looper. So there's no scenario where
Android userspace will call BC_DEAD_BINDER_DONE on a non-looper thread.

Cc: stable@vger.kernel.org
Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
Reported-by: syzbot+c8287e65a57a89e7fb72@syzkaller.appspotmail.com
Tested-by: syzbot+c8287e65a57a89e7fb72@syzkaller.appspotmail.com
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Sorry, no report link. Was reported via internal issue tracker.
---
 drivers/android/binder/process.rs | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
index 41de5593197c..f06498129aa9 100644
--- a/drivers/android/binder/process.rs
+++ b/drivers/android/binder/process.rs
@@ -1295,7 +1295,8 @@ pub(crate) fn clear_death(&self, reader: &mut UserSliceReader, thread: &Thread)
     }
 
     pub(crate) fn dead_binder_done(&self, cookie: u64, thread: &Thread) {
-        if let Some(death) = self.inner.lock().pull_delivered_death(cookie) {
+        let death = self.inner.lock().pull_delivered_death(cookie);
+        if let Some(death) = death {
             death.set_notification_done(thread);
         }
     }

---
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
change-id: 20260224-binder-dead-binder-done-proc-lock-e2d4825b2965

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

Re: [PATCH] rust_binder: call set_notification_done() without proc lock

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> Consider the following sequence of events on a death listener:
> 1. The remote process dies and sends a BR_DEAD_BINDER message.
> 2. The local process invokes the BC_CLEAR_DEATH_NOTIFICATION command.
> 3. The local process then invokes the BC_DEAD_BINDER_DONE.
> Then, the kernel will reply to the BC_DEAD_BINDER_DONE command with a
> BR_CLEAR_DEATH_NOTIFICATION_DONE reply using push_work_if_looper().
>
> However, this can result in a deadlock if the current thread is not a
> looper. This is because dead_binder_done() still holds the proc lock
> during set_notification_done(), which called push_work_if_looper().
> Normally, push_work_if_looper() takes the thread lock, which is fine to
> take under the proc lock. But if the current thread is not a looper,
> then it falls back to delivering the reply to the process work queue,
> which involves taking the proc lock. Since the proc lock is already
> held, this is a deadlock.
>
> Fix this by releasing the proc lock during set_notification_done(). It
> was not intentional that it was held during that function to begin with.
>
> I don't think this ever happens in Android because BC_DEAD_BINDER_DONE
> is only invoked in response to BR_DEAD_BINDER messages, and the kernel
> always delivers BR_DEAD_BINDER to a looper. So there's no scenario where
> Android userspace will call BC_DEAD_BINDER_DONE on a non-looper thread.
>
> Cc: stable@vger.kernel.org
> Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> Reported-by: syzbot+c8287e65a57a89e7fb72@syzkaller.appspotmail.com
> Tested-by: syzbot+c8287e65a57a89e7fb72@syzkaller.appspotmail.com
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>

Best regards,
Andreas Hindborg

Re: [PATCH] rust_binder: call set_notification_done() without proc lock

[Gary Guo]

On 2026-02-24 18:16, Alice Ryhl wrote:
> Consider the following sequence of events on a death listener:
> 1. The remote process dies and sends a BR_DEAD_BINDER message.
> 2. The local process invokes the BC_CLEAR_DEATH_NOTIFICATION command.
> 3. The local process then invokes the BC_DEAD_BINDER_DONE.
> Then, the kernel will reply to the BC_DEAD_BINDER_DONE command with a
> BR_CLEAR_DEATH_NOTIFICATION_DONE reply using push_work_if_looper().
> 
> However, this can result in a deadlock if the current thread is not a
> looper. This is because dead_binder_done() still holds the proc lock
> during set_notification_done(), which called push_work_if_looper().
> Normally, push_work_if_looper() takes the thread lock, which is fine to
> take under the proc lock. But if the current thread is not a looper,
> then it falls back to delivering the reply to the process work queue,
> which involves taking the proc lock. Since the proc lock is already
> held, this is a deadlock.
> 
> Fix this by releasing the proc lock during set_notification_done(). It
> was not intentional that it was held during that function to begin with.
> 
> I don't think this ever happens in Android because BC_DEAD_BINDER_DONE
> is only invoked in response to BR_DEAD_BINDER messages, and the kernel
> always delivers BR_DEAD_BINDER to a looper. So there's no scenario where
> Android userspace will call BC_DEAD_BINDER_DONE on a non-looper thread.
> 
> Cc: stable@vger.kernel.org
> Fixes: eafedbc7c050 ("rust_binder: add Rust Binder driver")
> Reported-by: syzbot+c8287e65a57a89e7fb72@syzkaller.appspotmail.com
> Tested-by: syzbot+c8287e65a57a89e7fb72@syzkaller.appspotmail.com
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> Sorry, no report link. Was reported via internal issue tracker.
> ---
>  drivers/android/binder/process.rs | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
> index 41de5593197c..f06498129aa9 100644
> --- a/drivers/android/binder/process.rs
> +++ b/drivers/android/binder/process.rs
> @@ -1295,7 +1295,8 @@ pub(crate) fn clear_death(&self, reader: &mut UserSliceReader, thread: &Thread)
>      }
>  
>      pub(crate) fn dead_binder_done(&self, cookie: u64, thread: &Thread) {
> -        if let Some(death) = self.inner.lock().pull_delivered_death(cookie) {
> +        let death = self.inner.lock().pull_delivered_death(cookie);
> +        if let Some(death) = death {

Reviewed-by: Gary Guo <gary@garyguo.net>

This is quite sneaky. I wonder if this is something that could be checked Clippy.
If a lock guard has lifetime extended and the final expression does not depend on
the lifetime of that lock guard, then a warning could be emitted.

The general case is probably very difficult to implement as it involves resolving
lifetimes. But I think a specific case where the final expression is `'static` is
easier to implement.

Best,
Gary

>              death.set_notification_done(thread);
>          }
>      }
> 
> ---
> base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
> change-id: 20260224-binder-dead-binder-done-proc-lock-e2d4825b2965
> 
> Best regards,