From ed74f781c9f704092556f860a00b0bb53fdedff7 Mon Sep 17 00:00:00 2001 From: Alan Somers Date: Thu, 18 Jul 2019 17:55:13 +0000 Subject: [PATCH] fusefs: add a intr/nointr mount option FUSE file systems can optionally support interrupting outstanding operations. However, the file system does not identify to the kernel at mount time whether it's capable of doing that. Instead it signals its noncapability by returning ENOSYS to the first FUSE_INTERRUPT operation it receives. That's a problem for reliable signal delivery, because the kernel must choose which thread should get a signal before it knows whether the FUSE server can handle interrupts. The problem is even worse because the FUSE protocol allows a file system to simply ignore all FUSE_INTERRUPT operations. Fix the signal delivery logic by making interruptibility an opt-in mount option. This will require a corresponding change to libfuse, but not to most file systems that link to libfuse. Bump __FreeBSD_version due to the new mount option. Sponsored by: The FreeBSD Foundation --- sbin/mount_fusefs/mount_fusefs.8 | 7 ++- sbin/mount_fusefs/mount_fusefs.c | 3 ++ sys/fs/fuse/fuse_ipc.c | 6 +-- sys/fs/fuse/fuse_ipc.h | 3 +- sys/fs/fuse/fuse_vfsops.c | 1 + sys/sys/param.h | 2 +- tests/sys/fs/fusefs/interrupt.cc | 75 ++++++++++++++++++++++++++++---- tests/sys/fs/fusefs/mockfs.cc | 9 +++- tests/sys/fs/fusefs/mockfs.hh | 2 +- tests/sys/fs/fusefs/utils.cc | 3 +- tests/sys/fs/fusefs/utils.hh | 2 + 11 files changed, 95 insertions(+), 18 deletions(-) diff --git a/sbin/mount_fusefs/mount_fusefs.8 b/sbin/mount_fusefs/mount_fusefs.8 index 821d0a44f741..a85ab11e7211 100644 --- a/sbin/mount_fusefs/mount_fusefs.8 +++ b/sbin/mount_fusefs/mount_fusefs.8 @@ -34,7 +34,7 @@ .\" .\" $FreeBSD$ .\" -.Dd June 14, 2019 +.Dd July 18, 2019 .Dt MOUNT_FUSEFS 8 .Os .Sh NAME @@ -150,6 +150,11 @@ I/O to the file system may be done asynchronously. Writes may delayed and/or reordered. .It Cm default_permissions Enable traditional (file mode based) permission checking in kernel +.It Cm intr +Allow signals to interrupt operations that are blocked waiting for a reply from the server. +When this option is in use, system calls may fail with +.Er EINTR +whenever a signal is received. .It Cm max_read Ns = Ns Ar n Limit size of read requests to .Ar n diff --git a/sbin/mount_fusefs/mount_fusefs.c b/sbin/mount_fusefs/mount_fusefs.c index fecab6f2ac54..3103d36fa332 100644 --- a/sbin/mount_fusefs/mount_fusefs.c +++ b/sbin/mount_fusefs/mount_fusefs.c @@ -84,6 +84,8 @@ static struct mntopt mopts[] = { */ #define ALTF_AUTOMOUNTED 0x100 { "automounted", 0, ALTF_AUTOMOUNTED, 1 }, + #define ALTF_INTR 0x200 + { "intr", 0, ALTF_INTR, 1 }, /* Linux specific options, we silently ignore them */ { "fsname=", 0, 0x00, 1 }, { "fd=", 0, 0x00, 1 }, @@ -469,6 +471,7 @@ helpmsg(void) " -o allow_other allow access to other users\n" /* " -o nonempty allow mounts over non-empty file/dir\n" */ " -o default_permissions enable permission checking by kernel\n" + " -o intr interruptible mount\n" /* " -o fsname=NAME set filesystem name\n" " -o large_read issue large read requests (2.4 only)\n" diff --git a/sys/fs/fuse/fuse_ipc.c b/sys/fs/fuse/fuse_ipc.c index 85a3e3ea3bcb..a7e89337bd0c 100644 --- a/sys/fs/fuse/fuse_ipc.c +++ b/sys/fs/fuse/fuse_ipc.c @@ -438,10 +438,11 @@ fticket_wait_answer(struct fuse_ticket *ftick) struct thread *td = curthread; sigset_t blockedset, oldset; int err = 0, stops_deferred; - struct fuse_data *data; + struct fuse_data *data = ftick->tk_data; bool interrupted = false; - if (fsess_isimpl(ftick->tk_data->mp, FUSE_INTERRUPT)) { + if (fsess_isimpl(ftick->tk_data->mp, FUSE_INTERRUPT) && + data->dataflags & FSESS_INTR) { SIGEMPTYSET(blockedset); } else { /* Block all signals except (implicitly) SIGKILL */ @@ -456,7 +457,6 @@ fticket_wait_answer(struct fuse_ticket *ftick) if (fticket_answered(ftick)) { goto out; } - data = ftick->tk_data; if (fdata_get_dead(data)) { err = ENOTCONN; diff --git a/sys/fs/fuse/fuse_ipc.h b/sys/fs/fuse/fuse_ipc.h index 8a7b000b209c..281a0f357359 100644 --- a/sys/fs/fuse/fuse_ipc.h +++ b/sys/fs/fuse/fuse_ipc.h @@ -232,9 +232,10 @@ struct fuse_data { #define FSESS_ASYNC_READ 0x1000 /* allow multiple reads of some file */ #define FSESS_POSIX_LOCKS 0x2000 /* daemon supports POSIX locks */ #define FSESS_EXPORT_SUPPORT 0x10000 /* daemon supports NFS-style lookups */ +#define FSESS_INTR 0x20000 /* interruptible mounts */ #define FSESS_MNTOPTS_MASK ( \ FSESS_DAEMON_CAN_SPY | FSESS_PUSH_SYMLINKS_IN | \ - FSESS_DEFAULT_PERMISSIONS) + FSESS_DEFAULT_PERMISSIONS | FSESS_INTR) extern int fuse_data_cache_mode; diff --git a/sys/fs/fuse/fuse_vfsops.c b/sys/fs/fuse/fuse_vfsops.c index 8229163d88a9..fff2cca67e77 100644 --- a/sys/fs/fuse/fuse_vfsops.c +++ b/sys/fs/fuse/fuse_vfsops.c @@ -336,6 +336,7 @@ fuse_vfsop_mount(struct mount *mp) FUSE_FLAGOPT(allow_other, FSESS_DAEMON_CAN_SPY); FUSE_FLAGOPT(push_symlinks_in, FSESS_PUSH_SYMLINKS_IN); FUSE_FLAGOPT(default_permissions, FSESS_DEFAULT_PERMISSIONS); + FUSE_FLAGOPT(intr, FSESS_INTR); (void)vfs_scanopt(opts, "max_read=", "%u", &max_read); if (vfs_scanopt(opts, "timeout=", "%u", &daemon_timeout) == 1) { diff --git a/sys/sys/param.h b/sys/sys/param.h index 9a68762388f2..bf1dc5e6d0ca 100644 --- a/sys/sys/param.h +++ b/sys/sys/param.h @@ -60,7 +60,7 @@ * in the range 5 to 9. */ #undef __FreeBSD_version -#define __FreeBSD_version 1300034 /* Master, propagated to newvers */ +#define __FreeBSD_version 1300035 /* Master, propagated to newvers */ /* * __FreeBSD_kernel__ indicates that this system uses the kernel of FreeBSD, diff --git a/tests/sys/fs/fusefs/interrupt.cc b/tests/sys/fs/fusefs/interrupt.cc index 98841db77850..9a2e55241dad 100644 --- a/tests/sys/fs/fusefs/interrupt.cc +++ b/tests/sys/fs/fusefs/interrupt.cc @@ -185,6 +185,15 @@ void TearDown() { } }; +class Intr: public Interrupt {}; + +class Nointr: public Interrupt { + void SetUp() { + m_nointr = true; + Interrupt::SetUp(); + } +}; + static void* mkdir0(void* arg __unused) { ssize_t r; @@ -213,7 +222,7 @@ static void* read1(void* arg) { * complete should generate an EAGAIN response. */ /* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236530 */ -TEST_F(Interrupt, already_complete) +TEST_F(Intr, already_complete) { uint64_t ino = 42; pthread_t self; @@ -271,7 +280,7 @@ TEST_F(Interrupt, already_complete) * kernel should not attempt to interrupt any other operations on that mount * point. */ -TEST_F(Interrupt, enosys) +TEST_F(Intr, enosys) { uint64_t ino0 = 42, ino1 = 43;; uint64_t mkdir_unique; @@ -356,7 +365,7 @@ TEST_F(Interrupt, enosys) * complete the original operation whenever it damn well pleases. */ /* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236530 */ -TEST_F(Interrupt, ignore) +TEST_F(Intr, ignore) { uint64_t ino = 42; pthread_t self; @@ -392,7 +401,7 @@ TEST_F(Interrupt, ignore) * that hasn't yet been sent to userland can be interrupted without sending * FUSE_INTERRUPT, and will be automatically restarted. */ -TEST_F(Interrupt, in_kernel_restartable) +TEST_F(Intr, in_kernel_restartable) { const char FULLPATH1[] = "mountpoint/other_file.txt"; const char RELPATH1[] = "other_file.txt"; @@ -462,7 +471,7 @@ TEST_F(Interrupt, in_kernel_restartable) * without sending FUSE_INTERRUPT. If it's a non-restartable operation (write * or setextattr) it will return EINTR. */ -TEST_F(Interrupt, in_kernel_nonrestartable) +TEST_F(Intr, in_kernel_nonrestartable) { const char FULLPATH1[] = "mountpoint/other_file.txt"; const char RELPATH1[] = "other_file.txt"; @@ -533,7 +542,7 @@ TEST_F(Interrupt, in_kernel_nonrestartable) * return EINTR to userspace */ /* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236530 */ -TEST_F(Interrupt, in_progress) +TEST_F(Intr, in_progress) { pthread_t self; uint64_t mkdir_unique; @@ -563,7 +572,7 @@ TEST_F(Interrupt, in_progress) } /* Reads should also be interruptible */ -TEST_F(Interrupt, in_progress_read) +TEST_F(Intr, in_progress_read) { const char FULLPATH[] = "mountpoint/some_file.txt"; const char RELPATH[] = "some_file.txt"; @@ -601,8 +610,56 @@ TEST_F(Interrupt, in_progress_read) EXPECT_EQ(EINTR, errno); } +/* + * When mounted with -o nointr, fusefs will block signals while waiting for the + * server. + */ +TEST_F(Nointr, block) +{ + uint64_t ino = 42; + pthread_t self; + sem_t sem0; + + ASSERT_EQ(0, sem_init(&sem0, 0, 0)) << strerror(errno); + signaled_semaphore = &sem0; + self = pthread_self(); + + EXPECT_LOOKUP(FUSE_ROOT_ID, RELDIRPATH0) + .WillOnce(Invoke(ReturnErrno(ENOENT))); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_MKDIR); + }, Eq(true)), + _) + ).WillOnce(Invoke(ReturnImmediate([&](auto in __unused, auto& out) { + /* Let the killer proceed */ + sem_post(blocked_semaphore); + + /* Wait until after the signal has been sent */ + sem_wait(signaled_semaphore); + /* Allow time for the mkdir thread to receive the signal */ + nap(); + + /* Finally, complete the original op */ + SET_OUT_HEADER_LEN(out, entry); + out.body.create.entry.attr.mode = S_IFDIR | MODE; + out.body.create.entry.nodeid = ino; + }))); + EXPECT_CALL(*m_mock, process( + ResultOf([&](auto in) { + return (in.header.opcode == FUSE_INTERRUPT); + }, Eq(true)), + _) + ).Times(0); + + setup_interruptor(self); + ASSERT_EQ(0, mkdir(FULLDIRPATH0, MODE)) << strerror(errno); + + sem_destroy(&sem0); +} + /* FUSE_INTERRUPT operations should take priority over other pending ops */ -TEST_F(Interrupt, priority) +TEST_F(Intr, priority) { Sequence seq; uint64_t ino1 = 43; @@ -688,7 +745,7 @@ TEST_F(Interrupt, priority) * successfully interrupts the original */ /* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236530 */ -TEST_F(Interrupt, too_soon) +TEST_F(Intr, too_soon) { Sequence seq; pthread_t self; diff --git a/tests/sys/fs/fusefs/mockfs.cc b/tests/sys/fs/fusefs/mockfs.cc index f219cb3f9546..f992387d272e 100644 --- a/tests/sys/fs/fusefs/mockfs.cc +++ b/tests/sys/fs/fusefs/mockfs.cc @@ -348,7 +348,7 @@ void MockFS::debug_response(const mockfs_buf_out &out) { MockFS::MockFS(int max_readahead, bool allow_other, bool default_permissions, bool push_symlinks_in, bool ro, enum poll_method pm, uint32_t flags, uint32_t kernel_minor_version, uint32_t max_write, bool async, - bool noclusterr, unsigned time_gran) + bool noclusterr, unsigned time_gran, bool nointr) { struct sigaction sa; struct iovec *iov = NULL; @@ -426,6 +426,13 @@ MockFS::MockFS(int max_readahead, bool allow_other, bool default_permissions, build_iovec(&iov, &iovlen, "noclusterr", __DECONST(void*, &trueval), sizeof(bool)); } + if (nointr) { + build_iovec(&iov, &iovlen, "nointr", + __DECONST(void*, &trueval), sizeof(bool)); + } else { + build_iovec(&iov, &iovlen, "intr", + __DECONST(void*, &trueval), sizeof(bool)); + } if (nmount(iov, iovlen, 0)) throw(std::system_error(errno, std::system_category(), "Couldn't mount filesystem")); diff --git a/tests/sys/fs/fusefs/mockfs.hh b/tests/sys/fs/fusefs/mockfs.hh index 91bb248c6f80..ccf3d915bdb5 100644 --- a/tests/sys/fs/fusefs/mockfs.hh +++ b/tests/sys/fs/fusefs/mockfs.hh @@ -323,7 +323,7 @@ class MockFS { bool default_permissions, bool push_symlinks_in, bool ro, enum poll_method pm, uint32_t flags, uint32_t kernel_minor_version, uint32_t max_write, bool async, - bool no_clusterr, unsigned time_gran); + bool no_clusterr, unsigned time_gran, bool nointr); virtual ~MockFS(); diff --git a/tests/sys/fs/fusefs/utils.cc b/tests/sys/fs/fusefs/utils.cc index cca5d3582c2c..c2451fc8bdc8 100644 --- a/tests/sys/fs/fusefs/utils.cc +++ b/tests/sys/fs/fusefs/utils.cc @@ -117,7 +117,8 @@ void FuseTest::SetUp() { m_mock = new MockFS(m_maxreadahead, m_allow_other, m_default_permissions, m_push_symlinks_in, m_ro, m_pm, m_init_flags, m_kernel_minor_version, - m_maxwrite, m_async, m_noclusterr, m_time_gran); + m_maxwrite, m_async, m_noclusterr, m_time_gran, + m_nointr); /* * FUSE_ACCESS is called almost universally. Expecting it in * each test case would be super-annoying. Instead, set a diff --git a/tests/sys/fs/fusefs/utils.hh b/tests/sys/fs/fusefs/utils.hh index 50b101e378c6..6fe870c091f6 100644 --- a/tests/sys/fs/fusefs/utils.hh +++ b/tests/sys/fs/fusefs/utils.hh @@ -57,6 +57,7 @@ class FuseTest : public ::testing::Test { bool m_ro; bool m_async; bool m_noclusterr; + bool m_nointr; unsigned m_time_gran; MockFS *m_mock = NULL; const static uint64_t FH = 0xdeadbeef1a7ebabe; @@ -77,6 +78,7 @@ class FuseTest : public ::testing::Test { m_ro(false), m_async(false), m_noclusterr(false), + m_nointr(false), m_time_gran(1) {}