syncer vnode leak because of nmount() race

[Jaakko Heinonen]

Hi,

I have been playing with devfs and stress2 recently and I was able to
trigger a syncer vnode leak with devfs.sh. As far as I can tell it is a
nmount(2) (initial) mount race against update mount. The code in
vfs_domount() is protected with vfs_busy()/vfs_unbusy() but it doesn't
protect against update mounts. The protection by Giant is defeated
because devfs_root() may sleep due to sx_xlock(). vfs_domount() calls
VFS_ROOT() before allocating the syncer vnode. VI_MOUNT vnode flag
doesn't provide sufficient protection against update mounts either.

I created following patch to work around the problem. I am unsure if the
approach is good however.

%%%
Index: sys/kern/vfs_mount.c
===================================================================
--- sys/kern/vfs_mount.c	(revision 208667)
+++ sys/kern/vfs_mount.c	(working copy)
@@ -906,6 +906,16 @@ vfs_domount(
 			vput(vp);
 			return (EBUSY);
 		}
+		if (mp->mnt_vnodecovered != NULL) {
+			VI_LOCK(mp->mnt_vnodecovered);
+			if ((mp->mnt_vnodecovered->v_iflag & VI_MOUNT) != 0) {
+				VI_UNLOCK(mp->mnt_vnodecovered);
+				vfs_unbusy(mp);
+				vput(vp);
+				return (EBUSY);
+			}
+			VI_UNLOCK(mp->mnt_vnodecovered);
+		}
 		VI_LOCK(vp);
 		if ((vp->v_iflag & VI_MOUNT) != 0 ||
 		    vp->v_mountedhere != NULL) {
@@ -1060,17 +1070,10 @@ vfs_domount(
 	 * Put the new filesystem on the mount list after root.
 	 */
 	cache_purge(vp);
-	VI_LOCK(vp);
-	vp->v_iflag &= ~VI_MOUNT;
-	VI_UNLOCK(vp);
 	if (!error) {
 		struct vnode *newdp;
 
 		vp->v_mountedhere = mp;
-		mtx_lock(&mountlist_mtx);
-		TAILQ_INSERT_TAIL(&mountlist, mp, mnt_list);
-		mtx_unlock(&mountlist_mtx);
-		vfs_event_signal(NULL, VQ_MOUNT, 0);
 		if (VFS_ROOT(mp, LK_EXCLUSIVE, &newdp))
 			panic("mount: lost mount");
 		VOP_UNLOCK(newdp, 0);
@@ -1079,10 +1082,20 @@ vfs_domount(
 		vrele(newdp);
 		if ((mp->mnt_flag & MNT_RDONLY) == 0)
 			error = vfs_allocate_syncvnode(mp);
+		VI_LOCK(vp);
+		vp->v_iflag &= ~VI_MOUNT;
+		VI_UNLOCK(vp);
+		mtx_lock(&mountlist_mtx);
+		TAILQ_INSERT_TAIL(&mountlist, mp, mnt_list);
+		mtx_unlock(&mountlist_mtx);
+		vfs_event_signal(NULL, VQ_MOUNT, 0);
 		vfs_unbusy(mp);
 		if (error)
 			vrele(vp);
 	} else {
+		VI_LOCK(vp);
+		vp->v_iflag &= ~VI_MOUNT;
+		VI_UNLOCK(vp);
 		vfs_unbusy(mp);
 		vfs_mount_destroy(mp);
 		vput(vp);
%%%

Comments?

PS. vfs_unbusy(9) manual page is out of date after r184554 and IMO
    vfs_busy(9) manual page is misleading because it talks about
    synchronizing access to a mount point.

-- 
Jaakko