svn commit: r229828 - in head/sys: kern ufs/ufs

[Konstantin Belousov]

Author: kib
Date: Sun Jan  8 23:06:53 2012
New Revision: 229828
URL: http://svn.freebsd.org/changeset/base/229828

Log:
  Avoid LOR between vfs_busy() lock and covered vnode lock on quotaon().
  The vfs_busy() is after covered vnode lock in the global lock order, but
  since quotaon() does recursive VFS call to open quota file, we usually
  end up locking covered vnode after mp is busied in sys_quotactl().
  
  Change the interface of VFS_QUOTACTL(), requiring that mp was unbusied
  by fs code, and do not try to pick up vfs_busy() reference in ufs quotaon,
  esp. if vfs_busy cannot succeed due to unmount being performed.
  
  Reported and tested by:	pho
  MFC after:	1 week

Modified:
  head/sys/kern/vfs_syscalls.c
  head/sys/ufs/ufs/ufs_quota.c

Modified: head/sys/kern/vfs_syscalls.c
==============================================================================
--- head/sys/kern/vfs_syscalls.c	Sun Jan  8 23:05:36 2012	(r229827)
+++ head/sys/kern/vfs_syscalls.c	Sun Jan  8 23:06:53 2012	(r229828)
@@ -86,6 +86,8 @@ __FBSDID("$FreeBSD$");
 #include <vm/vm_page.h>
 #include <vm/uma.h>
 
+#include <ufs/ufs/quota.h>
+
 static MALLOC_DEFINE(M_FADVISE, "fadvise", "posix_fadvise(2) information");
 
 SDT_PROVIDER_DEFINE(vfs);
@@ -212,7 +214,20 @@ sys_quotactl(td, uap)
 		return (error);
 	}
 	error = VFS_QUOTACTL(mp, uap->cmd, uap->uid, uap->arg);
-	vfs_unbusy(mp);
+
+	/*
+	 * Since quota on operation typically needs to open quota
+	 * file, the Q_QUOTAON handler needs to unbusy the mount point
+	 * before calling into namei.  Otherwise, unmount might be
+	 * started between two vfs_busy() invocations (first is our,
+	 * second is from mount point cross-walk code in lookup()),
+	 * causing deadlock.
+	 *
+	 * Require that Q_QUOTAON handles the vfs_busy() reference on
+	 * its own, always returning with ubusied mount point.
+	 */
+	if ((uap->cmd >> SUBCMDSHIFT) != Q_QUOTAON)
+		vfs_unbusy(mp);
 	VFS_UNLOCK_GIANT(vfslocked);
 	return (error);
 }

Modified: head/sys/ufs/ufs/ufs_quota.c
==============================================================================
--- head/sys/ufs/ufs/ufs_quota.c	Sun Jan  8 23:05:36 2012	(r229827)
+++ head/sys/ufs/ufs/ufs_quota.c	Sun Jan  8 23:06:53 2012	(r229828)
@@ -512,17 +512,29 @@ quotaon(struct thread *td, struct mount 
 
 	NDINIT(&nd, LOOKUP, FOLLOW | MPSAFE, UIO_USERSPACE, fname, td);
 	flags = FREAD | FWRITE;
+	vfs_ref(mp);
+	vfs_unbusy(mp);
 	error = vn_open(&nd, &flags, 0, NULL);
-	if (error)
+	if (error != 0) {
+		vfs_rel(mp);
 		return (error);
+	}
 	vfslocked = NDHASGIANT(&nd);
 	NDFREE(&nd, NDF_ONLY_PNBUF);
 	vp = nd.ni_vp;
-	if (vp->v_type != VREG) {
+	error = vfs_busy(mp, MBF_NOWAIT);
+	vfs_rel(mp);
+	if (error == 0) {
+		if (vp->v_type != VREG) {
+			error = EACCES;
+			vfs_unbusy(mp);
+		}
+	}
+	if (error != 0) {
 		VOP_UNLOCK(vp, 0);
 		(void) vn_close(vp, FREAD|FWRITE, td->td_ucred, td);
 		VFS_UNLOCK_GIANT(vfslocked);
-		return (EACCES);
+		return (error);
 	}
 
 	UFS_LOCK(ump);
@@ -531,6 +543,7 @@ quotaon(struct thread *td, struct mount 
 		VOP_UNLOCK(vp, 0);
 		(void) vn_close(vp, FREAD|FWRITE, td->td_ucred, td);
 		VFS_UNLOCK_GIANT(vfslocked);
+		vfs_unbusy(mp);
 		return (EALREADY);
 	}
 	ump->um_qflags[type] |= QTF_OPENING|QTF_CLOSING;
@@ -542,6 +555,7 @@ quotaon(struct thread *td, struct mount 
 		UFS_UNLOCK(ump);
 		(void) vn_close(vp, FREAD|FWRITE, td->td_ucred, td);
 		VFS_UNLOCK_GIANT(vfslocked);
+		vfs_unbusy(mp);
 		return (error);
 	}
 	VOP_UNLOCK(vp, 0);
@@ -619,6 +633,7 @@ again:
 		("quotaon: leaking flags"));
 	UFS_UNLOCK(ump);
 
+	vfs_unbusy(mp);
 	return (error);
 }