svn commit: r358867 - in head: sys/fs/fuse tests/sys/fs/fusefs
Alan Somers
asomers at FreeBSD.org
Wed Mar 11 04:29:48 UTC 2020
Author: asomers
Date: Wed Mar 11 04:29:45 2020
New Revision: 358867
URL: https://svnweb.freebsd.org/changeset/base/358867
Log:
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 at gmx.com>
Reviewed by: cem
MFC after: 2 weeks
Differential Revision: https://reviews.freebsd.org/D24012
Added:
head/tests/sys/fs/fusefs/cache.cc (contents, props changed)
Modified:
head/sys/fs/fuse/fuse_internal.c
head/sys/fs/fuse/fuse_node.c
head/sys/fs/fuse/fuse_node.h
head/sys/fs/fuse/fuse_vnops.c
head/tests/sys/fs/fusefs/Makefile
head/tests/sys/fs/fusefs/getattr.cc
head/tests/sys/fs/fusefs/io.cc
head/tests/sys/fs/fusefs/utils.cc
head/tests/sys/fs/fusefs/utils.hh
Modified: head/sys/fs/fuse/fuse_internal.c
==============================================================================
--- head/sys/fs/fuse/fuse_internal.c Wed Mar 11 03:45:32 2020 (r358866)
+++ head/sys/fs/fuse/fuse_internal.c Wed Mar 11 04:29:45 2020 (r358867)
@@ -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: head/sys/fs/fuse/fuse_node.c
==============================================================================
--- head/sys/fs/fuse/fuse_node.c Wed Mar 11 03:45:32 2020 (r358866)
+++ head/sys/fs/fuse/fuse_node.c Wed Mar 11 04:29:45 2020 (r358867)
@@ -450,7 +450,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: head/sys/fs/fuse/fuse_node.h
==============================================================================
--- head/sys/fs/fuse/fuse_node.h Wed Mar 11 03:45:32 2020 (r358866)
+++ head/sys/fs/fuse/fuse_node.h Wed Mar 11 04:29:45 2020 (r358867)
@@ -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: head/sys/fs/fuse/fuse_vnops.c
==============================================================================
--- head/sys/fs/fuse/fuse_vnops.c Wed Mar 11 03:45:32 2020 (r358866)
+++ head/sys/fs/fuse/fuse_vnops.c Wed Mar 11 04:29:45 2020 (r358867)
@@ -961,6 +961,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;
@@ -1137,6 +1139,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);
@@ -1157,22 +1160,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: head/tests/sys/fs/fusefs/Makefile
==============================================================================
--- head/tests/sys/fs/fusefs/Makefile Wed Mar 11 03:45:32 2020 (r358866)
+++ head/tests/sys/fs/fusefs/Makefile Wed Mar 11 04:29:45 2020 (r358867)
@@ -12,6 +12,7 @@ TESTSDIR= ${TESTSBASE}/sys/fs/fusefs
GTESTS+= access
GTESTS+= allow_other
GTESTS+= bmap
+GTESTS+= cache
GTESTS+= create
GTESTS+= default_permissions
GTESTS+= default_permissions_privileged
Added: head/tests/sys/fs/fusefs/cache.cc
==============================================================================
--- /dev/null 00:00:00 1970 (empty, because file is newly added)
+++ head/tests/sys/fs/fusefs/cache.cc Wed Mar 11 04:29:45 2020 (r358867)
@@ -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: head/tests/sys/fs/fusefs/getattr.cc
==============================================================================
--- head/tests/sys/fs/fusefs/getattr.cc Wed Mar 11 03:45:32 2020 (r358866)
+++ head/tests/sys/fs/fusefs/getattr.cc Wed Mar 11 04:29:45 2020 (r358867)
@@ -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: head/tests/sys/fs/fusefs/io.cc
==============================================================================
--- head/tests/sys/fs/fusefs/io.cc Wed Mar 11 03:45:32 2020 (r358866)
+++ head/tests/sys/fs/fusefs/io.cc Wed Mar 11 04:29:45 2020 (r358867)
@@ -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: head/tests/sys/fs/fusefs/utils.cc
==============================================================================
--- head/tests/sys/fs/fusefs/utils.cc Wed Mar 11 03:45:32 2020 (r358866)
+++ head/tests/sys/fs/fusefs/utils.cc Wed Mar 11 04:29:45 2020 (r358867)
@@ -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: head/tests/sys/fs/fusefs/utils.hh
==============================================================================
--- head/tests/sys/fs/fusefs/utils.hh Wed Mar 11 03:45:32 2020 (r358866)
+++ head/tests/sys/fs/fusefs/utils.hh Wed Mar 11 04:29:45 2020 (r358867)
@@ -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;
More information about the svn-src-head
mailing list