This change addresses 4 bugs in ZFS exposed by Richard Kojedzinszky's

crash.sh script attached to FreeNAS bug 4109:
https://bugs.freenas.org/issues/4109

Three are in the snapshot layer:
a) AVG explains in his notes: https://wiki.freebsd.org/AvgVfsSolarisVsFreeBSD

"VOP_INACTIVE must not do any destructive actions to a vnode
and its filesystem node, nor invalidate them in any way."
gfs_vop_inactive and zfsctl_snapshot_inactive did just that. In
OpenSolaris VOP_INACTIVE is much closer to FreeBSD's VOP_RECLAIM.
Rename & move them to gfs_vop_reclaim and zfsctl_snapshot_reclaim
and merge in the requisite vnode_destroy from zfsctl_common_reclaim.

b) gfs_lookup_dot and various zfsctl functions do not honor the
FreeBSD VFS convention of only locking from the root downward. When
looking up ".." the convention is to drop the current leaf vnode lock before
acquiring the directory vnode and then subsequently re-acquiring the lock on the
leaf vnode. This fixes that in all the places that our exercised by crash.sh.

c) The snapshot may already be unmounted when the directory vnode is reclaimed.
Check for this case and return.

One in the common layer:
d) Callers of traverse expect the reference to the vnode passed in to be
maintained. Don't release it.

This last one may be an unclear contract. There may in fact be some callers that
do expect the reference to be dropped on success in addition to callers that
expect it to be released. In this case a further audit of the callers is needed
and a consensus on the correct behavior.

PR:	184677
Submitted by:	kmacy
Reviewed by:	delphij, will, avg
MFC after:	2 weeks
Sponsored by:	iXsystems

4 files changed, 56 insertions, 29 deletions

diff --git a/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c b/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c
index 94383d6..848007e 100644
--- a/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c
+++ b/sys/cddl/compat/opensolaris/kern/opensolaris_lookup.c
@@ -91,11 +91,11 @@ traverse(vnode_t **cvpp, int lktype)
 		error = vfs_busy(vfsp, 0);
 		/*
 		 * tvp is NULL for *cvpp vnode, which we can't unlock.
+		 * At least some callers expect the reference to be
+		 * maintained to the original *cvpp
 		 */
 		if (tvp != NULL)
 			vput(cvp);
-		else
-			vrele(cvp);
 		if (error)
 			return (error);
 
diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c b/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c
index 59944a1..4bcdfc6 100644
--- a/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/gfs.c
@@ -90,7 +90,7 @@
  *	gfs_dir_lookup()
  *	gfs_dir_readdir()
  *
- * 	gfs_vop_inactive()
+ * 	gfs_vop_reclaim()
  * 	gfs_vop_lookup()
  * 	gfs_vop_readdir()
  * 	gfs_vop_map()
@@ -435,6 +435,8 @@ gfs_readdir_fini(gfs_readdir_state_t *st, int error, int *eofp, int eof)
 int
 gfs_lookup_dot(vnode_t **vpp, vnode_t *dvp, vnode_t *pvp, const char *nm)
 {
+	int ltype;
+
 	if (*nm == '\0' || strcmp(nm, ".") == 0) {
 		VN_HOLD(dvp);
 		*vpp = dvp;
@@ -444,11 +446,15 @@ gfs_lookup_dot(vnode_t **vpp, vnode_t *dvp, vnode_t *pvp, const char *nm)
 			ASSERT(dvp->v_flag & VROOT);
 			VN_HOLD(dvp);
 			*vpp = dvp;
+			ASSERT_VOP_ELOCKED(dvp, "gfs_lookup_dot: non-locked dvp");
 		} else {
+			ltype = VOP_ISLOCKED(dvp);
+			VOP_UNLOCK(dvp, 0);
 			VN_HOLD(pvp);
 			*vpp = pvp;
+			vn_lock(*vpp, LK_EXCLUSIVE | LK_RETRY);
+			vn_lock(dvp, ltype | LK_RETRY);
 		}
-		vn_lock(*vpp, LK_EXCLUSIVE | LK_RETRY);
 		return (0);
 	}
 
@@ -618,7 +624,7 @@ gfs_root_create_file(size_t size, vfs_t *vfsp, vnodeops_t *ops, ino64_t ino)
 /*
  * gfs_file_inactive()
  *
- * Called from the VOP_INACTIVE() routine.  If necessary, this routine will
+ * Called from the VOP_RECLAIM() routine.  If necessary, this routine will
  * remove the given vnode from the parent directory and clean up any references
  * in the VFS layer.
  *
@@ -1215,15 +1221,15 @@ gfs_vop_map(vnode_t *vp, offset_t off, struct as *as, caddr_t *addrp,
 #endif	/* sun */
 
 /*
- * gfs_vop_inactive: VOP_INACTIVE() entry point
+ * gfs_vop_reclaim: VOP_RECLAIM() entry point (solaris' VOP_INACTIVE())
  *
  * Given a vnode that is a GFS file or directory, call gfs_file_inactive() or
  * gfs_dir_inactive() as necessary, and kmem_free()s associated private data.
  */
 /* ARGSUSED */
 int
-gfs_vop_inactive(ap)
-	struct vop_inactive_args /* {
+gfs_vop_reclaim(ap)
+	struct vop_reclaim_args /* {
 		struct vnode *a_vp;
 		struct thread *a_td;
 	} */ *ap;
@@ -1236,6 +1242,7 @@ gfs_vop_inactive(ap)
 	else
 		gfs_file_inactive(vp);
 
+	vnode_destroy_vobject(vp);
 	VI_LOCK(vp);
 	vp->v_data = NULL;
 	VI_UNLOCK(vp);
diff --git a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
index 3cc70b2..01fae24 100644
--- a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
+++ b/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_ctldir.c
@@ -603,16 +603,27 @@ zfsctl_freebsd_root_lookup(ap)
 	int nameiop = ap->a_cnp->cn_nameiop;
 	char nm[NAME_MAX + 1];
 	int err;
+	int ltype;
 
 	if ((flags & ISLASTCN) && (nameiop == RENAME || nameiop == CREATE))
 		return (EOPNOTSUPP);
 
 	ASSERT(ap->a_cnp->cn_namelen < sizeof(nm));
 	strlcpy(nm, ap->a_cnp->cn_nameptr, ap->a_cnp->cn_namelen + 1);
-
 	err = zfsctl_root_lookup(dvp, nm, vpp, NULL, 0, NULL, cr, NULL, NULL, NULL);
-	if (err == 0 && (nm[0] != '.' || nm[1] != '\0'))
+	if (err == 0 && (nm[0] != '.' || nm[1] != '\0')) {
+		ltype = VOP_ISLOCKED(dvp);
+		if (flags & ISDOTDOT) {
+			VN_HOLD(*vpp);
+			VOP_UNLOCK(dvp, 0);
+		}
 		vn_lock(*vpp, LK_EXCLUSIVE | LK_RETRY);
+		if (flags & ISDOTDOT) {
+			VN_RELE(*vpp);
+			vn_lock(dvp, ltype| LK_RETRY);
+		}
+	}
+
 	return (err);
 }
 
@@ -625,8 +636,8 @@ static struct vop_vector zfsctl_ops_root = {
 	.vop_access =	zfsctl_common_access,
 	.vop_readdir =	gfs_vop_readdir,
 	.vop_lookup =	zfsctl_freebsd_root_lookup,
-	.vop_inactive =	gfs_vop_inactive,
-	.vop_reclaim =	zfsctl_common_reclaim,
+	.vop_inactive =	VOP_NULL,
+	.vop_reclaim =	gfs_vop_reclaim,
 #ifdef TODO
 	.vop_pathconf =	zfsctl_pathconf,
 #endif
@@ -679,7 +690,7 @@ zfsctl_unmount_snap(zfs_snapentry_t *sep, int fflags, cred_t *cr)
 	 * the sd_lock mutex held by our caller.
 	 */
 	ASSERT(svp->v_count == 1);
-	gfs_vop_inactive(svp, cr, NULL);
+	gfs_vop_reclaim(svp, cr, NULL);
 
 	kmem_free(sep->se_name, strlen(sep->se_name) + 1);
 	kmem_free(sep, sizeof (zfs_snapentry_t));
@@ -949,7 +960,7 @@ zfsctl_snapdir_lookup(ap)
 	avl_index_t where;
 	zfsvfs_t *zfsvfs = dvp->v_vfsp->vfs_data;
 	int err;
-	int flags = 0;
+	int ltype, flags = 0;
 
 	/*
 	 * No extended attributes allowed under .zfs
@@ -973,7 +984,6 @@ zfsctl_snapdir_lookup(ap)
 		return (SET_ERROR(ENOENT));
 
 	ZFS_ENTER(zfsvfs);
-
 	if (gfs_lookup_dot(vpp, dvp, zfsvfs->z_ctldir, nm) == 0) {
 		ZFS_EXIT(zfsvfs);
 		return (0);
@@ -1420,8 +1430,8 @@ static struct vop_vector zfsctl_ops_shares = {
 	.vop_access =	zfsctl_common_access,
 	.vop_readdir =	zfsctl_shares_readdir,
 	.vop_lookup =	zfsctl_shares_lookup,
-	.vop_inactive =	gfs_vop_inactive,
-	.vop_reclaim =	zfsctl_common_reclaim,
+	.vop_inactive =	VOP_NULL,
+	.vop_reclaim =	gfs_vop_reclaim,
 	.vop_fid =	zfsctl_shares_fid,
 };
 #endif	/* !sun */
@@ -1449,8 +1459,9 @@ zfsctl_snapshot_mknode(vnode_t *pvp, uint64_t objset)
 	return (vp);
 }
 
+
 static int
-zfsctl_snapshot_inactive(ap)
+zfsctl_snapshot_reclaim(ap)
 	struct vop_inactive_args /* {
 		struct vnode *a_vp;
 		struct thread *a_td;
@@ -1458,19 +1469,20 @@ zfsctl_snapshot_inactive(ap)
 {
 	vnode_t *vp = ap->a_vp;
 	cred_t *cr = ap->a_td->td_ucred;
-	struct vop_inactive_args iap;
+	struct vop_reclaim_args iap;
 	zfsctl_snapdir_t *sdp;
 	zfs_snapentry_t *sep, *next;
 	int locked;
 	vnode_t *dvp;
 
-	if (vp->v_count > 0)
-		goto end;
-
 	VERIFY(gfs_dir_lookup(vp, "..", &dvp, cr, 0, NULL, NULL) == 0);
 	sdp = dvp->v_data;
 	VOP_UNLOCK(dvp, 0);
-
+	/* this may already have been unmounted */
+	if (sdp == NULL) {
+		VN_RELE(dvp);
+		return (0);
+	}
 	if (!(locked = MUTEX_HELD(&sdp->sd_lock)))
 		mutex_enter(&sdp->sd_lock);
 
@@ -1494,7 +1506,6 @@ zfsctl_snapshot_inactive(ap)
 		mutex_exit(&sdp->sd_lock);
 	VN_RELE(dvp);
 
-end:
 	/*
 	 * Dispose of the vnode for the snapshot mount point.
 	 * This is safe to do because once this entry has been removed
@@ -1503,7 +1514,9 @@ end:
 	 * creating a new vnode.
 	 */
 	iap.a_vp = vp;
-	return (gfs_vop_inactive(&iap));
+	gfs_vop_reclaim(&iap);
+	return (0);
+
 }
 
 static int
@@ -1587,8 +1600,15 @@ zfsctl_snapshot_lookup(ap)
 
 	error = zfsctl_root_lookup(zfsvfs->z_ctldir, "snapshot", vpp,
 	    NULL, 0, NULL, cr, NULL, NULL, NULL);
-	if (error == 0)
+	if (error == 0) {
+		int ltype = VOP_ISLOCKED(dvp);
+		VN_HOLD(*vpp);
+		VOP_UNLOCK(dvp, 0);
 		vn_lock(*vpp, LK_EXCLUSIVE | LK_RETRY);
+		VN_RELE(*vpp);
+		vn_lock(dvp, ltype | LK_RETRY);
+	}
+
 	return (error);
 }
 
@@ -1640,9 +1660,9 @@ zfsctl_snapshot_vptocnp(struct vop_vptocnp_args *ap)
  */
 static struct vop_vector zfsctl_ops_snapshot = {
 	.vop_default =	&default_vnodeops,
-	.vop_inactive =	zfsctl_snapshot_inactive,
+	.vop_inactive =	VOP_NULL,
 	.vop_lookup =	zfsctl_snapshot_lookup,
-	.vop_reclaim =	zfsctl_common_reclaim,
+	.vop_reclaim =	zfsctl_snapshot_reclaim,
 	.vop_getattr =	zfsctl_snapshot_getattr,
 	.vop_fid =	zfsctl_snapshot_fid,
 	.vop_vptocnp =	zfsctl_snapshot_vptocnp,
diff --git a/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h b/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h
index f3fc634..0409172 100644
--- a/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h
+++ b/sys/cddl/contrib/opensolaris/uts/common/sys/gfs.h
@@ -149,7 +149,7 @@ extern int gfs_get_parent_ino(vnode_t *, cred_t *, caller_context_t *,
 extern int gfs_lookup_dot(vnode_t **, vnode_t *, vnode_t *, const char *);
 
 extern int gfs_vop_readdir(struct vop_readdir_args *);
-extern int gfs_vop_inactive(struct vop_inactive_args *);
+extern int gfs_vop_reclaim(struct vop_reclaim_args *);
 
 
 #ifdef	__cplusplus