From 4181e8c97482a3c787c2b508e6d75a21323ba515 Mon Sep 17 00:00:00 2001 From: Craig Chi Date: Wed, 9 Sep 2020 09:16:09 -0700 Subject: [PATCH] Add fh support for revise attr and fstat(2) test According to Linux 4.4's FUSE behavior, the flags and fh attributes in FUSE_GETATTR are only used in read, write, and lseek. fstat(2) doesn't use them either. Add tests to ensure the requests sent from FUSE module are consistent with Linux's. Updates #3655 --- pkg/abi/linux/fuse.go | 5 ++ pkg/sentry/fsimpl/fuse/fusefs.go | 21 +++-- pkg/sentry/fsimpl/fuse/regular_file.go | 2 +- test/fuse/linux/BUILD | 4 +- test/fuse/linux/stat_test.cc | 112 +++++++++++++++++++++++-- test/util/fuse_util.cc | 12 +-- test/util/fuse_util.h | 5 +- 7 files changed, 133 insertions(+), 28 deletions(-) diff --git a/pkg/abi/linux/fuse.go b/pkg/abi/linux/fuse.go index e49a92fb2..f0bef1e8e 100644 --- a/pkg/abi/linux/fuse.go +++ b/pkg/abi/linux/fuse.go @@ -227,6 +227,11 @@ type FUSEInitOut struct { _ [8]uint32 } +// FUSE_GETATTR_FH is currently the only flag of FUSEGetAttrIn.GetAttrFlags. +// If it is set, the file handle (FUSEGetAttrIn.Fh) is used to indicate the +// object instead of the node id attribute in the request header. +const FUSE_GETATTR_FH = (1 << 0) + // FUSEGetAttrIn is the request sent by the kernel to the daemon, // to get the attribute of a inode. // diff --git a/pkg/sentry/fsimpl/fuse/fusefs.go b/pkg/sentry/fsimpl/fuse/fusefs.go index 8cf13dcb6..821048d87 100644 --- a/pkg/sentry/fsimpl/fuse/fusefs.go +++ b/pkg/sentry/fsimpl/fuse/fusefs.go @@ -609,9 +609,9 @@ func statFromFUSEAttr(attr linux.FUSEAttr, mask, devMinor uint32) linux.Statx { } // getAttr gets the attribute of this inode by issuing a FUSE_GETATTR request -// or read from local cache. -// It updates the corresponding attributes if necessary. -func (i *inode) getAttr(ctx context.Context, fs *vfs.Filesystem, opts vfs.StatOptions) (linux.FUSEAttr, error) { +// or read from local cache. It updates the corresponding attributes if +// necessary. +func (i *inode) getAttr(ctx context.Context, fs *vfs.Filesystem, opts vfs.StatOptions, flags uint32, fh uint64) (linux.FUSEAttr, error) { attributeVersion := atomic.LoadUint64(&i.fs.conn.attributeVersion) // TODO(gvisor.dev/issue/3679): send the request only if @@ -631,11 +631,10 @@ func (i *inode) getAttr(ctx context.Context, fs *vfs.Filesystem, opts vfs.StatOp creds := auth.CredentialsFromContext(ctx) - var in linux.FUSEGetAttrIn - // We don't set any attribute in the request, because in VFS2 fstat(2) will - // finally be translated into vfs.FilesystemImpl.StatAt() (see - // pkg/sentry/syscalls/linux/vfs2/stat.go), resulting in the same flow - // as stat(2). Thus GetAttrFlags and Fh variable will never be used in VFS2. + in := linux.FUSEGetAttrIn{ + GetAttrFlags: flags, + Fh: fh, + } req, err := i.fs.conn.NewRequest(creds, uint32(task.ThreadID()), i.NodeID, linux.FUSE_GETATTR, &in) if err != nil { return linux.FUSEAttr{}, err @@ -676,17 +675,17 @@ func (i *inode) getAttr(ctx context.Context, fs *vfs.Filesystem, opts vfs.StatOp // reviseAttr attempts to update the attributes for internal purposes // by calling getAttr with a pre-specified mask. // Used by read, write, lseek. -func (i *inode) reviseAttr(ctx context.Context) error { +func (i *inode) reviseAttr(ctx context.Context, flags uint32, fh uint64) error { // Never need atime for internal purposes. _, err := i.getAttr(ctx, i.fs.VFSFilesystem(), vfs.StatOptions{ Mask: linux.STATX_BASIC_STATS &^ linux.STATX_ATIME, - }) + }, flags, fh) return err } // Stat implements kernfs.Inode.Stat. func (i *inode) Stat(ctx context.Context, fs *vfs.Filesystem, opts vfs.StatOptions) (linux.Statx, error) { - attr, err := i.getAttr(ctx, fs, opts) + attr, err := i.getAttr(ctx, fs, opts, 0, 0) if err != nil { return linux.Statx{}, err } diff --git a/pkg/sentry/fsimpl/fuse/regular_file.go b/pkg/sentry/fsimpl/fuse/regular_file.go index 193e77392..5bdd096c3 100644 --- a/pkg/sentry/fsimpl/fuse/regular_file.go +++ b/pkg/sentry/fsimpl/fuse/regular_file.go @@ -65,7 +65,7 @@ func (fd *regularFileFD) PRead(ctx context.Context, dst usermem.IOSequence, offs // Reading beyond EOF, update file size if outdated. if uint64(offset+size) > atomic.LoadUint64(&inode.size) { - if err := inode.reviseAttr(ctx); err != nil { + if err := inode.reviseAttr(ctx, linux.FUSE_GETATTR_FH, fd.Fh); err != nil { return 0, err } // If the offset after update is still too large, return error. diff --git a/test/fuse/linux/BUILD b/test/fuse/linux/BUILD index eb090cbfd..7a3e52fad 100644 --- a/test/fuse/linux/BUILD +++ b/test/fuse/linux/BUILD @@ -11,7 +11,9 @@ cc_binary( srcs = ["stat_test.cc"], deps = [ gtest, - ":fuse_base", + ":fuse_fd_util", + "//test/util:cleanup", + "//test/util:fs_util", "//test/util:fuse_util", "//test/util:test_main", "//test/util:test_util", diff --git a/test/fuse/linux/stat_test.cc b/test/fuse/linux/stat_test.cc index 717fd1fac..6f032cac1 100644 --- a/test/fuse/linux/stat_test.cc +++ b/test/fuse/linux/stat_test.cc @@ -18,12 +18,15 @@ #include #include #include +#include #include #include #include "gtest/gtest.h" -#include "test/fuse/linux/fuse_base.h" +#include "test/fuse/linux/fuse_fd_util.h" +#include "test/util/cleanup.h" +#include "test/util/fs_util.h" #include "test/util/fuse_util.h" #include "test/util/test_util.h" @@ -32,19 +35,30 @@ namespace testing { namespace { -class StatTest : public FuseTest { +class StatTest : public FuseFdTest { public: + void SetUp() override { + FuseFdTest::SetUp(); + test_file_path_ = JoinPath(mount_point_.path(), test_file_); + } + + protected: bool StatsAreEqual(struct stat expected, struct stat actual) { - // device number will be dynamically allocated by kernel, we cannot know - // in advance + // Device number will be dynamically allocated by kernel, we cannot know in + // advance. actual.st_dev = expected.st_dev; return memcmp(&expected, &actual, sizeof(struct stat)) == 0; } + + const std::string test_file_ = "testfile"; + const mode_t expected_mode = S_IFREG | S_IRUSR | S_IWUSR; + const uint64_t fh = 23; + + std::string test_file_path_; }; TEST_F(StatTest, StatNormal) { // Set up fixture. - mode_t expected_mode = S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH; struct fuse_attr attr = DefaultFuseAttr(expected_mode, 1); struct fuse_out_header out_header = { .len = sizeof(struct fuse_out_header) + sizeof(struct fuse_attr_out), @@ -55,7 +69,7 @@ TEST_F(StatTest, StatNormal) { auto iov_out = FuseGenerateIovecs(out_header, out_payload); SetServerResponse(FUSE_GETATTR, iov_out); - // Do integration test. + // Make syscall. struct stat stat_buf; EXPECT_THAT(stat(mount_point_.path().c_str(), &stat_buf), SyscallSucceeds()); @@ -99,7 +113,7 @@ TEST_F(StatTest, StatNotFound) { auto iov_out = FuseGenerateIovecs(out_header); SetServerResponse(FUSE_GETATTR, iov_out); - // Do integration test. + // Make syscall. struct stat stat_buf; EXPECT_THAT(stat(mount_point_.path().c_str(), &stat_buf), SyscallFailsWithErrno(ENOENT)); @@ -115,6 +129,90 @@ TEST_F(StatTest, StatNotFound) { EXPECT_EQ(in_payload.fh, 0); } +TEST_F(StatTest, FstatNormal) { + // Set up fixture. + SetServerInodeLookup(test_file_); + auto fd = ASSERT_NO_ERRNO_AND_VALUE(OpenPath(test_file_path_, O_RDONLY, fh)); + auto close_fd = CloseFD(fd); + + struct fuse_attr attr = DefaultFuseAttr(expected_mode, 2); + struct fuse_out_header out_header = { + .len = sizeof(struct fuse_out_header) + sizeof(struct fuse_attr_out), + }; + struct fuse_attr_out out_payload = { + .attr = attr, + }; + auto iov_out = FuseGenerateIovecs(out_header, out_payload); + SetServerResponse(FUSE_GETATTR, iov_out); + + // Make syscall. + struct stat stat_buf; + EXPECT_THAT(fstat(fd.get(), &stat_buf), SyscallSucceeds()); + + // Check filesystem operation result. + struct stat expected_stat = { + .st_ino = attr.ino, + .st_nlink = attr.nlink, + .st_mode = expected_mode, + .st_uid = attr.uid, + .st_gid = attr.gid, + .st_rdev = attr.rdev, + .st_size = static_cast(attr.size), + .st_blksize = attr.blksize, + .st_blocks = static_cast(attr.blocks), + .st_atim = (struct timespec){.tv_sec = static_cast(attr.atime), + .tv_nsec = attr.atimensec}, + .st_mtim = (struct timespec){.tv_sec = static_cast(attr.mtime), + .tv_nsec = attr.mtimensec}, + .st_ctim = (struct timespec){.tv_sec = static_cast(attr.ctime), + .tv_nsec = attr.ctimensec}, + }; + EXPECT_TRUE(StatsAreEqual(stat_buf, expected_stat)); + + // Check FUSE request. + struct fuse_in_header in_header; + struct fuse_getattr_in in_payload; + auto iov_in = FuseGenerateIovecs(in_header, in_payload); + + GetServerActualRequest(iov_in); + EXPECT_EQ(in_header.opcode, FUSE_GETATTR); + EXPECT_EQ(in_payload.getattr_flags, 0); + EXPECT_EQ(in_payload.fh, 0); +} + +TEST_F(StatTest, StatByFileHandle) { + // Set up fixture. + SetServerInodeLookup(test_file_, expected_mode, 0); + auto fd = ASSERT_NO_ERRNO_AND_VALUE(OpenPath(test_file_path_, O_RDONLY, fh)); + auto close_fd = CloseFD(fd); + + struct fuse_attr attr = DefaultFuseAttr(expected_mode, 2, 0); + struct fuse_out_header out_header = { + .len = sizeof(struct fuse_out_header) + sizeof(struct fuse_attr_out), + }; + struct fuse_attr_out out_payload = { + .attr = attr, + }; + auto iov_out = FuseGenerateIovecs(out_header, out_payload); + SetServerResponse(FUSE_GETATTR, iov_out); + + // Make syscall. + std::vector buf(1); + // Since this is an empty file, it won't issue FUSE_READ. But a FUSE_GETATTR + // will be issued before read completes. + EXPECT_THAT(read(fd.get(), buf.data(), buf.size()), SyscallSucceeds()); + + // Check FUSE request. + struct fuse_in_header in_header; + struct fuse_getattr_in in_payload; + auto iov_in = FuseGenerateIovecs(in_header, in_payload); + + GetServerActualRequest(iov_in); + EXPECT_EQ(in_header.opcode, FUSE_GETATTR); + EXPECT_EQ(in_payload.getattr_flags, FUSE_GETATTR_FH); + EXPECT_EQ(in_payload.fh, fh); +} + } // namespace } // namespace testing diff --git a/test/util/fuse_util.cc b/test/util/fuse_util.cc index 4db053335..595d0cebf 100644 --- a/test/util/fuse_util.cc +++ b/test/util/fuse_util.cc @@ -22,13 +22,13 @@ namespace gvisor { namespace testing { -// Create a default FuseAttr struct with specified mode and inode. -fuse_attr DefaultFuseAttr(mode_t mode, uint64_t inode) { +// Create a default FuseAttr struct with specified mode, inode, and size. +fuse_attr DefaultFuseAttr(mode_t mode, uint64_t inode, uint64_t size) { const int time_sec = 1595436289; const int time_nsec = 134150844; return (struct fuse_attr){ .ino = inode, - .size = 512, + .size = size, .blocks = 4, .atime = time_sec, .mtime = time_sec, @@ -45,8 +45,8 @@ fuse_attr DefaultFuseAttr(mode_t mode, uint64_t inode) { }; } -// Create response body with specified mode and nodeID. -fuse_entry_out DefaultEntryOut(mode_t mode, uint64_t node_id) { +// Create response body with specified mode, nodeID, and size. +fuse_entry_out DefaultEntryOut(mode_t mode, uint64_t node_id, uint64_t size) { struct fuse_entry_out default_entry_out = { .nodeid = node_id, .generation = 0, @@ -54,7 +54,7 @@ fuse_entry_out DefaultEntryOut(mode_t mode, uint64_t node_id) { .attr_valid = 0, .entry_valid_nsec = 0, .attr_valid_nsec = 0, - .attr = DefaultFuseAttr(mode, node_id), + .attr = DefaultFuseAttr(mode, node_id, size), }; return default_entry_out; }; diff --git a/test/util/fuse_util.h b/test/util/fuse_util.h index 6b5a8ce1f..544fe1b38 100644 --- a/test/util/fuse_util.h +++ b/test/util/fuse_util.h @@ -64,10 +64,11 @@ std::vector FuseGenerateIovecs(T &first, Types &...args) { } // Create a fuse_attr filled with the specified mode and inode. -fuse_attr DefaultFuseAttr(mode_t mode, uint64_t inode); +fuse_attr DefaultFuseAttr(mode_t mode, uint64_t inode, uint64_t size = 512); // Return a fuse_entry_out FUSE server response body. -fuse_entry_out DefaultEntryOut(mode_t mode, uint64_t nodeId); +fuse_entry_out DefaultEntryOut(mode_t mode, uint64_t node_id, + uint64_t size = 512); } // namespace testing } // namespace gvisor