Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 6 Jun 2021 01:18:22 GMT
From:      "Jason A. Harmening" <jah@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org
Subject:   git: 59409cb90fc0 - main - Add a generic mechanism for preventing forced unmount
Message-ID:  <202106060118.1561IMDK098547@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch main has been updated by jah:

URL: https://cgit.FreeBSD.org/src/commit/?id=59409cb90fc079bd388d8f7404679193e4d34889

commit 59409cb90fc079bd388d8f7404679193e4d34889
Author:     Jason A. Harmening <jah@FreeBSD.org>
AuthorDate: 2021-05-17 22:47:27 +0000
Commit:     Jason A. Harmening <jah@FreeBSD.org>
CommitDate: 2021-06-06 01:20:36 +0000

    Add a generic mechanism for preventing forced unmount
    
    This is aimed at preventing stacked filesystems like nullfs and unionfs
    from "losing" their lower mounts due to forced unmount.  Otherwise,
    VFS operations that are passed through to the lower filesystem(s) may
    crash or otherwise cause unpredictable behavior.
    
    Introduce two new functions: vfs_pin_from_vp() and vfs_unpin().
    which are intended to be called on the lower mount(s) when the stacked
    filesystem is mounted and unmounted, respectively.
    Much as registration in the mnt_uppers list previously did, pinning
    will prevent even forced unmount of the lower FS and will allow the
    stacked FS to freely operate on the lower mount either by direct
    use of the struct mount* or indirect use through a properly-referenced
    vnode's v_mount field.
    
    vfs_pin_from_vp() is modeled after vfs_ref_from_vp() in that it uses
    the mount interlock coupled with re-checking vp->v_mount to ensure
    that it will fail in the face of a pending unmount request, even if
    the concurrent unmount fully completes.
    
    Adopt these new functions in both nullfs and unionfs.
    
    Reviewed By:    kib, markj
    Differential Revision: https://reviews.freebsd.org/D30401
---
 sys/fs/nullfs/null_vfsops.c   |  9 ++++++++-
 sys/fs/unionfs/union_vfsops.c | 22 +++++++++++++++++++---
 sys/kern/vfs_mount.c          | 39 ++++++++++++++++++++++++++++++++++++++-
 sys/kern/vfs_subr.c           |  3 +++
 sys/sys/mount.h               |  3 +++
 5 files changed, 71 insertions(+), 5 deletions(-)

diff --git a/sys/fs/nullfs/null_vfsops.c b/sys/fs/nullfs/null_vfsops.c
index 0ad2385116a9..4914e5fc2dbf 100644
--- a/sys/fs/nullfs/null_vfsops.c
+++ b/sys/fs/nullfs/null_vfsops.c
@@ -163,7 +163,12 @@ nullfs_mount(struct mount *mp)
 	 * Save pointer to underlying FS and the reference to the
 	 * lower root vnode.
 	 */
-	xmp->nullm_vfs = lowerrootvp->v_mount;
+	xmp->nullm_vfs = vfs_pin_from_vp(lowerrootvp);
+	if (xmp->nullm_vfs == NULL) {
+		vput(lowerrootvp);
+		free(xmp, M_NULLFSMNT);
+		return (ENOENT);
+	}
 	vref(lowerrootvp);
 	xmp->nullm_lowerrootvp = lowerrootvp;
 	mp->mnt_data = xmp;
@@ -173,6 +178,7 @@ nullfs_mount(struct mount *mp)
 	 */
 	error = null_nodeget(mp, lowerrootvp, &nullm_rootvp);
 	if (error != 0) {
+		vfs_unpin(xmp->nullm_vfs);
 		vrele(lowerrootvp);
 		free(xmp, M_NULLFSMNT);
 		return (error);
@@ -263,6 +269,7 @@ nullfs_unmount(mp, mntflags)
 		TAILQ_REMOVE(&ump->mnt_uppers, mp, mnt_upper_link);
 		MNT_IUNLOCK(ump);
 	}
+	vfs_unpin(ump);
 	vrele(mntdata->nullm_lowerrootvp);
 	mp->mnt_data = NULL;
 	free(mntdata, M_NULLFSMNT);
diff --git a/sys/fs/unionfs/union_vfsops.c b/sys/fs/unionfs/union_vfsops.c
index bd264c7bcdb5..96a30f0ae8b5 100644
--- a/sys/fs/unionfs/union_vfsops.c
+++ b/sys/fs/unionfs/union_vfsops.c
@@ -75,6 +75,7 @@ static int
 unionfs_domount(struct mount *mp)
 {
 	int		error;
+	struct mount   *lowermp, *uppermp;
 	struct vnode   *lowerrootvp;
 	struct vnode   *upperrootvp;
 	struct unionfs_mount *ump;
@@ -285,15 +286,28 @@ unionfs_domount(struct mount *mp)
 	error = unionfs_nodeget(mp, ump->um_uppervp, ump->um_lowervp,
 	    NULLVP, &(ump->um_rootvp), NULL, td);
 	vrele(upperrootvp);
-	if (error) {
+	if (error != 0) {
 		free(ump, M_UNIONFSMNT);
 		mp->mnt_data = NULL;
 		return (error);
 	}
 
+	lowermp = vfs_pin_from_vp(ump->um_lowervp);
+	uppermp = vfs_pin_from_vp(ump->um_uppervp);
+
+	if (lowermp == NULL || uppermp == NULL) {
+		if (lowermp != NULL)
+			vfs_unpin(lowermp);
+		if (uppermp != NULL)
+			vfs_unpin(uppermp);
+		free(ump, M_UNIONFSMNT);
+		mp->mnt_data = NULL;
+		return (ENOENT);
+	}
+
 	MNT_ILOCK(mp);
-	if ((ump->um_lowervp->v_mount->mnt_flag & MNT_LOCAL) &&
-	    (ump->um_uppervp->v_mount->mnt_flag & MNT_LOCAL))
+	if ((lowermp->mnt_flag & MNT_LOCAL) != 0 &&
+	    (uppermp->mnt_flag & MNT_LOCAL) != 0)
 		mp->mnt_flag |= MNT_LOCAL;
 	mp->mnt_kern_flag |= MNTK_NOMSYNC | MNTK_UNIONFS;
 	MNT_IUNLOCK(mp);
@@ -343,6 +357,8 @@ unionfs_unmount(struct mount *mp, int mntflags)
 	if (error)
 		return (error);
 
+	vfs_unpin(ump->um_lowervp->v_mount);
+	vfs_unpin(ump->um_uppervp->v_mount);
 	free(ump, M_UNIONFSMNT);
 	mp->mnt_data = NULL;
 
diff --git a/sys/kern/vfs_mount.c b/sys/kern/vfs_mount.c
index 7dc6b795eefd..8b4426209818 100644
--- a/sys/kern/vfs_mount.c
+++ b/sys/kern/vfs_mount.c
@@ -49,6 +49,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/kernel.h>
 #include <sys/ktr.h>
 #include <sys/libkern.h>
+#include <sys/limits.h>
 #include <sys/malloc.h>
 #include <sys/mount.h>
 #include <sys/mutex.h>
@@ -504,6 +505,39 @@ vfs_ref(struct mount *mp)
 	MNT_IUNLOCK(mp);
 }
 
+struct mount *
+vfs_pin_from_vp(struct vnode *vp)
+{
+	struct mount *mp;
+
+	mp = atomic_load_ptr(&vp->v_mount);
+	if (mp == NULL)
+		return (NULL);
+	MNT_ILOCK(mp);
+	if (mp != vp->v_mount || (mp->mnt_kern_flag & MNTK_UNMOUNT) != 0) {
+		MNT_IUNLOCK(mp);
+		return (NULL);
+	}
+	MNT_REF(mp);
+	KASSERT(mp->mnt_pinned_count < INT_MAX,
+	    ("mount pinned count overflow"));
+	++mp->mnt_pinned_count;
+	MNT_IUNLOCK(mp);
+	return (mp);
+}
+
+void
+vfs_unpin(struct mount *mp)
+{
+	MNT_ILOCK(mp);
+	KASSERT(mp->mnt_pinned_count > 0, ("mount pinned count underflow"));
+	KASSERT((mp->mnt_kern_flag & MNTK_UNMOUNT) == 0,
+	    ("mount pinned with pending unmount"));
+	--mp->mnt_pinned_count;
+	MNT_REL(mp);
+	MNT_IUNLOCK(mp);
+}
+
 void
 vfs_rel(struct mount *mp)
 {
@@ -567,6 +601,7 @@ vfs_mount_alloc(struct vnode *vp, struct vfsconf *vfsp, const char *fspath,
 #endif
 	arc4rand(&mp->mnt_hashseed, sizeof mp->mnt_hashseed, 0);
 	TAILQ_INIT(&mp->mnt_uppers);
+	mp->mnt_pinned_count = 0;
 	return (mp);
 }
 
@@ -605,6 +640,8 @@ vfs_mount_destroy(struct mount *mp)
 			vn_printf(vp, "dangling vnode ");
 		panic("unmount: dangling vnode");
 	}
+	KASSERT(mp->mnt_pinned_count == 0,
+	   ("mnt_pinned_count = %d", mp->mnt_pinned_count));
 	KASSERT(TAILQ_EMPTY(&mp->mnt_uppers), ("mnt_uppers"));
 	if (mp->mnt_nvnodelistsize != 0)
 		panic("vfs_mount_destroy: nonzero nvnodelistsize");
@@ -1811,7 +1848,7 @@ dounmount(struct mount *mp, int flags, struct thread *td)
 	MNT_ILOCK(mp);
 	if ((mp->mnt_kern_flag & MNTK_UNMOUNT) != 0 ||
 	    (mp->mnt_flag & MNT_UPDATE) != 0 ||
-	    !TAILQ_EMPTY(&mp->mnt_uppers)) {
+	    mp->mnt_pinned_count != 0) {
 		dounmount_cleanup(mp, coveredvp, 0);
 		return (EBUSY);
 	}
diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c
index ad0f1c2a0c7c..a2f25bf78495 100644
--- a/sys/kern/vfs_subr.c
+++ b/sys/kern/vfs_subr.c
@@ -831,6 +831,9 @@ vfs_busy(struct mount *mp, int flags)
 	 * valid.
 	 */
 	while (mp->mnt_kern_flag & MNTK_UNMOUNT) {
+		KASSERT(mp->mnt_pinned_count == 0,
+		    ("%s: non-zero pinned count %d with pending unmount",
+		    __func__, mp->mnt_pinned_count));
 		if (flags & MBF_NOWAIT || mp->mnt_kern_flag & MNTK_REFEXPIRE) {
 			MNT_REL(mp);
 			MNT_IUNLOCK(mp);
diff --git a/sys/sys/mount.h b/sys/sys/mount.h
index 6b2e6e7f7f13..693293b12370 100644
--- a/sys/sys/mount.h
+++ b/sys/sys/mount.h
@@ -242,6 +242,7 @@ struct mount {
 	struct mtx	mnt_listmtx;
 	struct vnodelst	mnt_lazyvnodelist;	/* (l) list of lazy vnodes */
 	int		mnt_lazyvnodelistsize;	/* (l) # of lazy vnodes */
+	int		mnt_pinned_count;	/* (i) unmount prevented */
 	struct lock	mnt_explock;		/* vfs_export walkers lock */
 	TAILQ_ENTRY(mount) mnt_upper_link;	/* (i*) we in the all uppers */
 	TAILQ_HEAD(, mount) mnt_uppers;		/* (i) upper mounts over us */
@@ -1011,6 +1012,8 @@ struct mount *vfs_mount_alloc(struct vnode *, struct vfsconf *, const char *,
 int	vfs_suser(struct mount *, struct thread *);
 void	vfs_unbusy(struct mount *);
 void	vfs_unmountall(void);
+struct mount *vfs_pin_from_vp(struct vnode *);
+void	vfs_unpin(struct mount *);
 extern	TAILQ_HEAD(mntlist, mount) mountlist;	/* mounted filesystem list */
 extern	struct mtx_padalign mountlist_mtx;
 extern	struct nfs_public nfs_pub;



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