Re: [PATCH v1 2/2] binderfs: reserve devices for initial mount

[Christian Brauner]

On Thu, Jan 03, 2019 at 01:47:13PM -0800, Todd Kjos wrote:
> On Thu, Jan 3, 2019 at 12:34 PM Christian Brauner <christian@xxxxxxxxxx> wrote:
> >
> > On Thu, Jan 03, 2019 at 12:25:24PM -0800, Todd Kjos wrote:
> > > On Sun, Dec 23, 2018 at 6:36 AM Christian Brauner <christian@xxxxxxxxxx> wrote:
> > > >
> > > > The binderfs instance in the initial ipc namespace will always have a
> > > > reserve of 4 binder devices unless explicitly capped by specifying a lower
> > > > value via the "max" mount option.
> > > > This ensures when binder devices are removed (on accident or on purpose)
> > > > they can always be recreated without risking that all minor numbers have
> > > > already been used up.
> > > >
> > > > Cc: Todd Kjos <tkjos@xxxxxxxxxx>
> > > > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > > Signed-off-by: Christian Brauner <christian.brauner@xxxxxxxxxx>
> > > > ---
> > > > v1:
> > > > - patch introduced
> > > > v0:
> > > > - patch not present
> > > > ---
> > > >  drivers/android/binderfs.c | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > > > index 873adda064ac..aa635c7ea727 100644
> > > > --- a/drivers/android/binderfs.c
> > > > +++ b/drivers/android/binderfs.c
> > > > @@ -40,6 +40,8 @@
> > > >  #define INODE_OFFSET 3
> > > >  #define INTSTRLEN 21
> > > >  #define BINDERFS_MAX_MINOR (1U << MINORBITS)
> > > > +/* Ensure that the initial ipc namespace always has a devices available. */
> > > > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4)
> > >
> > > Why do you ever need more minors per instance than the number of
> > > devices listed in CONFIG_ANDROID_BINDER_DEVICES?
> >
> > Are you asking asking why this is 4 and not 3? In that case we should
> > probably parse CONFIG_ANDROID_BINDER_DEVICES once at init time and then
> > reserve that many binder devices. Thoughts?
> 
> I'm asking why CONFIG_ANDROID_BINDER_DEVICES isn't the source of truth
> for the number of devices (it normally specifies 3 devices, but could
> be different). I can't think of a reason why you'd want
> CONFIG_MAX_MINOR_CAPPED to be different than the number of devices
> indicated by CONFIG_ANDROID_BINDER_DEVICES and having it specified in
> two places seems error prone.

I'm not following. The CONFIG_MAX_MINOR_CAPPED and
CONFIG_ANDROID_BINDER_DEVICES do not have a relation to each other just
like binder devices requested in CONFIG_ANDROID_BINDER_DEVICES do not
have a relation to binderfs binder devices as was requested for this
patchset.
I also don't know what you mean "appear in two places".

The fact is, binderfs binder devices are independent of binderfs binder
devices. So it is perfectly reasonable for someone to say
CONFIG_ANDROID_BINDER_DEVICES="" and *only* rely on binderfs itself.
Which absolutely need to be possible.
What I want in such scenarios is that people always have a number of
binderfs binder devices guaranteed to be available in the binderfs mount
in the initial ipc namespace no matter how many devices are allocated in
non-initial ipc namespace binderfs mounts. That's why allo non-initial
ipc namespace binderfs mounts will use the CONFIG_MAX_MINOR_CAPPED macro
which guarantees that there's number of devices reserved for the
binderfs instance in the initial ipc namespace.