Re: [PATCH v2] binder: frozen notification

[Carlos Llamas <cmllamas@google.com>]

On Mon, Jun 24, 2024 at 08:50:43AM -0700, Yu-Ting Tseng wrote:
> On Mon, Jun 24, 2024 at 7:25 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > On Sat, Jun 22, 2024 at 5:22 AM Yu-Ting Tseng <yutingtseng@google.com> wrote:
> > >
> > > Frozen processes present a significant challenge in binder transactions.
> > > When a process is frozen, it cannot, by design, accept and/or respond to
> > > binder transactions. As a result, the sender needs to adjust its
> > > behavior, such as postponing transactions until the peer process
> > > unfreezes. However, there is currently no way to subscribe to these
> > > state change events, making it impossible to implement frozen-aware
> > > behaviors efficiently.
> > >
> > > Introduce a binder API for subscribing to frozen state change events.
> > > This allows programs to react to changes in peer process state,
> > > mitigating issues related to binder transactions sent to frozen
> > > processes.
> > >
> > > Implementation details:
> > > For a given binder_ref, the state of frozen notification can be one of
> > > the followings:
> > > 1. Userspace doesn't want a notification. binder_ref->freeze is null.
> > > 2. Userspace wants a notification but none is in flight.
> > >    list_empty(&binder_ref->freeze->work.entry) = true
> > > 3. A notification is in flight and waiting to be read by userspace.
> > >    binder_ref_freeze.sent is false.
> > > 4. A notification was read by userspace and kernel is waiting for an ack.
> > >    binder_ref_freeze.sent is true.
> > >
> > > When a notification is in flight, new state change events are coalesced into
> > > the existing binder_ref_freeze struct. If userspace hasn't picked up the
> > > notification yet, the driver simply rewrites the state. Otherwise, the
> > > notification is flagged as requiring a resend, which will be performed
> > > once userspace acks the original notification that's inflight.
> > >
> > > See https://android-review.googlesource.com/c/platform/frameworks/native/+/3070045
> > > for how userspace is going to use this feature.
> > >
> > > Signed-off-by: Yu-Ting Tseng <yutingtseng@google.com>
> >
> > [...]
> >
> > > +               /*
> > > +                * There is already a freeze notification. Take it over and rewrite
> > > +                * the work type. If it was already sent, flag it for re-sending;
> > > +                * Otherwise it's pending and will be sent soon.
> > > +                */
> > > +               freeze->work.type = BINDER_WORK_CLEAR_DEATH_NOTIFICATION;
> >
> > I have not done a comprehensive review yet, but this looks wrong.
> [resending as plain text]
> 
> Thanks for looking at the change!
> 
> The code here seems correct to me. Could you please elaborate why this
> looks wrong?
> 
> This part of code gets executed if freeze->work.entry is in a list.
> There are two possibilities:
> 1. The freeze work of type BINDER_WORK_FROZEN_BINDER is in the work
> queue, waiting to be picked up by binder_thread_read. Since it hasn't
> been picked up yet, this code rewrites the work type to
> BINDER_WORK_CLEAR_DEATH_NOTIFICATION, effectively canceling the state
> change notification and instead making binder_thread_read send a
> BR_CLEAR_FREEZE_NOTIFICATION_DONE to userspace. The API contract
> allows coalescing of events. I can explicitly mention this case if it
> helps.
> 2. The freeze work of type BINDER_WORK_FROZEN_BINDER is in the
> proc->delivered_freeze queue. This means a state change notification
> was just sent to userspace and the kernel is waiting for an ack.
> freeze->sent is true. In this case we set freeze->resend to true. Once
> the kernel receives the ack, it would queue up the work again, whose
> type was already set to BINDER_WORK_CLEAR_DEATH_NOTIFICATION.
> 
> Yu-Ting

Alice means you want to use BINDER_WORK_CLEAR_FREEZE_NOTIFICATION and
not BINDER_WORK_CLEAR_DEATH_NOTIFICATION. 

> 
> >
> >
> > Is there any chance that we could have a test in aosp that would have
> > caught this?
> >
> > Alice