Re: [PATCH V3] binder: ipc namespace support for android binder
On Thu, Nov 08, 2018 at 01:02:32PM +, chouryzhou(周威) wrote:
> We are working for running android in container, but we found that binder is
> not isolated by ipc namespace. Since binder is a form of IPC and therefore
> should
> be tied to ipc namespace. With this patch, we can run more than one android
> container on one host.
> This patch move "binder_procs" and "binder_context" into ipc_namespace,
> driver will find the context from it when opening. Although statistics in
> debugfs
> remain global.
>
> Signed-off-by: chouryzhou
> ---
> drivers/android/binder.c | 128
> +++---
> include/linux/ipc_namespace.h | 15 +
> ipc/namespace.c | 10 +++-
> 3 files changed, 117 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index cb30a524d16d..22e45bb937e6 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -68,6 +68,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -80,13 +81,18 @@
> #include "binder_alloc.h"
> #include "binder_trace.h"
>
> +
> +#if !defined(CONFIG_SYSVIPC) && !defined(CONFIG_POSIX_MQUEUE)
> +struct ipc_namespace init_ipc_ns;
> +#define ipcns (&init_ipc_ns)
> +#else
> +#define ipcns (current->nsproxy->ipc_ns)
> +#endif
> +
> static HLIST_HEAD(binder_deferred_list);
> static DEFINE_MUTEX(binder_deferred_lock);
>
> static HLIST_HEAD(binder_devices);
> -static HLIST_HEAD(binder_procs);
> -static DEFINE_MUTEX(binder_procs_lock);
> -
> static HLIST_HEAD(binder_dead_nodes);
> static DEFINE_SPINLOCK(binder_dead_nodes_lock);
>
> @@ -232,7 +238,7 @@ struct binder_transaction_log_entry {
> int return_error_line;
> uint32_t return_error;
> uint32_t return_error_param;
> - const char *context_name;
> + int context_device;
> };
> struct binder_transaction_log {
> atomic_t cur;
> @@ -263,19 +269,64 @@ static struct binder_transaction_log_entry
> *binder_transaction_log_add(
> }
>
> struct binder_context {
> + struct hlist_node hlist;
> struct binder_node *binder_context_mgr_node;
> struct mutex context_mgr_node_lock;
>
> kuid_t binder_context_mgr_uid;
> - const char *name;
> + intdevice;
> };
>
> struct binder_device {
> struct hlist_node hlist;
> struct miscdevice miscdev;
> - struct binder_context context;
> };
>
> +void binder_exit_ns(struct ipc_namespace *ns)
> +{
> + struct binder_context *context;
> + struct hlist_node *tmp;
> +
> + mutex_destroy(&ns->binder_procs_lock);
> + hlist_for_each_entry_safe(context, tmp, &ns->binder_contexts, hlist) {
> + mutex_destroy(&context->context_mgr_node_lock);
> + hlist_del(&context->hlist);
> + kfree(context);
> + }
> +}
> +
> +int binder_init_ns(struct ipc_namespace *ns)
> +{
> + int ret;
> + struct binder_device *device;
> +
> + mutex_init(&ns->binder_procs_lock);
> + INIT_HLIST_HEAD(&ns->binder_procs);
> + INIT_HLIST_HEAD(&ns->binder_contexts);
> +
> + hlist_for_each_entry(device, &binder_devices, hlist) {
> + struct binder_context *context;
> +
> + context = kzalloc(sizeof(*context), GFP_KERNEL);
> + if (!context) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + context->device = device->miscdev.minor;
> + context->binder_context_mgr_uid = INVALID_UID;
> + mutex_init(&context->context_mgr_node_lock);
> +
> + hlist_add_head(&context->hlist, &ns->binder_contexts);
> + }
> +
> + return 0;
> +err:
> + binder_exit_ns(ns);
> + return ret;
> +}
> +
> +
> /**
> * struct binder_work - work enqueued on a worklist
> * @entry: node enqueued on list
> @@ -2727,7 +2778,7 @@ static void binder_transaction(struct binder_proc *proc,
> e->target_handle = tr->target.handle;
> e->data_size = tr->data_size;
> e->offsets_size = tr->offsets_size;
> - e->context_name = proc->context->name;
> + e->context_device = proc->context->device;
>
> if (reply) {
> binder_inner_proc_lock(proc);
> @@ -4922,6 +4973,7 @@ static int binder_open(struct inode *nodp, struct file
> *filp)
> {
> struct binder_proc *proc;
> struct binder_device *binder_dev;
> + struct binder_context *context;
>
> binder_debug(BINDER_DEBUG_OPEN_CLOSE, "%s: %d:%d\n", __func__,
> current->group_leader->pid, current->pid);
> @@ -4937,7 +4989,15 @@ static int binder_open(struct inode *nodp, struct file
> *filp)
So the idea is that on open() of a binder device you attach an ipc
namespace to the opening process?
So you'd basically be able to create a new device node with th
Re: [PATCH V4] binder: ipc namespace support for android binder
On November 12, 2018 8:45:07 AM PST, Todd Kjos wrote:
>+christ...@brauner.io +Martijn Coenen
>
>Christian,
>
>Does this patch work for your container use-cases? If not, please
>comment on this thread. Let's discuss at LPC this week.
I have not received an answer to my questions in the last version of this patch
set. Also it would be good if I could be Cc'ed by default. I can't hunt down
all patches.
I do not know of any kernel entity, specifically devices, that change
namespaces on open().
This seems like an invitation for all kinds of security bugs.
A device node belongs to one namespace only which is attached to the underlying
kobject. Opening the device should never change that.
Please look at how mqueue or shm are doing this. They don't change namespaces
on open either.
I have to say that is one of the main reasons why I disagree with that design.
>
>-Todd
>
>On Mon, Nov 12, 2018 at 1:38 AM chouryzhou(周威)
>wrote:
>>
>> Currently android's binder is not isolated by ipc namespace. Since
>binder
>> is a form of IPC and therefore should be tied to ipc namespace. With
>this
>> patch, we can run multiple instances of android container on one
>host.
>>
>> This patch move "binder_procs" and "binder_context" into
>ipc_namespace,
>> driver will find the context from it when opening. For debugfs,
>binder_proc
>> is namespace-aware, but not for binder dead nodes, binder_stats and
>> binder_transaction_log_entry (we added ipc inum to trace it).
>>
>> Signed-off-by: chouryzhou
>> Reviewed-by: Davidlohr Bueso
>> ---
>> drivers/android/binder.c | 133
>--
>> include/linux/ipc_namespace.h | 15 +
>> ipc/namespace.c | 10 +++-
>> 3 files changed, 125 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index cb30a524d16d..453265505b04 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -67,6 +67,8 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> #include
>> #include
>> #include
>> @@ -80,13 +82,21 @@
>> #include "binder_alloc.h"
>> #include "binder_trace.h"
>>
>> +
>> +#ifndef CONFIG_IPC_NS
>> +static struct ipc_namespace binder_ipc_ns = {
>> + .ns.inum = PROC_IPC_INIT_INO,
>> +};
>> +
>> +#define ipcns (&binder_ipc_ns)
>> +#else
>> +#define ipcns (current->nsproxy->ipc_ns)
>> +#endif
>> +
>> static HLIST_HEAD(binder_deferred_list);
>> static DEFINE_MUTEX(binder_deferred_lock);
>>
>> static HLIST_HEAD(binder_devices);
>> -static HLIST_HEAD(binder_procs);
>> -static DEFINE_MUTEX(binder_procs_lock);
>> -
>> static HLIST_HEAD(binder_dead_nodes);
>> static DEFINE_SPINLOCK(binder_dead_nodes_lock);
>>
>> @@ -233,6 +243,7 @@ struct binder_transaction_log_entry {
>> uint32_t return_error;
>> uint32_t return_error_param;
>> const char *context_name;
>> + unsigned int ipc_inum;
>> };
>> struct binder_transaction_log {
>> atomic_t cur;
>> @@ -263,19 +274,66 @@ static struct binder_transaction_log_entry
>*binder_transaction_log_add(
>> }
>>
>> struct binder_context {
>> + struct hlist_node hlist;
>> struct binder_node *binder_context_mgr_node;
>> struct mutex context_mgr_node_lock;
>>
>> kuid_t binder_context_mgr_uid;
>> + int device;
>> const char *name;
>> };
>>
>> struct binder_device {
>> struct hlist_node hlist;
>> struct miscdevice miscdev;
>> - struct binder_context context;
>> };
>>
>> +void binder_exit_ns(struct ipc_namespace *ns)
>> +{
>> + struct binder_context *context;
>> + struct hlist_node *tmp;
>> +
>> + mutex_destroy(&ns->binder_procs_lock);
>> + hlist_for_each_entry_safe(context, tmp, &ns->binder_contexts,
>hlist) {
>> + mutex_destroy(&context->context_mgr_node_lock);
>> + hlist_del(&context->hlist);
>> + kfree(context);
>> + }
>> +}
>> +
>> +int binder_init_ns(struct ipc_namespace *ns)
>> +{
>> + int ret;
>> + struct binder_device *device;
>> +
>> + mutex_init(&ns->binder_procs_lock);
>> + INIT_HLIST_HEAD(&ns->binder_procs);
>> + INIT_HLIST_HEAD(&ns->binder_contexts);
>> +
>> + hlist_for_each_entry(device, &binder_devices, hlist) {
>> + struct binder_context *context;
>> +
>> + context = kzalloc(sizeof(*context), GFP_KERNEL);
>> + if (!context) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + context->device = device->miscdev.minor;
>> + context->name = device->miscdev.name;
>> + context->binder_context_mgr_uid = INVALID_UID;
>> + mutex_init(&context->context_mgr_node_lock);
>> +
>> + hlist_add_head(&context->hlist,
>&ns->binder_contexts);
>> + }
>> +
>> + return 0;
>> +err:
>> + binder_exit_ns(ns);
>>
Re: [PATCH V4] binder: ipc namespace support for android binder
On Fri, Nov 16, 2018 at 11:19:41AM -0800, Todd Kjos wrote: > On Thu, Nov 15, 2018 at 2:54 PM gre...@linuxfoundation.org > wrote: > ... > > > > A number of us have talked about this in the plumbers Android track, and > > a different proposal for how to solve this has been made that should be > > much more resiliant. So I will drop this patch from my queue and wait > > for the patches based on the discussions we had there. > > > > I think there's some notes/slides on the discussion online somewhere, > > but it hasn't been published as the conference is still happening, > > otherwise I would link to it here... > > Here is a link to the session where you can look at the slides [1]. > There was consensus that "binderfs" (slide 5) was the most promising > -- but would be behind a config option to provide backwards > compatibility for non-container use-cases. > > The etherpad notes are at [2] (look at "Dynamically Allocated Binder > Devices" section) > > Christian Brauner will be sending out more details. Ok, sorry for the delay I got caught up in other work. The idea is to implement binderfs which I'm starting to work on. binderfs will be a separate pseudo-filesystem that will be mountable per-ipc namespace. This has the advantage that the ipc namespace is attached to the superblock of the mount and can be easily retrieved by the binder driver. It also implies that - in contrast to the proposed patch here - an open() on a given binder device will not be able to change the ipc namespace of said devices. The obvious corollary is that you can bind-mount binder devices or the whole binderfs mount between different ipc namespaces and given the right permissions (CAP_IPC_OWNER in the owning userns of the ipcns) can see services on the host which is something that people wanted to be able to do. Additionally, each binderfs mount will come with a binder-control device. This device is functionally similar to loop-control and will allow for dynamic allocation (and possibly deallocation) of binder devices. With this we can remove the restriction to hard-code the number of binder devices at compile time. Backwards compatibility can either be guaranteed by hiding binderfs behind a compile flag or by special-casing the inital binderfs mount to pre-create the standard binder devices. The jury is still out on this. Christian > > -Todd > > [1] https://linuxplumbersconf.org/event/2/contributions/222/ > [2] https://etherpad.openstack.org/p/Android > > > > > thanks, > > > > greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] binder: implement binderfs
printf("Allocated new binder device with major %d, minor %d, and "
"name suffix %d\n", device.major, device.minor,
device.suffix);
exit(EXIT_SUCCESS);
}
/* Demo */
A demo of how binderfs works can be found under [2].
[1]: https://goo.gl/JL2tfX
[2]: https://asciinema.org/a/zYUCqL7OySASWK9S2yVFq2sxM
Cc: Martijn Coenen
Cc: Todd Kjos
Cc: Greg Kroah-Hartman
Signed-off-by: Christian Brauner
---
drivers/android/Kconfig | 12 +
drivers/android/Makefile| 1 +
drivers/android/binder.c| 25 +-
drivers/android/binder_internal.h | 52 +++
drivers/android/binderfs.c | 576
include/uapi/linux/android/binder_ctl.h | 33 ++
include/uapi/linux/magic.h | 1 +
7 files changed, 683 insertions(+), 17 deletions(-)
create mode 100644 drivers/android/binder_internal.h
create mode 100644 drivers/android/binderfs.c
create mode 100644 include/uapi/linux/android/binder_ctl.h
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
index 51e8250d113f..4c190f8d1f4c 100644
--- a/drivers/android/Kconfig
+++ b/drivers/android/Kconfig
@@ -20,6 +20,18 @@ config ANDROID_BINDER_IPC
Android process, using Binder to identify, invoke and pass arguments
between said processes.
+config ANDROID_BINDERFS
+ bool "Android Binderfs filesystem"
+ depends on ANDROID_BINDER_IPC
+ default n
+ ---help---
+ Binderfs is a pseudo-filesystem for the Android Binder IPC driver
+ which can be mounted per-ipc namespace allowing to run multiple
+ instances of Android.
+ Each binderfs mount initially only contains a binder-control device.
+ It can be used to dynamically allocate new binder IPC devices via
+ ioctls.
+
config ANDROID_BINDER_DEVICES
string "Android Binder devices"
depends on ANDROID_BINDER_IPC
diff --git a/drivers/android/Makefile b/drivers/android/Makefile
index a01254c43ee3..c7856e3200da 100644
--- a/drivers/android/Makefile
+++ b/drivers/android/Makefile
@@ -1,4 +1,5 @@
ccflags-y += -I$(src) # needed for trace events
+obj-$(CONFIG_ANDROID_BINDERFS) += binderfs.o
obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o binder_alloc.o
obj-$(CONFIG_ANDROID_BINDER_IPC_SELFTEST) += binder_alloc_selftest.o
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index cb30a524d16d..3ed8bc4b7451 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -78,6 +78,7 @@
#include
#include "binder_alloc.h"
+#include "binder_internal.h"
#include "binder_trace.h"
static HLIST_HEAD(binder_deferred_list);
@@ -262,20 +263,6 @@ static struct binder_transaction_log_entry
*binder_transaction_log_add(
return e;
}
-struct binder_context {
- struct binder_node *binder_context_mgr_node;
- struct mutex context_mgr_node_lock;
-
- kuid_t binder_context_mgr_uid;
- const char *name;
-};
-
-struct binder_device {
- struct hlist_node hlist;
- struct miscdevice miscdev;
- struct binder_context context;
-};
-
/**
* struct binder_work - work enqueued on a worklist
* @entry: node enqueued on list
@@ -4935,8 +4922,12 @@ static int binder_open(struct inode *nodp, struct file
*filp)
proc->tsk = current->group_leader;
INIT_LIST_HEAD(&proc->todo);
proc->default_priority = task_nice(current);
- binder_dev = container_of(filp->private_data, struct binder_device,
- miscdev);
+ /* binderfs stashes devices in i_private */
+ if (is_binderfs_device(nodp))
+ binder_dev = nodp->i_private;
+ else
+ binder_dev = container_of(filp->private_data,
+ struct binder_device, miscdev);
proc->context = &binder_dev->context;
binder_alloc_init(&proc->alloc);
@@ -5724,7 +5715,7 @@ static int binder_transaction_log_show(struct seq_file
*m, void *unused)
return 0;
}
-static const struct file_operations binder_fops = {
+const struct file_operations binder_fops = {
.owner = THIS_MODULE,
.poll = binder_poll,
.unlocked_ioctl = binder_ioctl,
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
new file mode 100644
index ..6879478196aa
--- /dev/null
+++ b/drivers/android/binder_internal.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_BINDER_INTERNAL_H
+#define _LINUX_BINDER_INTERNAL_H
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct binder_context {
+ struct binder_node *binder_context_mgr_node;
+ struct mutex context_mgr_node_lock;
+ kuid_t binder_context_mgr_uid;
+
Re: [PATCH] binder: implement binderfs
On Wed, Dec 05, 2018 at 09:01:45PM +0100, Greg KH wrote:
> On Tue, Dec 04, 2018 at 02:12:39PM +0100, Christian Brauner wrote:
> > As discussed at Linux Plumbers Conference 2018 in Vancouver [1] this is the
> > implementation of binderfs. If you want to skip reading and just see how it
> > works, please go to [2].
>
> First off, thanks for doing this so quickly. I think the overall idea
> and implementation is great, I just have some minor issues with the user
> api:
Thanks! :)
>
> > /* binder-control */
> > Each new binderfs instance comes with a binder-control device. No other
> > devices will be present at first. The binder-control device can be used to
> > dynamically allocate binder devices. All requests operate on the binderfs
> > mount the binder-control device resides in:
> > - BINDER_CTL_ADD
> > Allocate a new binder device.
> > Assuming a new instance of binderfs has been mounted at /dev/binderfs via
> > mount -t binderfs binderfs /dev/binderfs. Then a request to create a new
> > binder device can be made via:
> >
> > struct binderfs_device device = {0};
> > int fd = open("/dev/binderfs/binder-control", O_RDWR);
> > ioctl(fd, BINDER_CTL_ADD, &device);
> >
> > The struct binderfs_device will be used to return the major and minor
> > number, as well as the index used as the new name for the device.
> > Binderfs devices can simply be removed via unlink().
>
> I think you should provide a name in the BINDER_CTL_ADD command. That
> way you can easily emulate the existing binder queues, and it saves you
> a create/rename sequence that you will be forced to do otherwise. Why
> not do it just in a single command?
Sounds reasonable. How do you feel about capping the name length at 255
bytes aka the standard Linux file name length (e.g. xfs, ext4 etc.)?
#define BINDERFS_NAME_MAX 255
struct binderfs_device {
char name[BINDERFS_NAME_MAX + 1];
__u32 major;
__u32 minor;
}
>
> That way also you don't need to care about the major/minor number at
> all. Userspace should never need to worry about that, use a name,
> that's the best thing. Also, it allows you to drop the use of the idr,
> making the kernel code simpler overall.
>
> > /* Implementation details */
> > - When binderfs is registered as a new filesystem it will dynamically
> > allocate a new major number. The allocated major number will be returned
> > in struct binderfs_device when a new binder device is allocated.
>
> Why does userspace care about major/minor numbers at all? You should
Userspace cares for the sake of the devices cgroup which operates on
device numnbers to restrict access to devices. Since binderfs doesn't
have a static major number returning that information is helpful.
One example is a managining process sharing a binderfs mounts between
multiple ipc+mount namespaces via bind-mounts to avoid having separate
binderfs mounts. If the managing process only wants to grant access to binder0
and no other device for a given ipc+mnt namespace combination then it
can use the devices cgroup but for that it needs to know the major and
minor number.
> just be able to deal with the binder "names", that's all that userspace
> uses normally as you are not calling mknod() yourself.
>
> > Minor numbers that have been given out are tracked in a global idr struct
> > that is capped at BINDERFS_MAX_MINOR. The minor number tracker is
> > protected by a global mutex. This is the only point of contention between
> > binderfs mounts.
>
> I doubt this will be any real contention given that setting up / tearing
> down binder mounts is going to be rare, right? Well, hopefully, who
> knows with some container systems...
Yeah, it's very unlikely. Device allocation is a rare event that is
basically done once for most interesting use-cases. If one cares so much
about performance bind-mounts + device cgroup restrictions are possible
solution to this problem.
>
> > - The naming scheme for binder devices is binder%d. Each binderfs mount
> > starts numbering of new binder devices at 0 up to n. The indeces used in
> > constructing the name are tracked in a struct idr that is per-binderfs
> > super block.
>
> Again, let userspace pick the name, as you will have to rename it anyway
> to get userspace to work properly with it.
>
> I'll stop repeating myself now :)
Thanks for the feedback! :)
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: implement binderfs
On Thu, Dec 06, 2018 at 03:04:03PM +0100, Greg KH wrote:
> On Wed, Dec 05, 2018 at 10:42:06PM +0100, Christian Brauner wrote:
> > On Wed, Dec 05, 2018 at 09:01:45PM +0100, Greg KH wrote:
> > > > /* binder-control */
> > > > Each new binderfs instance comes with a binder-control device. No other
> > > > devices will be present at first. The binder-control device can be used
> > > > to
> > > > dynamically allocate binder devices. All requests operate on the
> > > > binderfs
> > > > mount the binder-control device resides in:
> > > > - BINDER_CTL_ADD
> > > > Allocate a new binder device.
> > > > Assuming a new instance of binderfs has been mounted at /dev/binderfs
> > > > via
> > > > mount -t binderfs binderfs /dev/binderfs. Then a request to create a new
> > > > binder device can be made via:
> > > >
> > > > struct binderfs_device device = {0};
> > > > int fd = open("/dev/binderfs/binder-control", O_RDWR);
> > > > ioctl(fd, BINDER_CTL_ADD, &device);
> > > >
> > > > The struct binderfs_device will be used to return the major and minor
> > > > number, as well as the index used as the new name for the device.
> > > > Binderfs devices can simply be removed via unlink().
> > >
> > > I think you should provide a name in the BINDER_CTL_ADD command. That
> > > way you can easily emulate the existing binder queues, and it saves you
> > > a create/rename sequence that you will be forced to do otherwise. Why
> > > not do it just in a single command?
> >
> > Sounds reasonable. How do you feel about capping the name length at 255
> > bytes aka the standard Linux file name length (e.g. xfs, ext4 etc.)?
> >
> > #define BINDERFS_NAME_MAX 255
> >
> > struct binderfs_device {
> > char name[BINDERFS_NAME_MAX + 1];
>
> __u8 is the proper type to cross the user/kernel boundry :)
Will switch. :)
>
> > __u32 major;
> > __u32 minor;
> > }
>
> Yes, limiting it to 255 is fine with me.
Perfect!
>
> > > That way also you don't need to care about the major/minor number at
> > > all. Userspace should never need to worry about that, use a name,
> > > that's the best thing. Also, it allows you to drop the use of the idr,
> > > making the kernel code simpler overall.
> > >
> > > > /* Implementation details */
> > > > - When binderfs is registered as a new filesystem it will dynamically
> > > > allocate a new major number. The allocated major number will be
> > > > returned
> > > > in struct binderfs_device when a new binder device is allocated.
> > >
> > > Why does userspace care about major/minor numbers at all? You should
> >
> > Userspace cares for the sake of the devices cgroup which operates on
> > device numnbers to restrict access to devices. Since binderfs doesn't
> > have a static major number returning that information is helpful.
>
> Ugh, ok, that makes sense. If we really want to make the kernel
> interface simpler, drop the major/minor and then have userspace do the
> stat(2) to see what the major/minor number they care about is.
>
> But yeah, keeping it here makes everyone's life simpler, the kernel
> already knows this, and it's trivial to pass it back to userspace this
> way.
>
> Care to make this change and resend?
For sure. I have a long-haul flight for ~15h so by the time I land I
have a new version I can send out. :)
Thanks!
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1] binder: implement binderfs
exit(EXIT_SUCCESS);
}
/* Demo */
A demo of how binderfs works can be found under [2].
[1]: https://goo.gl/JL2tfX
Cc: Martijn Coenen
Cc: Todd Kjos
Cc: Greg Kroah-Hartman
Signed-off-by: Christian Brauner
---
v1:
- simplify init_binderfs()
Move the creation of binder-control into binderfs_fill_super() so that we can
cleanly and without any complex error handling deallocate the super block on
failure.
- switch from __u32 to __u8 in struct binderfs_device
__u8 is the correct value to cross the kernel <-> userspace boundary.
- introduce BINDERFS_MAX_NAME
This determines the maximum length of a binderfs binder device name.
- add name member struct binderfs_device
This lets userspace specify a name for the binder device. The maximum length
is determined by BINDERFS_MAX_NAME.
- handle naming collisions
Since userspace now gives us a name to use for the device we need to handle
the case where userspace passes the same device name twice. This is done by
using d_lookup() (takes rename lock too). If a matching dentry under the root
dentry of the superblock is found we test whether it is still active and if
so return -EEXIST.
- remove per-super block idr tracking and locking
Since userspace now determines the name remove the idr tracking since it's
not needed anymore.
- remove ctl_mutex from struct binders_info
It was only needed to protect the per-super block idr which has been removed.
If I'm not mistaken we currently do not need to lock the ioctl() itself since
removing is handled by a simple unlink(). So remove the mutex.
- ensure that binderfs_evict_inode() doesn't cause double-frees on iput()
When setting up the inode fails at a step where a new inode already has been
allocated we need to set inode->i_private to NULL to ensure that
binderfs_evict_inode() doesn't try to free up stuff that we already freed
while handling the error.
---
drivers/android/Kconfig | 12 +
drivers/android/Makefile| 1 +
drivers/android/binder.c| 25 +-
drivers/android/binder_internal.h | 49 ++
drivers/android/binderfs.c | 565
include/uapi/linux/android/binder_ctl.h | 35 ++
include/uapi/linux/magic.h | 1 +
7 files changed, 671 insertions(+), 17 deletions(-)
create mode 100644 drivers/android/binder_internal.h
create mode 100644 drivers/android/binderfs.c
create mode 100644 include/uapi/linux/android/binder_ctl.h
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
index 51e8250d113f..4c190f8d1f4c 100644
--- a/drivers/android/Kconfig
+++ b/drivers/android/Kconfig
@@ -20,6 +20,18 @@ config ANDROID_BINDER_IPC
Android process, using Binder to identify, invoke and pass arguments
between said processes.
+config ANDROID_BINDERFS
+ bool "Android Binderfs filesystem"
+ depends on ANDROID_BINDER_IPC
+ default n
+ ---help---
+ Binderfs is a pseudo-filesystem for the Android Binder IPC driver
+ which can be mounted per-ipc namespace allowing to run multiple
+ instances of Android.
+ Each binderfs mount initially only contains a binder-control device.
+ It can be used to dynamically allocate new binder IPC devices via
+ ioctls.
+
config ANDROID_BINDER_DEVICES
string "Android Binder devices"
depends on ANDROID_BINDER_IPC
diff --git a/drivers/android/Makefile b/drivers/android/Makefile
index a01254c43ee3..c7856e3200da 100644
--- a/drivers/android/Makefile
+++ b/drivers/android/Makefile
@@ -1,4 +1,5 @@
ccflags-y += -I$(src) # needed for trace events
+obj-$(CONFIG_ANDROID_BINDERFS) += binderfs.o
obj-$(CONFIG_ANDROID_BINDER_IPC) += binder.o binder_alloc.o
obj-$(CONFIG_ANDROID_BINDER_IPC_SELFTEST) += binder_alloc_selftest.o
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index cb30a524d16d..3ed8bc4b7451 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -78,6 +78,7 @@
#include
#include "binder_alloc.h"
+#include "binder_internal.h"
#include "binder_trace.h"
static HLIST_HEAD(binder_deferred_list);
@@ -262,20 +263,6 @@ static struct binder_transaction_log_entry
*binder_transaction_log_add(
return e;
}
-struct binder_context {
- struct binder_node *binder_context_mgr_node;
- struct mutex context_mgr_node_lock;
-
- kuid_t binder_context_mgr_uid;
- const char *name;
-};
-
-struct binder_device {
- struct hlist_node hlist;
- struct miscdevice miscdev;
- struct binder_context context;
-};
-
/**
* struct binder_work - work enqueued on a worklist
* @entry: node enqueued on list
@@ -4935,8 +4922,12 @@ static int binder_open(struct inode *nodp, struct file
*filp)
proc->tsk = current->group_leader;
INIT_LIST_HEAD(&proc-&g
Re: [PATCH v1] binder: implement binderfs(Internet mail)
On Tue, Dec 11, 2018 at 03:44:48AM +, chouryzhou(周威) wrote: > > chouryzhou@, can you confirm that this implementation works for your > > android-in-container use-case? > > > > -Todd > > > We are running Android Pie in container now. If it works for later Android > release, it will works for us. The patchset is absolutely agnostic as to what Android version is running. :) It has no userspace consequences. If at all it makes it possible for Android to upgrade its userspace to use additional devices in the future without having to recompile the kernel. There's a whole lot of interesting things one could potentially do with this. :) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 0/3] Binder: Enable App Freezing Capability
On Mon, Mar 15, 2021 at 06:16:27PM -0700, Li Li wrote: > From: Li Li > > To improve the user experience when switching between recently used > applications, the background applications which are not currently needed > are cached in the memory. Normally, a well designed application will not > consume valuable CPU resources in the background. However, it's possible > some applications are not able or willing to behave as expected, wasting > energy even after being cached. > > It is a good idea to freeze those applications when they're only being > kept alive for the sake of faster startup and energy saving. These kernel > patches will provide the necessary infrastructure for user space framework > to freeze and thaw a cached process, check the current freezing status and > correctly deal with outstanding binder transactions to frozen processes. > > Changes in v2: avoid panic by using pr_warn for unexpected cases. > Changes in v3: improved errcode logic in binder_proc_transaction(). > > Marco Ballesio (3): > binder: BINDER_FREEZE ioctl > binder: use EINTR for interrupted wait for work > binder: BINDER_GET_FROZEN_INFO ioctl > > drivers/android/binder.c| 198 ++-- > drivers/android/binder_internal.h | 18 +++ > include/uapi/linux/android/binder.h | 20 +++ > 3 files changed, 224 insertions(+), 12 deletions(-) [+Cc Jann] Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] binder: BINDER_FREEZE ioctl
On Mon, Mar 15, 2021 at 06:16:28PM -0700, Li Li wrote:
> From: Marco Ballesio
>
> Frozen tasks can't process binder transactions, so a way is required to
> inform transmitting ends of communication failures due to the frozen
> state of their receiving counterparts. Additionally, races are possible
> between transitions to frozen state and binder transactions enqueued to
> a specific process.
>
> Implement BINDER_FREEZE ioctl for user space to inform the binder driver
> about the intention to freeze or unfreeze a process. When the ioctl is
> called, block the caller until any pending binder transactions toward
> the target process are flushed. Return an error to transactions to
> processes marked as frozen.
>
> Signed-off-by: Marco Ballesio
> Co-developed-by: Todd Kjos
> Signed-off-by: Todd Kjos
> Signed-off-by: Li Li
> ---
> drivers/android/binder.c| 139 ++--
> drivers/android/binder_internal.h | 12 +++
> include/uapi/linux/android/binder.h | 13 +++
> 3 files changed, 154 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index c119736ca56a..b93ca53bb90f 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -1506,6 +1506,12 @@ static void binder_free_transaction(struct
> binder_transaction *t)
>
> if (target_proc) {
> binder_inner_proc_lock(target_proc);
> + target_proc->outstanding_txns--;
> + if (target_proc->outstanding_txns < 0)
> + pr_warn("%s: Unexpected outstanding_txns %d\n",
> + __func__, target_proc->outstanding_txns);
> + if (!target_proc->outstanding_txns && target_proc->is_frozen)
> + wake_up_interruptible_all(&target_proc->freeze_wait);
> if (t->buffer)
> t->buffer->transaction = NULL;
> binder_inner_proc_unlock(target_proc);
> @@ -2331,10 +2337,11 @@ static int binder_fixup_parent(struct
> binder_transaction *t,
> * If the @thread parameter is not NULL, the transaction is always queued
> * to the waitlist of that specific thread.
> *
> - * Return: true if the transactions was successfully queued
> - * false if the target process or thread is dead
> + * Return: 0 if the transaction was successfully queued
> + * BR_DEAD_REPLY if the target process or thread is dead
> + * BR_FROZEN_REPLY if the target process or thread is frozen
> */
> -static bool binder_proc_transaction(struct binder_transaction *t,
> +static int binder_proc_transaction(struct binder_transaction *t,
> struct binder_proc *proc,
> struct binder_thread *thread)
> {
> @@ -2354,10 +2361,11 @@ static bool binder_proc_transaction(struct
> binder_transaction *t,
>
> binder_inner_proc_lock(proc);
>
> - if (proc->is_dead || (thread && thread->is_dead)) {
> + if ((proc->is_frozen && !oneway) || proc->is_dead ||
> + (thread && thread->is_dead)) {
> binder_inner_proc_unlock(proc);
> binder_node_unlock(node);
> - return false;
> + return proc->is_frozen ? BR_FROZEN_REPLY : BR_DEAD_REPLY;
> }
>
> if (!thread && !pending_async)
> @@ -2373,10 +2381,11 @@ static bool binder_proc_transaction(struct
> binder_transaction *t,
> if (!pending_async)
> binder_wakeup_thread_ilocked(proc, thread, !oneway /* sync */);
>
> + proc->outstanding_txns++;
> binder_inner_proc_unlock(proc);
> binder_node_unlock(node);
>
> - return true;
> + return 0;
> }
>
> /**
> @@ -3013,13 +3022,16 @@ static void binder_transaction(struct binder_proc
> *proc,
> if (reply) {
> binder_enqueue_thread_work(thread, tcomplete);
> binder_inner_proc_lock(target_proc);
> - if (target_thread->is_dead) {
> + if (target_thread->is_dead || target_proc->is_frozen) {
> + return_error = target_thread->is_dead ?
> + BR_DEAD_REPLY : BR_FROZEN_REPLY;
> binder_inner_proc_unlock(target_proc);
> goto err_dead_proc_or_thread;
> }
> BUG_ON(t->buffer->async_transaction != 0);
> binder_pop_transaction_ilocked(target_thread, in_reply_to);
> binder_enqueue_thread_work_ilocked(target_thread, &t->work);
> + target_proc->outstanding_txns++;
> binder_inner_proc_unlock(target_proc);
> wake_up_interruptible_sync(&target_thread->wait);
> binder_free_transaction(in_reply_to);
> @@ -3038,7 +3050,9 @@ static void binder_transaction(struct binder_proc *proc,
> t->from_parent = thread->transaction_stack;
> thread->transaction_stack = t;
> binder_inner_p
Re: [PATCH 0/2] Export receive_fd() to modules and do some cleanups
On Thu, Apr 01, 2021 at 05:09:30PM +0800, Xie Yongji wrote: > This series starts from Christian's comments on the series[1]. > We'd like to export receive_fd() which can not only be used by > our module in the series[1] but also allow further cleanups > like patch 2 does. > > Now this series is based on Christoph's patch[2]. > > [1] > https://lore.kernel.org/linux-fsdevel/20210331080519.172-1-xieyon...@bytedance.com/ > [2] https://lore.kernel.org/linux-fsdevel/20210325082209.1067987-2-...@lst.de > > Xie Yongji (2): > file: Export receive_fd() to modules > binder: Use receive_fd() to receive file from another process > > drivers/android/binder.c | 4 ++-- > fs/file.c| 6 ++ > include/linux/file.h | 7 +++ Hm, I think we're still talking a bit past each other. I'll try to illustrate what I mean in a patch series soon. Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Thu, Apr 01, 2021 at 11:54:45AM +0200, Greg KH wrote:
> On Thu, Apr 01, 2021 at 05:09:32PM +0800, Xie Yongji wrote:
> > Use receive_fd() to receive file from another process instead of
> > combination of get_unused_fd_flags() and fd_install(). This simplifies
> > the logic and also makes sure we don't miss any security stuff.
>
> But no logic is simplified here, and nothing is "missed", so I do not
> understand this change at all.
>
> >
> > Signed-off-by: Xie Yongji
> > ---
> > drivers/android/binder.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index c119736ca56a..080bcab7d632 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -3728,7 +3728,7 @@ static int binder_apply_fd_fixups(struct binder_proc
> > *proc,
> > int ret = 0;
> >
> > list_for_each_entry(fixup, &t->fd_fixups, fixup_entry) {
> > - int fd = get_unused_fd_flags(O_CLOEXEC);
> > + int fd = receive_fd(fixup->file, O_CLOEXEC);
>
> Why 2 spaces?
>
> >
> > if (fd < 0) {
> > binder_debug(BINDER_DEBUG_TRANSACTION,
> > @@ -3741,7 +3741,7 @@ static int binder_apply_fd_fixups(struct binder_proc
> > *proc,
> > "fd fixup txn %d fd %d\n",
> > t->debug_id, fd);
> > trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> > - fd_install(fd, fixup->file);
> > + fput(fixup->file);
>
> Are you sure this is the same???
>
> I d onot understand the need for this change at all, what is wrong with
> the existing code here?
I suggested something like this.
Some time back we added receive_fd() for seccomp and SCM_RIGHTS to have
a unified way of installing file descriptors including taking care of
handling sockets and running security hooks. The helper also encompasses
the whole get_unused_fd() + fd_install dance.
My suggestion was to look at all the places were we currently open-code
this in drivers/:
drivers/android/binder.c: int fd = get_unused_fd_flags(O_CLOEXEC);
drivers/char/tpm/tpm_vtpm_proxy.c: fd = get_unused_fd_flags(O_RDWR);
drivers/dma-buf/dma-buf.c: fd = get_unused_fd_flags(flags);
drivers/dma-buf/sw_sync.c: int fd = get_unused_fd_flags(O_CLOEXEC);
drivers/dma-buf/sync_file.c:int fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpio/gpiolib-cdev.c:fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
drivers/gpio/gpiolib-cdev.c:fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
drivers/gpio/gpiolib-cdev.c:fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c: fd =
get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/drm_atomic_uapi.c: fence_state->fd =
get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/drm_lease.c:fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC
| O_NONBLOCK));
drivers/gpu/drm/drm_syncobj.c: fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/drm_syncobj.c: int fd = get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c: out_fence_fd =
get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c: out_fence_fd =
get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/msm/msm_gem_submit.c: out_fence_fd =
get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/virtio/virtgpu_ioctl.c: out_fence_fd =
get_unused_fd_flags(O_CLOEXEC);
drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c:out_fence_fd =
get_unused_fd_flags(O_CLOEXEC);
drivers/infiniband/core/rdma_core.c:new_fd = get_unused_fd_flags(O_CLOEXEC);
drivers/media/mc/mc-request.c: fd = get_unused_fd_flags(O_CLOEXEC);
drivers/misc/cxl/api.c: rc = get_unused_fd_flags(flags);
drivers/scsi/cxlflash/ocxl_hw.c:rc = get_unused_fd_flags(flags);
drivers/scsi/cxlflash/ocxl_hw.c:dev_err(dev, "%s:
get_unused_fd_flags failed rc=%d\n",
drivers/tty/pty.c: fd = get_unused_fd_flags(flags);
drivers/vfio/vfio.c:ret = get_unused_fd_flags(O_CLOEXEC);
drivers/virt/nitro_enclaves/ne_misc_dev.c: enclave_fd =
get_unused_fd_flags(O_CLOEXEC);
and see whether all of them can be switched to simply using
receive_fd(). I did a completely untested rough sketch to illustrate
what I meant by using binder and devpts Xie seems to have just picked
those two. But the change is obviously only worth it if all or nearly
all callers can be switched over without risk of regression.
It would most likely simplify quite a few codepaths though especially in
the error paths since we can get rid of put_unused_fd() cleanup.
But it requires buy in from others obviously.
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 05:55:16AM +, Al Viro wrote:
> On Fri, Apr 16, 2021 at 05:19:50AM +, Al Viro wrote:
> > On Thu, Apr 01, 2021 at 12:40:34PM +0200, Christian Brauner wrote:
>
> > > and see whether all of them can be switched to simply using
> > > receive_fd(). I did a completely untested rough sketch to illustrate
> > > what I meant by using binder and devpts Xie seems to have just picked
> > > those two. But the change is obviously only worth it if all or nearly
> > > all callers can be switched over without risk of regression.
> > > It would most likely simplify quite a few codepaths though especially in
> > > the error paths since we can get rid of put_unused_fd() cleanup.
> > >
> > > But it requires buy in from others obviously.
> >
> > Opening a file can have non-trivial side effects; reserving a descriptor
> > can't. Moreover, if you look at the second hit in your list, you'll see
> > that we do *NOT* want that combined thing there - fd_install() is
> > completely irreversible, so we can't do that until we made sure the
> > reply (struct vtpm_proxy_new_dev) has been successfully copied to
> > userland. No, your receive_fd_user() does not cover that.
> >
> > There's a bunch of other cases like that - the next ones on your list
> > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc.
>
> FWIW, pretty much all ioctls that return descriptor as part of a structure
> stored to user-supplied address tend to be that way; some don't have any
> other output fields (in which case they probably would've been better off
> with just passing the descriptor as return value of ioctl(2)). Those
> might be served by that receive_fd_user() helper; anything that has several
> outputs won't be. The same goes for anything that has hard-to-undo
> operations as part of what they need to do:
> reserve fd
> set file up
> do hard-to-undo stuff
> install into descriptor table
> is the only feasible order of operations - reservation can fail, so
> it must go before the hard-to-undo part and install into descriptor
> table can't be undone at all, so it must come last. Looks like
> e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of
> that sort...
If receive_fd() or your receive_fd_user() proposal can replace a chunk
of open-coded places in modules where the split between reserving the
file descriptor and installing it is pointless it's probably already
worth it. Random example from io_uring where the file is already opened
way before (which yes, isn't a module afaik but another place where we
have that pattern):
static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)
{
int ret, fd;
fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
if (fd < 0)
return fd;
ret = io_uring_add_task_file(ctx);
if (ret) {
put_unused_fd(fd);
return ret;
}
fd_install(fd, file);
return fd;
}
A practical question also is whether the security receive hook thing
actually belongs into the receive_fd() and receive_fd_user() helpers for
the general case or whether that's just something that very few callers
would want. But in any case I'm unlikely to have time on my hands to
play with sm like this any time soon.
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 02:09:35PM +, Al Viro wrote:
> On Fri, Apr 16, 2021 at 03:42:52PM +0200, Christian Brauner wrote:
> > > > are drivers/dma-buf/sw_sync.c and drivers/dma-buf/sync_file.c, etc.
> > >
> > > FWIW, pretty much all ioctls that return descriptor as part of a structure
> > > stored to user-supplied address tend to be that way; some don't have any
> > > other output fields (in which case they probably would've been better off
> > > with just passing the descriptor as return value of ioctl(2)). Those
> > > might be served by that receive_fd_user() helper; anything that has
> > > several
> > > outputs won't be. The same goes for anything that has hard-to-undo
> > > operations as part of what they need to do:
> > > reserve fd
> > > set file up
> > > do hard-to-undo stuff
> > > install into descriptor table
> > > is the only feasible order of operations - reservation can fail, so
> > > it must go before the hard-to-undo part and install into descriptor
> > > table can't be undone at all, so it must come last. Looks like
> > > e.g. drivers/virt/nitro_enclaves/ne_misc_dev.c case might be of
> > > that sort...
> >
> > If receive_fd() or your receive_fd_user() proposal can replace a chunk
>
> My what proposal? The thing is currently in linux/file.h, put there
> by Kees half a year ago...
Yeah, I know. I was just referring to that line:
> > > might be served by that receive_fd_user() helper; anything that has
> > > several
I wasn't trying to imply you were the author or instigator of the api.
>
> > of open-coded places in modules where the split between reserving the
> > file descriptor and installing it is pointless it's probably already
> > worth it.
>
> A helper for use in some of the simplest cases, with big fat warnings
> not to touch if the things are not entirely trivial - sure, why not,
> same as we have anon_inode_getfd(). But that's a convenience helper,
> not a general purpose primitive.
>
> > Random example from io_uring where the file is already opened
> > way before (which yes, isn't a module afaik but another place where we
> > have that pattern):
> >
> > static int io_uring_install_fd(struct io_ring_ctx *ctx, struct file *file)
> > {
> > int ret, fd;
> >
> > fd = get_unused_fd_flags(O_RDWR | O_CLOEXEC);
> > if (fd < 0)
> > return fd;
> >
> > ret = io_uring_add_task_file(ctx);
>
> Huh? It's
> ret = io_uring_add_task_file(ctx, file);
> in the mainline and I don't see how that sucker could work without having
> file passed to it.
My point here was more that the _file_ has already been opened _before_
that call to io_uring_add_task_file(). But any potential non-trivial
side-effects of opening that file that you correctly pointed out in an
earlier mail has already happened by that time. Granted there are more
obvious examples, e.g. the binder stuff.
int fd = get_unused_fd_flags(O_CLOEXEC);
if (fd < 0) {
binder_debug(BINDER_DEBUG_TRANSACTION,
"failed fd fixup txn %d fd %d\n",
t->debug_id, fd);
ret = -ENOMEM;
break;
}
binder_debug(BINDER_DEBUG_TRANSACTION,
"fd fixup txn %d fd %d\n",
t->debug_id, fd);
trace_binder_transaction_fd_recv(t, fd, fixup->offset);
fd_install(fd, fixup->file);
fixup->file = NULL;
if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
fixup->offset, &fd,
sizeof(u32))) {
ret = -EINVAL;
break;
}
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 03:35:59PM +, Al Viro wrote:
> On Fri, Apr 16, 2021 at 05:13:10PM +0200, Christian Brauner wrote:
>
> > My point here was more that the _file_ has already been opened _before_
> > that call to io_uring_add_task_file(). But any potential non-trivial
> > side-effects of opening that file that you correctly pointed out in an
> > earlier mail has already happened by that time.
>
> The file comes from io_uring_get_file(), the entire thing is within the
> io_ring_ctx constructor and the only side effect there is ->ring_sock
> creation. And that stays until the io_ring_ctx is freed. I'm _not_
> saying I like io_uring style in general, BTW - in particular,
> ->ring_sock->file handling is a kludge (as is too much of interation
> with AF_UNIX machinery there). But from side effects POV we are fine
> there.
>
> > Granted there are more
> > obvious examples, e.g. the binder stuff.
> >
> > int fd = get_unused_fd_flags(O_CLOEXEC);
> >
> > if (fd < 0) {
> > binder_debug(BINDER_DEBUG_TRANSACTION,
> > "failed fd fixup txn %d fd %d\n",
> > t->debug_id, fd);
> > ret = -ENOMEM;
> > break;
> > }
> > binder_debug(BINDER_DEBUG_TRANSACTION,
> > "fd fixup txn %d fd %d\n",
> > t->debug_id, fd);
> > trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> > fd_install(fd, fixup->file);
> > fixup->file = NULL;
> > if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
> > fixup->offset, &fd,
> > sizeof(u32))) {
> > ret = -EINVAL;
> > break;
> > }
>
> ... and it's actually broken, since this
> /* All copies must be 32-bit aligned and 32-bit size */
> if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> return -EINVAL;
> in binder_alloc_copy_to_buffer() should've been done *before*
> fd_install(). If anything, it's an example of the situation when
> fd_receive() would be wrong...
They could probably refactor this but I'm not sure why they'd bother. If
they fail processing any of those files they end up aborting the
whole transaction.
(And the original code didn't check the error code btw.)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] binder: Use receive_fd() to receive file from another process
On Fri, Apr 16, 2021 at 05:58:25PM +0200, Christian Brauner wrote:
> On Fri, Apr 16, 2021 at 03:35:59PM +, Al Viro wrote:
> > On Fri, Apr 16, 2021 at 05:13:10PM +0200, Christian Brauner wrote:
> >
> > > My point here was more that the _file_ has already been opened _before_
> > > that call to io_uring_add_task_file(). But any potential non-trivial
> > > side-effects of opening that file that you correctly pointed out in an
> > > earlier mail has already happened by that time.
> >
> > The file comes from io_uring_get_file(), the entire thing is within the
> > io_ring_ctx constructor and the only side effect there is ->ring_sock
> > creation. And that stays until the io_ring_ctx is freed. I'm _not_
> > saying I like io_uring style in general, BTW - in particular,
> > ->ring_sock->file handling is a kludge (as is too much of interation
> > with AF_UNIX machinery there). But from side effects POV we are fine
> > there.
> >
> > > Granted there are more
> > > obvious examples, e.g. the binder stuff.
> > >
> > > int fd = get_unused_fd_flags(O_CLOEXEC);
> > >
> > > if (fd < 0) {
> > > binder_debug(BINDER_DEBUG_TRANSACTION,
> > >"failed fd fixup txn %d fd %d\n",
> > >t->debug_id, fd);
> > > ret = -ENOMEM;
> > > break;
> > > }
> > > binder_debug(BINDER_DEBUG_TRANSACTION,
> > >"fd fixup txn %d fd %d\n",
> > >t->debug_id, fd);
> > > trace_binder_transaction_fd_recv(t, fd, fixup->offset);
> > > fd_install(fd, fixup->file);
> > > fixup->file = NULL;
> > > if (binder_alloc_copy_to_buffer(&proc->alloc, t->buffer,
> > > fixup->offset, &fd,
> > > sizeof(u32))) {
> > > ret = -EINVAL;
> > > break;
> > > }
> >
> > ... and it's actually broken, since this
> > /* All copies must be 32-bit aligned and 32-bit size */
> > if (!check_buffer(alloc, buffer, buffer_offset, bytes))
> > return -EINVAL;
> > in binder_alloc_copy_to_buffer() should've been done *before*
> > fd_install(). If anything, it's an example of the situation when
> > fd_receive() would be wrong...
>
> They could probably refactor this but I'm not sure why they'd bother. If
> they fail processing any of those files they end up aborting the
> whole transaction.
> (And the original code didn't check the error code btw.)
(dma_buf_fd() seems like another good candidate. But again, I don't have
any plans to shove this down anyone's throat.)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: make sure fd closes complete
On Mon, Aug 30, 2021 at 12:51:46PM -0700, Todd Kjos wrote: > During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object > cleanup may close 1 or more fds. The close operations are > completed using the task work mechanism -- which means the thread > needs to return to userspace or the file object may never be > dereferenced -- which can lead to hung processes. > > Force the binder thread back to userspace if an fd is closed during > BC_FREE_BUFFER handling. > > Signed-off-by: Todd Kjos > --- Looks good. Thanks! Acked-by: Christian Brauner ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: don't detect sender/target during buffer cleanup
On Fri, Oct 15, 2021 at 04:38:11PM -0700, Todd Kjos wrote:
> When freeing txn buffers, binder_transaction_buffer_release()
> attempts to detect whether the current context is the target by
> comparing current->group_leader to proc->tsk. This is an unreliable
> test. Instead explicitly pass an 'is_failure' boolean.
>
> Detecting the sender was being used as a way to tell if the
> transaction failed to be sent. When cleaning up after
> failing to send a transaction, there is no need to close
> the fds associated with a BINDER_TYPE_FDA object. Now
> 'is_failure' can be used to accurately detect this case.
>
> Fixes: 44d8047f1d87 ("binder: use standard functions to allocate fds")
> Signed-off-by: Todd Kjos
> ---
Looks good to me.
Acked-by: Christian Brauner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: fix test regression due to sender_euid change
On Fri, Nov 12, 2021 at 10:07:20AM -0800, Todd Kjos wrote:
> This is a partial revert of commit
> 29bc22ac5e5b ("binder: use euid from cred instead of using task").
> Setting sender_euid using proc->cred caused some Android system test
> regressions that need further investigation. It is a partial
> reversion because subsequent patches rely on proc->cred.
>
> Cc: sta...@vger.kernel.org # 4.4+
> Fixes: 29bc22ac5e5b ("binder: use euid from cred instead of using task")
> Signed-off-by: Todd Kjos
> Change-Id: I9b1769a3510fed250bb21859ef8beebabe034c66
> ---
Acked-by: Christian Brauner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/4] binder: avoid potential data leakage when copying txn
On Tue, Nov 30, 2021 at 10:51:50AM -0800, Todd Kjos wrote:
> Transactions are copied from the sender to the target
> first and objects like BINDER_TYPE_PTR and BINDER_TYPE_FDA
> are then fixed up. This means there is a short period where
> the sender's version of these objects are visible to the
> target prior to the fixups.
>
> Instead of copying all of the data first, copy data only
> after any needed fixups have been applied.
>
> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> Reviewed-by: Martijn Coenen
> Signed-off-by: Todd Kjos
> ---
Looks good.
Acked-by: Christian Brauner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/4] binder: fix handling of error during copy
On Tue, Nov 30, 2021 at 10:51:49AM -0800, Todd Kjos wrote:
> If a memory copy function fails to copy the whole buffer,
> a positive integar with the remaining bytes is returned.
> In binder_translate_fd_array() this can result in an fd being
> skipped due to the failed copy, but the loop continues
> processing fds since the early return condition expects a
> negative integer on error.
>
> Fix by returning "ret > 0 ? -EINVAL : ret" to handle this case.
>
> Fixes: bb4a2e48d510 ("binder: return errors from buffer copy functions")
> Suggested-by: Dan Carpenter
> Signed-off-by: Todd Kjos
> ---
Looks good.
Acked-by: Christian Brauner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/4] binder: read pre-translated fds from sender buffer
On Tue, Nov 30, 2021 at 10:51:51AM -0800, Todd Kjos wrote: > This patch is to prepare for an up coming patch where we read > pre-translated fds from the sender buffer and translate them before > copying them to the target. It does not change run time. > > The patch adds two new parameters to binder_translate_fd_array() to > hold the sender buffer and sender buffer parent. These parameters let > us call copy_from_user() directly from the sender instead of using > binder_alloc_copy_from_buffer() to copy from the target. Also the patch > adds some new alignment checks. Previously the alignment checks would > have been done in a different place, but this lets us print more > useful error messages. > > Reviewed-by: Martijn Coenen > Signed-off-by: Todd Kjos > --- Looks good. Acked-by: Christian Brauner ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: fix async_free_space accounting for empty parcels
On Mon, Dec 20, 2021 at 11:01:50AM -0800, Todd Kjos wrote:
> In 4.13, commit 74310e06be4d ("android: binder: Move buffer out of area
> shared with user space")
> fixed a kernel structure visibility issue. As part of that patch,
> sizeof(void *) was used as the buffer size for 0-length data payloads so
> the driver could detect abusive clients sending 0-length asynchronous
> transactions to a server by enforcing limits on async_free_size.
>
> Unfortunately, on the "free" side, the accounting of async_free_space
> did not add the sizeof(void *) back. The result was that up to 8-bytes of
> async_free_space were leaked on every async transaction of 8-bytes or
> less. These small transactions are uncommon, so this accounting issue
> has gone undetected for several years.
>
> The fix is to use "buffer_size" (the allocated buffer size) instead of
> "size" (the logical buffer size) when updating the async_free_space
> during the free operation. These are the same except for this
> corner case of asynchronous transactions with payloads < 8 bytes.
>
> Fixes: 74310e06be4d ("android: binder: Move buffer out of area shared with
> user space")
> Signed-off-by: Todd Kjos
> ---
Looks good.
Acked-by: Christian Brauner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH][next] binder: remove redundant assignment to pointer n
On Thu, Sep 10, 2020 at 04:12:21PM +0100, Colin King wrote:
> From: Colin Ian King
>
> The pointer n is being initialized with a value that is
> never read and it is being updated later with a new value. The
> initialization is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King
> ---
Thanks!
Acked-by: Christian Brauner
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] binder: simplify the return expression of binder_mmap
On Mon, Sep 21, 2020 at 04:24:23PM +0800, Liu Shixin wrote:
> Simplify the return expression.
>
> Signed-off-by: Liu Shixin
> ---
Why not is all I can really say. :) But if this is about simplifying you
could get rid of the "ret" and "failure string" variables, and the goto
in that function completely by doing sm like this (__completely
untested__):
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f936530a19b0..26f4dc81b008 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5182,9 +5182,7 @@ static const struct vm_operations_struct binder_vm_ops = {
static int binder_mmap(struct file *filp, struct vm_area_struct *vma)
{
- int ret;
struct binder_proc *proc = filp->private_data;
- const char *failure_string;
if (proc->tsk != current->group_leader)
return -EINVAL;
@@ -5196,9 +5194,9 @@ static int binder_mmap(struct file *filp, struct
vm_area_struct *vma)
(unsigned long)pgprot_val(vma->vm_page_prot));
if (vma->vm_flags & FORBIDDEN_MMAP_FLAGS) {
- ret = -EPERM;
- failure_string = "bad vm_flags";
- goto err_bad_arg;
+ pr_err("%s: %d %lx-%lx %s failed %d\n", __func__,
+ proc->pid, vma->vm_start, vma->vm_end, "bad
vm_flags", -EPERM);
+ return -EPERM;
}
vma->vm_flags |= VM_DONTCOPY | VM_MIXEDMAP;
vma->vm_flags &= ~VM_MAYWRITE;
@@ -5206,15 +5204,7 @@ static int binder_mmap(struct file *filp, struct
vm_area_struct *vma)
vma->vm_ops = &binder_vm_ops;
vma->vm_private_data = proc;
- ret = binder_alloc_mmap_handler(&proc->alloc, vma);
- if (ret)
- return ret;
- return 0;
-
-err_bad_arg:
- pr_err("%s: %d %lx-%lx %s failed %d\n", __func__,
- proc->pid, vma->vm_start, vma->vm_end, failure_string, ret);
- return ret;
+ return binder_alloc_mmap_handler(&proc->alloc, vma);
}
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 -next] binder: simplify the return expression of binder_mmap
On Tue, Sep 29, 2020 at 09:52:16AM +0800, Liu Shixin wrote: > Simplify the return expression. > > Signed-off-by: Liu Shixin > --- Thanks! Acked-by: Christian Brauner ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 07/11] drivers/android/binder: convert stats, transaction_log to counter_atomic32
On Tue, Oct 06, 2020 at 02:44:38PM -0600, Shuah Khan wrote: > counter_atomic* is introduced to be used when a variable is used as > a simple counter and doesn't guard object lifetimes. This clearly > differentiates atomic_t usages that guard object lifetimes. > > counter_atomic* variables will wrap around to 0 when it overflows and > should not be used to guard resource lifetimes, device usage and > open counts that control state changes, and pm states. > > stats tracks per-process binder statistics. Unsure if there is a chance > of this overflowing, other than stats getting reset to 0. Convert it to > use counter_atomic. > > binder_transaction_log:cur is used to keep track of the current log entry > location. Overflow is handled in the code. Since it is used as a > counter, convert it to use counter_atomic32. > > This conversion doesn't change the overflow wrap around behavior. > > Signed-off-by: Shuah Khan > Reviewed-by: Joel Fernandes (Google) > Reviewed-by: Kees Cook > --- Thanks! Reviewed-by: Christian Brauner ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binder: fix UAF when releasing todo list
On Fri, Oct 09, 2020 at 04:24:55PM -0700, Todd Kjos wrote: > When releasing a thread todo list when tearing down > a binder_proc, the following race was possible which > could result in a use-after-free: > > 1. Thread 1: enter binder_release_work from binder_thread_release > 2. Thread 2: binder_update_ref_for_handle() -> binder_dec_node_ilocked() > 3. Thread 2: dec nodeA --> 0 (will free node) > 4. Thread 1: ACQ inner_proc_lock > 5. Thread 2: block on inner_proc_lock > 6. Thread 1: dequeue work (BINDER_WORK_NODE, part of nodeA) > 7. Thread 1: REL inner_proc_lock > 8. Thread 2: ACQ inner_proc_lock > 9. Thread 2: todo list cleanup, but work was already dequeued > 10. Thread 2: free node > 11. Thread 2: REL inner_proc_lock > 12. Thread 1: deref w->type (UAF) > > The problem was that for a BINDER_WORK_NODE, the binder_work element > must not be accessed after releasing the inner_proc_lock while > processing the todo list elements since another thread might be > handling a deref on the node containing the binder_work element > leading to the node being freed. > > Signed-off-by: Todd Kjos > --- Thanks! Acked-by: Christian Brauner ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] binder: change error code from postive to negative in binder_transaction
On Mon, Oct 26, 2020 at 07:03:14PM +0800, Zhang Qilong wrote: > Depending on the context, the error return value > here (extra_buffers_size < added_size) should be > negative. > > Signed-off-by: Zhang Qilong > --- Thanks! Acked-by: Christian Brauner ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1] binder: implement binderfs
> On Fri, Dec 7, 2018 at 3:26 PM Christian Brauner wrote:
> >
> > As discussed at Linux Plumbers Conference 2018 in Vancouver [1] this is the
> > implementation of binderfs.
> >
> > binderfs is a backwards-compatible filesystem for Android's binder ipc
> > mechanism. Each ipc namespace will mount a new binderfs instance. Mounting
> > binderfs multiple times at different locations in the same ipc namespace
> > will not cause a new super block to be allocated and hence it will be the
> > same filesystem instance.
> > Each new binderfs mount will have its own set of binder devices only
> > visible in the ipc namespace it has been mounted in. All devices in a new
> > binderfs mount will follow the scheme binder%d and numbering will always
> > start at 0.
> >
> > /* Backwards compatibility */
> > Devices requested in the Kconfig via CONFIG_ANDROID_BINDER_DEVICES for the
> > initial ipc namespace will work as before. They will be registered via
> > misc_register() and appear in the devtmpfs mount. Specifically, the
> > standard devices binder, hwbinder, and vndbinder will all appear in their
> > standard locations in /dev. Mounting or unmounting the binderfs mount in
> > the initial ipc namespace will have no effect on these devices, i.e. they
> > will neither show up in the binderfs mount nor will they disappear when the
> > binderfs mount is gone.
> >
> > /* binder-control */
> > Each new binderfs instance comes with a binder-control device. No other
> > devices will be present at first. The binder-control device can be used to
> > dynamically allocate binder devices. All requests operate on the binderfs
> > mount the binder-control device resides in:
> > - BINDER_CTL_ADD
> > Allocate a new binder device.
> > Assuming a new instance of binderfs has been mounted at /dev/binderfs via
> > mount -t binderfs binderfs /dev/binderfs. Then a request to create a new
> > binder device can be made via:
> >
> > struct binderfs_device device = {0};
> > snprintf(device.name, BINDERFS_MAX_NAME, "%s", "my-device");
> > int fd = open("/dev/binderfs/binder-control", O_RDWR);
> > ioctl(fd, BINDER_CTL_ADD, &device);
> >
> > binderfs will then allocate a new minor number and create the device
> > "my-device".
> > The struct binderfs_device will then be used to return the major and minor
> > number, for the device.
> > Binderfs devices can simply be removed via unlink().
> >
> > /* Implementation details */
> > - When binderfs is registered as a new filesystem it will dynamically
> > allocate a new major number. The allocated major number will be returned
> > in struct binderfs_device when a new binder device is allocated.
> > Minor numbers that have been given out are tracked in a global idr struct
> > that is capped at BINDERFS_MAX_MINOR. The minor number tracker is
> > protected by a global mutex. This is the only point of contention between
> > binderfs mounts.
> > - Each binderfs super block has its own struct binderfs_info that tracks
> > specific details about a binderfs instance: the ipc namespace, the dentry
> > of the binder-control device, the root uid and gid of the user namespace
> > the binderfs instance was mounted in.
> > - binderfs can be mounted by user namespace root in a non-initial user
> > namespace. The devices will be owned by user namespace root.
> > - New binder devices associated with a binderfs mount do not use the
> > full misc_register() infrastructure. The misc_register() infrastructure
> > can only create new devices in the host's devtmpfs mount. binderfs does
> > however only make devices appear under its own mountpoint and thus
> > allocates new character devices nodes from the inode of the root dentry
> > of the super block. This will have the side-effect that binderfs specific
> > device nodes do not appear in sysfs. This behavior is similar to devpts
> > allocated pts devices and has no effect on the functionality of the ipc
> > mechanism itself.
> >
> > /* Create a new binder device in a binderfs mount */
> > sudo mkdir /dev/binderfs
> > sudo mount -t binder binder /dev/binderfs
> >
> > #define _GNU_SOURCE
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> > #include
> >
> > int main(int argc, char *argv[])
> > {
> > int fd, ret, saved_errno;
> > stru
Re: [PATCH v1] binder: implement binderfs
On Thu, Dec 13, 2018 at 06:56:26PM +0100, Greg Kroah-Hartman wrote: > On Wed, Dec 12, 2018 at 01:51:27PM +0100, Christian Brauner wrote: > > > > Cc: Martijn Coenen > > > > Cc: Todd Kjos > > > > Cc: Greg Kroah-Hartman > > > > Signed-off-by: Christian Brauner > > > > Do we plan to bring this into mergeable shape before Christmas? I'm > > happy to do it. :) > > I haven't had the chance to give it a good review yet, but you should at > least fix up the kbuild warnings that the 0-day bot sent you :) __user is what's confusing kbot here. You want me to wait for your review or send out a v2 right now? :) Thanks! Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2] binder: implement binderfs
As discussed at Linux Plumbers Conference 2018 in Vancouver [1] this is the
implementation of binderfs.
/* Abstract */
binderfs is a backwards-compatible filesystem for Android's binder ipc
mechanism. Each ipc namespace will mount a new binderfs instance. Mounting
binderfs multiple times at different locations in the same ipc namespace
will not cause a new super block to be allocated and hence it will be the
same filesystem instance.
Each new binderfs mount will have its own set of binder devices only
visible in the ipc namespace it has been mounted in. All devices in a new
binderfs mount will follow the scheme binder%d and numbering will always
start at 0.
/* Backwards compatibility */
Devices requested in the Kconfig via CONFIG_ANDROID_BINDER_DEVICES for the
initial ipc namespace will work as before. They will be registered via
misc_register() and appear in the devtmpfs mount. Specifically, the
standard devices binder, hwbinder, and vndbinder will all appear in their
standard locations in /dev. Mounting or unmounting the binderfs mount in
the initial ipc namespace will have no effect on these devices, i.e. they
will neither show up in the binderfs mount nor will they disappear when the
binderfs mount is gone.
/* binder-control */
Each new binderfs instance comes with a binder-control device. No other
devices will be present at first. The binder-control device can be used to
dynamically allocate binder devices. All requests operate on the binderfs
mount the binder-control device resides in.
Assuming a new instance of binderfs has been mounted at /dev/binderfs
via mount -t binderfs binderfs /dev/binderfs. Then a request to create a
new binder device can be made as illustrated in [2].
Binderfs devices can simply be removed via unlink().
/* Implementation details */
- dynamic major number allocation:
When binderfs is registered as a new filesystem it will dynamically
allocate a new major number. The allocated major number will be returned
in struct binderfs_device when a new binder device is allocated.
- global minor number tracking:
Minor are tracked in a global idr struct that is capped at
BINDERFS_MAX_MINOR. The minor number tracker is protected by a global
mutex. This is the only point of contention between binderfs mounts.
- struct binderfs_info:
Each binderfs super block has its own struct binderfs_info that tracks
specific details about a binderfs instance:
- ipc namespace
- dentry of the binder-control device
- root uid and root gid of the user namespace the binderfs instance
was mounted in
- mountable by user namespace root:
binderfs can be mounted by user namespace root in a non-initial user
namespace. The devices will be owned by user namespace root.
- binderfs binder devices without misc infrastructure:
New binder devices associated with a binderfs mount do not use the
full misc_register() infrastructure.
The misc_register() infrastructure can only create new devices in the
host's devtmpfs mount. binderfs does however only make devices appear
under its own mountpoint and thus allocates new character device nodes
from the inode of the root dentry of the super block. This will have
the side-effect that binderfs specific device nodes do not appear in
sysfs. This behavior is similar to devpts allocated pts devices and
has no effect on the functionality of the ipc mechanism itself.
[1]: https://goo.gl/JL2tfX
[2]: program to allocate a new binderfs binder device:
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int main(int argc, char *argv[])
{
int fd, ret, saved_errno;
size_t len;
struct binderfs_device device = { 0 };
if (argc < 2)
exit(EXIT_FAILURE);
len = strlen(argv[1]);
if (len > BINDERFS_MAX_NAME)
exit(EXIT_FAILURE);
memcpy(device.name, argv[1], len);
fd = open("/dev/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
if (fd < 0) {
printf("%s - Failed to open binder-control device\n",
strerror(errno));
exit(EXIT_FAILURE);
}
ret = ioctl(fd, BINDER_CTL_ADD, &device);
saved_errno = errno;
close(fd);
errno = saved_errno;
if (ret < 0) {
printf("%s - Failed to allocate new binder device\n",
strerror(errno));
exit(EXIT_FAILURE);
}
printf("Allocated new binder device with major %d, minor %d, and "
"name %s\n", device.major, device.minor,
device.name);
exit(EXIT_SUCCESS);
}
Cc: Mar
Re: [PATCH v2] binder: implement binderfs
On Fri, Dec 14, 2018 at 10:11:23AM +0300, Dan Carpenter wrote:
> On Thu, Dec 13, 2018 at 10:59:11PM +0100, Christian Brauner wrote:
> > +/**
> > + * binderfs_new_inode - allocate inode from super block of a binderfs mount
> > + * @ref_inode: inode from wich the super block will be taken
> > + * @userp: buffer to copy information about new device for userspace to
> > + * @device:binder device for which the new inode will be allocated
> > + * @req: struct binderfs_device as copied from userspace
> > + *
> > + * This function will allocate a new inode from the super block of the
> > + * filesystem mount and attach a dentry to that inode.
> > + * Minor numbers are limited and tracked globally in binderfs_minors.
> > + * The function will stash a struct binder_device for the specific binder
> > + * device in i_private of the inode.
> > + *
> > + * Return: 0 on success, negative errno on failure
> > + */
> > +static int binderfs_new_inode(struct inode *ref_inode,
> > + struct binder_device *device,
> > + struct binderfs_device __user *userp,
> > + struct binderfs_device *req)
> > +{
> > + int minor, ret;
> > + struct dentry *dentry, *dup, *root;
> > + size_t name_len = BINDERFS_MAX_NAME + 1;
> > + char *name = NULL;
> > + struct inode *inode = NULL;
> > + struct super_block *sb = ref_inode->i_sb;
> > + struct binderfs_info *info = sb->s_fs_info;
> > +
> > + /* Reserve new minor number for the new device. */
> > + mutex_lock(&binderfs_minors_mutex);
> > + minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
> > + mutex_unlock(&binderfs_minors_mutex);
> > + if (minor < 0)
> > + return minor;
> > +
> > + ret = -ENOMEM;
> > + inode = new_inode(sb);
> > + if (!inode)
> > + goto err;
> > +
> > + inode->i_ino = minor + INODE_OFFSET;
> > + inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
> > + init_special_inode(inode, S_IFCHR | 0600,
> > + MKDEV(MAJOR(binderfs_dev), minor));
> > + inode->i_fop = &binder_fops;
> > + inode->i_uid = info->root_uid;
> > + inode->i_gid = info->root_gid;
> > + inode->i_private = device;
> > +
> > + name = kmalloc(name_len, GFP_KERNEL);
> > + if (!name)
> > + goto err;
> > +
> > + ret = snprintf(name, name_len, "%s", req->name);
> > + if (ret < 0 || (size_t)ret >= name_len) {
>
> kernel snprintf() doesn't return negatives and the cast isn't required
> either.
Good point. But I'd rather replace it with:
strscpy(name, req->name, name_len);
which is syntactically and semantically cleaner.
>
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > +
> > + device->binderfs_inode = inode;
> > + device->context.binder_context_mgr_uid = INVALID_UID;
> > + device->context.name = name;
> > + device->miscdev.name = name;
> > + device->miscdev.minor = minor;
> > + mutex_init(&device->context.context_mgr_node_lock);
> > +
> > + req->major = MAJOR(binderfs_dev);
> > + req->minor = minor;
> > +
> > + ret = copy_to_user(userp, req, sizeof(*req));
> > + if (ret)
> > + goto err;
>
> copy_to_user() returns the number of bytes remaining.
>
> ret = -EFAULT;
> if (copy_to_user(userp, req, sizeof(*req)))
> goto err;
>
> Also if this copy_to_user() fails, then does the kfree(name) and the
> iput(inode) lead to a double free of name in binderfs_evict_inode()?
I'm going to defer setting inode->i_private in all codepaths right
before d_add() after which no error handling occurs any more.
>
> > +
> > + root = sb->s_root;
> > + inode_lock(d_inode(root));
> > + dentry = d_alloc_name(root, name);
> > + if (!dentry) {
> > + inode_unlock(d_inode(root));
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + /* Verify that the name userspace gave us is not already in use. */
> > + dup = d_lookup(root, &dentry->d_name);
> > + if (dup) {
> > + if (d_really_is_positive(dup)) {
> > + dput(dup);
> > + dput(dentry);
> > + inode_unlock(d_inode(root));
> > + /*
> > +
[PATCH v3] binder: implement binderfs
As discussed at Linux Plumbers Conference 2018 in Vancouver [1] this is the
implementation of binderfs.
/* Abstract */
binderfs is a backwards-compatible filesystem for Android's binder ipc
mechanism. Each ipc namespace will mount a new binderfs instance. Mounting
binderfs multiple times at different locations in the same ipc namespace
will not cause a new super block to be allocated and hence it will be the
same filesystem instance.
Each new binderfs mount will have its own set of binder devices only
visible in the ipc namespace it has been mounted in. All devices in a new
binderfs mount will follow the scheme binder%d and numbering will always
start at 0.
/* Backwards compatibility */
Devices requested in the Kconfig via CONFIG_ANDROID_BINDER_DEVICES for the
initial ipc namespace will work as before. They will be registered via
misc_register() and appear in the devtmpfs mount. Specifically, the
standard devices binder, hwbinder, and vndbinder will all appear in their
standard locations in /dev. Mounting or unmounting the binderfs mount in
the initial ipc namespace will have no effect on these devices, i.e. they
will neither show up in the binderfs mount nor will they disappear when the
binderfs mount is gone.
/* binder-control */
Each new binderfs instance comes with a binder-control device. No other
devices will be present at first. The binder-control device can be used to
dynamically allocate binder devices. All requests operate on the binderfs
mount the binder-control device resides in.
Assuming a new instance of binderfs has been mounted at /dev/binderfs
via mount -t binderfs binderfs /dev/binderfs. Then a request to create a
new binder device can be made as illustrated in [2].
Binderfs devices can simply be removed via unlink().
/* Implementation details */
- dynamic major number allocation:
When binderfs is registered as a new filesystem it will dynamically
allocate a new major number. The allocated major number will be returned
in struct binderfs_device when a new binder device is allocated.
- global minor number tracking:
Minor are tracked in a global idr struct that is capped at
BINDERFS_MAX_MINOR. The minor number tracker is protected by a global
mutex. This is the only point of contention between binderfs mounts.
- struct binderfs_info:
Each binderfs super block has its own struct binderfs_info that tracks
specific details about a binderfs instance:
- ipc namespace
- dentry of the binder-control device
- root uid and root gid of the user namespace the binderfs instance
was mounted in
- mountable by user namespace root:
binderfs can be mounted by user namespace root in a non-initial user
namespace. The devices will be owned by user namespace root.
- binderfs binder devices without misc infrastructure:
New binder devices associated with a binderfs mount do not use the
full misc_register() infrastructure.
The misc_register() infrastructure can only create new devices in the
host's devtmpfs mount. binderfs does however only make devices appear
under its own mountpoint and thus allocates new character device nodes
from the inode of the root dentry of the super block. This will have
the side-effect that binderfs specific device nodes do not appear in
sysfs. This behavior is similar to devpts allocated pts devices and
has no effect on the functionality of the ipc mechanism itself.
[1]: https://goo.gl/JL2tfX
[2]: program to allocate a new binderfs binder device:
#define _GNU_SOURCE
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
int main(int argc, char *argv[])
{
int fd, ret, saved_errno;
size_t len;
struct binderfs_device device = { 0 };
if (argc < 2)
exit(EXIT_FAILURE);
len = strlen(argv[1]);
if (len > BINDERFS_MAX_NAME)
exit(EXIT_FAILURE);
memcpy(device.name, argv[1], len);
fd = open("/dev/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
if (fd < 0) {
printf("%s - Failed to open binder-control device\n",
strerror(errno));
exit(EXIT_FAILURE);
}
ret = ioctl(fd, BINDER_CTL_ADD, &device);
saved_errno = errno;
close(fd);
errno = saved_errno;
if (ret < 0) {
printf("%s - Failed to allocate new binder device\n",
strerror(errno));
exit(EXIT_FAILURE);
}
printf("Allocated new binder device with major %d, minor %d, and "
"name %s\n", device.major, device.minor,
device.name);
exit(EXIT_SUCCESS);
}
Cc: Mar
Re: [PATCH v3] binder: implement binderfs
On Wed, Dec 19, 2018 at 09:47:56AM +0100, Greg KH wrote: > On Fri, Dec 14, 2018 at 01:11:14PM +0100, Christian Brauner wrote: > > As discussed at Linux Plumbers Conference 2018 in Vancouver [1] this is the > > implementation of binderfs. > > > > Looks good to me, thanks so much for doing this, now queued up! Thanks for trusting me with this idea and work! Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] binderfs: implement sysctls
This implements three sysctls that have very specific goals:
1. /proc/sys/fs/binder/max:
Allow global root to globally limit the number of allocatable binder
devices.
2. /proc/sys/fs/binder/nr:
Allow global root to easily detect how many binder devices are currently
in use across all binderfs mounts.
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.
This is equivalent to sysctls of devpts.
Signed-off-by: Christian Brauner
---
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);
+
+static struct ctl_table binderfs_table[] = {
+ {
+ .procname = "max",
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .data = &device_limit,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &device_limit_min,
+ .extra2 = &device_limit_max,
+ },
+ {
+ .procname = "reserve",
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .data = &device_reserve,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &device_limit_min,
+ .extra2 = &device_limit_max,
+ },
+ {
+ .procname = "nr",
+ .maxlen = sizeof(int),
+ .mode = 0444,
+ .data = &device_count,
+ .proc_handler = proc_dointvec,
+ },
+ {}
+};
+
+static struct ctl_table binderfs_fs_table[] = {
+ {
+ .procname = "binder",
+ .mode = 0555,
+ .child = binderfs_table,
+ },
+ {}
+};
+
+static struct ctl_table binderfs_root_table[] = {
+ {
+ .procname = "fs",
+ .mode = 0555,
+ .child = binderfs_fs_table,
+ },
+ {}
+};
+
static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
{
return inode->i_sb->s_fs_info;
@@ -107,13 +172,21 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
struct inode *inode = NULL;
struct super_block *sb = ref_inode->i_sb;
struct binderfs_info *info = sb->s_fs_info;
+ bool use_reserved = (info->ipc_ns == &init_ipc_ns);
/* Reserve new minor number for the new device. */
mutex_lock(&binderfs_minors_mutex);
- minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
+ if (atomic_inc_return(&device_count) <
+ (device_limit - (use_reserved ? 0 : device_reserve)))
+ minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR,
+ GFP_KERNEL);
+ else
+ minor = -ENOSPC;
mutex_unlock(&binderfs_minors_mutex);
- if (minor < 0)
+ if (minor < 0) {
+ atomic_dec(&device_count);
return minor;
+ }
ret = -ENOMEM;
device = kzalloc(sizeof(*device), GFP_KERNEL);
@@ -187,6 +260,7 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
kfree(name);
kfree(device);
mutex_lock(&binderfs_minors_mutex);
+ atomic_dec(&device_count);
ida_free(&binderfs_minors, minor);
mutex_unlock(&binderfs_minors_mutex);
iput(inode);
@@ -239,6 +313,7 @@ static void binderfs_evict_inode(struct inode *inode)
return;
mutex_lock(&binderfs_minors_mutex);
+ atomic_dec(&device_count);
ida_free(&binderfs_minors, device->miscdev.minor);
mutex_unlock(&binderfs_minors_mutex);
@@ -536,6 +611,8 @@ static int __init init_binderfs(void)
binderfs_mnt = NULL;
unregister_filesystem(&binder_fs_type);
unregister
Re: [PATCH] binderfs: implement sysctls
On Fri, Dec 21, 2018 at 02:55:09PM +0100, Greg KH wrote:
> 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
Only global root in the initial user namespace. See the reasons below. :)
> are putting them in the "global" namespace, that feels like something
> that binderfs was trying to avoid in the first place.
There are a couple of reason imho:
- Global root needs a way to restrict how many binder devices can be
allocated across all user + ipc namespace pairs.
One obvious reason is that otherwise userns root in a non-initial user
namespace can allocate a huge number of binder devices (pick a random
number say 10.000) and use up a lot of kernel memory.
In addition they can pound on the binder.c code causing a lot of
contention for the remaining global lock in there.
We should let global root explicitly restrict non-initial namespaces
in this respect. Imho, that's just good security design. :)
- The reason for having a number of reserved devices is when the initial
binderfs mount needs to bump the number of binder devices after the
initial allocation done during say boot (e.g. it could've removed
devices and wants to reallocate new ones but all binder minor numbers
have been given out or just needs additional devices). By reserving an
initial pool of binder devices this can be easily accounted for and
future proofs userspace. This is to say: global root in the initial
userns + ipcns gets dibs on however many devices it wants. :)
- The fact that we have a single shared pool of binder device minor
numbers for all namespaces imho makes it necessary for the global root
user in the initial ipc + user namespace to manage device allocation
and delegation.
The binderfs sysctl stuff is really small code-wise and adds a lot of
security without any performance impact on the code itself. So we
actually very strictly adhere to the requirement to not blindly
sacrifice performance for security. :)
>
> > 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 :)
It's one of its saner design choices though. :)
>
> >
> > Signed-off-by: Christian Brauner
> > ---
> > 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binderfs: implement sysctls
On Fri, Dec 21, 2018 at 04:37:58PM +0100, Greg KH wrote: > On Fri, Dec 21, 2018 at 03:12:42PM +0100, Christian Brauner wrote: > > On Fri, Dec 21, 2018 at 02:55:09PM +0100, Greg KH wrote: > > > 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 > > > > Only global root in the initial user namespace. See the reasons below. :) > > > > > are putting them in the "global" namespace, that feels like something > > > that binderfs was trying to avoid in the first place. > > > > There are a couple of reason imho: > > - Global root needs a way to restrict how many binder devices can be > > allocated across all user + ipc namespace pairs. > > One obvious reason is that otherwise userns root in a non-initial user > > namespace can allocate a huge number of binder devices (pick a random > > number say 10.000) and use up a lot of kernel memory. > > Root can do tons of other bad things too, why are you picking on That's global root not userns root though. :) These sysctls are about global root gaining the ability to proactively restrict binder device delegation. > binderfs here? :) > > > In addition they can pound on the binder.c code causing a lot of > > contention for the remaining global lock in there. > > That's the problem of that container, don't let it do that. Or remove > the global lock :) > > > We should let global root explicitly restrict non-initial namespaces > > in this respect. Imho, that's just good security design. :) > > If you do not trust your container enough to have it properly allocate > the correct binder resources, then perhaps you shouldn't be allowing it > to allocate any resources at all? Containers just like VMs get delegated and you might not have control over what is running in there. That's AWS in a nutshell. :) Restricting it by saying "just don't do that" seems not something that is appropriate given the workloads out there in the wild. In general, I do *understand* the reasoning but I think the premise is flawed if we can somewhat trivially make this safe. > > > - The reason for having a number of reserved devices is when the initial > > binderfs mount needs to bump the number of binder devices after the > > initial allocation done during say boot (e.g. it could've removed > > devices and wants to reallocate new ones but all binder minor numbers > > have been given out or just needs additional devices). By reserving an > > initial pool of binder devices this can be easily accounted for and > > future proofs userspace. This is to say: global root in the initial > > userns + ipcns gets dibs on however many devices it wants. :) > > binder devices do not "come and go" at runtime, you need to set them up > initially and then all is fine. So there should never be a need for the > "global" instance to need "more" binder devices once it is up and > running. So I don't see what you are really trying to solve here. That's dismissing a whole range of use-cases where you might allocate and deallocate devices on the fly which this is somewhat designed for. But I guess ok for now. > > You seem to be trying to protect the system from the container you just > gave root to and trusted it with creating its own binder instances. > If you do not trust it to create binder instances then do not allow it > to create binder instances! :) Again, I get the reasoning but think that this dismisses major real-world use-cases not just for binderfs but for all instances where untrusted workloads are run which both containers and VMs aim to make sure are possible. Note, I mean untrusted not in the sense of necessarily being malicious but just "can't guarantee that things don't blow up in your face". > > > - The fact that we have a single shared pool of binder device minor > > numbers for all namespaces imho makes it necessary for the global root > > user in the initial ipc + user namespace to manage device allocation > > and delegation. > > You are managing the allocation, you are giving who ever asks for one a > device. If you run out of devices, oops, you run out of devices, that's > it. Are you really ever going to run out of a major's number of binder > devices? The point is more about someone intentionally trying to do that. > > > The binderfs sysctl stuff is really small code-wise and adds a
Re: [PATCH] binderfs: implement sysctls
On Fri, Dec 21, 2018 at 05:33:16PM +0100, Greg KH wrote: > On Fri, Dec 21, 2018 at 04:59:19PM +0100, Christian Brauner wrote: > > On Fri, Dec 21, 2018 at 04:37:58PM +0100, Greg KH wrote: > > > On Fri, Dec 21, 2018 at 03:12:42PM +0100, Christian Brauner wrote: > > > > On Fri, Dec 21, 2018 at 02:55:09PM +0100, Greg KH wrote: > > > > > 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 > > > > > > > > Only global root in the initial user namespace. See the reasons below. > > > > :) > > > > > > > > > are putting them in the "global" namespace, that feels like something > > > > > that binderfs was trying to avoid in the first place. > > > > > > > > There are a couple of reason imho: > > > > - Global root needs a way to restrict how many binder devices can be > > > > allocated across all user + ipc namespace pairs. > > > > One obvious reason is that otherwise userns root in a non-initial user > > > > namespace can allocate a huge number of binder devices (pick a random > > > > number say 10.000) and use up a lot of kernel memory. > > > > > > Root can do tons of other bad things too, why are you picking on > > > > That's global root not userns root though. :) These sysctls are about > > global root gaining the ability to proactively restrict binder device > > delegation. > > > > > binderfs here? :) > > > > > > > In addition they can pound on the binder.c code causing a lot of > > > > contention for the remaining global lock in there. > > > > > > That's the problem of that container, don't let it do that. Or remove > > > the global lock :) > > > > > > > We should let global root explicitly restrict non-initial namespaces > > > > in this respect. Imho, that's just good security design. :) > > > > > > If you do not trust your container enough to have it properly allocate > > > the correct binder resources, then perhaps you shouldn't be allowing it > > > to allocate any resources at all? > > > > Containers just like VMs get delegated and you might not have control > > over what is running in there. That's AWS in a nutshell. :) Restricting > > it by saying "just don't do that" seems not something that is > > appropriate given the workloads out there in the wild. > > In general, I do *understand* the reasoning but I think the premise is > > flawed if we can somewhat trivially make this safe. > > > > > > > > > - The reason for having a number of reserved devices is when the initial > > > > binderfs mount needs to bump the number of binder devices after the > > > > initial allocation done during say boot (e.g. it could've removed > > > > devices and wants to reallocate new ones but all binder minor numbers > > > > have been given out or just needs additional devices). By reserving an > > > > initial pool of binder devices this can be easily accounted for and > > > > future proofs userspace. This is to say: global root in the initial > > > > userns + ipcns gets dibs on however many devices it wants. :) > > > > > > binder devices do not "come and go" at runtime, you need to set them up > > > initially and then all is fine. So there should never be a need for the > > > "global" instance to need "more" binder devices once it is up and > > > running. So I don't see what you are really trying to solve here. > > > > That's dismissing a whole range of use-cases where you might allocate > > and deallocate devices on the fly which this is somewhat designed for. > > But I guess ok for now. > > > > > > > > You seem to be trying to protect the system from the container you just > > > gave root to and trusted it with creating its own binder instances. > > > If you do not trust it to create binder instances then do not allow it > > > to create binder instances! :) > > > > Again, I get the reasoning but think that this dismisses major > > real-world use-cases not just for binderfs but for all instances where > >
[PATCH] binderfs: implement "max" mount option
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.
Additionally, the binderfs instance in the initial ipc namespace will
always have a reserve of at least 1024 binder devices unless explicitly
capped via max=.
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
---
drivers/android/binderfs.c | 109 +++--
1 file changed, 104 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7496b10532aa..93aee00994d4 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -39,6 +40,11 @@
#define INODE_OFFSET 3
#define INTSTRLEN 21
#define BINDERFS_MAX_MINOR (1U << MINORBITS)
+/*
+ * Ensure that the initial ipc namespace always has a good chunk of devices
+ * available.
+ */
+#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 1024)
static struct vfsmount *binderfs_mnt;
@@ -46,6 +52,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 +79,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;
+ atomic_t device_count;
};
static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
@@ -107,13 +134,22 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
struct inode *inode = NULL;
struct super_block *sb = ref_inode->i_sb;
struct binderfs_info *info = sb->s_fs_info;
+ bool use_reserve = (info->ipc_ns == &init_ipc_ns);
/* Reserve new minor number for the new device. */
mutex_lock(&binderfs_minors_mutex);
- minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
+ if (atomic_inc_return(&info->device_count) < info->mount_opts.max)
+ minor = ida_alloc_max(&binderfs_minors,
+ use_reserve ? BINDERFS_MAX_MINOR :
+ BINDERFS_MAX_MINOR_CAPPED,
+ GFP_KERNEL);
+ else
+ minor = -ENOSPC;
mutex_unlock(&binderfs_minors_mutex);
- if (minor < 0)
+ if (minor < 0) {
+ atomic_dec(&info->device_count);
return minor;
+ }
ret = -ENOMEM;
device = kzalloc(sizeof(*device), GFP_KERNEL);
@@ -187,6 +223,7 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
kfree(name);
kfree(device);
mutex_lock(&binder
Re: [PATCH] binderfs: implement "max" mount option
On Sun, Dec 23, 2018 at 12:29:44PM +0100, Greg KH wrote:
> On Sat, Dec 22, 2018 at 10:18:06PM +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.
>
> Ok, this is fine, but why such a big default? You only need 4 to run a
> modern android system, and anyone using binder outside of android is
> really too crazy to ever be using it in a container :)
>
> > Additionally, the binderfs instance in the initial ipc namespace will
> > always have a reserve of at least 1024 binder devices unless explicitly
> > capped via max=.
>
> Again, why so many? And why wouldn't that initial ipc namespace already
> have their device nodes created _before_ anything else is mounted?
Right, my issue is with re-creating devices, like if binderfs gets
unmounted or if devices get removed via rm. But we can lower the number
to 4 (see below).
>
> Some comments on the patch below:
Thanks!
>
> > +/*
> > + * Ensure that the initial ipc namespace always has a good chunk of devices
> > + * available.
> > + */
> > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 1024)
>
> Again that seems crazy big, how about splitting this into two different
> patches, one for the max= stuff, and one for this "reserve some minors"
> thing, so we can review them separately.
Yes, let's do that. I will also lower this to 4 reserved devices.
>
> >
> > static struct vfsmount *binderfs_mnt;
> >
> > @@ -46,6 +52,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 +79,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;
> > + atomic_t device_count;
>
> Why atomic?
>
> You should already have the lock held every time this is accessed,
> so no need to use an atomic value, just use an int.
>
> > /* Reserve new minor number for the new device. */
> > mutex_lock(&binderfs_minors_mutex);
> > - minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
> > + if (atomic_inc_return(&info->device_count) < info->mount_opts.max)
>
> No need for atomic, see, your lock is held :)
Habit, to be honest.
Thanks, fixed version to follow in a bit.
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] binderfs: implement "max" mount option
On Sat, Dec 22, 2018 at 10:18:06PM +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.
> Additionally, the binderfs instance in the initial ipc namespace will
> always have a reserve of at least 1024 binder devices unless explicitly
> capped via max=.
>
> 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
> ---
> drivers/android/binderfs.c | 109 +++--
> 1 file changed, 104 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 7496b10532aa..93aee00994d4 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -39,6 +40,11 @@
> #define INODE_OFFSET 3
> #define INTSTRLEN 21
> #define BINDERFS_MAX_MINOR (1U << MINORBITS)
> +/*
> + * Ensure that the initial ipc namespace always has a good chunk of devices
> + * available.
> + */
> +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 1024)
>
> static struct vfsmount *binderfs_mnt;
>
> @@ -46,6 +52,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 +79,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;
> + atomic_t device_count;
> };
>
> static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
> @@ -107,13 +134,22 @@ static int binderfs_binder_device_create(struct inode
> *ref_inode,
> struct inode *inode = NULL;
> struct super_block *sb = ref_inode->i_sb;
> struct binderfs_info *info = sb->s_fs_info;
> + bool use_reserve = (info->ipc_ns == &init_ipc_ns);
>
> /* Reserve new minor number for the new device. */
> mutex_lock(&binderfs_minors_mutex);
> - minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
> + if (atomic_inc_return(&info->device_count) < info->mount_opts.max)
> + minor = ida_alloc_max(&binderfs_minors,
> + use_reserve ? BINDERFS_MAX_MINOR :
> +
[PATCH v1 2/2] binderfs: reserve devices for initial mount
The binderfs instance in the initial ipc namespace will always have a reserve of 4 binder devices unless explicitly capped by specifying a lower value via the "max" mount option. This ensures when binder devices are removed (on accident or on purpose) they can always be recreated without risking that all minor numbers have already been used up. Cc: Todd Kjos Cc: Greg Kroah-Hartman Signed-off-by: Christian Brauner --- v1: - patch introduced v0: - patch not present --- drivers/android/binderfs.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index 873adda064ac..aa635c7ea727 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -40,6 +40,8 @@ #define INODE_OFFSET 3 #define INTSTRLEN 21 #define BINDERFS_MAX_MINOR (1U << MINORBITS) +/* Ensure that the initial ipc namespace always has a devices available. */ +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) static struct vfsmount *binderfs_mnt; @@ -129,11 +131,14 @@ static int binderfs_binder_device_create(struct inode *ref_inode, struct inode *inode = NULL; struct super_block *sb = ref_inode->i_sb; struct binderfs_info *info = sb->s_fs_info; + bool use_reserve = (info->ipc_ns == &init_ipc_ns); /* Reserve new minor number for the new device. */ mutex_lock(&binderfs_minors_mutex); if (++info->device_count <= info->mount_opts.max) - minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, + minor = ida_alloc_max(&binderfs_minors, + use_reserve ? BINDERFS_MAX_MINOR : + BINDERFS_MAX_MINOR_CAPPED, GFP_KERNEL); else minor = -ENOSPC; -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 1/2] binderfs: implement "max" mount option
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(&binderfs_minors_mutex);
- minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
+ if (++info->device_count <= info->mount_opts.max)
+ minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR,
+ GFP_KERNEL);
+ else
+ minor = -ENOSPC;
mutex_unlock(&binderfs_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(&binderfs_minors_mutex);
+ --info->device_count;
ida_free(&binderfs_minors, minor);
mutex_unlock(&binderfs_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 b
Re: [PATCH v1 1/2] binderfs: implement "max" mount option
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/2] binderfs: implement "max" mount option
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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 1/2] binderfs: implement "max" mount option
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(&binderfs_minors_mutex);
> > - minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
> > + if (++info->device_count <= info->mount_opts.max)
> > + minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR,
> > + GFP_KERNEL);
> > + else
> > + minor = -ENOSPC;
> > mutex_unlock(&binderfs_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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/2] binderfs: implement "max" mount option
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
---
v2:
- grab mutex to decrement device_count in the one place where it was
missing (cf. [changelog-3]).
v1:
- split reserving devices for the binderfs instance in the initial ipc
namespace into a separate patch (cf. [changelog-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. [changelog-2])
v0:
- patch introduced
[changelog-1]: https://lore.kernel.org/lkml/20181223112944.gc27...@kroah.com/
[changelog-2]:
https://lore.kernel.org/lkml/20181223135755.sqnv5ave62jtj...@brauner.io/
[changelog-3]: https://lore.kernel.org/lkml/20190102091731.GB3781@kadam/
---
drivers/android/binderfs.c | 104 ++---
1 file changed, 98 insertions(+), 6 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7496b10532aa..386e286f077f 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,17 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
/* Reserve new minor number for the new device. */
mutex_lock(&binderfs_minors_mutex);
- minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
- mutex_unlock(&binderfs_minors_mutex);
- if (minor < 0)
+ if (++info->device_count <= info->mount_opts.max)
+ minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR,
+ GFP_KERNEL);
+ else
+ minor = -ENOSPC;
+ if (minor < 0) {
+ --info->device_count;
+ mutex_unlock(&binderfs_minors_mutex);
return minor;
+ }
+ mutex_unlock(&binderfs_minors_mutex);
ret = -ENOMEM;
device = kzalloc(sizeof(*device), GFP_KERNEL);
@@ -187,6 +216,7 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
kfree(name);
kfree(device);
mutex_lock(&binderfs_minors_mutex);
+ --info->device_count;
ida_free(&
[PATCH v2 2/2] binderfs: reserve devices for initial mount
The binderfs instance in the initial ipc namespace will always have a reserve of 4 binder devices unless explicitly capped by specifying a lower value via the "max" mount option. This ensures when binder devices are removed (on accident or on purpose) they can always be recreated without risking that all minor numbers have already been used up. Cc: Todd Kjos Cc: Greg Kroah-Hartman Signed-off-by: Christian Brauner --- v2: - patch unchanged v1: - patch introduced v0: - patch not present --- drivers/android/binderfs.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index 386e286f077f..8d54368b3c1a 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -40,6 +40,8 @@ #define INODE_OFFSET 3 #define INTSTRLEN 21 #define BINDERFS_MAX_MINOR (1U << MINORBITS) +/* Ensure that the initial ipc namespace always has devices available. */ +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) static struct vfsmount *binderfs_mnt; @@ -129,11 +131,14 @@ static int binderfs_binder_device_create(struct inode *ref_inode, struct inode *inode = NULL; struct super_block *sb = ref_inode->i_sb; struct binderfs_info *info = sb->s_fs_info; + bool use_reserve = (info->ipc_ns == &init_ipc_ns); /* Reserve new minor number for the new device. */ mutex_lock(&binderfs_minors_mutex); if (++info->device_count <= info->mount_opts.max) - minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, + minor = ida_alloc_max(&binderfs_minors, + use_reserve ? BINDERFS_MAX_MINOR : + BINDERFS_MAX_MINOR_CAPPED, GFP_KERNEL); else minor = -ENOSPC; -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 1/2] binderfs: implement "max" mount option
On Wed, Jan 2, 2019 at 12:32 PM 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 Fwiw, once this lands I intend to add a binderfs.txt file to Documentation/filesystems so that we have proper documentation too. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 2/2] binderfs: reserve devices for initial mount
On Thu, Jan 03, 2019 at 12:25:24PM -0800, Todd Kjos wrote: > On Sun, Dec 23, 2018 at 6:36 AM Christian Brauner > wrote: > > > > The binderfs instance in the initial ipc namespace will always have a > > reserve of 4 binder devices unless explicitly capped by specifying a lower > > value via the "max" mount option. > > This ensures when binder devices are removed (on accident or on purpose) > > they can always be recreated without risking that all minor numbers have > > already been used up. > > > > Cc: Todd Kjos > > Cc: Greg Kroah-Hartman > > Signed-off-by: Christian Brauner > > --- > > v1: > > - patch introduced > > v0: > > - patch not present > > --- > > drivers/android/binderfs.c | 7 ++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > > index 873adda064ac..aa635c7ea727 100644 > > --- a/drivers/android/binderfs.c > > +++ b/drivers/android/binderfs.c > > @@ -40,6 +40,8 @@ > > #define INODE_OFFSET 3 > > #define INTSTRLEN 21 > > #define BINDERFS_MAX_MINOR (1U << MINORBITS) > > +/* Ensure that the initial ipc namespace always has a devices available. */ > > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) > > Why do you ever need more minors per instance than the number of > devices listed in CONFIG_ANDROID_BINDER_DEVICES? Are you asking asking why this is 4 and not 3? In that case we should probably parse CONFIG_ANDROID_BINDER_DEVICES once at init time and then reserve that many binder devices. Thoughts? Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v1 2/2] binderfs: reserve devices for initial mount
On Thu, Jan 03, 2019 at 01:47:13PM -0800, Todd Kjos wrote: > On Thu, Jan 3, 2019 at 12:34 PM Christian Brauner > wrote: > > > > On Thu, Jan 03, 2019 at 12:25:24PM -0800, Todd Kjos wrote: > > > On Sun, Dec 23, 2018 at 6:36 AM Christian Brauner > > > wrote: > > > > > > > > The binderfs instance in the initial ipc namespace will always have a > > > > reserve of 4 binder devices unless explicitly capped by specifying a > > > > lower > > > > value via the "max" mount option. > > > > This ensures when binder devices are removed (on accident or on purpose) > > > > they can always be recreated without risking that all minor numbers have > > > > already been used up. > > > > > > > > Cc: Todd Kjos > > > > Cc: Greg Kroah-Hartman > > > > Signed-off-by: Christian Brauner > > > > --- > > > > v1: > > > > - patch introduced > > > > v0: > > > > - patch not present > > > > --- > > > > drivers/android/binderfs.c | 7 ++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > > > > index 873adda064ac..aa635c7ea727 100644 > > > > --- a/drivers/android/binderfs.c > > > > +++ b/drivers/android/binderfs.c > > > > @@ -40,6 +40,8 @@ > > > > #define INODE_OFFSET 3 > > > > #define INTSTRLEN 21 > > > > #define BINDERFS_MAX_MINOR (1U << MINORBITS) > > > > +/* Ensure that the initial ipc namespace always has a devices > > > > available. */ > > > > +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) > > > > > > Why do you ever need more minors per instance than the number of > > > devices listed in CONFIG_ANDROID_BINDER_DEVICES? > > > > Are you asking asking why this is 4 and not 3? In that case we should > > probably parse CONFIG_ANDROID_BINDER_DEVICES once at init time and then > > reserve that many binder devices. Thoughts? > > I'm asking why CONFIG_ANDROID_BINDER_DEVICES isn't the source of truth > for the number of devices (it normally specifies 3 devices, but could > be different). I can't think of a reason why you'd want > CONFIG_MAX_MINOR_CAPPED to be different than the number of devices > indicated by CONFIG_ANDROID_BINDER_DEVICES and having it specified in > two places seems error prone. I'm not following. The CONFIG_MAX_MINOR_CAPPED and CONFIG_ANDROID_BINDER_DEVICES do not have a relation to each other just like binder devices requested in CONFIG_ANDROID_BINDER_DEVICES do not have a relation to binderfs binder devices as was requested for this patchset. I also don't know what you mean "appear in two places". The fact is, binderfs binder devices are independent of binderfs binder devices. So it is perfectly reasonable for someone to say CONFIG_ANDROID_BINDER_DEVICES="" and *only* rely on binderfs itself. Which absolutely need to be possible. What I want in such scenarios is that people always have a number of binderfs binder devices guaranteed to be available in the binderfs mount in the initial ipc namespace no matter how many devices are allocated in non-initial ipc namespace binderfs mounts. That's why allo non-initial ipc namespace binderfs mounts will use the CONFIG_MAX_MINOR_CAPPED macro which guarantees that there's number of devices reserved for the binderfs instance in the initial ipc namespace. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] binderfs: remove wrong kern_mount() call
The binderfs filesystem never needs to be mounted by the kernel itself.
This is conceptually wrong and should never have been done in the first
place.
Fixes: 3ad20fe393b ("binder: implement binderfs")
Signed-off-by: Christian Brauner
---
drivers/android/binderfs.c | 10 --
1 file changed, 10 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7496b10532aa..6f68d6217eb3 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -40,8 +40,6 @@
#define INTSTRLEN 21
#define BINDERFS_MAX_MINOR (1U << MINORBITS)
-static struct vfsmount *binderfs_mnt;
-
static dev_t binderfs_dev;
static DEFINE_MUTEX(binderfs_minors_mutex);
static DEFINE_IDA(binderfs_minors);
@@ -530,14 +528,6 @@ static int __init init_binderfs(void)
return ret;
}
- binderfs_mnt = kern_mount(&binder_fs_type);
- if (IS_ERR(binderfs_mnt)) {
- ret = PTR_ERR(binderfs_mnt);
- binderfs_mnt = NULL;
- unregister_filesystem(&binder_fs_type);
- unregister_chrdev_region(binderfs_dev, BINDERFS_MAX_MINOR);
- }
-
return ret;
}
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/2] binderfs: fixes
Hey, This small series deals with two fix/improvements for binderfs. And both actually allows us to remove code. *Yay!* The first patch removes an unneeded and conceptually wrong kern_mount() call for binderfs. I think this even might have been pointed out at some point by Greg. I'm not sure right now. The second patch makes each mount of binderfs - similar to kdbusfs - a new instance. This mainly allows users to get private binderfs instances in the same ipc namespace which has been requested recently and makes the whole implementation more intuitive and simple for various reaons. More details can be found in the commit message for each patch. Thanks! Christian Christian Brauner (2): binderfs: remove wrong kern_mount() call binderfs: make each binderfs mount a new instance drivers/android/binderfs.c | 51 ++ 1 file changed, 2 insertions(+), 49 deletions(-) -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] binderfs: make each binderfs mount a new instance
When currently mounting binderfs in the same ipc namespace twice:
mount -t binder binder /A
mount -t binder binder /B
then the binderfs instances mounted on /A and /B will be the same, i.e.
they will have the same superblock. This was the first approach that seemed
reasonable. However, this leads to some problems and inconsistencies:
/* private binderfs instance in same ipc namespace */
There is no way for a user to request a private binderfs instance in the
same ipc namespace.
This request has been made in a private mail to me by two independent
people.
/* bind-mounts */
If users want the same binderfs instance to appear in multiple places they
can use bind mounts. So there is no value in having a request for a new
binderfs mount giving them the same instance.
/* unexpected behavior */
It's surprising that request to mount binderfs is not giving the user a new
instance like tmpfs, devpts, ramfs, and others do.
/* past mistakes */
Other pseudo-filesystems once made the same mistakes of giving back the
same superblock when actually requesting a new mount (cf. devpts's
deprecated "newinstance" option).
We should not make the same mistake. Once we've committed to always giving
back the same superblock in the same IPC namespace with the next kernel
release we will not be able to make that change so better to do it now.
/* kdbusfs */
It was pointed out to me that kdbusfs - which is conceptually closely
related to binderfs - also allowed users to get a private kdbusfs instance
in the same IPC namespace by making each mount of kdbusfs a separate
instance. I think that makes a lot of sense.
Signed-off-by: Christian Brauner
---
drivers/android/binderfs.c | 41 ++
1 file changed, 2 insertions(+), 39 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 6f68d6217eb3..4990d65d4850 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -379,7 +379,7 @@ static int binderfs_fill_super(struct super_block *sb, void
*data, int silent)
struct binderfs_info *info;
int ret = -ENOMEM;
struct inode *inode = NULL;
- struct ipc_namespace *ipc_ns = sb->s_fs_info;
+ struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
get_ipc_ns(ipc_ns);
@@ -450,48 +450,11 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
return ret;
}
-static int binderfs_test_super(struct super_block *sb, void *data)
-{
- struct binderfs_info *info = sb->s_fs_info;
-
- if (info)
- return info->ipc_ns == data;
-
- return 0;
-}
-
-static int binderfs_set_super(struct super_block *sb, void *data)
-{
- sb->s_fs_info = data;
- return set_anon_super(sb, NULL);
-}
-
static struct dentry *binderfs_mount(struct file_system_type *fs_type,
int flags, const char *dev_name,
void *data)
{
- struct super_block *sb;
- struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
-
- if (!ns_capable(ipc_ns->user_ns, CAP_SYS_ADMIN))
- return ERR_PTR(-EPERM);
-
- sb = sget_userns(fs_type, binderfs_test_super, binderfs_set_super,
-flags, ipc_ns->user_ns, ipc_ns);
- if (IS_ERR(sb))
- return ERR_CAST(sb);
-
- if (!sb->s_root) {
- int ret = binderfs_fill_super(sb, data, flags & SB_SILENT ? 1 :
0);
- if (ret) {
- deactivate_locked_super(sb);
- return ERR_PTR(ret);
- }
-
- sb->s_flags |= SB_ACTIVE;
- }
-
- return dget(sb->s_root);
+ return mount_nodev(fs_type, flags, data, binderfs_fill_super);
}
static void binderfs_kill_super(struct super_block *sb)
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] binderfs: rename header to binderfs.h
It doesn't make sense to call the header binder_ctl.h when its sole
existence is tied to binderfs. So give it a sensible name. Users will far
more easily remember binderfs.h than binder_ctl.h.
Signed-off-by: Christian Brauner
---
drivers/android/binderfs.c | 2 +-
include/uapi/linux/android/{binder_ctl.h => binderfs.h} | 0
2 files changed, 1 insertion(+), 1 deletion(-)
rename include/uapi/linux/android/{binder_ctl.h => binderfs.h} (100%)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7496b10532aa..b4ac9ecc16f7 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -30,7 +30,7 @@
#include
#include
#include
-#include
+#include
#include "binder_internal.h"
diff --git a/include/uapi/linux/android/binder_ctl.h
b/include/uapi/linux/android/binderfs.h
similarity index 100%
rename from include/uapi/linux/android/binder_ctl.h
rename to include/uapi/linux/android/binderfs.h
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 2/2] binderfs: reserve devices for initial mount
On Fri, Jan 11, 2019 at 10:18:18AM +0100, Greg KH wrote: > On Wed, Jan 02, 2019 at 12:32:19PM +0100, Christian Brauner wrote: > > The binderfs instance in the initial ipc namespace will always have a > > reserve of 4 binder devices unless explicitly capped by specifying a lower > > value via the "max" mount option. > > This ensures when binder devices are removed (on accident or on purpose) > > they can always be recreated without risking that all minor numbers have > > already been used up. > > > > Cc: Todd Kjos > > Cc: Greg Kroah-Hartman > > Signed-off-by: Christian Brauner > > --- > > v2: > > - patch unchanged > > v1: > > - patch introduced > > v0: > > - patch not present > > --- > > drivers/android/binderfs.c | 7 ++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > What did you make this patch against? It doesn't apply cleanly so I > don't want to take it until we get everything synced up properly > together :( > > I'll apply patch 1/2 now, can you rebase and resend this one? Yup. I'll rebase on top of char-misc-linus now. :) Thanks! Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RESEND PATCH] binderfs: reserve devices for initial mount
The binderfs instance in the initial ipc namespace will always have a reserve of 4 binder devices unless explicitly capped by specifying a lower value via the "max" mount option. This ensures when binder devices are removed (on accident or on purpose) they can always be recreated without risking that all minor numbers have already been used up. Cc: Todd Kjos Cc: Greg Kroah-Hartman Signed-off-by: Christian Brauner --- v2: - patch unchanged v1: - patch introduced v0: - patch not present --- drivers/android/binderfs.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index f6341893b5ba..ad3ad2f7f9f4 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -40,6 +40,8 @@ #define INODE_OFFSET 3 #define INTSTRLEN 21 #define BINDERFS_MAX_MINOR (1U << MINORBITS) +/* Ensure that the initial ipc namespace always has devices available. */ +#define BINDERFS_MAX_MINOR_CAPPED (BINDERFS_MAX_MINOR - 4) static dev_t binderfs_dev; static DEFINE_MUTEX(binderfs_minors_mutex); @@ -127,11 +129,14 @@ static int binderfs_binder_device_create(struct inode *ref_inode, struct inode *inode = NULL; struct super_block *sb = ref_inode->i_sb; struct binderfs_info *info = sb->s_fs_info; + bool use_reserve = (info->ipc_ns == &init_ipc_ns); /* Reserve new minor number for the new device. */ mutex_lock(&binderfs_minors_mutex); if (++info->device_count <= info->mount_opts.max) - minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, + minor = ida_alloc_max(&binderfs_minors, + use_reserve ? BINDERFS_MAX_MINOR : + BINDERFS_MAX_MINOR_CAPPED, GFP_KERNEL); else minor = -ENOSPC; -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RESEND PATCH] binderfs: reserve devices for initial mount
On Fri, Jan 11, 2019 at 11:19:40AM +0100, Christian Brauner wrote: > The binderfs instance in the initial ipc namespace will always have a > reserve of 4 binder devices unless explicitly capped by specifying a lower > value via the "max" mount option. > This ensures when binder devices are removed (on accident or on purpose) > they can always be recreated without risking that all minor numbers have > already been used up. > > Cc: Todd Kjos > Cc: Greg Kroah-Hartman > Signed-off-by: Christian Brauner Cherry-picked this commit on top of char-misc-linus and it applied cleanly. I hope I took the right branch? Thanks! Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] binderfs: handle !CONFIG_IPC_NS builds
kbuild reported a build faile in [1]. This is triggered when CONFIG_IPC_NS is not set. So let's make the use of init_ipc_ns conditional on CONFIG_IPC_NS being set. [1]: https://lists.01.org/pipermail/kbuild-all/2019-January/056903.html Signed-off-by: Christian Brauner --- drivers/android/binderfs.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index ad3ad2f7f9f4..9518e2e7da05 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -129,7 +129,11 @@ static int binderfs_binder_device_create(struct inode *ref_inode, struct inode *inode = NULL; struct super_block *sb = ref_inode->i_sb; struct binderfs_info *info = sb->s_fs_info; +#if defined(CONFIG_IPC_NS) bool use_reserve = (info->ipc_ns == &init_ipc_ns); +#else + bool use_reserve = true; +#endif /* Reserve new minor number for the new device. */ mutex_lock(&binderfs_minors_mutex); -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] binderfs: fix error return code in binderfs_fill_super()
On Wed, Jan 16, 2019 at 03:01:04AM +, Wei Yongjun wrote:
> Fix to return a negative error code -ENOMEM from the new_inode() and
> d_make_root() error handling cases instead of 0, as done elsewhere in
> this function.
>
> Fixes: 3ad20fe393b3 ("binder: implement binderfs")
> Signed-off-by: Wei Yongjun
> ---
> drivers/android/binderfs.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 9518e2e..2bf4b2b 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -519,8 +519,10 @@ static int binderfs_fill_super(struct super_block *sb,
> void *data, int silent)
> sb->s_fs_info = info;
>
> inode = new_inode(sb);
> - if (!inode)
> + if (!inode) {
> + ret = -ENOMEM;
Hey, thanks for the patch. Just a nit:
can you please just do?
ret = -ENOMEM;
inode = new_inode(sb);
This is more consistent with how we do it everywhere else and let's us
avoid shoving ret = -ENOMEM into two places.
Thanks!
Christian
> goto err_without_dentry;
> + }
>
> inode->i_ino = FIRST_INODE;
> inode->i_fop = &simple_dir_operations;
> @@ -530,8 +532,10 @@ static int binderfs_fill_super(struct super_block *sb,
> void *data, int silent)
> set_nlink(inode, 2);
>
> sb->s_root = d_make_root(inode);
> - if (!sb->s_root)
> + if (!sb->s_root) {
> + ret = -ENOMEM;
> goto err_without_dentry;
> + }
>
> ret = binderfs_binder_ctl_create(sb);
> if (ret)
>
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next] binderfs: fix error return code in binderfs_fill_super()
On Wed, Jan 16, 2019 at 07:25:46AM +0100, Christian Brauner wrote:
> On Wed, Jan 16, 2019 at 03:01:04AM +, Wei Yongjun wrote:
> > Fix to return a negative error code -ENOMEM from the new_inode() and
> > d_make_root() error handling cases instead of 0, as done elsewhere in
> > this function.
> >
> > Fixes: 3ad20fe393b3 ("binder: implement binderfs")
This Fixes tag is technically wrong since this codepath was introduced
by a commit that is still sitting in Greg's char-misc-linus branch. Not
sure how to handle that though. Might just leave it.
> > Signed-off-by: Wei Yongjun
> > ---
> > drivers/android/binderfs.c | 8 ++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > index 9518e2e..2bf4b2b 100644
> > --- a/drivers/android/binderfs.c
> > +++ b/drivers/android/binderfs.c
> > @@ -519,8 +519,10 @@ static int binderfs_fill_super(struct super_block *sb,
> > void *data, int silent)
> > sb->s_fs_info = info;
> >
> > inode = new_inode(sb);
> > - if (!inode)
> > + if (!inode) {
> > + ret = -ENOMEM;
>
> Hey, thanks for the patch. Just a nit:
> can you please just do?
>
> ret = -ENOMEM;
> inode = new_inode(sb);
>
> This is more consistent with how we do it everywhere else and let's us
> avoid shoving ret = -ENOMEM into two places.
>
> Thanks!
> Christian
>
> > goto err_without_dentry;
> > + }
> >
> > inode->i_ino = FIRST_INODE;
> > inode->i_fop = &simple_dir_operations;
> > @@ -530,8 +532,10 @@ static int binderfs_fill_super(struct super_block *sb,
> > void *data, int silent)
> > set_nlink(inode, 2);
> >
> > sb->s_root = d_make_root(inode);
> > - if (!sb->s_root)
> > + if (!sb->s_root) {
> > + ret = -ENOMEM;
> > goto err_without_dentry;
> > + }
> >
> > ret = binderfs_binder_ctl_create(sb);
> > if (ret)
> >
> >
> >
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next v2] binderfs: fix error return code in binderfs_fill_super()
On Wed, Jan 16, 2019 at 08:34:02AM +, Wei Yongjun wrote:
> Fix to return a negative error code -ENOMEM from the new_inode() and
> d_make_root() error handling cases instead of 0, as done elsewhere in
> this function.
>
> Fixes: 3ad20fe393b3 ("binder: implement binderfs")
This should be:
Fixes: 849d540ddfcd ("binderfs: implement "max" mount option")
If you have added that feel free to also add my Reviewed-by.
Thanks!
Christian
> Signed-off-by: Wei Yongjun
> ---
> v1 -> v2: move 'ret = -ENOMEM' out of if
> ---
> drivers/android/binderfs.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> index 9518e2e..e4ff4c3 100644
> --- a/drivers/android/binderfs.c
> +++ b/drivers/android/binderfs.c
> @@ -518,6 +518,7 @@ static int binderfs_fill_super(struct super_block *sb,
> void *data, int silent)
>
> sb->s_fs_info = info;
>
> + ret = -ENOMEM;
> inode = new_inode(sb);
> if (!inode)
> goto err_without_dentry;
>
>
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH -next v2] binderfs: fix error return code in binderfs_fill_super()
On Wed, Jan 16, 2019 at 09:41:08AM +0100, Christian Brauner wrote:
> On Wed, Jan 16, 2019 at 08:34:02AM +, Wei Yongjun wrote:
> > Fix to return a negative error code -ENOMEM from the new_inode() and
> > d_make_root() error handling cases instead of 0, as done elsewhere in
> > this function.
> >
> > Fixes: 3ad20fe393b3 ("binder: implement binderfs")
>
> This should be:
>
> Fixes: 849d540ddfcd ("binderfs: implement "max" mount option")
>
> If you have added that feel free to also add my Reviewed-by.
Also, this does need to go into char-misc-linus and not -next so you can
drop the -next prefix from the [PATCH ...] prefix. :) Greg will do the
right thing in any case but it's just clearer if you don't add it. :)
Thanks!
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] selftests: add binderfs selftests
This adds the promised selftest for binderfs. It will verify the following
things:
- binderfs mounting works
- binder device allocation works
- performing a binder ioctl() request through a binderfs device works
- binder device removal works
- binder-control removal fails
- binderfs unmounting works
Cc: Todd Kjos
Signed-off-by: Christian Brauner
---
tools/testing/selftests/Makefile | 1 +
.../selftests/filesystems/binderfs/.gitignore | 1 +
.../selftests/filesystems/binderfs/Makefile | 6 +
.../filesystems/binderfs/binderfs_test.c | 120 ++
.../selftests/filesystems/binderfs/config | 3 +
5 files changed, 131 insertions(+)
create mode 100644 tools/testing/selftests/filesystems/binderfs/.gitignore
create mode 100644 tools/testing/selftests/filesystems/binderfs/Makefile
create mode 100644 tools/testing/selftests/filesystems/binderfs/binderfs_test.c
create mode 100644 tools/testing/selftests/filesystems/binderfs/config
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 1a2bd15c5b6e..400ee81a3043 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
TARGETS += efivarfs
TARGETS += exec
TARGETS += filesystems
+TARGETS += filesystems/binderfs
TARGETS += firmware
TARGETS += ftrace
TARGETS += futex
diff --git a/tools/testing/selftests/filesystems/binderfs/.gitignore
b/tools/testing/selftests/filesystems/binderfs/.gitignore
new file mode 100644
index ..8a5d9bf63dd4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
@@ -0,0 +1 @@
+binderfs_test
diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
b/tools/testing/selftests/filesystems/binderfs/Makefile
new file mode 100644
index ..58cb659b56b4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+CFLAGS += -I../../../../../usr/include/
+TEST_GEN_PROGS := binderfs_test
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
new file mode 100644
index ..ca4d9b616e84
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "../../kselftest.h"
+
+int main(int argc, char *argv[])
+{
+ int fd, ret, saved_errno;
+ size_t len;
+ struct binderfs_device device = { 0 };
+ struct binder_version version = { 0 };
+
+ ret = unshare(CLONE_NEWNS);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s - Failed to unshare mount namespace\n",
+ strerror(errno));
+
+ ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s - Failed to mount / as private\n",
+ strerror(errno));
+
+ ret = mkdir("/dev/binderfs", 0755);
+ if (ret < 0 && errno != EEXIST)
+ ksft_exit_fail_msg(
+ "%s - Failed to create binderfs mountpoint\n",
+ strerror(errno));
+
+ ret = mount(NULL, "/dev/binderfs", "binder", 0, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
+ strerror(errno));
+
+ /* binderfs mount test passed */
+ ksft_inc_pass_cnt();
+
+ memcpy(device.name, "my-binder", strlen("my-binder"));
+
+ fd = open("/dev/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
+ if (fd < 0)
+ ksft_exit_fail_msg(
+ "%s - Failed to open binder-control device\n",
+ strerror(errno));
+
+ ret = ioctl(fd, BINDER_CTL_ADD, &device);
+ saved_errno = errno;
+ close(fd);
+ errno = saved_errno;
+ if (ret < 0)
+ ksft_exit_fail_msg(
+ "%s - Failed to allocate new binder device\n",
+ strerror(errno));
+
+ printf("Allocated new binder device with major %d, minor %d, and name
%s\n",
+ device.major, device.minor, device.name);
+
+ /* binder device allocation test passed */
+ ksft_inc_pass_cnt();
+
+ fd = open("/dev/binderfs/my-binder", O_CLOEXEC | O_RDONLY);
+ if (fd < 0)
+ ksft_exit_fail_msg("%s - Failed to open my-binder device\n",
+ strerror(errno));
+
+ ret = ioctl(fd, BINDER_VER
Re: [PATCH] selftests: add binderfs selftests
On January 16, 2019 5:54:50 PM GMT+02:00, Greg KH
wrote:
>On Wed, Jan 16, 2019 at 02:19:03PM +0100, Christian Brauner wrote:
>> This adds the promised selftest for binderfs. It will verify the
>following
>> things:
>> - binderfs mounting works
>> - binder device allocation works
>> - performing a binder ioctl() request through a binderfs device works
>> - binder device removal works
>> - binder-control removal fails
>> - binderfs unmounting works
>>
>> Cc: Todd Kjos
>> Signed-off-by: Christian Brauner
>> ---
>> tools/testing/selftests/Makefile | 1 +
>> .../selftests/filesystems/binderfs/.gitignore | 1 +
>> .../selftests/filesystems/binderfs/Makefile | 6 +
>> .../filesystems/binderfs/binderfs_test.c | 120
>++
>> .../selftests/filesystems/binderfs/config | 3 +
>> 5 files changed, 131 insertions(+)
>> create mode 100644
>tools/testing/selftests/filesystems/binderfs/.gitignore
>> create mode 100644
>tools/testing/selftests/filesystems/binderfs/Makefile
>> create mode 100644
>tools/testing/selftests/filesystems/binderfs/binderfs_test.c
>> create mode 100644
>tools/testing/selftests/filesystems/binderfs/config
>>
>> diff --git a/tools/testing/selftests/Makefile
>b/tools/testing/selftests/Makefile
>> index 1a2bd15c5b6e..400ee81a3043 100644
>> --- a/tools/testing/selftests/Makefile
>> +++ b/tools/testing/selftests/Makefile
>> @@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
>> TARGETS += efivarfs
>> TARGETS += exec
>> TARGETS += filesystems
>> +TARGETS += filesystems/binderfs
>> TARGETS += firmware
>> TARGETS += ftrace
>> TARGETS += futex
>> diff --git a/tools/testing/selftests/filesystems/binderfs/.gitignore
>b/tools/testing/selftests/filesystems/binderfs/.gitignore
>> new file mode 100644
>> index ..8a5d9bf63dd4
>> --- /dev/null
>> +++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
>> @@ -0,0 +1 @@
>> +binderfs_test
>> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
>b/tools/testing/selftests/filesystems/binderfs/Makefile
>> new file mode 100644
>> index ..58cb659b56b4
>> --- /dev/null
>> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +CFLAGS += -I../../../../../usr/include/
>> +TEST_GEN_PROGS := binderfs_test
>> +
>> +include ../../lib.mk
>> diff --git
>a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
>b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
>> new file mode 100644
>> index ..ca4d9b616e84
>> --- /dev/null
>> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
>> @@ -0,0 +1,120 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#define _GNU_SOURCE
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include "../../kselftest.h"
>> +
>> +int main(int argc, char *argv[])
>> +{
>> +int fd, ret, saved_errno;
>> +size_t len;
>> +struct binderfs_device device = { 0 };
>> +struct binder_version version = { 0 };
>> +
>> +ret = unshare(CLONE_NEWNS);
>> +if (ret < 0)
>> +ksft_exit_fail_msg("%s - Failed to unshare mount namespace\n",
>> + strerror(errno));
>> +
>> +ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
>> +if (ret < 0)
>> +ksft_exit_fail_msg("%s - Failed to mount / as private\n",
>> + strerror(errno));
>> +
>> +ret = mkdir("/dev/binderfs", 0755);
>> +if (ret < 0 && errno != EEXIST)
>> +ksft_exit_fail_msg(
>> +"%s - Failed to create binderfs mountpoint\n",
>> +strerror(errno));
>> +
>> +ret = mount(NULL, "/dev/binderfs", "binder", 0, 0);
>> +if (ret < 0)
>> +ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
>> + strerror(errno));
>
>Can you check first to see if the kernel under test really even has
>binderfs in it? If not, you need to just
Hm, I thought that's what the "config" file was for?
E.g. a conditional compile or is this just a hint to the
user what's needed to run the test?
Thanks!
Christian
Christian
abort the test, not fail it,
>so as to allow newer versions of kselftests to run on older kernels.
>
>thanks,
>
>greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] selftests: add binderfs selftests
On January 16, 2019 6:42:20 PM GMT+02:00, Greg KH
wrote:
>On Wed, Jan 16, 2019 at 06:21:12PM +0200, Christian Brauner wrote:
>> On January 16, 2019 5:54:50 PM GMT+02:00, Greg KH
> wrote:
>> >On Wed, Jan 16, 2019 at 02:19:03PM +0100, Christian Brauner wrote:
>> >> This adds the promised selftest for binderfs. It will verify the
>> >following
>> >> things:
>> >> - binderfs mounting works
>> >> - binder device allocation works
>> >> - performing a binder ioctl() request through a binderfs device
>works
>> >> - binder device removal works
>> >> - binder-control removal fails
>> >> - binderfs unmounting works
>> >>
>> >> Cc: Todd Kjos
>> >> Signed-off-by: Christian Brauner
>> >> ---
>> >> tools/testing/selftests/Makefile | 1 +
>> >> .../selftests/filesystems/binderfs/.gitignore | 1 +
>> >> .../selftests/filesystems/binderfs/Makefile | 6 +
>> >> .../filesystems/binderfs/binderfs_test.c | 120
>> >++
>> >> .../selftests/filesystems/binderfs/config | 3 +
>> >> 5 files changed, 131 insertions(+)
>> >> create mode 100644
>> >tools/testing/selftests/filesystems/binderfs/.gitignore
>> >> create mode 100644
>> >tools/testing/selftests/filesystems/binderfs/Makefile
>> >> create mode 100644
>> >tools/testing/selftests/filesystems/binderfs/binderfs_test.c
>> >> create mode 100644
>> >tools/testing/selftests/filesystems/binderfs/config
>> >>
>> >> diff --git a/tools/testing/selftests/Makefile
>> >b/tools/testing/selftests/Makefile
>> >> index 1a2bd15c5b6e..400ee81a3043 100644
>> >> --- a/tools/testing/selftests/Makefile
>> >> +++ b/tools/testing/selftests/Makefile
>> >> @@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
>> >> TARGETS += efivarfs
>> >> TARGETS += exec
>> >> TARGETS += filesystems
>> >> +TARGETS += filesystems/binderfs
>> >> TARGETS += firmware
>> >> TARGETS += ftrace
>> >> TARGETS += futex
>> >> diff --git
>a/tools/testing/selftests/filesystems/binderfs/.gitignore
>> >b/tools/testing/selftests/filesystems/binderfs/.gitignore
>> >> new file mode 100644
>> >> index ..8a5d9bf63dd4
>> >> --- /dev/null
>> >> +++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
>> >> @@ -0,0 +1 @@
>> >> +binderfs_test
>> >> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
>> >b/tools/testing/selftests/filesystems/binderfs/Makefile
>> >> new file mode 100644
>> >> index ..58cb659b56b4
>> >> --- /dev/null
>> >> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
>> >> @@ -0,0 +1,6 @@
>> >> +# SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +CFLAGS += -I../../../../../usr/include/
>> >> +TEST_GEN_PROGS := binderfs_test
>> >> +
>> >> +include ../../lib.mk
>> >> diff --git
>> >a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
>> >b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
>> >> new file mode 100644
>> >> index ..ca4d9b616e84
>> >> --- /dev/null
>> >> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
>> >> @@ -0,0 +1,120 @@
>> >> +// SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +#define _GNU_SOURCE
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include
>> >> +#include "../../kselftest.h"
>> >> +
>> >> +int main(int argc, char *argv[])
>> >> +{
>> >> + int fd, ret, saved_errno;
>> >> + size_t len;
>> >> + struct binderfs_device device = { 0 };
>> >> + struct binder_version version = { 0 };
>> >> +
>> >> + ret = unshare(CLONE_NEWNS);
>> >> + if (ret < 0)
>> >> + ksft_exit_fail_msg("%s - Failed to unshare mount namespace\n",
>> >> +strerror(errno));
>> >
Re: [PATCH] selftests: add binderfs selftests
On Wed, Jan 16, 2019 at 06:57:10PM +0200, Christian Brauner wrote: > On January 16, 2019 6:42:20 PM GMT+02:00, Greg KH > wrote: > >On Wed, Jan 16, 2019 at 06:21:12PM +0200, Christian Brauner wrote: > >> On January 16, 2019 5:54:50 PM GMT+02:00, Greg KH > > wrote: > >> >On Wed, Jan 16, 2019 at 02:19:03PM +0100, Christian Brauner wrote: > >> >> This adds the promised selftest for binderfs. It will verify the > >> >following > >> >> things: > >> >> - binderfs mounting works > >> >> - binder device allocation works > >> >> - performing a binder ioctl() request through a binderfs device > >works > >> >> - binder device removal works > >> >> - binder-control removal fails > >> >> - binderfs unmounting works > >> >> > >> >> Cc: Todd Kjos > >> >> Signed-off-by: Christian Brauner > >> >> --- > >> >> tools/testing/selftests/Makefile | 1 + > >> >> .../selftests/filesystems/binderfs/.gitignore | 1 + > >> >> .../selftests/filesystems/binderfs/Makefile | 6 + > >> >> .../filesystems/binderfs/binderfs_test.c | 120 > >> >++ > >> >> .../selftests/filesystems/binderfs/config | 3 + > >> >> 5 files changed, 131 insertions(+) > >> >> create mode 100644 > >> >tools/testing/selftests/filesystems/binderfs/.gitignore > >> >> create mode 100644 > >> >tools/testing/selftests/filesystems/binderfs/Makefile > >> >> create mode 100644 > >> >tools/testing/selftests/filesystems/binderfs/binderfs_test.c > >> >> create mode 100644 > >> >tools/testing/selftests/filesystems/binderfs/config > >> >> > >> >> diff --git a/tools/testing/selftests/Makefile > >> >b/tools/testing/selftests/Makefile > >> >> index 1a2bd15c5b6e..400ee81a3043 100644 > >> >> --- a/tools/testing/selftests/Makefile > >> >> +++ b/tools/testing/selftests/Makefile > >> >> @@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf > >> >> TARGETS += efivarfs > >> >> TARGETS += exec > >> >> TARGETS += filesystems > >> >> +TARGETS += filesystems/binderfs > >> >> TARGETS += firmware > >> >> TARGETS += ftrace > >> >> TARGETS += futex > >> >> diff --git > >a/tools/testing/selftests/filesystems/binderfs/.gitignore > >> >b/tools/testing/selftests/filesystems/binderfs/.gitignore > >> >> new file mode 100644 > >> >> index ..8a5d9bf63dd4 > >> >> --- /dev/null > >> >> +++ b/tools/testing/selftests/filesystems/binderfs/.gitignore > >> >> @@ -0,0 +1 @@ > >> >> +binderfs_test > >> >> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile > >> >b/tools/testing/selftests/filesystems/binderfs/Makefile > >> >> new file mode 100644 > >> >> index ..58cb659b56b4 > >> >> --- /dev/null > >> >> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile > >> >> @@ -0,0 +1,6 @@ > >> >> +# SPDX-License-Identifier: GPL-2.0 > >> >> + > >> >> +CFLAGS += -I../../../../../usr/include/ > >> >> +TEST_GEN_PROGS := binderfs_test > >> >> + > >> >> +include ../../lib.mk > >> >> diff --git > >> >a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c > >> >b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c > >> >> new file mode 100644 > >> >> index ..ca4d9b616e84 > >> >> --- /dev/null > >> >> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c > >> >> @@ -0,0 +1,120 @@ > >> >> +// SPDX-License-Identifier: GPL-2.0 > >> >> + > >> >> +#define _GNU_SOURCE > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include > >> >> +#include "../../kselftest.h" > >> >> + &
[PATCH v1] selftests: add binderfs selftests
This adds the promised selftest for binderfs. It will verify the following
things:
- binderfs mounting works
- binder device allocation works
- performing a binder ioctl() request through a binderfs device works
- binder device removal works
- binder-control removal fails
- binderfs unmounting works
Cc: Todd Kjos
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- check for ENODEV on mount failure to detect whether binderfs is
available
If it is not, skip the test and exit with success.
---
tools/testing/selftests/Makefile | 1 +
.../selftests/filesystems/binderfs/.gitignore | 1 +
.../selftests/filesystems/binderfs/Makefile | 6 +
.../filesystems/binderfs/binderfs_test.c | 126 ++
.../selftests/filesystems/binderfs/config | 3 +
5 files changed, 137 insertions(+)
create mode 100644 tools/testing/selftests/filesystems/binderfs/.gitignore
create mode 100644 tools/testing/selftests/filesystems/binderfs/Makefile
create mode 100644 tools/testing/selftests/filesystems/binderfs/binderfs_test.c
create mode 100644 tools/testing/selftests/filesystems/binderfs/config
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 1a2bd15c5b6e..400ee81a3043 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
TARGETS += efivarfs
TARGETS += exec
TARGETS += filesystems
+TARGETS += filesystems/binderfs
TARGETS += firmware
TARGETS += ftrace
TARGETS += futex
diff --git a/tools/testing/selftests/filesystems/binderfs/.gitignore
b/tools/testing/selftests/filesystems/binderfs/.gitignore
new file mode 100644
index ..8a5d9bf63dd4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
@@ -0,0 +1 @@
+binderfs_test
diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
b/tools/testing/selftests/filesystems/binderfs/Makefile
new file mode 100644
index ..58cb659b56b4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+CFLAGS += -I../../../../../usr/include/
+TEST_GEN_PROGS := binderfs_test
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
new file mode 100644
index ..34361efcb9c8
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -0,0 +1,126 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "../../kselftest.h"
+
+int main(int argc, char *argv[])
+{
+ int fd, ret, saved_errno;
+ size_t len;
+ struct binderfs_device device = { 0 };
+ struct binder_version version = { 0 };
+
+ ret = unshare(CLONE_NEWNS);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s - Failed to unshare mount namespace\n",
+ strerror(errno));
+
+ ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s - Failed to mount / as private\n",
+ strerror(errno));
+
+ ret = mkdir("/dev/binderfs", 0755);
+ if (ret < 0 && errno != EEXIST)
+ ksft_exit_fail_msg(
+ "%s - Failed to create binderfs mountpoint\n",
+ strerror(errno));
+
+ ret = mount(NULL, "/dev/binderfs", "binder", 0, 0);
+ if (ret < 0) {
+ if (errno != ENODEV)
+ ksft_exit_fail_msg("%s - Failed to mount binderfs\n",
+ strerror(errno));
+
+ ksft_test_result_skip(
+ "The Android binderfs filesystem is not available\n");
+ ksft_exit_pass();
+ }
+
+ /* binderfs mount test passed */
+ ksft_inc_pass_cnt();
+
+ memcpy(device.name, "my-binder", strlen("my-binder"));
+
+ fd = open("/dev/binderfs/binder-control", O_RDONLY | O_CLOEXEC);
+ if (fd < 0)
+ ksft_exit_fail_msg(
+ "%s - Failed to open binder-control device\n",
+ strerror(errno));
+
+ ret = ioctl(fd, BINDER_CTL_ADD, &device);
+ saved_errno = errno;
+ close(fd);
+ errno = saved_errno;
+ if (ret < 0)
+ ksft_exit_fail_msg(
+ "%s - Failed to allocate new binder device\n",
+ strerror(errno));
+
+ printf("Allocated new binder device with major %d, minor %d, and name
%s\n",
+ device.major, devi
Re: [PATCH v1] selftests: add binderfs selftests
On Wed, Jan 16, 2019 at 11:40:50PM +0100, Greg KH wrote:
> On Wed, Jan 16, 2019 at 11:27:45PM +0100, Christian Brauner wrote:
> > This adds the promised selftest for binderfs. It will verify the following
> > things:
> > - binderfs mounting works
> > - binder device allocation works
> > - performing a binder ioctl() request through a binderfs device works
> > - binder device removal works
> > - binder-control removal fails
> > - binderfs unmounting works
> >
> > Cc: Todd Kjos
> > Signed-off-by: Christian Brauner
> > ---
> > /* Changelog */
> >
> > v1:
> > - check for ENODEV on mount failure to detect whether binderfs is
> > available
> > If it is not, skip the test and exit with success.
> > ---
> > tools/testing/selftests/Makefile | 1 +
> > .../selftests/filesystems/binderfs/.gitignore | 1 +
> > .../selftests/filesystems/binderfs/Makefile | 6 +
> > .../filesystems/binderfs/binderfs_test.c | 126 ++
> > .../selftests/filesystems/binderfs/config | 3 +
> > 5 files changed, 137 insertions(+)
> > create mode 100644 tools/testing/selftests/filesystems/binderfs/.gitignore
> > create mode 100644 tools/testing/selftests/filesystems/binderfs/Makefile
> > create mode 100644
> > tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > create mode 100644 tools/testing/selftests/filesystems/binderfs/config
> >
> > diff --git a/tools/testing/selftests/Makefile
> > b/tools/testing/selftests/Makefile
> > index 1a2bd15c5b6e..400ee81a3043 100644
> > --- a/tools/testing/selftests/Makefile
> > +++ b/tools/testing/selftests/Makefile
> > @@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
> > TARGETS += efivarfs
> > TARGETS += exec
> > TARGETS += filesystems
> > +TARGETS += filesystems/binderfs
> > TARGETS += firmware
> > TARGETS += ftrace
> > TARGETS += futex
> > diff --git a/tools/testing/selftests/filesystems/binderfs/.gitignore
> > b/tools/testing/selftests/filesystems/binderfs/.gitignore
> > new file mode 100644
> > index ..8a5d9bf63dd4
> > --- /dev/null
> > +++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
> > @@ -0,0 +1 @@
> > +binderfs_test
> > diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
> > b/tools/testing/selftests/filesystems/binderfs/Makefile
> > new file mode 100644
> > index ..58cb659b56b4
> > --- /dev/null
> > +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +CFLAGS += -I../../../../../usr/include/
> > +TEST_GEN_PROGS := binderfs_test
> > +
> > +include ../../lib.mk
> > diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > new file mode 100644
> > index ..34361efcb9c8
> > --- /dev/null
> > +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > @@ -0,0 +1,126 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define _GNU_SOURCE
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "../../kselftest.h"
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + int fd, ret, saved_errno;
> > + size_t len;
> > + struct binderfs_device device = { 0 };
> > + struct binder_version version = { 0 };
> > +
> > + ret = unshare(CLONE_NEWNS);
> > + if (ret < 0)
> > + ksft_exit_fail_msg("%s - Failed to unshare mount namespace\n",
> > + strerror(errno));
> > +
> > + ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
> > + if (ret < 0)
> > + ksft_exit_fail_msg("%s - Failed to mount / as private\n",
> > + strerror(errno));
> > +
> > + ret = mkdir("/dev/binderfs", 0755);
> > + if (ret < 0 && errno != EEXIST)
> > + ksft_exit_fail_msg(
> > + "%s - Failed to create binderfs mountpoint\n",
> > + strerror(errno));
> > +
> > + ret = mount(NULL, "/dev/binderfs", "binder", 0, 0);
> > + if (ret < 0) {
> > + if (
Re: [PATCH v1] selftests: add binderfs selftests
On Wed, Jan 16, 2019 at 04:11:55PM -0700, shuah wrote:
> Hi Christian,
>
> On 1/16/19 3:27 PM, Christian Brauner wrote:
> > This adds the promised selftest for binderfs. It will verify the following
> > things:
> > - binderfs mounting works
> > - binder device allocation works
> > - performing a binder ioctl() request through a binderfs device works
> > - binder device removal works
> > - binder-control removal fails
> > - binderfs unmounting works
> >
>
> Thanks for the patch. A few comments below.
Thanks for the review!
>
> > Cc: Todd Kjos
> > Signed-off-by: Christian Brauner
> > ---
> > /* Changelog */
> >
> > v1:
> > - check for ENODEV on mount failure to detect whether binderfs is
> >available
> >If it is not, skip the test and exit with success.
> > ---
> > tools/testing/selftests/Makefile | 1 +
> > .../selftests/filesystems/binderfs/.gitignore | 1 +
> > .../selftests/filesystems/binderfs/Makefile | 6 +
> > .../filesystems/binderfs/binderfs_test.c | 126 ++
> > .../selftests/filesystems/binderfs/config | 3 +
> > 5 files changed, 137 insertions(+)
> > create mode 100644 tools/testing/selftests/filesystems/binderfs/.gitignore
> > create mode 100644 tools/testing/selftests/filesystems/binderfs/Makefile
> > create mode 100644
> > tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > create mode 100644 tools/testing/selftests/filesystems/binderfs/config
> >
> > diff --git a/tools/testing/selftests/Makefile
> > b/tools/testing/selftests/Makefile
> > index 1a2bd15c5b6e..400ee81a3043 100644
> > --- a/tools/testing/selftests/Makefile
> > +++ b/tools/testing/selftests/Makefile
> > @@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
> > TARGETS += efivarfs
> > TARGETS += exec
> > TARGETS += filesystems
> > +TARGETS += filesystems/binderfs
> > TARGETS += firmware
> > TARGETS += ftrace
> > TARGETS += futex
> > diff --git a/tools/testing/selftests/filesystems/binderfs/.gitignore
> > b/tools/testing/selftests/filesystems/binderfs/.gitignore
> > new file mode 100644
> > index ..8a5d9bf63dd4
> > --- /dev/null
> > +++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
> > @@ -0,0 +1 @@
> > +binderfs_test
> > diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
> > b/tools/testing/selftests/filesystems/binderfs/Makefile
> > new file mode 100644
> > index ..58cb659b56b4
> > --- /dev/null
> > +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +CFLAGS += -I../../../../../usr/include/
> > +TEST_GEN_PROGS := binderfs_test
> > +
> > +include ../../lib.mk
> > diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > new file mode 100644
> > index ..34361efcb9c8
> > --- /dev/null
> > +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> > @@ -0,0 +1,126 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#define _GNU_SOURCE
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include "../../kselftest.h"
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + int fd, ret, saved_errno;
> > + size_t len;
> > + struct binderfs_device device = { 0 };
> > + struct binder_version version = { 0 };
> > +
> > + ret = unshare(CLONE_NEWNS);
> > + if (ret < 0)
> > + ksft_exit_fail_msg("%s - Failed to unshare mount namespace\n",
> > + strerror(errno));
> > +
> > + ret = mount(NULL, "/", NULL, MS_REC | MS_PRIVATE, 0);
> > + if (ret < 0)
> > + ksft_exit_fail_msg("%s - Failed to mount / as private\n",
> > + strerror(errno));
> > +
>
> If this test requires user to be root, please add check for root uid and
> do a skip the test with kselftest skip code.
I think in addition to privileged tests I'll also add unprivileged tests
when mounting binderfs inside a user namespace so that we a) can run
this test as an unprivileged user and b) test that binde
[PATCH v2] selftests: add binderfs selftests
This adds the promised selftest for binderfs. It will verify the following
things:
- binderfs mounting works
- binder device allocation works
- performing a binder ioctl() request through a binderfs device works
- binder device removal works
- binder-control removal fails
- binderfs unmounting works
The tests are performed both privileged and unprivileged. The latter
verifies that binderfs behaves correctly in user namespaces.
Cc: Todd Kjos
Signed-off-by: Christian Brauner
---
/* Changelog */
v2:
- make failure to create /dev/binderfs directory fatal in all circumstances
- make tests run in user namespace to test whether binderfs can be mounted
in user namespaces and so that unprivileged users can run the tests
- use ksft_exit_skip()
v1:
- check for ENODEV on mount failure to detect whether binderfs is
available
If it is not, skip the test and exit with success.
---
tools/testing/selftests/Makefile | 1 +
.../selftests/filesystems/binderfs/.gitignore | 1 +
.../selftests/filesystems/binderfs/Makefile | 6 +
.../filesystems/binderfs/binderfs_test.c | 270 ++
.../selftests/filesystems/binderfs/config | 3 +
5 files changed, 281 insertions(+)
create mode 100644 tools/testing/selftests/filesystems/binderfs/.gitignore
create mode 100644 tools/testing/selftests/filesystems/binderfs/Makefile
create mode 100644 tools/testing/selftests/filesystems/binderfs/binderfs_test.c
create mode 100644 tools/testing/selftests/filesystems/binderfs/config
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 1a2bd15c5b6e..400ee81a3043 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
TARGETS += efivarfs
TARGETS += exec
TARGETS += filesystems
+TARGETS += filesystems/binderfs
TARGETS += firmware
TARGETS += ftrace
TARGETS += futex
diff --git a/tools/testing/selftests/filesystems/binderfs/.gitignore
b/tools/testing/selftests/filesystems/binderfs/.gitignore
new file mode 100644
index ..8a5d9bf63dd4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
@@ -0,0 +1 @@
+binderfs_test
diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
b/tools/testing/selftests/filesystems/binderfs/Makefile
new file mode 100644
index ..58cb659b56b4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+CFLAGS += -I../../../../../usr/include/
+TEST_GEN_PROGS := binderfs_test
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
new file mode 100644
index ..988f54f2d3b0
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -0,0 +1,270 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "../../kselftest.h"
+
+static ssize_t write_nointr(int fd, const void *buf, size_t count)
+{
+ ssize_t ret;
+again:
+ ret = write(fd, buf, count);
+ if (ret < 0 && errno == EINTR)
+ goto again;
+
+ return ret;
+}
+
+static void write_to_file(const char *filename, const void *buf, size_t count,
+ int allowed_errno)
+{
+ int fd, saved_errno;
+ ssize_t ret;
+
+ fd = open(filename, O_WRONLY | O_CLOEXEC);
+ if (fd < 0)
+ ksft_exit_fail_msg("%s - Failed to open file %s\n",
+ strerror(errno), filename);
+
+ ret = write_nointr(fd, buf, count);
+ if (ret < 0) {
+ if (allowed_errno && (errno == allowed_errno)) {
+ close(fd);
+ return;
+ }
+
+ goto on_error;
+ }
+
+ if ((size_t)ret != count)
+ goto on_error;
+
+ close(fd);
+ return;
+
+on_error:
+ saved_errno = errno;
+ close(fd);
+ errno = saved_errno;
+
+ ksft_exit_fail_msg("%s - Failed to write to file %s\n", strerror(errno),
+ filename);
+}
+
+static void change_to_userns(void)
+{
+ int ret;
+ uid_t uid;
+ gid_t gid;
+ /* {g,u}id_map files only allow a max of 4096 bytes written to them */
+ char idmap[4096];
+
+ uid = getuid();
+ gid = getgid();
+
+ ret = unshare(CLONE_NEWUSER);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s - Failed to unshare user namespace\n",
+ strerror(errno));
+
+ write_to_file("/proc/self/setgroups", "deny", strlen("deny"), ENOENT);
+
+ ret =
Re: [PATCH v2] selftests: add binderfs selftests
On Thu, Jan 17, 2019 at 11:55:49AM +0100, Greg KH wrote:
> On Thu, Jan 17, 2019 at 11:28:21AM +0100, Christian Brauner wrote:
> > This adds the promised selftest for binderfs. It will verify the following
> > things:
> > - binderfs mounting works
> > - binder device allocation works
> > - performing a binder ioctl() request through a binderfs device works
> > - binder device removal works
> > - binder-control removal fails
> > - binderfs unmounting works
> >
> > The tests are performed both privileged and unprivileged. The latter
> > verifies that binderfs behaves correctly in user namespaces.
> >
> > Cc: Todd Kjos
> > Signed-off-by: Christian Brauner
>
> Now I am just nit-picking:
I would've been surprised if someone wouldn't have. :)
>
> > +static void write_to_file(const char *filename, const void *buf, size_t
> > count,
> > + int allowed_errno)
> > +{
> > + int fd, saved_errno;
> > + ssize_t ret;
> > +
> > + fd = open(filename, O_WRONLY | O_CLOEXEC);
> > + if (fd < 0)
> > + ksft_exit_fail_msg("%s - Failed to open file %s\n",
> > + strerror(errno), filename);
> > +
> > + ret = write_nointr(fd, buf, count);
> > + if (ret < 0) {
> > + if (allowed_errno && (errno == allowed_errno)) {
> > + close(fd);
> > + return;
> > + }
> > +
> > + goto on_error;
> > + }
> > +
> > + if ((size_t)ret != count)
> > + goto on_error;
>
> if ret < count, you are supposed to try again with the remaining data,
> right? A write() implementation can just take one byte at a time.
>
> Yes, for your example here that isn't going to happen as the kernel
> should be handling a larger buffer than that, but note that if you use
> this code elsewhere, it's not really correct because:
Yeah, I know you should retry but for the test I'm not really willing to
keep track of where I was in the buffer and so on. If the test fails
because of that I'd say to count it as failed and move on.
>
> > +
> > + close(fd);
> > + return;
> > +
> > +on_error:
> > + saved_errno = errno;
>
> If you do a short write, there is no error, so who knows what errno you
> end up with here.
>
> Anyway, just one other minor question that might be relevant:
>
> > + printf("Allocated new binder device with major %d, minor %d, and name
> > %s\n",
> > + device.major, device.minor, device.name);
>
> Aren't tests supposed to print their output in some sort of normal
> format? I thought you were supposed to use ksft_print_msg() so that
> tools can properly parse the output.
I can switch the printf()s over to ksft_print_msg().
Thanks!
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v3] selftests: add binderfs selftests
This adds the promised selftest for binderfs. It will verify the following
things:
- binderfs mounting works
- binder device allocation works
- performing a binder ioctl() request through a binderfs device works
- binder device removal works
- binder-control removal fails
- binderfs unmounting works
The tests are performed both privileged and unprivileged. The latter
verifies that binderfs behaves correctly in user namespaces.
Cc: Todd Kjos
Signed-off-by: Christian Brauner
---
/* Changelog */
v3:
- s/printf/ksft_print_msg/g
- do not report misleading errno on short write
v2:
- make failure to create /dev/binderfs directory fatal in all circumstances
- make tests run in user namespace to test whether binderfs can be mounted
in user namespaces and so that unprivileged users can run the tests
- use ksft_exit_skip()
v1:
- check for ENODEV on mount failure to detect whether binderfs is
available
If it is not, skip the test and exit with success.
---
tools/testing/selftests/Makefile | 1 +
.../selftests/filesystems/binderfs/.gitignore | 1 +
.../selftests/filesystems/binderfs/Makefile | 6 +
.../filesystems/binderfs/binderfs_test.c | 275 ++
.../selftests/filesystems/binderfs/config | 3 +
5 files changed, 286 insertions(+)
create mode 100644 tools/testing/selftests/filesystems/binderfs/.gitignore
create mode 100644 tools/testing/selftests/filesystems/binderfs/Makefile
create mode 100644 tools/testing/selftests/filesystems/binderfs/binderfs_test.c
create mode 100644 tools/testing/selftests/filesystems/binderfs/config
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 1a2bd15c5b6e..400ee81a3043 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
TARGETS += efivarfs
TARGETS += exec
TARGETS += filesystems
+TARGETS += filesystems/binderfs
TARGETS += firmware
TARGETS += ftrace
TARGETS += futex
diff --git a/tools/testing/selftests/filesystems/binderfs/.gitignore
b/tools/testing/selftests/filesystems/binderfs/.gitignore
new file mode 100644
index ..8a5d9bf63dd4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
@@ -0,0 +1 @@
+binderfs_test
diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
b/tools/testing/selftests/filesystems/binderfs/Makefile
new file mode 100644
index ..58cb659b56b4
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/Makefile
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: GPL-2.0
+
+CFLAGS += -I../../../../../usr/include/
+TEST_GEN_PROGS := binderfs_test
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
new file mode 100644
index ..8c2ed962e1c7
--- /dev/null
+++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "../../kselftest.h"
+
+static ssize_t write_nointr(int fd, const void *buf, size_t count)
+{
+ ssize_t ret;
+again:
+ ret = write(fd, buf, count);
+ if (ret < 0 && errno == EINTR)
+ goto again;
+
+ return ret;
+}
+
+static void write_to_file(const char *filename, const void *buf, size_t count,
+ int allowed_errno)
+{
+ int fd, saved_errno;
+ ssize_t ret;
+
+ fd = open(filename, O_WRONLY | O_CLOEXEC);
+ if (fd < 0)
+ ksft_exit_fail_msg("%s - Failed to open file %s\n",
+ strerror(errno), filename);
+
+ ret = write_nointr(fd, buf, count);
+ if (ret < 0) {
+ if (allowed_errno && (errno == allowed_errno)) {
+ close(fd);
+ return;
+ }
+
+ goto on_error;
+ }
+
+ if ((size_t)ret != count)
+ goto on_error;
+
+ close(fd);
+ return;
+
+on_error:
+ saved_errno = errno;
+ close(fd);
+ errno = saved_errno;
+
+ if (ret < 0)
+ ksft_exit_fail_msg("%s - Failed to write to file %s\n",
+ strerror(errno), filename);
+
+ ksft_exit_fail_msg("Failed to write to file %s\n", filename);
+}
+
+static void change_to_userns(void)
+{
+ int ret;
+ uid_t uid;
+ gid_t gid;
+ /* {g,u}id_map files only allow a max of 4096 bytes written to them */
+ char idmap[4096];
+
+ uid = getuid();
+ gid = getgid();
+
+ ret = unshare(CLONE_NEWUSER);
+ if (ret < 0)
+ ksft_exit_fail_msg("%s - Failed to unshare user namespace\n&qu
[PATCH 2/5] binderfs: prevent renaming the control dentry
We don't allow to unlink it since it is crucial for binderfs to be useable
but if we allow to rename it we make the unlink trivial to bypass. So
prevent renaming too and simply treat the control dentry as immutable.
Take the opportunity and turn the check for the control dentry into a
separate helper is_binderfs_control_device() since it's now used in two
places.
Additionally, replace the custom rename dance we did with call to
simple_rename().
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
drivers/android/binderfs.c | 33 -
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 898d847f8505..02c96b5edfa9 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -346,34 +346,33 @@ static const struct super_operations binderfs_super_ops =
{
.statfs = simple_statfs,
};
+static inline bool is_binderfs_control_device(const struct inode *inode,
+ const struct dentry *dentry)
+{
+ return BINDERFS_I(inode)->control_dentry == dentry;
+}
+
static int binderfs_rename(struct inode *old_dir, struct dentry *old_dentry,
struct inode *new_dir, struct dentry *new_dentry,
unsigned int flags)
{
- struct inode *inode = d_inode(old_dentry);
-
- /* binderfs doesn't support directories. */
- if (d_is_dir(old_dentry))
- return -EPERM;
+ const struct inode *inode = d_inode(old_dentry);
- if (flags & ~RENAME_NOREPLACE)
- return -EINVAL;
-
- if (!simple_empty(new_dentry))
- return -ENOTEMPTY;
-
- if (d_really_is_positive(new_dentry))
- simple_unlink(new_dir, new_dentry);
+ if (is_binderfs_device(d_inode(old_dentry)))
+ inode = d_inode(old_dentry);
+ else
+ inode = d_inode(new_dentry);
- old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
- new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
+ if (is_binderfs_control_device(inode, old_dentry) ||
+ is_binderfs_control_device(inode, new_dentry))
+ return -EPERM;
- return 0;
+ return simple_rename(old_dir, old_dentry, new_dir, new_dentry, flags);
}
static int binderfs_unlink(struct inode *dir, struct dentry *dentry)
{
- if (BINDERFS_I(dir)->control_dentry == dentry)
+ if (is_binderfs_control_device(dir, dentry))
return -EPERM;
return simple_unlink(dir, dentry);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/5] binderfs: debug galore
Hey everyone, Al gave me a really helpful review of binderfs and pointed out a range of bugs. The most obvious and serious ones have fortunately already been taken care of by patches sitting in Greg's char-misc-linus tree. The others are hopefully all covered in this patchset. Thanks! Christian Christian Brauner (5): binderfs: remove outdated comment binderfs: prevent renaming the control dentry binderfs: rework binderfs_fill_super() binderfs: kill_litter_super() before cleanup binderfs: drop lock in binderfs_binder_ctl_create drivers/android/binderfs.c | 79 +- 1 file changed, 27 insertions(+), 52 deletions(-) -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 3/5] binderfs: rework binderfs_fill_super()
Al pointed out that on binderfs_fill_super() error
deactivate_locked_super() will call binderfs_kill_super() so all of the
freeing and putting we currently do in binderfs_fill_super() is unnecessary
and buggy. Let's simply return errors and let binderfs_fill_super() take
care of cleaning up on error.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
drivers/android/binderfs.c | 35 +--
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 02c96b5edfa9..c0fa495ee994 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -468,8 +468,8 @@ static const struct inode_operations
binderfs_dir_inode_operations = {
static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
{
+ int ret;
struct binderfs_info *info;
- int ret = -ENOMEM;
struct inode *inode = NULL;
struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
@@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
sb->s_op = &binderfs_super_ops;
sb->s_time_gran = 1;
- info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
- if (!info)
- goto err_without_dentry;
+ sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
+ if (!sb->s_fs_info)
+ return -ENOMEM;
+ info = sb->s_fs_info;
ret = binderfs_parse_mount_opts(data, &info->mount_opts);
if (ret)
- goto err_without_dentry;
+ return ret;
info->ipc_ns = ipc_ns;
info->root_gid = make_kgid(sb->s_user_ns, 0);
@@ -511,12 +512,9 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
if (!uid_valid(info->root_uid))
info->root_uid = GLOBAL_ROOT_UID;
- sb->s_fs_info = info;
-
- ret = -ENOMEM;
inode = new_inode(sb);
if (!inode)
- goto err_without_dentry;
+ return -ENOMEM;
inode->i_ino = FIRST_INODE;
inode->i_fop = &simple_dir_operations;
@@ -527,24 +525,9 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
sb->s_root = d_make_root(inode);
if (!sb->s_root)
- goto err_without_dentry;
-
- ret = binderfs_binder_ctl_create(sb);
- if (ret)
- goto err_with_dentry;
-
- return 0;
-
-err_with_dentry:
- dput(sb->s_root);
- sb->s_root = NULL;
-
-err_without_dentry:
- put_ipc_ns(ipc_ns);
- iput(inode);
- kfree(info);
+ return -ENOMEM;
- return ret;
+ return binderfs_binder_ctl_create(sb);
}
static struct dentry *binderfs_mount(struct file_system_type *fs_type,
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 5/5] binderfs: drop lock in binderfs_binder_ctl_create
The binderfs_binder_ctl_create() call is a no-op on subsequent calls and
the first call is done before we unlock the suberblock. Hence, there is no
need to take inode_lock() in there. Let's remove it.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
Note, that fs/devptfs/inode.c:mknod_ptmx() is currently holding
inode_lock() too under the exact same circumstances. Seems that we can drop
it from there too.
---
drivers/android/binderfs.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index c5feb9ffce0f..db09274a1b75 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -408,8 +408,6 @@ static int binderfs_binder_ctl_create(struct super_block
*sb)
if (!device)
return -ENOMEM;
- inode_lock(d_inode(root));
-
/* If we have already created a binder-control node, return. */
if (info->control_dentry) {
ret = 0;
@@ -448,12 +446,10 @@ static int binderfs_binder_ctl_create(struct super_block
*sb)
inode->i_private = device;
info->control_dentry = dentry;
d_add(dentry, inode);
- inode_unlock(d_inode(root));
return 0;
out:
- inode_unlock(d_inode(root));
kfree(device);
iput(inode);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/5] binderfs: remove outdated comment
The comment stems from an early version of that patchset and is just
confusing now.
Cc: Al Viro
Signed-off-by: Christian Brauner
---
drivers/android/binderfs.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e4ff4c3fa371..898d847f8505 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -373,10 +373,6 @@ static int binderfs_rename(struct inode *old_dir, struct
dentry *old_dentry,
static int binderfs_unlink(struct inode *dir, struct dentry *dentry)
{
- /*
-* The control dentry is only ever touched during mount so checking it
-* here should not require us to take lock.
-*/
if (BINDERFS_I(dir)->control_dentry == dentry)
return -EPERM;
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 4/5] binderfs: kill_litter_super() before cleanup
Al pointed out that first calling kill_litter_super() before cleaning up
info is more correct since destroying info doesn't depend on the state of
the dentries and inodes. That the opposite remains true is not guaranteed.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
drivers/android/binderfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index c0fa495ee994..c5feb9ffce0f 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -541,11 +541,12 @@ static void binderfs_kill_super(struct super_block *sb)
{
struct binderfs_info *info = sb->s_fs_info;
+ kill_litter_super(sb);
+
if (info && info->ipc_ns)
put_ipc_ns(info->ipc_ns);
kfree(info);
- kill_litter_super(sb);
}
static struct file_system_type binder_fs_type = {
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/5] binderfs: prevent renaming the control dentry
On Fri, Jan 18, 2019 at 10:55:52PM +, Al Viro wrote:
> On Fri, Jan 18, 2019 at 03:53:41PM +0100, Christian Brauner wrote:
> > We don't allow to unlink it since it is crucial for binderfs to be useable
> > but if we allow to rename it we make the unlink trivial to bypass. So
> > prevent renaming too and simply treat the control dentry as immutable.
> >
> > Take the opportunity and turn the check for the control dentry into a
> > separate helper is_binderfs_control_device() since it's now used in two
> > places.
> > Additionally, replace the custom rename dance we did with call to
> > simple_rename().
>
> Umm...
>
> > +static inline bool is_binderfs_control_device(const struct inode *inode,
> > + const struct dentry *dentry)
> > +{
> > + return BINDERFS_I(inode)->control_dentry == dentry;
> > +}
>
> What do you need an inode for?
BINDERFS_I() is called in is_binderfs_device() which is called in
binder.c:
static int binder_open(struct inode *nodp, struct file *filp)
{
/* binderfs stashes devices in i_private */
if (is_binderfs_device(nodp))
binder_dev = nodp->i_private;
else
binder_dev = container_of(filp->private_data,
struct binder_device, miscdev);
and it was just easier to have it take an inode as arg since that's what
binder_open() makes easily available. Just kept it this way for
is_binderfs_control_device() too but will switch the latter over now.
>
> static inline struct binderfs_info *BINDERFS_I(const struct inode *inode)
> {
> return inode->i_sb->s_fs_info;
> }
>
> so it looks like all you care about is the superblock. Which can be
> had simply as dentry->d_sb...
Hm yes, I think I'll just do:
static inline bool is_binderfs_control_device(const struct dentry *dentry)
{
struct binderfs_info *info = dentry->d_sb->s_fs_info;
return info->control_dentry == dentry;
}
and pick up what you scratched below.
>
> Besides, what's the point of calling is_binderfs_device() in ->rename()?
> If your directory methods are given dentries from another filesystem,
> the kernel is already FUBAR. So your rename should simply do
> if (is_binderfs_control_device(old_dentry) ||
> is_binderfs_control_device(new_dentry))
> return -EPERM;
> return simple_rename(..);
> and that's it...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] binderfs: rework binderfs_fill_super()
On Fri, Jan 18, 2019 at 11:03:54PM +, Al Viro wrote:
> On Fri, Jan 18, 2019 at 03:53:42PM +0100, Christian Brauner wrote:
> > static int binderfs_fill_super(struct super_block *sb, void *data, int
> > silent)
> > {
> > + int ret;
> > struct binderfs_info *info;
> > - int ret = -ENOMEM;
> > struct inode *inode = NULL;
> > struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
> >
> > @@ -495,13 +495,14 @@ static int binderfs_fill_super(struct super_block
> > *sb, void *data, int silent)
> > sb->s_op = &binderfs_super_ops;
> > sb->s_time_gran = 1;
> >
> > - info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
> > - if (!info)
> > - goto err_without_dentry;
> > + sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
> > + if (!sb->s_fs_info)
> > + return -ENOMEM;
> > + info = sb->s_fs_info;
>
> ... and that's when you should grab ipcns reference and stick it into
> info->ipc_ns, to match the logics in binderfs_kill_super().
>
> Otherwise the failure above
>
> > ret = binderfs_parse_mount_opts(data, &info->mount_opts);
> > if (ret)
> > - goto err_without_dentry;
> > + return ret;
>
> ... or here leaves you with an ipcns leak.
>
> Destructor does
> if ->s_fs_info is non-NULL
> release ->s_fs_info->ipc_ns
> free ->s_fs_info
> so constructor should not leave object in a state when ipcns is already
> grabbed, but not stored in ->s_fs_info->ipc_ns (including the case of
> allocation failure leaving it with NULL ->s_fs_info).
Yeah, total brainfart on my side. I shouldn't code in airports
apparently... Fixed.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/5] binderfs: debug galore
On Fri, Jan 18, 2019 at 11:26:34PM +, Al Viro wrote:
> On Fri, Jan 18, 2019 at 03:53:39PM +0100, Christian Brauner wrote:
> > Hey everyone,
> >
> > Al gave me a really helpful review of binderfs and pointed out a range
> > of bugs. The most obvious and serious ones have fortunately already been
> > taken care of by patches sitting in Greg's char-misc-linus tree. The
> > others are hopefully all covered in this patchset.
>
> BTW, binderfs_binder_device_create() looks rather odd - it would be easier
> to do this:
> inode_lock(d_inode(root));
> /* look it up */
> dentry = lookup_one_len(name, root, strlen(name));
It didn't seem obvious that that's what you should use to lookup
dentries instead of d_lookup(). Especially since d_lookup() points out
that it takes the rename lock which seemed crucial to me so that we
don't end up in a situation where we race against another dentry being
renamed to the name we're currently trying to used.
Thanks for the pointer!
> if (IS_ERR(dentry)) {
> /* some kind of error (ENOMEM, permissions) - report */
> inode_unlock(d_inode(root));
> ret = PTR_ERR(dentry);
> goto err;
> }
> if (d_really_is_positive(dentry)) {
> /* already exists */
> dput(dentry);
> inode_unlock(d_inode(root));
> ret = -EEXIST;
> goto err;
> }
> inode->i_private = device;
> ... and from that point on - as in your variant. Another thing in there:
Right, just read through the code for lookup_one_len() it seems to
allocate a new dentry if it can't find a hashed match for the old name.
That's surprising. The name of the function does not really give that
impression. :) (Would probably be better if lookup_or_alloc_one_len() or
something. But that's not important now.)
> name = kmalloc(name_len, GFP_KERNEL);
> if (!name)
> goto err;
>
> strscpy(name, req->name, name_len);
> is an odd way to go; more straightforward would be
> req->name[BINDERFS_MAX_NAME] = '\0';/* NUL-terminate */
> name = kmemdup(req->name, sizeof(req->name), GEP_KERNEL);
> if (!name)
>
Using kmemdup() now.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 0/7] binderfs: debug galore
Hey everyone, Al gave me a really helpful review of binderfs and pointed out a range of bugs. The most obvious and serious ones have fortunately already been taken care of by patches sitting in Greg's char-misc-linus tree. The others are hopefully all covered in this patchset. /* Changelog */ Nothing major apart from working in a bunch of good comments and suggestions from Al. The most interesting one is probably the switch from d_alloc_name() + d_lookup() to lookup_one_len() when detecting name clashes between binder devices. This also forces us to switch from d_add() to d_instantiate() since lookup_one_len() adds new dentries to the hashqueue. If we were to use d_add() after this we'd end up with the same dentry over the same inode twice. I moved the switch from d_add() to d_instantiate() into a separate commit. The rest should hopefully be pretty mundane. Thanks! Christian Christian Brauner (7): binderfs: remove outdated comment binderfs: prevent renaming the control dentry binderfs: rework binderfs_fill_super() binderfs: rework binderfs_binder_device_create() binderfs: kill_litter_super() before cleanup binderfs: drop lock in binderfs_binder_ctl_create binderfs: switch from d_add() to d_instantiate() drivers/android/binderfs.c | 121 + 1 file changed, 43 insertions(+), 78 deletions(-) -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 1/7] binderfs: remove outdated comment
The comment stems from an early version of that patchset and is just
confusing now.
Cc: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- patch unchanged
---
drivers/android/binderfs.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e4ff4c3fa371..898d847f8505 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -373,10 +373,6 @@ static int binderfs_rename(struct inode *old_dir, struct
dentry *old_dentry,
static int binderfs_unlink(struct inode *dir, struct dentry *dentry)
{
- /*
-* The control dentry is only ever touched during mount so checking it
-* here should not require us to take lock.
-*/
if (BINDERFS_I(dir)->control_dentry == dentry)
return -EPERM;
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 3/7] binderfs: rework binderfs_fill_super()
Al pointed out that on binderfs_fill_super() error
deactivate_locked_super() will call binderfs_kill_super() so all of the
freeing and putting we currently do in binderfs_fill_super() is unnecessary
and buggy. Let's simply return errors and let binderfs_fill_super() take
care of cleaning up on error.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- correctly grab and stash a reference to task's ipc_ns to prevent leaks
- replace d_alloc_name() + d_lookup() combination with lookup_one_len()
- replace kmalloc() + strscpy() with kmemdup()
---
drivers/android/binderfs.c | 41 ++
1 file changed, 11 insertions(+), 30 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index e73f9dbee099..89a2ee1a02f6 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -461,12 +461,9 @@ static const struct inode_operations
binderfs_dir_inode_operations = {
static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
{
+ int ret;
struct binderfs_info *info;
- int ret = -ENOMEM;
struct inode *inode = NULL;
- struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
-
- get_ipc_ns(ipc_ns);
sb->s_blocksize = PAGE_SIZE;
sb->s_blocksize_bits = PAGE_SHIFT;
@@ -488,15 +485,17 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
sb->s_op = &binderfs_super_ops;
sb->s_time_gran = 1;
- info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
- if (!info)
- goto err_without_dentry;
+ sb->s_fs_info = kzalloc(sizeof(struct binderfs_info), GFP_KERNEL);
+ if (!sb->s_fs_info)
+ return -ENOMEM;
+ info = sb->s_fs_info;
+
+ info->ipc_ns = get_ipc_ns(current->nsproxy->ipc_ns);
ret = binderfs_parse_mount_opts(data, &info->mount_opts);
if (ret)
- goto err_without_dentry;
+ return ret;
- info->ipc_ns = ipc_ns;
info->root_gid = make_kgid(sb->s_user_ns, 0);
if (!gid_valid(info->root_gid))
info->root_gid = GLOBAL_ROOT_GID;
@@ -504,12 +503,9 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
if (!uid_valid(info->root_uid))
info->root_uid = GLOBAL_ROOT_UID;
- sb->s_fs_info = info;
-
- ret = -ENOMEM;
inode = new_inode(sb);
if (!inode)
- goto err_without_dentry;
+ return -ENOMEM;
inode->i_ino = FIRST_INODE;
inode->i_fop = &simple_dir_operations;
@@ -520,24 +516,9 @@ static int binderfs_fill_super(struct super_block *sb,
void *data, int silent)
sb->s_root = d_make_root(inode);
if (!sb->s_root)
- goto err_without_dentry;
-
- ret = binderfs_binder_ctl_create(sb);
- if (ret)
- goto err_with_dentry;
-
- return 0;
-
-err_with_dentry:
- dput(sb->s_root);
- sb->s_root = NULL;
-
-err_without_dentry:
- put_ipc_ns(ipc_ns);
- iput(inode);
- kfree(info);
+ return -ENOMEM;
- return ret;
+ return binderfs_binder_ctl_create(sb);
}
static struct dentry *binderfs_mount(struct file_system_type *fs_type,
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 7/7] binderfs: switch from d_add() to d_instantiate()
In a previous commit we switched from a d_alloc_name() + d_lookup() combination to setup a new dentry and find potential duplicates to the more idiomatic lookup_one_len(). As far as I understand, this also means we need to switch from d_add() to d_instantiate() since lookup_one_len() will create a new dentry when it doesn't find an existing one and add the new dentry to the hash queues. So we only need to call d_instantiate() to connect the dentry to the inode and turn it into a positive dentry. If we were to use d_add() we sure see stack traces like the following indicating that adding the same dentry twice over the same inode: [ 744.441889] CPU: 4 PID: 2849 Comm: landscape-sysin Not tainted 5.0.0-rc1-brauner-binderfs #243 [ 744.441889] Hardware name: Dell DCS XS24-SC2 /XS24-SC2 , BIOS S59_3C20 04/07/2011 [ 744.441889] RIP: 0010:__d_lookup_rcu+0x76/0x190 [ 744.441889] Code: 89 75 c0 49 c1 e9 20 49 89 fd 45 89 ce 41 83 e6 07 42 8d 04 f5 00 00 00 00 89 45 c8 eb 0c 48 8b 1b 48 85 db 0f 84 81 00 00 00 <44> 8b 63 fc 4c 3b 6b 10 75 ea 48 83 7b 08 00 74 e3 41 83 e4 fe 41 [ 744.441889] RSP: 0018:b8c984e27ad0 EFLAGS: 0282 ORIG_RAX: ff13 [ 744.441889] RAX: 0038 RBX: 9407ef770c08 RCX: b8c980011000 [ 744.441889] RDX: b8c984e27b54 RSI: b8c984e27ce0 RDI: 9407e6689600 [ 744.441889] RBP: b8c984e27b28 R08: b8c984e27ba4 R09: 0007 [ 744.441889] R10: 9407e5c4f05c R11: 973f3eb9d84a94e5 R12: 0002 [ 744.441889] R13: 9407e6689600 R14: 0007 R15: 0007bfef7a13 [ 744.441889] FS: 7f0db13bb740() GS:9407f3b0() knlGS: [ 744.441889] CS: 0010 DS: ES: CR0: 80050033 [ 744.441889] CR2: 7f0dacc51024 CR3: 00032961a000 CR4: 06e0 [ 744.441889] Call Trace: [ 744.441889] lookup_fast+0x53/0x300 [ 744.441889] walk_component+0x49/0x350 [ 744.441889] ? inode_permission+0x63/0x1a0 [ 744.441889] link_path_walk.part.33+0x1bc/0x5a0 [ 744.441889] ? path_init+0x190/0x310 [ 744.441889] path_lookupat+0x95/0x210 [ 744.441889] filename_lookup+0xb6/0x190 [ 744.441889] ? __check_object_size+0xb8/0x1b0 [ 744.441889] ? strncpy_from_user+0x50/0x1a0 [ 744.441889] user_path_at_empty+0x36/0x40 [ 744.441889] ? user_path_at_empty+0x36/0x40 [ 744.441889] vfs_statx+0x76/0xe0 [ 744.441889] __do_sys_newstat+0x3d/0x70 [ 744.441889] __x64_sys_newstat+0x16/0x20 [ 744.441889] do_syscall_64+0x5a/0x120 [ 744.441889] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 744.441889] RIP: 0033:0x7f0db0ec2775 [ 744.441889] Code: 00 00 00 75 05 48 83 c4 18 c3 e8 26 55 02 00 66 0f 1f 44 00 00 83 ff 01 48 89 f0 77 30 48 89 c7 48 89 d6 b8 04 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 03 f3 c3 90 48 8b 15 e1 b6 2d 00 f7 d8 64 89 [ 744.441889] RSP: 002b:7ffc36bc9388 EFLAGS: 0246 ORIG_RAX: 0004 [ 744.441889] RAX: ffda RBX: 7ffc36bc9300 RCX: 7f0db0ec2775 [ 744.441889] RDX: 7ffc36bc9400 RSI: 7ffc36bc9400 RDI: 7f0dad26f050 [ 744.441889] RBP: 00c0bc60 R08: R09: 0001 [ 744.441889] R10: R11: 0246 R12: 7ffc36bc9400 [ 744.441889] R13: 0001 R14: ff9c R15: 00c0bc60 Cc: Al Viro Signed-off-by: Christian Brauner --- /* Changelog */ v1: - patch introduced --- drivers/android/binderfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c index d537dcdb5d65..6a2185eb66c5 100644 --- a/drivers/android/binderfs.c +++ b/drivers/android/binderfs.c @@ -212,7 +212,7 @@ static int binderfs_binder_device_create(struct inode *ref_inode, } inode->i_private = device; - d_add(dentry, inode); + d_instantiate(dentry, inode); fsnotify_create(root->d_inode, dentry); inode_unlock(d_inode(root)); -- 2.19.1 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 2/7] binderfs: prevent renaming the control dentry
- make binderfs control dentry immutable:
We don't allow to unlink it since it is crucial for binderfs to be
useable but if we allow to rename it we make the unlink trivial to
bypass. So prevent renaming too and simply treat the control dentry as
immutable.
- add is_binderfs_control_device() helper:
Take the opportunity and turn the check for the control dentry into a
separate helper is_binderfs_control_device() since it's now used in two
places.
- simplify binderfs_rename():
Instead of hand-rolling our custom version of simple_rename() just dumb
the whole function down to first check whether we're trying to rename the
control dentry. If we do EPERM the caller and if not call simple_rename().
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- simplify is_binderfs_control_device() to only take a dentry argument
instead of taking an unnecessary detour through the inode.
---
drivers/android/binderfs.c | 28 ++--
1 file changed, 10 insertions(+), 18 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 898d847f8505..e73f9dbee099 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -346,34 +346,26 @@ static const struct super_operations binderfs_super_ops =
{
.statfs = simple_statfs,
};
+static inline bool is_binderfs_control_device(const struct dentry *dentry)
+{
+ struct binderfs_info *info = dentry->d_sb->s_fs_info;
+ return info->control_dentry == dentry;
+}
+
static int binderfs_rename(struct inode *old_dir, struct dentry *old_dentry,
struct inode *new_dir, struct dentry *new_dentry,
unsigned int flags)
{
- struct inode *inode = d_inode(old_dentry);
-
- /* binderfs doesn't support directories. */
- if (d_is_dir(old_dentry))
+ if (is_binderfs_control_device(old_dentry) ||
+ is_binderfs_control_device(new_dentry))
return -EPERM;
- if (flags & ~RENAME_NOREPLACE)
- return -EINVAL;
-
- if (!simple_empty(new_dentry))
- return -ENOTEMPTY;
-
- if (d_really_is_positive(new_dentry))
- simple_unlink(new_dir, new_dentry);
-
- old_dir->i_ctime = old_dir->i_mtime = new_dir->i_ctime =
- new_dir->i_mtime = inode->i_ctime = current_time(old_dir);
-
- return 0;
+ return simple_rename(old_dir, old_dentry, new_dir, new_dentry, flags);
}
static int binderfs_unlink(struct inode *dir, struct dentry *dentry)
{
- if (BINDERFS_I(dir)->control_dentry == dentry)
+ if (is_binderfs_control_device(dentry))
return -EPERM;
return simple_unlink(dir, dentry);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 6/7] binderfs: drop lock in binderfs_binder_ctl_create
The binderfs_binder_ctl_create() call is a no-op on subsequent calls and
the first call is done before we unlock the suberblock. Hence, there is no
need to take inode_lock() in there. Let's remove it.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
Note, that fs/devptfs/inode.c:mknod_ptmx() is currently holding
inode_lock() too under the exact same circumstances. Seems that we can drop
it from there too.
/* Changelog */
v1:
- patch unchanged
---
drivers/android/binderfs.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index ba88be172aee..d537dcdb5d65 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -400,8 +400,6 @@ static int binderfs_binder_ctl_create(struct super_block
*sb)
if (!device)
return -ENOMEM;
- inode_lock(d_inode(root));
-
/* If we have already created a binder-control node, return. */
if (info->control_dentry) {
ret = 0;
@@ -440,12 +438,10 @@ static int binderfs_binder_ctl_create(struct super_block
*sb)
inode->i_private = device;
info->control_dentry = dentry;
d_add(dentry, inode);
- inode_unlock(d_inode(root));
return 0;
out:
- inode_unlock(d_inode(root));
kfree(device);
iput(inode);
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 5/7] binderfs: kill_litter_super() before cleanup
Al pointed out that first calling kill_litter_super() before cleaning up
info is more correct since destroying info doesn't depend on the state of
the dentries and inodes. That the opposite remains true is not guaranteed.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- patch unchanged
---
drivers/android/binderfs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 1e077498a507..ba88be172aee 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -531,11 +531,12 @@ static void binderfs_kill_super(struct super_block *sb)
{
struct binderfs_info *info = sb->s_fs_info;
+ kill_litter_super(sb);
+
if (info && info->ipc_ns)
put_ipc_ns(info->ipc_ns);
kfree(info);
- kill_litter_super(sb);
}
static struct file_system_type binder_fs_type = {
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1 4/7] binderfs: rework binderfs_binder_device_create()
- switch from d_alloc_name() + d_lookup() to lookup_one_len():
Instead of using d_alloc_name() and then doing a d_lookup() with the
allocated dentry to find whether a device with the name we're trying to
create already exists switch to using lookup_one_len(). The latter will
either return the existing dentry or a new one.
- switch from kmalloc() + strscpy() to kmemdup():
Use a more idiomatic way to copy the name for the new dentry that
userspace gave us.
Suggested-by: Al Viro
Signed-off-by: Christian Brauner
---
/* Changelog */
v1:
- patch introduced
---
drivers/android/binderfs.c | 39 +++---
1 file changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 89a2ee1a02f6..1e077498a507 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -106,7 +107,7 @@ bool is_binderfs_device(const struct inode *inode)
* @userp: buffer to copy information about new device for userspace to
* @req: struct binderfs_device as copied from userspace
*
- * This function allocated a new binder_device and reserves a new minor
+ * This function allocates a new binder_device and reserves a new minor
* number for it.
* Minor numbers are limited and tracked globally in binderfs_minors. The
* function will stash a struct binder_device for the specific binder
@@ -122,10 +123,10 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
struct binderfs_device *req)
{
int minor, ret;
- struct dentry *dentry, *dup, *root;
+ struct dentry *dentry, *root;
struct binder_device *device;
- size_t name_len = BINDERFS_MAX_NAME + 1;
char *name = NULL;
+ size_t name_len;
struct inode *inode = NULL;
struct super_block *sb = ref_inode->i_sb;
struct binderfs_info *info = sb->s_fs_info;
@@ -168,12 +169,13 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
inode->i_uid = info->root_uid;
inode->i_gid = info->root_gid;
- name = kmalloc(name_len, GFP_KERNEL);
+ req->name[BINDERFS_MAX_NAME] = '\0'; /* NUL-terminate */
+ name_len = strlen(req->name);
+ /* Make sure to include terminating NUL byte */
+ name = kmemdup(req->name, name_len + 1, GFP_KERNEL);
if (!name)
goto err;
- strscpy(name, req->name, name_len);
-
device->binderfs_inode = inode;
device->context.binder_context_mgr_uid = INVALID_UID;
device->context.name = name;
@@ -192,24 +194,21 @@ static int binderfs_binder_device_create(struct inode
*ref_inode,
root = sb->s_root;
inode_lock(d_inode(root));
- dentry = d_alloc_name(root, name);
- if (!dentry) {
+
+ /* look it up */
+ dentry = lookup_one_len(name, root, name_len);
+ if (IS_ERR(dentry)) {
inode_unlock(d_inode(root));
- ret = -ENOMEM;
+ ret = PTR_ERR(dentry);
goto err;
}
- /* Verify that the name userspace gave us is not already in use. */
- dup = d_lookup(root, &dentry->d_name);
- if (dup) {
- if (d_really_is_positive(dup)) {
- dput(dup);
- dput(dentry);
- inode_unlock(d_inode(root));
- ret = -EEXIST;
- goto err;
- }
- dput(dup);
+ if (d_really_is_positive(dentry)) {
+ /* already exists */
+ dput(dentry);
+ inode_unlock(d_inode(root));
+ ret = -EEXIST;
+ goto err;
}
inode->i_private = device;
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] binderfs: use __u32 for device numbers
We allow more then 255 binderfs binder devices to be created since there
are workloads that require more than that. If we use __u8 we'll overflow
after 255. So let's use a __u32.
Note that there's no released kernel with binderfs out there so this is
not a regression.
Signed-off-by: Christian Brauner
---
include/uapi/linux/android/binderfs.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/android/binderfs.h
b/include/uapi/linux/android/binderfs.h
index b41628b77120..87410477aea9 100644
--- a/include/uapi/linux/android/binderfs.h
+++ b/include/uapi/linux/android/binderfs.h
@@ -22,8 +22,8 @@
*/
struct binderfs_device {
char name[BINDERFS_MAX_NAME + 1];
- __u8 major;
- __u8 minor;
+ __u32 major;
+ __u32 minor;
};
/**
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] binderfs: use correct include guards in header
When we switched over from binder_ctl.h to binderfs.h we forgot to change
the include guards. It's minor but it's obviously correct.
Signed-off-by: Christian Brauner
---
include/uapi/linux/android/binderfs.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/uapi/linux/android/binderfs.h
b/include/uapi/linux/android/binderfs.h
index 65b2efd1a0a5..b41628b77120 100644
--- a/include/uapi/linux/android/binderfs.h
+++ b/include/uapi/linux/android/binderfs.h
@@ -4,8 +4,8 @@
*
*/
-#ifndef _UAPI_LINUX_BINDER_CTL_H
-#define _UAPI_LINUX_BINDER_CTL_H
+#ifndef _UAPI_LINUX_BINDERFS_H
+#define _UAPI_LINUX_BINDERFS_H
#include
#include
@@ -31,5 +31,5 @@ struct binderfs_device {
*/
#define BINDER_CTL_ADD _IOWR('b', 1, struct binderfs_device)
-#endif /* _UAPI_LINUX_BINDER_CTL_H */
+#endif /* _UAPI_LINUX_BINDERFS_H */
--
2.19.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] selftests: add binderfs selftests
On Thu, Jan 17, 2019 at 12:48:54PM +0100, Christian Brauner wrote:
> This adds the promised selftest for binderfs. It will verify the following
> things:
> - binderfs mounting works
> - binder device allocation works
> - performing a binder ioctl() request through a binderfs device works
> - binder device removal works
> - binder-control removal fails
> - binderfs unmounting works
>
> The tests are performed both privileged and unprivileged. The latter
> verifies that binderfs behaves correctly in user namespaces.
>
> Cc: Todd Kjos
> Signed-off-by: Christian Brauner
Hey Shuah,
If you're ok with the patch in its current form, can you please make
sure that this still lands in 5.0? If at all possible I'd like to have
all ducks in a row and release binderfs with selftests and everything.
:)
Thanks!
Christian
> ---
> /* Changelog */
> v3:
> - s/printf/ksft_print_msg/g
> - do not report misleading errno on short write
>
> v2:
> - make failure to create /dev/binderfs directory fatal in all circumstances
> - make tests run in user namespace to test whether binderfs can be mounted
> in user namespaces and so that unprivileged users can run the tests
> - use ksft_exit_skip()
>
> v1:
> - check for ENODEV on mount failure to detect whether binderfs is
> available
> If it is not, skip the test and exit with success.
> ---
> tools/testing/selftests/Makefile | 1 +
> .../selftests/filesystems/binderfs/.gitignore | 1 +
> .../selftests/filesystems/binderfs/Makefile | 6 +
> .../filesystems/binderfs/binderfs_test.c | 275 ++
> .../selftests/filesystems/binderfs/config | 3 +
> 5 files changed, 286 insertions(+)
> create mode 100644 tools/testing/selftests/filesystems/binderfs/.gitignore
> create mode 100644 tools/testing/selftests/filesystems/binderfs/Makefile
> create mode 100644
> tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> create mode 100644 tools/testing/selftests/filesystems/binderfs/config
>
> diff --git a/tools/testing/selftests/Makefile
> b/tools/testing/selftests/Makefile
> index 1a2bd15c5b6e..400ee81a3043 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -10,6 +10,7 @@ TARGETS += drivers/dma-buf
> TARGETS += efivarfs
> TARGETS += exec
> TARGETS += filesystems
> +TARGETS += filesystems/binderfs
> TARGETS += firmware
> TARGETS += ftrace
> TARGETS += futex
> diff --git a/tools/testing/selftests/filesystems/binderfs/.gitignore
> b/tools/testing/selftests/filesystems/binderfs/.gitignore
> new file mode 100644
> index ..8a5d9bf63dd4
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/binderfs/.gitignore
> @@ -0,0 +1 @@
> +binderfs_test
> diff --git a/tools/testing/selftests/filesystems/binderfs/Makefile
> b/tools/testing/selftests/filesystems/binderfs/Makefile
> new file mode 100644
> index ..58cb659b56b4
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/binderfs/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +CFLAGS += -I../../../../../usr/include/
> +TEST_GEN_PROGS := binderfs_test
> +
> +include ../../lib.mk
> diff --git a/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> new file mode 100644
> index ..8c2ed962e1c7
> --- /dev/null
> +++ b/tools/testing/selftests/filesystems/binderfs/binderfs_test.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "../../kselftest.h"
> +
> +static ssize_t write_nointr(int fd, const void *buf, size_t count)
> +{
> + ssize_t ret;
> +again:
> + ret = write(fd, buf, count);
> + if (ret < 0 && errno == EINTR)
> + goto again;
> +
> + return ret;
> +}
> +
> +static void write_to_file(const char *filename, const void *buf, size_t
> count,
> + int allowed_errno)
> +{
> + int fd, saved_errno;
> + ssize_t ret;
> +
> + fd = open(filename, O_WRONLY | O_CLOEXEC);
> + if (fd < 0)
> + ksft_exit_fail_msg("%s - Failed to open file %s\n",
> +strerror(errno), filename);
> +
> + ret = write_nointr(fd, buf, count);
> + if (ret < 0) {
> + if (allowed_errno && (errno == allowed_errno)) {
> + close(fd)
[PATCH 1/2] binderfs: respect limit on binder control creation
We currently adhere to the reserved devices limit when creating new
binderfs devices in binderfs instances not located in the inital ipc
namespace. But it is still possible to rob the host instances of their 4
reserved devices by creating the maximum allowed number of devices in a
single binderfs instance located in a non-initial ipc namespace and then
mounting 4 separate binderfs instances in non-initial ipc namespaces. That
happens because the limit is currently not respected for the creation of
the initial binder-control device node. Block this nonsense by performing
the same check in binderfs_binder_ctl_create() that we perform in
binderfs_binder_device_create().
Fixes: 36bdf3cae09d ("binderfs: reserve devices for initial mount")
Signed-off-by: Christian Brauner
---
drivers/android/binderfs.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 6a2185eb66c5..7a550104a722 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -395,6 +395,11 @@ static int binderfs_binder_ctl_create(struct super_block
*sb)
struct inode *inode = NULL;
struct dentry *root = sb->s_root;
struct binderfs_info *info = sb->s_fs_info;
+#if defined(CONFIG_IPC_NS)
+ bool use_reserve = (info->ipc_ns == &init_ipc_ns);
+#else
+ bool use_reserve = true;
+#endif
device = kzalloc(sizeof(*device), GFP_KERNEL);
if (!device)
@@ -413,7 +418,10 @@ static int binderfs_binder_ctl_create(struct super_block
*sb)
/* Reserve a new minor number for the new device. */
mutex_lock(&binderfs_minors_mutex);
- minor = ida_alloc_max(&binderfs_minors, BINDERFS_MAX_MINOR, GFP_KERNEL);
+ minor = ida_alloc_max(&binderfs_minors,
+ use_reserve ? BINDERFS_MAX_MINOR :
+ BINDERFS_MAX_MINOR_CAPPED,
+ GFP_KERNEL);
mutex_unlock(&binderfs_minors_mutex);
if (minor < 0) {
ret = minor;
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] binderfs: remove separate device_initcall()
binderfs should not have a separate device_initcall(). When a kernel is
compiled with CONFIG_ANDROID_BINDERFS register the filesystem alongside
CONFIG_ANDROID_IPC. This use-case is especially sensible when users specify
CONFIG_ANDROID_IPC=y, CONFIG_ANDROID_BINDERFS=y and
ANDROID_BINDER_DEVICES="".
When CONFIG_ANDROID_BINDERFS=n then this always succeeds so there's no
regression potential for legacy workloads.
Signed-off-by: Christian Brauner
---
drivers/android/binder.c | 4
drivers/android/binder_internal.h | 9 +
drivers/android/binderfs.c| 4 +---
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index cdfc87629efb..751d76173f81 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5915,6 +5915,10 @@ static int __init binder_init(void)
goto err_init_binder_device_failed;
}
+ ret = init_binderfs();
+ if (ret)
+ goto err_init_binder_device_failed;
+
return ret;
err_init_binder_device_failed:
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 7fb97f503ef2..045b3e42d98b 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -46,4 +46,13 @@ static inline bool is_binderfs_device(const struct inode
*inode)
}
#endif
+#ifdef CONFIG_ANDROID_BINDERFS
+extern int __init init_binderfs(void);
+#else
+static inline int __init init_binderfs(void)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7a550104a722..e773f45d19d9 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -550,7 +550,7 @@ static struct file_system_type binder_fs_type = {
.fs_flags = FS_USERNS_MOUNT,
};
-static int __init init_binderfs(void)
+int __init init_binderfs(void)
{
int ret;
@@ -568,5 +568,3 @@ static int __init init_binderfs(void)
return ret;
}
-
-device_initcall(init_binderfs);
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] selftests: add binderfs selftests
On Wed, Jan 23, 2019 at 07:15:01AM -0700, shuah wrote: > On 1/23/19 4:00 AM, Greg KH wrote: > > On Wed, Jan 23, 2019 at 11:54:58AM +0100, Christian Brauner wrote: > > > On Thu, Jan 17, 2019 at 12:48:54PM +0100, Christian Brauner wrote: > > > > This adds the promised selftest for binderfs. It will verify the > > > > following > > > > things: > > > > - binderfs mounting works > > > > - binder device allocation works > > > > - performing a binder ioctl() request through a binderfs device works > > > > - binder device removal works > > > > - binder-control removal fails > > > > - binderfs unmounting works > > > > > > > > The tests are performed both privileged and unprivileged. The latter > > > > verifies that binderfs behaves correctly in user namespaces. > > > > > > > > Cc: Todd Kjos > > > > Signed-off-by: Christian Brauner > > > > > > Hey Shuah, > > > > > > If you're ok with the patch in its current form, can you please make > > > sure that this still lands in 5.0? If at all possible I'd like to have > > > all ducks in a row and release binderfs with selftests and everything. > > > :) > > > > The patch is good and I was planning to get this into 5.1. > > > I can take it in my tree with the other binderfs patches if I can get an > > ack from Shuah. > > > > Great. It is good for the test patch to go with the other binderfs > patches. > > Acked-by: Shuah Khan Thanks for the quick response! Christian ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] binder: fix CONFIG_ANDROID_BINDER_DEVICES
Several users have tried to only rely on binderfs to provide binder devices
and set CONFIG_ANDROID_BINDER_DEVICES="" empty. This is a great use-case of
binderfs and one that was always intended to work. However, this is
currently not possible since setting CONFIG_ANDROID_BINDER_DEVICES="" emtpy
will simply panic the kernel:
kobject: (028c2f79): attempted to be registered with empty name!
WARNING: CPU: 7 PID: 1703 at lib/kobject.c:228 kobject_add_internal+0x288/0x2b0
Modules linked in: binder_linux(+) bridge stp llc ipmi_ssif gpio_ich dcdbas
coretemp kvm_intel kvm irqbypass serio_raw input_leds lpc_ich i5100_edac
mac_hid ipmi_si ipmi_devintf ipmi_msghandler sch_fq_codel ib_i
CPU: 7 PID: 1703 Comm: modprobe Not tainted 5.0.0-rc2-brauner-binderfs #263
Hardware name: Dell DCS XS24-SC2 /XS24-SC2 , BIOS
S59_3C20 04/07/2011
RIP: 0010:kobject_add_internal+0x288/0x2b0
Code: 12 95 48 c7 c7 78 63 3b 95 e8 77 35 71 ff e9 91 fe ff ff 0f 0b eb a7 0f
0b eb 9a 48 89 de 48 c7 c7 00 63 3b 95 e8 f8 95 6a ff <0f> 0b 41 bc ea ff ff ff
e9 6d fe ff ff 41 bc fe ff ff ff e9 62 fe
RSP: 0018:973f84237a30 EFLAGS: 00010282
RAX: RBX: 8b53e2472010 RCX: 0006
RDX: 0007 RSI: 0086 RDI: 8b53edbd63a0
RBP: 973f84237a60 R08: 0342 R09: 0004
R10: 973f84237af0 R11: 0001 R12:
R13: 8b53e9f1a1e0 R14: e9f1a1e0 R15: 00a00037
FS: 7fbac36f7540() GS:8b53edbc() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7fbac364cfa7 CR3: 0004a6d48000 CR4: 000406e0
Call Trace:
kobject_add+0x71/0xd0
? _cond_resched+0x19/0x40
? mutex_lock+0x12/0x40
device_add+0x12e/0x6b0
device_create_groups_vargs+0xe4/0xf0
device_create_with_groups+0x3f/0x60
? _cond_resched+0x19/0x40
misc_register+0x140/0x180
binder_init+0x1ed/0x2d4 [binder_linux]
? trace_event_define_fields_binder_transaction_fd_send+0x8e/0x8e [binder_linux]
do_one_initcall+0x4a/0x1c9
? _cond_resched+0x19/0x40
? kmem_cache_alloc_trace+0x151/0x1c0
do_init_module+0x5f/0x216
load_module+0x223d/0x2b20
__do_sys_finit_module+0xfc/0x120
? __do_sys_finit_module+0xfc/0x120
__x64_sys_finit_module+0x1a/0x20
do_syscall_64+0x5a/0x120
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x7fbac3202839
Code: 00 f3 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48
89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01
c3 48 8b 0d 1f f6 2c 00 f7 d8 64 89 01 48
RSP: 002b:7ffd1494a908 EFLAGS: 0246 ORIG_RAX: 0139
RAX: ffda RBX: 55b629ebec60 RCX: 7fbac3202839
RDX: RSI: 55b629c20d2e RDI: 0003
RBP: 55b629c20d2e R08: R09: 55b629ec2310
R10: 0003 R11: 0246 R12:
R13: 55b629ebed70 R14: 0004 R15: 55b629ebec60
So check for the empty string since strsep() will otherwise return the
emtpy string which will cause kobject_add_internal() to panic when trying
to add a kobject with an emtpy name.
Fixes: ac4812c5ffbb ("binder: Support multiple /dev instances")
Cc: Martijn Coenen
Signed-off-by: Christian Brauner
---
drivers/android/binder.c | 30 --
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 751d76173f81..5f7d6fe06eec 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5898,21 +5898,23 @@ static int __init binder_init(void)
&transaction_log_fops);
}
- /*
-* Copy the module_parameter string, because we don't want to
-* tokenize it in-place.
-*/
- device_names = kstrdup(binder_devices_param, GFP_KERNEL);
- if (!device_names) {
- ret = -ENOMEM;
- goto err_alloc_device_names_failed;
- }
+ if (strcmp(binder_devices_param, "") != 0) {
+ /*
+ * Copy the module_parameter string, because we don't want to
+ * tokenize it in-place.
+*/
+ device_names = kstrdup(binder_devices_param, GFP_KERNEL);
+ if (!device_names) {
+ ret = -ENOMEM;
+ goto err_alloc_device_names_failed;
+ }
- device_tmp = device_names;
- while ((device_name = strsep(&device_tmp, ","))) {
- ret = init_binder_device(device_name);
- if (ret)
- goto err_init_binder_device_failed;
+ device_tmp = device_names;
+ while ((device_name = strsep(&device_tmp, ","))) {
+ ret = init_binder_device(device_name);
+
Re: [PATCH 2/2] binderfs: remove separate device_initcall()
On Wed, Jan 30, 2019 at 03:24:12PM +0100, Greg KH wrote:
> On Wed, Jan 23, 2019 at 12:41:16PM +0100, Christian Brauner wrote:
> > binderfs should not have a separate device_initcall(). When a kernel is
> > compiled with CONFIG_ANDROID_BINDERFS register the filesystem alongside
> > CONFIG_ANDROID_IPC. This use-case is especially sensible when users specify
> > CONFIG_ANDROID_IPC=y, CONFIG_ANDROID_BINDERFS=y and
> > ANDROID_BINDER_DEVICES="".
> > When CONFIG_ANDROID_BINDERFS=n then this always succeeds so there's no
> > regression potential for legacy workloads.
> >
> > Signed-off-by: Christian Brauner
> > ---
> > drivers/android/binder.c | 4
> > drivers/android/binder_internal.h | 9 +
> > drivers/android/binderfs.c| 4 +---
> > 3 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index cdfc87629efb..751d76173f81 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -5915,6 +5915,10 @@ static int __init binder_init(void)
> > goto err_init_binder_device_failed;
> > }
> >
> > + ret = init_binderfs();
> > + if (ret)
> > + goto err_init_binder_device_failed;
> > +
> > return ret;
> >
> > err_init_binder_device_failed:
> > diff --git a/drivers/android/binder_internal.h
> > b/drivers/android/binder_internal.h
> > index 7fb97f503ef2..045b3e42d98b 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -46,4 +46,13 @@ static inline bool is_binderfs_device(const struct inode
> > *inode)
> > }
> > #endif
> >
> > +#ifdef CONFIG_ANDROID_BINDERFS
> > +extern int __init init_binderfs(void);
> > +#else
> > +static inline int __init init_binderfs(void)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > #endif /* _LINUX_BINDER_INTERNAL_H */
> > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > index 7a550104a722..e773f45d19d9 100644
> > --- a/drivers/android/binderfs.c
> > +++ b/drivers/android/binderfs.c
> > @@ -550,7 +550,7 @@ static struct file_system_type binder_fs_type = {
> > .fs_flags = FS_USERNS_MOUNT,
> > };
> >
> > -static int __init init_binderfs(void)
> > +int __init init_binderfs(void)
> > {
> > int ret;
> >
> > @@ -568,5 +568,3 @@ static int __init init_binderfs(void)
> >
> > return ret;
> > }
> > -
> > -device_initcall(init_binderfs);
>
> I get a build warning when applying this patch :(
Hm, I can't reproduce that build error with this patch applied to what
you currently have in char-misc-linus. :(
Any chance you can give me the config that produced this warning?
I tried with CONFIG_BINDERFS=y and CONFIG_BINDERFS=n.
Thanks!
Christian
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v1] binderfs: remove separate device_initcall()
binderfs should not have a separate device_initcall(). When a kernel is
compiled with CONFIG_ANDROID_BINDERFS register the filesystem alongside
CONFIG_ANDROID_IPC. This use-case is especially sensible when users specify
CONFIG_ANDROID_IPC=y, CONFIG_ANDROID_BINDERFS=y and
ANDROID_BINDER_DEVICES="".
When CONFIG_ANDROID_BINDERFS=n then this always succeeds so there's no
regression potential for legacy workloads.
Signed-off-by: Christian Brauner
---
/* Changelog */
- ensure that device_name is set to NULL so kfree() doesn't freak out
---
drivers/android/binder.c | 7 ++-
drivers/android/binder_internal.h | 9 +
drivers/android/binderfs.c| 4 +---
3 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index 57cf259de600..4d2b2ad1ee0e 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -5854,9 +5854,10 @@ static int __init init_binder_device(const char *name)
static int __init binder_init(void)
{
int ret;
- char *device_name, *device_names, *device_tmp;
+ char *device_name, *device_tmp;
struct binder_device *device;
struct hlist_node *tmp;
+ char *device_names = NULL;
ret = binder_alloc_shrinker_init();
if (ret)
@@ -5917,6 +5918,10 @@ static int __init binder_init(void)
}
}
+ ret = init_binderfs();
+ if (ret)
+ goto err_init_binder_device_failed;
+
return ret;
err_init_binder_device_failed:
diff --git a/drivers/android/binder_internal.h
b/drivers/android/binder_internal.h
index 7fb97f503ef2..045b3e42d98b 100644
--- a/drivers/android/binder_internal.h
+++ b/drivers/android/binder_internal.h
@@ -46,4 +46,13 @@ static inline bool is_binderfs_device(const struct inode
*inode)
}
#endif
+#ifdef CONFIG_ANDROID_BINDERFS
+extern int __init init_binderfs(void);
+#else
+static inline int __init init_binderfs(void)
+{
+ return 0;
+}
+#endif
+
#endif /* _LINUX_BINDER_INTERNAL_H */
diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 7a550104a722..e773f45d19d9 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -550,7 +550,7 @@ static struct file_system_type binder_fs_type = {
.fs_flags = FS_USERNS_MOUNT,
};
-static int __init init_binderfs(void)
+int __init init_binderfs(void)
{
int ret;
@@ -568,5 +568,3 @@ static int __init init_binderfs(void)
return ret;
}
-
-device_initcall(init_binderfs);
--
2.20.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
