Re: [PATCH v1 1/2] binderfs: implement "max" mount option

2019-01-02 Thread Christian Brauner 
On Wed, Jan 02, 2019 at 12:17:31PM +0300, Dan Carpenter wrote:
> On Sun, Dec 23, 2018 at 03:35:49PM +0100, Christian Brauner wrote:
> >  static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
> > @@ -110,10 +132,16 @@ static int binderfs_binder_device_create(struct inode 
> > *ref_inode,
> >  
> > /* Reserve new minor number for the new device. */
> > mutex_lock(_minors_mutex);
> > -   minor = ida_alloc_max(_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
> > +   if (++info->device_count <= info->mount_opts.max)
> > +   minor = ida_alloc_max(_minors, BINDERFS_MAX_MINOR,
> > + GFP_KERNEL);
> > +   else
> > +   minor = -ENOSPC;
> > mutex_unlock(_minors_mutex);
> > -   if (minor < 0)
> > +   if (minor < 0) {
> > +   --info->device_count;
> 
> Isn't this decrement supposed to happen under binderfs_minors_mutex?

Indeed. Good catch!
Leftover from when this was an atomic_t.

Thanks!
Christian

Re: [PATCH v1 1/2] binderfs: implement "max" mount option

2019-01-02 Thread Dan Carpenter 
On Sun, Dec 23, 2018 at 03:35:49PM +0100, Christian Brauner wrote:
>  static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
> @@ -110,10 +132,16 @@ static int binderfs_binder_device_create(struct inode 
> *ref_inode,
>  
>   /* Reserve new minor number for the new device. */
>   mutex_lock(_minors_mutex);
> - minor = ida_alloc_max(_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
> + if (++info->device_count <= info->mount_opts.max)
> + minor = ida_alloc_max(_minors, BINDERFS_MAX_MINOR,
> +   GFP_KERNEL);
> + else
> + minor = -ENOSPC;
>   mutex_unlock(_minors_mutex);
> - if (minor < 0)
> + if (minor < 0) {
> + --info->device_count;

Isn't this decrement supposed to happen under binderfs_minors_mutex?

>   return minor;
> + }


regards,
dan carpenter

Re: [PATCH v1 1/2] binderfs: implement "max" mount option

2018-12-24 Thread Christian Brauner 
On Mon, Dec 24, 2018 at 12:45:59PM +0100, Greg KH wrote:
> On Mon, Dec 24, 2018 at 12:09:37PM +0100, Christian Brauner wrote:
> > On Sun, Dec 23, 2018 at 03:35:49PM +0100, Christian Brauner wrote:
> > > Since binderfs can be mounted by userns root in non-initial user 
> > > namespaces
> > > some precautions are in order. First, a way to set a maximum on the number
> > > of binder devices that can be allocated per binderfs instance and second, 
> > > a
> > > way to reserve a reasonable chunk of binderfs devices for the initial ipc
> > > namespace.
> > > A first approach as seen in [1] used sysctls similiar to devpts but was
> > > shown to be flawed (cf. [2] and [3]) since some aspects were unneeded. 
> > > This
> > > is an alternative approach which avoids sysctls completely and instead
> > > switches to a single mount option.
> > > 
> > > Starting with this commit binderfs instances can be mounted with a limit 
> > > on
> > > the number of binder devices that can be allocated. The max= mount
> > > option serves as a per-instance limit. If max= is set then only
> > >  number of binder devices can be allocated in this binderfs
> > > instance.
> > > 
> > > This allows to safely bind-mount binderfs instances into unprivileged user
> > > namespaces since userns root in a non-initial user namespace cannot change
> > > the mount option as long as it does not own the mount namespace the
> > > binderfs mount was created in and hence cannot drain the host of minor
> > > device numbers
> > > 
> > > [1]: 
> > > https://lore.kernel.org/lkml/20181221133909.18794-1-christ...@brauner.io/
> > > [2]; https://lore.kernel.org/lkml/20181221163316.ga8...@kroah.com/
> > > [3]: 
> > > https://lore.kernel.org/lkml/cahrssex+gdvw4fkkk8oznair9g5icjlyodo8hykv3o0o1jt...@mail.gmail.com/
> > > [4]: 
> > > https://lore.kernel.org/lkml/20181221192044.5yvfnuri7gdop...@brauner.io/
> > > 
> > > Cc: Todd Kjos 
> > > Cc: Greg Kroah-Hartman 
> > > Signed-off-by: Christian Brauner 
> > 
> > Right, I forgot to ask. Do we still have time to land this alongside the
> > other patches in 4.21? :)
> 
> It's too late for 4.21-rc1, but let's see what happens after that :)

Sweet! Much appreciated. :)

Christian

Re: [PATCH v1 1/2] binderfs: implement "max" mount option

2018-12-24 Thread Greg KH 
On Mon, Dec 24, 2018 at 12:09:37PM +0100, Christian Brauner wrote:
> On Sun, Dec 23, 2018 at 03:35:49PM +0100, Christian Brauner wrote:
> > Since binderfs can be mounted by userns root in non-initial user namespaces
> > some precautions are in order. First, a way to set a maximum on the number
> > of binder devices that can be allocated per binderfs instance and second, a
> > way to reserve a reasonable chunk of binderfs devices for the initial ipc
> > namespace.
> > A first approach as seen in [1] used sysctls similiar to devpts but was
> > shown to be flawed (cf. [2] and [3]) since some aspects were unneeded. This
> > is an alternative approach which avoids sysctls completely and instead
> > switches to a single mount option.
> > 
> > Starting with this commit binderfs instances can be mounted with a limit on
> > the number of binder devices that can be allocated. The max= mount
> > option serves as a per-instance limit. If max= is set then only
> >  number of binder devices can be allocated in this binderfs
> > instance.
> > 
> > This allows to safely bind-mount binderfs instances into unprivileged user
> > namespaces since userns root in a non-initial user namespace cannot change
> > the mount option as long as it does not own the mount namespace the
> > binderfs mount was created in and hence cannot drain the host of minor
> > device numbers
> > 
> > [1]: 
> > https://lore.kernel.org/lkml/20181221133909.18794-1-christ...@brauner.io/
> > [2]; https://lore.kernel.org/lkml/20181221163316.ga8...@kroah.com/
> > [3]: 
> > https://lore.kernel.org/lkml/cahrssex+gdvw4fkkk8oznair9g5icjlyodo8hykv3o0o1jt...@mail.gmail.com/
> > [4]: 
> > https://lore.kernel.org/lkml/20181221192044.5yvfnuri7gdop...@brauner.io/
> > 
> > Cc: Todd Kjos 
> > Cc: Greg Kroah-Hartman 
> > Signed-off-by: Christian Brauner 
> 
> Right, I forgot to ask. Do we still have time to land this alongside the
> other patches in 4.21? :)

It's too late for 4.21-rc1, but let's see what happens after that :)

greg k-h

Re: [PATCH v1 1/2] binderfs: implement "max" mount option

2018-12-24 Thread Christian Brauner 
On Sun, Dec 23, 2018 at 03:35:49PM +0100, Christian Brauner wrote:
> Since binderfs can be mounted by userns root in non-initial user namespaces
> some precautions are in order. First, a way to set a maximum on the number
> of binder devices that can be allocated per binderfs instance and second, a
> way to reserve a reasonable chunk of binderfs devices for the initial ipc
> namespace.
> A first approach as seen in [1] used sysctls similiar to devpts but was
> shown to be flawed (cf. [2] and [3]) since some aspects were unneeded. This
> is an alternative approach which avoids sysctls completely and instead
> switches to a single mount option.
> 
> Starting with this commit binderfs instances can be mounted with a limit on
> the number of binder devices that can be allocated. The max= mount
> option serves as a per-instance limit. If max= is set then only
>  number of binder devices can be allocated in this binderfs
> instance.
> 
> This allows to safely bind-mount binderfs instances into unprivileged user
> namespaces since userns root in a non-initial user namespace cannot change
> the mount option as long as it does not own the mount namespace the
> binderfs mount was created in and hence cannot drain the host of minor
> device numbers
> 
> [1]: https://lore.kernel.org/lkml/20181221133909.18794-1-christ...@brauner.io/
> [2]; https://lore.kernel.org/lkml/20181221163316.ga8...@kroah.com/
> [3]: 
> https://lore.kernel.org/lkml/cahrssex+gdvw4fkkk8oznair9g5icjlyodo8hykv3o0o1jt...@mail.gmail.com/
> [4]: https://lore.kernel.org/lkml/20181221192044.5yvfnuri7gdop...@brauner.io/
> 
> Cc: Todd Kjos 
> Cc: Greg Kroah-Hartman 
> Signed-off-by: Christian Brauner 

Right, I forgot to ask. Do we still have time to land this alongside the
other patches in 4.21? :)

Christian

[PATCH v1 1/2] binderfs: implement "max" mount option

2018-12-23 Thread Christian Brauner 
Since binderfs can be mounted by userns root in non-initial user namespaces
some precautions are in order. First, a way to set a maximum on the number
of binder devices that can be allocated per binderfs instance and second, a
way to reserve a reasonable chunk of binderfs devices for the initial ipc
namespace.
A first approach as seen in [1] used sysctls similiar to devpts but was
shown to be flawed (cf. [2] and [3]) since some aspects were unneeded. This
is an alternative approach which avoids sysctls completely and instead
switches to a single mount option.

Starting with this commit binderfs instances can be mounted with a limit on
the number of binder devices that can be allocated. The max= mount
option serves as a per-instance limit. If max= is set then only
 number of binder devices can be allocated in this binderfs
instance.

This allows to safely bind-mount binderfs instances into unprivileged user
namespaces since userns root in a non-initial user namespace cannot change
the mount option as long as it does not own the mount namespace the
binderfs mount was created in and hence cannot drain the host of minor
device numbers

[1]: https://lore.kernel.org/lkml/20181221133909.18794-1-christ...@brauner.io/
[2]; https://lore.kernel.org/lkml/20181221163316.ga8...@kroah.com/
[3]: 
https://lore.kernel.org/lkml/cahrssex+gdvw4fkkk8oznair9g5icjlyodo8hykv3o0o1jt...@mail.gmail.com/
[4]: https://lore.kernel.org/lkml/20181221192044.5yvfnuri7gdop...@brauner.io/

Cc: Todd Kjos 
Cc: Greg Kroah-Hartman 
Signed-off-by: Christian Brauner 
---
v1:
- split reserving devices for the binderfs instance in the initial ipc
  namespace into a separate patch (cf. [1])
- s/
- use simple int instead of atomic_t for counting devices since we're
  incrementing and decrementing under a mutex anyway (cf. [1])
- correctly use match_int() (cf. [2])
v0:
- patch introduced

[1]: https://lore.kernel.org/lkml/20181223112944.gc27...@kroah.com/
[2]: https://lore.kernel.org/lkml/20181223135755.sqnv5ave62jtj...@brauner.io/
---
 drivers/android/binderfs.c | 101 +++--
 1 file changed, 96 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7496b10532aa..873adda064ac 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -46,6 +47,24 @@ static dev_t binderfs_dev;
 static DEFINE_MUTEX(binderfs_minors_mutex);
 static DEFINE_IDA(binderfs_minors);
 
+/**
+ * binderfs_mount_opts - mount options for binderfs
+ * @max: maximum number of allocatable binderfs binder devices
+ */
+struct binderfs_mount_opts {
+   int max;
+};
+
+enum {
+   Opt_max,
+   Opt_err
+};
+
+static const match_table_t tokens = {
+   { Opt_max, "max=%d" },
+   { Opt_err, NULL }
+};
+
 /**
  * binderfs_info - information about a binderfs mount
  * @ipc_ns: The ipc namespace the binderfs mount belongs to.
@@ -55,13 +74,16 @@ static DEFINE_IDA(binderfs_minors);
  *  created.
  * @root_gid:   gid that needs to be used when a new binder device is
  *  created.
+ * @mount_opts: The mount options in use.
+ * @device_count:   The current number of allocated binder devices.
  */
 struct binderfs_info {
struct ipc_namespace *ipc_ns;
struct dentry *control_dentry;
kuid_t root_uid;
kgid_t root_gid;
-
+   struct binderfs_mount_opts mount_opts;
+   int device_count;
 };
 
 static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
@@ -110,10 +132,16 @@ static int binderfs_binder_device_create(struct inode 
*ref_inode,
 
/* Reserve new minor number for the new device. */
mutex_lock(_minors_mutex);
-   minor = ida_alloc_max(_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
+   if (++info->device_count <= info->mount_opts.max)
+   minor = ida_alloc_max(_minors, BINDERFS_MAX_MINOR,
+ GFP_KERNEL);
+   else
+   minor = -ENOSPC;
mutex_unlock(_minors_mutex);
-   if (minor < 0)
+   if (minor < 0) {
+   --info->device_count;
return minor;
+   }
 
ret = -ENOMEM;
device = kzalloc(sizeof(*device), GFP_KERNEL);
@@ -187,6 +215,7 @@ static int binderfs_binder_device_create(struct inode 
*ref_inode,
kfree(name);
kfree(device);
mutex_lock(_minors_mutex);
+   --info->device_count;
ida_free(_minors, minor);
mutex_unlock(_minors_mutex);
iput(inode);
@@ -232,6 +261,7 @@ static long binder_ctl_ioctl(struct file *file, unsigned 
int cmd,
 static void binderfs_evict_inode(struct inode *inode)
 {
struct binder_device *device = inode->i_private;
+   struct binderfs_info *info = BINDERFS_I(inode);
 
clear_inode(inode);
 
@@ -239,6 +269,7 @@ static void binderfs_evict_inode(struct inode