git: 8df4bc48c89a - main - ufs rename: ensure that the result of ufs_checkpath() is stable

[Konstantin Belousov]

The branch main has been updated by kib:

URL: https://cgit.FreeBSD.org/src/commit/?id=8df4bc48c89a1302078282f22139a8368dc06971

commit 8df4bc48c89a1302078282f22139a8368dc06971
Author:     Konstantin Belousov <kib at FreeBSD.org>
AuthorDate: 2021-08-06 01:03:19 +0000
Commit:     Konstantin Belousov <kib at FreeBSD.org>
CommitDate: 2021-08-13 14:52:26 +0000

    ufs rename: ensure that the result of ufs_checkpath() is stable
    
    ufs_rename() calls ufs_checkpath() to ensure that the target directory
    is not a child of the source.  If not, rename would create a loop.
    For instance:
            source->X1->X2->target
    and if source moved under target, we get corrupted filesystem.
    Suppose that we initially have
            source->X1 .... and X2->target
    where X1 is not on path from root to X2.  Then ufs_checkpath() accepts
    the inodes, but there is nothing preventing parallel rename of X2 to become
    under X1, after checkpath finished.
    
    Ensure stability of ufs_checkpath() result by taking a per-mount sx in
    ufs_rename right before ufs_checkpath() and till the end.
    
    Reviewed by:    chs, mckusick
    Tested by:      pho
    Sponsored by:   The FreeBSD Foundation
    MFC after:      2 weeks
---
 sys/ufs/ffs/ffs_vfsops.c |  5 ++++-
 sys/ufs/ufs/ufs_lookup.c |  2 ++
 sys/ufs/ufs/ufs_vnops.c  | 12 ++++++++++--
 sys/ufs/ufs/ufsmount.h   |  3 +++
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/sys/ufs/ffs/ffs_vfsops.c b/sys/ufs/ffs/ffs_vfsops.c
index 2ff71cc3e4d1..aad7b4f2decd 100644
--- a/sys/ufs/ffs/ffs_vfsops.c
+++ b/sys/ufs/ffs/ffs_vfsops.c
@@ -1151,6 +1151,7 @@ ffs_mountfs(odevvp, mp, td)
 	else
 		ump->um_check_blkno = NULL;
 	mtx_init(UFS_MTX(ump), "FFS", "FFS Lock", MTX_DEF);
+	sx_init(&ump->um_checkpath_lock, "uchpth");
 	ffs_oldfscompat_read(fs, ump, fs->fs_sblockloc);
 	fs->fs_ronly = ronly;
 	fs->fs_active = NULL;
@@ -1318,8 +1319,9 @@ out:
 		g_vfs_close(cp);
 		g_topology_unlock();
 	}
-	if (ump) {
+	if (ump != NULL) {
 		mtx_destroy(UFS_MTX(ump));
+		sx_destroy(&ump->um_checkpath_lock);
 		if (mp->mnt_gjprovider != NULL) {
 			free(mp->mnt_gjprovider, M_UFSMNT);
 			mp->mnt_gjprovider = NULL;
@@ -1545,6 +1547,7 @@ ffs_unmount(mp, mntflags)
 	vrele(ump->um_odevvp);
 	dev_rel(ump->um_dev);
 	mtx_destroy(UFS_MTX(ump));
+	sx_destroy(&ump->um_checkpath_lock);
 	if (mp->mnt_gjprovider != NULL) {
 		free(mp->mnt_gjprovider, M_UFSMNT);
 		mp->mnt_gjprovider = NULL;
diff --git a/sys/ufs/ufs/ufs_lookup.c b/sys/ufs/ufs/ufs_lookup.c
index ac3a8ee641a0..fc78c017e2c6 100644
--- a/sys/ufs/ufs/ufs_lookup.c
+++ b/sys/ufs/ufs/ufs_lookup.c
@@ -1450,6 +1450,8 @@ ufs_checkpath(ino_t source_ino, ino_t parent_ino, struct inode *target,
 	vp = tvp = ITOV(target);
 	mp = vp->v_mount;
 	*wait_ino = 0;
+	sx_assert(&VFSTOUFS(mp)->um_checkpath_lock, SA_XLOCKED);
+
 	if (target->i_number == source_ino)
 		return (EEXIST);
 	if (target->i_number == parent_ino)
diff --git a/sys/ufs/ufs/ufs_vnops.c b/sys/ufs/ufs/ufs_vnops.c
index 2dfc2e24f772..00ec8f41f432 100644
--- a/sys/ufs/ufs/ufs_vnops.c
+++ b/sys/ufs/ufs/ufs_vnops.c
@@ -1249,9 +1249,9 @@ ufs_rename(ap)
 	struct mount *mp;
 	ino_t ino;
 	seqc_t fdvp_s, fvp_s, tdvp_s, tvp_s;
-	bool want_seqc_end;
+	bool checkpath_locked, want_seqc_end;
 
-	want_seqc_end = false;
+	checkpath_locked = want_seqc_end = false;
 
 #ifdef INVARIANTS
 	if ((tcnp->cn_flags & HASBUF) == 0 ||
@@ -1453,6 +1453,9 @@ relock:
 		error = VOP_ACCESS(fvp, VWRITE, tcnp->cn_cred, tcnp->cn_thread);
 		if (error)
 			goto unlockout;
+
+		sx_xlock(&VFSTOUFS(mp)->um_checkpath_lock);
+		checkpath_locked = true;
 		error = ufs_checkpath(ino, fdp->i_number, tdp, tcnp->cn_cred,
 		    &ino);
 		/*
@@ -1460,6 +1463,8 @@ relock:
 		 * everything else and VGET before restarting.
 		 */
 		if (ino) {
+			sx_xunlock(&VFSTOUFS(mp)->um_checkpath_lock);
+			checkpath_locked = false;
 			VOP_UNLOCK(fdvp);
 			VOP_UNLOCK(fvp);
 			VOP_UNLOCK(tdvp);
@@ -1690,6 +1695,9 @@ unlockout:
 		vn_seqc_write_end(fdvp);
 	}
 
+	if (checkpath_locked)
+		sx_xunlock(&VFSTOUFS(mp)->um_checkpath_lock);
+
 	vput(fdvp);
 	vput(fvp);
 
diff --git a/sys/ufs/ufs/ufsmount.h b/sys/ufs/ufs/ufsmount.h
index f2951d74d44c..da9a22127125 100644
--- a/sys/ufs/ufs/ufsmount.h
+++ b/sys/ufs/ufs/ufsmount.h
@@ -70,6 +70,7 @@ LIST_HEAD(trimlist_hashhead, ffs_blkfree_trim_params);
 
 #include <sys/_lock.h>
 #include <sys/_mutex.h>
+#include <sys/_sx.h>
 
 /*
  * This structure describes the UFS specific mount structure data.
@@ -99,6 +100,8 @@ struct ufsmount {
 	uint64_t um_maxsymlinklen;		/* (c) max size of short
 						       symlink */
 	struct	mtx um_lock;			/* (c) Protects ufsmount & fs */
+	struct	sx um_checkpath_lock;		/* (c) Protects ufs_checkpath()
+						       result */
 	pid_t	um_fsckpid;			/* (u) PID can do fsck sysctl */
 	struct	mount_softdeps *um_softdep;	/* (c) softdep mgmt structure */
 	struct	vnode *um_quotas[MAXQUOTAS];	/* (q) pointer to quota files */