From owner-freebsd-fs@FreeBSD.ORG Thu Jun 3 14:35:07 2010 Return-Path: Delivered-To: freebsd-fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 747B31065678; Thu, 3 Jun 2010 14:35:07 +0000 (UTC) (envelope-from jh@FreeBSD.org) Received: from gw03.mail.saunalahti.fi (gw03.mail.saunalahti.fi [195.197.172.111]) by mx1.freebsd.org (Postfix) with ESMTP id E37F38FC14; Thu, 3 Jun 2010 14:35:06 +0000 (UTC) Received: from a91-153-117-195.elisa-laajakaista.fi (a91-153-117-195.elisa-laajakaista.fi [91.153.117.195]) by gw03.mail.saunalahti.fi (Postfix) with SMTP id 1DDE62164E9; Thu, 3 Jun 2010 17:35:02 +0300 (EEST) Date: Thu, 3 Jun 2010 17:35:01 +0300 From: Jaakko Heinonen To: freebsd-fs@FreeBSD.org Message-ID: <20100603143501.GA3176@a91-153-117-195.elisa-laajakaista.fi> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.20 (2009-06-14) Cc: attilio@FreeBSD.org, kib@FreeBSD.org Subject: syncer vnode leak because of nmount() race X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Jun 2010 14:35:07 -0000 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