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

[Dave Chinner <david@fromorbit.com>]

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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs