git: 2d05cbe00272 - stable/13 - fusefs: fix some bugs updating atime during close
- Go to: [ bottom of page ] [ top of archives ] [ this month ]
Date: Thu, 05 Oct 2023 17:34:45 UTC
The branch stable/13 has been updated by asomers:
URL: https://cgit.FreeBSD.org/src/commit/?id=2d05cbe002726a28e11060a601d1877784f6d587
commit 2d05cbe002726a28e11060a601d1877784f6d587
Author: Alan Somers <asomers@FreeBSD.org>
AuthorDate: 2023-09-20 21:37:31 +0000
Commit: Alan Somers <asomers@FreeBSD.org>
CommitDate: 2023-10-05 16:16:11 +0000
fusefs: fix some bugs updating atime during close
When using cached attributes, we must update a file's atime during
close, if it has been read since the last attribute refresh. But,
* Don't update atime if we lack write permissions to the file or if the
file system is readonly.
* If the daemon fails our atime update request for any reason, don't
report this as a failure for VOP_CLOSE.
PR: 270749
Reported by: Jamie Landeg-Jones <jamie@catflap.org>
Sponsored by: Axcient
Reviewed by: pfg
Differential Revision: https://reviews.freebsd.org/D41925
(cherry picked from commit fb619c94c679e939496fe0cf94b8d2cba95e6e63)
fusefs: fix unused variables from fb619c94c67
PR: 270749
Reported by: cy
Sponsored by: Axcient
(cherry picked from commit e5236d25f2c0709acf3547e6af45f4bb4eec4f02)
---
sys/fs/fuse/fuse_vnops.c | 27 ++++++++--
tests/sys/fs/fusefs/access.cc | 2 +-
tests/sys/fs/fusefs/default_permissions.cc | 41 ++++++++++++++-
tests/sys/fs/fusefs/read.cc | 83 ++++++++++++++++++++++++++++++
4 files changed, 147 insertions(+), 6 deletions(-)
diff --git a/sys/fs/fuse/fuse_vnops.c b/sys/fs/fuse/fuse_vnops.c
index 5cd67d9af5a3..8fa60ff95d4c 100644
--- a/sys/fs/fuse/fuse_vnops.c
+++ b/sys/fs/fuse/fuse_vnops.c
@@ -780,6 +780,7 @@ static int
fuse_vnop_close(struct vop_close_args *ap)
{
struct vnode *vp = ap->a_vp;
+ struct mount *mp = vnode_mount(vp);
struct ucred *cred = ap->a_cred;
int fflag = ap->a_fflag;
struct thread *td = ap->a_td;
@@ -795,12 +796,30 @@ fuse_vnop_close(struct vop_close_args *ap)
return 0;
err = fuse_flush(vp, cred, pid, fflag);
- if (err == 0 && (fvdat->flag & FN_ATIMECHANGE)) {
+ if (err == 0 && (fvdat->flag & FN_ATIMECHANGE) && !vfs_isrdonly(mp)) {
struct vattr vap;
+ struct fuse_data *data;
+ int dataflags;
+ int access_e = 0;
- VATTR_NULL(&vap);
- vap.va_atime = fvdat->cached_attrs.va_atime;
- err = fuse_internal_setattr(vp, &vap, td, NULL);
+ data = fuse_get_mpdata(mp);
+ dataflags = data->dataflags;
+ if (dataflags & FSESS_DEFAULT_PERMISSIONS) {
+ struct vattr va;
+
+ fuse_internal_getattr(vp, &va, cred, td);
+ access_e = vaccess(vp->v_type, va.va_mode, va.va_uid,
+ va.va_gid, VWRITE, cred);
+ }
+ if (access_e == 0) {
+ VATTR_NULL(&vap);
+ vap.va_atime = fvdat->cached_attrs.va_atime;
+ /*
+ * Ignore errors setting when setting atime. That
+ * should not cause close(2) to fail.
+ */
+ fuse_internal_setattr(vp, &vap, td, NULL);
+ }
}
/* TODO: close the file handle, if we're sure it's no longer used */
if ((fvdat->flag & FN_SIZECHANGE) != 0) {
diff --git a/tests/sys/fs/fusefs/access.cc b/tests/sys/fs/fusefs/access.cc
index 3d6cddb9417b..5762269fac7b 100644
--- a/tests/sys/fs/fusefs/access.cc
+++ b/tests/sys/fs/fusefs/access.cc
@@ -55,7 +55,7 @@ void expect_lookup(const char *relpath, uint64_t ino)
}
/*
- * Expect tha FUSE_ACCESS will never be called for the given inode, with any
+ * Expect that FUSE_ACCESS will never be called for the given inode, with any
* bits in the supplied access_mask set
*/
void expect_noaccess(uint64_t ino, mode_t access_mask)
diff --git a/tests/sys/fs/fusefs/default_permissions.cc b/tests/sys/fs/fusefs/default_permissions.cc
index 8527b0c6deb5..ff25d37f5b4d 100644
--- a/tests/sys/fs/fusefs/default_permissions.cc
+++ b/tests/sys/fs/fusefs/default_permissions.cc
@@ -30,7 +30,7 @@
/*
* Tests for the "default_permissions" mount option. They must be in their own
- * file so they can be run as an unprivileged user
+ * file so they can be run as an unprivileged user.
*/
extern "C" {
@@ -162,6 +162,7 @@ class CopyFileRange: public DefaultPermissions {};
class Lookup: public DefaultPermissions {};
class Open: public DefaultPermissions {};
class PosixFallocate: public DefaultPermissions {};
+class Read: public DefaultPermissions {};
class Setattr: public DefaultPermissions {};
class Unlink: public DefaultPermissions {};
class Utimensat: public DefaultPermissions {};
@@ -1140,6 +1141,44 @@ TEST_F(Rename, ok_to_remove_src_because_of_stickiness)
ASSERT_EQ(0, rename(FULLSRC, FULLDST)) << strerror(errno);
}
+// Don't update atime during close after read, if we lack permissions to write
+// that file.
+TEST_F(Read, atime_during_close)
+{
+ const char FULLPATH[] = "mountpoint/some_file.txt";
+ const char RELPATH[] = "some_file.txt";
+ uint64_t ino = 42;
+ int fd;
+ ssize_t bufsize = 100;
+ uint8_t buf[bufsize];
+ const char *CONTENTS = "abcdefgh";
+ ssize_t fsize = sizeof(CONTENTS);
+
+ expect_getattr(FUSE_ROOT_ID, S_IFDIR | 0755, UINT64_MAX, 1);
+ FuseTest::expect_lookup(RELPATH, ino, S_IFREG | 0755, fsize,
+ 1, UINT64_MAX, 0, 0);
+ expect_open(ino, 0, 1);
+ expect_read(ino, 0, fsize, fsize, CONTENTS);
+ EXPECT_CALL(*m_mock, process(
+ ResultOf([&](auto in) {
+ return (in.header.opcode == FUSE_SETATTR);
+ }, Eq(true)),
+ _)
+ ).Times(0);
+ expect_flush(ino, 1, ReturnErrno(0));
+ expect_release(ino, FuseTest::FH);
+
+ fd = open(FULLPATH, O_RDONLY);
+ ASSERT_LE(0, fd) << strerror(errno);
+
+ /* Ensure atime will be different than during lookup */
+ nap();
+
+ ASSERT_EQ(fsize, read(fd, buf, bufsize)) << strerror(errno);
+
+ close(fd);
+}
+
TEST_F(Setattr, ok)
{
const char FULLPATH[] = "mountpoint/some_file.txt";
diff --git a/tests/sys/fs/fusefs/read.cc b/tests/sys/fs/fusefs/read.cc
index 3b2a04fe0a52..5beb59e9f3be 100644
--- a/tests/sys/fs/fusefs/read.cc
+++ b/tests/sys/fs/fusefs/read.cc
@@ -57,6 +57,14 @@ void expect_lookup(const char *relpath, uint64_t ino, uint64_t size)
}
};
+class RofsRead: public Read {
+public:
+virtual void SetUp() {
+ m_ro = true;
+ Read::SetUp();
+}
+};
+
class Read_7_8: public FuseTest {
public:
virtual void SetUp() {
@@ -454,6 +462,47 @@ TEST_F(Read, atime_during_close)
close(fd);
}
+/*
+ * When not using -o default_permissions, the daemon may make its own decisions
+ * regarding access permissions, and these may be unpredictable. If it rejects
+ * our attempt to set atime, that should not cause close(2) to fail.
+ */
+TEST_F(Read, atime_during_close_eacces)
+{
+ const char FULLPATH[] = "mountpoint/some_file.txt";
+ const char RELPATH[] = "some_file.txt";
+ const char *CONTENTS = "abcdefgh";
+ uint64_t ino = 42;
+ int fd;
+ ssize_t bufsize = strlen(CONTENTS);
+ uint8_t buf[bufsize];
+
+ expect_lookup(RELPATH, ino, bufsize);
+ expect_open(ino, 0, 1);
+ expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+ EXPECT_CALL(*m_mock, process(
+ ResultOf([&](auto in) {
+ uint32_t valid = FATTR_ATIME;
+ return (in.header.opcode == FUSE_SETATTR &&
+ in.header.nodeid == ino &&
+ in.body.setattr.valid == valid);
+ }, Eq(true)),
+ _)
+ ).WillOnce(Invoke(ReturnErrno(EACCES)));
+ expect_flush(ino, 1, ReturnErrno(0));
+ expect_release(ino, FuseTest::FH);
+
+ fd = open(FULLPATH, O_RDONLY);
+ ASSERT_LE(0, fd) << strerror(errno);
+
+ /* Ensure atime will be different than during lookup */
+ nap();
+
+ ASSERT_EQ(bufsize, read(fd, buf, bufsize)) << strerror(errno);
+
+ ASSERT_EQ(0, close(fd));
+}
+
/* A cached atime should be flushed during FUSE_SETATTR */
TEST_F(Read, atime_during_setattr)
{
@@ -1321,6 +1370,40 @@ INSTANTIATE_TEST_CASE_P(RA, ReadAhead,
tuple<bool, int>(true, 1),
tuple<bool, int>(true, 2)));
+/* With read-only mounts, fuse should never update atime during close */
+TEST_F(RofsRead, atime_during_close)
+{
+ const char FULLPATH[] = "mountpoint/some_file.txt";
+ const char RELPATH[] = "some_file.txt";
+ const char *CONTENTS = "abcdefgh";
+ uint64_t ino = 42;
+ int fd;
+ ssize_t bufsize = strlen(CONTENTS);
+ uint8_t buf[bufsize];
+
+ expect_lookup(RELPATH, ino, bufsize);
+ expect_open(ino, 0, 1);
+ expect_read(ino, 0, bufsize, bufsize, CONTENTS);
+ EXPECT_CALL(*m_mock, process(
+ ResultOf([&](auto in) {
+ return (in.header.opcode == FUSE_SETATTR);
+ }, Eq(true)),
+ _)
+ ).Times(0);
+ expect_flush(ino, 1, ReturnErrno(0));
+ expect_release(ino, FuseTest::FH);
+
+ fd = open(FULLPATH, O_RDONLY);
+ ASSERT_LE(0, fd) << strerror(errno);
+
+ /* Ensure atime will be different than during lookup */
+ nap();
+
+ ASSERT_EQ(bufsize, read(fd, buf, bufsize)) << strerror(errno);
+
+ close(fd);
+}
+
/* fuse_init_out.time_gran controls the granularity of timestamps */
TEST_P(TimeGran, atime_during_setattr)
{