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 <[email protected]>
MFC after:	1 week
Sponsored by:	Axcient
Reviewed by:	pfg
Differential Revision: https://reviews.freebsd.org/D41925

sys/fs/fuse/fuse_vnops.c

@@ -779,6 +779,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;
@@ -794,12 +795,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) {

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)

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" {
@@ -163,6 +163,7 @@ class Fspacectl: 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 {};
@@ -1239,6 +1240,45 @@ 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";
+	struct stat sb;
+	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";

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,48 @@ 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";
+	struct stat sb;
+	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 +1371,41 @@ INSTANTIATE_TEST_SUITE_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";
+	struct stat sb;
+	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)
 {