| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aOaXMrGD1xIGIHkP@google.com>
Date: Wed, 8 Oct 2025 16:54:10 +0000
From: Carlos Llamas <cmllamas@...gle.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arve Hjønnevåg <arve@...roid.com>,
Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>,
Joel Fernandes <joelagnelf@...dia.com>,
Christian Brauner <brauner@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>,
linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 3/3] rust_binder: report freeze notification only when
fully frozen
On Tue, Oct 07, 2025 at 09:39:53AM +0000, Alice Ryhl wrote:
> Binder only sends out freeze notifications when ioctl_freeze() completes
> and the process has become fully frozen. However, if a freeze
> notification is registered during the freeze operation, then it
> registers an initial state of 'frozen'. This is a problem because if
> the freeze operation fails, then the listener is not told about that
> state change, leading to lost updates.
>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
> This is also an issue in C Binder. A fix for that will be sent
> separately.
> ---
> drivers/android/binder/freeze.rs | 4 +--
> drivers/android/binder/process.rs | 46 ++++++++++++++++++++++++++++-------
> drivers/android/binder/transaction.rs | 6 ++---
> 3 files changed, 42 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/android/binder/freeze.rs b/drivers/android/binder/freeze.rs
> index e304aceca7f31c15444cf67bb13488cd144345e6..220de35ae85ac8de2af717129011e0ace0677b7d 100644
> --- a/drivers/android/binder/freeze.rs
> +++ b/drivers/android/binder/freeze.rs
> @@ -121,7 +121,7 @@ fn do_work(
> writer.write_payload(&self.cookie.0)?;
> Ok(true)
> } else {
> - let is_frozen = freeze.node.owner.inner.lock().is_frozen;
> + let is_frozen = freeze.node.owner.inner.lock().is_frozen.is_fully_frozen();
> if freeze.last_is_frozen == Some(is_frozen) {
> return Ok(true);
> }
> @@ -254,7 +254,7 @@ pub(crate) fn freeze_notif_done(self: &Arc<Self>, reader: &mut UserSliceReader)
> );
> return Err(EINVAL);
> }
> - let is_frozen = freeze.node.owner.inner.lock().is_frozen;
> + let is_frozen = freeze.node.owner.inner.lock().is_frozen.is_fully_frozen();
> if freeze.is_clearing || freeze.last_is_frozen != Some(is_frozen) {
> // Immediately send another FreezeMessage.
> clear_msg = Some(FreezeMessage::init(alloc, cookie));
> diff --git a/drivers/android/binder/process.rs b/drivers/android/binder/process.rs
> index f13a747e784c84a0fb09cbf47442712106eba07c..2da9344397506a3f6d6b13411c45d5ded92d6db1 100644
> --- a/drivers/android/binder/process.rs
> +++ b/drivers/android/binder/process.rs
> @@ -72,6 +72,33 @@ fn new(address: usize, size: usize) -> Self {
> const PROC_DEFER_FLUSH: u8 = 1;
> const PROC_DEFER_RELEASE: u8 = 2;
>
> +#[derive(Copy, Clone)]
> +pub(crate) enum IsFrozen {
> + Yes,
> + No,
> + InProgress,
> +}
> +
> +impl IsFrozen {
> + /// Whether incoming transactions should be rejected due to freeze.
> + pub(crate) fn is_frozen(self) -> bool {
> + match self {
> + IsFrozen::Yes => true,
> + IsFrozen::No => false,
> + IsFrozen::InProgress => true,
> + }
> + }
> +
> + /// Whether freeze notifications consider this process frozen.
> + pub(crate) fn is_fully_frozen(self) -> bool {
> + match self {
> + IsFrozen::Yes => true,
> + IsFrozen::No => false,
> + IsFrozen::InProgress => false,
> + }
> + }
> +}
> +
> /// The fields of `Process` protected by the spinlock.
> pub(crate) struct ProcessInner {
> is_manager: bool,
> @@ -98,7 +125,7 @@ pub(crate) struct ProcessInner {
> /// are woken up.
> outstanding_txns: u32,
> /// Process is frozen and unable to service binder transactions.
> - pub(crate) is_frozen: bool,
> + pub(crate) is_frozen: IsFrozen,
> /// Process received sync transactions since last frozen.
> pub(crate) sync_recv: bool,
> /// Process received async transactions since last frozen.
> @@ -124,7 +151,7 @@ fn new() -> Self {
> started_thread_count: 0,
> defer_work: 0,
> outstanding_txns: 0,
> - is_frozen: false,
> + is_frozen: IsFrozen::No,
> sync_recv: false,
> async_recv: false,
> binderfs_file: None,
> @@ -1260,7 +1287,7 @@ fn deferred_release(self: Arc<Self>) {
> let is_manager = {
> let mut inner = self.inner.lock();
> inner.is_dead = true;
> - inner.is_frozen = false;
> + inner.is_frozen = IsFrozen::No;
> inner.sync_recv = false;
> inner.async_recv = false;
> inner.is_manager
> @@ -1371,7 +1398,7 @@ pub(crate) fn drop_outstanding_txn(&self) {
> return;
> }
> inner.outstanding_txns -= 1;
> - inner.is_frozen && inner.outstanding_txns == 0
> + inner.is_frozen.is_frozen() && inner.outstanding_txns == 0
> };
>
> if wake {
> @@ -1385,7 +1412,7 @@ pub(crate) fn ioctl_freeze(&self, info: &BinderFreezeInfo) -> Result {
> let mut inner = self.inner.lock();
> inner.sync_recv = false;
> inner.async_recv = false;
> - inner.is_frozen = false;
> + inner.is_frozen = IsFrozen::No;
> drop(inner);
> msgs.send_messages();
> return Ok(());
> @@ -1394,7 +1421,7 @@ pub(crate) fn ioctl_freeze(&self, info: &BinderFreezeInfo) -> Result {
> let mut inner = self.inner.lock();
> inner.sync_recv = false;
> inner.async_recv = false;
> - inner.is_frozen = true;
> + inner.is_frozen = IsFrozen::InProgress;
>
> if info.timeout_ms > 0 {
> let mut jiffies = kernel::time::msecs_to_jiffies(info.timeout_ms);
> @@ -1408,7 +1435,7 @@ pub(crate) fn ioctl_freeze(&self, info: &BinderFreezeInfo) -> Result {
> .wait_interruptible_timeout(&mut inner, jiffies)
> {
> CondVarTimeoutResult::Signal { .. } => {
> - inner.is_frozen = false;
> + inner.is_frozen = IsFrozen::No;
> return Err(ERESTARTSYS);
> }
> CondVarTimeoutResult::Woken { jiffies: remaining } => {
> @@ -1422,17 +1449,18 @@ pub(crate) fn ioctl_freeze(&self, info: &BinderFreezeInfo) -> Result {
> }
>
> if inner.txns_pending_locked() {
> - inner.is_frozen = false;
> + inner.is_frozen = IsFrozen::No;
> Err(EAGAIN)
> } else {
> drop(inner);
> match self.prepare_freeze_messages() {
> Ok(batch) => {
> + self.inner.lock().is_frozen = IsFrozen::Yes;
> batch.send_messages();
> Ok(())
> }
> Err(kernel::alloc::AllocError) => {
> - self.inner.lock().is_frozen = false;
> + self.inner.lock().is_frozen = IsFrozen::No;
> Err(ENOMEM)
> }
> }
> diff --git a/drivers/android/binder/transaction.rs b/drivers/android/binder/transaction.rs
> index 02512175d6229535373f2d3e543ba8c91ecd72f0..4bd3c0e417eb93d5d62d9c20daadde1fb0e4990f 100644
> --- a/drivers/android/binder/transaction.rs
> +++ b/drivers/android/binder/transaction.rs
> @@ -249,7 +249,7 @@ pub(crate) fn submit(self: DLArc<Self>) -> BinderResult {
>
> if oneway {
> if let Some(target_node) = self.target_node.clone() {
> - if process_inner.is_frozen {
> + if process_inner.is_frozen.is_frozen() {
> process_inner.async_recv = true;
> if self.flags & TF_UPDATE_TXN != 0 {
> if let Some(t_outdated) =
> @@ -270,7 +270,7 @@ pub(crate) fn submit(self: DLArc<Self>) -> BinderResult {
> }
> }
>
> - if process_inner.is_frozen {
> + if process_inner.is_frozen.is_frozen() {
> return Err(BinderError::new_frozen_oneway());
> } else {
> return Ok(());
> @@ -280,7 +280,7 @@ pub(crate) fn submit(self: DLArc<Self>) -> BinderResult {
> }
> }
>
> - if process_inner.is_frozen {
> + if process_inner.is_frozen.is_frozen() {
> process_inner.sync_recv = true;
> return Err(BinderError::new_frozen());
> }
>
> --
> 2.51.0.618.g983fd99d29-goog
>
Acked-by: Carlos Llamas <cmllamas@...gle.com>
Powered by blists - more mailing lists