Received-SPF: pass (google.com: domain of linux-kernel+bounces-152393-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Sat, 20 Apr 2024 23:39:41 +0000
From: Carlos Llamas <cmllamas@google.com>
To: Alice Ryhl <aliceryhl@google.com>
Cc: arve@android.com, brauner@kernel.org, gregkh@linuxfoundation.org,
	joel@joelfernandes.org, kernel-team@android.com,
	linux-kernel@vger.kernel.org, maco@android.com, surenb@google.com,
	tkjos@android.com
Subject: Re: [PATCH 1/4] binder: introduce BINDER_SET_PROC_FLAGS ioctl
Message-ID: <ZiRSPXSuSMyUO0Cw@google.com>
References: <20240417191418.1341988-2-cmllamas@google.com>
 <20240418083447.3877366-1-aliceryhl@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240418083447.3877366-1-aliceryhl@google.com>

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.)
> 
> 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.

--
Carlos Llamas