DriverDev-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 0/2] Add default binderfs devices
@ 2019-08-08 22:27 hridya
  2019-08-08 22:27 ` [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured hridya
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: hridya @ 2019-08-08 22:27 UTC (permalink / raw)


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        | 35 ++++++++++++++++++++++++++++---
 3 files changed, 37 insertions(+), 5 deletions(-)

-- 
2.22.0.770.g0f2c4a37fd-goog

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
  2019-08-08 22:27 [PATCH v3 0/2] Add default binderfs devices hridya
@ 2019-08-08 22:27 ` hridya
  2019-08-09 14:50   ` gregkh
                     ` (2 more replies)
  2019-08-08 22:27 ` [PATCH v3 2/2] binder: Validate the default binderfs device names hridya
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 22+ messages in thread
From: hridya @ 2019-08-08 22:27 UTC (permalink / raw)


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 at ubuntu.com>
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
Signed-off-by: Hridya Valsaraju <hridya at 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.

Changes in v3:
-Removed unnecessary empty lines as per Dan Carpenter.

 drivers/android/binder.c          |  5 +++--
 drivers/android/binder_internal.h |  2 ++
 drivers/android/binderfs.c        | 23 ++++++++++++++++++++---
 3 files changed, 25 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..aee46dd1be91 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,22 @@ 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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-08-08 22:27 [PATCH v3 0/2] Add default binderfs devices hridya
  2019-08-08 22:27 ` [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured hridya
@ 2019-08-08 22:27 ` hridya
  2019-08-09 14:51   ` gregkh
                     ` (2 more replies)
  2019-08-15 16:00 ` [PATCH v3 0/2] Add default binderfs devices gregkh
  2019-09-04 11:07 ` [RESEND PATCH " Christian Brauner
  3 siblings, 3 replies; 22+ messages in thread
From: hridya @ 2019-08-08 22:27 UTC (permalink / raw)


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 at ubuntu.com>
Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
Signed-off-by: Hridya Valsaraju <hridya at 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 aee46dd1be91..55c5adb87585 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -570,6 +570,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

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
  2019-08-08 22:27 ` [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured hridya
@ 2019-08-09 14:50   ` gregkh
  2019-08-09 18:22     ` christian.brauner
  2019-08-15 16:28   ` joel
  2019-08-15 16:30   ` joel
  2 siblings, 1 reply; 22+ messages in thread
From: gregkh @ 2019-08-09 14:50 UTC (permalink / raw)


On Thu, Aug 08, 2019@03:27:25PM -0700, Hridya Valsaraju wrote:
> 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.

Wasn't that a design goal of binderfs?

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

This is going to change how things work today, what is going to break
because of this change?

I don't object to this, except for the worry of changing the default
behavior.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-08-08 22:27 ` [PATCH v3 2/2] binder: Validate the default binderfs device names hridya
@ 2019-08-09 14:51   ` gregkh
  2019-08-09 14:55   ` gregkh
  2019-08-15 16:31   ` joel
  2 siblings, 0 replies; 22+ messages in thread
From: gregkh @ 2019-08-09 14:51 UTC (permalink / raw)


On Thu, Aug 08, 2019@03:27:26PM -0700, Hridya Valsaraju wrote:
> 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 at ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya at 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 aee46dd1be91..55c5adb87585 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -570,6 +570,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++;
> +	}

Shouldn't this be a bugfix separate from patch 1/2?

And shouldn't it be backported to older kernels that currently have this
issue?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-08-08 22:27 ` [PATCH v3 2/2] binder: Validate the default binderfs device names hridya
  2019-08-09 14:51   ` gregkh
@ 2019-08-09 14:55   ` gregkh
  2019-08-09 18:14     ` christian.brauner
  2019-08-15 16:31   ` joel
  2 siblings, 1 reply; 22+ messages in thread
From: gregkh @ 2019-08-09 14:55 UTC (permalink / raw)


On Thu, Aug 08, 2019@03:27:26PM -0700, Hridya Valsaraju wrote:
> 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 at ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya at 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 aee46dd1be91..55c5adb87585 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -570,6 +570,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. */

And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?

> +	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++;
> +	}

We already tokenize the binderfs device names in binder_init(), why not
check this there instead?  Parsing the same string over and over isn't
the nicest.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-08-09 14:55   ` gregkh
@ 2019-08-09 18:14     ` christian.brauner
  2019-08-09 18:41       ` hridya
  0 siblings, 1 reply; 22+ messages in thread
From: christian.brauner @ 2019-08-09 18:14 UTC (permalink / raw)


On Fri, Aug 09, 2019@04:55:08PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 08, 2019@03:27:26PM -0700, Hridya Valsaraju wrote:
> > 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 at ubuntu.com>
> > Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> > Signed-off-by: Hridya Valsaraju <hridya at 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 aee46dd1be91..55c5adb87585 100644
> > --- a/drivers/android/binderfs.c
> > +++ b/drivers/android/binderfs.c
> > @@ -570,6 +570,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. */
> 
> And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?
> 
> > +	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++;
> > +	}
> 
> We already tokenize the binderfs device names in binder_init(), why not
> check this there instead?  Parsing the same string over and over isn't
> the nicest.

non-binderfs binder devices do not have their limit set to
BINDERFS_NAME_MAX. That's why the check has likely been made specific to
binderfs binder devices which do have that limit.

But, in practice, 255 is the standard path-part limit that no-one really
exceeds especially not for stuff such as device nodes which usually have
rather standard naming schemes (e.g. binder, vndbinder, hwbinder, etc.).
So yes, we can move that check before both the binderfs binder device
and non-binderfs binder device parsing code and treat it as a generic
check.
Then we can also backport that check as you requested in the other mail.
Unless Hridya or Todd have objections, of course.

Christian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
  2019-08-09 14:50   ` gregkh
@ 2019-08-09 18:22     ` christian.brauner
  2019-08-09 20:08       ` hridya
  0 siblings, 1 reply; 22+ messages in thread
From: christian.brauner @ 2019-08-09 18:22 UTC (permalink / raw)


On Fri, Aug 09, 2019@04:50:16PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Aug 08, 2019@03:27:25PM -0700, Hridya Valsaraju wrote:
> > 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.
> 
> Wasn't that a design goal of binderfs?

Sure, but if you solely rely binderfs to be used to provide binder
devices having them pre-created on each mount makes quite some sense,
imho.

> 
> > 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.
> 
> This is going to change how things work today, what is going to break
> because of this change?
> 
> I don't object to this, except for the worry of changing the default
> behavior.

This is something that Hridya and Todd can speak better to given that
they suggested this change. :)
>From my perspective, binderfs binder devices and the regular binder
driver are mostly used mutually exclusive in practice atm so that this
change has little chance of breaking anyone.

Now I really need to go back to vacation time - which I suck at
apparently. :)

Christian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-08-09 18:14     ` christian.brauner
@ 2019-08-09 18:41       ` hridya
  2019-09-04  7:19         ` Greg Kroah-Hartman
  0 siblings, 1 reply; 22+ messages in thread
From: hridya @ 2019-08-09 18:41 UTC (permalink / raw)


On Fri, Aug 9, 2019 at 11:14 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Fri, Aug 09, 2019@04:55:08PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 08, 2019@03:27:26PM -0700, Hridya Valsaraju wrote:
> > > 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 at ubuntu.com>
> > > Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> > > Signed-off-by: Hridya Valsaraju <hridya at 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 aee46dd1be91..55c5adb87585 100644
> > > --- a/drivers/android/binderfs.c
> > > +++ b/drivers/android/binderfs.c
> > > @@ -570,6 +570,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. */
> >
> > And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?
> >
> > > +   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++;
> > > +   }
> >
> > We already tokenize the binderfs device names in binder_init(), why not
> > check this there instead?  Parsing the same string over and over isn't
> > the nicest.
>
> non-binderfs binder devices do not have their limit set to
> BINDERFS_NAME_MAX. That's why the check has likely been made specific to
> binderfs binder devices which do have that limit.


Thank you Greg and Christian, for taking another look. Yes,
non-binderfs binder devices not having this limitation is the reason
why the check was made specific to binderfs devices. Also, when
CONFIG_ANDROID_BINDERFS is set, patch 1/2 disabled the same string
being parsed in binder_init().

>
> But, in practice, 255 is the standard path-part limit that no-one really
> exceeds especially not for stuff such as device nodes which usually have
> rather standard naming schemes (e.g. binder, vndbinder, hwbinder, etc.).
> So yes, we can move that check before both the binderfs binder device
> and non-binderfs binder device parsing code and treat it as a generic
> check.
> Then we can also backport that check as you requested in the other mail.
> Unless Hridya or Todd have objections, of course.

I do not have any objections to adding a generic check in binder_init() instead.

>
> Christian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
  2019-08-09 18:22     ` christian.brauner
@ 2019-08-09 20:08       ` hridya
  0 siblings, 0 replies; 22+ messages in thread
From: hridya @ 2019-08-09 20:08 UTC (permalink / raw)


On Fri, Aug 9, 2019 at 11:22 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Fri, Aug 09, 2019@04:50:16PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Aug 08, 2019@03:27:25PM -0700, Hridya Valsaraju wrote:
> > > 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.
> >
> > Wasn't that a design goal of binderfs?
>
> Sure, but if you solely rely binderfs to be used to provide binder
> devices having them pre-created on each mount makes quite some sense,
> imho.
>
> >
> > > 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.
> >
> > This is going to change how things work today, what is going to break
> > because of this change?
> >
> > I don't object to this, except for the worry of changing the default
> > behavior.
>
> This is something that Hridya and Todd can speak better to given that
> they suggested this change. :)
> From my perspective, binderfs binder devices and the regular binder
> driver are mostly used mutually exclusive in practice atm so that this
> change has little chance of breaking anyone.

As Christian says, we do not anticipate the change to break any
existing use cases since binderfs binder devices and regular binder
devices are generally not used simultaneously. Hopefully, there are
not a lot of unusual use cases since binderfs itself is relatively new
as well :)


>
> Now I really need to go back to vacation time - which I suck at
> apparently. :)
>
> Christian

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 0/2] Add default binderfs devices
  2019-08-08 22:27 [PATCH v3 0/2] Add default binderfs devices hridya
  2019-08-08 22:27 ` [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured hridya
  2019-08-08 22:27 ` [PATCH v3 2/2] binder: Validate the default binderfs device names hridya
@ 2019-08-15 16:00 ` gregkh
  2019-09-04 11:07 ` [RESEND PATCH " Christian Brauner
  3 siblings, 0 replies; 22+ messages in thread
From: gregkh @ 2019-08-15 16:00 UTC (permalink / raw)


On Thu, Aug 08, 2019@03:27:24PM -0700, Hridya Valsaraju wrote:
> 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.

I'd like to get a reviewed-by from the other binder maintainers before
taking this series....

{hint}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
  2019-08-08 22:27 ` [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured hridya
  2019-08-09 14:50   ` gregkh
@ 2019-08-15 16:28   ` joel
  2019-08-15 16:30   ` joel
  2 siblings, 0 replies; 22+ messages in thread
From: joel @ 2019-08-15 16:28 UTC (permalink / raw)


On Thu, Aug 08, 2019@03:27:25PM -0700, Hridya Valsaraju wrote:
> 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 at ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya at google.com>

Reviewed-by: Joel Fernandes (Google) <joel at joelfernandes.org>

thanks,

 - Joel

> ---
> 
> 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.
> 
> Changes in v3:
> -Removed unnecessary empty lines as per Dan Carpenter.
> 
>  drivers/android/binder.c          |  5 +++--
>  drivers/android/binder_internal.h |  2 ++
>  drivers/android/binderfs.c        | 23 ++++++++++++++++++++---
>  3 files changed, 25 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..aee46dd1be91 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,22 @@ 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
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
  2019-08-08 22:27 ` [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured hridya
  2019-08-09 14:50   ` gregkh
  2019-08-15 16:28   ` joel
@ 2019-08-15 16:30   ` joel
  2 siblings, 0 replies; 22+ messages in thread
From: joel @ 2019-08-15 16:30 UTC (permalink / raw)


On Thu, Aug 08, 2019@03:27:25PM -0700, Hridya Valsaraju wrote:
> 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 at ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya at google.com>
> ---

Reviewed-by: Joel Fernandes (Google) <joel at joelfernandes.org>

thanks,

 - Joel

> 
> 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.
> 
> Changes in v3:
> -Removed unnecessary empty lines as per Dan Carpenter.
> 
>  drivers/android/binder.c          |  5 +++--
>  drivers/android/binder_internal.h |  2 ++
>  drivers/android/binderfs.c        | 23 ++++++++++++++++++++---
>  3 files changed, 25 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..aee46dd1be91 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,22 @@ 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
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-08-08 22:27 ` [PATCH v3 2/2] binder: Validate the default binderfs device names hridya
  2019-08-09 14:51   ` gregkh
  2019-08-09 14:55   ` gregkh
@ 2019-08-15 16:31   ` joel
  2 siblings, 0 replies; 22+ messages in thread
From: joel @ 2019-08-15 16:31 UTC (permalink / raw)


On Thu, Aug 08, 2019@03:27:26PM -0700, Hridya Valsaraju wrote:
> 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 at ubuntu.com>
> Signed-off-by: Christian Brauner <christian.brauner at ubuntu.com>
> Signed-off-by: Hridya Valsaraju <hridya at google.com>
> ---

Reviewed-by: Joel Fernandes (Google) <joel at joelfernandes.org>

thanks,

 - Joel

>  drivers/android/binderfs.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index aee46dd1be91..55c5adb87585 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -570,6 +570,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
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-08-09 18:41       ` hridya
@ 2019-09-04  7:19         ` Greg Kroah-Hartman
  2019-09-04 10:44           ` Christian Brauner
  0 siblings, 1 reply; 22+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-04  7:19 UTC (permalink / raw)
  To: Hridya Valsaraju
  Cc: devel, Todd Kjos, Martijn Coenen, linux-kernel,
	Arve Hjønnevåg, Joel Fernandes, Christian Brauner,
	kernel-team

On Fri, Aug 09, 2019 at 11:41:12AM -0700, Hridya Valsaraju wrote:
> On Fri, Aug 9, 2019 at 11:14 AM Christian Brauner
> <christian.brauner@ubuntu.com> wrote:
> >
> > On Fri, Aug 09, 2019 at 04:55:08PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Aug 08, 2019 at 03:27:26PM -0700, Hridya Valsaraju wrote:
> > > > 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 aee46dd1be91..55c5adb87585 100644
> > > > --- a/drivers/android/binderfs.c
> > > > +++ b/drivers/android/binderfs.c
> > > > @@ -570,6 +570,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. */
> > >
> > > And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?
> > >
> > > > +   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++;
> > > > +   }
> > >
> > > We already tokenize the binderfs device names in binder_init(), why not
> > > check this there instead?  Parsing the same string over and over isn't
> > > the nicest.
> >
> > non-binderfs binder devices do not have their limit set to
> > BINDERFS_NAME_MAX. That's why the check has likely been made specific to
> > binderfs binder devices which do have that limit.
> 
> 
> Thank you Greg and Christian, for taking another look. Yes,
> non-binderfs binder devices not having this limitation is the reason
> why the check was made specific to binderfs devices. Also, when
> CONFIG_ANDROID_BINDERFS is set, patch 1/2 disabled the same string
> being parsed in binder_init().
> 
> >
> > But, in practice, 255 is the standard path-part limit that no-one really
> > exceeds especially not for stuff such as device nodes which usually have
> > rather standard naming schemes (e.g. binder, vndbinder, hwbinder, etc.).
> > So yes, we can move that check before both the binderfs binder device
> > and non-binderfs binder device parsing code and treat it as a generic
> > check.
> > Then we can also backport that check as you requested in the other mail.
> > Unless Hridya or Todd have objections, of course.
> 
> I do not have any objections to adding a generic check in binder_init() instead.

Was this patchset going to be redone based on this?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-09-04  7:19         ` Greg Kroah-Hartman
@ 2019-09-04 10:44           ` Christian Brauner
  2019-09-04 10:49             ` Greg Kroah-Hartman
  0 siblings, 1 reply; 22+ messages in thread
From: Christian Brauner @ 2019-09-04 10:44 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: devel, Todd Kjos, Martijn Coenen, linux-kernel, Joel Fernandes,
	Arve Hjønnevåg, Hridya Valsaraju, kernel-team

On Wed, Sep 04, 2019 at 09:19:29AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 09, 2019 at 11:41:12AM -0700, Hridya Valsaraju wrote:
> > On Fri, Aug 9, 2019 at 11:14 AM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> > >
> > > On Fri, Aug 09, 2019 at 04:55:08PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Aug 08, 2019 at 03:27:26PM -0700, Hridya Valsaraju wrote:
> > > > > 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 aee46dd1be91..55c5adb87585 100644
> > > > > --- a/drivers/android/binderfs.c
> > > > > +++ b/drivers/android/binderfs.c
> > > > > @@ -570,6 +570,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. */
> > > >
> > > > And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?
> > > >
> > > > > +   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++;
> > > > > +   }
> > > >
> > > > We already tokenize the binderfs device names in binder_init(), why not
> > > > check this there instead?  Parsing the same string over and over isn't
> > > > the nicest.
> > >
> > > non-binderfs binder devices do not have their limit set to
> > > BINDERFS_NAME_MAX. That's why the check has likely been made specific to
> > > binderfs binder devices which do have that limit.
> > 
> > 
> > Thank you Greg and Christian, for taking another look. Yes,
> > non-binderfs binder devices not having this limitation is the reason
> > why the check was made specific to binderfs devices. Also, when
> > CONFIG_ANDROID_BINDERFS is set, patch 1/2 disabled the same string
> > being parsed in binder_init().
> > 
> > >
> > > But, in practice, 255 is the standard path-part limit that no-one really
> > > exceeds especially not for stuff such as device nodes which usually have
> > > rather standard naming schemes (e.g. binder, vndbinder, hwbinder, etc.).
> > > So yes, we can move that check before both the binderfs binder device
> > > and non-binderfs binder device parsing code and treat it as a generic
> > > check.
> > > Then we can also backport that check as you requested in the other mail.
> > > Unless Hridya or Todd have objections, of course.
> > 
> > I do not have any objections to adding a generic check in binder_init() instead.
> 
> Was this patchset going to be redone based on this?

No, we decided to leave this check specific to binderfs for now because
the length limit only applies to binderfs devices. If you really want to
have this check in binder we can send a follow-up. I would prefer to
take the series as is.

Btw, for the two binderfs series from Hridya, do you want me to get a
branch ready and send you a PR for both of them together?

Christian
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-09-04 10:44           ` Christian Brauner
@ 2019-09-04 10:49             ` Greg Kroah-Hartman
  2019-09-04 11:07               ` Christian Brauner
  0 siblings, 1 reply; 22+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-04 10:49 UTC (permalink / raw)
  To: Christian Brauner
  Cc: devel, Todd Kjos, kernel-team, linux-kernel, Hridya Valsaraju,
	Arve Hjønnevåg, Joel Fernandes, Martijn Coenen

On Wed, Sep 04, 2019 at 12:44:32PM +0200, Christian Brauner wrote:
> On Wed, Sep 04, 2019 at 09:19:29AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Aug 09, 2019 at 11:41:12AM -0700, Hridya Valsaraju wrote:
> > > On Fri, Aug 9, 2019 at 11:14 AM Christian Brauner
> > > <christian.brauner@ubuntu.com> wrote:
> > > >
> > > > On Fri, Aug 09, 2019 at 04:55:08PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Thu, Aug 08, 2019 at 03:27:26PM -0700, Hridya Valsaraju wrote:
> > > > > > 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 aee46dd1be91..55c5adb87585 100644
> > > > > > --- a/drivers/android/binderfs.c
> > > > > > +++ b/drivers/android/binderfs.c
> > > > > > @@ -570,6 +570,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. */
> > > > >
> > > > > And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?
> > > > >
> > > > > > +   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++;
> > > > > > +   }
> > > > >
> > > > > We already tokenize the binderfs device names in binder_init(), why not
> > > > > check this there instead?  Parsing the same string over and over isn't
> > > > > the nicest.
> > > >
> > > > non-binderfs binder devices do not have their limit set to
> > > > BINDERFS_NAME_MAX. That's why the check has likely been made specific to
> > > > binderfs binder devices which do have that limit.
> > > 
> > > 
> > > Thank you Greg and Christian, for taking another look. Yes,
> > > non-binderfs binder devices not having this limitation is the reason
> > > why the check was made specific to binderfs devices. Also, when
> > > CONFIG_ANDROID_BINDERFS is set, patch 1/2 disabled the same string
> > > being parsed in binder_init().
> > > 
> > > >
> > > > But, in practice, 255 is the standard path-part limit that no-one really
> > > > exceeds especially not for stuff such as device nodes which usually have
> > > > rather standard naming schemes (e.g. binder, vndbinder, hwbinder, etc.).
> > > > So yes, we can move that check before both the binderfs binder device
> > > > and non-binderfs binder device parsing code and treat it as a generic
> > > > check.
> > > > Then we can also backport that check as you requested in the other mail.
> > > > Unless Hridya or Todd have objections, of course.
> > > 
> > > I do not have any objections to adding a generic check in binder_init() instead.
> > 
> > Was this patchset going to be redone based on this?
> 
> No, we decided to leave this check specific to binderfs for now because
> the length limit only applies to binderfs devices. If you really want to
> have this check in binder we can send a follow-up. I would prefer to
> take the series as is.
> 
> Btw, for the two binderfs series from Hridya, do you want me to get a
> branch ready and send you a PR for both of them together?

Patches in email is fine, but can someone resend this one as I no longer
have this series in my queue anymore?

thanks,

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RESEND PATCH v3 0/2] Add default binderfs devices
  2019-08-08 22:27 [PATCH v3 0/2] Add default binderfs devices hridya
                   ` (2 preceding siblings ...)
  2019-08-15 16:00 ` [PATCH v3 0/2] Add default binderfs devices gregkh
@ 2019-09-04 11:07 ` " Christian Brauner
  2019-09-04 11:07   ` [RESEND PATCH v3 1/2] binder: Add default binder devices through binderfs when configured Christian Brauner
                     ` (2 more replies)
  3 siblings, 3 replies; 22+ messages in thread
From: Christian Brauner @ 2019-09-04 11:07 UTC (permalink / raw)
  To: hridya
  Cc: devel, tkjos, gregkh, linux-kernel, arve, maco, joel,
	Christian Brauner, kernel-team, christian

Hey,

This is a resend of Hridya's series to add default binderfs devices. No
semantical changes were made. Only Joel's Acks were added by me.

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.

/* v0 */
Link: https://lore.kernel.org/lkml/20190801223556.209184-1-hridya@google.com

/* v1 */
Link: https://lore.kernel.org/lkml/20190806184007.60739-1-hridya@google.com

/* v2 */
Link: https://lore.kernel.org/lkml/20190808222727.132744-1-hridya@google.com/

Thanks!
Christian

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        | 35 ++++++++++++++++++++++++++++---
 3 files changed, 37 insertions(+), 5 deletions(-)

-- 
2.23.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RESEND PATCH v3 1/2] binder: Add default binder devices through binderfs when configured
  2019-09-04 11:07 ` [RESEND PATCH " Christian Brauner
@ 2019-09-04 11:07   ` Christian Brauner
  2019-09-04 11:07   ` [RESEND PATCH v3 2/2] binder: Validate the default binderfs device names Christian Brauner
  2019-09-04 11:18   ` [RESEND PATCH v3 0/2] Add default binderfs devices Greg KH
  2 siblings, 0 replies; 22+ messages in thread
From: Christian Brauner @ 2019-09-04 11:07 UTC (permalink / raw)
  To: hridya
  Cc: devel, tkjos, gregkh, linux-kernel, arve, maco, joel,
	Christian Brauner, kernel-team, christian

From: Hridya Valsaraju <hridya@google.com>

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>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Link: https://lore.kernel.org/r/20190808222727.132744-2-hridya@google.com
---
 drivers/android/binder.c          |  5 +++--
 drivers/android/binder_internal.h |  2 ++
 drivers/android/binderfs.c        | 23 ++++++++++++++++++++---
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index dc1c83eafc22..ef2d3e582368 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -122,7 +122,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);
@@ -6131,7 +6131,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..aee46dd1be91 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,22 @@ 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.23.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [RESEND PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-09-04 11:07 ` [RESEND PATCH " Christian Brauner
  2019-09-04 11:07   ` [RESEND PATCH v3 1/2] binder: Add default binder devices through binderfs when configured Christian Brauner
@ 2019-09-04 11:07   ` Christian Brauner
  2019-09-04 11:18   ` [RESEND PATCH v3 0/2] Add default binderfs devices Greg KH
  2 siblings, 0 replies; 22+ messages in thread
From: Christian Brauner @ 2019-09-04 11:07 UTC (permalink / raw)
  To: hridya
  Cc: devel, tkjos, gregkh, linux-kernel, arve, maco, joel,
	Christian Brauner, kernel-team, christian

From: Hridya Valsaraju <hridya@google.com>

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>
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
Link: https://lore.kernel.org/r/20190808222727.132744-3-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 aee46dd1be91..55c5adb87585 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -570,6 +570,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.23.0

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 2/2] binder: Validate the default binderfs device names.
  2019-09-04 10:49             ` Greg Kroah-Hartman
@ 2019-09-04 11:07               ` Christian Brauner
  0 siblings, 0 replies; 22+ messages in thread
From: Christian Brauner @ 2019-09-04 11:07 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: devel, Todd Kjos, kernel-team, linux-kernel, Hridya Valsaraju,
	Arve Hjønnevåg, Joel Fernandes, Martijn Coenen

On Wed, Sep 04, 2019 at 12:49:39PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Sep 04, 2019 at 12:44:32PM +0200, Christian Brauner wrote:
> > On Wed, Sep 04, 2019 at 09:19:29AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Aug 09, 2019 at 11:41:12AM -0700, Hridya Valsaraju wrote:
> > > > On Fri, Aug 9, 2019 at 11:14 AM Christian Brauner
> > > > <christian.brauner@ubuntu.com> wrote:
> > > > >
> > > > > On Fri, Aug 09, 2019 at 04:55:08PM +0200, Greg Kroah-Hartman wrote:
> > > > > > On Thu, Aug 08, 2019 at 03:27:26PM -0700, Hridya Valsaraju wrote:
> > > > > > > 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 aee46dd1be91..55c5adb87585 100644
> > > > > > > --- a/drivers/android/binderfs.c
> > > > > > > +++ b/drivers/android/binderfs.c
> > > > > > > @@ -570,6 +570,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. */
> > > > > >
> > > > > > And by "valid" you only mean "not bigger than BINDERFS_MAX_NAME, right?
> > > > > >
> > > > > > > +   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++;
> > > > > > > +   }
> > > > > >
> > > > > > We already tokenize the binderfs device names in binder_init(), why not
> > > > > > check this there instead?  Parsing the same string over and over isn't
> > > > > > the nicest.
> > > > >
> > > > > non-binderfs binder devices do not have their limit set to
> > > > > BINDERFS_NAME_MAX. That's why the check has likely been made specific to
> > > > > binderfs binder devices which do have that limit.
> > > > 
> > > > 
> > > > Thank you Greg and Christian, for taking another look. Yes,
> > > > non-binderfs binder devices not having this limitation is the reason
> > > > why the check was made specific to binderfs devices. Also, when
> > > > CONFIG_ANDROID_BINDERFS is set, patch 1/2 disabled the same string
> > > > being parsed in binder_init().
> > > > 
> > > > >
> > > > > But, in practice, 255 is the standard path-part limit that no-one really
> > > > > exceeds especially not for stuff such as device nodes which usually have
> > > > > rather standard naming schemes (e.g. binder, vndbinder, hwbinder, etc.).
> > > > > So yes, we can move that check before both the binderfs binder device
> > > > > and non-binderfs binder device parsing code and treat it as a generic
> > > > > check.
> > > > > Then we can also backport that check as you requested in the other mail.
> > > > > Unless Hridya or Todd have objections, of course.
> > > > 
> > > > I do not have any objections to adding a generic check in binder_init() instead.
> > > 
> > > Was this patchset going to be redone based on this?
> > 
> > No, we decided to leave this check specific to binderfs for now because
> > the length limit only applies to binderfs devices. If you really want to
> > have this check in binder we can send a follow-up. I would prefer to
> > take the series as is.
> > 
> > Btw, for the two binderfs series from Hridya, do you want me to get a
> > branch ready and send you a PR for both of them together?
> 
> Patches in email is fine, but can someone resend this one as I no longer
> have this series in my queue anymore?

Done!
Christian
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [RESEND PATCH v3 0/2] Add default binderfs devices
  2019-09-04 11:07 ` [RESEND PATCH " Christian Brauner
  2019-09-04 11:07   ` [RESEND PATCH v3 1/2] binder: Add default binder devices through binderfs when configured Christian Brauner
  2019-09-04 11:07   ` [RESEND PATCH v3 2/2] binder: Validate the default binderfs device names Christian Brauner
@ 2019-09-04 11:18   ` Greg KH
  2 siblings, 0 replies; 22+ messages in thread
From: Greg KH @ 2019-09-04 11:18 UTC (permalink / raw)
  To: Christian Brauner
  Cc: devel, tkjos, kernel-team, linux-kernel, joel, arve, hridya,
	maco, christian

On Wed, Sep 04, 2019 at 01:07:02PM +0200, Christian Brauner wrote:
> Hey,
> 
> This is a resend of Hridya's series to add default binderfs devices. No
> semantical changes were made. Only Joel's Acks were added by me.
> 
> 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.

All now applied, thanks!

greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, back to index

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-08 22:27 [PATCH v3 0/2] Add default binderfs devices hridya
2019-08-08 22:27 ` [PATCH v3 1/2] binder: Add default binder devices through binderfs when configured hridya
2019-08-09 14:50   ` gregkh
2019-08-09 18:22     ` christian.brauner
2019-08-09 20:08       ` hridya
2019-08-15 16:28   ` joel
2019-08-15 16:30   ` joel
2019-08-08 22:27 ` [PATCH v3 2/2] binder: Validate the default binderfs device names hridya
2019-08-09 14:51   ` gregkh
2019-08-09 14:55   ` gregkh
2019-08-09 18:14     ` christian.brauner
2019-08-09 18:41       ` hridya
2019-09-04  7:19         ` Greg Kroah-Hartman
2019-09-04 10:44           ` Christian Brauner
2019-09-04 10:49             ` Greg Kroah-Hartman
2019-09-04 11:07               ` Christian Brauner
2019-08-15 16:31   ` joel
2019-08-15 16:00 ` [PATCH v3 0/2] Add default binderfs devices gregkh
2019-09-04 11:07 ` [RESEND PATCH " Christian Brauner
2019-09-04 11:07   ` [RESEND PATCH v3 1/2] binder: Add default binder devices through binderfs when configured Christian Brauner
2019-09-04 11:07   ` [RESEND PATCH v3 2/2] binder: Validate the default binderfs device names Christian Brauner
2019-09-04 11:18   ` [RESEND PATCH v3 0/2] Add default binderfs devices Greg KH

DriverDev-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/driverdev-devel/0 driverdev-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 driverdev-devel driverdev-devel/ https://lore.kernel.org/driverdev-devel \
		driverdev-devel@linuxdriverproject.org devel@driverdev.osuosl.org driverdev-devel@archiver.kernel.org
	public-inbox-index driverdev-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linuxdriverproject.driverdev-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox