From 18a2264e27f6220cbd50f1692d64736f3bf5adfa Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Wed, 22 May 2019 23:30:51 +0000 Subject: [PATCH] fusefs: fix "recursing on non recursive lockmgr" panic When mounted with -o default_permissions and when vfs.fusefs.data_cache_mode=2, fuse_io_strategy would try to clear the suid bit after a successful write by a non-owner. When combined with a not-yet-committed attribute-caching patch I'm working on, and if the FUSE_SETATTR response indicates an unexpected filesize (legal, if the file system has other clients), this would end up calling vtruncbuf. That would panic, because the buffer lock was already held by bufwrite or bufstrategy or something else upstack from fuse_vnop_strategy. Sponsored by: The FreeBSD Foundation --- sys/fs/fuse/fuse_io.c | 3 -- tests/sys/fs/fusefs/default_permissions.cc | 38 ++++++++++++++++++++-- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/sys/fs/fuse/fuse_io.c b/sys/fs/fuse/fuse_io.c index 489f53f51852..c5a8fb8d9d1a 100644 --- a/sys/fs/fuse/fuse_io.c +++ b/sys/fs/fuse/fuse_io.c @@ -883,9 +883,6 @@ fuse_io_strategy(struct vnode *vp, struct buf *bp) bp->b_ioflags |= BIO_ERROR; bp->b_flags |= B_INVAL; bp->b_error = error; - } else { - fuse_io_clear_suid_on_write(vp, cred, - uio.uio_td); } bp->b_dirtyoff = bp->b_dirtyend = 0; } diff --git a/tests/sys/fs/fusefs/default_permissions.cc b/tests/sys/fs/fusefs/default_permissions.cc index 1f69a9b9bbec..88f6d76c6b4a 100644 --- a/tests/sys/fs/fusefs/default_permissions.cc +++ b/tests/sys/fs/fusefs/default_permissions.cc @@ -68,7 +68,7 @@ virtual void SetUp() { } public: -void expect_chmod(uint64_t ino, mode_t mode) +void expect_chmod(uint64_t ino, mode_t mode, uint64_t size = 0) { EXPECT_CALL(*m_mock, process( ResultOf([=](auto in) { @@ -82,6 +82,7 @@ void expect_chmod(uint64_t ino, mode_t mode) SET_OUT_HEADER_LEN(out, attr); out->body.attr.attr.ino = ino; // Must match nodeid out->body.attr.attr.mode = S_IFREG | mode; + out->body.attr.attr.size = size; out->body.attr.attr_valid = UINT64_MAX; }))); } @@ -1229,7 +1230,7 @@ TEST_F(Write, clear_suid) expect_lookup(RELPATH, ino, S_IFREG | oldmode, UINT64_MAX); expect_open(ino, 0, 1); expect_write(ino, 0, sizeof(wbuf), sizeof(wbuf), 0, wbuf); - expect_chmod(ino, newmode); + expect_chmod(ino, newmode, sizeof(wbuf)); fd = open(FULLPATH, O_WRONLY); ASSERT_LE(0, fd) << strerror(errno); @@ -1255,7 +1256,7 @@ TEST_F(Write, clear_sgid) expect_lookup(RELPATH, ino, S_IFREG | oldmode, UINT64_MAX); expect_open(ino, 0, 1); expect_write(ino, 0, sizeof(wbuf), sizeof(wbuf), 0, wbuf); - expect_chmod(ino, newmode); + expect_chmod(ino, newmode, sizeof(wbuf)); fd = open(FULLPATH, O_WRONLY); ASSERT_LE(0, fd) << strerror(errno); @@ -1264,3 +1265,34 @@ TEST_F(Write, clear_sgid) EXPECT_EQ(S_IFREG | newmode, sb.st_mode); /* Deliberately leak fd. close(2) will be tested in release.cc */ } + +/* Regression test for a specific recurse-of-nonrecursive-lock panic + * + * With writeback caching, we can't call vtruncbuf from fuse_io_strategy, or it + * may panic. That happens if the FUSE_SETATTR response indicates that the + * file's size has changed since the write. + */ +TEST_F(Write, recursion_panic_while_clearing_suid) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + uint64_t ino = 42; + mode_t oldmode = 04777; + mode_t newmode = 0777; + char wbuf[1] = {'x'}; + int fd; + + expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1); + expect_lookup(RELPATH, ino, S_IFREG | oldmode, UINT64_MAX); + expect_open(ino, 0, 1); + expect_write(ino, 0, sizeof(wbuf), sizeof(wbuf), 0, wbuf); + /* XXX Return a smaller file size than what we just wrote! */ + expect_chmod(ino, newmode, 0); + + fd = open(FULLPATH, O_WRONLY); + ASSERT_LE(0, fd) << strerror(errno); + ASSERT_EQ(1, write(fd, wbuf, sizeof(wbuf))) << strerror(errno); + /* Deliberately leak fd. close(2) will be tested in release.cc */ +} + +