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.
