svn commit: r346135 - in projects/fuse2: sys/fs/fuse tests/sys/fs/fusefs
Alan Somers
asomers at FreeBSD.org
Tue Sep 3 14:06:43 UTC 2019
Author: asomers
Date: Thu Apr 11 21:00:40 2019
New Revision: 346135
URL: https://svnweb.freebsd.org/changeset/base/346135
Log:
fusefs: Finish supporting -o default_permissions
I got most of -o default_permissions working in r346088. This commit adds
sticky bit checks. One downside is that sometimes there will be an extra
FUSE_GETATTR call for the parent directory during unlink or rename. But in
actual use I think those attributes will almost always be cached.
PR: 216391
Sponsored by: The FreeBSD Foundation
Modified:
projects/fuse2/sys/fs/fuse/fuse_node.h
projects/fuse2/sys/fs/fuse/fuse_vnops.c
projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc
projects/fuse2/tests/sys/fs/fusefs/destroy.cc
projects/fuse2/tests/sys/fs/fusefs/rename.cc
projects/fuse2/tests/sys/fs/fusefs/rmdir.cc
projects/fuse2/tests/sys/fs/fusefs/unlink.cc
Modified: projects/fuse2/sys/fs/fuse/fuse_node.h
==============================================================================
--- projects/fuse2/sys/fs/fuse/fuse_node.h Thu Apr 11 20:39:12 2019 (r346134)
+++ projects/fuse2/sys/fs/fuse/fuse_node.h Thu Apr 11 21:00:40 2019 (r346135)
@@ -90,6 +90,7 @@ struct fuse_vnode_data {
/* The monotonic time after which the attr cache is invalid */
struct bintime attr_cache_timeout;
struct vattr cached_attrs;
+ /* TODO: use cached_attrs.size instead */
off_t filesize;
uint64_t nlookup;
enum vtype vtype;
Modified: projects/fuse2/sys/fs/fuse/fuse_vnops.c
==============================================================================
--- projects/fuse2/sys/fs/fuse/fuse_vnops.c Thu Apr 11 20:39:12 2019 (r346134)
+++ projects/fuse2/sys/fs/fuse/fuse_vnops.c Thu Apr 11 21:00:40 2019 (r346135)
@@ -797,6 +797,11 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
/* Cache timeout */
atomic_add_acq_long(&fuse_lookup_cache_misses,
1);
+ /*
+ * XXX is fuse_internal_vnode_disappear ok to
+ * call if another process is still using the
+ * vnode?
+ */
fuse_internal_vnode_disappear(*vpp);
if (dvp != *vpp)
vput(*vpp);
@@ -864,99 +869,21 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
/* Entry not found */
if ((nameiop == CREATE || nameiop == RENAME) && islastcn) {
err = fuse_internal_access(dvp, VWRITE, td, cred);
- if (err)
- goto out;
+ if (!err) {
+ /*
+ * Set the SAVENAME flag to hold onto the
+ * pathname for use later in VOP_CREATE or
+ * VOP_RENAME.
+ */
+ cnp->cn_flags |= SAVENAME;
- /*
- * Set the SAVENAME flag to hold onto the
- * pathname for use later in VOP_CREATE or VOP_RENAME.
- */
- cnp->cn_flags |= SAVENAME;
-
- err = EJUSTRETURN;
- goto out;
+ err = EJUSTRETURN;
+ }
+ } else {
+ err = ENOENT;
}
-
- err = ENOENT;
- goto out;
-
} else {
/* Entry was found */
- if ((nameiop == DELETE || nameiop == RENAME) && islastcn) {
- err = fuse_internal_access(dvp, VWRITE, td, cred);
- if (err != 0)
- goto out;
- /*
- * TODO: if the parent's sticky bit is set, check
- * whether we're allowed to remove the file.
- * Need to figure out the vnode locking to make this
- * work.
- */
-#if defined(NOT_YET)
- struct vattr dvattr;
- fuse_internal_getattr(dvp, &dvattr, cred, td);
- if ((dvattr.va_mode & S_ISTXT) &&
- fuse_internal_access(dvp, VADMIN, td, cred) &&
- fuse_internal_access(*vpp, VADMIN, td, cred)) {
- err = EPERM;
- goto out;
- }
-#endif
- }
-
- /*
- * If deleting, and at end of pathname, return parameters
- * which can be used to remove file. If the wantparent flag
- * isn't set, we return only the directory, otherwise we go on
- * and lock the inode, being careful with ".".
- */
- if (nameiop == DELETE && islastcn) {
- if (nid == VTOI(dvp)) {
- vref(dvp);
- *vpp = dvp;
- } else {
- err = fuse_vnode_get(dvp->v_mount, feo, nid,
- dvp, &vp, cnp, vtyp);
- if (err)
- goto out;
- *vpp = vp;
- }
-
- /*
- * Save the name for use in VOP_RMDIR and VOP_REMOVE
- * later.
- */
- cnp->cn_flags |= SAVENAME;
- goto out;
-
- }
- /*
- * If rewriting (RENAME), return the inode and the
- * information required to rewrite the present directory
- * Must get inode of directory entry to verify it's a
- * regular file, or empty directory.
- */
- if (nameiop == RENAME && wantparent && islastcn) {
- /*
- * Check for "."
- */
- if (nid == VTOI(dvp)) {
- err = EISDIR;
- goto out;
- }
- err = fuse_vnode_get(vnode_mount(dvp), feo, nid, dvp,
- &vp, cnp, vtyp);
- if (err) {
- goto out;
- }
- *vpp = vp;
- /*
- * Save the name for use in VOP_RENAME later.
- */
- cnp->cn_flags |= SAVENAME;
-
- goto out;
- }
if (flags & ISDOTDOT) {
struct fuse_lookup_alloc_arg flaa;
@@ -966,8 +893,6 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
flaa.vtyp = vtyp;
err = vn_vget_ino_gen(dvp, fuse_lookup_alloc, &flaa, 0,
&vp);
- if (err)
- goto out;
*vpp = vp;
} else if (nid == VTOI(dvp)) {
vref(dvp);
@@ -977,9 +902,10 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
err = fuse_vnode_get(vnode_mount(dvp), feo, nid, dvp,
&vp, cnp, vtyp);
- if (err) {
+ if (err)
goto out;
- }
+ *vpp = vp;
+
fuse_vnode_setparent(vp, dvp);
/*
@@ -989,8 +915,10 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
* the vnode's cached size, fix the vnode cache to
* match the real object size.
*
- * This can occur via FUSE distributed filesystems,
- * irregular files, etc.
+ * We can get here:
+ * * following attribute cache expiration, or
+ * * due a bug in the daemon, or
+ * * the first time that we looked up the file.
*/
fvdat = VTOFUD(vp);
if (vnode_isreg(vp) &&
@@ -1012,28 +940,52 @@ fuse_vnop_lookup(struct vop_lookup_args *ap)
(void)fuse_vnode_setsize(vp, cred, filesize);
fvdat->flag &= ~FN_SIZECHANGE;
}
- *vpp = vp;
- }
- if (feo != NULL) {
+ MPASS(feo != NULL);
fuse_internal_cache_attrs(*vpp, &feo->attr,
feo->attr_valid, feo->attr_valid_nsec, NULL);
- }
- }
-out:
- if (!lookup_err) {
- /* No lookup error; need to clean up. */
+ if ((nameiop == DELETE || nameiop == RENAME) &&
+ islastcn)
+ {
+ struct vattr dvattr;
- if (err) { /* Found inode; exit with no vnode. */
- if (feo != NULL) {
- fuse_internal_forget_send(vnode_mount(dvp), td,
- cred, nid, 1);
+ err = fuse_internal_access(dvp, VWRITE, td,
+ cred);
+ if (err != 0)
+ goto out;
+ /*
+ * if the parent's sticky bit is set, check
+ * whether we're allowed to remove the file.
+ * Need to figure out the vnode locking to make
+ * this work.
+ */
+ fuse_internal_getattr(dvp, &dvattr, cred, td);
+ if ((dvattr.va_mode & S_ISTXT) &&
+ fuse_internal_access(dvp, VADMIN, td,
+ cred) &&
+ fuse_internal_access(*vpp, VADMIN, td,
+ cred)) {
+ err = EPERM;
+ goto out;
+ }
}
- if (did_lookup)
- fdisp_destroy(&fdi);
- return err;
+
+ if (islastcn && (
+ (nameiop == DELETE) ||
+ (nameiop == RENAME && wantparent))) {
+ cnp->cn_flags |= SAVENAME;
+ }
+
}
+ }
+out:
+ if (err) {
+ if (vp != NULL && dvp != vp)
+ vput(vp);
+ else if (vp != NULL)
+ vrele(vp);
+ *vpp = NULL;
}
if (did_lookup)
fdisp_destroy(&fdi);
Modified: projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc
==============================================================================
--- projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc Thu Apr 11 20:39:12 2019 (r346134)
+++ projects/fuse2/tests/sys/fs/fusefs/default_permissions.cc Thu Apr 11 21:00:40 2019 (r346135)
@@ -71,7 +71,6 @@ virtual void SetUp() { (public)
void expect_getattr(uint64_t ino, mode_t mode, uint64_t attr_valid, int times,
uid_t uid = 0)
{
- /* Until the attr cache is working, we may send an additional GETATTR */
EXPECT_CALL(*m_mock, process(
ResultOf([=](auto in) {
return (in->header.opcode == FUSE_GETATTR &&
@@ -453,7 +452,6 @@ TEST_F(Lookup, eacces)
{
const char FULLPATH[] = "mountpoint/some_dir/some_file.txt";
const char RELDIRPATH[] = "some_dir";
- //const char FINALPATH[] = "some_file.txt";
uint64_t dir_ino = 42;
expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1);
@@ -548,32 +546,25 @@ TEST_F(Rename, eacces_on_dstdir_for_removing)
ASSERT_EQ(EACCES, errno);
}
-/* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216391 */
-TEST_F(Rename, DISABLED_eperm_on_sticky_srcdir)
+TEST_F(Rename, eperm_on_sticky_srcdir)
{
const char FULLDST[] = "mountpoint/d/dst";
- const char RELDSTDIR[] = "d";
- const char RELDST[] = "dst";
const char FULLSRC[] = "mountpoint/src";
const char RELSRC[] = "src";
uint64_t ino = 42;
- uint64_t dstdir_ino = 43;
expect_getattr(1, S_IFDIR | 01777, UINT64_MAX, 1, 0);
expect_lookup(RELSRC, ino, S_IFREG | 0644, UINT64_MAX);
- expect_lookup(RELDSTDIR, dstdir_ino, S_IFDIR | 0777, UINT64_MAX);
- EXPECT_LOOKUP(dstdir_ino, RELDST).WillOnce(Invoke(ReturnErrno(ENOENT)));
ASSERT_EQ(-1, rename(FULLSRC, FULLDST));
ASSERT_EQ(EPERM, errno);
}
-/* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216391 */
-TEST_F(Rename, DISABLED_eperm_on_sticky_dstdir)
+TEST_F(Rename, eperm_on_sticky_dstdir)
{
const char FULLDST[] = "mountpoint/d/dst";
const char RELDSTDIR[] = "d";
- const char RELDST[] = "d/dst";
+ const char RELDST[] = "dst";
const char FULLSRC[] = "mountpoint/src";
const char RELSRC[] = "src";
uint64_t src_ino = 42;
@@ -583,7 +574,15 @@ TEST_F(Rename, DISABLED_eperm_on_sticky_dstdir)
expect_getattr(1, S_IFDIR | 0777, UINT64_MAX, 1, 0);
expect_lookup(RELSRC, src_ino, S_IFREG | 0644, UINT64_MAX);
expect_lookup(RELDSTDIR, dstdir_ino, S_IFDIR | 01777, UINT64_MAX);
- expect_lookup(RELDST, dst_ino, S_IFREG | 0644, UINT64_MAX, 0);
+ EXPECT_LOOKUP(dstdir_ino, RELDST)
+ .WillOnce(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 = dst_ino;
+ out->body.entry.attr_valid = UINT64_MAX;
+ out->body.entry.entry_valid = UINT64_MAX;
+ out->body.entry.attr.uid = 0;
+ })));
ASSERT_EQ(-1, rename(FULLSRC, FULLDST));
ASSERT_EQ(EPERM, errno);
@@ -755,6 +754,38 @@ TEST_F(Unlink, ok)
ASSERT_EQ(0, unlink(FULLPATH)) << strerror(errno);
}
+/*
+ * Ensure that a cached name doesn't cause unlink to bypass permission checks
+ * in VOP_LOOKUP.
+ *
+ * This test should pass because lookup(9) purges the namecache entry by doing
+ * a vfs_cache_lookup with ~MAKEENTRY when nameiop == DELETE.
+ */
+TEST_F(Unlink, cached_unwritable_directory)
+{
+ const char FULLPATH[] = "mountpoint/some_file.txt";
+ const char RELPATH[] = "some_file.txt";
+ uint64_t ino = 42;
+
+ expect_getattr(1, S_IFDIR | 0755, UINT64_MAX, 1);
+ EXPECT_LOOKUP(1, RELPATH)
+ .Times(AnyNumber())
+ .WillRepeatedly(Invoke(
+ ReturnImmediate([=](auto i __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.entry_valid = UINT64_MAX;
+ }))
+ );
+
+ /* Fill name cache */
+ ASSERT_EQ(0, access(FULLPATH, F_OK)) << strerror(errno);
+ /* Despite cached name , unlink should fail */
+ ASSERT_EQ(-1, unlink(FULLPATH));
+ ASSERT_EQ(EACCES, errno);
+}
+
TEST_F(Unlink, unwritable_directory)
{
const char FULLPATH[] = "mountpoint/some_file.txt";
@@ -768,8 +799,7 @@ TEST_F(Unlink, unwritable_directory)
ASSERT_EQ(EACCES, errno);
}
-/* https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216391 */
-TEST_F(Unlink, DISABLED_sticky_directory)
+TEST_F(Unlink, sticky_directory)
{
const char FULLPATH[] = "mountpoint/some_file.txt";
const char RELPATH[] = "some_file.txt";
Modified: projects/fuse2/tests/sys/fs/fusefs/destroy.cc
==============================================================================
--- projects/fuse2/tests/sys/fs/fusefs/destroy.cc Thu Apr 11 20:39:12 2019 (r346134)
+++ projects/fuse2/tests/sys/fs/fusefs/destroy.cc Thu Apr 11 21:00:40 2019 (r346135)
@@ -50,7 +50,7 @@ void expect_destroy(int error)
/*
* On unmount the kernel should send a FUSE_DESTROY operation. It should also
* send FUSE_FORGET operations for all inodes with lookup_count > 0. It's hard
- * to trigger FUSE_FORGET in way except by unmounting, so this is the only
+ * to trigger FUSE_FORGET in any way except by unmounting, so this is the only
* testing that FUSE_FORGET gets.
*/
TEST_F(Destroy, ok)
Modified: projects/fuse2/tests/sys/fs/fusefs/rename.cc
==============================================================================
--- projects/fuse2/tests/sys/fs/fusefs/rename.cc Thu Apr 11 20:39:12 2019 (r346134)
+++ projects/fuse2/tests/sys/fs/fusefs/rename.cc Thu Apr 11 21:00:40 2019 (r346135)
@@ -51,6 +51,24 @@ class Rename: public FuseTest {
FuseTest::TearDown();
}
+
+ void expect_getattr(uint64_t ino, mode_t mode)
+ {
+ EXPECT_CALL(*m_mock, process(
+ ResultOf([=](auto in) {
+ return (in->header.opcode == FUSE_GETATTR &&
+ in->header.nodeid == ino);
+ }, Eq(true)),
+ _)
+ ).WillOnce(Invoke(
+ ReturnImmediate([=](auto i __unused, auto out) {
+ SET_OUT_HEADER_LEN(out, attr);
+ out->body.attr.attr.ino = ino; // Must match nodeid
+ out->body.attr.attr.mode = mode;
+ out->body.attr.attr_valid = UINT64_MAX;
+ })));
+ }
+
};
// EINVAL, dst is subdir of src
@@ -62,6 +80,7 @@ TEST_F(Rename, einval)
const char RELSRC[] = "src";
uint64_t src_ino = 42;
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELSRC, src_ino, S_IFDIR | 0755, 0, 2);
EXPECT_LOOKUP(src_ino, RELDST).WillOnce(Invoke(ReturnErrno(ENOENT)));
@@ -102,6 +121,7 @@ TEST_F(Rename, entry_cache_negative)
*/
struct timespec entry_valid = {.tv_sec = 0, .tv_nsec = 0};
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELSRC, ino, S_IFREG | 0644, 0, 1);
/* LOOKUP returns a negative cache entry for dst */
EXPECT_LOOKUP(1, RELDST).WillOnce(ReturnNegativeCache(&entry_valid));
@@ -136,6 +156,7 @@ TEST_F(Rename, entry_cache_negative_purge)
uint64_t ino = 42;
struct timespec entry_valid = {.tv_sec = TIME_T_MAX, .tv_nsec = 0};
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELSRC, ino, S_IFREG | 0644, 0, 1);
/* LOOKUP returns a negative cache entry for dst */
EXPECT_LOOKUP(1, RELDST).WillOnce(ReturnNegativeCache(&entry_valid))
@@ -172,6 +193,7 @@ TEST_F(Rename, exdev)
tmpfd = mkstemp(tmpfile);
ASSERT_LE(0, tmpfd) << strerror(errno);
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELB, b_ino, S_IFREG | 0644, 0, 2);
ASSERT_NE(0, rename(tmpfile, FULLB));
@@ -191,6 +213,7 @@ TEST_F(Rename, ok)
uint64_t dst_dir_ino = 1;
uint64_t ino = 42;
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELSRC, ino, S_IFREG | 0644, 0, 1);
EXPECT_LOOKUP(1, RELDST).WillOnce(Invoke(ReturnErrno(ENOENT)));
@@ -223,6 +246,7 @@ TEST_F(Rename, overwrite)
uint64_t dst_dir_ino = 1;
uint64_t ino = 42;
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELSRC, ino, S_IFREG | 0644, 0, 1);
expect_lookup(RELDST, dst_ino, S_IFREG | 0644, 0, 1);
EXPECT_CALL(*m_mock, process(
Modified: projects/fuse2/tests/sys/fs/fusefs/rmdir.cc
==============================================================================
--- projects/fuse2/tests/sys/fs/fusefs/rmdir.cc Thu Apr 11 20:39:12 2019 (r346134)
+++ projects/fuse2/tests/sys/fs/fusefs/rmdir.cc Thu Apr 11 21:00:40 2019 (r346135)
@@ -39,6 +39,22 @@ using namespace testing;
class Rmdir: public FuseTest {
public:
+void expect_getattr(uint64_t ino, mode_t mode)
+{
+ EXPECT_CALL(*m_mock, process(
+ ResultOf([=](auto in) {
+ return (in->header.opcode == FUSE_GETATTR &&
+ in->header.nodeid == ino);
+ }, Eq(true)),
+ _)
+ ).WillOnce(Invoke(ReturnImmediate([=](auto i __unused, auto out) {
+ SET_OUT_HEADER_LEN(out, attr);
+ out->body.attr.attr.ino = ino; // Must match nodeid
+ out->body.attr.attr.mode = mode;
+ out->body.attr.attr_valid = UINT64_MAX;
+ })));
+}
+
void expect_lookup(const char *relpath, uint64_t ino)
{
EXPECT_LOOKUP(1, relpath)
@@ -57,6 +73,7 @@ TEST_F(Rmdir, enotempty)
const char RELPATH[] = "some_dir";
uint64_t ino = 42;
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELPATH, ino);
EXPECT_CALL(*m_mock, process(
ResultOf([=](auto in) {
@@ -77,6 +94,7 @@ TEST_F(Rmdir, ok)
const char RELPATH[] = "some_dir";
uint64_t ino = 42;
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELPATH, ino);
EXPECT_CALL(*m_mock, process(
ResultOf([=](auto in) {
Modified: projects/fuse2/tests/sys/fs/fusefs/unlink.cc
==============================================================================
--- projects/fuse2/tests/sys/fs/fusefs/unlink.cc Thu Apr 11 20:39:12 2019 (r346134)
+++ projects/fuse2/tests/sys/fs/fusefs/unlink.cc Thu Apr 11 21:00:40 2019 (r346135)
@@ -38,6 +38,22 @@ using namespace testing;
class Unlink: public FuseTest {
public:
+void expect_getattr(uint64_t ino, mode_t mode)
+{
+ EXPECT_CALL(*m_mock, process(
+ ResultOf([=](auto in) {
+ return (in->header.opcode == FUSE_GETATTR &&
+ in->header.nodeid == ino);
+ }, Eq(true)),
+ _)
+ ).WillOnce(Invoke(ReturnImmediate([=](auto i __unused, auto out) {
+ SET_OUT_HEADER_LEN(out, attr);
+ out->body.attr.attr.ino = ino; // Must match nodeid
+ out->body.attr.attr.mode = mode;
+ out->body.attr.attr_valid = UINT64_MAX;
+ })));
+}
+
void expect_lookup(const char *relpath, uint64_t ino, int times)
{
FuseTest::expect_lookup(relpath, ino, S_IFREG | 0644, 0, times);
@@ -51,6 +67,7 @@ TEST_F(Unlink, eperm)
const char RELPATH[] = "some_file.txt";
uint64_t ino = 42;
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELPATH, ino, 1);
expect_unlink(1, RELPATH, EPERM);
@@ -64,6 +81,7 @@ TEST_F(Unlink, ok)
const char RELPATH[] = "some_file.txt";
uint64_t ino = 42;
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELPATH, ino, 1);
expect_unlink(1, RELPATH, 0);
@@ -78,6 +96,7 @@ TEST_F(Unlink, open_but_deleted)
uint64_t ino = 42;
int fd;
+ expect_getattr(1, S_IFDIR | 0755);
expect_lookup(RELPATH, ino, 2);
expect_open(ino, 0, 1);
expect_unlink(1, RELPATH, 0);
More information about the svn-src-projects
mailing list