* Re: [PATCH] rust_binder: call set_notification_done() without proc lock
2026-02-24 18:16 ` [PATCH] rust_binder: call set_notification_done() without proc lock Alice Ryhl
@ 2026-02-24 18:56 ` Andreas Hindborg
2026-02-25 3:04 ` Gary Guo
1 sibling, 0 replies; 3+ messages in thread
From: Andreas Hindborg @ 2026-02-24 18:56 UTC (permalink / raw)
To: Alice Ryhl, Greg Kroah-Hartman, Carlos Llamas
Cc: Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Alice Ryhl, Trevor Gross, Danilo Krummrich,
rust-for-linux, linux-kernel, stable,
syzbot+c8287e65a57a89e7fb72
"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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] rust_binder: call set_notification_done() without proc lock
2026-02-24 18:16 ` [PATCH] rust_binder: call set_notification_done() without proc lock Alice Ryhl
2026-02-24 18:56 ` Andreas Hindborg
@ 2026-02-25 3:04 ` Gary Guo
1 sibling, 0 replies; 3+ messages in thread
From: Gary Guo @ 2026-02-25 3:04 UTC (permalink / raw)
To: Alice Ryhl
Cc: Greg Kroah-Hartman, Carlos Llamas, Miguel Ojeda, Boqun Feng,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, rust-for-linux, linux-kernel,
stable, syzbot+c8287e65a57a89e7fb72
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,
^ permalink raw reply [flat|nested] 3+ messages in thread