Re: [PATCH 1/7] xfs: fix dentry aliasing issues in open_by_handle

Dave Chinner
2009-01-11T23:33:12Z

On Fri, Jan 09, 2009 at 05:11:05PM -0500, Christoph Hellwig wrote:
> Open by handle just grabs an inode by handle and then creates itself
> a dentry for it. While this works for regular files it is horribly
> broken for directories, where the VFS locking relies on the fact that
> there is only just one single dentry for a given inode, and that
> these are always connected to the root of the filesystem so that
> it's locking algorithms work (see Documentations/filesystems/Locking)
>
> Remove all the existing open by handle code and replace it with a small
> wrapper around the exportfs code which deals with all these issues.
> At the same time we also make the checks for a valid handle strict
> enough to reject all not perfectly well formed handles - given that
> we never hand out others that's okay and simplifies the code.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
....
> +handle_acceptable(
> + void *context,
> + struct dentry *dentry)
> +{
> + return 1;
> +}

That should probably be namespaced correctly because it won't be
static on debug builds. i.e. xfs_handle_acceptable()

> - dentry = d_obtain_alias(inode);
> - if (IS_ERR(dentry)) {
> - put_unused_fd(new_fd);
> - return PTR_ERR(dentry);
> + fd = get_unused_fd();
> + if (fd < 0) {
> + error = fd;
> + goto out_dput;
> }
>
> - /* Ensure umount returns EBUSY on umounts while this file is open. */
> - mntget(parfilp->f_path.mnt);
> -
> - /* Create file pointer. */
> - filp = dentry_open(dentry, parfilp->f_path.mnt, hreq->oflags, cred);
> + filp = dentry_open(dentry, mntget(parfilp->f_path.mnt),
> + hreq->oflags, cred);
> if (IS_ERR(filp)) {
> - put_unused_fd(new_fd);
> - return -XFS_ERROR(-PTR_ERR(filp));
> + put_unused_fd(fd);
> + return PTR_ERR(filp);
> }

Doesn't that error leak a mount+dentry reference? i.e. we do a mntget()
when calling dentry_open(), but we don't drop the reference on
error.

Ah, no, dentry_open() drops both the reference and the dentry on error.
That's ok, then.

> STATIC int
> xfs_attrlist_by_handle(
> - xfs_mount_t *mp,
> void __user *arg,
> - struct inode *parinode)
> + struct file *parfilp)

The args in this function are back to front compared to all the
other functions - the others are (filp, arg), this one is the
opposite.

> case XFS_IOC_READLINK_BY_HANDLE: {
> xfs_fsop_handlereq_t hreq;
>
> if (copy_from_user(&hreq, arg, sizeof(xfs_fsop_handlereq_t)))
> return -XFS_ERROR(EFAULT);
> - return xfs_readlink_by_handle(mp, &hreq, inode);
> + return xfs_readlink_by_handle(filp, &hreq);
> }
> case XFS_IOC_ATTRLIST_BY_HANDLE:
> - return xfs_attrlist_by_handle(mp, arg, inode);
> + return xfs_attrlist_by_handle(arg, filp);

As can be seen here.

Other than that, it looks ok.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com