Re: [PATCH 0/5] binderfs: debug galore

[Christian Brauner <christian@brauner.io>]

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.