Page MenuHomeFreeBSD
Files F147392085

D21287.id61388.diff
No OneTemporary
Actions

D21287.id61388.diff
View Options

Index: head/sys/fs/fuse/fuse_vnops.c
===================================================================
--- head/sys/fs/fuse/fuse_vnops.c
+++ head/sys/fs/fuse/fuse_vnops.c
@@ -2225,6 +2225,20 @@
}
/*
+ * 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 @@
*/
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.
Index: head/tests/sys/fs/fusefs/mockfs.cc
===================================================================
--- head/tests/sys/fs/fusefs/mockfs.cc
+++ head/tests/sys/fs/fusefs/mockfs.cc
@@ -204,6 +204,9 @@
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;
Index: head/tests/sys/fs/fusefs/xattr.cc
===================================================================
--- head/tests/sys/fs/fusefs/xattr.cc
+++ head/tests/sys/fs/fusefs/xattr.cc
@@ -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 @@
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 @@
).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 @@
};
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 @@
* behavior.
*
* This test case covers a filesystem that uses the Linux behavior
+ * TODO: require FreeBSD Behavior.
*/
TEST_F(Getxattr, erange)
{
@@ -300,29 +359,111 @@
}
/*
- * 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 @@
{
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 @@
<< 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 @@
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 @@
})
);
- // 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 @@
})
);
- // 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);

File Metadata

Mime Type
text/plain
Expires
Wed, Mar 11, 2:38 PM (11 h, 23 m)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
29541771
Default Alt Text
D21287.id61388.diff (13 KB)

Event Timeline