Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 3 Jan 2022 02:51:49 GMT
From:      Alan Somers <asomers@FreeBSD.org>
To:        src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org
Subject:   git: 139764c4613c - stable/13 - fusefs: correctly handle an inode that changes file types
Message-ID:  <202201030251.2032pnMT082105@gitrepo.freebsd.org>

next in thread | raw e-mail | index | archive | help
The branch stable/13 has been updated by asomers:

URL: https://cgit.FreeBSD.org/src/commit/?id=139764c4613cde14c97ff8dd8007eb0f27f5fb9f

commit 139764c4613cde14c97ff8dd8007eb0f27f5fb9f
Author:     Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2021-12-06 05:43:17 +0000
Commit:     Alan Somers <asomers@FreeBSD.org>
CommitDate: 2022-01-03 02:36:38 +0000

    fusefs: correctly handle an inode that changes file types
    
    Correctly handle the situation where a FUSE server unlinks a file, then
    creates a new file of a different type but with the same inode number.
    Previously fuse_vnop_lookup in this situation would return EAGAIN.  But
    since it didn't call vgone(), the vnode couldn't be reused right away.
    Fix this by immediately calling vgone() and reallocating a new vnode.
    
    This problem can occur in three code paths, during VOP_LOOKUP,
    VOP_SETATTR, or following FUSE_GETATTR, which usually happens during
    VOP_GETATTR but can occur during other vops, too.  Note that the correct
    response actually doesn't depend on whether the entry cache has expired.
    In fact, during VOP_LOOKUP, we can't even tell.  Either it has expired
    already, or else the vnode got reclaimed by vnlru.
    
    Also, correct the error code during the VOP_SETATTR path.
    
    PR:             258022
    Reported by:    chogata@moosefs.pro
    Reviewed by:    pfg
    Differential Revision: https://reviews.freebsd.org/D33283
    
    (cherry picked from commit 25927e068fcbcac0a5111a881de723bd984b04b3)
---
 sys/fs/fuse/fuse_internal.c    |  9 +++++---
 sys/fs/fuse/fuse_node.c        | 25 +++++++++++----------
 tests/sys/fs/fusefs/getattr.cc | 50 ++++++++++++++++++++++++++++++++++++++++++
 tests/sys/fs/fusefs/lookup.cc  | 32 +++++++++++++++++++++------
 tests/sys/fs/fusefs/setattr.cc | 47 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 142 insertions(+), 21 deletions(-)

diff --git a/sys/fs/fuse/fuse_internal.c b/sys/fs/fuse/fuse_internal.c
index 403116593bd9..b9df64921fc8 100644
--- a/sys/fs/fuse/fuse_internal.c
+++ b/sys/fs/fuse/fuse_internal.c
@@ -1256,12 +1256,15 @@ int fuse_internal_setattr(struct vnode *vp, struct vattr *vap,
 	                 * STALE vnode, ditch
 	                 *
 			 * The vnode has changed its type "behind our back".
+			 * This probably means that the file got deleted and
+			 * recreated on the server, with the same inode.
 			 * There's nothing really we can do, so let us just
-			 * force an internal revocation and tell the caller to
-			 * try again, if interested.
+			 * return ENOENT.  After all, the entry must not have
+			 * existed in the recent past.  If the user tries
+			 * again, it will work.
 	                 */
 			fuse_internal_vnode_disappear(vp);
-			err = EAGAIN;
+			err = ENOENT;
 		}
 	}
 	if (err == 0) {
diff --git a/sys/fs/fuse/fuse_node.c b/sys/fs/fuse/fuse_node.c
index f74de2faa702..b5f46daf025d 100644
--- a/sys/fs/fuse/fuse_node.c
+++ b/sys/fs/fuse/fuse_node.c
@@ -212,24 +212,27 @@ fuse_vnode_alloc(struct mount *mp,
 		return (err);
 
 	if (*vpp) {
-		if ((*vpp)->v_type != vtyp) {
+		if ((*vpp)->v_type == vtyp) {
+			/* Reuse a vnode that hasn't yet been reclaimed */
+			MPASS((*vpp)->v_data != NULL);
+			MPASS(VTOFUD(*vpp)->nid == nodeid);
+			SDT_PROBE2(fusefs, , node, trace, 1,
+				"vnode taken from hash");
+			return (0);
+		} else {
 			/*
-			 * STALE vnode!  This probably indicates a buggy
-			 * server, but it could also be the result of a race
-			 * between FUSE_LOOKUP and another client's
-			 * FUSE_UNLINK/FUSE_CREATE
+			 * The inode changed types!  If we get here, we can't
+			 * tell whether the inode's entry cache had expired
+			 * yet.  So this could be the result of a buggy server,
+			 * but more likely the server just reused an inode
+			 * number following an entry cache expiration.
 			 */
 			SDT_PROBE3(fusefs, , node, stale_vnode, *vpp, vtyp,
 				nodeid);
 			fuse_internal_vnode_disappear(*vpp);
+			vgone(*vpp);
 			lockmgr((*vpp)->v_vnlock, LK_RELEASE, NULL);
-			*vpp = NULL;
-			return (EAGAIN);
 		}
-		MPASS((*vpp)->v_data != NULL);
-		MPASS(VTOFUD(*vpp)->nid == nodeid);
-		SDT_PROBE2(fusefs, , node, trace, 1, "vnode taken from hash");
-		return (0);
 	}
 	fvdat = malloc(sizeof(*fvdat), M_FUSEVN, M_WAITOK | M_ZERO);
 	switch (vtyp) {
diff --git a/tests/sys/fs/fusefs/getattr.cc b/tests/sys/fs/fusefs/getattr.cc
index fb91f8c049d0..6bca7e0af7c7 100644
--- a/tests/sys/fs/fusefs/getattr.cc
+++ b/tests/sys/fs/fusefs/getattr.cc
@@ -256,6 +256,56 @@ TEST_F(Getattr, ok)
 	//FUSE can't set st_blksize until protocol 7.9
 }
 
+/*
+ * FUSE_GETATTR returns a different file type, even though the entry cache
+ * hasn't expired.  This is a server bug!  It probably means that the server
+ * removed the file and recreated it with the same inode but a different vtyp.
+ * The best thing fusefs can do is return ENOENT to the caller.  After all, the
+ * entry must not have existed recently.
+ */
+TEST_F(Getattr, vtyp_conflict)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const uint64_t ino = 42;
+	struct stat sb;
+	sem_t sem;
+
+	ASSERT_EQ(0, sem_init(&sem, 0, 0)) << strerror(errno);
+
+	EXPECT_LOOKUP(FUSE_ROOT_ID, RELPATH)
+	.WillOnce(Invoke(
+		ReturnImmediate([=](auto in __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, entry);
+		out.body.entry.attr.mode = S_IFREG | 0644;
+		out.body.entry.nodeid = ino;
+		out.body.entry.attr.nlink = 1;
+		out.body.entry.attr_valid = 0;
+		out.body.entry.entry_valid = UINT64_MAX;
+	})));
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([](auto in) {
+			return (in.header.opcode == FUSE_GETATTR &&
+				in.body.getattr.getattr_flags == 0 &&
+				in.header.nodeid == ino);
+		}, Eq(true)),
+		_)
+	).WillOnce(Invoke(ReturnImmediate([](auto i __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, attr);
+		out.body.attr.attr.ino = ino;	// Must match nodeid
+		out.body.attr.attr.mode = S_IFDIR | 0755;	// Changed!
+		out.body.attr.attr.nlink = 2;
+	})));
+	// We should reclaim stale vnodes
+	expect_forget(ino, 1, &sem);
+
+	ASSERT_NE(0, stat(FULLPATH, &sb));
+	EXPECT_EQ(errno, ENOENT);
+
+	sem_wait(&sem);
+	sem_destroy(&sem);
+}
+
 TEST_F(Getattr_7_8, ok)
 {
 	const char FULLPATH[] = "mountpoint/some_file.txt";
diff --git a/tests/sys/fs/fusefs/lookup.cc b/tests/sys/fs/fusefs/lookup.cc
index cb9d0bb27527..d301990c2048 100644
--- a/tests/sys/fs/fusefs/lookup.cc
+++ b/tests/sys/fs/fusefs/lookup.cc
@@ -342,9 +342,10 @@ TEST_F(Lookup, subdir)
 	ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno);
 }
 
-/* 
- * The server returns two different vtypes for the same nodeid.  This is a bad
- * server!  But we shouldn't crash.
+/*
+ * The server returns two different vtypes for the same nodeid.  This is
+ * technically allowed if the entry's cache has already expired.
+ * https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=258022
  */
 TEST_F(Lookup, vtype_conflict)
 {
@@ -354,12 +355,29 @@ TEST_F(Lookup, vtype_conflict)
 	const char SECONDRELPATH[] = "bar";
 	uint64_t ino = 42;
 
-	expect_lookup(FIRSTRELPATH, ino, S_IFREG | 0644, 0, 1, UINT64_MAX);
-	expect_lookup(SECONDRELPATH, ino, S_IFDIR | 0755, 0, 1, UINT64_MAX);
+	EXPECT_LOOKUP(FUSE_ROOT_ID, FIRSTRELPATH)
+	.WillOnce(Invoke(
+		ReturnImmediate([=](auto in __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, entry);
+		out.body.entry.attr.mode = S_IFDIR | 0644;
+		out.body.entry.nodeid = ino;
+		out.body.entry.attr.nlink = 1;
+	})));
+	expect_lookup(SECONDRELPATH, ino, S_IFREG | 0755, 0, 1, UINT64_MAX);
+	// VOP_FORGET happens asynchronously, so it may or may not arrive
+	// before the test completes.
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([=](auto in) {
+			return (in.header.opcode == FUSE_FORGET &&
+				in.header.nodeid == ino &&
+				in.body.forget.nlookup == 1);
+		}, Eq(true)),
+		_)
+	).Times(AtMost(1))
+	.WillOnce(Invoke([=](auto in __unused, auto &out __unused) { }));
 
 	ASSERT_EQ(0, access(FIRSTFULLPATH, F_OK)) << strerror(errno);
-	ASSERT_EQ(-1, access(SECONDFULLPATH, F_OK));
-	ASSERT_EQ(EAGAIN, errno);
+	EXPECT_EQ(0, access(SECONDFULLPATH, F_OK)) << strerror(errno);
 }
 
 TEST_F(Lookup_7_8, ok)
diff --git a/tests/sys/fs/fusefs/setattr.cc b/tests/sys/fs/fusefs/setattr.cc
index 48aa8385517f..e4458db9f8ee 100644
--- a/tests/sys/fs/fusefs/setattr.cc
+++ b/tests/sys/fs/fusefs/setattr.cc
@@ -724,6 +724,53 @@ TEST_F(Setattr, utimensat_utime_now) {
 	EXPECT_EQ(now[1].tv_nsec, sb.st_mtim.tv_nsec);
 }
 
+/*
+ * FUSE_SETATTR returns a different file type, even though the entry cache
+ * hasn't expired.  This is a server bug!  It probably means that the server
+ * removed the file and recreated it with the same inode but a different vtyp.
+ * The best thing fusefs can do is return ENOENT to the caller.  After all, the
+ * entry must not have existed recently.
+ */
+TEST_F(Setattr, vtyp_conflict)
+{
+	const char FULLPATH[] = "mountpoint/some_file.txt";
+	const char RELPATH[] = "some_file.txt";
+	const uint64_t ino = 42;
+	uid_t newuser = 12345;
+	sem_t sem;
+
+	ASSERT_EQ(0, sem_init(&sem, 0, 0)) << strerror(errno);
+
+	EXPECT_LOOKUP(FUSE_ROOT_ID, RELPATH)
+	.WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, entry);
+		out.body.entry.attr.mode = S_IFREG | 0777;
+		out.body.entry.nodeid = ino;
+		out.body.entry.entry_valid = UINT64_MAX;
+	})));
+
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([](auto in) {
+			return (in.header.opcode == FUSE_SETATTR &&
+				in.header.nodeid == ino);
+		}, Eq(true)),
+		_)
+	).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) {
+		SET_OUT_HEADER_LEN(out, attr);
+		out.body.attr.attr.ino = ino;
+		out.body.attr.attr.mode = S_IFDIR | 0777;	// Changed!
+		out.body.attr.attr.uid = newuser;
+	})));
+	// We should reclaim stale vnodes
+	expect_forget(ino, 1, &sem);
+
+	EXPECT_NE(0, chown(FULLPATH, newuser, -1));
+	EXPECT_EQ(ENOENT, errno);
+
+	sem_wait(&sem);
+	sem_destroy(&sem);
+}
+
 /* On a read-only mount, no attributes may be changed */
 TEST_F(RofsSetattr, erofs)
 {



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