Index: head/sys/fs/fuse/fuse_internal.c =================================================================== --- head/sys/fs/fuse/fuse_internal.c +++ head/sys/fs/fuse/fuse_internal.c @@ -857,6 +857,9 @@ fdisp_destroy(&fdi); } +SDT_PROBE_DEFINE2(fusefs, , internal, getattr_cache_incoherent, + "struct vnode*", "struct fuse_attr_out*"); + /* Fetch the vnode's attributes from the daemon*/ int fuse_internal_do_getattr(struct vnode *vp, struct vattr *vap, @@ -898,6 +901,24 @@ if (fvdat->flag & FN_MTIMECHANGE) { fao->attr.mtime = old_mtime.tv_sec; fao->attr.mtimensec = old_mtime.tv_nsec; + } + if (vnode_isreg(vp) && + fvdat->cached_attrs.va_size != VNOVAL && + fao->attr.size != fvdat->cached_attrs.va_size) { + /* + * The server changed the file's size even though we had it + * cached! That's a server bug. + */ + SDT_PROBE2(fusefs, , internal, getattr_cache_incoherent, vp, + fao); + printf("%s: cache incoherent on %s! " + "Buggy FUSE server detected. To prevent data corruption, " + "disable the data cache by mounting with -o direct_io, or " + "as directed otherwise by your FUSE server's " + "documentation\n", __func__, + vnode_mount(vp)->mnt_stat.f_mntonname); + int iosize = fuse_iosize(vp); + v_inval_buf_range(vp, 0, INT64_MAX, iosize); } fuse_internal_cache_attrs(vp, &fao->attr, fao->attr_valid, fao->attr_valid_nsec, vap); Index: head/sys/fs/fuse/fuse_node.h =================================================================== --- head/sys/fs/fuse/fuse_node.h +++ head/sys/fs/fuse/fuse_node.h @@ -134,13 +134,19 @@ #define VTOFUD(vp) \ ((struct fuse_vnode_data *)((vp)->v_data)) #define VTOI(vp) (VTOFUD(vp)->nid) -static inline struct vattr* -VTOVA(struct vnode *vp) +static inline bool +fuse_vnode_attr_cache_valid(struct vnode *vp) { struct bintime now; getbinuptime(&now); - if (bintime_cmp(&(VTOFUD(vp)->attr_cache_timeout), &now, >)) + return (bintime_cmp(&(VTOFUD(vp)->attr_cache_timeout), &now, >)); +} + +static inline struct vattr* +VTOVA(struct vnode *vp) +{ + if (fuse_vnode_attr_cache_valid(vp)) return &(VTOFUD(vp)->cached_attrs); else return NULL; Index: head/sys/fs/fuse/fuse_node.c =================================================================== --- head/sys/fs/fuse/fuse_node.c +++ head/sys/fs/fuse/fuse_node.c @@ -450,7 +450,8 @@ int error = 0; if (!(fvdat->flag & FN_SIZECHANGE) && - (VTOVA(vp) == NULL || fvdat->cached_attrs.va_size == VNOVAL)) + (!fuse_vnode_attr_cache_valid(vp) || + fvdat->cached_attrs.va_size == VNOVAL)) error = fuse_internal_do_getattr(vp, NULL, cred, td); if (!error) Index: head/sys/fs/fuse/fuse_vnops.c =================================================================== --- head/sys/fs/fuse/fuse_vnops.c +++ head/sys/fs/fuse/fuse_vnops.c @@ -961,6 +961,8 @@ SDT_PROBE_DEFINE3(fusefs, , vnops, cache_lookup, "int", "struct timespec*", "struct timespec*"); +SDT_PROBE_DEFINE2(fusefs, , vnops, lookup_cache_incoherent, + "struct vnode*", "struct fuse_entry_out*"); /* struct vnop_lookup_args { struct vnodeop_desc *a_desc; @@ -1137,6 +1139,7 @@ *vpp = dvp; } else { struct fuse_vnode_data *fvdat; + struct vattr *vap; err = fuse_vnode_get(vnode_mount(dvp), feo, nid, dvp, &vp, cnp, vtyp); @@ -1157,22 +1160,27 @@ */ fvdat = VTOFUD(vp); if (vnode_isreg(vp) && - filesize != fvdat->cached_attrs.va_size && - fvdat->flag & FN_SIZECHANGE) { + ((filesize != fvdat->cached_attrs.va_size && + fvdat->flag & FN_SIZECHANGE) || + ((vap = VTOVA(vp)) && + filesize != vap->va_size))) + { + SDT_PROBE2(fusefs, , vnops, lookup_cache_incoherent, vp, feo); + fvdat->flag &= ~FN_SIZECHANGE; /* - * The FN_SIZECHANGE flag reflects a dirty - * append. If userspace lets us know our cache - * is invalid, that write was lost. (Dirty - * writes that do not cause append are also - * lost, but we don't detect them here.) - * - * XXX: Maybe disable WB caching on this mount. + * The server changed the file's size even + * though we had it cached, or had dirty writes + * in the WB cache! */ - printf("%s: WB cache incoherent on %s!\n", - __func__, + printf("%s: cache incoherent on %s! " + "Buggy FUSE server detected. To prevent " + "data corruption, disable the data cache " + "by mounting with -o direct_io, or as " + "directed otherwise by your FUSE server's " + "documentation\n", __func__, vnode_mount(vp)->mnt_stat.f_mntonname); - - fvdat->flag &= ~FN_SIZECHANGE; + int iosize = fuse_iosize(vp); + v_inval_buf_range(vp, 0, INT64_MAX, iosize); } MPASS(feo != NULL); Index: head/tests/sys/fs/fusefs/Makefile =================================================================== --- head/tests/sys/fs/fusefs/Makefile +++ head/tests/sys/fs/fusefs/Makefile @@ -12,6 +12,7 @@ GTESTS+= access GTESTS+= allow_other GTESTS+= bmap +GTESTS+= cache GTESTS+= create GTESTS+= default_permissions GTESTS+= default_permissions_privileged Index: head/tests/sys/fs/fusefs/cache.cc =================================================================== --- head/tests/sys/fs/fusefs/cache.cc +++ head/tests/sys/fs/fusefs/cache.cc @@ -0,0 +1,219 @@ +/*- + * SPDX-License-Identifier: BSD-2-Clause-FreeBSD + * + * Copyright (c) 2020 Alan Somers + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * 1. Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + * + * $FreeBSD$ + */ + +extern "C" { +#include +#include +} + +#include "mockfs.hh" +#include "utils.hh" + +/* + * Tests for thorny cache problems not specific to any one opcode + */ + +using namespace testing; + +/* + * Parameters + * - reopen file - If true, close and reopen the file between reads + * - cache lookups - If true, allow lookups to be cached + * - cache attrs - If true, allow file attributes to be cached + * - cache_mode - uncached, writeback, or writethrough + * - initial size - File size before truncation + * - truncated size - File size after truncation + */ +typedef tuple, cache_mode, ssize_t, ssize_t> CacheParam; + +class Cache: public FuseTest, public WithParamInterface { +public: +bool m_direct_io; + +Cache(): m_direct_io(false) {}; + +virtual void SetUp() { + int cache_mode = get<1>(GetParam()); + switch (cache_mode) { + case Uncached: + m_direct_io = true; + break; + case WritebackAsync: + m_async = true; + /* FALLTHROUGH */ + case Writeback: + m_init_flags |= FUSE_WRITEBACK_CACHE; + /* FALLTHROUGH */ + case Writethrough: + break; + default: + FAIL() << "Unknown cache mode"; + } + + FuseTest::SetUp(); + if (IsSkipped()) + return; +} + +void expect_getattr(uint64_t ino, int times, uint64_t size, uint64_t attr_valid) +{ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_GETATTR && + in.header.nodeid == ino); + }, Eq(true)), + _) + ).Times(times) + .WillRepeatedly(Invoke(ReturnImmediate([=](auto i __unused, auto& out) { + SET_OUT_HEADER_LEN(out, attr); + out.body.attr.attr_valid = attr_valid; + out.body.attr.attr.ino = ino; + out.body.attr.attr.mode = S_IFREG | 0644; + out.body.attr.attr.size = size; + }))); +} + +void expect_lookup(const char *relpath, uint64_t ino, + uint64_t size, uint64_t entry_valid, uint64_t attr_valid) +{ + EXPECT_LOOKUP(FUSE_ROOT_ID, relpath) + .WillRepeatedly(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 = attr_valid; + out.body.entry.attr.size = size; + out.body.entry.entry_valid = entry_valid; + }))); +} + +void expect_open(uint64_t ino, int times) +{ + FuseTest::expect_open(ino, m_direct_io ? FOPEN_DIRECT_IO: 0, times); +} + +void expect_release(uint64_t ino, ProcessMockerT r) +{ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_RELEASE && + in.header.nodeid == ino); + }, Eq(true)), + _) + ).WillRepeatedly(Invoke(r)); +} + +}; + +// If the server truncates the file behind the kernel's back, the kernel should +// invalidate cached pages beyond the new EOF +TEST_P(Cache, truncate_by_surprise_invalidates_cache) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + const char *CONTENTS = "abcdefghijklmnopqrstuvwxyz"; + uint64_t ino = 42; + uint64_t attr_valid, entry_valid; + int fd; + ssize_t bufsize = strlen(CONTENTS); + uint8_t buf[bufsize]; + bool reopen = get<0>(get<0>(GetParam())); + bool cache_lookups = get<1>(get<0>(GetParam())); + bool cache_attrs = get<2>(get<0>(GetParam())); + ssize_t osize = get<2>(GetParam()); + ssize_t nsize = get<3>(GetParam()); + + ASSERT_LE(osize, bufsize); + ASSERT_LE(nsize, bufsize); + if (cache_attrs) + attr_valid = UINT64_MAX; + else + attr_valid = 0; + if (cache_lookups) + entry_valid = UINT64_MAX; + else + entry_valid = 0; + + expect_lookup(RELPATH, ino, osize, entry_valid, attr_valid); + expect_open(ino, 1); + if (!cache_attrs) + expect_getattr(ino, 2, osize, attr_valid); + expect_read(ino, 0, osize, osize, CONTENTS); + + fd = open(FULLPATH, O_RDONLY); + ASSERT_LE(0, fd) << strerror(errno); + + ASSERT_EQ(osize, read(fd, buf, bufsize)) << strerror(errno); + ASSERT_EQ(0, memcmp(buf, CONTENTS, osize)); + + // Now truncate the file behind the kernel's back. The next read + // should discard cache and fetch from disk again. + if (reopen) { + // Close and reopen the file + expect_flush(ino, 1, ReturnErrno(ENOSYS)); + expect_release(ino, ReturnErrno(0)); + ASSERT_EQ(0, close(fd)); + expect_lookup(RELPATH, ino, nsize, entry_valid, attr_valid); + expect_open(ino, 1); + fd = open(FULLPATH, O_RDONLY); + ASSERT_LE(0, fd) << strerror(errno); + } + + if (!cache_attrs) + expect_getattr(ino, 1, nsize, attr_valid); + expect_read(ino, 0, nsize, nsize, CONTENTS); + ASSERT_EQ(0, lseek(fd, 0, SEEK_SET)); + ASSERT_EQ(nsize, read(fd, buf, bufsize)) << strerror(errno); + ASSERT_EQ(0, memcmp(buf, CONTENTS, nsize)); + + leak(fd); +} + +INSTANTIATE_TEST_CASE_P(Cache, Cache, + Combine( + /* Test every combination that: + * - does not cache at least one of entries and attrs + * - either doesn't cache attrs, or reopens the file + * In the other combinations, the kernel will never learn that + * the file's size has changed. + */ + Values( + std::make_tuple(false, false, false), + std::make_tuple(false, true, false), + std::make_tuple(true, false, false), + std::make_tuple(true, false, true), + std::make_tuple(true, true, false) + ), + Values(Writethrough, Writeback), + /* Test both reductions and extensions to file size */ + Values(20), + Values(10, 25) + ) +); Index: head/tests/sys/fs/fusefs/getattr.cc =================================================================== --- head/tests/sys/fs/fusefs/getattr.cc +++ head/tests/sys/fs/fusefs/getattr.cc @@ -159,6 +159,7 @@ out.body.attr.attr.mode = S_IFREG | 0644; out.body.attr.attr.ino = ino; // Must match nodeid out.body.attr.attr.blksize = 0; + out.body.attr.attr.size = 1; }))); ASSERT_EQ(0, stat(FULLPATH, &sb)) << strerror(errno); Index: head/tests/sys/fs/fusefs/io.cc =================================================================== --- head/tests/sys/fs/fusefs/io.cc +++ head/tests/sys/fs/fusefs/io.cc @@ -50,28 +50,6 @@ using namespace testing; -enum cache_mode { - Uncached, - Writethrough, - Writeback, - WritebackAsync -}; - -const char *cache_mode_to_s(enum cache_mode cm) { - switch (cm) { - case Uncached: - return "Uncached"; - case Writethrough: - return "Writethrough"; - case Writeback: - return "Writeback"; - case WritebackAsync: - return "WritebackAsync"; - default: - return "Unknown"; - } -} - const char FULLPATH[] = "mountpoint/some_file.txt"; const char RELPATH[] = "some_file.txt"; const uint64_t ino = 42; Index: head/tests/sys/fs/fusefs/utils.hh =================================================================== --- head/tests/sys/fs/fusefs/utils.hh +++ head/tests/sys/fs/fusefs/utils.hh @@ -44,6 +44,14 @@ usleep(NAP_NS / 1000); } +enum cache_mode { + Uncached, + Writethrough, + Writeback, + WritebackAsync +}; + +const char *cache_mode_to_s(enum cache_mode cm); bool is_unsafe_aio_enabled(void); extern const uint32_t libfuse_max_write; Index: head/tests/sys/fs/fusefs/utils.cc =================================================================== --- head/tests/sys/fs/fusefs/utils.cc +++ head/tests/sys/fs/fusefs/utils.cc @@ -90,6 +90,21 @@ GTEST_SKIP() << "current user is not allowed to mount"; } +const char *cache_mode_to_s(enum cache_mode cm) { + switch (cm) { + case Uncached: + return "Uncached"; + case Writethrough: + return "Writethrough"; + case Writeback: + return "Writeback"; + case WritebackAsync: + return "WritebackAsync"; + default: + return "Unknown"; + } +} + bool is_unsafe_aio_enabled(void) { const char *node = "vfs.aio.enable_unsafe"; int val = 0;