[PATCH 0/7] Remaining 2.6.29 queue

[PATCH 0/7] Remaining 2.6.29 queue

[Christoph Hellwig]

These are my current outstanding patches for 2.6.29, consisting of
small enough fixes and lockdep annotations, and an early warning
for a attr corruption frequently seen on kerneloops.org.

All but the last patch in the series still need some proper review.

-- 

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

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

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-open-by-handle --]
[-- Type: text/plain, Size: 23890 bytes --]

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>

Index: xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2008-12-30 09:29:46.787493310 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_ioctl.c	2008-12-30 09:37:24.840514983 +0100
@@ -50,12 +50,14 @@
 #include "xfs_vnodeops.h"
 #include "xfs_quota.h"
 #include "xfs_inode_item.h"
+#include "xfs_export.h"
 
 #include <linux/capability.h>
 #include <linux/dcache.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
 #include <linux/pagemap.h>
+#include <linux/exportfs.h>
 
 /*
  * xfs_find_handle maps from userspace xfs_fsop_handlereq structure to
@@ -164,97 +166,69 @@ xfs_find_handle(
 	return 0;
 }
 
-
 /*
- * Convert userspace handle data into inode.
- *
- * We use the fact that all the fsop_handlereq ioctl calls have a data
- * structure argument whose first component is always a xfs_fsop_handlereq_t,
- * so we can pass that sub structure into this handy, shared routine.
- *
- * If no error, caller must always iput the returned inode.
+ * No need to do permission checks on the various pathname components
+ * as the handle operations are privileged.
  */
 STATIC int
-xfs_vget_fsop_handlereq(
-	xfs_mount_t		*mp,
-	struct inode		*parinode,	/* parent inode pointer    */
-	xfs_fsop_handlereq_t	*hreq,
-	struct inode		**inode)
-{
-	void			__user *hanp;
-	size_t			hlen;
-	xfs_fid_t		*xfid;
-	xfs_handle_t		*handlep;
+handle_acceptable(
+	void			*context,
+	struct dentry		*dentry)
+{
+	return 1;
+}
+
+/*
+ * Convert userspace handle data into a dentry.
+ */
+struct dentry *
+xfs_handle_to_dentry(
+	struct file		*parfilp,
+	void __user		*uhandle,
+	u32			hlen)
+{
 	xfs_handle_t		handle;
-	xfs_inode_t		*ip;
-	xfs_ino_t		ino;
-	__u32			igen;
-	int			error;
+	struct xfs_fid64	fid;
 
 	/*
 	 * Only allow handle opens under a directory.
 	 */
-	if (!S_ISDIR(parinode->i_mode))
-		return XFS_ERROR(ENOTDIR);
+	if (!S_ISDIR(parfilp->f_path.dentry->d_inode->i_mode))
+		return ERR_PTR(-ENOTDIR);
 
-	hanp = hreq->ihandle;
-	hlen = hreq->ihandlen;
-	handlep = &handle;
-
-	if (hlen < sizeof(handlep->ha_fsid) || hlen > sizeof(*handlep))
-		return XFS_ERROR(EINVAL);
-	if (copy_from_user(handlep, hanp, hlen))
-		return XFS_ERROR(EFAULT);
-	if (hlen < sizeof(*handlep))
-		memset(((char *)handlep) + hlen, 0, sizeof(*handlep) - hlen);
-	if (hlen > sizeof(handlep->ha_fsid)) {
-		if (handlep->ha_fid.fid_len !=
-		    (hlen - sizeof(handlep->ha_fsid) -
-		            sizeof(handlep->ha_fid.fid_len)) ||
-		    handlep->ha_fid.fid_pad)
-			return XFS_ERROR(EINVAL);
-	}
-
-	/*
-	 * Crack the handle, obtain the inode # & generation #
-	 */
-	xfid = (struct xfs_fid *)&handlep->ha_fid;
-	if (xfid->fid_len == sizeof(*xfid) - sizeof(xfid->fid_len)) {
-		ino  = xfid->fid_ino;
-		igen = xfid->fid_gen;
-	} else {
-		return XFS_ERROR(EINVAL);
-	}
-
-	/*
-	 * Get the XFS inode, building a Linux inode to go with it.
-	 */
-	error = xfs_iget(mp, NULL, ino, 0, XFS_ILOCK_SHARED, &ip, 0);
-	if (error)
-		return error;
-	if (ip == NULL)
-		return XFS_ERROR(EIO);
-	if (ip->i_d.di_gen != igen) {
-		xfs_iput_new(ip, XFS_ILOCK_SHARED);
-		return XFS_ERROR(ENOENT);
-	}
-
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	if (hlen != sizeof(xfs_handle_t))
+		return ERR_PTR(-EINVAL);
+	if (copy_from_user(&handle, uhandle, hlen))
+		return ERR_PTR(-EFAULT);
+	if (handle.ha_fid.fid_len !=
+	    sizeof(handle.ha_fid) - sizeof(handle.ha_fid.fid_len))
+		return ERR_PTR(-EINVAL);
+
+	memset(&fid, 0, sizeof(struct fid));
+	fid.ino = handle.ha_fid.fid_ino;
+	fid.gen = handle.ha_fid.fid_gen;
+
+	return exportfs_decode_fh(parfilp->f_path.mnt, (struct fid *)&fid, 3,
+			FILEID_INO32_GEN | XFS_FILEID_TYPE_64FLAG,
+			handle_acceptable, NULL);
+}
 
-	*inode = VFS_I(ip);
-	return 0;
+STATIC struct dentry *
+xfs_handlereq_to_dentry(
+	struct file		*parfilp,
+	xfs_fsop_handlereq_t	*hreq)
+{
+	return xfs_handle_to_dentry(parfilp, hreq->ihandle, hreq->ihandlen);
 }
 
 int
 xfs_open_by_handle(
-	xfs_mount_t		*mp,
-	xfs_fsop_handlereq_t	*hreq,
 	struct file		*parfilp,
-	struct inode		*parinode)
+	xfs_fsop_handlereq_t	*hreq)
 {
 	const struct cred	*cred = current_cred();
 	int			error;
-	int			new_fd;
+	int			fd;
 	int			permflag;
 	struct file		*filp;
 	struct inode		*inode;
@@ -263,19 +237,21 @@ xfs_open_by_handle(
 	if (!capable(CAP_SYS_ADMIN))
 		return -XFS_ERROR(EPERM);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, hreq, &inode);
-	if (error)
-		return -error;
+	dentry = xfs_handlereq_to_dentry(parfilp, hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
+	inode = dentry->d_inode;
 
 	/* Restrict xfs_open_by_handle to directories & regular files. */
 	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
-		iput(inode);
-		return -XFS_ERROR(EINVAL);
+		error = -XFS_ERROR(EPERM);
+		goto out_dput;
 	}
 
 #if BITS_PER_LONG != 32
 	hreq->oflags |= O_LARGEFILE;
 #endif
+
 	/* Put open permission in namei format. */
 	permflag = hreq->oflags;
 	if ((permflag+1) & O_ACCMODE)
@@ -285,50 +261,45 @@ xfs_open_by_handle(
 
 	if ((!(permflag & O_APPEND) || (permflag & O_TRUNC)) &&
 	    (permflag & FMODE_WRITE) && IS_APPEND(inode)) {
-		iput(inode);
-		return -XFS_ERROR(EPERM);
+		error = -XFS_ERROR(EPERM);
+		goto out_dput;
 	}
 
 	if ((permflag & FMODE_WRITE) && IS_IMMUTABLE(inode)) {
-		iput(inode);
-		return -XFS_ERROR(EACCES);
+		error = -XFS_ERROR(EACCES);
+		goto out_dput;
 	}
 
 	/* Can't write directories. */
-	if ( S_ISDIR(inode->i_mode) && (permflag & FMODE_WRITE)) {
-		iput(inode);
-		return -XFS_ERROR(EISDIR);
-	}
-
-	if ((new_fd = get_unused_fd()) < 0) {
-		iput(inode);
-		return new_fd;
+	if (S_ISDIR(inode->i_mode) && (permflag & FMODE_WRITE)) {
+		error = -XFS_ERROR(EISDIR);
+		goto out_dput;
 	}
 
-	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);
 	}
 
 	if (inode->i_mode & S_IFREG) {
-		/* invisible operation should not change atime */
 		filp->f_flags |= O_NOATIME;
 		filp->f_mode |= FMODE_NOCMTIME;
 	}
 
-	fd_install(new_fd, filp);
-	return new_fd;
+	fd_install(fd, filp);
+	return fd;
+
+ out_dput:
+	dput(dentry);
+	return error;
 }
 
 /*
@@ -359,11 +330,10 @@ do_readlink(
 
 int
 xfs_readlink_by_handle(
-	xfs_mount_t		*mp,
-	xfs_fsop_handlereq_t	*hreq,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	xfs_fsop_handlereq_t	*hreq)
 {
-	struct inode		*inode;
+	struct dentry		*dentry;
 	__u32			olen;
 	void			*link;
 	int			error;
@@ -371,26 +341,28 @@ xfs_readlink_by_handle(
 	if (!capable(CAP_SYS_ADMIN))
 		return -XFS_ERROR(EPERM);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, hreq, &inode);
-	if (error)
-		return -error;
+	dentry = xfs_handlereq_to_dentry(parfilp, hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
 	/* Restrict this handle operation to symlinks only. */
-	if (!S_ISLNK(inode->i_mode)) {
+	if (!S_ISLNK(dentry->d_inode->i_mode)) {
 		error = -XFS_ERROR(EINVAL);
-		goto out_iput;
+		goto out_dput;
 	}
 
 	if (copy_from_user(&olen, hreq->ohandlen, sizeof(__u32))) {
 		error = -XFS_ERROR(EFAULT);
-		goto out_iput;
+		goto out_dput;
 	}
 
 	link = kmalloc(MAXPATHLEN+1, GFP_KERNEL);
-	if (!link)
-		goto out_iput;
+	if (!link) {
+		error = -XFS_ERROR(ENOMEM);
+		goto out_dput;
+	}
 
-	error = -xfs_readlink(XFS_I(inode), link);
+	error = -xfs_readlink(XFS_I(dentry->d_inode), link);
 	if (error)
 		goto out_kfree;
 	error = do_readlink(hreq->ohandle, olen, link);
@@ -399,32 +371,31 @@ xfs_readlink_by_handle(
 
  out_kfree:
 	kfree(link);
- out_iput:
-	iput(inode);
+ out_dput:
+	dput(dentry);
 	return error;
 }
 
 STATIC int
 xfs_fssetdm_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	void			__user *arg)
 {
 	int			error;
 	struct fsdmidata	fsd;
 	xfs_fsop_setdm_handlereq_t dmhreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 
 	if (!capable(CAP_MKNOD))
 		return -XFS_ERROR(EPERM);
 	if (copy_from_user(&dmhreq, arg, sizeof(xfs_fsop_setdm_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, &dmhreq.hreq, &inode);
-	if (error)
-		return -error;
+	dentry = xfs_handlereq_to_dentry(parfilp, &dmhreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) {
+	if (IS_IMMUTABLE(dentry->d_inode) || IS_APPEND(dentry->d_inode)) {
 		error = -XFS_ERROR(EPERM);
 		goto out;
 	}
@@ -434,24 +405,23 @@ xfs_fssetdm_by_handle(
 		goto out;
 	}
 
-	error = -xfs_set_dmattrs(XFS_I(inode), fsd.fsd_dmevmask,
+	error = -xfs_set_dmattrs(XFS_I(dentry->d_inode), fsd.fsd_dmevmask,
 				 fsd.fsd_dmstate);
 
  out:
-	iput(inode);
+	dput(dentry);
 	return error;
 }
 
 STATIC int
 xfs_attrlist_by_handle(
-	xfs_mount_t		*mp,
 	void			__user *arg,
-	struct inode		*parinode)
+	struct file		*parfilp)
 {
-	int			error;
+	int			error = -ENOMEM;
 	attrlist_cursor_kern_t	*cursor;
 	xfs_fsop_attrlist_handlereq_t al_hreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 	char			*kbuf;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -467,16 +437,16 @@ xfs_attrlist_by_handle(
 	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
 		return -XFS_ERROR(EINVAL);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, &al_hreq.hreq, &inode);
-	if (error)
-		goto out;
+	dentry = xfs_handlereq_to_dentry(parfilp, &al_hreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
 	kbuf = kmalloc(al_hreq.buflen, GFP_KERNEL);
 	if (!kbuf)
-		goto out_vn_rele;
+		goto out_dput;
 
 	cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
-	error = xfs_attr_list(XFS_I(inode), kbuf, al_hreq.buflen,
+	error = -xfs_attr_list(XFS_I(dentry->d_inode), kbuf, al_hreq.buflen,
 					al_hreq.flags, cursor);
 	if (error)
 		goto out_kfree;
@@ -486,10 +456,9 @@ xfs_attrlist_by_handle(
 
  out_kfree:
 	kfree(kbuf);
- out_vn_rele:
-	iput(inode);
- out:
-	return -error;
+ out_dput:
+	dput(dentry);
+	return error;
 }
 
 int
@@ -564,15 +533,13 @@ xfs_attrmulti_attr_remove(
 
 STATIC int
 xfs_attrmulti_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
 	struct file		*parfilp,
-	struct inode		*parinode)
+	void			__user *arg)
 {
 	int			error;
 	xfs_attr_multiop_t	*ops;
 	xfs_fsop_attrmulti_handlereq_t am_hreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 	unsigned int		i, size;
 	char			*attr_name;
 
@@ -581,19 +548,19 @@ xfs_attrmulti_by_handle(
 	if (copy_from_user(&am_hreq, arg, sizeof(xfs_fsop_attrmulti_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, &am_hreq.hreq, &inode);
-	if (error)
-		goto out;
+	dentry = xfs_handlereq_to_dentry(parfilp, &am_hreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
 	error = E2BIG;
 	size = am_hreq.opcount * sizeof(xfs_attr_multiop_t);
 	if (!size || size > 16 * PAGE_SIZE)
-		goto out_vn_rele;
+		goto out_dput;
 
 	error = ENOMEM;
 	ops = kmalloc(size, GFP_KERNEL);
 	if (!ops)
-		goto out_vn_rele;
+		goto out_dput;
 
 	error = EFAULT;
 	if (copy_from_user(ops, am_hreq.ops, size))
@@ -615,25 +582,28 @@ xfs_attrmulti_by_handle(
 
 		switch (ops[i].am_opcode) {
 		case ATTR_OP_GET:
-			ops[i].am_error = xfs_attrmulti_attr_get(inode,
-					attr_name, ops[i].am_attrvalue,
-					&ops[i].am_length, ops[i].am_flags);
+			ops[i].am_error = xfs_attrmulti_attr_get(
+					dentry->d_inode, attr_name,
+					ops[i].am_attrvalue, &ops[i].am_length,
+					ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
 			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
 			if (ops[i].am_error)
 				break;
-			ops[i].am_error = xfs_attrmulti_attr_set(inode,
-					attr_name, ops[i].am_attrvalue,
-					ops[i].am_length, ops[i].am_flags);
+			ops[i].am_error = xfs_attrmulti_attr_set(
+					dentry->d_inode, attr_name,
+					ops[i].am_attrvalue, ops[i].am_length,
+					ops[i].am_flags);
 			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		case ATTR_OP_REMOVE:
 			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
 			if (ops[i].am_error)
 				break;
-			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
-					attr_name, ops[i].am_flags);
+			ops[i].am_error = xfs_attrmulti_attr_remove(
+					dentry->d_inode, attr_name,
+					ops[i].am_flags);
 			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		default:
@@ -647,9 +617,8 @@ xfs_attrmulti_by_handle(
 	kfree(attr_name);
  out_kfree_ops:
 	kfree(ops);
- out_vn_rele:
-	iput(inode);
- out:
+ out_dput:
+	dput(dentry);
 	return -error;
 }
 
@@ -1440,23 +1409,23 @@ xfs_file_ioctl(
 
 		if (copy_from_user(&hreq, arg, sizeof(xfs_fsop_handlereq_t)))
 			return -XFS_ERROR(EFAULT);
-		return xfs_open_by_handle(mp, &hreq, filp, inode);
+		return xfs_open_by_handle(filp, &hreq);
 	}
 	case XFS_IOC_FSSETDM_BY_HANDLE:
-		return xfs_fssetdm_by_handle(mp, arg, inode);
+		return xfs_fssetdm_by_handle(filp, arg);
 
 	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);
 
 	case XFS_IOC_ATTRMULTI_BY_HANDLE:
-		return xfs_attrmulti_by_handle(mp, arg, filp, inode);
+		return xfs_attrmulti_by_handle(filp, arg);
 
 	case XFS_IOC_SWAPEXT: {
 		struct xfs_swapext	sxp;
Index: xfs/fs/xfs/linux-2.6/xfs_ioctl.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.h	2008-12-30 09:29:07.512648632 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_ioctl.h	2008-12-30 09:30:16.267493085 +0100
@@ -34,16 +34,13 @@ xfs_find_handle(
 
 extern int
 xfs_open_by_handle(
-	xfs_mount_t		*mp,
-	xfs_fsop_handlereq_t	*hreq,
 	struct file		*parfilp,
-	struct inode		*parinode);
+	xfs_fsop_handlereq_t	*hreq);
 
 extern int
 xfs_readlink_by_handle(
-	xfs_mount_t		*mp,
-	xfs_fsop_handlereq_t	*hreq,
-	struct inode		*parinode);
+	struct file		*parfilp,
+	xfs_fsop_handlereq_t	*hreq);
 
 extern int
 xfs_attrmulti_attr_get(
@@ -67,6 +64,12 @@ xfs_attrmulti_attr_remove(
 	char			*name,
 	__uint32_t		flags);
 
+extern struct dentry *
+xfs_handle_to_dentry(
+	struct file		*parfilp,
+	void __user		*uhandle,
+	u32			hlen);
+
 extern long
 xfs_file_ioctl(
 	struct file		*filp,
Index: xfs/fs/xfs/linux-2.6/xfs_ioctl32.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl32.c	2008-12-30 09:29:07.519618655 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_ioctl32.c	2008-12-30 09:30:16.271523207 +0100
@@ -340,96 +340,24 @@ xfs_compat_handlereq_copyin(
 	return 0;
 }
 
-/*
- * Convert userspace handle data into inode.
- *
- * We use the fact that all the fsop_handlereq ioctl calls have a data
- * structure argument whose first component is always a xfs_fsop_handlereq_t,
- * so we can pass that sub structure into this handy, shared routine.
- *
- * If no error, caller must always iput the returned inode.
- */
-STATIC int
-xfs_vget_fsop_handlereq_compat(
-	xfs_mount_t		*mp,
-	struct inode		*parinode,	/* parent inode pointer    */
-	compat_xfs_fsop_handlereq_t	*hreq,
-	struct inode		**inode)
+STATIC struct dentry *
+xfs_compat_handlereq_to_dentry(
+	struct file		*parfilp,
+	compat_xfs_fsop_handlereq_t *hreq)
 {
-	void			__user *hanp;
-	size_t			hlen;
-	xfs_fid_t		*xfid;
-	xfs_handle_t		*handlep;
-	xfs_handle_t		handle;
-	xfs_inode_t		*ip;
-	xfs_ino_t		ino;
-	__u32			igen;
-	int			error;
-
-	/*
-	 * Only allow handle opens under a directory.
-	 */
-	if (!S_ISDIR(parinode->i_mode))
-		return XFS_ERROR(ENOTDIR);
-
-	hanp = compat_ptr(hreq->ihandle);
-	hlen = hreq->ihandlen;
-	handlep = &handle;
-
-	if (hlen < sizeof(handlep->ha_fsid) || hlen > sizeof(*handlep))
-		return XFS_ERROR(EINVAL);
-	if (copy_from_user(handlep, hanp, hlen))
-		return XFS_ERROR(EFAULT);
-	if (hlen < sizeof(*handlep))
-		memset(((char *)handlep) + hlen, 0, sizeof(*handlep) - hlen);
-	if (hlen > sizeof(handlep->ha_fsid)) {
-		if (handlep->ha_fid.fid_len !=
-		    (hlen - sizeof(handlep->ha_fsid) -
-			    sizeof(handlep->ha_fid.fid_len)) ||
-		    handlep->ha_fid.fid_pad)
-			return XFS_ERROR(EINVAL);
-	}
-
-	/*
-	 * Crack the handle, obtain the inode # & generation #
-	 */
-	xfid = (struct xfs_fid *)&handlep->ha_fid;
-	if (xfid->fid_len == sizeof(*xfid) - sizeof(xfid->fid_len)) {
-		ino  = xfid->fid_ino;
-		igen = xfid->fid_gen;
-	} else {
-		return XFS_ERROR(EINVAL);
-	}
-
-	/*
-	 * Get the XFS inode, building a Linux inode to go with it.
-	 */
-	error = xfs_iget(mp, NULL, ino, 0, XFS_ILOCK_SHARED, &ip, 0);
-	if (error)
-		return error;
-	if (ip == NULL)
-		return XFS_ERROR(EIO);
-	if (ip->i_d.di_gen != igen) {
-		xfs_iput_new(ip, XFS_ILOCK_SHARED);
-		return XFS_ERROR(ENOENT);
-	}
-
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
-
-	*inode = VFS_I(ip);
-	return 0;
+	return xfs_handle_to_dentry(parfilp,
+			compat_ptr(hreq->ihandle), hreq->ihandlen);
 }
 
 STATIC int
 xfs_compat_attrlist_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	void			__user *arg)
 {
 	int			error;
 	attrlist_cursor_kern_t	*cursor;
 	compat_xfs_fsop_attrlist_handlereq_t al_hreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 	char			*kbuf;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -446,17 +374,17 @@ xfs_compat_attrlist_by_handle(
 	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
 		return -XFS_ERROR(EINVAL);
 
-	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &al_hreq.hreq,
-					       &inode);
-	if (error)
-		goto out;
+	dentry = xfs_compat_handlereq_to_dentry(parfilp, &al_hreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
+	error = -ENOMEM;
 	kbuf = kmalloc(al_hreq.buflen, GFP_KERNEL);
 	if (!kbuf)
-		goto out_vn_rele;
+		goto out_dput;
 
 	cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
-	error = xfs_attr_list(XFS_I(inode), kbuf, al_hreq.buflen,
+	error = -xfs_attr_list(XFS_I(dentry->d_inode), kbuf, al_hreq.buflen,
 					al_hreq.flags, cursor);
 	if (error)
 		goto out_kfree;
@@ -466,22 +394,20 @@ xfs_compat_attrlist_by_handle(
 
  out_kfree:
 	kfree(kbuf);
- out_vn_rele:
-	iput(inode);
- out:
-	return -error;
+ out_dput:
+	dput(dentry);
+	return error;
 }
 
 STATIC int
 xfs_compat_attrmulti_by_handle(
-	xfs_mount_t				*mp,
-	void					__user *arg,
-	struct inode				*parinode)
+	struct file				*parfilp,
+	void					__user *arg)
 {
 	int					error;
 	compat_xfs_attr_multiop_t		*ops;
 	compat_xfs_fsop_attrmulti_handlereq_t	am_hreq;
-	struct inode				*inode;
+	struct dentry				*dentry;
 	unsigned int				i, size;
 	char					*attr_name;
 
@@ -491,20 +417,19 @@ xfs_compat_attrmulti_by_handle(
 			   sizeof(compat_xfs_fsop_attrmulti_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
 
-	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &am_hreq.hreq,
-					       &inode);
-	if (error)
-		goto out;
+	dentry = xfs_compat_handlereq_to_dentry(parfilp, &am_hreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
 	error = E2BIG;
 	size = am_hreq.opcount * sizeof(compat_xfs_attr_multiop_t);
 	if (!size || size > 16 * PAGE_SIZE)
-		goto out_vn_rele;
+		goto out_dput;
 
 	error = ENOMEM;
 	ops = kmalloc(size, GFP_KERNEL);
 	if (!ops)
-		goto out_vn_rele;
+		goto out_dput;
 
 	error = EFAULT;
 	if (copy_from_user(ops, compat_ptr(am_hreq.ops), size))
@@ -527,20 +452,21 @@ xfs_compat_attrmulti_by_handle(
 
 		switch (ops[i].am_opcode) {
 		case ATTR_OP_GET:
-			ops[i].am_error = xfs_attrmulti_attr_get(inode,
-					attr_name,
+			ops[i].am_error = xfs_attrmulti_attr_get(
+					dentry->d_inode, attr_name,
 					compat_ptr(ops[i].am_attrvalue),
 					&ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
-			ops[i].am_error = xfs_attrmulti_attr_set(inode,
-					attr_name,
+			ops[i].am_error = xfs_attrmulti_attr_set(
+					dentry->d_inode, attr_name,
 					compat_ptr(ops[i].am_attrvalue),
 					ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_REMOVE:
-			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
-					attr_name, ops[i].am_flags);
+			ops[i].am_error = xfs_attrmulti_attr_remove(
+					dentry->d_inode, attr_name,
+					ops[i].am_flags);
 			break;
 		default:
 			ops[i].am_error = EINVAL;
@@ -553,22 +479,20 @@ xfs_compat_attrmulti_by_handle(
 	kfree(attr_name);
  out_kfree_ops:
 	kfree(ops);
- out_vn_rele:
-	iput(inode);
- out:
+ out_dput:
+	dput(dentry);
 	return -error;
 }
 
 STATIC int
 xfs_compat_fssetdm_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	void			__user *arg)
 {
 	int			error;
 	struct fsdmidata	fsd;
 	compat_xfs_fsop_setdm_handlereq_t dmhreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 
 	if (!capable(CAP_MKNOD))
 		return -XFS_ERROR(EPERM);
@@ -576,12 +500,11 @@ xfs_compat_fssetdm_by_handle(
 			   sizeof(compat_xfs_fsop_setdm_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
 
-	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &dmhreq.hreq,
-					       &inode);
-	if (error)
-		return -error;
+	dentry = xfs_compat_handlereq_to_dentry(parfilp, &dmhreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) {
+	if (IS_IMMUTABLE(dentry->d_inode) || IS_APPEND(dentry->d_inode)) {
 		error = -XFS_ERROR(EPERM);
 		goto out;
 	}
@@ -591,11 +514,11 @@ xfs_compat_fssetdm_by_handle(
 		goto out;
 	}
 
-	error = -xfs_set_dmattrs(XFS_I(inode), fsd.fsd_dmevmask,
+	error = -xfs_set_dmattrs(XFS_I(dentry->d_inode), fsd.fsd_dmevmask,
 				 fsd.fsd_dmstate);
 
 out:
-	iput(inode);
+	dput(dentry);
 	return error;
 }
 
@@ -724,21 +647,21 @@ xfs_file_compat_ioctl(
 
 		if (xfs_compat_handlereq_copyin(&hreq, arg))
 			return -XFS_ERROR(EFAULT);
-		return xfs_open_by_handle(mp, &hreq, filp, inode);
+		return xfs_open_by_handle(filp, &hreq);
 	}
 	case XFS_IOC_READLINK_BY_HANDLE_32: {
 		struct xfs_fsop_handlereq	hreq;
 
 		if (xfs_compat_handlereq_copyin(&hreq, arg))
 			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_32:
-		return xfs_compat_attrlist_by_handle(mp, arg, inode);
+		return xfs_compat_attrlist_by_handle(filp, arg);
 	case XFS_IOC_ATTRMULTI_BY_HANDLE_32:
-		return xfs_compat_attrmulti_by_handle(mp, arg, inode);
+		return xfs_compat_attrmulti_by_handle(filp, arg);
 	case XFS_IOC_FSSETDM_BY_HANDLE_32:
-		return xfs_compat_fssetdm_by_handle(mp, arg, inode);
+		return xfs_compat_fssetdm_by_handle(filp, arg);
 	default:
 		return -XFS_ERROR(ENOIOCTLCMD);
 	}

-- 

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

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

[Dave Chinner]

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

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

[Christoph Hellwig]

On Mon, Jan 12, 2009 at 10:33:06AM +1100, Dave Chinner wrote:
> That should probably be namespaced correctly because it won't be
> static on debug builds. i.e.  xfs_handle_acceptable()

Ok.

> 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.

Yeah, I first had all this way and then changed it around to match
the non-handle ioctls more closely but forgot this one.

Updated patch below:

---

Subject: xfs: fix dentry aliasing issues in open_by_handle
From: Christoph Hellwig <hch@lst.de>

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>

Index: xfs/fs/xfs/linux-2.6/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2009-01-12 14:37:49.580796051 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_ioctl.c	2009-01-12 14:42:10.723958332 +0100
@@ -50,12 +50,14 @@
 #include "xfs_vnodeops.h"
 #include "xfs_quota.h"
 #include "xfs_inode_item.h"
+#include "xfs_export.h"
 
 #include <linux/capability.h>
 #include <linux/dcache.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
 #include <linux/pagemap.h>
+#include <linux/exportfs.h>
 
 /*
  * xfs_find_handle maps from userspace xfs_fsop_handlereq structure to
@@ -164,97 +166,69 @@ xfs_find_handle(
 	return 0;
 }
 
-
 /*
- * Convert userspace handle data into inode.
- *
- * We use the fact that all the fsop_handlereq ioctl calls have a data
- * structure argument whose first component is always a xfs_fsop_handlereq_t,
- * so we can pass that sub structure into this handy, shared routine.
- *
- * If no error, caller must always iput the returned inode.
+ * No need to do permission checks on the various pathname components
+ * as the handle operations are privileged.
  */
 STATIC int
-xfs_vget_fsop_handlereq(
-	xfs_mount_t		*mp,
-	struct inode		*parinode,	/* parent inode pointer    */
-	xfs_fsop_handlereq_t	*hreq,
-	struct inode		**inode)
-{
-	void			__user *hanp;
-	size_t			hlen;
-	xfs_fid_t		*xfid;
-	xfs_handle_t		*handlep;
+xfs_handle_acceptable(
+	void			*context,
+	struct dentry		*dentry)
+{
+	return 1;
+}
+
+/*
+ * Convert userspace handle data into a dentry.
+ */
+struct dentry *
+xfs_handle_to_dentry(
+	struct file		*parfilp,
+	void __user		*uhandle,
+	u32			hlen)
+{
 	xfs_handle_t		handle;
-	xfs_inode_t		*ip;
-	xfs_ino_t		ino;
-	__u32			igen;
-	int			error;
+	struct xfs_fid64	fid;
 
 	/*
 	 * Only allow handle opens under a directory.
 	 */
-	if (!S_ISDIR(parinode->i_mode))
-		return XFS_ERROR(ENOTDIR);
+	if (!S_ISDIR(parfilp->f_path.dentry->d_inode->i_mode))
+		return ERR_PTR(-ENOTDIR);
 
-	hanp = hreq->ihandle;
-	hlen = hreq->ihandlen;
-	handlep = &handle;
-
-	if (hlen < sizeof(handlep->ha_fsid) || hlen > sizeof(*handlep))
-		return XFS_ERROR(EINVAL);
-	if (copy_from_user(handlep, hanp, hlen))
-		return XFS_ERROR(EFAULT);
-	if (hlen < sizeof(*handlep))
-		memset(((char *)handlep) + hlen, 0, sizeof(*handlep) - hlen);
-	if (hlen > sizeof(handlep->ha_fsid)) {
-		if (handlep->ha_fid.fid_len !=
-		    (hlen - sizeof(handlep->ha_fsid) -
-		            sizeof(handlep->ha_fid.fid_len)) ||
-		    handlep->ha_fid.fid_pad)
-			return XFS_ERROR(EINVAL);
-	}
-
-	/*
-	 * Crack the handle, obtain the inode # & generation #
-	 */
-	xfid = (struct xfs_fid *)&handlep->ha_fid;
-	if (xfid->fid_len == sizeof(*xfid) - sizeof(xfid->fid_len)) {
-		ino  = xfid->fid_ino;
-		igen = xfid->fid_gen;
-	} else {
-		return XFS_ERROR(EINVAL);
-	}
-
-	/*
-	 * Get the XFS inode, building a Linux inode to go with it.
-	 */
-	error = xfs_iget(mp, NULL, ino, 0, XFS_ILOCK_SHARED, &ip, 0);
-	if (error)
-		return error;
-	if (ip == NULL)
-		return XFS_ERROR(EIO);
-	if (ip->i_d.di_gen != igen) {
-		xfs_iput_new(ip, XFS_ILOCK_SHARED);
-		return XFS_ERROR(ENOENT);
-	}
-
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
+	if (hlen != sizeof(xfs_handle_t))
+		return ERR_PTR(-EINVAL);
+	if (copy_from_user(&handle, uhandle, hlen))
+		return ERR_PTR(-EFAULT);
+	if (handle.ha_fid.fid_len !=
+	    sizeof(handle.ha_fid) - sizeof(handle.ha_fid.fid_len))
+		return ERR_PTR(-EINVAL);
+
+	memset(&fid, 0, sizeof(struct fid));
+	fid.ino = handle.ha_fid.fid_ino;
+	fid.gen = handle.ha_fid.fid_gen;
+
+	return exportfs_decode_fh(parfilp->f_path.mnt, (struct fid *)&fid, 3,
+			FILEID_INO32_GEN | XFS_FILEID_TYPE_64FLAG,
+			xfs_handle_acceptable, NULL);
+}
 
-	*inode = VFS_I(ip);
-	return 0;
+STATIC struct dentry *
+xfs_handlereq_to_dentry(
+	struct file		*parfilp,
+	xfs_fsop_handlereq_t	*hreq)
+{
+	return xfs_handle_to_dentry(parfilp, hreq->ihandle, hreq->ihandlen);
 }
 
 int
 xfs_open_by_handle(
-	xfs_mount_t		*mp,
-	xfs_fsop_handlereq_t	*hreq,
 	struct file		*parfilp,
-	struct inode		*parinode)
+	xfs_fsop_handlereq_t	*hreq)
 {
 	const struct cred	*cred = current_cred();
 	int			error;
-	int			new_fd;
+	int			fd;
 	int			permflag;
 	struct file		*filp;
 	struct inode		*inode;
@@ -263,19 +237,21 @@ xfs_open_by_handle(
 	if (!capable(CAP_SYS_ADMIN))
 		return -XFS_ERROR(EPERM);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, hreq, &inode);
-	if (error)
-		return -error;
+	dentry = xfs_handlereq_to_dentry(parfilp, hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
+	inode = dentry->d_inode;
 
 	/* Restrict xfs_open_by_handle to directories & regular files. */
 	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
-		iput(inode);
-		return -XFS_ERROR(EINVAL);
+		error = -XFS_ERROR(EPERM);
+		goto out_dput;
 	}
 
 #if BITS_PER_LONG != 32
 	hreq->oflags |= O_LARGEFILE;
 #endif
+
 	/* Put open permission in namei format. */
 	permflag = hreq->oflags;
 	if ((permflag+1) & O_ACCMODE)
@@ -285,50 +261,45 @@ xfs_open_by_handle(
 
 	if ((!(permflag & O_APPEND) || (permflag & O_TRUNC)) &&
 	    (permflag & FMODE_WRITE) && IS_APPEND(inode)) {
-		iput(inode);
-		return -XFS_ERROR(EPERM);
+		error = -XFS_ERROR(EPERM);
+		goto out_dput;
 	}
 
 	if ((permflag & FMODE_WRITE) && IS_IMMUTABLE(inode)) {
-		iput(inode);
-		return -XFS_ERROR(EACCES);
+		error = -XFS_ERROR(EACCES);
+		goto out_dput;
 	}
 
 	/* Can't write directories. */
-	if ( S_ISDIR(inode->i_mode) && (permflag & FMODE_WRITE)) {
-		iput(inode);
-		return -XFS_ERROR(EISDIR);
-	}
-
-	if ((new_fd = get_unused_fd()) < 0) {
-		iput(inode);
-		return new_fd;
+	if (S_ISDIR(inode->i_mode) && (permflag & FMODE_WRITE)) {
+		error = -XFS_ERROR(EISDIR);
+		goto out_dput;
 	}
 
-	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);
 	}
 
 	if (inode->i_mode & S_IFREG) {
-		/* invisible operation should not change atime */
 		filp->f_flags |= O_NOATIME;
 		filp->f_mode |= FMODE_NOCMTIME;
 	}
 
-	fd_install(new_fd, filp);
-	return new_fd;
+	fd_install(fd, filp);
+	return fd;
+
+ out_dput:
+	dput(dentry);
+	return error;
 }
 
 /*
@@ -359,11 +330,10 @@ do_readlink(
 
 int
 xfs_readlink_by_handle(
-	xfs_mount_t		*mp,
-	xfs_fsop_handlereq_t	*hreq,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	xfs_fsop_handlereq_t	*hreq)
 {
-	struct inode		*inode;
+	struct dentry		*dentry;
 	__u32			olen;
 	void			*link;
 	int			error;
@@ -371,26 +341,28 @@ xfs_readlink_by_handle(
 	if (!capable(CAP_SYS_ADMIN))
 		return -XFS_ERROR(EPERM);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, hreq, &inode);
-	if (error)
-		return -error;
+	dentry = xfs_handlereq_to_dentry(parfilp, hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
 	/* Restrict this handle operation to symlinks only. */
-	if (!S_ISLNK(inode->i_mode)) {
+	if (!S_ISLNK(dentry->d_inode->i_mode)) {
 		error = -XFS_ERROR(EINVAL);
-		goto out_iput;
+		goto out_dput;
 	}
 
 	if (copy_from_user(&olen, hreq->ohandlen, sizeof(__u32))) {
 		error = -XFS_ERROR(EFAULT);
-		goto out_iput;
+		goto out_dput;
 	}
 
 	link = kmalloc(MAXPATHLEN+1, GFP_KERNEL);
-	if (!link)
-		goto out_iput;
+	if (!link) {
+		error = -XFS_ERROR(ENOMEM);
+		goto out_dput;
+	}
 
-	error = -xfs_readlink(XFS_I(inode), link);
+	error = -xfs_readlink(XFS_I(dentry->d_inode), link);
 	if (error)
 		goto out_kfree;
 	error = do_readlink(hreq->ohandle, olen, link);
@@ -399,32 +371,31 @@ xfs_readlink_by_handle(
 
  out_kfree:
 	kfree(link);
- out_iput:
-	iput(inode);
+ out_dput:
+	dput(dentry);
 	return error;
 }
 
 STATIC int
 xfs_fssetdm_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	void			__user *arg)
 {
 	int			error;
 	struct fsdmidata	fsd;
 	xfs_fsop_setdm_handlereq_t dmhreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 
 	if (!capable(CAP_MKNOD))
 		return -XFS_ERROR(EPERM);
 	if (copy_from_user(&dmhreq, arg, sizeof(xfs_fsop_setdm_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, &dmhreq.hreq, &inode);
-	if (error)
-		return -error;
+	dentry = xfs_handlereq_to_dentry(parfilp, &dmhreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) {
+	if (IS_IMMUTABLE(dentry->d_inode) || IS_APPEND(dentry->d_inode)) {
 		error = -XFS_ERROR(EPERM);
 		goto out;
 	}
@@ -434,24 +405,23 @@ xfs_fssetdm_by_handle(
 		goto out;
 	}
 
-	error = -xfs_set_dmattrs(XFS_I(inode), fsd.fsd_dmevmask,
+	error = -xfs_set_dmattrs(XFS_I(dentry->d_inode), fsd.fsd_dmevmask,
 				 fsd.fsd_dmstate);
 
  out:
-	iput(inode);
+	dput(dentry);
 	return error;
 }
 
 STATIC int
 xfs_attrlist_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	void			__user *arg)
 {
-	int			error;
+	int			error = -ENOMEM;
 	attrlist_cursor_kern_t	*cursor;
 	xfs_fsop_attrlist_handlereq_t al_hreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 	char			*kbuf;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -467,16 +437,16 @@ xfs_attrlist_by_handle(
 	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
 		return -XFS_ERROR(EINVAL);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, &al_hreq.hreq, &inode);
-	if (error)
-		goto out;
+	dentry = xfs_handlereq_to_dentry(parfilp, &al_hreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
 	kbuf = kmalloc(al_hreq.buflen, GFP_KERNEL);
 	if (!kbuf)
-		goto out_vn_rele;
+		goto out_dput;
 
 	cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
-	error = xfs_attr_list(XFS_I(inode), kbuf, al_hreq.buflen,
+	error = -xfs_attr_list(XFS_I(dentry->d_inode), kbuf, al_hreq.buflen,
 					al_hreq.flags, cursor);
 	if (error)
 		goto out_kfree;
@@ -486,10 +456,9 @@ xfs_attrlist_by_handle(
 
  out_kfree:
 	kfree(kbuf);
- out_vn_rele:
-	iput(inode);
- out:
-	return -error;
+ out_dput:
+	dput(dentry);
+	return error;
 }
 
 int
@@ -564,15 +533,13 @@ xfs_attrmulti_attr_remove(
 
 STATIC int
 xfs_attrmulti_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
 	struct file		*parfilp,
-	struct inode		*parinode)
+	void			__user *arg)
 {
 	int			error;
 	xfs_attr_multiop_t	*ops;
 	xfs_fsop_attrmulti_handlereq_t am_hreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 	unsigned int		i, size;
 	char			*attr_name;
 
@@ -581,19 +548,19 @@ xfs_attrmulti_by_handle(
 	if (copy_from_user(&am_hreq, arg, sizeof(xfs_fsop_attrmulti_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
 
-	error = xfs_vget_fsop_handlereq(mp, parinode, &am_hreq.hreq, &inode);
-	if (error)
-		goto out;
+	dentry = xfs_handlereq_to_dentry(parfilp, &am_hreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
 	error = E2BIG;
 	size = am_hreq.opcount * sizeof(xfs_attr_multiop_t);
 	if (!size || size > 16 * PAGE_SIZE)
-		goto out_vn_rele;
+		goto out_dput;
 
 	error = ENOMEM;
 	ops = kmalloc(size, GFP_KERNEL);
 	if (!ops)
-		goto out_vn_rele;
+		goto out_dput;
 
 	error = EFAULT;
 	if (copy_from_user(ops, am_hreq.ops, size))
@@ -615,25 +582,28 @@ xfs_attrmulti_by_handle(
 
 		switch (ops[i].am_opcode) {
 		case ATTR_OP_GET:
-			ops[i].am_error = xfs_attrmulti_attr_get(inode,
-					attr_name, ops[i].am_attrvalue,
-					&ops[i].am_length, ops[i].am_flags);
+			ops[i].am_error = xfs_attrmulti_attr_get(
+					dentry->d_inode, attr_name,
+					ops[i].am_attrvalue, &ops[i].am_length,
+					ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
 			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
 			if (ops[i].am_error)
 				break;
-			ops[i].am_error = xfs_attrmulti_attr_set(inode,
-					attr_name, ops[i].am_attrvalue,
-					ops[i].am_length, ops[i].am_flags);
+			ops[i].am_error = xfs_attrmulti_attr_set(
+					dentry->d_inode, attr_name,
+					ops[i].am_attrvalue, ops[i].am_length,
+					ops[i].am_flags);
 			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		case ATTR_OP_REMOVE:
 			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
 			if (ops[i].am_error)
 				break;
-			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
-					attr_name, ops[i].am_flags);
+			ops[i].am_error = xfs_attrmulti_attr_remove(
+					dentry->d_inode, attr_name,
+					ops[i].am_flags);
 			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		default:
@@ -647,9 +617,8 @@ xfs_attrmulti_by_handle(
 	kfree(attr_name);
  out_kfree_ops:
 	kfree(ops);
- out_vn_rele:
-	iput(inode);
- out:
+ out_dput:
+	dput(dentry);
 	return -error;
 }
 
@@ -1440,23 +1409,23 @@ xfs_file_ioctl(
 
 		if (copy_from_user(&hreq, arg, sizeof(xfs_fsop_handlereq_t)))
 			return -XFS_ERROR(EFAULT);
-		return xfs_open_by_handle(mp, &hreq, filp, inode);
+		return xfs_open_by_handle(filp, &hreq);
 	}
 	case XFS_IOC_FSSETDM_BY_HANDLE:
-		return xfs_fssetdm_by_handle(mp, arg, inode);
+		return xfs_fssetdm_by_handle(filp, arg);
 
 	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(filp, arg);
 
 	case XFS_IOC_ATTRMULTI_BY_HANDLE:
-		return xfs_attrmulti_by_handle(mp, arg, filp, inode);
+		return xfs_attrmulti_by_handle(filp, arg);
 
 	case XFS_IOC_SWAPEXT: {
 		struct xfs_swapext	sxp;
Index: xfs/fs/xfs/linux-2.6/xfs_ioctl.h
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.h	2009-01-12 14:37:49.584796561 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_ioctl.h	2009-01-12 14:37:56.056795349 +0100
@@ -34,16 +34,13 @@ xfs_find_handle(
 
 extern int
 xfs_open_by_handle(
-	xfs_mount_t		*mp,
-	xfs_fsop_handlereq_t	*hreq,
 	struct file		*parfilp,
-	struct inode		*parinode);
+	xfs_fsop_handlereq_t	*hreq);
 
 extern int
 xfs_readlink_by_handle(
-	xfs_mount_t		*mp,
-	xfs_fsop_handlereq_t	*hreq,
-	struct inode		*parinode);
+	struct file		*parfilp,
+	xfs_fsop_handlereq_t	*hreq);
 
 extern int
 xfs_attrmulti_attr_get(
@@ -67,6 +64,12 @@ xfs_attrmulti_attr_remove(
 	char			*name,
 	__uint32_t		flags);
 
+extern struct dentry *
+xfs_handle_to_dentry(
+	struct file		*parfilp,
+	void __user		*uhandle,
+	u32			hlen);
+
 extern long
 xfs_file_ioctl(
 	struct file		*filp,
Index: xfs/fs/xfs/linux-2.6/xfs_ioctl32.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl32.c	2009-01-12 14:37:49.590796417 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_ioctl32.c	2009-01-12 14:42:15.829795554 +0100
@@ -340,96 +340,24 @@ xfs_compat_handlereq_copyin(
 	return 0;
 }
 
-/*
- * Convert userspace handle data into inode.
- *
- * We use the fact that all the fsop_handlereq ioctl calls have a data
- * structure argument whose first component is always a xfs_fsop_handlereq_t,
- * so we can pass that sub structure into this handy, shared routine.
- *
- * If no error, caller must always iput the returned inode.
- */
-STATIC int
-xfs_vget_fsop_handlereq_compat(
-	xfs_mount_t		*mp,
-	struct inode		*parinode,	/* parent inode pointer    */
-	compat_xfs_fsop_handlereq_t	*hreq,
-	struct inode		**inode)
+STATIC struct dentry *
+xfs_compat_handlereq_to_dentry(
+	struct file		*parfilp,
+	compat_xfs_fsop_handlereq_t *hreq)
 {
-	void			__user *hanp;
-	size_t			hlen;
-	xfs_fid_t		*xfid;
-	xfs_handle_t		*handlep;
-	xfs_handle_t		handle;
-	xfs_inode_t		*ip;
-	xfs_ino_t		ino;
-	__u32			igen;
-	int			error;
-
-	/*
-	 * Only allow handle opens under a directory.
-	 */
-	if (!S_ISDIR(parinode->i_mode))
-		return XFS_ERROR(ENOTDIR);
-
-	hanp = compat_ptr(hreq->ihandle);
-	hlen = hreq->ihandlen;
-	handlep = &handle;
-
-	if (hlen < sizeof(handlep->ha_fsid) || hlen > sizeof(*handlep))
-		return XFS_ERROR(EINVAL);
-	if (copy_from_user(handlep, hanp, hlen))
-		return XFS_ERROR(EFAULT);
-	if (hlen < sizeof(*handlep))
-		memset(((char *)handlep) + hlen, 0, sizeof(*handlep) - hlen);
-	if (hlen > sizeof(handlep->ha_fsid)) {
-		if (handlep->ha_fid.fid_len !=
-		    (hlen - sizeof(handlep->ha_fsid) -
-			    sizeof(handlep->ha_fid.fid_len)) ||
-		    handlep->ha_fid.fid_pad)
-			return XFS_ERROR(EINVAL);
-	}
-
-	/*
-	 * Crack the handle, obtain the inode # & generation #
-	 */
-	xfid = (struct xfs_fid *)&handlep->ha_fid;
-	if (xfid->fid_len == sizeof(*xfid) - sizeof(xfid->fid_len)) {
-		ino  = xfid->fid_ino;
-		igen = xfid->fid_gen;
-	} else {
-		return XFS_ERROR(EINVAL);
-	}
-
-	/*
-	 * Get the XFS inode, building a Linux inode to go with it.
-	 */
-	error = xfs_iget(mp, NULL, ino, 0, XFS_ILOCK_SHARED, &ip, 0);
-	if (error)
-		return error;
-	if (ip == NULL)
-		return XFS_ERROR(EIO);
-	if (ip->i_d.di_gen != igen) {
-		xfs_iput_new(ip, XFS_ILOCK_SHARED);
-		return XFS_ERROR(ENOENT);
-	}
-
-	xfs_iunlock(ip, XFS_ILOCK_SHARED);
-
-	*inode = VFS_I(ip);
-	return 0;
+	return xfs_handle_to_dentry(parfilp,
+			compat_ptr(hreq->ihandle), hreq->ihandlen);
 }
 
 STATIC int
 xfs_compat_attrlist_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	void			__user *arg)
 {
 	int			error;
 	attrlist_cursor_kern_t	*cursor;
 	compat_xfs_fsop_attrlist_handlereq_t al_hreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 	char			*kbuf;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -446,17 +374,17 @@ xfs_compat_attrlist_by_handle(
 	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
 		return -XFS_ERROR(EINVAL);
 
-	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &al_hreq.hreq,
-					       &inode);
-	if (error)
-		goto out;
+	dentry = xfs_compat_handlereq_to_dentry(parfilp, &al_hreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
+	error = -ENOMEM;
 	kbuf = kmalloc(al_hreq.buflen, GFP_KERNEL);
 	if (!kbuf)
-		goto out_vn_rele;
+		goto out_dput;
 
 	cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
-	error = xfs_attr_list(XFS_I(inode), kbuf, al_hreq.buflen,
+	error = -xfs_attr_list(XFS_I(dentry->d_inode), kbuf, al_hreq.buflen,
 					al_hreq.flags, cursor);
 	if (error)
 		goto out_kfree;
@@ -466,22 +394,20 @@ xfs_compat_attrlist_by_handle(
 
  out_kfree:
 	kfree(kbuf);
- out_vn_rele:
-	iput(inode);
- out:
-	return -error;
+ out_dput:
+	dput(dentry);
+	return error;
 }
 
 STATIC int
 xfs_compat_attrmulti_by_handle(
-	xfs_mount_t				*mp,
-	void					__user *arg,
-	struct inode				*parinode)
+	struct file				*parfilp,
+	void					__user *arg)
 {
 	int					error;
 	compat_xfs_attr_multiop_t		*ops;
 	compat_xfs_fsop_attrmulti_handlereq_t	am_hreq;
-	struct inode				*inode;
+	struct dentry				*dentry;
 	unsigned int				i, size;
 	char					*attr_name;
 
@@ -491,20 +417,19 @@ xfs_compat_attrmulti_by_handle(
 			   sizeof(compat_xfs_fsop_attrmulti_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
 
-	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &am_hreq.hreq,
-					       &inode);
-	if (error)
-		goto out;
+	dentry = xfs_compat_handlereq_to_dentry(parfilp, &am_hreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
 	error = E2BIG;
 	size = am_hreq.opcount * sizeof(compat_xfs_attr_multiop_t);
 	if (!size || size > 16 * PAGE_SIZE)
-		goto out_vn_rele;
+		goto out_dput;
 
 	error = ENOMEM;
 	ops = kmalloc(size, GFP_KERNEL);
 	if (!ops)
-		goto out_vn_rele;
+		goto out_dput;
 
 	error = EFAULT;
 	if (copy_from_user(ops, compat_ptr(am_hreq.ops), size))
@@ -527,20 +452,21 @@ xfs_compat_attrmulti_by_handle(
 
 		switch (ops[i].am_opcode) {
 		case ATTR_OP_GET:
-			ops[i].am_error = xfs_attrmulti_attr_get(inode,
-					attr_name,
+			ops[i].am_error = xfs_attrmulti_attr_get(
+					dentry->d_inode, attr_name,
 					compat_ptr(ops[i].am_attrvalue),
 					&ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
-			ops[i].am_error = xfs_attrmulti_attr_set(inode,
-					attr_name,
+			ops[i].am_error = xfs_attrmulti_attr_set(
+					dentry->d_inode, attr_name,
 					compat_ptr(ops[i].am_attrvalue),
 					ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_REMOVE:
-			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
-					attr_name, ops[i].am_flags);
+			ops[i].am_error = xfs_attrmulti_attr_remove(
+					dentry->d_inode, attr_name,
+					ops[i].am_flags);
 			break;
 		default:
 			ops[i].am_error = EINVAL;
@@ -553,22 +479,20 @@ xfs_compat_attrmulti_by_handle(
 	kfree(attr_name);
  out_kfree_ops:
 	kfree(ops);
- out_vn_rele:
-	iput(inode);
- out:
+ out_dput:
+	dput(dentry);
 	return -error;
 }
 
 STATIC int
 xfs_compat_fssetdm_by_handle(
-	xfs_mount_t		*mp,
-	void			__user *arg,
-	struct inode		*parinode)
+	struct file		*parfilp,
+	void			__user *arg)
 {
 	int			error;
 	struct fsdmidata	fsd;
 	compat_xfs_fsop_setdm_handlereq_t dmhreq;
-	struct inode		*inode;
+	struct dentry		*dentry;
 
 	if (!capable(CAP_MKNOD))
 		return -XFS_ERROR(EPERM);
@@ -576,12 +500,11 @@ xfs_compat_fssetdm_by_handle(
 			   sizeof(compat_xfs_fsop_setdm_handlereq_t)))
 		return -XFS_ERROR(EFAULT);
 
-	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &dmhreq.hreq,
-					       &inode);
-	if (error)
-		return -error;
+	dentry = xfs_compat_handlereq_to_dentry(parfilp, &dmhreq.hreq);
+	if (IS_ERR(dentry))
+		return PTR_ERR(dentry);
 
-	if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) {
+	if (IS_IMMUTABLE(dentry->d_inode) || IS_APPEND(dentry->d_inode)) {
 		error = -XFS_ERROR(EPERM);
 		goto out;
 	}
@@ -591,11 +514,11 @@ xfs_compat_fssetdm_by_handle(
 		goto out;
 	}
 
-	error = -xfs_set_dmattrs(XFS_I(inode), fsd.fsd_dmevmask,
+	error = -xfs_set_dmattrs(XFS_I(dentry->d_inode), fsd.fsd_dmevmask,
 				 fsd.fsd_dmstate);
 
 out:
-	iput(inode);
+	dput(dentry);
 	return error;
 }
 
@@ -724,21 +647,21 @@ xfs_file_compat_ioctl(
 
 		if (xfs_compat_handlereq_copyin(&hreq, arg))
 			return -XFS_ERROR(EFAULT);
-		return xfs_open_by_handle(mp, &hreq, filp, inode);
+		return xfs_open_by_handle(filp, &hreq);
 	}
 	case XFS_IOC_READLINK_BY_HANDLE_32: {
 		struct xfs_fsop_handlereq	hreq;
 
 		if (xfs_compat_handlereq_copyin(&hreq, arg))
 			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_32:
-		return xfs_compat_attrlist_by_handle(mp, arg, inode);
+		return xfs_compat_attrlist_by_handle(filp, arg);
 	case XFS_IOC_ATTRMULTI_BY_HANDLE_32:
-		return xfs_compat_attrmulti_by_handle(mp, arg, inode);
+		return xfs_compat_attrmulti_by_handle(filp, arg);
 	case XFS_IOC_FSSETDM_BY_HANDLE_32:
-		return xfs_compat_fssetdm_by_handle(mp, arg, inode);
+		return xfs_compat_fssetdm_by_handle(filp, arg);
 	default:
 		return -XFS_ERROR(ENOIOCTLCMD);
 	}

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

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

[Dave Chinner]

On Mon, Jan 12, 2009 at 10:39:34AM -0500, Christoph Hellwig wrote:
> On Mon, Jan 12, 2009 at 10:33:06AM +1100, Dave Chinner wrote:
> > That should probably be namespaced correctly because it won't be
> > static on debug builds. i.e.  xfs_handle_acceptable()
> 
> Ok.
> 
> > 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.
> 
> Yeah, I first had all this way and then changed it around to match
> the non-handle ioctls more closely but forgot this one.
> 
> Updated patch below:
> 
> ---
> 
> Subject: xfs: fix dentry aliasing issues in open_by_handle
> From: Christoph Hellwig <hch@lst.de>
> 
> 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>

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com

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

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

[Lachlan McIlroy]

Christoph,

When I build with this patch it fails with:

   CC      init/version.o
   LD      init/built-in.o
   LD      .tmp_vmlinux1
fs/built-in.o: In function `xfs_handle_to_dentry':
/home/lachlan/git/xfs-update/fs/xfs/linux-2.6/xfs_ioctl.c:211: undefined reference to `exportfs_decode_fh'
make: *** [.tmp_vmlinux1] Error 1

Christoph Hellwig wrote:
> On Mon, Jan 12, 2009 at 10:33:06AM +1100, Dave Chinner wrote:
>> That should probably be namespaced correctly because it won't be
>> static on debug builds. i.e.  xfs_handle_acceptable()
> 
> Ok.
> 
>> 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.
> 
> Yeah, I first had all this way and then changed it around to match
> the non-handle ioctls more closely but forgot this one.
> 
> Updated patch below:
> 
> ---
> 
> Subject: xfs: fix dentry aliasing issues in open_by_handle
> From: Christoph Hellwig <hch@lst.de>
> 
> 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>
> 
> Index: xfs/fs/xfs/linux-2.6/xfs_ioctl.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.c	2009-01-12 14:37:49.580796051 +0100
> +++ xfs/fs/xfs/linux-2.6/xfs_ioctl.c	2009-01-12 14:42:10.723958332 +0100
> @@ -50,12 +50,14 @@
>  #include "xfs_vnodeops.h"
>  #include "xfs_quota.h"
>  #include "xfs_inode_item.h"
> +#include "xfs_export.h"
>  
>  #include <linux/capability.h>
>  #include <linux/dcache.h>
>  #include <linux/mount.h>
>  #include <linux/namei.h>
>  #include <linux/pagemap.h>
> +#include <linux/exportfs.h>
>  
>  /*
>   * xfs_find_handle maps from userspace xfs_fsop_handlereq structure to
> @@ -164,97 +166,69 @@ xfs_find_handle(
>  	return 0;
>  }
>  
> -
>  /*
> - * Convert userspace handle data into inode.
> - *
> - * We use the fact that all the fsop_handlereq ioctl calls have a data
> - * structure argument whose first component is always a xfs_fsop_handlereq_t,
> - * so we can pass that sub structure into this handy, shared routine.
> - *
> - * If no error, caller must always iput the returned inode.
> + * No need to do permission checks on the various pathname components
> + * as the handle operations are privileged.
>   */
>  STATIC int
> -xfs_vget_fsop_handlereq(
> -	xfs_mount_t		*mp,
> -	struct inode		*parinode,	/* parent inode pointer    */
> -	xfs_fsop_handlereq_t	*hreq,
> -	struct inode		**inode)
> -{
> -	void			__user *hanp;
> -	size_t			hlen;
> -	xfs_fid_t		*xfid;
> -	xfs_handle_t		*handlep;
> +xfs_handle_acceptable(
> +	void			*context,
> +	struct dentry		*dentry)
> +{
> +	return 1;
> +}
> +
> +/*
> + * Convert userspace handle data into a dentry.
> + */
> +struct dentry *
> +xfs_handle_to_dentry(
> +	struct file		*parfilp,
> +	void __user		*uhandle,
> +	u32			hlen)
> +{
>  	xfs_handle_t		handle;
> -	xfs_inode_t		*ip;
> -	xfs_ino_t		ino;
> -	__u32			igen;
> -	int			error;
> +	struct xfs_fid64	fid;
>  
>  	/*
>  	 * Only allow handle opens under a directory.
>  	 */
> -	if (!S_ISDIR(parinode->i_mode))
> -		return XFS_ERROR(ENOTDIR);
> +	if (!S_ISDIR(parfilp->f_path.dentry->d_inode->i_mode))
> +		return ERR_PTR(-ENOTDIR);
>  
> -	hanp = hreq->ihandle;
> -	hlen = hreq->ihandlen;
> -	handlep = &handle;
> -
> -	if (hlen < sizeof(handlep->ha_fsid) || hlen > sizeof(*handlep))
> -		return XFS_ERROR(EINVAL);
> -	if (copy_from_user(handlep, hanp, hlen))
> -		return XFS_ERROR(EFAULT);
> -	if (hlen < sizeof(*handlep))
> -		memset(((char *)handlep) + hlen, 0, sizeof(*handlep) - hlen);
> -	if (hlen > sizeof(handlep->ha_fsid)) {
> -		if (handlep->ha_fid.fid_len !=
> -		    (hlen - sizeof(handlep->ha_fsid) -
> -		            sizeof(handlep->ha_fid.fid_len)) ||
> -		    handlep->ha_fid.fid_pad)
> -			return XFS_ERROR(EINVAL);
> -	}
> -
> -	/*
> -	 * Crack the handle, obtain the inode # & generation #
> -	 */
> -	xfid = (struct xfs_fid *)&handlep->ha_fid;
> -	if (xfid->fid_len == sizeof(*xfid) - sizeof(xfid->fid_len)) {
> -		ino  = xfid->fid_ino;
> -		igen = xfid->fid_gen;
> -	} else {
> -		return XFS_ERROR(EINVAL);
> -	}
> -
> -	/*
> -	 * Get the XFS inode, building a Linux inode to go with it.
> -	 */
> -	error = xfs_iget(mp, NULL, ino, 0, XFS_ILOCK_SHARED, &ip, 0);
> -	if (error)
> -		return error;
> -	if (ip == NULL)
> -		return XFS_ERROR(EIO);
> -	if (ip->i_d.di_gen != igen) {
> -		xfs_iput_new(ip, XFS_ILOCK_SHARED);
> -		return XFS_ERROR(ENOENT);
> -	}
> -
> -	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> +	if (hlen != sizeof(xfs_handle_t))
> +		return ERR_PTR(-EINVAL);
> +	if (copy_from_user(&handle, uhandle, hlen))
> +		return ERR_PTR(-EFAULT);
> +	if (handle.ha_fid.fid_len !=
> +	    sizeof(handle.ha_fid) - sizeof(handle.ha_fid.fid_len))
> +		return ERR_PTR(-EINVAL);
> +
> +	memset(&fid, 0, sizeof(struct fid));
> +	fid.ino = handle.ha_fid.fid_ino;
> +	fid.gen = handle.ha_fid.fid_gen;
> +
> +	return exportfs_decode_fh(parfilp->f_path.mnt, (struct fid *)&fid, 3,
> +			FILEID_INO32_GEN | XFS_FILEID_TYPE_64FLAG,
> +			xfs_handle_acceptable, NULL);
> +}
>  
> -	*inode = VFS_I(ip);
> -	return 0;
> +STATIC struct dentry *
> +xfs_handlereq_to_dentry(
> +	struct file		*parfilp,
> +	xfs_fsop_handlereq_t	*hreq)
> +{
> +	return xfs_handle_to_dentry(parfilp, hreq->ihandle, hreq->ihandlen);
>  }
>  
>  int
>  xfs_open_by_handle(
> -	xfs_mount_t		*mp,
> -	xfs_fsop_handlereq_t	*hreq,
>  	struct file		*parfilp,
> -	struct inode		*parinode)
> +	xfs_fsop_handlereq_t	*hreq)
>  {
>  	const struct cred	*cred = current_cred();
>  	int			error;
> -	int			new_fd;
> +	int			fd;
>  	int			permflag;
>  	struct file		*filp;
>  	struct inode		*inode;
> @@ -263,19 +237,21 @@ xfs_open_by_handle(
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -XFS_ERROR(EPERM);
>  
> -	error = xfs_vget_fsop_handlereq(mp, parinode, hreq, &inode);
> -	if (error)
> -		return -error;
> +	dentry = xfs_handlereq_to_dentry(parfilp, hreq);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
> +	inode = dentry->d_inode;
>  
>  	/* Restrict xfs_open_by_handle to directories & regular files. */
>  	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode))) {
> -		iput(inode);
> -		return -XFS_ERROR(EINVAL);
> +		error = -XFS_ERROR(EPERM);
> +		goto out_dput;
>  	}
>  
>  #if BITS_PER_LONG != 32
>  	hreq->oflags |= O_LARGEFILE;
>  #endif
> +
>  	/* Put open permission in namei format. */
>  	permflag = hreq->oflags;
>  	if ((permflag+1) & O_ACCMODE)
> @@ -285,50 +261,45 @@ xfs_open_by_handle(
>  
>  	if ((!(permflag & O_APPEND) || (permflag & O_TRUNC)) &&
>  	    (permflag & FMODE_WRITE) && IS_APPEND(inode)) {
> -		iput(inode);
> -		return -XFS_ERROR(EPERM);
> +		error = -XFS_ERROR(EPERM);
> +		goto out_dput;
>  	}
>  
>  	if ((permflag & FMODE_WRITE) && IS_IMMUTABLE(inode)) {
> -		iput(inode);
> -		return -XFS_ERROR(EACCES);
> +		error = -XFS_ERROR(EACCES);
> +		goto out_dput;
>  	}
>  
>  	/* Can't write directories. */
> -	if ( S_ISDIR(inode->i_mode) && (permflag & FMODE_WRITE)) {
> -		iput(inode);
> -		return -XFS_ERROR(EISDIR);
> -	}
> -
> -	if ((new_fd = get_unused_fd()) < 0) {
> -		iput(inode);
> -		return new_fd;
> +	if (S_ISDIR(inode->i_mode) && (permflag & FMODE_WRITE)) {
> +		error = -XFS_ERROR(EISDIR);
> +		goto out_dput;
>  	}
>  
> -	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);
>  	}
>  
>  	if (inode->i_mode & S_IFREG) {
> -		/* invisible operation should not change atime */
>  		filp->f_flags |= O_NOATIME;
>  		filp->f_mode |= FMODE_NOCMTIME;
>  	}
>  
> -	fd_install(new_fd, filp);
> -	return new_fd;
> +	fd_install(fd, filp);
> +	return fd;
> +
> + out_dput:
> +	dput(dentry);
> +	return error;
>  }
>  
>  /*
> @@ -359,11 +330,10 @@ do_readlink(
>  
>  int
>  xfs_readlink_by_handle(
> -	xfs_mount_t		*mp,
> -	xfs_fsop_handlereq_t	*hreq,
> -	struct inode		*parinode)
> +	struct file		*parfilp,
> +	xfs_fsop_handlereq_t	*hreq)
>  {
> -	struct inode		*inode;
> +	struct dentry		*dentry;
>  	__u32			olen;
>  	void			*link;
>  	int			error;
> @@ -371,26 +341,28 @@ xfs_readlink_by_handle(
>  	if (!capable(CAP_SYS_ADMIN))
>  		return -XFS_ERROR(EPERM);
>  
> -	error = xfs_vget_fsop_handlereq(mp, parinode, hreq, &inode);
> -	if (error)
> -		return -error;
> +	dentry = xfs_handlereq_to_dentry(parfilp, hreq);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
>  
>  	/* Restrict this handle operation to symlinks only. */
> -	if (!S_ISLNK(inode->i_mode)) {
> +	if (!S_ISLNK(dentry->d_inode->i_mode)) {
>  		error = -XFS_ERROR(EINVAL);
> -		goto out_iput;
> +		goto out_dput;
>  	}
>  
>  	if (copy_from_user(&olen, hreq->ohandlen, sizeof(__u32))) {
>  		error = -XFS_ERROR(EFAULT);
> -		goto out_iput;
> +		goto out_dput;
>  	}
>  
>  	link = kmalloc(MAXPATHLEN+1, GFP_KERNEL);
> -	if (!link)
> -		goto out_iput;
> +	if (!link) {
> +		error = -XFS_ERROR(ENOMEM);
> +		goto out_dput;
> +	}
>  
> -	error = -xfs_readlink(XFS_I(inode), link);
> +	error = -xfs_readlink(XFS_I(dentry->d_inode), link);
>  	if (error)
>  		goto out_kfree;
>  	error = do_readlink(hreq->ohandle, olen, link);
> @@ -399,32 +371,31 @@ xfs_readlink_by_handle(
>  
>   out_kfree:
>  	kfree(link);
> - out_iput:
> -	iput(inode);
> + out_dput:
> +	dput(dentry);
>  	return error;
>  }
>  
>  STATIC int
>  xfs_fssetdm_by_handle(
> -	xfs_mount_t		*mp,
> -	void			__user *arg,
> -	struct inode		*parinode)
> +	struct file		*parfilp,
> +	void			__user *arg)
>  {
>  	int			error;
>  	struct fsdmidata	fsd;
>  	xfs_fsop_setdm_handlereq_t dmhreq;
> -	struct inode		*inode;
> +	struct dentry		*dentry;
>  
>  	if (!capable(CAP_MKNOD))
>  		return -XFS_ERROR(EPERM);
>  	if (copy_from_user(&dmhreq, arg, sizeof(xfs_fsop_setdm_handlereq_t)))
>  		return -XFS_ERROR(EFAULT);
>  
> -	error = xfs_vget_fsop_handlereq(mp, parinode, &dmhreq.hreq, &inode);
> -	if (error)
> -		return -error;
> +	dentry = xfs_handlereq_to_dentry(parfilp, &dmhreq.hreq);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
>  
> -	if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) {
> +	if (IS_IMMUTABLE(dentry->d_inode) || IS_APPEND(dentry->d_inode)) {
>  		error = -XFS_ERROR(EPERM);
>  		goto out;
>  	}
> @@ -434,24 +405,23 @@ xfs_fssetdm_by_handle(
>  		goto out;
>  	}
>  
> -	error = -xfs_set_dmattrs(XFS_I(inode), fsd.fsd_dmevmask,
> +	error = -xfs_set_dmattrs(XFS_I(dentry->d_inode), fsd.fsd_dmevmask,
>  				 fsd.fsd_dmstate);
>  
>   out:
> -	iput(inode);
> +	dput(dentry);
>  	return error;
>  }
>  
>  STATIC int
>  xfs_attrlist_by_handle(
> -	xfs_mount_t		*mp,
> -	void			__user *arg,
> -	struct inode		*parinode)
> +	struct file		*parfilp,
> +	void			__user *arg)
>  {
> -	int			error;
> +	int			error = -ENOMEM;
>  	attrlist_cursor_kern_t	*cursor;
>  	xfs_fsop_attrlist_handlereq_t al_hreq;
> -	struct inode		*inode;
> +	struct dentry		*dentry;
>  	char			*kbuf;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -467,16 +437,16 @@ xfs_attrlist_by_handle(
>  	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
>  		return -XFS_ERROR(EINVAL);
>  
> -	error = xfs_vget_fsop_handlereq(mp, parinode, &al_hreq.hreq, &inode);
> -	if (error)
> -		goto out;
> +	dentry = xfs_handlereq_to_dentry(parfilp, &al_hreq.hreq);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
>  
>  	kbuf = kmalloc(al_hreq.buflen, GFP_KERNEL);
>  	if (!kbuf)
> -		goto out_vn_rele;
> +		goto out_dput;
>  
>  	cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
> -	error = xfs_attr_list(XFS_I(inode), kbuf, al_hreq.buflen,
> +	error = -xfs_attr_list(XFS_I(dentry->d_inode), kbuf, al_hreq.buflen,
>  					al_hreq.flags, cursor);
>  	if (error)
>  		goto out_kfree;
> @@ -486,10 +456,9 @@ xfs_attrlist_by_handle(
>  
>   out_kfree:
>  	kfree(kbuf);
> - out_vn_rele:
> -	iput(inode);
> - out:
> -	return -error;
> + out_dput:
> +	dput(dentry);
> +	return error;
>  }
>  
>  int
> @@ -564,15 +533,13 @@ xfs_attrmulti_attr_remove(
>  
>  STATIC int
>  xfs_attrmulti_by_handle(
> -	xfs_mount_t		*mp,
> -	void			__user *arg,
>  	struct file		*parfilp,
> -	struct inode		*parinode)
> +	void			__user *arg)
>  {
>  	int			error;
>  	xfs_attr_multiop_t	*ops;
>  	xfs_fsop_attrmulti_handlereq_t am_hreq;
> -	struct inode		*inode;
> +	struct dentry		*dentry;
>  	unsigned int		i, size;
>  	char			*attr_name;
>  
> @@ -581,19 +548,19 @@ xfs_attrmulti_by_handle(
>  	if (copy_from_user(&am_hreq, arg, sizeof(xfs_fsop_attrmulti_handlereq_t)))
>  		return -XFS_ERROR(EFAULT);
>  
> -	error = xfs_vget_fsop_handlereq(mp, parinode, &am_hreq.hreq, &inode);
> -	if (error)
> -		goto out;
> +	dentry = xfs_handlereq_to_dentry(parfilp, &am_hreq.hreq);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
>  
>  	error = E2BIG;
>  	size = am_hreq.opcount * sizeof(xfs_attr_multiop_t);
>  	if (!size || size > 16 * PAGE_SIZE)
> -		goto out_vn_rele;
> +		goto out_dput;
>  
>  	error = ENOMEM;
>  	ops = kmalloc(size, GFP_KERNEL);
>  	if (!ops)
> -		goto out_vn_rele;
> +		goto out_dput;
>  
>  	error = EFAULT;
>  	if (copy_from_user(ops, am_hreq.ops, size))
> @@ -615,25 +582,28 @@ xfs_attrmulti_by_handle(
>  
>  		switch (ops[i].am_opcode) {
>  		case ATTR_OP_GET:
> -			ops[i].am_error = xfs_attrmulti_attr_get(inode,
> -					attr_name, ops[i].am_attrvalue,
> -					&ops[i].am_length, ops[i].am_flags);
> +			ops[i].am_error = xfs_attrmulti_attr_get(
> +					dentry->d_inode, attr_name,
> +					ops[i].am_attrvalue, &ops[i].am_length,
> +					ops[i].am_flags);
>  			break;
>  		case ATTR_OP_SET:
>  			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
>  			if (ops[i].am_error)
>  				break;
> -			ops[i].am_error = xfs_attrmulti_attr_set(inode,
> -					attr_name, ops[i].am_attrvalue,
> -					ops[i].am_length, ops[i].am_flags);
> +			ops[i].am_error = xfs_attrmulti_attr_set(
> +					dentry->d_inode, attr_name,
> +					ops[i].am_attrvalue, ops[i].am_length,
> +					ops[i].am_flags);
>  			mnt_drop_write(parfilp->f_path.mnt);
>  			break;
>  		case ATTR_OP_REMOVE:
>  			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
>  			if (ops[i].am_error)
>  				break;
> -			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
> -					attr_name, ops[i].am_flags);
> +			ops[i].am_error = xfs_attrmulti_attr_remove(
> +					dentry->d_inode, attr_name,
> +					ops[i].am_flags);
>  			mnt_drop_write(parfilp->f_path.mnt);
>  			break;
>  		default:
> @@ -647,9 +617,8 @@ xfs_attrmulti_by_handle(
>  	kfree(attr_name);
>   out_kfree_ops:
>  	kfree(ops);
> - out_vn_rele:
> -	iput(inode);
> - out:
> + out_dput:
> +	dput(dentry);
>  	return -error;
>  }
>  
> @@ -1440,23 +1409,23 @@ xfs_file_ioctl(
>  
>  		if (copy_from_user(&hreq, arg, sizeof(xfs_fsop_handlereq_t)))
>  			return -XFS_ERROR(EFAULT);
> -		return xfs_open_by_handle(mp, &hreq, filp, inode);
> +		return xfs_open_by_handle(filp, &hreq);
>  	}
>  	case XFS_IOC_FSSETDM_BY_HANDLE:
> -		return xfs_fssetdm_by_handle(mp, arg, inode);
> +		return xfs_fssetdm_by_handle(filp, arg);
>  
>  	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(filp, arg);
>  
>  	case XFS_IOC_ATTRMULTI_BY_HANDLE:
> -		return xfs_attrmulti_by_handle(mp, arg, filp, inode);
> +		return xfs_attrmulti_by_handle(filp, arg);
>  
>  	case XFS_IOC_SWAPEXT: {
>  		struct xfs_swapext	sxp;
> Index: xfs/fs/xfs/linux-2.6/xfs_ioctl.h
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.h	2009-01-12 14:37:49.584796561 +0100
> +++ xfs/fs/xfs/linux-2.6/xfs_ioctl.h	2009-01-12 14:37:56.056795349 +0100
> @@ -34,16 +34,13 @@ xfs_find_handle(
>  
>  extern int
>  xfs_open_by_handle(
> -	xfs_mount_t		*mp,
> -	xfs_fsop_handlereq_t	*hreq,
>  	struct file		*parfilp,
> -	struct inode		*parinode);
> +	xfs_fsop_handlereq_t	*hreq);
>  
>  extern int
>  xfs_readlink_by_handle(
> -	xfs_mount_t		*mp,
> -	xfs_fsop_handlereq_t	*hreq,
> -	struct inode		*parinode);
> +	struct file		*parfilp,
> +	xfs_fsop_handlereq_t	*hreq);
>  
>  extern int
>  xfs_attrmulti_attr_get(
> @@ -67,6 +64,12 @@ xfs_attrmulti_attr_remove(
>  	char			*name,
>  	__uint32_t		flags);
>  
> +extern struct dentry *
> +xfs_handle_to_dentry(
> +	struct file		*parfilp,
> +	void __user		*uhandle,
> +	u32			hlen);
> +
>  extern long
>  xfs_file_ioctl(
>  	struct file		*filp,
> Index: xfs/fs/xfs/linux-2.6/xfs_ioctl32.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl32.c	2009-01-12 14:37:49.590796417 +0100
> +++ xfs/fs/xfs/linux-2.6/xfs_ioctl32.c	2009-01-12 14:42:15.829795554 +0100
> @@ -340,96 +340,24 @@ xfs_compat_handlereq_copyin(
>  	return 0;
>  }
>  
> -/*
> - * Convert userspace handle data into inode.
> - *
> - * We use the fact that all the fsop_handlereq ioctl calls have a data
> - * structure argument whose first component is always a xfs_fsop_handlereq_t,
> - * so we can pass that sub structure into this handy, shared routine.
> - *
> - * If no error, caller must always iput the returned inode.
> - */
> -STATIC int
> -xfs_vget_fsop_handlereq_compat(
> -	xfs_mount_t		*mp,
> -	struct inode		*parinode,	/* parent inode pointer    */
> -	compat_xfs_fsop_handlereq_t	*hreq,
> -	struct inode		**inode)
> +STATIC struct dentry *
> +xfs_compat_handlereq_to_dentry(
> +	struct file		*parfilp,
> +	compat_xfs_fsop_handlereq_t *hreq)
>  {
> -	void			__user *hanp;
> -	size_t			hlen;
> -	xfs_fid_t		*xfid;
> -	xfs_handle_t		*handlep;
> -	xfs_handle_t		handle;
> -	xfs_inode_t		*ip;
> -	xfs_ino_t		ino;
> -	__u32			igen;
> -	int			error;
> -
> -	/*
> -	 * Only allow handle opens under a directory.
> -	 */
> -	if (!S_ISDIR(parinode->i_mode))
> -		return XFS_ERROR(ENOTDIR);
> -
> -	hanp = compat_ptr(hreq->ihandle);
> -	hlen = hreq->ihandlen;
> -	handlep = &handle;
> -
> -	if (hlen < sizeof(handlep->ha_fsid) || hlen > sizeof(*handlep))
> -		return XFS_ERROR(EINVAL);
> -	if (copy_from_user(handlep, hanp, hlen))
> -		return XFS_ERROR(EFAULT);
> -	if (hlen < sizeof(*handlep))
> -		memset(((char *)handlep) + hlen, 0, sizeof(*handlep) - hlen);
> -	if (hlen > sizeof(handlep->ha_fsid)) {
> -		if (handlep->ha_fid.fid_len !=
> -		    (hlen - sizeof(handlep->ha_fsid) -
> -			    sizeof(handlep->ha_fid.fid_len)) ||
> -		    handlep->ha_fid.fid_pad)
> -			return XFS_ERROR(EINVAL);
> -	}
> -
> -	/*
> -	 * Crack the handle, obtain the inode # & generation #
> -	 */
> -	xfid = (struct xfs_fid *)&handlep->ha_fid;
> -	if (xfid->fid_len == sizeof(*xfid) - sizeof(xfid->fid_len)) {
> -		ino  = xfid->fid_ino;
> -		igen = xfid->fid_gen;
> -	} else {
> -		return XFS_ERROR(EINVAL);
> -	}
> -
> -	/*
> -	 * Get the XFS inode, building a Linux inode to go with it.
> -	 */
> -	error = xfs_iget(mp, NULL, ino, 0, XFS_ILOCK_SHARED, &ip, 0);
> -	if (error)
> -		return error;
> -	if (ip == NULL)
> -		return XFS_ERROR(EIO);
> -	if (ip->i_d.di_gen != igen) {
> -		xfs_iput_new(ip, XFS_ILOCK_SHARED);
> -		return XFS_ERROR(ENOENT);
> -	}
> -
> -	xfs_iunlock(ip, XFS_ILOCK_SHARED);
> -
> -	*inode = VFS_I(ip);
> -	return 0;
> +	return xfs_handle_to_dentry(parfilp,
> +			compat_ptr(hreq->ihandle), hreq->ihandlen);
>  }
>  
>  STATIC int
>  xfs_compat_attrlist_by_handle(
> -	xfs_mount_t		*mp,
> -	void			__user *arg,
> -	struct inode		*parinode)
> +	struct file		*parfilp,
> +	void			__user *arg)
>  {
>  	int			error;
>  	attrlist_cursor_kern_t	*cursor;
>  	compat_xfs_fsop_attrlist_handlereq_t al_hreq;
> -	struct inode		*inode;
> +	struct dentry		*dentry;
>  	char			*kbuf;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -446,17 +374,17 @@ xfs_compat_attrlist_by_handle(
>  	if (al_hreq.flags & ~(ATTR_ROOT | ATTR_SECURE))
>  		return -XFS_ERROR(EINVAL);
>  
> -	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &al_hreq.hreq,
> -					       &inode);
> -	if (error)
> -		goto out;
> +	dentry = xfs_compat_handlereq_to_dentry(parfilp, &al_hreq.hreq);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
>  
> +	error = -ENOMEM;
>  	kbuf = kmalloc(al_hreq.buflen, GFP_KERNEL);
>  	if (!kbuf)
> -		goto out_vn_rele;
> +		goto out_dput;
>  
>  	cursor = (attrlist_cursor_kern_t *)&al_hreq.pos;
> -	error = xfs_attr_list(XFS_I(inode), kbuf, al_hreq.buflen,
> +	error = -xfs_attr_list(XFS_I(dentry->d_inode), kbuf, al_hreq.buflen,
>  					al_hreq.flags, cursor);
>  	if (error)
>  		goto out_kfree;
> @@ -466,22 +394,20 @@ xfs_compat_attrlist_by_handle(
>  
>   out_kfree:
>  	kfree(kbuf);
> - out_vn_rele:
> -	iput(inode);
> - out:
> -	return -error;
> + out_dput:
> +	dput(dentry);
> +	return error;
>  }
>  
>  STATIC int
>  xfs_compat_attrmulti_by_handle(
> -	xfs_mount_t				*mp,
> -	void					__user *arg,
> -	struct inode				*parinode)
> +	struct file				*parfilp,
> +	void					__user *arg)
>  {
>  	int					error;
>  	compat_xfs_attr_multiop_t		*ops;
>  	compat_xfs_fsop_attrmulti_handlereq_t	am_hreq;
> -	struct inode				*inode;
> +	struct dentry				*dentry;
>  	unsigned int				i, size;
>  	char					*attr_name;
>  
> @@ -491,20 +417,19 @@ xfs_compat_attrmulti_by_handle(
>  			   sizeof(compat_xfs_fsop_attrmulti_handlereq_t)))
>  		return -XFS_ERROR(EFAULT);
>  
> -	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &am_hreq.hreq,
> -					       &inode);
> -	if (error)
> -		goto out;
> +	dentry = xfs_compat_handlereq_to_dentry(parfilp, &am_hreq.hreq);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
>  
>  	error = E2BIG;
>  	size = am_hreq.opcount * sizeof(compat_xfs_attr_multiop_t);
>  	if (!size || size > 16 * PAGE_SIZE)
> -		goto out_vn_rele;
> +		goto out_dput;
>  
>  	error = ENOMEM;
>  	ops = kmalloc(size, GFP_KERNEL);
>  	if (!ops)
> -		goto out_vn_rele;
> +		goto out_dput;
>  
>  	error = EFAULT;
>  	if (copy_from_user(ops, compat_ptr(am_hreq.ops), size))
> @@ -527,20 +452,21 @@ xfs_compat_attrmulti_by_handle(
>  
>  		switch (ops[i].am_opcode) {
>  		case ATTR_OP_GET:
> -			ops[i].am_error = xfs_attrmulti_attr_get(inode,
> -					attr_name,
> +			ops[i].am_error = xfs_attrmulti_attr_get(
> +					dentry->d_inode, attr_name,
>  					compat_ptr(ops[i].am_attrvalue),
>  					&ops[i].am_length, ops[i].am_flags);
>  			break;
>  		case ATTR_OP_SET:
> -			ops[i].am_error = xfs_attrmulti_attr_set(inode,
> -					attr_name,
> +			ops[i].am_error = xfs_attrmulti_attr_set(
> +					dentry->d_inode, attr_name,
>  					compat_ptr(ops[i].am_attrvalue),
>  					ops[i].am_length, ops[i].am_flags);
>  			break;
>  		case ATTR_OP_REMOVE:
> -			ops[i].am_error = xfs_attrmulti_attr_remove(inode,
> -					attr_name, ops[i].am_flags);
> +			ops[i].am_error = xfs_attrmulti_attr_remove(
> +					dentry->d_inode, attr_name,
> +					ops[i].am_flags);
>  			break;
>  		default:
>  			ops[i].am_error = EINVAL;
> @@ -553,22 +479,20 @@ xfs_compat_attrmulti_by_handle(
>  	kfree(attr_name);
>   out_kfree_ops:
>  	kfree(ops);
> - out_vn_rele:
> -	iput(inode);
> - out:
> + out_dput:
> +	dput(dentry);
>  	return -error;
>  }
>  
>  STATIC int
>  xfs_compat_fssetdm_by_handle(
> -	xfs_mount_t		*mp,
> -	void			__user *arg,
> -	struct inode		*parinode)
> +	struct file		*parfilp,
> +	void			__user *arg)
>  {
>  	int			error;
>  	struct fsdmidata	fsd;
>  	compat_xfs_fsop_setdm_handlereq_t dmhreq;
> -	struct inode		*inode;
> +	struct dentry		*dentry;
>  
>  	if (!capable(CAP_MKNOD))
>  		return -XFS_ERROR(EPERM);
> @@ -576,12 +500,11 @@ xfs_compat_fssetdm_by_handle(
>  			   sizeof(compat_xfs_fsop_setdm_handlereq_t)))
>  		return -XFS_ERROR(EFAULT);
>  
> -	error = xfs_vget_fsop_handlereq_compat(mp, parinode, &dmhreq.hreq,
> -					       &inode);
> -	if (error)
> -		return -error;
> +	dentry = xfs_compat_handlereq_to_dentry(parfilp, &dmhreq.hreq);
> +	if (IS_ERR(dentry))
> +		return PTR_ERR(dentry);
>  
> -	if (IS_IMMUTABLE(inode) || IS_APPEND(inode)) {
> +	if (IS_IMMUTABLE(dentry->d_inode) || IS_APPEND(dentry->d_inode)) {
>  		error = -XFS_ERROR(EPERM);
>  		goto out;
>  	}
> @@ -591,11 +514,11 @@ xfs_compat_fssetdm_by_handle(
>  		goto out;
>  	}
>  
> -	error = -xfs_set_dmattrs(XFS_I(inode), fsd.fsd_dmevmask,
> +	error = -xfs_set_dmattrs(XFS_I(dentry->d_inode), fsd.fsd_dmevmask,
>  				 fsd.fsd_dmstate);
>  
>  out:
> -	iput(inode);
> +	dput(dentry);
>  	return error;
>  }
>  
> @@ -724,21 +647,21 @@ xfs_file_compat_ioctl(
>  
>  		if (xfs_compat_handlereq_copyin(&hreq, arg))
>  			return -XFS_ERROR(EFAULT);
> -		return xfs_open_by_handle(mp, &hreq, filp, inode);
> +		return xfs_open_by_handle(filp, &hreq);
>  	}
>  	case XFS_IOC_READLINK_BY_HANDLE_32: {
>  		struct xfs_fsop_handlereq	hreq;
>  
>  		if (xfs_compat_handlereq_copyin(&hreq, arg))
>  			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_32:
> -		return xfs_compat_attrlist_by_handle(mp, arg, inode);
> +		return xfs_compat_attrlist_by_handle(filp, arg);
>  	case XFS_IOC_ATTRMULTI_BY_HANDLE_32:
> -		return xfs_compat_attrmulti_by_handle(mp, arg, inode);
> +		return xfs_compat_attrmulti_by_handle(filp, arg);
>  	case XFS_IOC_FSSETDM_BY_HANDLE_32:
> -		return xfs_compat_fssetdm_by_handle(mp, arg, inode);
> +		return xfs_compat_fssetdm_by_handle(filp, arg);
>  	default:
>  		return -XFS_ERROR(ENOIOCTLCMD);
>  	}
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

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

[Dave Chinner]

On Wed, Jan 14, 2009 at 06:02:40PM +1100, Lachlan McIlroy wrote:
> Christoph,
> 
> When I build with this patch it fails with:
> 
>    CC      init/version.o
>    LD      init/built-in.o
>    LD      .tmp_vmlinux1
> fs/built-in.o: In function `xfs_handle_to_dentry':
> /home/lachlan/git/xfs-update/fs/xfs/linux-2.6/xfs_ioctl.c:211: undefined reference to `exportfs_decode_fh'
> make: *** [.tmp_vmlinux1] Error 1

Given:

EXPORT_SYMBOL_GPL(exportfs_decode_fh);

Did you build with EXPORTFS support in your kernel?

If not, then I think that fs/xfs/Kconfig needs to select EXPORTFS
when XFS is configured in (i.e. it's a new dependency).

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Lachlan McIlroy]

Dave Chinner wrote:
> On Wed, Jan 14, 2009 at 06:02:40PM +1100, Lachlan McIlroy wrote:
>> Christoph,
>>
>> When I build with this patch it fails with:
>>
>>    CC      init/version.o
>>    LD      init/built-in.o
>>    LD      .tmp_vmlinux1
>> fs/built-in.o: In function `xfs_handle_to_dentry':
>> /home/lachlan/git/xfs-update/fs/xfs/linux-2.6/xfs_ioctl.c:211: undefined reference to `exportfs_decode_fh'
>> make: *** [.tmp_vmlinux1] Error 1
> 
> Given:
> 
> EXPORT_SYMBOL_GPL(exportfs_decode_fh);
> 
> Did you build with EXPORTFS support in your kernel?
Not explicitly.  I can't find a reference to it anywhere in my .config.

> 
> If not, then I think that fs/xfs/Kconfig needs to select EXPORTFS
> when XFS is configured in (i.e. it's a new dependency).
> 
> Cheers,
> 
> Dave.

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

[PATCH 2/7] xfs: use mnt_want_write in compat_attrmulti ioctl

[Christoph Hellwig]

[-- Attachment #1: xfs-compat-attrmulti-writecount --]
[-- Type: text/plain, Size: 1531 bytes --]

The compat version of the attrmulti ioctl needs to ask for and then
later release write access to the mount just like the native version,
otherwise we could potentially write to read-only mounts.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/linux-2.6/xfs_ioctl32.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_ioctl32.c	2008-12-29 01:25:19.000000000 +0000
+++ xfs/fs/xfs/linux-2.6/xfs_ioctl32.c	2008-12-29 01:26:24.000000000 +0000
@@ -17,6 +17,7 @@
  */
 #include <linux/compat.h>
 #include <linux/ioctl.h>
+#include <linux/mount.h>
 #include <asm/uaccess.h>
 #include "xfs.h"
 #include "xfs_fs.h"
@@ -458,15 +459,23 @@
 					&ops[i].am_length, ops[i].am_flags);
 			break;
 		case ATTR_OP_SET:
+			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_set(
 					dentry->d_inode, attr_name,
 					compat_ptr(ops[i].am_attrvalue),
 					ops[i].am_length, ops[i].am_flags);
+			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		case ATTR_OP_REMOVE:
+			ops[i].am_error = mnt_want_write(parfilp->f_path.mnt);
+			if (ops[i].am_error)
+				break;
 			ops[i].am_error = xfs_attrmulti_attr_remove(
 					dentry->d_inode, attr_name,
 					ops[i].am_flags);
+			mnt_drop_write(parfilp->f_path.mnt);
 			break;
 		default:
 			ops[i].am_error = EINVAL;

-- 

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

Re: [PATCH 2/7] xfs: use mnt_want_write in compat_attrmulti ioctl

[Dave Chinner]

On Fri, Jan 09, 2009 at 05:11:06PM -0500, Christoph Hellwig wrote:
> The compat version of the attrmulti ioctl needs to ask for and then
> later release write access to the mount just like the native version,
> otherwise we could potentially write to read-only mounts.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 3/7] xfs: add a separate lock class for the per-mount list of dquots

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-xfs_qm_dqusage_adjust-lockdep-warning --]
[-- Type: text/plain, Size: 1250 bytes --]

We can have both a a quota hash chain and the per-mount list locked at
the same time.  But given that both use the same struct dqhash as list
head we have to tell lockdep that they are different lock classes.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/quota/xfs_qm.c
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_qm.c	2008-12-29 01:27:21.000000000 +0000
+++ xfs/fs/xfs/quota/xfs_qm.c	2008-12-29 01:34:48.000000000 +0000
@@ -1068,6 +1068,13 @@
 	return 0;
 }
 
+/*
+ * The hash chains and the mplist use the same xfs_dqhash structure as
+ * their list head, but we can take the mplist qh_lock and one of the
+ * hash qh_locks at the same time without any problem as they aren't
+ * related.
+ */
+static struct lock_class_key xfs_quota_mplist_class;
 
 /*
  * This initializes all the quota information that's kept in the
@@ -1103,6 +1110,8 @@
 	}
 
 	xfs_qm_list_init(&qinf->qi_dqlist, "mpdqlist", 0);
+	lockdep_set_class(&qinf->qi_dqlist.qh_lock, &xfs_quota_mplist_class);
+
 	qinf->qi_dqreclaims = 0;
 
 	/* mutex used to serialize quotaoffs */

-- 

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

Re: [PATCH 3/7] xfs: add a separate lock class for the per-mount list of dquots

[Dave Chinner]

On Fri, Jan 09, 2009 at 05:11:07PM -0500, Christoph Hellwig wrote:
> We can have both a a quota hash chain and the per-mount list locked at
> the same time.  But given that both use the same struct dqhash as list
> head we have to tell lockdep that they are different lock classes.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 4/7] xfs: lockdep annotations for xfs_dqlock2

[Christoph Hellwig]

[-- Attachment #1: xfs-fix-xfs_dqlock2-lockdep-warning --]
[-- Type: text/plain, Size: 1824 bytes --]

xfs_dqlock2 locks two xfs_dquots, which is fine as it always locks the
dquot with the lower id first.  Use mutex_lock_nested to tell lockdep 
about this fact.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/quota/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_dquot.c	2008-12-29 11:28:54.459613573 +0100
+++ xfs/fs/xfs/quota/xfs_dquot.c	2008-12-29 11:29:31.037738842 +0100
@@ -1383,6 +1383,12 @@ xfs_dqunlock_nonotify(
 	mutex_unlock(&(dqp->q_qlock));
 }
 
+/*
+ * Lock two xfs_dquot structures.
+ *
+ * To avoid deadlocks we always lock the quota structure with
+ * the lowerd id first.
+ */
 void
 xfs_dqlock2(
 	xfs_dquot_t	*d1,
@@ -1393,10 +1399,10 @@ xfs_dqlock2(
 		if (be32_to_cpu(d1->q_core.d_id) >
 		    be32_to_cpu(d2->q_core.d_id)) {
 			xfs_dqlock(d2);
-			xfs_dqlock(d1);
+			mutex_lock_nested(&d1->q_qlock, XFS_QLOCK_2ND);
 		} else {
 			xfs_dqlock(d1);
-			xfs_dqlock(d2);
+			mutex_lock_nested(&d2->q_qlock, XFS_QLOCK_2ND);
 		}
 	} else {
 		if (d1) {
Index: xfs/fs/xfs/quota/xfs_dquot.h
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_dquot.h	2008-12-27 17:55:09.920686659 +0100
+++ xfs/fs/xfs/quota/xfs_dquot.h	2008-12-29 11:29:31.043768451 +0100
@@ -97,6 +97,16 @@ typedef struct xfs_dquot {
 #define dq_hashlist	q_lists.dqm_hashlist
 #define dq_flags	q_lists.dqm_flags
 
+/*
+ * Lock hierachy for q_qlock:
+ *	XFS_QLOCK_NORMAL is the implicit default,
+ * 	XFS_QLOCK_DOUBLE is the dquot with the higher id in xfs_dqlock2
+ */
+enum {
+	XFS_QLOCK_NORMAL = 0,
+	XFS_QLOCK_2ND,
+};
+
 #define XFS_DQHOLD(dqp)		((dqp)->q_nrefs++)
 
 #ifdef DEBUG

-- 

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

Re: [PATCH 4/7] xfs: lockdep annotations for xfs_dqlock2

[Dave Chinner]

On Fri, Jan 09, 2009 at 05:11:08PM -0500, Christoph Hellwig wrote:
> xfs_dqlock2 locks two xfs_dquots, which is fine as it always locks the
> dquot with the lower id first.  Use mutex_lock_nested to tell lockdep 
> about this fact.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfs/fs/xfs/quota/xfs_dquot.c
> ===================================================================
> --- xfs.orig/fs/xfs/quota/xfs_dquot.c	2008-12-29 11:28:54.459613573 +0100
> +++ xfs/fs/xfs/quota/xfs_dquot.c	2008-12-29 11:29:31.037738842 +0100
> @@ -1383,6 +1383,12 @@ xfs_dqunlock_nonotify(
>  	mutex_unlock(&(dqp->q_qlock));
>  }
>  
> +/*
> + * Lock two xfs_dquot structures.
> + *
> + * To avoid deadlocks we always lock the quota structure with
> + * the lowerd id first.
> + */
>  void
>  xfs_dqlock2(
>  	xfs_dquot_t	*d1,
> @@ -1393,10 +1399,10 @@ xfs_dqlock2(
>  		if (be32_to_cpu(d1->q_core.d_id) >
>  		    be32_to_cpu(d2->q_core.d_id)) {
>  			xfs_dqlock(d2);
> -			xfs_dqlock(d1);
> +			mutex_lock_nested(&d1->q_qlock, XFS_QLOCK_2ND);
>  		} else {
>  			xfs_dqlock(d1);
> -			xfs_dqlock(d2);
> +			mutex_lock_nested(&d2->q_qlock, XFS_QLOCK_2ND);

This looks a bit wierd.

Yes, xfs_dqlock() is just a wrapper around mutex_lock, but we should
be consistent here. Can you add a xfs_dqlock_nested() wrapper to do
this?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH 4/7] xfs: lockdep annotations for xfs_dqlock2

[Christoph Hellwig]

On Mon, Jan 12, 2009 at 10:06:37AM +1100, Dave Chinner wrote:
> This looks a bit wierd.
> 
> Yes, xfs_dqlock() is just a wrapper around mutex_lock, but we should
> be consistent here. Can you add a xfs_dqlock_nested() wrapper to do
> this?

I don't think we should add more of the silly wrappers.  What about
the version below that always uses plain mutex_lock* in xfs_dqlock2?

---

Subject: xfs: lockdep annotations for xfs_dqlock2
From: Christoph Hellwig <hch@lst.de>

xfs_dqlock2 locks two xfs_dquots, which is fine as it always locks the
dquot with the lower id first.  Use mutex_lock_nested to tell lockdep 
about this fact.  Also clean up xfs_dqlock2 a bit by rationalizing
the conditionals and always using the mutex_lock family of functions
directly.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/quota/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_dquot.c	2009-01-12 14:37:49.445796033 +0100
+++ xfs/fs/xfs/quota/xfs_dquot.c	2009-01-12 14:46:59.913795998 +0100
@@ -1383,6 +1383,12 @@ xfs_dqunlock_nonotify(
 	mutex_unlock(&(dqp->q_qlock));
 }
 
+/*
+ * Lock two xfs_dquot structures.
+ *
+ * To avoid deadlocks we always lock the quota structure with
+ * the lowerd id first.
+ */
 void
 xfs_dqlock2(
 	xfs_dquot_t	*d1,
@@ -1392,18 +1398,16 @@ xfs_dqlock2(
 		ASSERT(d1 != d2);
 		if (be32_to_cpu(d1->q_core.d_id) >
 		    be32_to_cpu(d2->q_core.d_id)) {
-			xfs_dqlock(d2);
-			xfs_dqlock(d1);
+			mutex_lock(&d2->q_qlock);
+			mutex_lock_nested(&d1->q_qlock, XFS_QLOCK_NESTED);
 		} else {
-			xfs_dqlock(d1);
-			xfs_dqlock(d2);
-		}
-	} else {
-		if (d1) {
-			xfs_dqlock(d1);
-		} else if (d2) {
-			xfs_dqlock(d2);
+			mutex_lock(&d1->q_qlock);
+			mutex_lock_nested(&d2->q_qlock, XFS_QLOCK_NESTED);
 		}
+	} else if (d1) {
+		mutex_lock(&d1->q_qlock);
+	} else if (d2) {
+		mutex_lock(&d2->q_qlock);
 	}
 }
 
Index: xfs/fs/xfs/quota/xfs_dquot.h
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_dquot.h	2009-01-12 14:37:49.459795861 +0100
+++ xfs/fs/xfs/quota/xfs_dquot.h	2009-01-12 14:45:30.902817471 +0100
@@ -97,6 +97,16 @@ typedef struct xfs_dquot {
 #define dq_hashlist	q_lists.dqm_hashlist
 #define dq_flags	q_lists.dqm_flags
 
+/*
+ * Lock hierachy for q_qlock:
+ *	XFS_QLOCK_NORMAL is the implicit default,
+ * 	XFS_QLOCK_NESTED is the dquot with the higher id in xfs_dqlock2
+ */
+enum {
+	XFS_QLOCK_NORMAL = 0,
+	XFS_QLOCK_NESTED,
+};
+
 #define XFS_DQHOLD(dqp)		((dqp)->q_nrefs++)
 
 #ifdef DEBUG

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

Re: [PATCH 4/7] xfs: lockdep annotations for xfs_dqlock2

[Dave Chinner]

On Mon, Jan 12, 2009 at 10:15:44AM -0500, Christoph Hellwig wrote:
> On Mon, Jan 12, 2009 at 10:06:37AM +1100, Dave Chinner wrote:
> > This looks a bit wierd.
> > 
> > Yes, xfs_dqlock() is just a wrapper around mutex_lock, but we should
> > be consistent here. Can you add a xfs_dqlock_nested() wrapper to do
> > this?
> 
> I don't think we should add more of the silly wrappers.  What about
> the version below that always uses plain mutex_lock* in xfs_dqlock2?

Fair enough.

> ---
> 
> Subject: xfs: lockdep annotations for xfs_dqlock2
> From: Christoph Hellwig <hch@lst.de>
> 
> xfs_dqlock2 locks two xfs_dquots, which is fine as it always locks the
> dquot with the lower id first.  Use mutex_lock_nested to tell lockdep 
> about this fact.  Also clean up xfs_dqlock2 a bit by rationalizing
> the conditionals and always using the mutex_lock family of functions
> directly.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 5/7] xfs: add a lock class for group/project dquots

[Christoph Hellwig]

[-- Attachment #1: xfs-add-quota-classes --]
[-- Type: text/plain, Size: 1372 bytes --]

We can have both a user and a group/project dquot locked at the same time,
as long as the user dquot is locked first.  Tell lockdep about that fact
by making the group/project dquots a different lock class.


Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: xfs/fs/xfs/quota/xfs_dquot.c
===================================================================
--- xfs.orig/fs/xfs/quota/xfs_dquot.c	2008-12-29 12:24:33.018737674 +0100
+++ xfs/fs/xfs/quota/xfs_dquot.c	2008-12-29 12:29:35.132612895 +0100
@@ -73,6 +73,8 @@ int xfs_dqreq_num;
 int xfs_dqerror_mod = 33;
 #endif
 
+static struct lock_class_key xfs_dquot_other_class;
+
 /*
  * Allocate and initialize a dquot. We don't always allocate fresh memory;
  * we try to reclaim a free dquot if the number of incore dquots are above
@@ -139,7 +141,15 @@ xfs_qm_dqinit(
 		 ASSERT(dqp->q_trace);
 		 xfs_dqtrace_entry(dqp, "DQRECLAIMED_INIT");
 #endif
-	 }
+	}
+
+	/*
+	 * In either case we need to make sure group quotas have a different
+	 * lock class than user quotas, to make sure lockdep knows we can
+	 * locks of one of each at the same time.
+	 */
+	if (!(type & XFS_DQ_USER))
+		lockdep_set_class(&dqp->q_qlock, &xfs_dquot_other_class);
 
 	/*
 	 * log item gets initialized later

-- 

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

Re: [PATCH 5/7] xfs: add a lock class for group/project dquots

[Dave Chinner]

On Fri, Jan 09, 2009 at 05:11:09PM -0500, Christoph Hellwig wrote:
> We can have both a user and a group/project dquot locked at the same time,
> as long as the user dquot is locked first.  Tell lockdep about that fact
> by making the group/project dquots a different lock class.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Dave Chinner <david@fromorbit.com>

-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 6/7] xfs: fix bad_features2 fixups for the root filesystem

[Christoph Hellwig]

[-- Attachment #1: xfs-update-flags-on-remount-rw --]
[-- Type: text/plain, Size: 6005 bytes --]

Currently the bad_features2 fixup and the alignment updates in the superblock
are skipped if we mount a filesystem read-only.  But for the root filesystem
the typical case is to mount read-only first and only later remount writeable
so we'll never perform this update at all.  It's not a big problem but means
the logs of people needing the fixup get spammed at every boot because they
never happen on disk.

Reported-by: Arkadiusz Miskiewicz <arekm@maven.pl>
Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: xfs/fs/xfs/xfs_mount.c
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.c	2009-01-02 18:39:11.579671462 +0100
+++ xfs/fs/xfs/xfs_mount.c	2009-01-03 21:42:36.741548942 +0100
@@ -45,7 +45,6 @@
 #include "xfs_fsops.h"
 #include "xfs_utils.h"
 
-STATIC int	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
 STATIC int	xfs_uuid_mount(xfs_mount_t *);
 STATIC void	xfs_unmountfs_wait(xfs_mount_t *);
 
@@ -682,7 +681,7 @@ xfs_initialize_perag_data(xfs_mount_t *m
  * Update alignment values based on mount options and sb values
  */
 STATIC int
-xfs_update_alignment(xfs_mount_t *mp, __uint64_t *update_flags)
+xfs_update_alignment(xfs_mount_t *mp)
 {
 	xfs_sb_t	*sbp = &(mp->m_sb);
 
@@ -736,11 +735,11 @@ xfs_update_alignment(xfs_mount_t *mp, __
 		if (xfs_sb_version_hasdalign(sbp)) {
 			if (sbp->sb_unit != mp->m_dalign) {
 				sbp->sb_unit = mp->m_dalign;
-				*update_flags |= XFS_SB_UNIT;
+				mp->m_update_flags |= XFS_SB_UNIT;
 			}
 			if (sbp->sb_width != mp->m_swidth) {
 				sbp->sb_width = mp->m_swidth;
-				*update_flags |= XFS_SB_WIDTH;
+				mp->m_update_flags |= XFS_SB_WIDTH;
 			}
 		}
 	} else if ((mp->m_flags & XFS_MOUNT_NOALIGN) != XFS_MOUNT_NOALIGN &&
@@ -905,7 +904,6 @@ xfs_mountfs(
 	xfs_sb_t	*sbp = &(mp->m_sb);
 	xfs_inode_t	*rip;
 	__uint64_t	resblks;
-	__int64_t	update_flags = 0LL;
 	uint		quotamount, quotaflags;
 	int		uuid_mounted = 0;
 	int		error = 0;
@@ -933,7 +931,7 @@ xfs_mountfs(
 			"XFS: correcting sb_features alignment problem");
 		sbp->sb_features2 |= sbp->sb_bad_features2;
 		sbp->sb_bad_features2 = sbp->sb_features2;
-		update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
+		mp->m_update_flags |= XFS_SB_FEATURES2 | XFS_SB_BAD_FEATURES2;
 
 		/*
 		 * Re-check for ATTR2 in case it was found in bad_features2
@@ -947,11 +945,11 @@ xfs_mountfs(
 	if (xfs_sb_version_hasattr2(&mp->m_sb) &&
 	   (mp->m_flags & XFS_MOUNT_NOATTR2)) {
 		xfs_sb_version_removeattr2(&mp->m_sb);
-		update_flags |= XFS_SB_FEATURES2;
+		mp->m_update_flags |= XFS_SB_FEATURES2;
 
 		/* update sb_versionnum for the clearing of the morebits */
 		if (!sbp->sb_features2)
-			update_flags |= XFS_SB_VERSIONNUM;
+			mp->m_update_flags |= XFS_SB_VERSIONNUM;
 	}
 
 	/*
@@ -960,7 +958,7 @@ xfs_mountfs(
 	 * allocator alignment is within an ag, therefore ag has
 	 * to be aligned at stripe boundary.
 	 */
-	error = xfs_update_alignment(mp, &update_flags);
+	error = xfs_update_alignment(mp);
 	if (error)
 		goto error1;
 
@@ -1137,10 +1135,12 @@ xfs_mountfs(
 	}
 
 	/*
-	 * If fs is not mounted readonly, then update the superblock changes.
+	 * If this is a read-only mount defer the superblock updates until
+	 * the next remount into writeable mode.  Otherwise we would never
+	 * perform the update e.g. for the root filesystem.
 	 */
-	if (update_flags && !(mp->m_flags & XFS_MOUNT_RDONLY)) {
-		error = xfs_mount_log_sb(mp, update_flags);
+	if (mp->m_update_flags && !(mp->m_flags & XFS_MOUNT_RDONLY)) {
+		error = xfs_mount_log_sb(mp, mp->m_update_flags);
 		if (error) {
 			cmn_err(CE_WARN, "XFS: failed to write sb changes");
 			goto error4;
@@ -1820,7 +1820,7 @@ xfs_uuid_mount(
  * be altered by the mount options, as well as any potential sb_features2
  * fixup. Only the first superblock is updated.
  */
-STATIC int
+int
 xfs_mount_log_sb(
 	xfs_mount_t	*mp,
 	__int64_t	fields)
Index: xfs/fs/xfs/xfs_mount.h
===================================================================
--- xfs.orig/fs/xfs/xfs_mount.h	2009-01-01 11:53:17.541576193 +0100
+++ xfs/fs/xfs/xfs_mount.h	2009-01-03 21:38:22.555577508 +0100
@@ -327,6 +327,8 @@ typedef struct xfs_mount {
 	spinlock_t		m_sync_lock;	/* work item list lock */
 	int			m_sync_seq;	/* sync thread generation no. */
 	wait_queue_head_t	m_wait_single_sync_task;
+	__int64_t		m_update_flags;	/* sb flags we need to update
+						   on the next remount,rw */
 } xfs_mount_t;
 
 /*
@@ -514,6 +516,7 @@ extern int	xfs_mod_incore_sb_unlocked(xf
 			int64_t, int);
 extern int	xfs_mod_incore_sb_batch(xfs_mount_t *, xfs_mod_sb_t *,
 			uint, int);
+extern int	xfs_mount_log_sb(xfs_mount_t *, __int64_t);
 extern struct xfs_buf *xfs_getsb(xfs_mount_t *, int);
 extern int	xfs_readsb(xfs_mount_t *, int);
 extern void	xfs_freesb(xfs_mount_t *);
Index: xfs/fs/xfs/linux-2.6/xfs_super.c
===================================================================
--- xfs.orig/fs/xfs/linux-2.6/xfs_super.c	2009-01-01 11:53:17.294547000 +0100
+++ xfs/fs/xfs/linux-2.6/xfs_super.c	2009-01-03 21:47:58.367578173 +0100
@@ -1197,6 +1197,7 @@ xfs_fs_remount(
 	struct xfs_mount	*mp = XFS_M(sb);
 	substring_t		args[MAX_OPT_ARGS];
 	char			*p;
+	int			error;
 
 	while ((p = strsep(&options, ",")) != NULL) {
 		int token;
@@ -1247,11 +1248,25 @@ xfs_fs_remount(
 		}
 	}
 
-	/* rw/ro -> rw */
+	/* ro -> rw */
 	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(*flags & MS_RDONLY)) {
 		mp->m_flags &= ~XFS_MOUNT_RDONLY;
 		if (mp->m_flags & XFS_MOUNT_BARRIER)
 			xfs_mountfs_check_barriers(mp);
+
+		/*
+		 * If this is the first remount to writeable state we
+		 * might have some superblock changes to update.
+		 */
+		if (mp->m_update_flags) {
+			error = xfs_mount_log_sb(mp, mp->m_update_flags);
+			if (error) {
+				cmn_err(CE_WARN,
+					"XFS: failed to write sb changes");
+				return error;
+			}
+			mp->m_update_flags = 0;
+		}
 	}
 
 	/* rw -> ro */

-- 

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

Re: [PATCH 6/7] xfs: fix bad_features2 fixups for the root filesystem

[Dave Chinner]

On Fri, Jan 09, 2009 at 05:11:10PM -0500, Christoph Hellwig wrote:
> Currently the bad_features2 fixup and the alignment updates in the superblock
> are skipped if we mount a filesystem read-only.  But for the root filesystem
> the typical case is to mount read-only first and only later remount writeable
> so we'll never perform this update at all.  It's not a big problem but means
> the logs of people needing the fixup get spammed at every boot because they
> never happen on disk.
> 
> Reported-by: Arkadiusz Miskiewicz <arekm@maven.pl>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks sane.

Reviewed-by: Dave Chinner <david@fromorbit.com>


-- 
Dave Chinner
david@fromorbit.com

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

[PATCH 7/7] xfs: sanity check attr fork size

[Christoph Hellwig]

[-- Attachment #1: xfs-attr-sanity-check --]
[-- Type: text/plain, Size: 1612 bytes --]

Recently we have quite a few kerneloops reports about dereferencing a NULL
if_data in the attribute fork.  From looking over the code this can only
happen if we pass a 0 size argument to xfs_iformat_local.  This implies some
sort of corruption and in fact the only mailinglist report about this from
earlier this year was after a powerfail presumably on a system with write
cache and without barriers.

Add a quick sanity check for the attr fork size in xfs_iformat to catch
these early and without an oops.


Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Dave Chinner <david@fromorbit.com>

Index: linux-2.6/fs/xfs/xfs_inode.c
===================================================================
--- linux-2.6.orig/fs/xfs/xfs_inode.c	2009-01-04 17:35:11.955671640 +0100
+++ linux-2.6/fs/xfs/xfs_inode.c	2009-01-04 17:38:04.643673527 +0100
@@ -424,6 +424,19 @@ xfs_iformat(
 	case XFS_DINODE_FMT_LOCAL:
 		atp = (xfs_attr_shortform_t *)XFS_DFORK_APTR(dip);
 		size = be16_to_cpu(atp->hdr.totsize);
+
+		if (unlikely(size < sizeof(struct xfs_attr_sf_hdr))) {
+			xfs_fs_repair_cmn_err(CE_WARN, ip->i_mount,
+				"corrupt inode %Lu "
+				"(bad attr fork size %Ld).",
+				(unsigned long long) ip->i_ino,
+				(long long) size);
+			XFS_CORRUPTION_ERROR("xfs_iformat(8)",
+					     XFS_ERRLEVEL_LOW,
+					     ip->i_mount, dip);
+			return XFS_ERROR(EFSCORRUPTED);
+		}
+
 		error = xfs_iformat_local(ip, dip, XFS_ATTR_FORK, size);
 		break;
 	case XFS_DINODE_FMT_EXTENTS:

-- 

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

Re: [PATCH 0/7] Remaining 2.6.29 queue

[Christoph Hellwig]

On Fri, Jan 09, 2009 at 05:11:04PM -0500, Christoph Hellwig wrote:
> These are my current outstanding patches for 2.6.29, consisting of
> small enough fixes and lockdep annotations, and an early warning
> for a attr corruption frequently seen on kerneloops.org.

Can we please get these pushed out to Linus?  Thanks.

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

Re: [PATCH 0/7] Remaining 2.6.29 queue

[Felix Blyakher]

Hi Christoph,

On Jan 18, 2009, at 11:02 AM, Christoph Hellwig wrote:

> On Fri, Jan 09, 2009 at 05:11:04PM -0500, Christoph Hellwig wrote:
>> These are my current outstanding patches for 2.6.29, consisting of
>> small enough fixes and lockdep annotations, and an early warning
>> for a attr corruption frequently seen on kerneloops.org.
>
> Can we please get these pushed out to Linus?  Thanks.

IIRC, Lachlan had problem applying your patches.
I'll check it myself and let you know where we're
here.

Felix


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

Re: [PATCH 0/7] Remaining 2.6.29 queue

[Lachlan McIlroy]

Felix Blyakher wrote:
> Hi Christoph,
> 
> On Jan 18, 2009, at 11:02 AM, Christoph Hellwig wrote:
> 
>> On Fri, Jan 09, 2009 at 05:11:04PM -0500, Christoph Hellwig wrote:
>>> These are my current outstanding patches for 2.6.29, consisting of
>>> small enough fixes and lockdep annotations, and an early warning
>>> for a attr corruption frequently seen on kerneloops.org.
>> Can we please get these pushed out to Linus?  Thanks.
> 
> IIRC, Lachlan had problem applying your patches.
> I'll check it myself and let you know where we're
> here.

They applied okay but the first patch caused this build failure.

    CC      init/version.o
    LD      init/built-in.o
    LD      .tmp_vmlinux1
fs/built-in.o: In function `xfs_handle_to_dentry':
/home/lachlan/git/xfs-update/fs/xfs/linux-2.6/xfs_ioctl.c:211: undefined reference to `exportfs_decode_fh'
make: *** [.tmp_vmlinux1] Error 1

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

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

Re: [PATCH 0/7] Remaining 2.6.29 queue

[Christoph Hellwig]

On Mon, Jan 19, 2009 at 12:52:24PM +1100, Lachlan McIlroy wrote:
> They applied okay but the first patch caused this build failure.
>
>    CC      init/version.o
>    LD      init/built-in.o
>    LD      .tmp_vmlinux1
> fs/built-in.o: In function `xfs_handle_to_dentry':
> /home/lachlan/git/xfs-update/fs/xfs/linux-2.6/xfs_ioctl.c:211: undefined reference to `exportfs_decode_fh'
> make: *** [.tmp_vmlinux1] Error 1

That was, as noted in the thread a missing select EXPORTFS in the
Kconfig.  It's fixed in the kernel.org git tree.

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