Merge release-20201030.0-93-g89517eca4 (automated)

4 files changed, 28 insertions, 21 deletions

diff --git a/pkg/sentry/fsimpl/fuse/connection.go b/pkg/sentry/fsimpl/fuse/connection.go
index 8ccda1264..34d25a61e 100644
--- a/pkg/sentry/fsimpl/fuse/connection.go
+++ b/pkg/sentry/fsimpl/fuse/connection.go
@@ -21,7 +21,6 @@ import (
 	"gvisor.dev/gvisor/pkg/context"
 	"gvisor.dev/gvisor/pkg/log"
 	"gvisor.dev/gvisor/pkg/sentry/kernel"
-	"gvisor.dev/gvisor/pkg/sentry/vfs"
 	"gvisor.dev/gvisor/pkg/syserror"
 	"gvisor.dev/gvisor/pkg/waiter"
 )
@@ -193,11 +192,12 @@ func (conn *connection) loadInitializedChan(closed bool) {
 	}
 }
 
-// newFUSEConnection creates a FUSE connection to fd.
-func newFUSEConnection(_ context.Context, fd *vfs.FileDescription, opts *filesystemOptions) (*connection, error) {
-	// Mark the device as ready so it can be used. /dev/fuse can only be used if the FD was used to
-	// mount a FUSE filesystem.
-	fuseFD := fd.Impl().(*DeviceFD)
+// newFUSEConnection creates a FUSE connection to fuseFD.
+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
+	// synchronization and without checking if fuseFD has already been used to
+	// mount another filesystem.
 
 	// Create the writeBuf for the header to be stored in.
 	hdrLen := uint32((*linux.FUSEHeaderOut)(nil).SizeBytes())
diff --git a/pkg/sentry/fsimpl/fuse/connection_control.go b/pkg/sentry/fsimpl/fuse/connection_control.go
index bfde78559..1b3459c1d 100644
--- a/pkg/sentry/fsimpl/fuse/connection_control.go
+++ b/pkg/sentry/fsimpl/fuse/connection_control.go
@@ -198,7 +198,6 @@ func (conn *connection) Abort(ctx context.Context) {
 	if !conn.connected {
 		conn.asyncMu.Unlock()
 		conn.mu.Unlock()
-		conn.fd.mu.Unlock()
 		return
 	}
 
diff --git a/pkg/sentry/fsimpl/fuse/dev.go b/pkg/sentry/fsimpl/fuse/dev.go
index 1b86a4b4c..89c3ef079 100644
--- a/pkg/sentry/fsimpl/fuse/dev.go
+++ b/pkg/sentry/fsimpl/fuse/dev.go
@@ -94,7 +94,8 @@ type DeviceFD struct {
 	// unprocessed in-flight requests.
 	fullQueueCh chan struct{} `state:".(int)"`
 
-	// fs is the FUSE filesystem that this FD is being used for.
+	// fs is the FUSE filesystem that this FD is being used for. A reference is
+	// held on fs.
 	fs *filesystem
 }
 
@@ -135,12 +136,6 @@ func (fd *DeviceFD) Read(ctx context.Context, dst usermem.IOSequence, opts vfs.R
 		return 0, syserror.EPERM
 	}
 
-	// Return ENODEV if the filesystem is umounted.
-	if fd.fs.umounted {
-		// TODO(gvisor.dev/issue/3525): return ECONNABORTED if aborted via fuse control fs.
-		return 0, syserror.ENODEV
-	}
-
 	// We require that any Read done on this filesystem have a sane minimum
 	// read buffer. It must have the capacity for the fixed parts of any request
 	// header (Linux uses the request header and the FUSEWriteIn header for this
diff --git a/pkg/sentry/fsimpl/fuse/fusefs.go b/pkg/sentry/fsimpl/fuse/fusefs.go
index cd0eb56e5..23e827f90 100644
--- a/pkg/sentry/fsimpl/fuse/fusefs.go
+++ b/pkg/sentry/fsimpl/fuse/fusefs.go
@@ -127,7 +127,13 @@ func (fsType FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt
 		log.Warningf("%s.GetFilesystem: couldn't get kernel task from context", fsType.Name())
 		return nil, nil, syserror.EINVAL
 	}
-	fuseFd := kernelTask.GetFileVFS2(int32(deviceDescriptor))
+	fuseFDGeneric := kernelTask.GetFileVFS2(int32(deviceDescriptor))
+	defer fuseFDGeneric.DecRef(ctx)
+	fuseFD, ok := fuseFDGeneric.Impl().(*DeviceFD)
+	if !ok {
+		log.Warningf("%s.GetFilesystem: device FD is %T, not a FUSE device", fsType.Name, fuseFDGeneric)
+		return nil, nil, syserror.EINVAL
+	}
 
 	// Parse and set all the other supported FUSE mount options.
 	// TODO(gVisor.dev/issue/3229): Expand the supported mount options.
@@ -189,18 +195,17 @@ func (fsType FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt
 	}
 
 	// Create a new FUSE filesystem.
-	fs, err := newFUSEFilesystem(ctx, devMinor, &fsopts, fuseFd)
+	fs, err := newFUSEFilesystem(ctx, vfsObj, &fsType, fuseFD, devMinor, &fsopts)
 	if err != nil {
 		log.Warningf("%s.NewFUSEFilesystem: failed with error: %v", fsType.Name(), err)
 		return nil, nil, err
 	}
 
-	fs.VFSFilesystem().Init(vfsObj, &fsType, fs)
-
 	// 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
 	}
 
@@ -211,20 +216,28 @@ func (fsType FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt
 }
 
 // newFUSEFilesystem creates a new FUSE filesystem.
-func newFUSEFilesystem(ctx context.Context, devMinor uint32, opts *filesystemOptions, device *vfs.FileDescription) (*filesystem, error) {
-	conn, err := newFUSEConnection(ctx, device, opts)
+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, syserror.EINVAL
 	}
 
-	fuseFD := device.Impl().(*DeviceFD)
 	fs := &filesystem{
 		devMinor: devMinor,
 		opts:     opts,
 		conn:     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
+	// FIXME(gvisor.dev/issue/4813): fuseFD.fs is accessed without
+	// synchronization.
 	fuseFD.fs = fs
+
 	return fs, nil
 }