From ce4f4283badb6b07baf9f8e6d99e7a5fd15c92db Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Fri, 5 Nov 2021 10:40:53 -0700 Subject: Make {Un}Marshal{Bytes/Unsafe} return remaining buffer. Change marshal.Marshallable method signatures to return the remaining buffer. This makes it easier to implement these method manually. Without this, we would have to manually do buffer shifting which is error prone. tools/go_marshal/test:benchmark test does not show change in performance. Additionally fixed some marshalling bugs in fsimpl/fuse. Updated multiple callpoints to get rid of redundant slice indexing work and simplified code using this new signature. Updates #6450 PiperOrigin-RevId: 407857019 --- pkg/sentry/fsimpl/fuse/BUILD | 3 +- pkg/sentry/fsimpl/fuse/connection_test.go | 11 ++---- pkg/sentry/fsimpl/fuse/dev_test.go | 19 +++++----- pkg/sentry/fsimpl/fuse/fusefs.go | 16 ++++---- pkg/sentry/fsimpl/fuse/request_response.go | 4 +- pkg/sentry/fsimpl/fuse/utils_test.go | 59 ------------------------------ 6 files changed, 24 insertions(+), 88 deletions(-) (limited to 'pkg/sentry/fsimpl/fuse') diff --git a/pkg/sentry/fsimpl/fuse/BUILD b/pkg/sentry/fsimpl/fuse/BUILD index 05c4fbeb2..18497a880 100644 --- a/pkg/sentry/fsimpl/fuse/BUILD +++ b/pkg/sentry/fsimpl/fuse/BUILD @@ -77,8 +77,7 @@ go_test( deps = [ "//pkg/abi/linux", "//pkg/errors/linuxerr", - "//pkg/hostarch", - "//pkg/marshal", + "//pkg/marshal/primitive", "//pkg/sentry/fsimpl/testutil", "//pkg/sentry/kernel", "//pkg/sentry/kernel/auth", diff --git a/pkg/sentry/fsimpl/fuse/connection_test.go b/pkg/sentry/fsimpl/fuse/connection_test.go index 1fddd858e..d98d2832b 100644 --- a/pkg/sentry/fsimpl/fuse/connection_test.go +++ b/pkg/sentry/fsimpl/fuse/connection_test.go @@ -20,6 +20,7 @@ import ( "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/errors/linuxerr" + "gvisor.dev/gvisor/pkg/marshal/primitive" "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" ) @@ -69,14 +70,10 @@ func TestConnectionAbort(t *testing.T) { t.Fatalf("newTestConnection: %v", err) } - testObj := &testPayload{ - data: rand.Uint32(), - } - var futNormal []*futureResponse - + testObj := primitive.Uint32(rand.Uint32()) for i := 0; i < int(numRequests); i++ { - req := conn.NewRequest(creds, uint32(i), uint64(i), 0, testObj) + req := conn.NewRequest(creds, uint32(i), uint64(i), 0, &testObj) fut, err := conn.callFutureLocked(task, req) if err != nil { t.Fatalf("callFutureLocked failed: %v", err) @@ -102,7 +99,7 @@ func TestConnectionAbort(t *testing.T) { } // After abort, Call() should return directly with ENOTCONN. - req := conn.NewRequest(creds, 0, 0, 0, testObj) + req := conn.NewRequest(creds, 0, 0, 0, &testObj) _, err = conn.Call(task, req) if !linuxerr.Equals(linuxerr.ENOTCONN, err) { t.Fatalf("Incorrect error code received for Call() after connection aborted") diff --git a/pkg/sentry/fsimpl/fuse/dev_test.go b/pkg/sentry/fsimpl/fuse/dev_test.go index 8951b5ba8..13b32fc7c 100644 --- a/pkg/sentry/fsimpl/fuse/dev_test.go +++ b/pkg/sentry/fsimpl/fuse/dev_test.go @@ -21,6 +21,7 @@ import ( "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/errors/linuxerr" + "gvisor.dev/gvisor/pkg/marshal/primitive" "gvisor.dev/gvisor/pkg/sentry/fsimpl/testutil" "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" @@ -215,11 +216,9 @@ func fuseClientRun(t *testing.T, s *testutil.System, k *kernel.Kernel, conn *con if err != nil { t.Fatal(err) } - testObj := &testPayload{ - data: rand.Uint32(), - } - req := conn.NewRequest(creds, pid, inode, echoTestOpcode, testObj) + testObj := primitive.Uint32(rand.Uint32()) + req := conn.NewRequest(creds, pid, inode, echoTestOpcode, &testObj) // Queue up a request. // Analogous to Call except it doesn't block on the task. @@ -232,7 +231,7 @@ func fuseClientRun(t *testing.T, s *testutil.System, k *kernel.Kernel, conn *con t.Fatalf("Server responded with an error: %v", err) } - var respTestPayload testPayload + var respTestPayload primitive.Uint32 if err := resp.UnmarshalPayload(&respTestPayload); err != nil { t.Fatalf("Unmarshalling payload error: %v", err) } @@ -242,8 +241,8 @@ func fuseClientRun(t *testing.T, s *testutil.System, k *kernel.Kernel, conn *con req.hdr.Unique, resp.hdr.Unique) } - if respTestPayload.data != testObj.data { - t.Fatalf("read incorrect data. Data expected: %v, but got %v", testObj.data, respTestPayload.data) + if respTestPayload != testObj { + t.Fatalf("read incorrect data. Data expected: %d, but got %d", testObj, respTestPayload) } } @@ -256,8 +255,8 @@ func fuseServerRun(t *testing.T, s *testutil.System, k *kernel.Kernel, fd *vfs.F // Create the tasks that the server will be using. tc := k.NewThreadGroup(nil, k.RootPIDNamespace(), kernel.NewSignalHandlers(), linux.SIGCHLD, k.GlobalInit().Limits()) - var readPayload testPayload + var readPayload primitive.Uint32 serverTask, err := testutil.CreateTask(s.Ctx, "fuse-server", tc, s.MntNs, s.Root, s.Root) if err != nil { t.Fatal(err) @@ -291,8 +290,8 @@ func fuseServerRun(t *testing.T, s *testutil.System, k *kernel.Kernel, fd *vfs.F } var readFUSEHeaderIn linux.FUSEHeaderIn - readFUSEHeaderIn.UnmarshalUnsafe(inBuf[:inHdrLen]) - readPayload.UnmarshalUnsafe(inBuf[inHdrLen : inHdrLen+payloadLen]) + inBuf = readFUSEHeaderIn.UnmarshalUnsafe(inBuf) + readPayload.UnmarshalUnsafe(inBuf) if readFUSEHeaderIn.Opcode != echoTestOpcode { t.Fatalf("read incorrect data. Header: %v, Payload: %v", readFUSEHeaderIn, readPayload) diff --git a/pkg/sentry/fsimpl/fuse/fusefs.go b/pkg/sentry/fsimpl/fuse/fusefs.go index af16098d2..00b520c31 100644 --- a/pkg/sentry/fsimpl/fuse/fusefs.go +++ b/pkg/sentry/fsimpl/fuse/fusefs.go @@ -489,7 +489,7 @@ func (i *inode) Open(ctx context.Context, rp *vfs.ResolvingPath, d *kernfs.Dentr // Lookup implements kernfs.Inode.Lookup. func (i *inode) Lookup(ctx context.Context, name string) (kernfs.Inode, error) { - in := linux.FUSELookupIn{Name: name} + in := linux.FUSELookupIn{Name: linux.CString(name)} return i.newEntry(ctx, name, 0, linux.FUSE_LOOKUP, &in) } @@ -520,7 +520,7 @@ func (i *inode) NewFile(ctx context.Context, name string, opts vfs.OpenOptions) Mode: uint32(opts.Mode) | linux.S_IFREG, Umask: uint32(kernelTask.FSContext().Umask()), }, - Name: name, + Name: linux.CString(name), } return i.newEntry(ctx, name, linux.S_IFREG, linux.FUSE_CREATE, &in) } @@ -533,7 +533,7 @@ func (i *inode) NewNode(ctx context.Context, name string, opts vfs.MknodOptions) Rdev: linux.MakeDeviceID(uint16(opts.DevMajor), opts.DevMinor), Umask: uint32(kernel.TaskFromContext(ctx).FSContext().Umask()), }, - Name: name, + Name: linux.CString(name), } return i.newEntry(ctx, name, opts.Mode.FileType(), linux.FUSE_MKNOD, &in) } @@ -541,8 +541,8 @@ func (i *inode) NewNode(ctx context.Context, name string, opts vfs.MknodOptions) // NewSymlink implements kernfs.Inode.NewSymlink. func (i *inode) NewSymlink(ctx context.Context, name, target string) (kernfs.Inode, error) { in := linux.FUSESymLinkIn{ - Name: name, - Target: target, + Name: linux.CString(name), + Target: linux.CString(target), } return i.newEntry(ctx, name, linux.S_IFLNK, linux.FUSE_SYMLINK, &in) } @@ -554,7 +554,7 @@ func (i *inode) Unlink(ctx context.Context, name string, child kernfs.Inode) err log.Warningf("fusefs.Inode.newEntry: couldn't get kernel task from context", i.nodeID) return linuxerr.EINVAL } - in := linux.FUSEUnlinkIn{Name: name} + in := linux.FUSEUnlinkIn{Name: linux.CString(name)} req := i.fs.conn.NewRequest(auth.CredentialsFromContext(ctx), uint32(kernelTask.ThreadID()), i.nodeID, linux.FUSE_UNLINK, &in) res, err := i.fs.conn.Call(kernelTask, req) if err != nil { @@ -571,7 +571,7 @@ func (i *inode) NewDir(ctx context.Context, name string, opts vfs.MkdirOptions) Mode: uint32(opts.Mode), Umask: uint32(kernel.TaskFromContext(ctx).FSContext().Umask()), }, - Name: name, + Name: linux.CString(name), } return i.newEntry(ctx, name, linux.S_IFDIR, linux.FUSE_MKDIR, &in) } @@ -581,7 +581,7 @@ func (i *inode) RmDir(ctx context.Context, name string, child kernfs.Inode) erro fusefs := i.fs task, creds := kernel.TaskFromContext(ctx), auth.CredentialsFromContext(ctx) - in := linux.FUSERmDirIn{Name: name} + in := linux.FUSERmDirIn{Name: linux.CString(name)} req := fusefs.conn.NewRequest(creds, uint32(task.ThreadID()), i.nodeID, linux.FUSE_RMDIR, &in) res, err := i.fs.conn.Call(task, req) if err != nil { diff --git a/pkg/sentry/fsimpl/fuse/request_response.go b/pkg/sentry/fsimpl/fuse/request_response.go index 8a72489fa..ec76ec2a4 100644 --- a/pkg/sentry/fsimpl/fuse/request_response.go +++ b/pkg/sentry/fsimpl/fuse/request_response.go @@ -41,7 +41,7 @@ type fuseInitRes struct { } // UnmarshalBytes deserializes src to the initOut attribute in a fuseInitRes. -func (r *fuseInitRes) UnmarshalBytes(src []byte) { +func (r *fuseInitRes) UnmarshalBytes(src []byte) []byte { out := &r.initOut // Introduced before FUSE kernel version 7.13. @@ -70,7 +70,7 @@ func (r *fuseInitRes) UnmarshalBytes(src []byte) { out.MaxPages = uint16(hostarch.ByteOrder.Uint16(src[:2])) src = src[2:] } - _ = src // Remove unused warning. + return src } // SizeBytes is the size of the payload of the FUSE_INIT response. diff --git a/pkg/sentry/fsimpl/fuse/utils_test.go b/pkg/sentry/fsimpl/fuse/utils_test.go index b0bab0066..8d4a2fad3 100644 --- a/pkg/sentry/fsimpl/fuse/utils_test.go +++ b/pkg/sentry/fsimpl/fuse/utils_test.go @@ -15,17 +15,13 @@ package fuse import ( - "io" "testing" "gvisor.dev/gvisor/pkg/abi/linux" - "gvisor.dev/gvisor/pkg/marshal" "gvisor.dev/gvisor/pkg/sentry/fsimpl/testutil" "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/vfs" - - "gvisor.dev/gvisor/pkg/hostarch" ) func setup(t *testing.T) *testutil.System { @@ -70,58 +66,3 @@ func newTestConnection(system *testutil.System, k *kernel.Kernel, maxActiveReque } return fs.conn, &fuseDev.vfsfd, nil } - -type testPayload struct { - marshal.StubMarshallable - data uint32 -} - -// SizeBytes implements marshal.Marshallable.SizeBytes. -func (t *testPayload) SizeBytes() int { - return 4 -} - -// MarshalBytes implements marshal.Marshallable.MarshalBytes. -func (t *testPayload) MarshalBytes(dst []byte) { - hostarch.ByteOrder.PutUint32(dst[:4], t.data) -} - -// UnmarshalBytes implements marshal.Marshallable.UnmarshalBytes. -func (t *testPayload) UnmarshalBytes(src []byte) { - *t = testPayload{data: hostarch.ByteOrder.Uint32(src[:4])} -} - -// Packed implements marshal.Marshallable.Packed. -func (t *testPayload) Packed() bool { - return true -} - -// MarshalUnsafe implements marshal.Marshallable.MarshalUnsafe. -func (t *testPayload) MarshalUnsafe(dst []byte) { - t.MarshalBytes(dst) -} - -// UnmarshalUnsafe implements marshal.Marshallable.UnmarshalUnsafe. -func (t *testPayload) UnmarshalUnsafe(src []byte) { - t.UnmarshalBytes(src) -} - -// CopyOutN implements marshal.Marshallable.CopyOutN. -func (t *testPayload) CopyOutN(task marshal.CopyContext, addr hostarch.Addr, limit int) (int, error) { - panic("not implemented") -} - -// CopyOut implements marshal.Marshallable.CopyOut. -func (t *testPayload) CopyOut(task marshal.CopyContext, addr hostarch.Addr) (int, error) { - panic("not implemented") -} - -// CopyIn implements marshal.Marshallable.CopyIn. -func (t *testPayload) CopyIn(task marshal.CopyContext, addr hostarch.Addr) (int, error) { - panic("not implemented") -} - -// WriteTo implements io.WriterTo.WriteTo. -func (t *testPayload) WriteTo(w io.Writer) (int64, error) { - panic("not implemented") -} -- cgit v1.2.3