Clean up FUSE device methods.

Most of this is just style cleanup. There some comments that are not
really necessary (describing what, not why), control flow that
is more complex than is necessary, and redefinitions of common constant
values.

In the case of writeLocked, using DropFirst() is actually broken when reading
from a pipe, so that change is functional.

PiperOrigin-RevId: 504608319

pkg/abi/linux/fuse.go

@@ -113,6 +113,9 @@ type FUSEHeaderIn struct {
 	_ uint32
 }
 
+// SizeOfFUSEHeaderIn is the size of the FUSEHeaderIn struct.
+var SizeOfFUSEHeaderIn = uint32((*FUSEHeaderIn)(nil).SizeBytes())
+
 // FUSEHeaderOut is the header written by the daemon when it processes
 // a request and wants to send a reply (almost all operations require a
 // reply; if they do not, this will be explicitly documented).
@@ -129,6 +132,9 @@ type FUSEHeaderOut struct {
 	Unique FUSEOpID
 }
 
+// SizeOfFUSEHeaderOut is the size of the FUSEHeaderOut struct.
+var SizeOfFUSEHeaderOut = uint32((*FUSEHeaderOut)(nil).SizeBytes())
+
 // FUSE_INIT flags, consistent with the ones in include/uapi/linux/fuse.h.
 // Our taget version is 7.23 but we have few implemented in advance.
 const (
@@ -442,7 +448,7 @@ const MAX_NON_LFS = ((1 << 31) - 1)
 const (
 	// FOPEN_DIRECT_IO indicates bypassing page cache for this opened file.
 	FOPEN_DIRECT_IO = 1 << 0
-	// FOPEN_KEEP_CACHE avoids invalidate of data cache on open.
+	// FOPEN_KEEP_CACHE avoids invalidating the data cache on open.
 	FOPEN_KEEP_CACHE = 1 << 1
 	// FOPEN_NONSEEKABLE indicates the file cannot be seeked.
 	FOPEN_NONSEEKABLE = 1 << 2
@@ -464,10 +470,10 @@ type FUSEOpenIn struct {
 //
 // +marshal
 type FUSEOpenOut struct {
-	// Fh is the file handler for opened file.
+	// Fh is the file handler for opened files.
 	Fh uint64
 
-	// OpenFlag for the opened file.
+	// OpenFlag for the opened files.
 	OpenFlag uint32
 
 	_ uint32
@@ -545,6 +551,9 @@ type FUSEWriteIn struct {
 	_ uint32
 }
 
+// SizeOfFUSEWriteIn is the size of the FUSEWriteIn struct.
+var SizeOfFUSEWriteIn = uint32((*FUSEWriteIn)(nil).SizeBytes())
+
 // FUSEWritePayloadIn combines header - FUSEWriteIn and payload
 // in a single marshallable struct when sending request by the
 // kernel to the daemon
@@ -651,7 +660,7 @@ func (r *FUSERenameIn) SizeBytes() int {
 //
 // +marshal dynamic
 type FUSECreateIn struct {
-	// CreateMeta contains mode, rdev and umash field for FUSE_MKNODS.
+	// CreateMeta contains mode, rdev and umash fields for FUSE_MKNODS.
 	CreateMeta FUSECreateMeta
 
 	// Name is the name of the node to create.
@@ -696,7 +705,7 @@ type FUSEMknodMeta struct {
 //
 // +marshal dynamic
 type FUSEMknodIn struct {
-	// MknodMeta contains mode, rdev and umash field for FUSE_MKNODS.
+	// MknodMeta contains mode, rdev and umash fields for FUSE_MKNODS.
 	MknodMeta FUSEMknodMeta
 	// Name is the name of the node to create.
 	Name CString

pkg/sentry/fsimpl/fuse/connection.go

@@ -210,11 +210,8 @@ func newFUSEConnection(_ context.Context, fuseFD *DeviceFD, opts *filesystemOpti
 	// mount another filesystem.
 
 	// Create the writeBuf for the header to be stored in.
-	hdrLen := uint32((*linux.FUSEHeaderOut)(nil).SizeBytes())
-	fuseFD.writeBuf = make([]byte, hdrLen)
 	fuseFD.completions = make(map[linux.FUSEOpID]*futureResponse)
 	fuseFD.fullQueueCh = make(chan struct{}, opts.maxActiveRequests)
-	fuseFD.writeCursor = 0
 
 	return &connection{
 		fd:                       fuseFD,
@@ -255,7 +252,7 @@ func (conn *connection) CallAsync(t *kernel.Task, r *Request) error {
 // The forget request does not have a reply,
 // as documented in include/uapi/linux/fuse.h:FUSE_FORGET.
 func (conn *connection) Call(t *kernel.Task, r *Request) (*Response, error) {
-	// Block requests sent before connection is initalized.
+	// Block requests sent before connection is initialized.
 	if !conn.Initialized() && r.hdr.Opcode != linux.FUSE_INIT {
 		if err := t.Block(conn.initializedChan); err != nil {
 			return nil, err

pkg/sentry/fsimpl/fuse/dev.go

@@ -29,6 +29,9 @@ import (
 
 const fuseDevMinor = 229
 
+// This is equivalent to linux.SizeOfFUSEHeaderIn
+const fuseHeaderOutSize = 16
+
 // fuseDevice implements vfs.Device for /dev/fuse.
 //
 // +stateify savable
@@ -84,17 +87,10 @@ type DeviceFD struct {
 	// +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
+	writeBuf [fuseHeaderOutSize]byte
 
 	// conn is the FUSE connection that this FD is being used for.
 	// +checklocks:mu
@@ -144,9 +140,6 @@ 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 there is an active connection we know there is at least one
-	// filesystem mounted.
 	fd.mu.Lock()
 	defer fd.mu.Unlock()
 	if !fd.connected() {
@@ -158,11 +151,8 @@ func (fd *DeviceFD) Read(ctx context.Context, dst usermem.IOSequence, opts vfs.R
 	// header (Linux uses the request header and the FUSEWriteIn header for this
 	// calculation) + the negotiated MaxWrite room for the data.
 	minBuffSize := linux.FUSE_MIN_READ_BUFFER
-	inHdrLen := uint32((*linux.FUSEHeaderIn)(nil).SizeBytes())
-	writeHdrLen := uint32((*linux.FUSEWriteIn)(nil).SizeBytes())
-
 	fd.conn.mu.Lock()
-	negotiatedMinBuffSize := inHdrLen + writeHdrLen + fd.conn.maxWrite
+	negotiatedMinBuffSize := linux.SizeOfFUSEHeaderIn + linux.SizeOfFUSEHeaderOut + fd.conn.maxWrite
 	fd.conn.mu.Unlock()
 	if minBuffSize < negotiatedMinBuffSize {
 		minBuffSize = negotiatedMinBuffSize
@@ -180,44 +170,32 @@ func (fd *DeviceFD) Read(ctx context.Context, dst usermem.IOSequence, opts vfs.R
 // Preconditions: dst is large enough for any reasonable request.
 // +checklocks:fd.mu
 func (fd *DeviceFD) readLocked(ctx context.Context, dst usermem.IOSequence, opts vfs.ReadOptions) (int64, error) {
+	// Find the first valid request. For the normal case this loop only executes
+	// once.
 	var req *Request
-
-	// Find the first valid request.
-	// For the normal case this loop only execute once.
-	for !fd.queue.Empty() {
-		req = fd.queue.Front()
-
+	for req = fd.queue.Front(); !fd.queue.Empty(); req = fd.queue.Front() {
 		if int64(req.hdr.Len) <= dst.NumBytes() {
 			break
 		}
-
-		// The request is too large. Cannot process it. All requests must be smaller than the
-		// negotiated size as specified by Connection.MaxWrite set as part of the FUSE_INIT
-		// handshake.
+		// The request is too large so we cannot process it. All requests must be
+		// smaller than the negotiated size as specified by Connection.MaxWrite set
+		// as part of the FUSE_INIT handshake.
 		errno := -int32(unix.EIO)
 		if req.hdr.Opcode == linux.FUSE_SETXATTR {
 			errno = -int32(unix.E2BIG)
 		}
 
-		// Return the error to the calling task.
 		if err := fd.sendError(ctx, errno, req.hdr.Unique); err != nil {
 			return 0, err
 		}
-
-		// We're done with this request.
 		fd.queue.Remove(req)
 		req = nil
 	}
-
 	if req == nil {
 		return 0, linuxerr.ErrWouldBlock
 	}
 
 	// We already checked the size: dst must be able to fit the whole request.
-	// Now we write the marshalled header, the payload,
-	// and the potential additional payload
-	// to the user memory IOSequence.
-
 	n, err := dst.CopyOut(ctx, req.data)
 	if err != nil {
 		return 0, err
@@ -225,13 +203,10 @@ func (fd *DeviceFD) readLocked(ctx context.Context, dst usermem.IOSequence, opts
 	if n != len(req.data) {
 		return 0, linuxerr.EIO
 	}
-
-	// Fully done with this req, remove it from the queue.
 	fd.queue.Remove(req)
-
-	// Remove noReply ones from map of requests expecting a reply.
+	// Remove noReply ones from the map of requests expecting a reply.
 	if req.noReply {
-		fd.numActiveRequests -= 1
+		fd.numActiveRequests--
 		delete(fd.completions, req.hdr.Unique)
 	}
 
@@ -262,102 +237,36 @@ 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 there is an active connection we know there is at least one
-	// filesystem mounted.
 	if !fd.connected() {
 		return 0, linuxerr.EPERM
 	}
 
-	var cn, n int64
-	hdrLen := uint32((*linux.FUSEHeaderOut)(nil).SizeBytes())
-
-	for src.NumBytes() > 0 {
-		if fd.writeCursorFR != nil {
-			// Already have common header, and we're now copying the payload.
-			wantBytes := fd.writeCursorFR.hdr.Len
-
-			// Note that the FR data doesn't have the header. Copy it over if its necessary.
-			if fd.writeCursorFR.data == nil {
-				fd.writeCursorFR.data = make([]byte, wantBytes)
-			}
-
-			bytesCopied, err := src.CopyIn(ctx, fd.writeCursorFR.data[fd.writeCursor:wantBytes])
-			if err != nil {
-				return 0, err
-			}
-			src = src.DropFirst(bytesCopied)
-
-			cn = int64(bytesCopied)
-			n += cn
-			fd.writeCursor += uint32(cn)
-			if fd.writeCursor == wantBytes {
-				// Done reading this full response. Clean up and unblock the
-				// initiator.
-				break
-			}
-
-			// Check if we have more data in src.
-			continue
-		}
-
-		// Assert that the header isn't read into the writeBuf yet.
-		if fd.writeCursor >= hdrLen {
-			return 0, linuxerr.EINVAL
-		}
-
-		// We don't have the full common response header yet.
-		wantBytes := hdrLen - fd.writeCursor
-		bytesCopied, err := src.CopyIn(ctx, fd.writeBuf[fd.writeCursor:wantBytes])
-		if err != nil {
-			return 0, err
-		}
-		src = src.DropFirst(bytesCopied)
-
-		cn = int64(bytesCopied)
-		n += cn
-		fd.writeCursor += uint32(cn)
-		if fd.writeCursor == hdrLen {
-			// Have full header in the writeBuf. Use it to fetch the actual futureResponse
-			// from the device's completions map.
-			var hdr linux.FUSEHeaderOut
-			hdr.UnmarshalBytes(fd.writeBuf)
-
-			// We have the header now and so the writeBuf has served its purpose.
-			// We could reset it manually here but instead of doing that, at the
-			// end of the write, the writeCursor will be set to 0 thereby allowing
-			// the next request to overwrite whats in the buffer,
-
-			fut, ok := fd.completions[hdr.Unique]
-			if !ok {
-				// Server sent us a response for a request we never sent,
-				// or for which we already received a reply (e.g. aborted), an unlikely event.
-				return 0, linuxerr.EINVAL
-			}
-
-			delete(fd.completions, hdr.Unique)
-
-			// Copy over the header into the future response. The rest of the payload
-			// will be copied over to the FR's data in the next iteration.
-			fut.hdr = &hdr
-			fd.writeCursorFR = fut
-
-			// Next iteration will now try read the complete request, if src has
-			// any data remaining. Otherwise we're done.
-		}
+	if _, err := src.CopyIn(ctx, fd.writeBuf[:]); err != nil {
+		return 0, err
 	}
+	var hdr linux.FUSEHeaderOut
+	hdr.UnmarshalBytes(fd.writeBuf[:])
 
-	if fd.writeCursorFR != nil {
-		if err := fd.sendResponse(ctx, fd.writeCursorFR); err != nil {
-			return 0, err
-		}
-
-		// Ready the device for the next request.
-		fd.writeCursorFR = nil
-		fd.writeCursor = 0
+	fut, ok := fd.completions[hdr.Unique]
+	if !ok {
+		// Server sent us a response for a request we never sent, or for which we
+		// already received a reply (e.g. aborted), an unlikely event.
+		return 0, linuxerr.EINVAL
 	}
+	delete(fd.completions, hdr.Unique)
 
-	return n, nil
+	// Copy over the header into the future response. The rest of the payload
+	// will be copied over to the FR's data in the next iteration.
+	fut.hdr = &hdr
+	fut.data = make([]byte, fut.hdr.Len)
+	n, err := src.CopyIn(ctx, fut.data)
+	if err != nil {
+		return 0, err
+	}
+	if err := fd.sendResponse(ctx, fut); err != nil {
+		return 0, err
+	}
+	return int64(n), nil
 }
 
 // Readiness implements vfs.FileDescriptionImpl.Readiness.
@@ -448,9 +357,8 @@ func (fd *DeviceFD) sendResponse(ctx context.Context, fut *futureResponse) error
 // +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())
 	respHdr := linux.FUSEHeaderOut{
-		Len:    outHdrLen,
+		Len:    linux.SizeOfFUSEHeaderOut,
 		Error:  errno,
 		Unique: unique,
 	}

pkg/sentry/fsimpl/fuse/dev_test.go

@@ -295,11 +295,10 @@ func fuseServerRun(t *testing.T, s *testutil.System, k *kernel.Kernel, fd *vfs.F
 
 	// Read the request.
 	for {
-		inHdrLen := uint32((*linux.FUSEHeaderIn)(nil).SizeBytes())
 		payloadLen := uint32(readPayload.SizeBytes())
 
-		// The raed buffer must meet some certain size criteria.
-		buffSize := inHdrLen + payloadLen
+		// The read buffer must meet some certain size criteria.
+		buffSize := linux.SizeOfFUSEHeaderIn + payloadLen
 		if buffSize < linux.FUSE_MIN_READ_BUFFER {
 			buffSize = linux.FUSE_MIN_READ_BUFFER
 		}
@@ -311,7 +310,7 @@ func fuseServerRun(t *testing.T, s *testutil.System, k *kernel.Kernel, fd *vfs.F
 			t.Fatalf("Read failed :%v", err)
 		}
 
-		// Server should shut down. No new requests are going to be made.
+		// The server should shut down. No new requests are going to be made.
 		if serverKilled {
 			break
 		}
@@ -329,17 +328,16 @@ func fuseServerRun(t *testing.T, s *testutil.System, k *kernel.Kernel, fd *vfs.F
 		}
 
 		// Write the response.
-		outHdrLen := uint32((*linux.FUSEHeaderOut)(nil).SizeBytes())
-		outBuf := make([]byte, outHdrLen+payloadLen)
+		outBuf := make([]byte, linux.SizeOfFUSEHeaderOut+payloadLen)
 		outHeader := linux.FUSEHeaderOut{
-			Len:    outHdrLen + payloadLen,
+			Len:    linux.SizeOfFUSEHeaderOut + payloadLen,
 			Error:  0,
 			Unique: readFUSEHeaderIn.Unique,
 		}
 
 		// Echo the payload back.
-		outHeader.MarshalUnsafe(outBuf[:outHdrLen])
-		readPayload.MarshalUnsafe(outBuf[outHdrLen:])
+		outHeader.MarshalUnsafe(outBuf[:linux.SizeOfFUSEHeaderOut])
+		readPayload.MarshalUnsafe(outBuf[linux.SizeOfFUSEHeaderOut:])
 		outIOseq := usermem.BytesIOSequence(outBuf)
 
 		_, err = fd.Write(s.Ctx, outIOseq, vfs.WriteOptions{})