Date: Sun, 22 Mar 2020 15:24:25 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Subject: svn commit: r359214 - in stable/12: sys/fs/fuse tests/sys/fs/fusefs Message-ID: <202003221524.02MFOP2R071290@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Sun Mar 22 15:24:25 2020 New Revision: 359214 URL: https://svnweb.freebsd.org/changeset/base/359214 Log: MFC r358867: fusefs: avoid cache corruption with buggy fuse servers The FUSE protocol allows the client (kernel) to cache a file's size, if the server (userspace daemon) allows it. A well-behaved daemon obviously should not change a file's size while a client has it cached. But a buggy daemon might. If the kernel ever detects that that has happened, then it should invalidate the entire cache for that file. Previously, we would not only cache stale data, but in the case of a file extension while we had the size cached, we accidentally extended the cache with zeros. PR: 244178 Reported by: Ben RUBSON <ben.rubson@gmx.com> Reviewed by: cem Differential Revision: https://reviews.freebsd.org/D24012 Added: stable/12/tests/sys/fs/fusefs/cache.cc - copied unchanged from r358867, head/tests/sys/fs/fusefs/cache.cc Modified: stable/12/sys/fs/fuse/fuse_internal.c stable/12/sys/fs/fuse/fuse_node.c stable/12/sys/fs/fuse/fuse_node.h stable/12/sys/fs/fuse/fuse_vnops.c stable/12/tests/sys/fs/fusefs/Makefile stable/12/tests/sys/fs/fusefs/getattr.cc stable/12/tests/sys/fs/fusefs/io.cc stable/12/tests/sys/fs/fusefs/utils.cc stable/12/tests/sys/fs/fusefs/utils.hh Directory Properties: stable/12/ (props changed) Modified: stable/12/sys/fs/fuse/fuse_internal.c ============================================================================== --- stable/12/sys/fs/fuse/fuse_internal.c Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/sys/fs/fuse/fuse_internal.c Sun Mar 22 15:24:25 2020 (r359214) @@ -857,6 +857,9 @@ fuse_internal_forget_send(struct mount *mp, 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 @@ fuse_internal_do_getattr(struct vnode *vp, struct vatt 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); Modified: stable/12/sys/fs/fuse/fuse_node.c ============================================================================== --- stable/12/sys/fs/fuse/fuse_node.c Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/sys/fs/fuse/fuse_node.c Sun Mar 22 15:24:25 2020 (r359214) @@ -449,7 +449,8 @@ fuse_vnode_size(struct vnode *vp, off_t *filesize, str 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) Modified: stable/12/sys/fs/fuse/fuse_node.h ============================================================================== --- stable/12/sys/fs/fuse/fuse_node.h Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/sys/fs/fuse/fuse_node.h Sun Mar 22 15:24:25 2020 (r359214) @@ -134,13 +134,19 @@ struct fuse_fid { #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; Modified: stable/12/sys/fs/fuse/fuse_vnops.c ============================================================================== --- stable/12/sys/fs/fuse/fuse_vnops.c Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/sys/fs/fuse/fuse_vnops.c Sun Mar 22 15:24:25 2020 (r359214) @@ -960,6 +960,8 @@ fuse_lookup_alloc(struct mount *mp, void *arg, int lkf 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; @@ -1136,6 +1138,7 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) *vpp = dvp; } else { struct fuse_vnode_data *fvdat; + struct vattr *vap; err = fuse_vnode_get(vnode_mount(dvp), feo, nid, dvp, &vp, cnp, vtyp); @@ -1156,22 +1159,27 @@ fuse_vnop_lookup(struct vop_lookup_args *ap) */ 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); Modified: stable/12/tests/sys/fs/fusefs/Makefile ============================================================================== --- stable/12/tests/sys/fs/fusefs/Makefile Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/tests/sys/fs/fusefs/Makefile Sun Mar 22 15:24:25 2020 (r359214) @@ -10,6 +10,7 @@ TESTSDIR= ${TESTSBASE}/sys/fs/fusefs GTESTS+= access GTESTS+= allow_other GTESTS+= bmap +GTESTS+= cache GTESTS+= create GTESTS+= default_permissions GTESTS+= default_permissions_privileged Copied: stable/12/tests/sys/fs/fusefs/cache.cc (from r358867, head/tests/sys/fs/fusefs/cache.cc) ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ stable/12/tests/sys/fs/fusefs/cache.cc Sun Mar 22 15:24:25 2020 (r359214, copy of r358867, 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 <sys/param.h> +#include <fcntl.h> +} + +#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<tuple<bool, bool, bool>, cache_mode, ssize_t, ssize_t> CacheParam; + +class Cache: public FuseTest, public WithParamInterface<CacheParam> { +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) + ) +); Modified: stable/12/tests/sys/fs/fusefs/getattr.cc ============================================================================== --- stable/12/tests/sys/fs/fusefs/getattr.cc Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/tests/sys/fs/fusefs/getattr.cc Sun Mar 22 15:24:25 2020 (r359214) @@ -159,6 +159,7 @@ TEST_F(Getattr, blksize_zero) 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); Modified: stable/12/tests/sys/fs/fusefs/io.cc ============================================================================== --- stable/12/tests/sys/fs/fusefs/io.cc Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/tests/sys/fs/fusefs/io.cc Sun Mar 22 15:24:25 2020 (r359214) @@ -50,28 +50,6 @@ extern "C" { 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; Modified: stable/12/tests/sys/fs/fusefs/utils.cc ============================================================================== --- stable/12/tests/sys/fs/fusefs/utils.cc Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/tests/sys/fs/fusefs/utils.cc Sun Mar 22 15:24:25 2020 (r359214) @@ -90,6 +90,21 @@ void check_environment() 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; Modified: stable/12/tests/sys/fs/fusefs/utils.hh ============================================================================== --- stable/12/tests/sys/fs/fusefs/utils.hh Sun Mar 22 15:16:59 2020 (r359213) +++ stable/12/tests/sys/fs/fusefs/utils.hh Sun Mar 22 15:24:25 2020 (r359214) @@ -44,6 +44,14 @@ inline void nap() 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;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202003221524.02MFOP2R071290>