Re: [PATCH] binderfs: implement sysctls

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Dec 21, 2018 at 02:39:09PM +0100, Christian Brauner wrote:
> This implements three sysctls that have very specific goals:

Ick, why?

What are these going to be used for?  Who will "control" them?  As you
are putting them in the "global" namespace, that feels like something
that binderfs was trying to avoid in the first place.

> 1. /proc/sys/fs/binder/max:
>    Allow global root to globally limit the number of allocatable binder
>    devices.

Why?  Who cares?  Memory should be your only limit here, and when you
run into that limit, you will start failing :)

> 2. /proc/sys/fs/binder/nr:
>    Allow global root to easily detect how many binder devices are currently
>    in use across all binderfs mounts.

Why?  Again, who cares?

> 3. /proc/sys/fs/binder/reserved:
>    Ensure that global root can reserve binder devices for the initial
>    binderfs mount in the initial ipc namespace to prevent DOS attacks.

Huh?  Can't you just create your "global root" devices first?  Doesn't
the code do that already anyway?

And what kind of DoS attack could this ever prevent from anyway?

> This is equivalent to sysctls of devpts.

devpts isn't exactly the best thing to emulate :)

> 
> Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
> ---
>  drivers/android/binderfs.c | 81 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 79 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 7496b10532aa..5ff015f82314 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -64,6 +64,71 @@ struct binderfs_info {
>  
>  };
>  
> +/* Global default limit on the number of binder devices. */
> +static int device_limit = 4096;
> +
> +/*
> + * Number of binder devices reserved for the initial binderfs mount in the
> + * initial ipc namespace.
> + */
> +static int device_reserve = 1024;
> +
> +/* Dummy sysctl minimum. */
> +static int device_limit_min;
> +
> +/* Cap sysctl at BINDERFS_MAX_MINOR. */
> +static int device_limit_max = BINDERFS_MAX_MINOR;
> +
> +/* Current number of allocated binder devices. */
> +static atomic_t device_count = ATOMIC_INIT(0);

You have a lock you are using, just rely on that, don't create
yet-another-type-of-unneeded-lock with an atomic here.

Anyway, I really don't see the need for any of this just yet, so I
didn't read beyond this point in the code :)

thanks,

greg k-h