d/freebsd-nq
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fusefs: Fix some bugs regarding the size of the LISTXATTR list

Browse Source 
* 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
This commit is contained in:
Alan Somers 2019-08-28 04:19:37 +00:00
parent 2319489b6e
commit 5e63333052
3 changed files with 293 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
sys/fs/fuse/fuse_vnops.c
View File

@ -2224,6 +2224,20 @@ fuse_xattrlist_convert(char *prefix, const char *list, int list_len,
return (0);
}
/*
* 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;
@ -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.

3
tests/sys/fs/fusefs/mockfs.cc
View File

@ -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;

296
tests/sys/fs/fusefs/xattr.cc
View File

@ -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,6 +48,17 @@ 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:
@ -62,17 +76,31 @@ void expect_getxattr(uint64_t ino, const char *attr, ProcessMockerT r)
).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, const char *value,
};
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,27 +359,109 @@ 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));
}
/*
@ -351,21 +492,102 @@ 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;
})
);
ASSERT_EQ(4, extattr_list_file(FULLPATH, ns, NULL, 0))
<< 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);
})
);
@ -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);