From bfffd351080752652c9f850d1b5540f1097bc7c8 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Wed, 1 Dec 2021 19:38:04 -0700 Subject: [PATCH] fusefs: in the tests, always assume debug.try_reclaim_vnode is available In an earlier version of the revision that created that sysctl (D20519) the sysctl was gated by INVARIANTS, so the test had to check for it. But in the committed version it is always available. (cherry picked from commit 19ab361045343bb777176bb08468f7706d7649c4) fusefs: move common code from forget.cc to utils.cc (cherry picked from commit 8d99a6b91b788b7ddf88f975f288f7c6479f4be3) fusefs: fix .. lookups when the parent has been reclaimed. By default, FUSE file systems are assumed not to support lookups for "." and "..". They must opt-in to that. To cope with this limitation, the fusefs kernel module caches every fuse vnode's parent's inode number, and uses that during VOP_LOOKUP for "..". But if the parent's vnode has been reclaimed that won't be possible. Previously we paniced in this situation. Now, we'll return ESTALE instead. Or, if the file system has opted into ".." lookups, we'll just do that instead. This commit also fixes VOP_LOOKUP to respect the cache timeout for ".." lookups, if the FUSE file system specified a finite timeout. PR: 259974 Reviewed by: pfg Differential Revision: https://reviews.freebsd.org/D33239 (cherry picked from commit 1613087a8127122b03a3730046d051adf4edd14f) --- sys/fs/fuse/fuse_vnops.c | 26 +++-- tests/sys/fs/fusefs/forget.cc | 14 +-- tests/sys/fs/fusefs/lookup.cc | 203 ++++++++++++++++++++++++++++++++++ tests/sys/fs/fusefs/utils.cc | 9 ++ tests/sys/fs/fusefs/utils.hh | 4 + 5 files changed, 236 insertions(+), 20 deletions(-) diff --git a/sys/fs/fuse/fuse_vnops.c b/sys/fs/fuse/fuse_vnops.c index 5cef2a7ea62a..f57f376e5685 100644 --- a/sys/fs/fuse/fuse_vnops.c +++ b/sys/fs/fuse/fuse_vnops.c @@ -1320,9 +1320,15 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) else if ((err = fuse_internal_access(dvp, VEXEC, td, cred))) return err; - if (flags & ISDOTDOT) { - KASSERT(VTOFUD(dvp)->flag & FN_PARENT_NID, - ("Looking up .. is TODO")); + if ((flags & ISDOTDOT) && !(data->dataflags & FSESS_EXPORT_SUPPORT)) + { + if (!(VTOFUD(dvp)->flag & FN_PARENT_NID)) { + /* + * Since the file system doesn't support ".." lookups, + * we have no way to find this entry. + */ + return ESTALE; + } nid = VTOFUD(dvp)->parent_nid; if (nid == 0) return ENOENT; @@ -1375,9 +1381,8 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) return err; } - nid = VTOI(dvp); fdisp_init(&fdi, cnp->cn_namelen + 1); - fdisp_make(&fdi, FUSE_LOOKUP, mp, nid, td, cred); + fdisp_make(&fdi, FUSE_LOOKUP, mp, VTOI(dvp), td, cred); memcpy(fdi.indata, cnp->cn_nameptr, cnp->cn_namelen); ((char *)fdi.indata)[cnp->cn_namelen] = '\0'; @@ -1396,11 +1401,16 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) lookup_err = ENOENT; if (cnp->cn_flags & MAKEENTRY) { fuse_validity_2_timespec(feo, &timeout); + /* Use the same entry_time for .. as for + * the file itself. That doesn't honor + * exactly what the fuse server tells + * us, but to do otherwise would require + * another cache lookup at this point. + */ + struct timespec *dtsp = NULL; cache_enter_time(dvp, *vpp, cnp, - &timeout, NULL); + &timeout, dtsp); } - } else if (nid == FUSE_ROOT_ID) { - lookup_err = EINVAL; } vtyp = IFTOVT(feo->attr.mode); filesize = feo->attr.size; diff --git a/tests/sys/fs/fusefs/forget.cc b/tests/sys/fs/fusefs/forget.cc index c138b7acc4aa..84fc271df57c 100644 --- a/tests/sys/fs/fusefs/forget.cc +++ b/tests/sys/fs/fusefs/forget.cc @@ -44,18 +44,12 @@ extern "C" { using namespace testing; -const char reclaim_mib[] = "debug.try_reclaim_vnode"; - class Forget: public FuseTest { public: void SetUp() { if (geteuid() != 0) GTEST_SKIP() << "Only root may use " << reclaim_mib; - if (-1 == sysctlbyname(reclaim_mib, NULL, 0, NULL, 0) && - errno == ENOENT) - GTEST_SKIP() << reclaim_mib << " is not available"; - FuseTest::SetUp(); } @@ -71,7 +65,6 @@ TEST_F(Forget, ok) uint64_t ino = 42; mode_t mode = S_IFREG | 0755; sem_t sem; - int err; ASSERT_EQ(0, sem_init(&sem, 0, 0)) << strerror(errno); @@ -94,8 +87,7 @@ TEST_F(Forget, ok) ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno); ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno); - err = sysctlbyname(reclaim_mib, NULL, 0, FULLPATH, sizeof(FULLPATH)); - ASSERT_EQ(0, err) << strerror(errno); + reclaim_vnode(FULLPATH); sem_wait(&sem); sem_destroy(&sem); @@ -113,7 +105,6 @@ TEST_F(Forget, invalidate_names) const char FNAME[] = "some_file.txt"; uint64_t dir_ino = 42; uint64_t file_ino = 43; - int err; EXPECT_LOOKUP(FUSE_ROOT_ID, DNAME) .Times(2) @@ -149,8 +140,7 @@ TEST_F(Forget, invalidate_names) ASSERT_EQ(0, access(FULLFPATH, F_OK)) << strerror(errno); /* Reclaim the directory, invalidating its children from namecache */ - err = sysctlbyname(reclaim_mib, NULL, 0, FULLDPATH, sizeof(FULLDPATH)); - ASSERT_EQ(0, err) << strerror(errno); + reclaim_vnode(FULLDPATH); /* Access the file again, causing another lookup */ ASSERT_EQ(0, access(FULLFPATH, F_OK)) << strerror(errno); diff --git a/tests/sys/fs/fusefs/lookup.cc b/tests/sys/fs/fusefs/lookup.cc index d301990c2048..2dfa10730ec8 100644 --- a/tests/sys/fs/fusefs/lookup.cc +++ b/tests/sys/fs/fusefs/lookup.cc @@ -31,6 +31,10 @@ */ extern "C" { +#include +#include + +#include #include } @@ -40,6 +44,7 @@ extern "C" { using namespace testing; class Lookup: public FuseTest {}; + class Lookup_7_8: public Lookup { public: virtual void SetUp() { @@ -48,6 +53,14 @@ virtual void SetUp() { } }; +class LookupExportable: public Lookup { +public: +virtual void SetUp() { + m_init_flags = FUSE_EXPORT_SUPPORT; + Lookup::SetUp(); +} +}; + /* * If lookup returns a non-zero cache timeout, then subsequent VOP_GETATTRs * should use the cached attributes, rather than query the daemon @@ -181,6 +194,89 @@ TEST_F(Lookup, dotdot) ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno); } +/* + * Lookup ".." when that vnode's entry cache has timed out, but its child's + * hasn't. Since this file system doesn't set FUSE_EXPORT_SUPPORT, we have no + * choice but to use the cached entry, even though it expired. + */ +TEST_F(Lookup, dotdot_entry_cache_timeout) +{ + uint64_t foo_ino = 42; + uint64_t bar_ino = 43; + + EXPECT_LOOKUP(FUSE_ROOT_ID, "foo") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = foo_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = 0; // immediate timeout + }))); + EXPECT_LOOKUP(foo_ino, "bar") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = bar_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + expect_opendir(bar_ino); + + int fd = open("mountpoint/foo/bar", O_EXEC| O_DIRECTORY); + ASSERT_LE(0, fd) << strerror(errno); + EXPECT_EQ(0, faccessat(fd, "../..", F_OK, 0)) << strerror(errno); +} + +/* + * Lookup ".." for a vnode with no valid parent nid + * Regression test for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=259974 + * Since the file system is not exportable, we have no choice but to return an + * error. + */ +TEST_F(Lookup, dotdot_no_parent_nid) +{ + uint64_t foo_ino = 42; + uint64_t bar_ino = 43; + int fd; + + EXPECT_LOOKUP(FUSE_ROOT_ID, "foo") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = foo_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + EXPECT_LOOKUP(foo_ino, "bar") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = bar_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_OPENDIR); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, open); + }))); + expect_forget(FUSE_ROOT_ID, 1, NULL); + expect_forget(foo_ino, 1, NULL); + + fd = open("mountpoint/foo/bar", O_EXEC| O_DIRECTORY); + ASSERT_LE(0, fd) << strerror(errno); + // Try (and fail) to unmount the file system, to reclaim the mountpoint + // and foo vnodes. + ASSERT_NE(0, unmount("mountpoint", 0)); + EXPECT_EQ(EBUSY, errno); + nap(); // Because vnode reclamation is asynchronous + EXPECT_NE(0, faccessat(fd, "../..", F_OK, 0)); + EXPECT_EQ(ESTALE, errno); +} + TEST_F(Lookup, enoent) { const char FULLPATH[] = "mountpoint/does_not_exist"; @@ -398,4 +494,111 @@ TEST_F(Lookup_7_8, ok) ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno); } +/* + * Lookup ".." when that vnode's entry cache has timed out, but its child's + * hasn't. + */ +TEST_F(LookupExportable, dotdot_entry_cache_timeout) +{ + uint64_t foo_ino = 42; + uint64_t bar_ino = 43; + EXPECT_LOOKUP(FUSE_ROOT_ID, "foo") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = foo_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = 0; // immediate timeout + }))); + EXPECT_LOOKUP(foo_ino, "bar") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = bar_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + expect_opendir(bar_ino); + EXPECT_LOOKUP(foo_ino, "..") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = FUSE_ROOT_ID; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + + int fd = open("mountpoint/foo/bar", O_EXEC| O_DIRECTORY); + ASSERT_LE(0, fd) << strerror(errno); + /* FreeBSD's fusefs driver always uses the same cache expiration time + * for ".." as for the directory itself. So we need to look up two + * levels to find an expired ".." cache entry. + */ + EXPECT_EQ(0, faccessat(fd, "../..", F_OK, 0)) << strerror(errno); +} + +/* + * Lookup ".." for a vnode with no valid parent nid + * Regression test for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=259974 + * Since the file system is exportable, we should resolve the problem by + * sending a FUSE_LOOKUP for "..". + */ +TEST_F(LookupExportable, dotdot_no_parent_nid) +{ + uint64_t foo_ino = 42; + uint64_t bar_ino = 43; + int fd; + + EXPECT_LOOKUP(FUSE_ROOT_ID, "foo") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = foo_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + EXPECT_LOOKUP(foo_ino, "bar") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = bar_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_OPENDIR); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, open); + }))); + expect_forget(FUSE_ROOT_ID, 1, NULL); + expect_forget(foo_ino, 1, NULL); + EXPECT_LOOKUP(bar_ino, "..") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = foo_ino; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + EXPECT_LOOKUP(foo_ino, "..") + .WillOnce(Invoke(ReturnImmediate([=](auto in __unused, auto& out) { + SET_OUT_HEADER_LEN(out, entry); + out.body.entry.attr.mode = S_IFDIR | 0755; + out.body.entry.nodeid = FUSE_ROOT_ID; + out.body.entry.attr_valid = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + + fd = open("mountpoint/foo/bar", O_EXEC| O_DIRECTORY); + ASSERT_LE(0, fd) << strerror(errno); + // Try (and fail) to unmount the file system, to reclaim the mountpoint + // and foo vnodes. + ASSERT_NE(0, unmount("mountpoint", 0)); + EXPECT_EQ(EBUSY, errno); + nap(); // Because vnode reclamation is asynchronous + EXPECT_EQ(0, faccessat(fd, "../..", F_OK, 0)) << strerror(errno); +} diff --git a/tests/sys/fs/fusefs/utils.cc b/tests/sys/fs/fusefs/utils.cc index f733fef7ebe0..fb2109e1e9c4 100644 --- a/tests/sys/fs/fusefs/utils.cc +++ b/tests/sys/fs/fusefs/utils.cc @@ -623,6 +623,15 @@ out: return; } +void +FuseTest::reclaim_vnode(const char *path) +{ + int err; + + err = sysctlbyname(reclaim_mib, NULL, 0, path, strlen(path) + 1); + ASSERT_EQ(0, err) << strerror(errno); +} + static void usage(char* progname) { fprintf(stderr, "Usage: %s [-v]\n\t-v increase verbosity\n", progname); exit(2); diff --git a/tests/sys/fs/fusefs/utils.hh b/tests/sys/fs/fusefs/utils.hh index 6f1f91b02c97..610d2126fa52 100644 --- a/tests/sys/fs/fusefs/utils.hh +++ b/tests/sys/fs/fusefs/utils.hh @@ -73,6 +73,7 @@ class FuseTest : public ::testing::Test { unsigned m_time_gran; MockFS *m_mock = NULL; const static uint64_t FH = 0xdeadbeef1a7ebabe; + const char *reclaim_mib = "debug.try_reclaim_vnode"; public: int m_maxbcachebuf; @@ -256,4 +257,7 @@ class FuseTest : public ::testing::Test { * See comments for FuseTest::leak */ static void leakdir(DIR* dirp __unused) {} + + /* Manually reclaim a vnode. Requires root privileges. */ + void reclaim_vnode(const char *fullpath); };