[PATCH v2 0/2] Add default binderfs devices

[PATCH v2 0/2] Add default binderfs devices

[Hridya Valsaraju]

Binderfs was created to help provide private binder devices to
containers in their own IPC namespace. Currently, every time a new binderfs
instance is mounted, its private binder devices need to be created via
IOCTL calls. This patch series eliminates the effort for creating
the default binder devices for each binderfs instance by creating them
automatically.

Hridya Valsaraju (2):
  binder: Add default binder devices through binderfs when configured
  binder: Validate the default binderfs device names.

 drivers/android/binder.c          |  5 +++--
 drivers/android/binder_internal.h |  2 ++
 drivers/android/binderfs.c        | 37 ++++++++++++++++++++++++++++---
 3 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v2 1/2] binder: Add default binder devices through binderfs when configured

[Hridya Valsaraju]

Currently, since each binderfs instance needs its own
private binder devices, every time a binderfs instance is
mounted, all the default binder devices need to be created
via the BINDER_CTL_ADD IOCTL. This patch aims to
add a solution to automatically create the default binder
devices for each binderfs instance that gets mounted.
To achieve this goal, when CONFIG_ANDROID_BINDERFS is set,
the default binder devices specified by CONFIG_ANDROID_BINDER_DEVICES
are created in each binderfs instance instead of global devices
being created by the binder driver.

Co-developed-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Hridya Valsaraju <hridya@google.com>
---

Changes in v2:
- Updated commit message as per Greg Kroah-Hartman.
- Removed new module parameter creation as per Greg
  Kroah-Hartman/Christian Brauner.
- Refactored device name length check into a new patch as per Greg Kroah-Hartman.

 drivers/android/binder.c          |  5 +++--
 drivers/android/binder_internal.h |  2 ++
 drivers/android/binderfs.c        | 25 ++++++++++++++++++++++---
 3 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 466b6a7f8ab7..ca6b21a53321 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -123,7 +123,7 @@ static uint32_t binder_debug_mask = BINDER_DEBUG_USER_ERROR |
 	BINDER_DEBUG_FAILED_TRANSACTION | BINDER_DEBUG_DEAD_TRANSACTION;
 module_param_named(debug_mask, binder_debug_mask, uint, 0644);
 
-static char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
+char *binder_devices_param = CONFIG_ANDROID_BINDER_DEVICES;
 module_param_named(devices, binder_devices_param, charp, 0444);
 
 static DECLARE_WAIT_QUEUE_HEAD(binder_user_error_wait);
@@ -6279,7 +6279,8 @@ static int __init binder_init(void)
 				    &transaction_log_fops);
 	}
 
-	if (strcmp(binder_devices_param, "") != 0) {
+	if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
+	    strcmp(binder_devices_param, "") != 0) {
 		/*
 		* Copy the module_parameter string, because we don't want to
 		* tokenize it in-place.
diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
index 045b3e42d98b..fe8c745dc8e0 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -37,6 +37,8 @@ struct binder_device {
 
 extern const struct file_operations binder_fops;
 
+extern char *binder_devices_param;
+
 #ifdef CONFIG_ANDROID_BINDERFS
 extern bool is_binderfs_device(const struct inode *inode);
 #else
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e773f45d19d9..886b4e0f482f 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -186,8 +186,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode,
 	req->major = MAJOR(binderfs_dev);
 	req->minor = minor;
 
-	ret = copy_to_user(userp, req, sizeof(*req));
-	if (ret) {
+	if (userp && copy_to_user(userp, req, sizeof(*req))) {
 		ret = -EFAULT;
 		goto err;
 	}
@@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 	int ret;
 	struct binderfs_info *info;
 	struct inode *inode = NULL;
+	struct binderfs_device device_info = { 0 };
+	const char *name;
+	size_t len;
 
 	sb->s_blocksize = PAGE_SIZE;
 	sb->s_blocksize_bits = PAGE_SHIFT;
@@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
 	if (!sb->s_root)
 		return -ENOMEM;
 
-	return binderfs_binder_ctl_create(sb);
+	ret = binderfs_binder_ctl_create(sb);
+	if (ret)
+		return ret;
+
+	name = binder_devices_param;
+	for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+		strscpy(device_info.name, name, len + 1);
+		ret = binderfs_binder_device_create(inode, NULL, &device_info);
+		if (ret)
+			return ret;
+		name += len;
+		if (*name == ',')
+			name++;
+
+	}
+
+	return 0;
+
 }
 
 static struct dentry *binderfs_mount(struct file_system_type *fs_type,
-- 
2.22.0.770.g0f2c4a37fd-goog

[PATCH v2 2/2] binder: Validate the default binderfs device names.

[Hridya Valsaraju]

Length of a binderfs device name cannot exceed BINDERFS_MAX_NAME.
This patch adds a check in binderfs_init() to ensure the same
for the default binder devices that will be created in every
binderfs instance.

Co-developed-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Signed-off-by: Hridya Valsaraju <hridya@google.com>
---
 drivers/android/binderfs.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 886b4e0f482f..52c8bd361906 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -572,6 +572,18 @@ static struct file_system_type binder_fs_type = {
 int __init init_binderfs(void)
 {
 	int ret;
+	const char *name;
+	size_t len;
+
+	/* Verify that the default binderfs device names are valid. */
+	name = binder_devices_param;
+	for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
+		if (len > BINDERFS_MAX_NAME)
+			return -E2BIG;
+		name += len;
+		if (*name == ',')
+			name++;
+	}
 
 	/* Allocate new major number for binderfs. */
 	ret = alloc_chrdev_region(&binderfs_dev, 0, BINDERFS_MAX_MINOR,
-- 
2.22.0.770.g0f2c4a37fd-goog

Re: [PATCH v2 1/2] binder: Add default binder devices through binderfs when configured

[Dan Carpenter]

On Tue, Aug 06, 2019 at 11:40:05AM -0700, Hridya Valsaraju wrote:
> @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  	int ret;
>  	struct binderfs_info *info;
>  	struct inode *inode = NULL;
> +	struct binderfs_device device_info = { 0 };
> +	const char *name;
> +	size_t len;
>  
>  	sb->s_blocksize = PAGE_SIZE;
>  	sb->s_blocksize_bits = PAGE_SHIFT;
> @@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
>  	if (!sb->s_root)
>  		return -ENOMEM;
>  
> -	return binderfs_binder_ctl_create(sb);
> +	ret = binderfs_binder_ctl_create(sb);
> +	if (ret)
> +		return ret;
> +
> +	name = binder_devices_param;
> +	for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> +		strscpy(device_info.name, name, len + 1);
> +		ret = binderfs_binder_device_create(inode, NULL, &device_info);
> +		if (ret)
> +			return ret;

We should probably clean up before returning...  The error handling code
would probably be tricky to write though and it's not super common.

> +		name += len;
> +		if (*name == ',')
> +			name++;
> +
> +	}
> +
> +	return 0;
> +

Remove this extra blank line.

>  }

regards,
dan carpenter

Re: [PATCH v2 1/2] binder: Add default binder devices through binderfs when configured

[Hridya Valsaraju]

On Wed, Aug 7, 2019 at 4:02 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Tue, Aug 06, 2019 at 11:40:05AM -0700, Hridya Valsaraju wrote:
> > @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> >       int ret;
> >       struct binderfs_info *info;
> >       struct inode *inode = NULL;
> > +     struct binderfs_device device_info = { 0 };
> > +     const char *name;
> > +     size_t len;
> >
> >       sb->s_blocksize = PAGE_SIZE;
> >       sb->s_blocksize_bits = PAGE_SHIFT;
> > @@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> >       if (!sb->s_root)
> >               return -ENOMEM;
> >
> > -     return binderfs_binder_ctl_create(sb);
> > +     ret = binderfs_binder_ctl_create(sb);
> > +     if (ret)
> > +             return ret;
> > +
> > +     name = binder_devices_param;
> > +     for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> > +             strscpy(device_info.name, name, len + 1);
> > +             ret = binderfs_binder_device_create(inode, NULL, &device_info);
> > +             if (ret)
> > +                     return ret;
>
> We should probably clean up before returning...  The error handling code
> would probably be tricky to write though and it's not super common.

Thank you for taking a look Dan. Did you mean cleaning up the default
devices that were already created? They will actually be cleaned up by
binderfs_evict_inode() during the super block's cleanup since the
mount operation will fail due to an error here.

>
> > +             name += len;
> > +             if (*name == ',')
> > +                     name++;
> > +
> > +     }
> > +
> > +     return 0;
> > +
>
> Remove this extra blank line.

Will do in v3, thanks for catching this Dan!

>
> >  }
>
> regards,
> dan carpenter
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
>

Re: [PATCH v2 1/2] binder: Add default binder devices through binderfs when configured

[Christian Brauner]

On Wed, Aug 07, 2019 at 02:02:05PM +0300, Dan Carpenter wrote:
> On Tue, Aug 06, 2019 at 11:40:05AM -0700, Hridya Valsaraju wrote:
> > @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> >  	int ret;
> >  	struct binderfs_info *info;
> >  	struct inode *inode = NULL;
> > +	struct binderfs_device device_info = { 0 };
> > +	const char *name;
> > +	size_t len;
> >  
> >  	sb->s_blocksize = PAGE_SIZE;
> >  	sb->s_blocksize_bits = PAGE_SHIFT;
> > @@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> >  	if (!sb->s_root)
> >  		return -ENOMEM;
> >  
> > -	return binderfs_binder_ctl_create(sb);
> > +	ret = binderfs_binder_ctl_create(sb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	name = binder_devices_param;
> > +	for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> > +		strscpy(device_info.name, name, len + 1);
> > +		ret = binderfs_binder_device_create(inode, NULL, &device_info);
> > +		if (ret)
> > +			return ret;
> 
> We should probably clean up before returning...  The error handling code
> would probably be tricky to write though and it's not super common.

struct dentry *mount_nodev(struct file_system_type *fs_type,
	int flags, void *data,
	int (*fill_super)(struct super_block *, void *, int))
{
	<snip>

	error = fill_super(s, data, flags & SB_SILENT ? 1 : 0);
	if (error) {
		deactivate_locked_super(s);
		return ERR_PTR(error);
	}

	<snip>
}

	deactivate_locked_super()
will call
	fs->kill_sb(s)
which calls
	binderfs_kill_super()
which calls
	kill_litter_super()
the latter will destory any remaining dentries and then calls
	generic_shutdown_super()
which calls
	evict_inodes()
which calls
	evict(inode)
which calls the binderfs specific
	binderfs_evict_inode()
and get rid of the rest.

So manually cleaning up is not needed, imho.

Christian

Re: [PATCH v2 1/2] binder: Add default binder devices through binderfs when configured

[Dan Carpenter]

On Wed, Aug 07, 2019 at 09:51:46AM -0700, Hridya Valsaraju wrote:
> On Wed, Aug 7, 2019 at 4:02 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Tue, Aug 06, 2019 at 11:40:05AM -0700, Hridya Valsaraju wrote:
> > > @@ -467,6 +466,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> > >       int ret;
> > >       struct binderfs_info *info;
> > >       struct inode *inode = NULL;
> > > +     struct binderfs_device device_info = { 0 };
> > > +     const char *name;
> > > +     size_t len;
> > >
> > >       sb->s_blocksize = PAGE_SIZE;
> > >       sb->s_blocksize_bits = PAGE_SHIFT;
> > > @@ -521,7 +523,24 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> > >       if (!sb->s_root)
> > >               return -ENOMEM;
> > >
> > > -     return binderfs_binder_ctl_create(sb);
> > > +     ret = binderfs_binder_ctl_create(sb);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     name = binder_devices_param;
> > > +     for (len = strcspn(name, ","); len > 0; len = strcspn(name, ",")) {
> > > +             strscpy(device_info.name, name, len + 1);
> > > +             ret = binderfs_binder_device_create(inode, NULL, &device_info);
> > > +             if (ret)
> > > +                     return ret;
> >
> > We should probably clean up before returning...  The error handling code
> > would probably be tricky to write though and it's not super common.
> 
> Thank you for taking a look Dan. Did you mean cleaning up the default
> devices that were already created? They will actually be cleaned up by
> binderfs_evict_inode() during the super block's cleanup since the
> mount operation will fail due to an error here.

Yeah.  I meant the binderfs_binder_device_create() from previous
iterations through this loop.

Good to know that it's handled.  Thanks for taking the time to look at
this.

regards,
dan carpenter