From 35cf0e7e56ca4b89e81163f01b3cdeabf3246251 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Wed, 3 Apr 2019 20:57:43 +0000 Subject: [PATCH] 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 | 35 +++++----------------- tests/sys/fs/fusefs/readdir.cc | 50 +++++++++++++++++++++++++++++-- tests/sys/fs/fusefs/releasedir.cc | 12 -------- tests/sys/fs/fusefs/utils.cc | 12 ++++++++ tests/sys/fs/fusefs/utils.hh | 6 ++++ 5 files changed, 73 insertions(+), 42 deletions(-) diff --git a/sys/fs/fuse/fuse_vnops.c b/sys/fs/fuse/fuse_vnops.c index 7fb4de395c1d..0db8da80c44a 100644 --- a/sys/fs/fuse/fuse_vnops.c +++ b/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; } diff --git a/tests/sys/fs/fusefs/readdir.cc b/tests/sys/fs/fusefs/readdir.cc index 5c0789ebf4af..706292ae2229 100644 --- a/tests/sys/fs/fusefs/readdir.cc +++ b/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. diff --git a/tests/sys/fs/fusefs/releasedir.cc b/tests/sys/fs/fusefs/releasedir.cc index 8319402f6a60..588ea092eb7c 100644 --- a/tests/sys/fs/fusefs/releasedir.cc +++ b/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 */ diff --git a/tests/sys/fs/fusefs/utils.cc b/tests/sys/fs/fusefs/utils.cc index 958378b5ebfa..e00b9d3c148b 100644 --- a/tests/sys/fs/fusefs/utils.cc +++ b/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) { diff --git a/tests/sys/fs/fusefs/utils.hh b/tests/sys/fs/fusefs/utils.hh index b016e695d5d0..4022be7da302 100644 --- a/tests/sys/fs/fusefs/utils.hh +++ b/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,