[0/5] binderfs: debug galore

Message

Christian Brauner Jan. 18, 2019, 2:53 p.m. UTC

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(-)

Comments

Al Viro Jan. 18, 2019, 11:26 p.m. UTC | #1

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));
	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:
        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)
		....

Christian Brauner Jan. 19, 2019, 3:55 p.m. UTC | #2

On Fri, Jan 18, 2019 at 11:26:34PM +0000, 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.