Date: Thu, 11 Apr 2019 21:00:40 +0000 (UTC) From: Alan Somers <asomers@FreeBSD.org> To: src-committers@freebsd.org, svn-src-projects@freebsd.org Subject: svn commit: r346135 - in projects/fuse2: sys/fs/fuse tests/sys/fs/fusefs Message-ID: <201904112100.x3BL0eDJ024889@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904112100.x3BL0eDJ024889>