fusefs: fix a panic in VOP_READDIR

The original fusefs import, r238402, contained a bug in fuse_vnop_close that
could close a directory's file handle while there were still other open file
descriptors.  The code looks deliberate, but there is no explanation for it.
This necessitated a workaround in fuse_vnop_readdir that would open a new
file handle if, "for some mysterious reason", that vnode didn't have any
open file handles.  r345781 had the effect of causing the workaround to
panic, making the problem more visible.

This commit removes the workaround and the original bug, which also fixes
the panic.

Sponsored by:	The FreeBSD Foundation

sys/fs/fuse/fuse_vnops.c

@@ -324,21 +324,13 @@ fuse_vnop_close(struct vop_close_args *ap)
 	pid_t pid = td->td_proc->p_pid;
 	int err = 0;
 
-	if (fuse_isdeadfs(vp)) {
+	if (fuse_isdeadfs(vp))
+		return 0;
+	if (vnode_isdir(vp))
+		return 0;
+	if (fflag & IO_NDELAY)
 		return 0;
-	}
-	if (vnode_isdir(vp)) {
-		struct fuse_filehandle *fufh;
 
-		// XXX: what if two file descriptors have the same directory
-		// opened?  We shouldn't close the file handle too soon.
-		if (fuse_filehandle_get_dir(vp, &fufh, cred, pid) == 0)
-			fuse_filehandle_close(vp, fufh, NULL, cred);
-		return 0;
-	}
-	if (fflag & IO_NDELAY) {
-		return 0;
-	}
 	err = fuse_flush(vp, cred, pid, fflag);
 	/* TODO: close the file handle, if we're sure it's no longer used */
 	if ((VTOFUD(vp)->flag & FN_SIZECHANGE) != 0) {
@@ -1342,7 +1334,6 @@ fuse_vnop_readdir(struct vop_readdir_args *ap)
 	struct fuse_filehandle *fufh = NULL;
 	struct fuse_iov cookediov;
 	int err = 0;
-	int freefufh = 0;
 	pid_t pid = curthread->td_proc->p_pid;
 
 	if (fuse_isdeadfs(vp)) {
@@ -1353,27 +1344,15 @@ fuse_vnop_readdir(struct vop_readdir_args *ap)
 		return EINVAL;
 	}
 
-	if ((err = fuse_filehandle_get_dir(vp, &fufh, cred, pid)) != 0) {
-		SDT_PROBE2(fuse, , vnops, trace, 1,
-			"calling readdir() before open()");
-		/* 
-		 * This was seen to happen in getdirentries as used by
-		 * shells/fish, but I can't reproduce it.
-		 */
-		err = fuse_filehandle_open(vp, FREAD, &fufh, NULL, cred);
-		freefufh = 1;
-	}
-	if (err) {
+	err = fuse_filehandle_get_dir(vp, &fufh, cred, pid);
+	if (err)
 		return (err);
-	}
 #define DIRCOOKEDSIZE FUSE_DIRENT_ALIGN(FUSE_NAME_OFFSET + MAXNAMLEN + 1)
 	fiov_init(&cookediov, DIRCOOKEDSIZE);
 
 	err = fuse_internal_readdir(vp, uio, fufh, &cookediov);
 
 	fiov_teardown(&cookediov);
-	if (freefufh)
-		fuse_filehandle_close(vp, fufh, NULL, cred);
 
 	return err;
 }

tests/sys/fs/fusefs/readdir.cc

@@ -213,15 +213,61 @@ TEST_F(Readdir, getdirentries)
 		out->header.len = sizeof(out->header);
 	})));
 
-	errno = 0;
 	fd = open(FULLPATH, O_DIRECTORY);
 	ASSERT_LE(0, fd) << strerror(errno);
 	r = getdirentries(fd, buf, sizeof(buf), 0);
-	ASSERT_EQ(0, r);
+	ASSERT_EQ(0, r) << strerror(errno);
 
 	/* Deliberately leak fd.  RELEASEDIR will be tested separately */
 }
 
+/* 
+ * Nothing bad should happen if getdirentries is called on two file descriptors
+ * which were concurrently open, but one has already been closed.
+ * This is a regression test for a specific bug dating from r238402.
+ */
+TEST_F(Readdir, getdirentries_concurrent)
+{
+	const char FULLPATH[] = "mountpoint/some_dir";
+	const char RELPATH[] = "some_dir";
+	uint64_t ino = 42;
+	int fd0, fd1;
+	char buf[8192];
+	ssize_t r;
+
+	FuseTest::expect_lookup(RELPATH, ino, S_IFDIR | 0755, 0, 2);
+	expect_opendir(ino);
+
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([=](auto in) {
+			return (in->header.opcode == FUSE_READDIR &&
+				in->header.nodeid == ino &&
+				in->body.readdir.size == 8192);
+		}, Eq(true)),
+		_)
+	).Times(2)
+	.WillRepeatedly(Invoke(ReturnImmediate([=](auto in __unused, auto out) {
+		out->header.error = 0;
+		out->header.len = sizeof(out->header);
+	})));
+
+	fd0 = open(FULLPATH, O_DIRECTORY);
+	ASSERT_LE(0, fd0) << strerror(errno);
+
+	fd1 = open(FULLPATH, O_DIRECTORY);
+	ASSERT_LE(0, fd1) << strerror(errno);
+
+	r = getdirentries(fd0, buf, sizeof(buf), 0);
+	ASSERT_EQ(0, r) << strerror(errno);
+
+	EXPECT_EQ(0, close(fd0)) << strerror(errno);
+
+	r = getdirentries(fd1, buf, sizeof(buf), 0);
+	ASSERT_EQ(0, r) << strerror(errno);
+
+	/* Deliberately leak fd1. */
+}
+
 /*
  * FUSE_READDIR returns nothing, not even "." and "..".  This is legal, though
  * the filesystem obviously won't be fully functional.

tests/sys/fs/fusefs/releasedir.cc

@@ -45,18 +45,6 @@ void expect_lookup(const char *relpath, uint64_t ino)
 {
 	FuseTest::expect_lookup(relpath, ino, S_IFDIR | 0755, 0, 1);
 }
-
-void expect_releasedir(uint64_t ino, ProcessMockerT r)
-{
-	EXPECT_CALL(*m_mock, process(
-		ResultOf([=](auto in) {
-			return (in->header.opcode == FUSE_RELEASEDIR &&
-				in->header.nodeid == ino &&
-				in->body.release.fh == FH);
-		}, Eq(true)),
-		_)
-	).WillOnce(Invoke(r));
-}
 };
 
 /* If a file descriptor is duplicated, only the last close causes RELEASE */

tests/sys/fs/fusefs/utils.cc

@@ -232,6 +232,18 @@ void FuseTest::expect_release(uint64_t ino, uint64_t fh)
 	).WillOnce(Invoke(ReturnErrno(0)));
 }
 
+void FuseTest::expect_releasedir(uint64_t ino, ProcessMockerT r)
+{
+	EXPECT_CALL(*m_mock, process(
+		ResultOf([=](auto in) {
+			return (in->header.opcode == FUSE_RELEASEDIR &&
+				in->header.nodeid == ino &&
+				in->body.release.fh == FH);
+		}, Eq(true)),
+		_)
+	).WillOnce(Invoke(r));
+}
+
 void FuseTest::expect_write(uint64_t ino, uint64_t offset, uint64_t isize,
 	uint64_t osize, uint32_t flags, const void *contents)
 {

tests/sys/fs/fusefs/utils.hh

@@ -123,6 +123,12 @@ class FuseTest : public ::testing::Test {
 	 */
 	void expect_release(uint64_t ino, uint64_t fh);
 
+	/*
+	 * Create an expectation that FUSE_RELEASEDIR will be called exactly
+	 * once for the given inode
+	 */
+	void expect_releasedir(uint64_t ino, ProcessMockerT r);
+
 	/*
 	 * Create an expectation that FUSE_WRITE will be called exactly once
 	 * for the given inode, at offset offset, with write_flags flags,