Skip to content

Commit

Permalink
MFC r358867:
Browse files Browse the repository at this point in the history
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
  • Loading branch information
asomers committed Mar 22, 2020
1 parent 69a5eb5 commit f0ebf80
Show file tree
Hide file tree
Showing 10 changed files with 297 additions and 39 deletions.
21 changes: 21 additions & 0 deletions sys/fs/fuse/fuse_internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -899,6 +902,24 @@ fuse_internal_do_getattr(struct vnode *vp, struct vattr *vap,
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);
if (vtyp != vnode_vtype(vp)) {
Expand Down
3 changes: 2 additions & 1 deletion sys/fs/fuse/fuse_node.c
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,8 @@ fuse_vnode_size(struct vnode *vp, off_t *filesize, struct ucred *cred,
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)
Expand Down
12 changes: 9 additions & 3 deletions sys/fs/fuse/fuse_node.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
34 changes: 21 additions & 13 deletions sys/fs/fuse/fuse_vnops.c
Original file line number Diff line number Diff line change
Expand Up @@ -960,6 +960,8 @@ fuse_lookup_alloc(struct mount *mp, void *arg, int lkflags, struct vnode **vpp)

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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
1 change: 1 addition & 0 deletions tests/sys/fs/fusefs/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
219 changes: 219 additions & 0 deletions tests/sys/fs/fusefs/cache.cc
Original file line number Diff line number Diff line change
@@ -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)
)
);
1 change: 1 addition & 0 deletions tests/sys/fs/fusefs/getattr.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
22 changes: 0 additions & 22 deletions tests/sys/fs/fusefs/io.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading

0 comments on commit f0ebf80

Please sign in to comment.