Re: [PATCH] binder: implement binderfs

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Dec 05, 2018 at 10:42:06PM +0100, Christian Brauner wrote:
> On Wed, Dec 05, 2018 at 09:01:45PM +0100, Greg KH wrote:
> > > /* binder-control */
> > > Each new binderfs instance comes with a binder-control device. No other
> > > devices will be present at first. The binder-control device can be used to
> > > dynamically allocate binder devices. All requests operate on the binderfs
> > > mount the binder-control device resides in:
> > > - BINDER_CTL_ADD
> > >   Allocate a new binder device.
> > > Assuming a new instance of binderfs has been mounted at /dev/binderfs via
> > > mount -t binderfs binderfs /dev/binderfs. Then a request to create a new
> > > binder device can be made via:
> > > 
> > > struct binderfs_device device = {0};
> > > int fd = open("/dev/binderfs/binder-control", O_RDWR);
> > > ioctl(fd, BINDER_CTL_ADD, &device);
> > > 
> > > The struct binderfs_device will be used to return the major and minor
> > > number, as well as the index used as the new name for the device.
> > > Binderfs devices can simply be removed via unlink().
> > 
> > I think you should provide a name in the BINDER_CTL_ADD command.  That
> > way you can easily emulate the existing binder queues, and it saves you
> > a create/rename sequence that you will be forced to do otherwise.  Why
> > not do it just in a single command?
> 
> Sounds reasonable. How do you feel about capping the name length at 255
> bytes aka the standard Linux file name length (e.g. xfs, ext4 etc.)?
> 
> #define BINDERFS_NAME_MAX 255
> 
> struct binderfs_device {
>         char name[BINDERFS_NAME_MAX + 1];

__u8 is the proper type to cross the user/kernel boundry :)

> 	__u32 major;
> 	__u32 minor;
> }

Yes, limiting it to 255 is fine with me.

> > That way also you don't need to care about the major/minor number at
> > all.  Userspace should never need to worry about that, use a name,
> > that's the best thing.  Also, it allows you to drop the use of the idr,
> > making the kernel code simpler overall.
> > 
> > > /* Implementation details */
> > > - When binderfs is registered as a new filesystem it will dynamically
> > >   allocate a new major number. The allocated major number will be returned
> > >   in struct binderfs_device when a new binder device is allocated.
> > 
> > Why does userspace care about major/minor numbers at all?  You should
> 
> Userspace cares for the sake of the devices cgroup which operates on
> device numnbers to restrict access to devices. Since binderfs doesn't
> have a static major number returning that information is helpful.

Ugh, ok, that makes sense.  If we really want to make the kernel
interface simpler, drop the major/minor and then have userspace do the
stat(2) to see what the major/minor number they care about is.

But yeah, keeping it here makes everyone's life simpler, the kernel
already knows this, and it's trivial to pass it back to userspace this
way.

Care to make this change and resend?

thanks,

greg k-h