Skip site navigation (1)Skip section navigation (2) 
Date:      Sat, 19 Nov 2011 07:50:49 +0000 (UTC)
From:      Konstantin Belousov <kib@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r227697 - in head/sys: cddl/contrib/opensolaris/uts/common/fs/zfs fs/devfs fs/nullfs fs/pseudofs kern
Message-ID:  <201111190750.pAJ7onQ2030087@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: kib
Date: Sat Nov 19 07:50:49 2011
New Revision: 227697
URL: http://svn.freebsd.org/changeset/base/227697

Log:
  Existing VOP_VPTOCNP() interface has a fatal flow that is critical for
  nullfs.  The problem is that resulting vnode is only required to be
  held on return from the successfull call to vop, instead of being
  referenced.
  
  Nullfs VOP_INACTIVE() method reclaims the vnode, which in combination
  with the VOP_VPTOCNP() interface means that the directory vnode
  returned from VOP_VPTOCNP() is reclaimed in advance, causing
  vn_fullpath() to error with EBADF or like.
  
  Change the interface for VOP_VPTOCNP(), now the dvp must be
  referenced. Convert all in-tree implementations of VOP_VPTOCNP(),
  which is trivial, because vhold(9) and vref(9) are similar in the
  locking prerequisites. Out-of-tree fs implementation of VOP_VPTOCNP(),
  if any, should have no trouble with the fix.
  
  Tested by:	pho
  Reviewed by:	mckusick
  MFC after:	3 weeks (subject of re approval)

Modified:
  head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
  head/sys/fs/devfs/devfs_vnops.c
  head/sys/fs/nullfs/null_vnops.c
  head/sys/fs/pseudofs/pseudofs_vnops.c
  head/sys/kern/vfs_cache.c
  head/sys/kern/vfs_default.c

Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
==============================================================================
--- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c	Sat Nov 19 07:41:37 2011	(r227696)
+++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c	Sat Nov 19 07:50:49 2011	(r227697)
@@ -1594,7 +1594,7 @@ zfsctl_snapshot_vptocnp(struct vop_vptoc
 		*ap->a_buflen -= len;
 		bcopy(sep->se_name, ap->a_buf + *ap->a_buflen, len);
 		mutex_exit(&sdp->sd_lock);
-		vhold(dvp);
+		vref(dvp);
 		*ap->a_vpp = dvp;
 	}
 	VN_RELE(dvp);

Modified: head/sys/fs/devfs/devfs_vnops.c
==============================================================================
--- head/sys/fs/devfs/devfs_vnops.c	Sat Nov 19 07:41:37 2011	(r227696)
+++ head/sys/fs/devfs/devfs_vnops.c	Sat Nov 19 07:50:49 2011	(r227697)
@@ -261,7 +261,7 @@ devfs_vptocnp(struct vop_vptocnp_args *a
 	} else if (vp->v_type == VDIR) {
 		if (dd == dmp->dm_rootdir) {
 			*dvp = vp;
-			vhold(*dvp);
+			vref(*dvp);
 			goto finished;
 		}
 		i -= dd->de_dirent->d_namlen;
@@ -289,6 +289,8 @@ devfs_vptocnp(struct vop_vptocnp_args *a
 		mtx_unlock(&devfs_de_interlock);
 		vholdl(*dvp);
 		VI_UNLOCK(*dvp);
+		vref(*dvp);
+		vdrop(*dvp);
 	} else {
 		mtx_unlock(&devfs_de_interlock);
 		error = ENOENT;

Modified: head/sys/fs/nullfs/null_vnops.c
==============================================================================
--- head/sys/fs/nullfs/null_vnops.c	Sat Nov 19 07:41:37 2011	(r227696)
+++ head/sys/fs/nullfs/null_vnops.c	Sat Nov 19 07:50:49 2011	(r227697)
@@ -784,6 +784,7 @@ null_vptocnp(struct vop_vptocnp_args *ap
 	vhold(lvp);
 	VOP_UNLOCK(vp, 0); /* vp is held by vn_vptocnp_locked that called us */
 	ldvp = lvp;
+	vref(lvp);
 	error = vn_vptocnp(&ldvp, cred, ap->a_buf, ap->a_buflen);
 	vdrop(lvp);
 	if (error != 0) {
@@ -797,19 +798,17 @@ null_vptocnp(struct vop_vptocnp_args *ap
 	 */
 	error = vn_lock(ldvp, LK_EXCLUSIVE);
 	if (error != 0) {
+		vrele(ldvp);
 		vn_lock(vp, locked | LK_RETRY);
-		vdrop(ldvp);
 		return (ENOENT);
 	}
 	vref(ldvp);
-	vdrop(ldvp);
 	error = null_nodeget(vp->v_mount, ldvp, dvp);
 	if (error == 0) {
 #ifdef DIAGNOSTIC
 		NULLVPTOLOWERVP(*dvp);
 #endif
-		vhold(*dvp);
-		vput(*dvp);
+		VOP_UNLOCK(*dvp, 0); /* keep reference on *dvp */
 	} else
 		vput(ldvp);
 

Modified: head/sys/fs/pseudofs/pseudofs_vnops.c
==============================================================================
--- head/sys/fs/pseudofs/pseudofs_vnops.c	Sat Nov 19 07:41:37 2011	(r227696)
+++ head/sys/fs/pseudofs/pseudofs_vnops.c	Sat Nov 19 07:50:49 2011	(r227697)
@@ -410,8 +410,7 @@ pfs_vptocnp(struct vop_vptocnp_args *ap)
 	}
 
 	*buflen = i;
-	vhold(*dvp);
-	vput(*dvp);
+	VOP_UNLOCK(*dvp, 0);
 	vn_lock(vp, locked | LK_RETRY);
 	vfs_unbusy(mp);
 

Modified: head/sys/kern/vfs_cache.c
==============================================================================
--- head/sys/kern/vfs_cache.c	Sat Nov 19 07:41:37 2011	(r227696)
+++ head/sys/kern/vfs_cache.c	Sat Nov 19 07:50:49 2011	(r227697)
@@ -1068,16 +1068,8 @@ vn_vptocnp(struct vnode **vp, struct ucr
 
 	CACHE_RLOCK();
 	error = vn_vptocnp_locked(vp, cred, buf, buflen);
-	if (error == 0) {
-		/*
-		 * vn_vptocnp_locked() dropped hold acquired by
-		 * VOP_VPTOCNP immediately after locking the
-		 * cache. Since we are going to drop the cache rlock,
-		 * re-hold the result.
-		 */
-		vhold(*vp);
+	if (error == 0)
 		CACHE_RUNLOCK();
-	}
 	return (error);
 }
 
@@ -1096,6 +1088,9 @@ vn_vptocnp_locked(struct vnode **vp, str
 	if (ncp != NULL) {
 		if (*buflen < ncp->nc_nlen) {
 			CACHE_RUNLOCK();
+			vfslocked = VFS_LOCK_GIANT((*vp)->v_mount);
+			vrele(*vp);
+			VFS_UNLOCK_GIANT(vfslocked);
 			numfullpathfail4++;
 			error = ENOMEM;
 			SDT_PROBE(vfs, namecache, fullpath, return, error,
@@ -1106,18 +1101,23 @@ vn_vptocnp_locked(struct vnode **vp, str
 		memcpy(buf + *buflen, ncp->nc_name, ncp->nc_nlen);
 		SDT_PROBE(vfs, namecache, fullpath, hit, ncp->nc_dvp,
 		    ncp->nc_name, vp, 0, 0);
+		dvp = *vp;
 		*vp = ncp->nc_dvp;
+		vref(*vp);
+		CACHE_RUNLOCK();
+		vfslocked = VFS_LOCK_GIANT(dvp->v_mount);
+		vrele(dvp);
+		VFS_UNLOCK_GIANT(vfslocked);
+		CACHE_RLOCK();
 		return (0);
 	}
 	SDT_PROBE(vfs, namecache, fullpath, miss, vp, 0, 0, 0, 0);
 
-	vhold(*vp);
 	CACHE_RUNLOCK();
 	vfslocked = VFS_LOCK_GIANT((*vp)->v_mount);
 	vn_lock(*vp, LK_SHARED | LK_RETRY);
 	error = VOP_VPTOCNP(*vp, &dvp, cred, buf, buflen);
-	VOP_UNLOCK(*vp, 0);
-	vdrop(*vp);
+	vput(*vp);
 	VFS_UNLOCK_GIANT(vfslocked);
 	if (error) {
 		numfullpathfail2++;
@@ -1128,16 +1128,20 @@ vn_vptocnp_locked(struct vnode **vp, str
 
 	*vp = dvp;
 	CACHE_RLOCK();
-	if ((*vp)->v_iflag & VI_DOOMED) {
+	if (dvp->v_iflag & VI_DOOMED) {
 		/* forced unmount */
 		CACHE_RUNLOCK();
-		vdrop(*vp);
+		vfslocked = VFS_LOCK_GIANT(dvp->v_mount);
+		vrele(dvp);
+		VFS_UNLOCK_GIANT(vfslocked);
 		error = ENOENT;
 		SDT_PROBE(vfs, namecache, fullpath, return, error, vp,
 		    NULL, 0, 0);
 		return (error);
 	}
-	vdrop(*vp);
+	/*
+	 * *vp has its use count incremented still.
+	 */
 
 	return (0);
 }
@@ -1149,10 +1153,11 @@ static int
 vn_fullpath1(struct thread *td, struct vnode *vp, struct vnode *rdir,
     char *buf, char **retbuf, u_int buflen)
 {
-	int error, slash_prefixed;
+	int error, slash_prefixed, vfslocked;
 #ifdef KDTRACE_HOOKS
 	struct vnode *startvp = vp;
 #endif
+	struct vnode *vp1;
 
 	buflen--;
 	buf[buflen] = '\0';
@@ -1161,6 +1166,7 @@ vn_fullpath1(struct thread *td, struct v
 
 	SDT_PROBE(vfs, namecache, fullpath, entry, vp, 0, 0, 0, 0);
 	numfullpathcalls++;
+	vref(vp);
 	CACHE_RLOCK();
 	if (vp->v_type != VDIR) {
 		error = vn_vptocnp_locked(&vp, td->td_ucred, buf, &buflen);
@@ -1168,6 +1174,9 @@ vn_fullpath1(struct thread *td, struct v
 			return (error);
 		if (buflen == 0) {
 			CACHE_RUNLOCK();
+			vfslocked = VFS_LOCK_GIANT(vp->v_mount);
+			vrele(vp);
+			VFS_UNLOCK_GIANT(vfslocked);
 			return (ENOMEM);
 		}
 		buf[--buflen] = '/';
@@ -1177,16 +1186,29 @@ vn_fullpath1(struct thread *td, struct v
 		if (vp->v_vflag & VV_ROOT) {
 			if (vp->v_iflag & VI_DOOMED) {	/* forced unmount */
 				CACHE_RUNLOCK();
+				vfslocked = VFS_LOCK_GIANT(vp->v_mount);
+				vrele(vp);
+				VFS_UNLOCK_GIANT(vfslocked);
 				error = ENOENT;
 				SDT_PROBE(vfs, namecache, fullpath, return,
 				    error, vp, NULL, 0, 0);
 				break;
 			}
-			vp = vp->v_mount->mnt_vnodecovered;
+			vp1 = vp->v_mount->mnt_vnodecovered;
+			vref(vp1);
+			CACHE_RUNLOCK();
+			vfslocked = VFS_LOCK_GIANT(vp->v_mount);
+			vrele(vp);
+			VFS_UNLOCK_GIANT(vfslocked);
+			vp = vp1;
+			CACHE_RLOCK();
 			continue;
 		}
 		if (vp->v_type != VDIR) {
 			CACHE_RUNLOCK();
+			vfslocked = VFS_LOCK_GIANT(vp->v_mount);
+			vrele(vp);
+			VFS_UNLOCK_GIANT(vfslocked);
 			numfullpathfail1++;
 			error = ENOTDIR;
 			SDT_PROBE(vfs, namecache, fullpath, return,
@@ -1198,6 +1220,9 @@ vn_fullpath1(struct thread *td, struct v
 			break;
 		if (buflen == 0) {
 			CACHE_RUNLOCK();
+			vfslocked = VFS_LOCK_GIANT(vp->v_mount);
+			vrele(vp);
+			VFS_UNLOCK_GIANT(vfslocked);
 			error = ENOMEM;
 			SDT_PROBE(vfs, namecache, fullpath, return, error,
 			    startvp, NULL, 0, 0);
@@ -1211,6 +1236,9 @@ vn_fullpath1(struct thread *td, struct v
 	if (!slash_prefixed) {
 		if (buflen == 0) {
 			CACHE_RUNLOCK();
+			vfslocked = VFS_LOCK_GIANT(vp->v_mount);
+			vrele(vp);
+			VFS_UNLOCK_GIANT(vfslocked);
 			numfullpathfail4++;
 			SDT_PROBE(vfs, namecache, fullpath, return, ENOMEM,
 			    startvp, NULL, 0, 0);
@@ -1220,6 +1248,9 @@ vn_fullpath1(struct thread *td, struct v
 	}
 	numfullpathfound++;
 	CACHE_RUNLOCK();
+	vfslocked = VFS_LOCK_GIANT(vp->v_mount);
+	vrele(vp);
+	VFS_UNLOCK_GIANT(vfslocked);
 
 	SDT_PROBE(vfs, namecache, fullpath, return, 0, startvp, buf + buflen,
 	    0, 0);

Modified: head/sys/kern/vfs_default.c
==============================================================================
--- head/sys/kern/vfs_default.c	Sat Nov 19 07:41:37 2011	(r227696)
+++ head/sys/kern/vfs_default.c	Sat Nov 19 07:50:49 2011	(r227697)
@@ -844,7 +844,7 @@ out:
 	free(dirbuf, M_TEMP);
 	if (!error) {
 		*buflen = i;
-		vhold(*dvp);
+		vref(*dvp);
 	}
 	if (covered) {
 		vput(*dvp);

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201111190750.pAJ7onQ2030087>