- Reorder calls to vrele() after calls to vput() when the vrele is a

   directory.  vrele() may lock the passed vnode, which in these cases would
   give an invalid lock order of child -> parent.  These situations are
   deadlock prone although do not typically deadlock because the vrele
   is typically not releasing the last reference to the vnode.  Users of
   vrele must consider it as a call to vn_lock() and order it appropriately.

MFC After: 	1 week
Sponsored by:	Isilon Systems, Inc.
Tested by:	kkenn

8 files changed, 81 insertions, 88 deletions

diff --git a/sys/coda/coda_vnops.c b/sys/coda/coda_vnops.c
index 94d95d9..75de19f 100644
--- a/sys/coda/coda_vnops.c
+++ b/sys/coda/coda_vnops.c
@@ -1289,21 +1289,18 @@ coda_rename(struct vop_rename_args *ap)
     /* XXX - do we need to call cache pureg on the moved vnode? */
     cache_purge(ap->a_fvp);
 
-    /* It seems to be incumbent on us to drop locks on all four vnodes */
-    /* From-vnodes are not locked, only ref'd.  To-vnodes are locked. */
-
-    vrele(ap->a_fvp);
+    /* Release parents first, then children. */
     vrele(odvp);
-
     if (ap->a_tvp) {
-	if (ap->a_tvp == ndvp) {
-	    vrele(ap->a_tvp);
-	} else {
-	    vput(ap->a_tvp);
-	}
-    }
+	if (ap->a_tvp == ndvp)
+	    vrele(ndvp);
+	else
+	    vput(ndvp);
+	vput(ap->a_tvp);
+    } else
+	vput(ndvp);
+    vrele(ap->a_fvp);
 
-    vput(ndvp);
     return(error);
 }
 
diff --git a/sys/fs/coda/coda_vnops.c b/sys/fs/coda/coda_vnops.c
index 94d95d9..75de19f 100644
--- a/sys/fs/coda/coda_vnops.c
+++ b/sys/fs/coda/coda_vnops.c
@@ -1289,21 +1289,18 @@ coda_rename(struct vop_rename_args *ap)
     /* XXX - do we need to call cache pureg on the moved vnode? */
     cache_purge(ap->a_fvp);
 
-    /* It seems to be incumbent on us to drop locks on all four vnodes */
-    /* From-vnodes are not locked, only ref'd.  To-vnodes are locked. */
-
-    vrele(ap->a_fvp);
+    /* Release parents first, then children. */
     vrele(odvp);
-
     if (ap->a_tvp) {
-	if (ap->a_tvp == ndvp) {
-	    vrele(ap->a_tvp);
-	} else {
-	    vput(ap->a_tvp);
-	}
-    }
+	if (ap->a_tvp == ndvp)
+	    vrele(ndvp);
+	else
+	    vput(ndvp);
+	vput(ap->a_tvp);
+    } else
+	vput(ndvp);
+    vrele(ap->a_fvp);
 
-    vput(ndvp);
     return(error);
 }
 
diff --git a/sys/fs/msdosfs/msdosfs_vnops.c b/sys/fs/msdosfs/msdosfs_vnops.c
index 15665e0..abf4e8a 100644
--- a/sys/fs/msdosfs/msdosfs_vnops.c
+++ b/sys/fs/msdosfs/msdosfs_vnops.c
@@ -1122,10 +1122,10 @@ abortit:
 		 */
 		if (doingdirectory)
 			panic("rename: lost dir entry");
-		vrele(ap->a_fvp);
 		if (newparent)
 			VOP_UNLOCK(tdvp, 0, td);
 		vrele(tdvp);
+		vrele(ap->a_fvp);
 		return 0;
 	}
 	xp = VTODE(fvp);
@@ -1143,10 +1143,10 @@ abortit:
 	if (xp != ip) {
 		if (doingdirectory)
 			panic("rename: lost dir entry");
-		vrele(ap->a_fvp);
 		VOP_UNLOCK(fvp, 0, td);
 		if (newparent)
 			VOP_UNLOCK(fdvp, 0, td);
+		vrele(ap->a_fvp);
 		xp = NULL;
 	} else {
 		vrele(fvp);
diff --git a/sys/kern/vfs_extattr.c b/sys/kern/vfs_extattr.c
index 2d61977..deff4a73 100644
--- a/sys/kern/vfs_extattr.c
+++ b/sys/kern/vfs_extattr.c
@@ -1185,11 +1185,11 @@ restart:
 	vp = nd.ni_vp;
 	if (vp != NULL) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		vrele(vp);
 		if (vp == nd.ni_dvp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		vrele(vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (EEXIST);
 	} else {
@@ -1288,11 +1288,11 @@ restart:
 	vfslocked = NDHASGIANT(&nd);
 	if (nd.ni_vp != NULL) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		vrele(nd.ni_vp);
 		if (nd.ni_vp == nd.ni_dvp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		vrele(nd.ni_vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (EEXIST);
 	}
@@ -1425,11 +1425,11 @@ kern_link(struct thread *td, char *path, char *link, enum uio_seg segflg)
 	if ((error = namei(&nd)) == 0) {
 		lvfslocked = NDHASGIANT(&nd);
 		if (nd.ni_vp != NULL) {
-			vrele(nd.ni_vp);
 			if (nd.ni_dvp == nd.ni_vp)
 				vrele(nd.ni_dvp);
 			else
 				vput(nd.ni_dvp);
+			vrele(nd.ni_vp);
 			error = EEXIST;
 		} else if ((error = vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td))
 		    == 0) {
@@ -1502,11 +1502,11 @@ restart:
 	vfslocked = NDHASGIANT(&nd);
 	if (nd.ni_vp) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		vrele(nd.ni_vp);
 		if (nd.ni_vp == nd.ni_dvp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		vrele(nd.ni_vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		error = EEXIST;
 		goto out;
@@ -1573,12 +1573,12 @@ restart:
 
 	if (nd.ni_vp != NULLVP || !(nd.ni_cnd.cn_flags & ISWHITEOUT)) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		if (nd.ni_vp)
-			vrele(nd.ni_vp);
 		if (nd.ni_vp == nd.ni_dvp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		if (nd.ni_vp)
+			vrele(nd.ni_vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (EEXIST);
 	}
@@ -1650,11 +1650,11 @@ restart:
 	if (error == 0) {
 		if (vn_start_write(nd.ni_dvp, &mp, V_NOWAIT) != 0) {
 			NDFREE(&nd, NDF_ONLY_PNBUF);
+			vput(nd.ni_dvp);
 			if (vp == nd.ni_dvp)
 				vrele(vp);
 			else
 				vput(vp);
-			vput(nd.ni_dvp);
 			VFS_UNLOCK_GIANT(vfslocked);
 			if ((error = vn_start_write(NULL, &mp,
 			    V_XSLEEP | PCATCH)) != 0)
@@ -1675,11 +1675,11 @@ out:
 		vn_finished_write(mp);
 	}
 	NDFREE(&nd, NDF_ONLY_PNBUF);
+	vput(nd.ni_dvp);
 	if (vp == nd.ni_dvp)
 		vrele(vp);
 	else
 		vput(vp);
-	vput(nd.ni_dvp);
 	VFS_UNLOCK_GIANT(vfslocked);
 	return (error);
 }
@@ -3322,7 +3322,6 @@ restart:
 	vp = nd.ni_vp;
 	if (vp != NULL) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		vrele(vp);
 		/*
 		 * XXX namei called with LOCKPARENT but not LOCKLEAF has
 		 * the strange behaviour of leaving the vnode unlocked
@@ -3332,6 +3331,7 @@ restart:
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		vrele(vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (EEXIST);
 	}
@@ -3429,11 +3429,11 @@ restart:
 #endif
 	if (vn_start_write(nd.ni_dvp, &mp, V_NOWAIT) != 0) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
+		vput(vp);
 		if (nd.ni_dvp == vp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
-		vput(vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		if ((error = vn_start_write(NULL, &mp, V_XSLEEP | PCATCH)) != 0)
 			return (error);
@@ -3445,11 +3445,11 @@ restart:
 	vn_finished_write(mp);
 out:
 	NDFREE(&nd, NDF_ONLY_PNBUF);
+	vput(vp);
 	if (nd.ni_dvp == vp)
 		vrele(nd.ni_dvp);
 	else
 		vput(nd.ni_dvp);
-	vput(vp);
 	VFS_UNLOCK_GIANT(vfslocked);
 	return (error);
 }
diff --git a/sys/kern/vfs_syscalls.c b/sys/kern/vfs_syscalls.c
index 2d61977..deff4a73 100644
--- a/sys/kern/vfs_syscalls.c
+++ b/sys/kern/vfs_syscalls.c
@@ -1185,11 +1185,11 @@ restart:
 	vp = nd.ni_vp;
 	if (vp != NULL) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		vrele(vp);
 		if (vp == nd.ni_dvp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		vrele(vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (EEXIST);
 	} else {
@@ -1288,11 +1288,11 @@ restart:
 	vfslocked = NDHASGIANT(&nd);
 	if (nd.ni_vp != NULL) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		vrele(nd.ni_vp);
 		if (nd.ni_vp == nd.ni_dvp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		vrele(nd.ni_vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (EEXIST);
 	}
@@ -1425,11 +1425,11 @@ kern_link(struct thread *td, char *path, char *link, enum uio_seg segflg)
 	if ((error = namei(&nd)) == 0) {
 		lvfslocked = NDHASGIANT(&nd);
 		if (nd.ni_vp != NULL) {
-			vrele(nd.ni_vp);
 			if (nd.ni_dvp == nd.ni_vp)
 				vrele(nd.ni_dvp);
 			else
 				vput(nd.ni_dvp);
+			vrele(nd.ni_vp);
 			error = EEXIST;
 		} else if ((error = vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td))
 		    == 0) {
@@ -1502,11 +1502,11 @@ restart:
 	vfslocked = NDHASGIANT(&nd);
 	if (nd.ni_vp) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		vrele(nd.ni_vp);
 		if (nd.ni_vp == nd.ni_dvp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		vrele(nd.ni_vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		error = EEXIST;
 		goto out;
@@ -1573,12 +1573,12 @@ restart:
 
 	if (nd.ni_vp != NULLVP || !(nd.ni_cnd.cn_flags & ISWHITEOUT)) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		if (nd.ni_vp)
-			vrele(nd.ni_vp);
 		if (nd.ni_vp == nd.ni_dvp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		if (nd.ni_vp)
+			vrele(nd.ni_vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (EEXIST);
 	}
@@ -1650,11 +1650,11 @@ restart:
 	if (error == 0) {
 		if (vn_start_write(nd.ni_dvp, &mp, V_NOWAIT) != 0) {
 			NDFREE(&nd, NDF_ONLY_PNBUF);
+			vput(nd.ni_dvp);
 			if (vp == nd.ni_dvp)
 				vrele(vp);
 			else
 				vput(vp);
-			vput(nd.ni_dvp);
 			VFS_UNLOCK_GIANT(vfslocked);
 			if ((error = vn_start_write(NULL, &mp,
 			    V_XSLEEP | PCATCH)) != 0)
@@ -1675,11 +1675,11 @@ out:
 		vn_finished_write(mp);
 	}
 	NDFREE(&nd, NDF_ONLY_PNBUF);
+	vput(nd.ni_dvp);
 	if (vp == nd.ni_dvp)
 		vrele(vp);
 	else
 		vput(vp);
-	vput(nd.ni_dvp);
 	VFS_UNLOCK_GIANT(vfslocked);
 	return (error);
 }
@@ -3322,7 +3322,6 @@ restart:
 	vp = nd.ni_vp;
 	if (vp != NULL) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
-		vrele(vp);
 		/*
 		 * XXX namei called with LOCKPARENT but not LOCKLEAF has
 		 * the strange behaviour of leaving the vnode unlocked
@@ -3332,6 +3331,7 @@ restart:
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
+		vrele(vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		return (EEXIST);
 	}
@@ -3429,11 +3429,11 @@ restart:
 #endif
 	if (vn_start_write(nd.ni_dvp, &mp, V_NOWAIT) != 0) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
+		vput(vp);
 		if (nd.ni_dvp == vp)
 			vrele(nd.ni_dvp);
 		else
 			vput(nd.ni_dvp);
-		vput(vp);
 		VFS_UNLOCK_GIANT(vfslocked);
 		if ((error = vn_start_write(NULL, &mp, V_XSLEEP | PCATCH)) != 0)
 			return (error);
@@ -3445,11 +3445,11 @@ restart:
 	vn_finished_write(mp);
 out:
 	NDFREE(&nd, NDF_ONLY_PNBUF);
+	vput(vp);
 	if (nd.ni_dvp == vp)
 		vrele(nd.ni_dvp);
 	else
 		vput(nd.ni_dvp);
-	vput(vp);
 	VFS_UNLOCK_GIANT(vfslocked);
 	return (error);
 }
diff --git a/sys/nfsserver/nfs_serv.c b/sys/nfsserver/nfs_serv.c
index 0b0aaa3..8117cbb 100644
--- a/sys/nfsserver/nfs_serv.c
+++ b/sys/nfsserver/nfs_serv.c
@@ -664,13 +664,13 @@ nfsmout:
 	NFSD_LOCK_ASSERT();
 	NFSD_UNLOCK();
 	mtx_lock(&Giant);	/* VFS */
+	if (ndp->ni_vp)
+		vput(ndp->ni_vp);
 	if (dirp)
 		vrele(dirp);
 	NDFREE(&nd, NDF_ONLY_PNBUF);
 	if (ndp->ni_startdir)
 		vrele(ndp->ni_startdir);
-	if (ndp->ni_vp)
-		vput(ndp->ni_vp);
 	mtx_unlock(&Giant);	/* VFS */
 	NFSD_LOCK();
 	return (error);
@@ -2004,13 +2004,6 @@ nfsmout:
 	NFSD_LOCK_ASSERT();
 	NFSD_UNLOCK();
 	mtx_lock(&Giant);	/* VFS */
-	if (nd.ni_startdir) {
-		vrele(nd.ni_startdir);
-		nd.ni_startdir = NULL;
-	}
-	if (dirp)
-		vrele(dirp);
-	NDFREE(&nd, NDF_ONLY_PNBUF);
 	if (nd.ni_dvp) {
 		if (nd.ni_dvp == nd.ni_vp)
 			vrele(nd.ni_dvp);
@@ -2019,6 +2012,13 @@ nfsmout:
 	}
 	if (nd.ni_vp)
 		vput(nd.ni_vp);
+	if (nd.ni_startdir) {
+		vrele(nd.ni_startdir);
+		nd.ni_startdir = NULL;
+	}
+	if (dirp)
+		vrele(dirp);
+	NDFREE(&nd, NDF_ONLY_PNBUF);
 	vn_finished_write(mp);
 	mtx_unlock(&Giant);	/* VFS */
 	NFSD_LOCK();
@@ -2162,18 +2162,6 @@ nfsrv_mknod(struct nfsrv_descript *nfsd, struct nfssvc_sock *slp,
 	 */
 out:
 	NFSD_UNLOCK_ASSERT();
-	if (nd.ni_startdir) {
-		vrele(nd.ni_startdir);
-		nd.ni_startdir = NULL;
-	}
-	NDFREE(&nd, NDF_ONLY_PNBUF);
-	if (nd.ni_dvp) {
-		if (nd.ni_dvp == nd.ni_vp)
-			vrele(nd.ni_dvp);
-		else
-			vput(nd.ni_dvp);
-		nd.ni_dvp = NULL;
-	}
 	vp = nd.ni_vp;
 	if (!error) {
 		bzero((caddr_t)fhp, sizeof(nfh));
@@ -2182,11 +2170,23 @@ out:
 		if (!error)
 			error = VOP_GETATTR(vp, vap, cred, td);
 	}
+	if (nd.ni_dvp) {
+		if (nd.ni_dvp == nd.ni_vp)
+			vrele(nd.ni_dvp);
+		else
+			vput(nd.ni_dvp);
+		nd.ni_dvp = NULL;
+	}
 	if (vp) {
 		vput(vp);
 		vp = NULL;
 		nd.ni_vp = NULL;
 	}
+	if (nd.ni_startdir) {
+		vrele(nd.ni_startdir);
+		nd.ni_startdir = NULL;
+	}
+	NDFREE(&nd, NDF_ONLY_PNBUF);
 	if (dirp) {
 		vn_lock(dirp, LK_EXCLUSIVE | LK_RETRY, td);
 		diraft_ret = VOP_GETATTR(dirp, &diraft, cred, td);
@@ -2214,11 +2214,6 @@ nfsmout:
 	NFSD_LOCK_ASSERT();
 	NFSD_UNLOCK();
 	mtx_lock(&Giant);	/* VFS */
-	if (dirp)
-		vrele(dirp);
-	if (nd.ni_startdir)
-		vrele(nd.ni_startdir);
-	NDFREE(&nd, NDF_ONLY_PNBUF);
 	if (nd.ni_dvp) {
 		if (nd.ni_dvp == nd.ni_vp)
 			vrele(nd.ni_dvp);
@@ -2227,6 +2222,11 @@ nfsmout:
 	}
 	if (nd.ni_vp)
 		vput(nd.ni_vp);
+	if (dirp)
+		vrele(dirp);
+	if (nd.ni_startdir)
+		vrele(nd.ni_startdir);
+	NDFREE(&nd, NDF_ONLY_PNBUF);
 	vn_finished_write(mp);
 	mtx_unlock(&Giant);	/* VFS */
 	NFSD_LOCK();
@@ -2577,11 +2577,6 @@ nfsmout:
 	NFSD_LOCK_ASSERT();
 	NFSD_UNLOCK();
 	mtx_lock(&Giant);	/* VFS */
-	if (tdirp)
-		vrele(tdirp);
-	if (tond.ni_startdir)
-		vrele(tond.ni_startdir);
-	NDFREE(&tond, NDF_ONLY_PNBUF);
 	if (tond.ni_dvp) {
 		if (tond.ni_dvp == tond.ni_vp)
 			vrele(tond.ni_dvp);
@@ -2590,7 +2585,11 @@ nfsmout:
 	}
 	if (tond.ni_vp)
 		vput(tond.ni_vp);
-
+	if (tdirp)
+		vrele(tdirp);
+	if (tond.ni_startdir)
+		vrele(tond.ni_startdir);
+	NDFREE(&tond, NDF_ONLY_PNBUF);
 	/*
 	 * Clear out fromnd related fields
 	 */
@@ -2751,8 +2750,6 @@ nfsmout:
 	NFSD_UNLOCK();
 	mtx_lock(&Giant);	/* VFS */
 	NDFREE(&nd, NDF_ONLY_PNBUF);
-	if (dirp)
-		vrele(dirp);
 	if (vp)
 		vput(vp);
 	if (nd.ni_dvp) {
@@ -2761,6 +2758,8 @@ nfsmout:
 		else
 			vput(nd.ni_dvp);
 	}
+	if (dirp)
+		vrele(dirp);
 	if (nd.ni_vp)
 		vrele(nd.ni_vp);
 	vn_finished_write(mp);
@@ -3117,8 +3116,6 @@ nfsmout:
 	NFSD_LOCK_ASSERT();
 	NFSD_UNLOCK();
 	mtx_lock(&Giant);	/* VFS */
-	if (dirp)
-		vrele(dirp);
 	if (nd.ni_dvp) {
 		NDFREE(&nd, NDF_ONLY_PNBUF);
 		if (nd.ni_dvp == nd.ni_vp && vpexcl)
@@ -3132,6 +3129,8 @@ nfsmout:
 		else
 			vrele(nd.ni_vp);
 	}
+	if (dirp)
+		vrele(dirp);
 	vn_finished_write(mp);
 	mtx_unlock(&Giant);	/* VFS */
 	NFSD_LOCK();
@@ -3259,8 +3258,6 @@ nfsmout:
 	NFSD_UNLOCK();
 	mtx_lock(&Giant);	/* VFS */
 	NDFREE(&nd, NDF_ONLY_PNBUF);
-	if (dirp)
-		vrele(dirp);
 	if (nd.ni_dvp) {
 		if (nd.ni_dvp == nd.ni_vp)
 			vrele(nd.ni_dvp);
@@ -3269,6 +3266,8 @@ nfsmout:
 	}
 	if (nd.ni_vp)
 		vput(nd.ni_vp);
+	if (dirp)
+		vrele(dirp);
 
 	vn_finished_write(mp);
 	mtx_unlock(&Giant);	/* VFS */
diff --git a/sys/nfsserver/nfs_srvsubs.c b/sys/nfsserver/nfs_srvsubs.c
index 437dd63..1aa976a 100644
--- a/sys/nfsserver/nfs_srvsubs.c
+++ b/sys/nfsserver/nfs_srvsubs.c
@@ -831,8 +831,8 @@ nfs_namei(struct nameidata *ndp, fhandle_t *fhp, int len,
 			if (ndp->ni_pathlen > 1)
 				uma_zfree(namei_zone, cp);
 		badlink2:
-			vrele(ndp->ni_dvp);
 			vput(ndp->ni_vp);
+			vrele(ndp->ni_dvp);
 			break;
 		}
 		linklen = MAXPATHLEN - auio.uio_resid;
diff --git a/sys/ufs/ufs/ufs_extattr.c b/sys/ufs/ufs/ufs_extattr.c
index 6f6de78..e6153f3 100644
--- a/sys/ufs/ufs/ufs_extattr.c
+++ b/sys/ufs/ufs/ufs_extattr.c
@@ -469,8 +469,8 @@ ufs_extattr_autostart(struct mount *mp, struct thread *td)
 	}
 	if (rvp == attr_dvp) {
 		/* Should never happen. */
-		vrele(attr_dvp);
 		vput(rvp);
+		vrele(attr_dvp);
 		return (EINVAL);
 	}
 	vrele(rvp);