Date: Thu, 3 Jun 2010 17:35:01 +0300 From: Jaakko Heinonen <jh@FreeBSD.org> To: freebsd-fs@FreeBSD.org Cc: attilio@FreeBSD.org, kib@FreeBSD.org Subject: syncer vnode leak because of nmount() race Message-ID: <20100603143501.GA3176@a91-153-117-195.elisa-laajakaista.fi>
next in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100603143501.GA3176>