Message-ID: <CAH5fLgjuK2M_iDQdxT7f_CJHZN9S5u0p7raBwhV8tNCYnCN8Jg@mail.gmail.com>
Date: Tue, 23 Apr 2024 10:18:36 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: arve@...roid.com, brauner@...nel.org, gregkh@...uxfoundation.org, 
	joel@...lfernandes.org, kernel-team@...roid.com, linux-kernel@...r.kernel.org, 
	maco@...roid.com, surenb@...gle.com, tkjos@...roid.com
Subject: Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl

On Tue, Apr 23, 2024 at 12:48 AM Carlos Llamas <cmllamas@...gle.com> wrote:
>
> On Mon, Apr 22, 2024 at 10:56:31AM +0200, Alice Ryhl wrote:
> > On Sun, Apr 21, 2024 at 1:39 AM Carlos Llamas <cmllamas@...gle.com> wrote:
> > >
> > > On Thu, Apr 18, 2024 at 08:34:47AM +0000, Alice Ryhl wrote:
> > > > Carlos Llamas <cmllamas@...gle.com> writes:
> > > > > This new ioctl enables userspace to control the individual behavior of
> > > > > the 'struct binder_proc' instance via flags. The driver validates and
> > > > > returns the supported subset. Some existing ioctls are migrated to use
> > > > > these flags in subsequent commits.
> > > > >
> > > > > Suggested-by: Arve Hjønnevåg <arve@...roid.com>
> > > > > Signed-off-by: Carlos Llamas <cmllamas@...gle.com>
> > > > > ---
> > > > >  drivers/android/binder.c            | 25 +++++++++++++++++++++++++
> > > > >  drivers/android/binder_internal.h   |  4 +++-
> > > > >  include/uapi/linux/android/binder.h |  6 ++++++
> > > > >  3 files changed, 34 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > > > index bad28cf42010..e0d193bfb237 100644
> > > > > --- a/drivers/android/binder.c
> > > > > +++ b/drivers/android/binder.c
> > > > > @@ -5334,6 +5334,26 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
> > > > >     return 0;
> > > > >  }
> > > > >
> > > > > +static int binder_ioctl_set_proc_flags(struct binder_proc *proc,
> > > > > +                                  u32 __user *user)
> > > > > +{
> > > > > +   u32 flags;
> > > > > +
> > > > > +   if (get_user(flags, user))
> > > > > +           return -EFAULT;
> > > > > +
> > > > > +   binder_inner_proc_lock(proc);
> > > > > +   flags &= PF_SUPPORTED_FLAGS_MASK;
> > > > > +   proc->flags = flags;
> > > > > +   binder_inner_proc_unlock(proc);
> > > > > +
> > > > > +   /* confirm supported flags with user */
> > > > > +   if (put_user(flags, user))
> > > > > +           return -EFAULT;
> > > > > +
> > > > > +   return 0;
> > > > > +}
> > > >
> > > > I'm just thinking out loud here, but is this the best API for this
> > > > ioctl? Using this API, if I want to toggle the oneway-spam-detection
> > > > flag, then I can't do so without knowing the value of all other flags,
> > > > and I also need to synchronize all calls to this ioctl.
> > > >
> > > > That's fine for the current use-case where these flags are only set
> > > > during startup, but are we confident that no future flag will be toggled
> > > > while a process is active?
> > >
> > > hmmm, this is a very good point. It would probably lead to userspace
> > > having to cache its flags for every binder instance. This is not a good
> > > solution at all.
> > >
> > > >
> > > > How about these alternatives?
> > > >
> > > > 1. Userspace passes two masks, one containing bits to set, and another
> > > >    containing bits to unset. Userspace returns new value of flags. (If
> > > >    the same bit is set in both masks, we can fail with EINVAL.)
> >
> > To add to this one, one could also say that if a bit is set in both,
> > then the value is toggled.
> >
> > > > 2. Compare and swap. Userspace passes the expected previous value and
> > > >    the desired new value. The kernel returns the actual previous value
> > > >    and updates it only if userspace gave the right previous value.
> > > >
> > > > 3. Set or unset only. Userspace passes a boolean and a mask. Boolean
> > > >    determines whether userspace wants to set or unset the bits set in
> > > >    the mask.
> > > >
> > > > I don't know what the usual kernel convention is for this kind of
> > > > ioctl, so I'm happy with whatever you all think is best.
> > >
> > > I've never come across these types of alternatives personally. What I've
> > > seen however, is the typical SET/GET ioctl pairs. This is a "simpler"
> > > interface I guess but it has the downside of an extra roundtrip. e.g.
> > >
> > >         ioctl(fd, BINDER_GET_PROC_FLAGS, &flags);
> > >         flags |= BF_LARGE_HANDLES;
> > >         ioctl(fd, BINDER_SET_PROC_FLAGS, &flags);
> > >
> > > What seems tempting about the SET/GET pair is that we could replace the
> > > BINDER_ENABLE_ONEWAY_SPAM_DETECTION with the SET. Instead of maintaining
> > > legacy code for the "deprecated" ioctl.
> > >
> > > wdyt?
> > >
> > > I'll have a second look at the alternatives you mentioned. Perhaps I can
> > > reference some other existing ioctl that does something similar.
> >
> > Hmm. I don't think a get/set pair improves the situation much.
> > Userspace still needs a global mutex for making changes to the flag in
> > that case. Otherwise, two threads changing two different flags in
> > parallel could result in a race condition where one of the changes is
> > lost.
>
> I'm not sure this would ever be a problem, libbinder currently has a
> mutex for this kind of things already and it seems unlikely that these
> process-wide flags would be toggled outside of initial config. However,
> it is worth discussing for sure as things can change.
>
> I'm mainly concern about overloading what should be a very simple ioctl.
> With that said, I think one more option that is fairly simple/common is
> a SET/CLEAR ioctl pair. A little adaption from your 3rd option I think?
> I would be fine with that.
>
> Unfortunately, this would require new ioctl IDs so we would still need
> to maintain the ONEWAY_SPAM thing. I suppose that's ok.

As long as your decision has taken the things I mentioned into
account, I'm happy with whatever you think is best. If you think a
GET/SET pair is reasonable because binder already has a mutex, then
that's fine with me.

Alice

Powered by blists - more mailing lists