Date: Tue, 03 Sep 2019 14:07:06 -0000 From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r346357 - in projects/fuse2: sys/fs/fuse tests/sys/fs/fusefs Message-ID: <201904181916.x3IJGZfe025435@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Thu Apr 18 19:16:34 2019 New Revision: 346357 URL: https://svnweb.freebsd.org/changeset/base/346357 Log: fusefs: improvements to interruptibility * If a process receives a fatal signal while blocked on a fuse operation, return ASAP without waiting for the operation to complete. But still send the FUSE_INTERRUPT op to the daemon. * Plug memory leaks from r346339 Interruptibility is now fully functional, but it could be better: * Operations that haven't been sent to the server yet should be aborted without sending FUSE_INTERRUPT. * It would be great if write operations could be made restartable. That would require delaying uiomove until the last possible moment, which would be sometime during fuse_device_read. * It would be nice if we didn't have to guess which EAGAIN responses were for FUSE_INTERRUPT operations. PR: 236530 Sponsored by: The FreeBSD Foundation Modified: projects/fuse2/sys/fs/fuse/fuse_device.c projects/fuse2/sys/fs/fuse/fuse_ipc.c projects/fuse2/sys/fs/fuse/fuse_ipc.h projects/fuse2/tests/sys/fs/fusefs/allow_other.cc projects/fuse2/tests/sys/fs/fusefs/interrupt.cc projects/fuse2/tests/sys/fs/fusefs/open.cc projects/fuse2/tests/sys/fs/fusefs/utils.cc projects/fuse2/tests/sys/fs/fusefs/utils.hh Modified: projects/fuse2/sys/fs/fuse/fuse_device.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_device.c Thu Apr 18 19:04:07 2019 (r346356) +++ projects/fuse2/sys/fs/fuse/fuse_device.c Thu Apr 18 19:16:34 2019 (r346357) @@ -360,7 +360,7 @@ fuse_device_write(struct cdev *dev, struct uio *uio, i struct fuse_out_header ohead; int err = 0; struct fuse_data *data; - struct fuse_ticket *tick, *x_tick; + struct fuse_ticket *tick, *itick, *x_tick; int found = 0; err = devfs_get_cdevpriv((void **)&data); @@ -401,6 +401,20 @@ fuse_device_write(struct cdev *dev, struct uio *uio, i break; } } + if (found && tick->irq_unique > 0) { + /* + * Discard the FUSE_INTERRUPT ticket that tried to interrupt + * this operation + */ + TAILQ_FOREACH_SAFE(itick, &data->aw_head, tk_aw_link, + x_tick) { + if (itick->tk_unique == tick->irq_unique) { + fuse_aw_remove(itick); + break; + } + } + tick->irq_unique = 0; + } fuse_lck_mtx_unlock(data->aw_mtx); if (found) { @@ -433,7 +447,20 @@ fuse_device_write(struct cdev *dev, struct uio *uio, i /* no callback at all! */ SDT_PROBE1(fuse, , device, fuse_device_write_missing_ticket, ohead.unique); - err = EINVAL; + if (ohead.error == EAGAIN) { + /* + * This was probably a response to a FUSE_INTERRUPT + * operation whose original operation is already + * complete. We can't store FUSE_INTERRUPT tickets + * indefinitely because their responses are optional. + * So we delete them when the original operation + * completes. And sadly the fuse_header_out doesn't + * identify the opcode, so we have to guess. + */ + err = 0; + } else { + err = EINVAL; + } } return (err); Modified: projects/fuse2/sys/fs/fuse/fuse_ipc.c ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_ipc.c Thu Apr 18 19:04:07 2019 (r346356) +++ projects/fuse2/sys/fs/fuse/fuse_ipc.c Thu Apr 18 19:16:34 2019 (r346357) @@ -138,6 +138,32 @@ static uma_zone_t ticket_zone; static int fuse_interrupt_callback(struct fuse_ticket *tick, struct uio *uio) { + struct fuse_ticket *otick, *x_tick; + struct fuse_interrupt_in *fii; + struct fuse_data *data; + data = tick->tk_data; + bool found = false; + + fii = (struct fuse_interrupt_in*)((char*)tick->tk_ms_fiov.base + + sizeof(struct fuse_in_header)); + + fuse_lck_mtx_lock(data->aw_mtx); + TAILQ_FOREACH_SAFE(otick, &data->aw_head, tk_aw_link, x_tick) { + if (otick->tk_unique == fii->unique) { + found = true; + break; + } + } + fuse_lck_mtx_unlock(data->aw_mtx); + + if (!found) { + /* Original is already complete. Just return */ + return 0; + } + + /* Clear the original ticket's interrupt association */ + otick->irq_unique = 0; + if (tick->tk_aw_ohead.error == EAGAIN) { /* * There are two reasons we might get this: @@ -148,28 +174,8 @@ fuse_interrupt_callback(struct fuse_ticket *tick, stru * In the first case we should re-send the INTERRUPT. In the * second, we should ignore it. */ - struct fuse_interrupt_in *fii; - struct fuse_data *data; - struct fuse_ticket *otick, *x_tick; - bool found = false; - - data = tick->tk_data; - fii = (struct fuse_interrupt_in*)((char*)tick->tk_ms_fiov.base + - sizeof(struct fuse_in_header)); - fuse_lck_mtx_lock(data->aw_mtx); - TAILQ_FOREACH_SAFE(otick, &data->aw_head, tk_aw_link, x_tick) { - if (otick->tk_unique == fii->unique) { - found = true; - break; - } - } - fuse_lck_mtx_unlock(data->aw_mtx); - if (found) { - /* Resend */ - fuse_interrupt_send(otick); - } else { - /* Original is already complete; nothing to do */ - } + /* Resend */ + fuse_interrupt_send(otick); return 0; } else { /* Illegal FUSE_INTERRUPT response */ @@ -186,19 +192,24 @@ fuse_interrupt_send(struct fuse_ticket *otick) struct fuse_data *data = otick->tk_data; struct ucred reused_creds; - ftick_hdr = fticket_in_header(otick); - reused_creds.cr_uid = ftick_hdr->uid; - reused_creds.cr_rgid = ftick_hdr->gid; - fdisp_init(&fdi, sizeof(*fii)); - fdisp_make_pid(&fdi, FUSE_INTERRUPT, data, ftick_hdr->nodeid, - ftick_hdr->pid, &reused_creds); + if (otick->irq_unique == 0) { + ftick_hdr = fticket_in_header(otick); + reused_creds.cr_uid = ftick_hdr->uid; + reused_creds.cr_rgid = ftick_hdr->gid; + fdisp_init(&fdi, sizeof(*fii)); + fdisp_make_pid(&fdi, FUSE_INTERRUPT, data, ftick_hdr->nodeid, + ftick_hdr->pid, &reused_creds); - fii = fdi.indata; - fii->unique = otick->tk_unique; - fuse_insert_callback(fdi.tick, fuse_interrupt_callback); + fii = fdi.indata; + fii->unique = otick->tk_unique; + fuse_insert_callback(fdi.tick, fuse_interrupt_callback); - fuse_insert_message(fdi.tick); - fdisp_destroy(&fdi); + otick->irq_unique = fdi.tick->tk_unique; + fuse_insert_message(fdi.tick); + fdisp_destroy(&fdi); + } else { + /* This ticket has already been interrupted */ + } } void @@ -278,6 +289,8 @@ fticket_ctor(void *mem, int size, void *arg, int flags if (ftick->tk_unique == 0) ftick->tk_unique = atomic_fetchadd_long(&data->ticketer, 1); + ftick->irq_unique = 0; + refcount_init(&ftick->tk_refcount, 1); atomic_add_acq_int(&fuse_ticket_count, 1); @@ -436,20 +449,23 @@ retry: fuse_interrupt_send(ftick); fuse_lck_mtx_lock(ftick->tk_aw_mtx); - /* TODO: return, rather than retry, for fatal signals */ - - /* - * Block the just-delivered signal while we wait for an - * interrupt response - */ PROC_LOCK(td->td_proc); mtx_lock(&td->td_proc->p_sigacts->ps_mtx); sig = cursig(td); mtx_unlock(&td->td_proc->p_sigacts->ps_mtx); PROC_UNLOCK(td->td_proc); - SIGADDSET(blockedset, sig); - kern_sigprocmask(curthread, SIG_BLOCK, &blockedset, NULL, 0); - goto retry; + if (!sig_isfatal(td->td_proc, sig)) { + /* + * Block the just-delivered signal while we wait for an + * interrupt response + */ + SIGADDSET(blockedset, sig); + kern_sigprocmask(curthread, SIG_BLOCK, &blockedset, + NULL, 0); + goto retry; + } else { + /* Return immediately for fatal signals */ + } } else if (err) { SDT_PROBE2(fuse, , ipc, trace, 6, "fticket_wait_answer: other error"); Modified: projects/fuse2/sys/fs/fuse/fuse_ipc.h ============================================================================== --- projects/fuse2/sys/fs/fuse/fuse_ipc.h Thu Apr 18 19:04:07 2019 (r346356) +++ projects/fuse2/sys/fs/fuse/fuse_ipc.h Thu Apr 18 19:16:34 2019 (r346357) @@ -103,6 +103,12 @@ struct fuse_ticket { struct fuse_data *tk_data; int tk_flag; u_int tk_refcount; + /* + * If this ticket's operation has been interrupted, this will hold the + * unique value of the FUSE_INTERRUPT operation. Otherwise, it will be + * 0. + */ + uint64_t irq_unique; /* fields for initiating an upgoing message */ struct fuse_iov tk_ms_fiov; Modified: projects/fuse2/tests/sys/fs/fusefs/allow_other.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/allow_other.cc Thu Apr 18 19:04:07 2019 (r346356) +++ projects/fuse2/tests/sys/fs/fusefs/allow_other.cc Thu Apr 18 19:16:34 2019 (r346357) @@ -36,6 +36,7 @@ extern "C" { #include <sys/types.h> #include <sys/extattr.h> +#include <sys/wait.h> #include <fcntl.h> #include <unistd.h> } @@ -74,7 +75,9 @@ virtual void SetUp() { TEST_F(AllowOther, allowed) { - fork(true, [&] { + int status; + + fork(true, &status, [&] { uint64_t ino = 42; expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 1); @@ -92,6 +95,7 @@ TEST_F(AllowOther, allowed) return 0; } ); + ASSERT_EQ(0, WEXITSTATUS(status)); } /* @@ -104,12 +108,12 @@ TEST_F(AllowOther, privilege_escalation) { const static char FULLPATH[] = "mountpoint/some_file.txt"; const static char RELPATH[] = "some_file.txt"; - int fd1; + int fd1, status; const static uint64_t ino = 42; const static uint64_t fh = 100; /* Fork a child to open the file with different credentials */ - fork(true, [&] { + fork(true, &status, [&] { expect_lookup(RELPATH, ino, S_IFREG | 0600, 0, 2); EXPECT_CALL(*m_mock, process( @@ -156,12 +160,15 @@ TEST_F(AllowOther, privilege_escalation) return 0; } ); + ASSERT_EQ(0, WEXITSTATUS(status)); /* Deliberately leak fd1. close(2) will be tested in release.cc */ } TEST_F(NoAllowOther, disallowed) { - fork(true, [] { + int status; + + fork(true, &status, [] { }, []() { int fd; @@ -177,6 +184,7 @@ TEST_F(NoAllowOther, disallowed) return 0; } ); + ASSERT_EQ(0, WEXITSTATUS(status)); } /* @@ -191,7 +199,7 @@ TEST_F(NoAllowOther, disallowed_beneath_root) const static char RELPATH2[] = "other_dir"; const static uint64_t ino = 42; const static uint64_t ino2 = 43; - int dfd; + int dfd, status; expect_lookup(RELPATH, ino, S_IFDIR | 0755, 0, 1); EXPECT_LOOKUP(ino, RELPATH2) @@ -206,7 +214,7 @@ TEST_F(NoAllowOther, disallowed_beneath_root) dfd = open(FULLPATH, O_DIRECTORY); ASSERT_LE(0, dfd) << strerror(errno); - fork(true, [] { + fork(true, &status, [] { }, [&]() { int fd; @@ -222,6 +230,7 @@ TEST_F(NoAllowOther, disallowed_beneath_root) return 0; } ); + ASSERT_EQ(0, WEXITSTATUS(status)); } /* @@ -230,9 +239,9 @@ TEST_F(NoAllowOther, disallowed_beneath_root) */ TEST_F(NoAllowOther, setextattr) { - int ino = 42; + int ino = 42, status; - fork(true, [&] { + fork(true, &status, [&] { EXPECT_LOOKUP(1, RELPATH) .WillOnce(Invoke( ReturnImmediate([=](auto in __unused, auto out) { @@ -268,4 +277,5 @@ TEST_F(NoAllowOther, setextattr) return 0; } ); + ASSERT_EQ(0, WEXITSTATUS(status)); } Modified: projects/fuse2/tests/sys/fs/fusefs/interrupt.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/interrupt.cc Thu Apr 18 19:04:07 2019 (r346356) +++ projects/fuse2/tests/sys/fs/fusefs/interrupt.cc Thu Apr 18 19:16:34 2019 (r346357) @@ -29,6 +29,7 @@ */ extern "C" { +#include <sys/wait.h> #include <fcntl.h> #include <pthread.h> #include <signal.h> @@ -68,10 +69,27 @@ Interrupt(): m_child(NULL) {}; void expect_lookup(const char *relpath, uint64_t ino) { - FuseTest::expect_lookup(relpath, ino, S_IFREG | 0644, 0, 1); + FuseTest::expect_lookup(relpath, ino, S_IFREG | 0644, 1000, 1); } /* + * Expect a FUSE_READ but don't reply. Instead, just record the unique value + * to the provided pointer + */ +void expect_read(uint64_t ino, uint64_t *read_unique) +{ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in->header.opcode == FUSE_READ && + in->header.nodeid == ino); + }, Eq(true)), + _) + ).WillOnce(Invoke([=](auto in, auto &out __unused) { + *read_unique = in->header.unique; + })); +} + +/* * Expect a FUSE_WRITE but don't reply. Instead, just record the unique value * to the provided pointer */ @@ -90,7 +108,7 @@ void expect_write(uint64_t ino, uint64_t *write_unique void setup_interruptor(pthread_t self) { - ASSERT_EQ(0, signal(SIGUSR2, sigusr2_handler)) << strerror(errno); + ASSERT_NE(SIG_ERR, signal(SIGUSR2, sigusr2_handler)) << strerror(errno); ASSERT_EQ(0, pthread_create(&m_child, NULL, killer, (void*)self)) << strerror(errno); } @@ -162,6 +180,74 @@ TEST_F(Interrupt, already_complete) } /* + * Upon receipt of a fatal signal, fusefs should return ASAP after sending + * FUSE_INTERRUPT. + */ +TEST_F(Interrupt, fatal_signal) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char *CONTENTS = "abcdefgh"; + const char RELPATH[] = "some_file.txt"; + ssize_t bufsize = strlen(CONTENTS); + uint64_t ino = 42; + int status; + pthread_t self; + uint64_t write_unique; + + self = pthread_self(); + + expect_lookup(RELPATH, ino); + expect_open(ino, 0, 1); + expect_write(ino, &write_unique); + EXPECT_CALL(*m_mock, process( + ResultOf([&](auto in) { + return (in->header.opcode == FUSE_INTERRUPT && + in->body.interrupt.unique == write_unique); + }, Eq(true)), + _) + ).WillOnce(Invoke([&](auto in __unused, auto &out __unused) { + /* Don't respond. The process should exit anyway */ + })); + expect_flush(ino, 1, ReturnErrno(0)); + expect_release(ino, FH); + + fork(false, &status, [&] { + }, [&]() { + struct sigaction sa; + int fd, r; + pthread_t killer_th; + pthread_t self; + + fd = open(FULLPATH, O_WRONLY); + if (fd < 0) { + perror("open"); + return 1; + } + + /* SIGUSR2 terminates the process by default */ + bzero(&sa, sizeof(sa)); + sa.sa_handler = SIG_DFL; + r = sigaction(SIGUSR2, &sa, NULL); + if (r != 0) { + perror("sigaction"); + return 1; + } + self = pthread_self(); + r = pthread_create(&killer_th, NULL, killer, (void*)self); + if (r != 0) { + perror("pthread_create"); + return 1; + } + + write(fd, CONTENTS, bufsize); + return 1; + }); + ASSERT_EQ(SIGUSR2, WTERMSIG(status)); + + /* Deliberately leak fd. close(2) will be tested in release.cc */ +} + +/* * A FUSE filesystem is legally allowed to ignore INTERRUPT operations, and * complete the original operation whenever it damn well pleases. */ @@ -253,6 +339,47 @@ TEST_F(Interrupt, in_progress) /* Deliberately leak fd. close(2) will be tested in release.cc */ } +/* Reads should also be interruptible */ +TEST_F(Interrupt, in_progress_read) +{ + const char FULLPATH[] = "mountpoint/some_file.txt"; + const char RELPATH[] = "some_file.txt"; + const size_t bufsize = 80; + char buf[bufsize]; + uint64_t ino = 42; + int fd; + pthread_t self; + uint64_t read_unique; + + self = pthread_self(); + + expect_lookup(RELPATH, ino); + expect_open(ino, 0, 1); + expect_read(ino, &read_unique); + EXPECT_CALL(*m_mock, process( + ResultOf([&](auto in) { + return (in->header.opcode == FUSE_INTERRUPT && + in->body.interrupt.unique == read_unique); + }, Eq(true)), + _) + ).WillOnce(Invoke([&](auto in __unused, auto &out) { + auto out0 = new mockfs_buf_out; + out0->header.error = -EINTR; + out0->header.unique = read_unique; + out0->header.len = sizeof(out0->header); + out.push_back(out0); + })); + + fd = open(FULLPATH, O_RDONLY); + ASSERT_LE(0, fd) << strerror(errno); + + setup_interruptor(self); + ASSERT_EQ(-1, read(fd, buf, bufsize)); + EXPECT_EQ(EINTR, errno); + + /* Deliberately leak fd. close(2) will be tested in release.cc */ +} + /* * If the FUSE filesystem receives the FUSE_INTERRUPT operation before * processing the original, then it should wait for "some timeout" for the @@ -320,9 +447,7 @@ TEST_F(Interrupt, too_soon) // TODO: add a test that uses siginterrupt and an interruptible signal // TODO: add a test that verifies a process can be cleanly killed even if a // FUSE_WRITE command never returns. -// TODO: write in-progress tests for read and other operations +// TODO: write in-progress tests for other operations // TODO: add a test where write returns EWOULDBLOCK -// TODO: test that if a fatal signal is received, fticket_wait_answer will -// return without waiting for a response to the interrupted operation. // TODO: test that operations that haven't been received by the server can be // interrupted without generating a FUSE_INTERRUPT. Modified: projects/fuse2/tests/sys/fs/fusefs/open.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/open.cc Thu Apr 18 19:04:07 2019 (r346356) +++ projects/fuse2/tests/sys/fs/fusefs/open.cc Thu Apr 18 19:16:34 2019 (r346357) @@ -29,6 +29,7 @@ */ extern "C" { +#include <sys/wait.h> #include <fcntl.h> } @@ -166,12 +167,12 @@ TEST_F(Open, multiple_creds) { const static char FULLPATH[] = "mountpoint/some_file.txt"; const static char RELPATH[] = "some_file.txt"; - int fd1; + int fd1, status; const static uint64_t ino = 42; const static uint64_t fh0 = 100, fh1 = 200; /* Fork a child to open the file with different credentials */ - fork(false, [&] { + fork(false, &status, [&] { expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 2); EXPECT_CALL(*m_mock, process( @@ -218,6 +219,7 @@ TEST_F(Open, multiple_creds) return 0; } ); + ASSERT_EQ(0, WEXITSTATUS(status)); close(fd1); } Modified: projects/fuse2/tests/sys/fs/fusefs/utils.cc ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/utils.cc Thu Apr 18 19:04:07 2019 (r346356) +++ projects/fuse2/tests/sys/fs/fusefs/utils.cc Thu Apr 18 19:16:34 2019 (r346357) @@ -336,7 +336,8 @@ get_unprivileged_uid(uid_t *uid) } void -FuseTest::fork(bool drop_privs, std::function<void()> parent_func, +FuseTest::fork(bool drop_privs, int *child_status, + std::function<void()> parent_func, std::function<int()> child_func) { sem_t *sem; @@ -376,8 +377,6 @@ out: sem_destroy(sem); _exit(err); } else if (child > 0) { - int child_status; - /* * In parent. Cleanup must happen here, because it's still * privileged. @@ -388,12 +387,12 @@ out: /* Signal the child process to go */ ASSERT_EQ(0, sem_post(sem)) << strerror(errno); - ASSERT_LE(0, wait(&child_status)) << strerror(errno); - ASSERT_EQ(0, WEXITSTATUS(child_status)); + ASSERT_LE(0, wait(child_status)) << strerror(errno); } else { FAIL() << strerror(errno); } munmap(sem, sizeof(*sem)); + return; } static void usage(char* progname) { Modified: projects/fuse2/tests/sys/fs/fusefs/utils.hh ============================================================================== --- projects/fuse2/tests/sys/fs/fusefs/utils.hh Thu Apr 18 19:04:07 2019 (r346356) +++ projects/fuse2/tests/sys/fs/fusefs/utils.hh Thu Apr 18 19:16:34 2019 (r346357) @@ -106,7 +106,7 @@ class FuseTest : public ::testing::Test { uid_t uid = 0); /* - * Create an expectation that FUSE_GETATTR will be called for the given + * Create an expectation that FUSE_OPEN will be called for the given * inode exactly times times. It will return with open_flags flags and * file handle FH. */ @@ -162,11 +162,11 @@ class FuseTest : public ::testing::Test { * * # Returns * - * fusetest_fork will FAIL the test if child_func returns nonzero. - * It may SKIP the test, which the caller should detect with the - * IsSkipped() method. + * fusetest_fork may SKIP the test, which the caller should detect with + * the IsSkipped() method. If not, then the child's exit status will + * be returned in status. */ - void fork(bool drop_privs, + void fork(bool drop_privs, int *status, std::function<void()> parent_func, std::function<int()> child_func); };
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904181916.x3IJGZfe025435>