Date: Wed, 3 Apr 2019 20:57:44 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r345854 - in projects/fuse2: sys/fs/fuse tests/sys/fs/fusefs Message-ID: <201904032057.x33Kvinu081270@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Wed Apr 3 20:57:43 2019 New Revision: 345854 URL: https://svnweb.freebsd.org/changeset/base/345854 Log: 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 Modified: projects/fuse2/sys/fs/fuse/fuse_vnops.c projects/fuse2/tests/sys/fs/fusefs/readdir.cc projects/fuse2/tests/sys/fs/fusefs/releasedir.cc projects/fuse2/tests/sys/fs/fusefs/utils.cc projects/fuse2/tests/sys/fs/fusefs/utils.hh Modified: projects/fuse2/sys/fs/fuse/fuse_vnops.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_vnops.c Wed Apr 3 20:37:14 2019 (r345853) +++ projects/fuse2/sys/fs/fuse/fuse_vnops.c Wed Apr 3 20:57:43 2019 (r345854) @@ -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)) { - 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); + if (vnode_isdir(vp)) return 0; - } - if (fflag & IO_NDELAY) { + 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; } Modified: projects/fuse2/tests/sys/fs/fusefs/readdir.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/readdir.cc Wed Apr 3 20:37:14 2019 (r345853) +++ projects/fuse2/tests/sys/fs/fusefs/readdir.cc Wed Apr 3 20:57:43 2019 (r345854) @@ -213,13 +213,59 @@ 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. */ } /* Modified: projects/fuse2/tests/sys/fs/fusefs/releasedir.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/releasedir.cc Wed Apr 3 20:37:14 2019 (r345853) +++ projects/fuse2/tests/sys/fs/fusefs/releasedir.cc Wed Apr 3 20:57:43 2019 (r345854) @@ -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 */ Modified: projects/fuse2/tests/sys/fs/fusefs/utils.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/utils.cc Wed Apr 3 20:37:14 2019 (r345853) +++ projects/fuse2/tests/sys/fs/fusefs/utils.cc Wed Apr 3 20:57:43 2019 (r345854) @@ -232,6 +232,18 @@ void FuseTest::expect_release(uint64_t ino, uint64_t f ).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) { Modified: projects/fuse2/tests/sys/fs/fusefs/utils.hh ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/utils.hh Wed Apr 3 20:37:14 2019 (r345853) +++ projects/fuse2/tests/sys/fs/fusefs/utils.hh Wed Apr 3 20:57:43 2019 (r345854) @@ -124,6 +124,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, * size isize and buffer contents. It will return osize
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904032057.x33Kvinu081270>