[PATCH v2 1/3] rust_binder: freeze_notif_done should resend if wrong state

Alice Ryhl posted 3 patches 4 months ago
[PATCH v2 1/3] rust_binder: freeze_notif_done should resend if wrong state
Posted by Alice Ryhl 4 months ago
Consider the following scenario:
1. A freeze notification is delivered to thread 1.
2. The process becomes frozen or unfrozen.
3. The message for step 2 is delivered to thread 2 and ignored because
   there is already a pending notification from step 1.
4. Thread 1 acknowledges the notification from step 1.
In this case, step 4 should ensure that the message ignored in step 3 is
resent as it can now be delivered.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 drivers/android/binder/freeze.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/android/binder/freeze.rs b/drivers/android/binder/freeze.rs
index e68c3c8bc55a203c32261c23915d8c427569e3b0..74bebb8d4d9b24860eed34363ce69b1c6df58028 100644
--- a/drivers/android/binder/freeze.rs
+++ b/drivers/android/binder/freeze.rs
@@ -245,8 +245,9 @@ pub(crate) fn freeze_notif_done(self: &Arc<Self>, reader: &mut UserSliceReader)
                 );
                 return Err(EINVAL);
             }
-            if freeze.is_clearing {
-                // Immediately send another FreezeMessage for BR_CLEAR_FREEZE_NOTIFICATION_DONE.
+            let is_frozen = freeze.node.owner.inner.lock().is_frozen;
+            if freeze.is_clearing || freeze.last_is_frozen != Some(is_frozen) {
+                // Immediately send another FreezeMessage.
                 clear_msg = Some(FreezeMessage::init(alloc, cookie));
             }
             freeze.is_pending = false;

-- 
2.51.0.618.g983fd99d29-goog
Re: [PATCH v2 1/3] rust_binder: freeze_notif_done should resend if wrong state
Posted by Carlos Llamas 4 months ago
On Tue, Oct 07, 2025 at 09:39:51AM +0000, Alice Ryhl wrote:
> Consider the following scenario:
> 1. A freeze notification is delivered to thread 1.
> 2. The process becomes frozen or unfrozen.
> 3. The message for step 2 is delivered to thread 2 and ignored because
>    there is already a pending notification from step 1.
> 4. Thread 1 acknowledges the notification from step 1.
> In this case, step 4 should ensure that the message ignored in step 3 is
> resent as it can now be delivered.

hmmm, I wonder what happens with 3 threads involved where the state goes
back to the (unconsumed) initial freeze notification. Userspace will
probably see two separate notifications of the same state?

> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  drivers/android/binder/freeze.rs | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/android/binder/freeze.rs b/drivers/android/binder/freeze.rs
> index e68c3c8bc55a203c32261c23915d8c427569e3b0..74bebb8d4d9b24860eed34363ce69b1c6df58028 100644
> --- a/drivers/android/binder/freeze.rs
> +++ b/drivers/android/binder/freeze.rs
> @@ -245,8 +245,9 @@ pub(crate) fn freeze_notif_done(self: &Arc<Self>, reader: &mut UserSliceReader)
>                  );
>                  return Err(EINVAL);
>              }
> -            if freeze.is_clearing {
> -                // Immediately send another FreezeMessage for BR_CLEAR_FREEZE_NOTIFICATION_DONE.
> +            let is_frozen = freeze.node.owner.inner.lock().is_frozen;
> +            if freeze.is_clearing || freeze.last_is_frozen != Some(is_frozen) {
> +                // Immediately send another FreezeMessage.
>                  clear_msg = Some(FreezeMessage::init(alloc, cookie));
>              }
>              freeze.is_pending = false;
> 
> -- 
> 2.51.0.618.g983fd99d29-goog
> 

Acked-by: Carlos Llamas <cmllamas@google.com>
Re: [PATCH v2 1/3] rust_binder: freeze_notif_done should resend if wrong state
Posted by Alice Ryhl 4 months ago
On Wed, Oct 8, 2025 at 6:32 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Tue, Oct 07, 2025 at 09:39:51AM +0000, Alice Ryhl wrote:
> > Consider the following scenario:
> > 1. A freeze notification is delivered to thread 1.
> > 2. The process becomes frozen or unfrozen.
> > 3. The message for step 2 is delivered to thread 2 and ignored because
> >    there is already a pending notification from step 1.
> > 4. Thread 1 acknowledges the notification from step 1.
> > In this case, step 4 should ensure that the message ignored in step 3 is
> > resent as it can now be delivered.
>
> hmmm, I wonder what happens with 3 threads involved where the state goes
> back to the (unconsumed) initial freeze notification. Userspace will
> probably see two separate notifications of the same state?

The way I implemented it, the work items report the current state when
the work item is *executed*, and they do nothing if there's no change
since last notification.

Alice
Re: [PATCH v2 1/3] rust_binder: freeze_notif_done should resend if wrong state
Posted by Carlos Llamas 4 months ago
On Wed, Oct 08, 2025 at 06:34:54PM +0200, Alice Ryhl wrote:
> On Wed, Oct 8, 2025 at 6:32 PM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > On Tue, Oct 07, 2025 at 09:39:51AM +0000, Alice Ryhl wrote:
> > > Consider the following scenario:
> > > 1. A freeze notification is delivered to thread 1.
> > > 2. The process becomes frozen or unfrozen.
> > > 3. The message for step 2 is delivered to thread 2 and ignored because
> > >    there is already a pending notification from step 1.
> > > 4. Thread 1 acknowledges the notification from step 1.
> > > In this case, step 4 should ensure that the message ignored in step 3 is
> > > resent as it can now be delivered.
> >
> > hmmm, I wonder what happens with 3 threads involved where the state goes
> > back to the (unconsumed) initial freeze notification. Userspace will
> > probably see two separate notifications of the same state?
> 
> The way I implemented it, the work items report the current state when
> the work item is *executed*, and they do nothing if there's no change
> since last notification.

Oh I see, then that means the 2nd and 3rd notifications would do nothing
as the state went back to the last notification, correct?
Re: [PATCH v2 1/3] rust_binder: freeze_notif_done should resend if wrong state
Posted by Alice Ryhl 4 months ago
On Wed, Oct 8, 2025 at 6:38 PM Carlos Llamas <cmllamas@google.com> wrote:
>
> On Wed, Oct 08, 2025 at 06:34:54PM +0200, Alice Ryhl wrote:
> > On Wed, Oct 8, 2025 at 6:32 PM Carlos Llamas <cmllamas@google.com> wrote:
> > >
> > > On Tue, Oct 07, 2025 at 09:39:51AM +0000, Alice Ryhl wrote:
> > > > Consider the following scenario:
> > > > 1. A freeze notification is delivered to thread 1.
> > > > 2. The process becomes frozen or unfrozen.
> > > > 3. The message for step 2 is delivered to thread 2 and ignored because
> > > >    there is already a pending notification from step 1.
> > > > 4. Thread 1 acknowledges the notification from step 1.
> > > > In this case, step 4 should ensure that the message ignored in step 3 is
> > > > resent as it can now be delivered.
> > >
> > > hmmm, I wonder what happens with 3 threads involved where the state goes
> > > back to the (unconsumed) initial freeze notification. Userspace will
> > > probably see two separate notifications of the same state?
> >
> > The way I implemented it, the work items report the current state when
> > the work item is *executed*, and they do nothing if there's no change
> > since last notification.
>
> Oh I see, then that means the 2nd and 3rd notifications would do nothing
> as the state went back to the last notification, correct?

Yeah.

If the state flips quickly, userspace might not get told about that if
it's too slow to receive the update, but that's no different from C
Binder.

Alice
Re: [PATCH v2 1/3] rust_binder: freeze_notif_done should resend if wrong state
Posted by Carlos Llamas 4 months ago
On Wed, Oct 08, 2025 at 06:41:20PM +0200, Alice Ryhl wrote:
> On Wed, Oct 8, 2025 at 6:38 PM Carlos Llamas <cmllamas@google.com> wrote:
> >
> > On Wed, Oct 08, 2025 at 06:34:54PM +0200, Alice Ryhl wrote:
> > > On Wed, Oct 8, 2025 at 6:32 PM Carlos Llamas <cmllamas@google.com> wrote:
> > > >
> > > > On Tue, Oct 07, 2025 at 09:39:51AM +0000, Alice Ryhl wrote:
> > > > > Consider the following scenario:
> > > > > 1. A freeze notification is delivered to thread 1.
> > > > > 2. The process becomes frozen or unfrozen.
> > > > > 3. The message for step 2 is delivered to thread 2 and ignored because
> > > > >    there is already a pending notification from step 1.
> > > > > 4. Thread 1 acknowledges the notification from step 1.
> > > > > In this case, step 4 should ensure that the message ignored in step 3 is
> > > > > resent as it can now be delivered.
> > > >
> > > > hmmm, I wonder what happens with 3 threads involved where the state goes
> > > > back to the (unconsumed) initial freeze notification. Userspace will
> > > > probably see two separate notifications of the same state?
> > >
> > > The way I implemented it, the work items report the current state when
> > > the work item is *executed*, and they do nothing if there's no change
> > > since last notification.
> >
> > Oh I see, then that means the 2nd and 3rd notifications would do nothing
> > as the state went back to the last notification, correct?
> 
> Yeah.
> 
> If the state flips quickly, userspace might not get told about that if
> it's too slow to receive the update, but that's no different from C
> Binder.

I believe the difference is C binder doesn't report the current state at
the time of consuming the notification. So I'm thinking that it would
report two notifications regardless of the state, even if they are both
the same. Oh well.

Thanks,
Carlos Llamas
Re: [PATCH v2 1/3] rust_binder: freeze_notif_done should resend if wrong state
Posted by Alice Ryhl 4 months ago
On Wed, Oct 08, 2025 at 04:52:54PM +0000, Carlos Llamas wrote:
> On Wed, Oct 08, 2025 at 06:41:20PM +0200, Alice Ryhl wrote:
> > On Wed, Oct 8, 2025 at 6:38 PM Carlos Llamas <cmllamas@google.com> wrote:
> > >
> > > On Wed, Oct 08, 2025 at 06:34:54PM +0200, Alice Ryhl wrote:
> > > > On Wed, Oct 8, 2025 at 6:32 PM Carlos Llamas <cmllamas@google.com> wrote:
> > > > >
> > > > > On Tue, Oct 07, 2025 at 09:39:51AM +0000, Alice Ryhl wrote:
> > > > > > Consider the following scenario:
> > > > > > 1. A freeze notification is delivered to thread 1.
> > > > > > 2. The process becomes frozen or unfrozen.
> > > > > > 3. The message for step 2 is delivered to thread 2 and ignored because
> > > > > >    there is already a pending notification from step 1.
> > > > > > 4. Thread 1 acknowledges the notification from step 1.
> > > > > > In this case, step 4 should ensure that the message ignored in step 3 is
> > > > > > resent as it can now be delivered.
> > > > >
> > > > > hmmm, I wonder what happens with 3 threads involved where the state goes
> > > > > back to the (unconsumed) initial freeze notification. Userspace will
> > > > > probably see two separate notifications of the same state?
> > > >
> > > > The way I implemented it, the work items report the current state when
> > > > the work item is *executed*, and they do nothing if there's no change
> > > > since last notification.
> > >
> > > Oh I see, then that means the 2nd and 3rd notifications would do nothing
> > > as the state went back to the last notification, correct?
> > 
> > Yeah.
> > 
> > If the state flips quickly, userspace might not get told about that if
> > it's too slow to receive the update, but that's no different from C
> > Binder.
> 
> I believe the difference is C binder doesn't report the current state at
> the time of consuming the notification. So I'm thinking that it would
> report two notifications regardless of the state, even if they are both
> the same. Oh well.

Yeah, I guess if it only toggles once, then C Binder will report that.
On the other hand, if it toggles multiple times, then C Binder might not
report it that many times.

We could make Rust Binder faithfully report the callback the right
number of times in the face of toggling, but it seems not worth it.

Alice