Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 3 Dec 2012 22:15:16 +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: r243835 - head/sys/kern
Message-ID:  <201212032215.qB3MFGST062681@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: kib
Date: Mon Dec  3 22:15:16 2012
New Revision: 243835
URL: http://svnweb.freebsd.org/changeset/base/243835

Log:
  The vnode_free_list_mtx is required unconditionally when iterating
  over the active list. The mount interlock is not enough to guarantee
  the validity of the tailq link pointers. The __mnt_vnode_next_active()
  and __mnt_vnode_first_active() active lists iterators helper functions
  did not provided the neccessary stability for the list, allowing the
  iterators to pick garbage.
  
  This was uncovered after the r243599 made the active list iterators
  non-nop.
  
  Since a vnode interlock is before the vnode_free_list_mtx, obtain the
  vnode ilock in the non-blocking manner when under vnode_free_list_mtx,
  and restart iteration after the yield if the lock attempt failed.
  
  Assert that a vnode found on the list is active, and assert that the
  helpers return the vnode with interlock owned.
  
  Reported and tested by:	pho
  MFC after:	1 week

Modified:
  head/sys/kern/vfs_subr.c

Modified: head/sys/kern/vfs_subr.c
==============================================================================
--- head/sys/kern/vfs_subr.c	Mon Dec  3 21:49:37 2012	(r243834)
+++ head/sys/kern/vfs_subr.c	Mon Dec  3 22:15:16 2012	(r243835)
@@ -4718,10 +4718,20 @@ __mnt_vnode_next_active(struct vnode **m
 	if (should_yield())
 		kern_yield(PRI_UNCHANGED);
 	MNT_ILOCK(mp);
+restart:
+	mtx_lock(&vnode_free_list_mtx);
 	KASSERT((*mvp)->v_mount == mp, ("marker vnode mount list mismatch"));
 	vp = TAILQ_NEXT(*mvp, v_actfreelist);
 	while (vp != NULL) {
-		VI_LOCK(vp);
+		if (vp->v_type == VMARKER) {
+			vp = TAILQ_NEXT(vp, v_actfreelist);
+			continue;
+		}
+		if (!VI_TRYLOCK(vp)) {
+			mtx_unlock(&vnode_free_list_mtx);
+			kern_yield(PRI_UNCHANGED);
+			goto restart;
+		}
 		if (vp->v_mount == mp && vp->v_type != VMARKER &&
 		    (vp->v_iflag & VI_DOOMED) == 0)
 			break;
@@ -4732,16 +4742,18 @@ __mnt_vnode_next_active(struct vnode **m
 
 	/* Check if we are done */
 	if (vp == NULL) {
+		mtx_unlock(&vnode_free_list_mtx);
 		__mnt_vnode_markerfree_active(mvp, mp);
 		/* MNT_IUNLOCK(mp); -- done in above function */
 		mtx_assert(MNT_MTX(mp), MA_NOTOWNED);
 		return (NULL);
 	}
-	mtx_lock(&vnode_free_list_mtx);
 	TAILQ_REMOVE(&mp->mnt_activevnodelist, *mvp, v_actfreelist);
 	TAILQ_INSERT_AFTER(&mp->mnt_activevnodelist, vp, *mvp, v_actfreelist);
 	mtx_unlock(&vnode_free_list_mtx);
 	MNT_IUNLOCK(mp);
+	ASSERT_VI_LOCKED(vp, "active iter");
+	KASSERT((vp->v_iflag & VI_ACTIVE) != 0, ("Non-active vp %p", vp));
 	return (vp);
 }
 
@@ -4755,9 +4767,19 @@ __mnt_vnode_first_active(struct vnode **
 	MNT_REF(mp);
 	(*mvp)->v_type = VMARKER;
 
+restart:
+	mtx_lock(&vnode_free_list_mtx);
 	vp = TAILQ_FIRST(&mp->mnt_activevnodelist);
 	while (vp != NULL) {
-		VI_LOCK(vp);
+		if (vp->v_type == VMARKER) {
+			vp = TAILQ_NEXT(vp, v_actfreelist);
+			continue;
+		}
+		if (!VI_TRYLOCK(vp)) {
+			mtx_unlock(&vnode_free_list_mtx);
+			kern_yield(PRI_UNCHANGED);
+			goto restart;
+		}
 		if (vp->v_mount == mp && vp->v_type != VMARKER &&
 		    (vp->v_iflag & VI_DOOMED) == 0)
 			break;
@@ -4768,6 +4790,7 @@ __mnt_vnode_first_active(struct vnode **
 
 	/* Check if we are done */
 	if (vp == NULL) {
+		mtx_unlock(&vnode_free_list_mtx);
 		MNT_REL(mp);
 		MNT_IUNLOCK(mp);
 		free(*mvp, M_VNODE_MARKER);
@@ -4775,10 +4798,11 @@ __mnt_vnode_first_active(struct vnode **
 		return (NULL);
 	}
 	(*mvp)->v_mount = mp;
-	mtx_lock(&vnode_free_list_mtx);
 	TAILQ_INSERT_AFTER(&mp->mnt_activevnodelist, vp, *mvp, v_actfreelist);
 	mtx_unlock(&vnode_free_list_mtx);
 	MNT_IUNLOCK(mp);
+	ASSERT_VI_LOCKED(vp, "active iter first");
+	KASSERT((vp->v_iflag & VI_ACTIVE) != 0, ("Non-active vp %p", vp));
 	return (vp);
 }

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