Messages in this thread | | | From | Alice Ryhl <> | Date | Tue, 23 Apr 2024 10:18:36 +0200 | Subject | Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl |
| |
On Tue, Apr 23, 2024 at 12:48 AM Carlos Llamas <cmllamas@google.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@google.com> wrote: > > > > > > On Thu, Apr 18, 2024 at 08:34:47AM +0000, Alice Ryhl wrote: > > > > Carlos Llamas <cmllamas@google.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@android.com> > > > > > Signed-off-by: Carlos Llamas <cmllamas@google.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
| |