[PATCH v1 0/7] binderfs: debug galore

[PATCH v1 0/7] binderfs: debug galore

[Christian Brauner]

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

[PATCH v1 1/7] binderfs: remove outdated comment

[Christian Brauner]

The comment stems from an early version of that patchset and is just
confusing now.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* 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

[PATCH v1 2/7] binderfs: prevent renaming the control dentry

[Christian Brauner]

- 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 <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* 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

[PATCH v1 3/7] binderfs: rework binderfs_fill_super()

[Christian Brauner]

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 <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* 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

[PATCH v1 4/7] binderfs: rework binderfs_binder_device_create()

[Christian Brauner]

- 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 <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* 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 <linux/kdev_t.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/namei.h>
 #include <linux/magic.h>
 #include <linux/major.h>
 #include <linux/miscdevice.h>
@@ -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

[PATCH v1 5/7] binderfs: kill_litter_super() before cleanup

[Christian Brauner]

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 <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* 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

[PATCH v1 6/7] binderfs: drop lock in binderfs_binder_ctl_create

[Christian Brauner]

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 <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
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

[PATCH v1 7/7] binderfs: switch from d_add() to d_instantiate()

[Christian Brauner]

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:ffffb8c984e27ad0 EFLAGS: 00000282 ORIG_RAX: ffffffffffffff13
[  744.441889] RAX: 0000000000000038 RBX: ffff9407ef770c08 RCX: ffffb8c980011000
[  744.441889] RDX: ffffb8c984e27b54 RSI: ffffb8c984e27ce0 RDI: ffff9407e6689600
[  744.441889] RBP: ffffb8c984e27b28 R08: ffffb8c984e27ba4 R09: 0000000000000007
[  744.441889] R10: ffff9407e5c4f05c R11: 973f3eb9d84a94e5 R12: 0000000000000002
[  744.441889] R13: ffff9407e6689600 R14: 0000000000000007 R15: 00000007bfef7a13
[  744.441889] FS:  00007f0db13bb740(0000) GS:ffff9407f3b00000(0000) knlGS:0000000000000000
[  744.441889] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  744.441889] CR2: 00007f0dacc51024 CR3: 000000032961a000 CR4: 00000000000006e0
[  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:00007ffc36bc9388 EFLAGS: 00000246 ORIG_RAX: 0000000000000004
[  744.441889] RAX: ffffffffffffffda RBX: 00007ffc36bc9300 RCX: 00007f0db0ec2775
[  744.441889] RDX: 00007ffc36bc9400 RSI: 00007ffc36bc9400 RDI: 00007f0dad26f050
[  744.441889] RBP: 0000000000c0bc60 R08: 0000000000000000 R09: 0000000000000001
[  744.441889] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffc36bc9400
[  744.441889] R13: 0000000000000001 R14: 00000000ffffff9c R15: 0000000000c0bc60

Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Christian Brauner <christian@brauner.io>
---
/* 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