Date: Wed, 28 Aug 2019 04:19:37 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r351560 - in head: sys/fs/fuse tests/sys/fs/fusefs Message-ID: <201908280419.x7S4JbW5095814@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: asomers Date: Wed Aug 28 04:19:37 2019 New Revision: 351560 URL: https://svnweb.freebsd.org/changeset/base/351560 Log: fusefs: Fix some bugs regarding the size of the LISTXATTR list * A small error in r338152 let to the returned size always being exactly eight bytes too large. * The FUSE_LISTXATTR operation works like Linux's listxattr(2): if the caller does not provide enough space, then the server should return ERANGE rather than return a truncated list. That's true even though in FUSE's case the kernel doesn't provide space to the client at all; it simply requests a maximum size for the list. We previously weren't handling the case where the server returns ERANGE even though the kernel requested as much size as the server had told us it needs; that can happen due to a race. * We also need to ensure that a pathological server that always returns ERANGE no matter what size we request in FUSE_LISTXATTR won't cause an infinite loop in the kernel. As of this commit, it will instead cause an infinite loop that exits and enters the kernel on each iteration, allowing signals to be processed. Reviewed by: cem MFC after: 2 weeks Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D21287 Modified: head/sys/fs/fuse/fuse_vnops.c head/tests/sys/fs/fusefs/mockfs.cc head/tests/sys/fs/fusefs/xattr.cc Modified: head/sys/fs/fuse/fuse_vnops.c ============================================================================== --- head/sys/fs/fuse/fuse_vnops.c Wed Aug 28 01:22:21 2019 (r351559) +++ head/sys/fs/fuse/fuse_vnops.c Wed Aug 28 04:19:37 2019 (r351560) @@ -2225,6 +2225,20 @@ fuse_xattrlist_convert(char *prefix, const char *list, } /* + * List extended attributes + * + * The FUSE_LISTXATTR operation is based on Linux's listxattr(2) syscall, which + * has a number of differences compared to its FreeBSD equivalent, + * extattr_list_file: + * + * - FUSE_LISTXATTR returns all extended attributes across all namespaces, + * whereas listxattr(2) only returns attributes for a single namespace + * - FUSE_LISTXATTR prepends each attribute name with "namespace." + * - If the provided buffer is not large enough to hold the result, + * FUSE_LISTXATTR should return ERANGE, whereas listxattr is expected to + * return as many results as will fit. + */ +/* struct vop_listextattr_args { struct vop_generic_args a_gen; struct vnode *a_vp; @@ -2303,14 +2317,31 @@ fuse_vnop_listextattr(struct vop_listextattr_args *ap) */ fdisp_refresh_vp(&fdi, FUSE_LISTXATTR, vp, td, cred); list_xattr_in = fdi.indata; - list_xattr_in->size = linux_list_len + sizeof(*list_xattr_out); + list_xattr_in->size = linux_list_len; err = fdisp_wait_answ(&fdi); - if (err != 0) + if (err == ERANGE) { + /* + * Race detected. The attribute list must've grown since the + * first FUSE_LISTXATTR call. Start over. Go all the way back + * to userland so we can process signals, if necessary, before + * restarting. + */ + err = ERESTART; goto out; + } else if (err != 0) + goto out; linux_list = fdi.answ; - linux_list_len = fdi.iosize; + /* FUSE doesn't allow the server to return more data than requested */ + if (fdi.iosize > linux_list_len) { + printf("WARNING: FUSE protocol violation. Server returned " + "more extended attribute data than requested; " + "should've returned ERANGE instead"); + } else { + /* But returning less data is fine */ + linux_list_len = fdi.iosize; + } /* * Retrieve the BSD compatible list values. Modified: head/tests/sys/fs/fusefs/mockfs.cc ============================================================================== --- head/tests/sys/fs/fusefs/mockfs.cc Wed Aug 28 01:22:21 2019 (r351559) +++ head/tests/sys/fs/fusefs/mockfs.cc Wed Aug 28 04:19:37 2019 (r351560) @@ -204,6 +204,9 @@ void MockFS::debug_request(const mockfs_buf_in &in) case FUSE_LINK: printf(" oldnodeid=%" PRIu64, in.body.link.oldnodeid); break; + case FUSE_LISTXATTR: + printf(" size=%" PRIu32, in.body.listxattr.size); + break; case FUSE_LOOKUP: printf(" %s", in.body.lookup); break; Modified: head/tests/sys/fs/fusefs/xattr.cc ============================================================================== --- head/tests/sys/fs/fusefs/xattr.cc Wed Aug 28 01:22:21 2019 (r351559) +++ head/tests/sys/fs/fusefs/xattr.cc Wed Aug 28 04:19:37 2019 (r351560) @@ -35,6 +35,9 @@ extern "C" { #include <sys/types.h> #include <sys/extattr.h> +#include <sys/wait.h> +#include <semaphore.h> +#include <signal.h> #include <string.h> } @@ -45,7 +48,18 @@ using namespace testing; const char FULLPATH[] = "mountpoint/some_file.txt"; const char RELPATH[] = "some_file.txt"; +static sem_t killer_semaphore; +void* killer(void* target) { + pid_t pid = *(pid_t*)target; + sem_wait(&killer_semaphore); + if (verbosity > 1) + printf("Killing! pid %d\n", pid); + kill(pid, SIGINT); + + return(NULL); +} + class Xattr: public FuseTest { public: void expect_getxattr(uint64_t ino, const char *attr, ProcessMockerT r) @@ -62,17 +76,31 @@ void expect_getxattr(uint64_t ino, const char *attr, P ).WillOnce(Invoke(r)); } -void expect_listxattr(uint64_t ino, uint32_t size, ProcessMockerT r) +void expect_listxattr(uint64_t ino, uint32_t size, ProcessMockerT r, + Sequence *seq = NULL) { - EXPECT_CALL(*m_mock, process( - ResultOf([=](auto in) { - return (in.header.opcode == FUSE_LISTXATTR && - in.header.nodeid == ino && - in.body.listxattr.size == size); - }, Eq(true)), - _) - ).WillOnce(Invoke(r)) - .RetiresOnSaturation(); + if (seq == NULL) { + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_LISTXATTR && + in.header.nodeid == ino && + in.body.listxattr.size == size); + }, Eq(true)), + _) + ).WillOnce(Invoke(r)) + .RetiresOnSaturation(); + } else { + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_LISTXATTR && + in.header.nodeid == ino && + in.body.listxattr.size == size); + }, Eq(true)), + _) + ).InSequence(*seq) + .WillOnce(Invoke(r)) + .RetiresOnSaturation(); + } } void expect_removexattr(uint64_t ino, const char *attr, int error) @@ -108,7 +136,37 @@ void expect_setxattr(uint64_t ino, const char *attr, c }; class Getxattr: public Xattr {}; + class Listxattr: public Xattr {}; + +/* Listxattr tests that need to use a signal */ +class ListxattrSig: public Listxattr { +public: +pthread_t m_killer_th; +pid_t m_child; + +void SetUp() { + /* + * Mount with -o nointr so the mount can't get interrupted while + * waiting for a response from the server + */ + m_nointr = true; + FuseTest::SetUp(); + + ASSERT_EQ(0, sem_init(&killer_semaphore, 0, 0)) << strerror(errno); +} + +void TearDown() { + if (m_killer_th != NULL) { + pthread_join(m_killer_th, NULL); + } + + sem_destroy(&killer_semaphore); + + FuseTest::TearDown(); +} +}; + class Removexattr: public Xattr {}; class Setxattr: public Xattr {}; class RofsXattr: public Xattr { @@ -175,6 +233,7 @@ TEST_F(Getxattr, enosys) * behavior. * * This test case covers a filesystem that uses the Linux behavior + * TODO: require FreeBSD Behavior. */ TEST_F(Getxattr, erange) { @@ -300,29 +359,111 @@ TEST_F(Listxattr, enotsup) } /* - * On FreeBSD, if the user passes an insufficiently large buffer then the - * filesystem is supposed to copy as much of the attribute's value as will fit. + * On FreeBSD, if the user passes an insufficiently large buffer to + * extattr_list_file(2) or VOP_LISTEXTATTR(9), then the file system is supposed + * to copy as much of the attribute's value as will fit. * - * On Linux, however, the filesystem is supposed to return ERANGE. + * On Linux, however, the file system is supposed to return ERANGE if an + * insufficiently large buffer is passed to listxattr(2). * - * libfuse specifies the Linux behavior. However, that's probably an error. - * It would probably be correct for the filesystem to use platform-dependent - * behavior. - * - * This test case covers a filesystem that uses the Linux behavior + * fusefs(5) must guarantee the usual FreeBSD behavior. */ TEST_F(Listxattr, erange) { uint64_t ino = 42; int ns = EXTATTR_NAMESPACE_USER; + char attrs[9] = "user.foo"; + char expected[3] = {3, 'f', 'o'}; + char buf[3]; expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 1); - expect_listxattr(ino, 0, ReturnErrno(ERANGE)); + expect_listxattr(ino, 0, ReturnImmediate([](auto i __unused, auto& out) + { + out.body.listxattr.size = sizeof(attrs); + SET_OUT_HEADER_LEN(out, listxattr); + })); + expect_listxattr(ino, sizeof(attrs), + ReturnImmediate([&](auto in __unused, auto& out) { + memcpy((void*)out.body.bytes, attrs, sizeof(attrs)); + out.header.len = sizeof(fuse_out_header) + sizeof(attrs); + })); - ASSERT_EQ(-1, extattr_list_file(FULLPATH, ns, NULL, 0)); - ASSERT_EQ(ERANGE, errno); + + ASSERT_EQ(static_cast<ssize_t>(sizeof(buf)), + extattr_list_file(FULLPATH, ns, buf, sizeof(buf))); + ASSERT_EQ(0, memcmp(expected, buf, sizeof(buf))); } +/* + * A buggy or malicious file system always returns ERANGE, even if we pass an + * appropriately sized buffer. That will send the kernel into an infinite + * loop. This test will ensure that the loop is interruptible by killing the + * blocked process with SIGINT. + */ +TEST_F(ListxattrSig, erange_forever) +{ + uint64_t ino = 42; + uint32_t lie_size = 10; + int status; + + fork(false, &status, [&] { + 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 = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_LISTXATTR && + in.header.nodeid == ino && + in.body.listxattr.size == 0); + }, Eq(true)), + _) + ).WillRepeatedly(ReturnImmediate([=](auto i __unused, auto& out) + { + /* The file system requests 10 bytes, but it's a lie */ + out.body.listxattr.size = lie_size; + SET_OUT_HEADER_LEN(out, listxattr); + /* + * We can send the signal any time after fusefs enters + * VOP_LISTEXTATTR + */ + sem_post(&killer_semaphore); + })); + /* + * Even though the kernel faithfully respects our size request, + * we'll return ERANGE anyway. + */ + EXPECT_CALL(*m_mock, process( + ResultOf([=](auto in) { + return (in.header.opcode == FUSE_LISTXATTR && + in.header.nodeid == ino && + in.body.listxattr.size == lie_size); + }, Eq(true)), + _) + ).WillRepeatedly(ReturnErrno(ERANGE)); + + ASSERT_EQ(0, pthread_create(&m_killer_th, NULL, killer, + &m_mock->m_child_pid)) + << strerror(errno); + + }, [] { + /* Child process will block until it gets signaled */ + int ns = EXTATTR_NAMESPACE_USER; + char buf[3]; + extattr_list_file(FULLPATH, ns, buf, sizeof(buf)); + return 0; + } + ); + + ASSERT_TRUE(WIFSIGNALED(status)); +} + /* * Get the size of the list that it would take to list no extended attributes */ @@ -351,21 +492,20 @@ TEST_F(Listxattr, size_only_nonempty) { uint64_t ino = 42; int ns = EXTATTR_NAMESPACE_USER; + char attrs[9] = "user.foo"; expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 1); - expect_listxattr(ino, 0, ReturnImmediate([](auto i __unused, auto& out) { - out.body.listxattr.size = 45; + expect_listxattr(ino, 0, ReturnImmediate([](auto i __unused, auto& out) + { + out.body.listxattr.size = sizeof(attrs); SET_OUT_HEADER_LEN(out, listxattr); })); - // TODO: fix the expected size after fixing the size calculation bug in - // fuse_vnop_listextattr. It should be exactly 45. - expect_listxattr(ino, 53, - ReturnImmediate([](auto in __unused, auto& out) { - const char l[] = "user.foo"; - strlcpy((char*)out.body.bytes, l, - sizeof(out.body.bytes)); - out.header.len = sizeof(fuse_out_header) + sizeof(l); + expect_listxattr(ino, sizeof(attrs), + ReturnImmediate([=](auto in __unused, auto& out) { + size_t l = sizeof(attrs); + strlcpy((char*)out.body.bytes, attrs, l); + out.header.len = sizeof(fuse_out_header) + l; }) ); @@ -373,6 +513,88 @@ TEST_F(Listxattr, size_only_nonempty) << strerror(errno); } +/* + * The list of extended attributes grows in between the server's two calls to + * FUSE_LISTXATTR. + */ +TEST_F(Listxattr, size_only_race_bigger) +{ + uint64_t ino = 42; + int ns = EXTATTR_NAMESPACE_USER; + char attrs0[9] = "user.foo"; + char attrs1[18] = "user.foo\0user.bar"; + Sequence seq; + + 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 = UINT64_MAX; + out.body.entry.entry_valid = UINT64_MAX; + }))); + expect_listxattr(ino, 0, ReturnImmediate([](auto i __unused, auto& out) + { + out.body.listxattr.size = sizeof(attrs0); + SET_OUT_HEADER_LEN(out, listxattr); + }), &seq); + + /* + * After the first FUSE_LISTXATTR the list grew, so the second + * operation returns ERANGE. + */ + expect_listxattr(ino, sizeof(attrs0), ReturnErrno(ERANGE), &seq); + + /* And now the kernel retries */ + expect_listxattr(ino, 0, ReturnImmediate([](auto i __unused, auto& out) + { + out.body.listxattr.size = sizeof(attrs1); + SET_OUT_HEADER_LEN(out, listxattr); + }), &seq); + expect_listxattr(ino, sizeof(attrs1), + ReturnImmediate([&](auto in __unused, auto& out) { + memcpy((char*)out.body.bytes, attrs1, sizeof(attrs1)); + out.header.len = sizeof(fuse_out_header) + + sizeof(attrs1); + }), &seq + ); + + /* Userspace should never know about the retry */ + ASSERT_EQ(8, extattr_list_file(FULLPATH, ns, NULL, 0)) + << strerror(errno); +} + +/* + * The list of extended attributes shrinks in between the server's two calls to + * FUSE_LISTXATTR + */ +TEST_F(Listxattr, size_only_race_smaller) +{ + uint64_t ino = 42; + int ns = EXTATTR_NAMESPACE_USER; + char attrs0[18] = "user.foo\0user.bar"; + char attrs1[9] = "user.foo"; + + expect_lookup(RELPATH, ino, S_IFREG | 0644, 0, 1); + expect_listxattr(ino, 0, ReturnImmediate([](auto i __unused, auto& out) + { + out.body.listxattr.size = sizeof(attrs0); + SET_OUT_HEADER_LEN(out, listxattr); + })); + expect_listxattr(ino, sizeof(attrs0), + ReturnImmediate([&](auto in __unused, auto& out) { + strlcpy((char*)out.body.bytes, attrs1, sizeof(attrs1)); + out.header.len = sizeof(fuse_out_header) + + sizeof(attrs1); + }) + ); + + ASSERT_EQ(4, extattr_list_file(FULLPATH, ns, NULL, 0)) + << strerror(errno); +} + TEST_F(Listxattr, size_only_really_big) { uint64_t ino = 42; @@ -384,9 +606,7 @@ TEST_F(Listxattr, size_only_really_big) SET_OUT_HEADER_LEN(out, listxattr); })); - // TODO: fix the expected size after fixing the size calculation bug in - // fuse_vnop_listextattr. It should be exactly 16000. - expect_listxattr(ino, 16008, + expect_listxattr(ino, 16000, ReturnImmediate([](auto in __unused, auto& out) { const char l[16] = "user.foobarbang"; for (int i=0; i < 1000; i++) { @@ -420,9 +640,7 @@ TEST_F(Listxattr, user) }) ); - // TODO: fix the expected size after fixing the size calculation bug in - // fuse_vnop_listextattr. - expect_listxattr(ino, sizeof(attrs) + 8, + expect_listxattr(ino, sizeof(attrs), ReturnImmediate([&](auto in __unused, auto& out) { memcpy((void*)out.body.bytes, attrs, sizeof(attrs)); out.header.len = sizeof(fuse_out_header) + sizeof(attrs); @@ -454,9 +672,7 @@ TEST_F(Listxattr, system) }) ); - // TODO: fix the expected size after fixing the size calculation bug in - // fuse_vnop_listextattr. - expect_listxattr(ino, sizeof(attrs) + 8, + expect_listxattr(ino, sizeof(attrs), ReturnImmediate([&](auto in __unused, auto& out) { memcpy((void*)out.body.bytes, attrs, sizeof(attrs)); out.header.len = sizeof(fuse_out_header) + sizeof(attrs);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201908280419.x7S4JbW5095814>