From 32c474d82f653e0d25b77fb07f29f55a769802a0 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Mon, 2 May 2022 11:38:55 -0700 Subject: [PATCH] Allow multiple FUSE filesystems to share a connection. Before this change FUSE connections were shared 1:1 with FUSE filesystems, which is incorrect behavior. A FUSE FD should have a 1:1 relationship with a FUSE connection, and any number of FUSE filesystems can use the same connection. PiperOrigin-RevId: 445988328 --- pkg/sentry/fsimpl/fuse/BUILD | 1 + pkg/sentry/fsimpl/fuse/connection.go | 3 +- pkg/sentry/fsimpl/fuse/connection_control.go | 2 +- pkg/sentry/fsimpl/fuse/connection_test.go | 2 + pkg/sentry/fsimpl/fuse/dev.go | 163 +++++++++++-------- pkg/sentry/fsimpl/fuse/dev_state.go | 23 +++ pkg/sentry/fsimpl/fuse/dev_test.go | 25 ++- pkg/sentry/fsimpl/fuse/fusefs.go | 52 ++---- pkg/sentry/fsimpl/fuse/utils_test.go | 33 +++- test/fuse/BUILD | 5 + test/fuse/linux/BUILD | 4 + test/fuse/linux/fuse_base.cc | 19 ++- test/fuse/linux/fuse_base.h | 8 +- test/fuse/linux/mount_test.cc | 84 ++++++++++ 14 files changed, 307 insertions(+), 117 deletions(-) create mode 100644 pkg/sentry/fsimpl/fuse/dev_state.go diff --git a/pkg/sentry/fsimpl/fuse/BUILD b/pkg/sentry/fsimpl/fuse/BUILD index 1a97f240f..955160757 100644 --- a/pkg/sentry/fsimpl/fuse/BUILD +++ b/pkg/sentry/fsimpl/fuse/BUILD @@ -32,6 +32,7 @@ go_library( "connection.go", "connection_control.go", "dev.go", + "dev_state.go", "directory.go", "file.go", "fusefs.go", diff --git a/pkg/sentry/fsimpl/fuse/connection.go b/pkg/sentry/fsimpl/fuse/connection.go index 07fc8e035..240fdf5aa 100644 --- a/pkg/sentry/fsimpl/fuse/connection.go +++ b/pkg/sentry/fsimpl/fuse/connection.go @@ -202,6 +202,7 @@ func (conn *connection) loadInitializedChan(closed bool) { } // newFUSEConnection creates a FUSE connection to fuseFD. +// +checklocks:fuseFD.mu func newFUSEConnection(_ context.Context, fuseFD *DeviceFD, opts *filesystemOptions) (*connection, error) { // Mark the device as ready so it can be used. // FIXME(gvisor.dev/issue/4813): fuseFD's fields are accessed without @@ -210,12 +211,10 @@ func newFUSEConnection(_ context.Context, fuseFD *DeviceFD, opts *filesystemOpti // Create the writeBuf for the header to be stored in. hdrLen := uint32((*linux.FUSEHeaderOut)(nil).SizeBytes()) - fuseFD.mu.Lock() fuseFD.writeBuf = make([]byte, hdrLen) fuseFD.completions = make(map[linux.FUSEOpID]*futureResponse) fuseFD.fullQueueCh = make(chan struct{}, opts.maxActiveRequests) fuseFD.writeCursor = 0 - fuseFD.mu.Unlock() return &connection{ fd: fuseFD, diff --git a/pkg/sentry/fsimpl/fuse/connection_control.go b/pkg/sentry/fsimpl/fuse/connection_control.go index 5204f893d..14461f924 100644 --- a/pkg/sentry/fsimpl/fuse/connection_control.go +++ b/pkg/sentry/fsimpl/fuse/connection_control.go @@ -188,7 +188,7 @@ func (conn *connection) initProcessReply(out *linux.FUSEInitOut, hasSysAdminCap // It tries to acquire conn.fd.mu, conn.lock, conn.bgLock in order. // All possible requests waiting or blocking will be aborted. // -// Preconditions: conn.fd.mu is locked. +// +checklocks:conn.fd.mu func (conn *connection) Abort(ctx context.Context) { conn.mu.Lock() conn.asyncMu.Lock() diff --git a/pkg/sentry/fsimpl/fuse/connection_test.go b/pkg/sentry/fsimpl/fuse/connection_test.go index 11eff255b..848a5bbf5 100644 --- a/pkg/sentry/fsimpl/fuse/connection_test.go +++ b/pkg/sentry/fsimpl/fuse/connection_test.go @@ -83,7 +83,9 @@ func TestConnectionAbort(t *testing.T) { futNormal = append(futNormal, fut) } + conn.fd.mu.Lock() conn.Abort(s.Ctx) + conn.fd.mu.Unlock() // Abort should unblock the initialization channel. // Note: no test requests are actually blocked on `conn.initializedChan`. diff --git a/pkg/sentry/fsimpl/fuse/dev.go b/pkg/sentry/fsimpl/fuse/dev.go index fba7168e2..87a7a6c05 100644 --- a/pkg/sentry/fsimpl/fuse/dev.go +++ b/pkg/sentry/fsimpl/fuse/dev.go @@ -58,32 +58,6 @@ type DeviceFD struct { vfs.DentryMetadataFileDescriptionImpl vfs.NoLockFD - // nextOpID is used to create new requests. - nextOpID linux.FUSEOpID - - // queue is the list of requests that need to be processed by the FUSE server. - queue requestList - - // numActiveRequests is the number of requests made by the Sentry that has - // yet to be responded to. - numActiveRequests uint64 - - // completions is used to map a request to its response. A Writer will use this - // to notify the caller of a completed response. - completions map[linux.FUSEOpID]*futureResponse - - writeCursor uint32 - - // writeBuf is the memory buffer used to copy in the FUSE out header from - // userspace. - writeBuf []byte - - // writeCursorFR current FR being copied from server. - writeCursorFR *futureResponse - - // mu protects all the queues, maps, buffers and cursors and nextOpID. - mu sync.Mutex `state:"nosave"` - // waitQueue is used to notify interested parties when the device becomes // readable or writable. waitQueue waiter.Queue @@ -93,42 +67,79 @@ type DeviceFD struct { // unprocessed in-flight requests. fullQueueCh chan struct{} `state:".(int)"` - // fs is the FUSE filesystem that this FD is being used for. A reference is - // held on fs. - fs *filesystem -} + // mu protects all the queues, maps, buffers and cursors and nextOpID. + mu sync.Mutex `state:"nosave"` -func (fd *DeviceFD) saveFullQueueCh() int { - return cap(fd.fullQueueCh) -} + // nextOpID is used to create new requests. + // +checklocks:mu + nextOpID linux.FUSEOpID -func (fd *DeviceFD) loadFullQueueCh(capacity int) { - fd.fullQueueCh = make(chan struct{}, capacity) + // queue is the list of requests that need to be processed by the FUSE server. + // +checklocks:mu + queue requestList + + // numActiveRequests is the number of requests made by the Sentry that has + // yet to be responded to. + // +checklocks:mu + numActiveRequests uint64 + + // completions is used to map a request to its response. A Writer will use this + // to notify the caller of a completed response. + // +checklocks:mu + completions map[linux.FUSEOpID]*futureResponse + + // +checklocks:mu + writeCursor uint32 + + // writeBuf is the memory buffer used to copy in the FUSE out header from + // userspace. + // +checklocks:mu + writeBuf []byte + + // writeCursorFR current FR being copied from server. + // +checklocks:mu + writeCursorFR *futureResponse + + // conn is the FUSE connection that this FD is being used for. + // +checklocks:mu + conn *connection } // Release implements vfs.FileDescriptionImpl.Release. func (fd *DeviceFD) Release(ctx context.Context) { - if fd.fs != nil { - fd.fs.conn.mu.Lock() - fd.fs.conn.connected = false - fd.fs.conn.mu.Unlock() + fd.mu.Lock() + defer fd.mu.Unlock() + if fd.conn != nil { + fd.conn.mu.Lock() + fd.conn.connected = false + fd.conn.mu.Unlock() - fd.fs.VFSFilesystem().DecRef(ctx) - fd.fs = nil + fd.conn.Abort(ctx) // +checklocksforce: fd.conn.fd.mu=fd.mu + fd.waitQueue.Notify(waiter.ReadableEvents) + fd.conn = nil } } -// filesystemIsInitialized returns true if fd.fs is set and the connection is -// initialized. -func (fd *DeviceFD) filesystemIsInitialized() bool { - // FIXME(gvisor.dev/issue/4813): Access to fd.fs should be synchronized. - return fd.fs != nil +// connected returns true if fd.conn is set and the connection has not been +// aborted. +// +checklocks:fd.mu +func (fd *DeviceFD) connected() bool { + if fd.conn != nil { + fd.conn.mu.Lock() + defer fd.conn.mu.Unlock() + return fd.conn.connected + } + return false } // PRead implements vfs.FileDescriptionImpl.PRead. func (fd *DeviceFD) PRead(ctx context.Context, dst usermem.IOSequence, offset int64, opts vfs.ReadOptions) (int64, error) { - // Operations on /dev/fuse don't make sense until a FUSE filesystem is mounted. - if !fd.filesystemIsInitialized() { + // Operations on /dev/fuse don't make sense until a FUSE filesystem is + // mounted. If there is an active connection we know there is at least one + // filesystem mounted. + fd.mu.Lock() + defer fd.mu.Unlock() + if !fd.connected() { return 0, linuxerr.EPERM } @@ -137,8 +148,12 @@ func (fd *DeviceFD) PRead(ctx context.Context, dst usermem.IOSequence, offset in // Read implements vfs.FileDescriptionImpl.Read. func (fd *DeviceFD) Read(ctx context.Context, dst usermem.IOSequence, opts vfs.ReadOptions) (int64, error) { - // Operations on /dev/fuse don't make sense until a FUSE filesystem is mounted. - if !fd.filesystemIsInitialized() { + // Operations on /dev/fuse don't make sense until a FUSE filesystem is + // mounted. If there is an active connection we know there is at least one + // filesystem mounted. + fd.mu.Lock() + defer fd.mu.Unlock() + if !fd.connected() { return 0, linuxerr.EPERM } @@ -150,11 +165,9 @@ func (fd *DeviceFD) Read(ctx context.Context, dst usermem.IOSequence, opts vfs.R inHdrLen := uint32((*linux.FUSEHeaderIn)(nil).SizeBytes()) writeHdrLen := uint32((*linux.FUSEWriteIn)(nil).SizeBytes()) - fd.mu.Lock() - defer fd.mu.Unlock() - fd.fs.conn.mu.Lock() - negotiatedMinBuffSize := inHdrLen + writeHdrLen + fd.fs.conn.maxWrite - fd.fs.conn.mu.Unlock() + fd.conn.mu.Lock() + negotiatedMinBuffSize := inHdrLen + writeHdrLen + fd.conn.maxWrite + fd.conn.mu.Unlock() if minBuffSize < negotiatedMinBuffSize { minBuffSize = negotiatedMinBuffSize } @@ -231,8 +244,12 @@ func (fd *DeviceFD) readLocked(ctx context.Context, dst usermem.IOSequence, opts // PWrite implements vfs.FileDescriptionImpl.PWrite. func (fd *DeviceFD) PWrite(ctx context.Context, src usermem.IOSequence, offset int64, opts vfs.WriteOptions) (int64, error) { - // Operations on /dev/fuse don't make sense until a FUSE filesystem is mounted. - if !fd.filesystemIsInitialized() { + // Operations on /dev/fuse don't make sense until a FUSE filesystem is + // mounted. If there is an active connection we know there is at least one + // filesystem mounted. + fd.mu.Lock() + defer fd.mu.Unlock() + if !fd.connected() { return 0, linuxerr.EPERM } @@ -249,16 +266,13 @@ func (fd *DeviceFD) Write(ctx context.Context, src usermem.IOSequence, opts vfs. // writeLocked implements writing to the fuse device while locked with DeviceFD.mu. // +checklocks:fd.mu func (fd *DeviceFD) writeLocked(ctx context.Context, src usermem.IOSequence, opts vfs.WriteOptions) (int64, error) { - // Operations on /dev/fuse don't make sense until a FUSE filesystem is mounted. - if !fd.filesystemIsInitialized() { + // Operations on /dev/fuse don't make sense until a FUSE filesystem is + // mounted. If there is an active connection we know there is at least one + // filesystem mounted. + if !fd.connected() { return 0, linuxerr.EPERM } - // Return ENODEV if the filesystem is umounted. - if fd.fs.umounted { - return 0, linuxerr.ENODEV - } - var cn, n int64 hdrLen := uint32((*linux.FUSEHeaderOut)(nil).SizeBytes()) @@ -363,7 +377,7 @@ func (fd *DeviceFD) Readiness(mask waiter.EventMask) waiter.EventMask { func (fd *DeviceFD) readinessLocked(mask waiter.EventMask) waiter.EventMask { var ready waiter.EventMask - if !fd.filesystemIsInitialized() || fd.fs.umounted { + if !fd.connected() { ready |= waiter.EventErr return ready & mask } @@ -380,12 +394,16 @@ func (fd *DeviceFD) readinessLocked(mask waiter.EventMask) waiter.EventMask { // EventRegister implements waiter.Waitable.EventRegister. func (fd *DeviceFD) EventRegister(e *waiter.Entry) error { + fd.mu.Lock() + defer fd.mu.Unlock() fd.waitQueue.EventRegister(e) return nil } // EventUnregister implements waiter.Waitable.EventUnregister. func (fd *DeviceFD) EventUnregister(e *waiter.Entry) { + fd.mu.Lock() + defer fd.mu.Unlock() fd.waitQueue.EventUnregister(e) } @@ -396,8 +414,12 @@ func (fd *DeviceFD) Epollable() bool { // Seek implements vfs.FileDescriptionImpl.Seek. func (fd *DeviceFD) Seek(ctx context.Context, offset int64, whence int32) (int64, error) { - // Operations on /dev/fuse don't make sense until a FUSE filesystem is mounted. - if !fd.filesystemIsInitialized() { + // Operations on /dev/fuse don't make sense until a FUSE filesystem is + // mounted. If there is an active connection we know there is at least one + // filesystem mounted. + fd.mu.Lock() + defer fd.mu.Unlock() + if !fd.connected() { return 0, linuxerr.EPERM } @@ -406,7 +428,7 @@ func (fd *DeviceFD) Seek(ctx context.Context, offset int64, whence int32) (int64 // sendResponse sends a response to the waiting task (if any). // -// Preconditions: fd.mu must be held. +// +checklocks:fd.mu func (fd *DeviceFD) sendResponse(ctx context.Context, fut *futureResponse) error { // Signal the task waiting on a response if any. defer close(fut.ch) @@ -427,7 +449,7 @@ func (fd *DeviceFD) sendResponse(ctx context.Context, fut *futureResponse) error // sendError sends an error response to the waiting task (if any) by calling sendResponse(). // -// Preconditions: fd.mu must be held. +// +checklocks:fd.mu func (fd *DeviceFD) sendError(ctx context.Context, errno int32, unique linux.FUSEOpID) error { // Return the error to the calling task. outHdrLen := uint32((*linux.FUSEHeaderOut)(nil).SizeBytes()) @@ -451,12 +473,13 @@ func (fd *DeviceFD) sendError(ctx context.Context, errno int32, unique linux.FUS // asyncCallBack executes pre-defined callback function for async requests. // Currently used by: FUSE_INIT. +// +checklocks:fd.mu func (fd *DeviceFD) asyncCallBack(ctx context.Context, r *Response) error { switch r.opcode { case linux.FUSE_INIT: creds := auth.CredentialsFromContext(ctx) rootUserNs := kernel.KernelFromContext(ctx).RootUserNamespace() - return fd.fs.conn.InitRecv(r, creds.HasCapabilityIn(linux.CAP_SYS_ADMIN, rootUserNs)) + return fd.conn.InitRecv(r, creds.HasCapabilityIn(linux.CAP_SYS_ADMIN, rootUserNs)) // TODO(gvisor.dev/issue/3247): support async read: correctly process the response. } diff --git a/pkg/sentry/fsimpl/fuse/dev_state.go b/pkg/sentry/fsimpl/fuse/dev_state.go new file mode 100644 index 000000000..0c79566f9 --- /dev/null +++ b/pkg/sentry/fsimpl/fuse/dev_state.go @@ -0,0 +1,23 @@ +// Copyright 2022 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package fuse + +func (fd *DeviceFD) saveFullQueueCh() int { + return cap(fd.fullQueueCh) +} + +func (fd *DeviceFD) loadFullQueueCh(capacity int) { + fd.fullQueueCh = make(chan struct{}, capacity) +} diff --git a/pkg/sentry/fsimpl/fuse/dev_test.go b/pkg/sentry/fsimpl/fuse/dev_test.go index cf494d2f8..8f39e12aa 100644 --- a/pkg/sentry/fsimpl/fuse/dev_test.go +++ b/pkg/sentry/fsimpl/fuse/dev_test.go @@ -136,6 +136,29 @@ func TestFUSECommunication(t *testing.T) { } } +func TestReuseFd(t *testing.T) { + s := setup(t) + defer s.Destroy() + k := kernel.KernelFromContext(s.Ctx) + _, fd, err := newTestConnection(s, k, maxActiveRequestsDefault) + if err != nil { + t.Fatalf("newTestConnection: %v", err) + } + fs1, err := newTestFilesystem(s, fd, maxActiveRequestsDefault) + if err != nil { + t.Fatalf("newTestFilesystem: %v", err) + } + defer fs1.Release(s.Ctx) + fs2, err := newTestFilesystem(s, fd, maxActiveRequestsDefault) + if err != nil { + t.Fatalf("newTestFilesystem: %v", err) + } + defer fs2.Release(s.Ctx) + if fs1.conn != fs2.conn { + t.Errorf("second fs connection = %v, want = %v", fs2.conn, fs1.conn) + } +} + // CallTest makes a request to the server and blocks the invoking // goroutine until a server responds with a response. Doesn't block // a kernel.Task. Analogous to Connection.Call but used for testing. @@ -143,7 +166,7 @@ func CallTest(conn *connection, t *kernel.Task, r *Request, i uint32) (*Response conn.fd.mu.Lock() // Wait until we're certain that a new request can be processed. - for conn.fd.numActiveRequests == conn.fd.fs.opts.maxActiveRequests { + for conn.fd.numActiveRequests == conn.maxActiveRequests { conn.fd.mu.Unlock() select { case <-conn.fd.fullQueueCh: diff --git a/pkg/sentry/fsimpl/fuse/fusefs.go b/pkg/sentry/fsimpl/fuse/fusefs.go index ae927da20..2ec5f6760 100644 --- a/pkg/sentry/fsimpl/fuse/fusefs.go +++ b/pkg/sentry/fsimpl/fuse/fusefs.go @@ -31,7 +31,6 @@ import ( "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/vfs" - "gvisor.dev/gvisor/pkg/waiter" ) // Name is the default filesystem name. @@ -97,9 +96,6 @@ type filesystem struct { // opts is the options the fusefs is initialized with. opts *filesystemOptions - - // umounted is true if filesystem.Release() has been called. - umounted bool } // Name implements vfs.FilesystemType.Name. @@ -233,19 +229,24 @@ func (fsType FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt return nil, nil, linuxerr.EINVAL } + fuseFD.mu.Lock() + connected := fuseFD.connected() // Create a new FUSE filesystem. fs, err := newFUSEFilesystem(ctx, vfsObj, &fsType, fuseFD, devMinor, &fsopts) if err != nil { log.Warningf("%s.NewFUSEFilesystem: failed with error: %v", fsType.Name(), err) + fuseFD.mu.Unlock() return nil, nil, err } + fuseFD.mu.Unlock() // Send a FUSE_INIT request to the FUSE daemon server before returning. // This call is not blocking. - if err := fs.conn.InitSend(creds, uint32(kernelTask.ThreadID())); err != nil { - log.Warningf("%s.InitSend: failed with error: %v", fsType.Name(), err) - fs.VFSFilesystem().DecRef(ctx) // returned by newFUSEFilesystem - return nil, nil, err + if !connected { + if err := fs.conn.InitSend(creds, uint32(kernelTask.ThreadID())); err != nil { + log.Warningf("%s.InitSend: failed with error: %v", fsType.Name(), err) + return nil, nil, err + } } // root is the fusefs root directory. @@ -255,45 +256,28 @@ func (fsType FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt } // newFUSEFilesystem creates a new FUSE filesystem. +// +checklocks:fuseFD.mu func newFUSEFilesystem(ctx context.Context, vfsObj *vfs.VirtualFilesystem, fsType *FilesystemType, fuseFD *DeviceFD, devMinor uint32, opts *filesystemOptions) (*filesystem, error) { - conn, err := newFUSEConnection(ctx, fuseFD, opts) - if err != nil { - log.Warningf("fuse.NewFUSEFilesystem: NewFUSEConnection failed with error: %v", err) - return nil, linuxerr.EINVAL + if !fuseFD.connected() { + conn, err := newFUSEConnection(ctx, fuseFD, opts) + if err != nil { + log.Warningf("fuse.NewFUSEFilesystem: NewFUSEConnection failed with error: %v", err) + return nil, linuxerr.EINVAL + } + fuseFD.conn = conn } fs := &filesystem{ devMinor: devMinor, opts: opts, - conn: conn, + conn: fuseFD.conn, } fs.VFSFilesystem().Init(vfsObj, fsType, fs) - - // FIXME(gvisor.dev/issue/4813): Doesn't conn or fs need to hold a - // reference on fuseFD, since conn uses fuseFD for communication with the - // server? Wouldn't doing so create a circular reference? - fs.VFSFilesystem().IncRef() // for fuseFD.fs - - fuseFD.mu.Lock() - fs.conn.mu.Lock() - fuseFD.fs = fs - fs.conn.mu.Unlock() - fuseFD.mu.Unlock() - return fs, nil } // Release implements vfs.FilesystemImpl.Release. func (fs *filesystem) Release(ctx context.Context) { - fs.conn.fd.mu.Lock() - - fs.umounted = true - fs.conn.Abort(ctx) - // Notify all the waiters on this fd. - fs.conn.fd.waitQueue.Notify(waiter.ReadableEvents) - - fs.conn.fd.mu.Unlock() - fs.Filesystem.VFSFilesystem().VirtualFilesystem().PutAnonBlockDevMinor(fs.devMinor) fs.Filesystem.Release(ctx) } diff --git a/pkg/sentry/fsimpl/fuse/utils_test.go b/pkg/sentry/fsimpl/fuse/utils_test.go index 8d4a2fad3..73c53e4bd 100644 --- a/pkg/sentry/fsimpl/fuse/utils_test.go +++ b/pkg/sentry/fsimpl/fuse/utils_test.go @@ -15,6 +15,7 @@ package fuse import ( + "fmt" "testing" "gvisor.dev/gvisor/pkg/abi/linux" @@ -60,9 +61,37 @@ func newTestConnection(system *testutil.System, k *kernel.Kernel, maxActiveReque fsopts := filesystemOptions{ maxActiveRequests: maxActiveRequests, } - fs, err := newFUSEFilesystem(system.Ctx, system.VFS, &FilesystemType{}, fuseDev, 0, &fsopts) + fuseDev.mu.Lock() + conn, err := newFUSEConnection(system.Ctx, fuseDev, &fsopts) if err != nil { return nil, nil, err } - return fs.conn, &fuseDev.vfsfd, nil + fuseDev.conn = conn + fuseDev.mu.Unlock() + + // Fake the connection being properly initialized for testing purposes. + conn.mu.Lock() + conn.connInitSuccess = true + conn.mu.Unlock() + return conn, &fuseDev.vfsfd, nil +} + +// newTestFilesystem creates a filesystem that the sentry can communicate with +// and the FD for the server to communicate with. +func newTestFilesystem(system *testutil.System, fd *vfs.FileDescription, maxActiveRequests uint64) (*filesystem, error) { + fuseFD, ok := fd.Impl().(*DeviceFD) + if !ok { + return nil, fmt.Errorf("newTestFilesystem: FD is %T, not a FUSE device", fd) + } + fsopts := filesystemOptions{ + maxActiveRequests: maxActiveRequests, + } + + fuseFD.mu.Lock() + defer fuseFD.mu.Unlock() + fs, err := newFUSEFilesystem(system.Ctx, system.VFS, &FilesystemType{}, fuseFD, 0, &fsopts) + if err != nil { + return nil, err + } + return fs, nil } diff --git a/test/fuse/BUILD b/test/fuse/BUILD index 0073f9f28..353942580 100644 --- a/test/fuse/BUILD +++ b/test/fuse/BUILD @@ -76,3 +76,8 @@ syscall_test( fuse = "True", test = "//test/fuse/linux:setstat_test", ) + +syscall_test( + fuse = "True", + test = "//test/fuse/linux:mount_test", +) diff --git a/test/fuse/linux/BUILD b/test/fuse/linux/BUILD index e355d923f..e5d2b205b 100644 --- a/test/fuse/linux/BUILD +++ b/test/fuse/linux/BUILD @@ -252,8 +252,12 @@ cc_binary( srcs = ["mount_test.cc"], deps = [ gtest, + ":fuse_base", + "//test/util:fs_util", + "//test/util:fuse_util", "//test/util:mount_util", "//test/util:temp_path", + "//test/util:temp_umask", "//test/util:test_main", "//test/util:test_util", ], diff --git a/test/fuse/linux/fuse_base.cc b/test/fuse/linux/fuse_base.cc index a6a8db717..1cd16067f 100644 --- a/test/fuse/linux/fuse_base.cc +++ b/test/fuse/linux/fuse_base.cc @@ -24,6 +24,7 @@ #include #include +#include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/strings/str_format.h" #include "test/util/fuse_util.h" @@ -149,13 +150,20 @@ void FuseTest::SetServerInodeLookup(const std::string& path, mode_t mode, WaitServerComplete(); } -void FuseTest::MountFuse(const char* mountOpts) { - EXPECT_THAT(dev_fd_ = open("/dev/fuse", O_RDWR), SyscallSucceeds()); +void FuseTest::MountFuse(const char* mount_opts) { + int dev_fd; + EXPECT_THAT(dev_fd = open("/dev/fuse", O_RDWR), SyscallSucceeds()); + std::string fmt_mount_opts = absl::StrFormat("fd=%d,%s", dev_fd, mount_opts); + TempPath mount_point = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); + MountFuse(dev_fd, mount_point, fmt_mount_opts.c_str()); +} - std::string mount_opts = absl::StrFormat("fd=%d,%s", dev_fd_, mountOpts); - mount_point_ = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); +void FuseTest::MountFuse(int fd, TempPath& mount_point, + const char* mount_opts) { + mount_point_ = std::move(mount_point); + dev_fd_ = fd; EXPECT_THAT(mount("fuse", mount_point_.path().c_str(), "fuse", - MS_NODEV | MS_NOSUID, mount_opts.c_str()), + MS_NODEV | MS_NOSUID, mount_opts), SyscallSucceeds()); } @@ -163,6 +171,7 @@ void FuseTest::UnmountFuse() { EXPECT_THAT(umount(mount_point_.path().c_str()), SyscallSucceeds()); shutdown(sock_[0], SHUT_RDWR); fuse_server_->Join(); + EXPECT_THAT(close(dev_fd_), SyscallSucceeds()); // TODO(gvisor.dev/issue/3330): ensure the process is terminated successfully. } diff --git a/test/fuse/linux/fuse_base.h b/test/fuse/linux/fuse_base.h index 32ec7d8c2..573fe730a 100644 --- a/test/fuse/linux/fuse_base.h +++ b/test/fuse/linux/fuse_base.h @@ -170,9 +170,14 @@ class FuseTest : public ::testing::Test { protected: TempPath mount_point_; + int dev_fd_; // Opens /dev/fuse and inherit the file descriptor for the FUSE server. - void MountFuse(const char* mountOpts = kMountOpts); + void MountFuse(const char* mount_opts = kMountOpts); + + // Mounts a fuse fs with a fuse fd connection at the specified point. + void MountFuse(int fd, TempPath& mount_point, + const char* mount_opts = kMountOpts); // Creates a socketpair for communication and forks FUSE server. void SetUpFuseServer( @@ -236,7 +241,6 @@ class FuseTest : public ::testing::Test { // Responds an error header to /dev/fuse when bad thing happens. void ServerRespondFuseError(uint64_t unique); - int dev_fd_; int sock_[2]; std::unique_ptr fuse_server_; diff --git a/test/fuse/linux/mount_test.cc b/test/fuse/linux/mount_test.cc index 1e3620d88..28616a433 100644 --- a/test/fuse/linux/mount_test.cc +++ b/test/fuse/linux/mount_test.cc @@ -18,8 +18,12 @@ #include #include "gtest/gtest.h" +#include "test/fuse/linux/fuse_base.h" +#include "test/util/fs_util.h" +#include "test/util/fuse_util.h" #include "test/util/mount_util.h" #include "test/util/temp_path.h" +#include "test/util/temp_umask.h" #include "test/util/test_util.h" namespace gvisor { @@ -27,6 +31,65 @@ namespace testing { namespace { +class MountTest : public FuseTest { + protected: + void CheckFUSECreateFile(std::string_view test_file_path) { + std::string_view test_file_name = Basename(test_file_path.data()); + const mode_t mode = S_IFREG | S_IRWXU | S_IRWXG | S_IRWXO; + // Ensure the file doesn't exist. + struct fuse_out_header out_header = { + .len = sizeof(struct fuse_out_header), + .error = -ENOENT, + }; + auto iov_out = FuseGenerateIovecs(out_header); + SetServerResponse(FUSE_LOOKUP, iov_out); + + // creat(2) is equal to open(2) with open_flags O_CREAT | O_WRONLY | + // O_TRUNC. + const mode_t new_mask = S_IWGRP | S_IWOTH; + const int open_flags = O_CREAT | O_WRONLY | O_TRUNC; + out_header.error = 0; + out_header.len = sizeof(struct fuse_out_header) + + sizeof(struct fuse_entry_out) + + sizeof(struct fuse_open_out); + struct fuse_entry_out entry_payload = DefaultEntryOut(mode & ~new_mask, 2); + struct fuse_open_out out_payload = { + .fh = 1, + .open_flags = open_flags, + }; + iov_out = FuseGenerateIovecs(out_header, entry_payload, out_payload); + SetServerResponse(FUSE_CREATE, iov_out); + + int fd; + TempUmask mask(new_mask); + EXPECT_THAT(fd = creat(test_file_path.data(), mode), SyscallSucceeds()); + EXPECT_THAT(fcntl(fd, F_GETFL), + SyscallSucceedsWithValue(open_flags & O_ACCMODE)); + + struct fuse_in_header in_header; + struct fuse_create_in in_payload; + std::vector name(test_file_name.size() + 1); + auto iov_in = FuseGenerateIovecs(in_header, in_payload, name); + + // Skip the request of FUSE_LOOKUP. + SkipServerActualRequest(); + + // Get the first FUSE_CREATE. + GetServerActualRequest(iov_in); + EXPECT_EQ(in_header.len, sizeof(in_header) + sizeof(in_payload) + + test_file_name.size() + 1); + EXPECT_EQ(in_header.opcode, FUSE_CREATE); + EXPECT_EQ(in_payload.flags, open_flags); + EXPECT_EQ(in_payload.mode, mode & ~new_mask); + EXPECT_EQ(in_payload.umask, new_mask); + EXPECT_EQ(std::string(name.data()), test_file_name); + + EXPECT_THAT(close(fd), SyscallSucceeds()); + // Skip the FUSE_RELEASE. + SkipServerActualRequest(); + } +}; + TEST(FuseMount, Success) { const FileDescriptor fd = ASSERT_NO_ERRNO_AND_VALUE(Open("/dev/fuse", O_WRONLY)); @@ -93,6 +156,27 @@ TEST(FuseMount, BadFD) { SyscallFailsWithErrno(EINVAL)); } +TEST_F(MountTest, ReuseFD) { + std::string mopts = + absl::StrFormat("fd=%d,user_id=%d,group_id=%d,rootmode=0777", dev_fd_, + getuid(), getgid()); + + const std::string test_file1_path = + JoinPath(mount_point_.path().c_str(), "testfile1"); + CheckFUSECreateFile(test_file1_path); + + auto mount_point1 = std::move(mount_point_); + + auto dir2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir()); + MountFuse(dev_fd_, dir2, mopts.c_str()); + + std::string test_file2_path = + JoinPath(mount_point_.path().c_str(), "testfile2"); + CheckFUSECreateFile(test_file2_path); + + EXPECT_THAT(umount(mount_point1.path().c_str()), SyscallSucceeds()); +} + } // namespace } // namespace testing