From ab4f6830bc4d65be886ac0c5b4d2fe2cc52fc052 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Mon, 13 Jun 2022 10:21:34 -0700 Subject: [PATCH 01/29] Add tool to save remote trace sessions This tool allows testing of trace servers without the need to setup runsc and workloads during testing. More details in the readme file. Updates #4805 PiperOrigin-RevId: 454641010 --- .../seccheck/checkers/remote/remote_test.go | 6 +- .../seccheck/checkers/remote/server/BUILD | 19 ++ .../seccheck/checkers/remote/server/server.go | 245 ++++++++++++++++++ .../seccheck/checkers/remote/test/BUILD | 6 +- .../seccheck/checkers/remote/test/server.go | 211 +++------------ runsc/container/trace_test.go | 8 +- test/trace/trace_test.go | 2 +- tools/tracereplay/BUILD | 36 +++ tools/tracereplay/README.md | 77 ++++++ tools/tracereplay/main/BUILD | 15 ++ tools/tracereplay/main/main.go | 152 +++++++++++ tools/tracereplay/replay.go | 133 ++++++++++ tools/tracereplay/save.go | 111 ++++++++ tools/tracereplay/testdata/client-0001 | Bin 0 -> 804 bytes tools/tracereplay/tracereplay.go | 83 ++++++ tools/tracereplay/tracereplay_test.go | 76 ++++++ 16 files changed, 997 insertions(+), 183 deletions(-) create mode 100644 pkg/sentry/seccheck/checkers/remote/server/BUILD create mode 100644 pkg/sentry/seccheck/checkers/remote/server/server.go create mode 100644 tools/tracereplay/BUILD create mode 100644 tools/tracereplay/README.md create mode 100644 tools/tracereplay/main/BUILD create mode 100644 tools/tracereplay/main/main.go create mode 100644 tools/tracereplay/replay.go create mode 100644 tools/tracereplay/save.go create mode 100644 tools/tracereplay/testdata/client-0001 create mode 100644 tools/tracereplay/tracereplay.go create mode 100644 tools/tracereplay/tracereplay_test.go diff --git a/pkg/sentry/seccheck/checkers/remote/remote_test.go b/pkg/sentry/seccheck/checkers/remote/remote_test.go index 61ae35605..57aa2b80f 100644 --- a/pkg/sentry/seccheck/checkers/remote/remote_test.go +++ b/pkg/sentry/seccheck/checkers/remote/remote_test.go @@ -115,7 +115,7 @@ func TestBasic(t *testing.T) { } defer server.Close() - endpoint, err := setup(server.Path) + endpoint, err := setup(server.Endpoint) if err != nil { t.Fatalf("setup(): %v", err) } @@ -163,7 +163,7 @@ func TestVersionUnsupported(t *testing.T) { server.SetVersion(0) - _, err = setup(server.Path) + _, err = setup(server.Endpoint) if err == nil || !strings.Contains(err.Error(), "remote version") { t.Fatalf("Wrong error: %v", err) } @@ -178,7 +178,7 @@ func TestVersionNewer(t *testing.T) { server.SetVersion(wire.CurrentVersion + 10) - endpoint, err := setup(server.Path) + endpoint, err := setup(server.Endpoint) if err != nil { t.Fatalf("setup(): %v", err) } diff --git a/pkg/sentry/seccheck/checkers/remote/server/BUILD b/pkg/sentry/seccheck/checkers/remote/server/BUILD new file mode 100644 index 000000000..cefced63d --- /dev/null +++ b/pkg/sentry/seccheck/checkers/remote/server/BUILD @@ -0,0 +1,19 @@ +load("//tools:defs.bzl", "go_library") + +package(licenses = ["notice"]) + +go_library( + name = "server", + srcs = ["server.go"], + visibility = ["//:sandbox"], + deps = [ + "//pkg/cleanup", + "//pkg/log", + "//pkg/sentry/seccheck/checkers/remote/wire", + "//pkg/sentry/seccheck/points:points_go_proto", + "//pkg/sync", + "//pkg/unet", + "@org_golang_google_protobuf//proto:go_default_library", + "@org_golang_x_sys//unix:go_default_library", + ], +) diff --git a/pkg/sentry/seccheck/checkers/remote/server/server.go b/pkg/sentry/seccheck/checkers/remote/server/server.go new file mode 100644 index 000000000..cbfbd59e1 --- /dev/null +++ b/pkg/sentry/seccheck/checkers/remote/server/server.go @@ -0,0 +1,245 @@ +// 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 server provides a common server implementation that can connect with +// remote.Remote. +package server + +import ( + "errors" + "fmt" + "io" + "os" + + "golang.org/x/sys/unix" + "google.golang.org/protobuf/proto" + "gvisor.dev/gvisor/pkg/cleanup" + "gvisor.dev/gvisor/pkg/log" + "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire" + pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" + "gvisor.dev/gvisor/pkg/sync" + "gvisor.dev/gvisor/pkg/unet" +) + +// ClientHandler is used to interface with client that connect to the server. +type ClientHandler interface { + // NewClient is called when a new client connects to the server. It returns + // a handler that will be bound to the client. + NewClient() (MessageHandler, error) +} + +// MessageHandler is used to process messages from a client. +type MessageHandler interface { + // Message processes a single message. raw contains the entire unparsed + // message. hdr is the parser message header and payload is the unparsed + // message data. + Message(raw []byte, hdr wire.Header, payload []byte) error + + // Version returns what wire version of the protocol is supported. + Version() uint32 + + // Close closes the handler. + Close() +} + +type client struct { + socket *unet.Socket + handler MessageHandler +} + +func (c client) close() { + _ = c.socket.Close() + c.handler.Close() +} + +// CommonServer provides common functionality to connect and process messages +// from different clients. Implementors decide how clients and messages are +// handled, e.g. counting messages for testing. +type CommonServer struct { + // Endpoint is the path to the socket that the server listens to. + Endpoint string + + socket *unet.ServerSocket + + handler ClientHandler + + cond sync.Cond + + // +checklocks:cond.L + clients []client +} + +// Init initializes the server. It must be called before it is used. +func (s *CommonServer) Init(path string, handler ClientHandler) { + s.Endpoint = path + s.handler = handler + s.cond = sync.Cond{L: &sync.Mutex{}} +} + +// Start creates the socket file and listens for new connections. +func (s *CommonServer) Start() error { + socket, err := unix.Socket(unix.AF_UNIX, unix.SOCK_SEQPACKET, 0) + if err != nil { + return fmt.Errorf("socket(AF_UNIX, SOCK_SEQPACKET, 0): %w", err) + } + cu := cleanup.Make(func() { + _ = unix.Close(socket) + }) + defer cu.Clean() + + sa := &unix.SockaddrUnix{Name: s.Endpoint} + if err := unix.Bind(socket, sa); err != nil { + return fmt.Errorf("bind(%q): %w", s.Endpoint, err) + } + + s.socket, err = unet.NewServerSocket(socket) + if err != nil { + return err + } + cu.Add(func() { s.socket.Close() }) + + if err := s.socket.Listen(); err != nil { + return err + } + + go s.run() + cu.Release() + return nil +} + +func (s *CommonServer) run() { + for { + socket, err := s.socket.Accept() + if err != nil { + // EBADF returns when the socket closes. + if !errors.Is(err, unix.EBADF) { + log.Warningf("socket.Accept(): %v", err) + } + return + } + msgHandler, err := s.handler.NewClient() + if err != nil { + log.Warningf("handler.NewClient: %v", err) + return + } + client := client{ + socket: socket, + handler: msgHandler, + } + s.cond.L.Lock() + s.clients = append(s.clients, client) + s.cond.Broadcast() + s.cond.L.Unlock() + + if err := s.handshake(client); err != nil { + log.Warningf(err.Error()) + s.closeClient(client) + continue + } + go s.handleClient(client) + } +} + +// handshake performs version exchange with client. See common.proto for details +// about the protocol. +func (s *CommonServer) handshake(client client) error { + var in [1024]byte + read, err := client.socket.Read(in[:]) + if err != nil { + return fmt.Errorf("reading handshake message: %w", err) + } + hsIn := pb.Handshake{} + if err := proto.Unmarshal(in[:read], &hsIn); err != nil { + return fmt.Errorf("unmarshalling handshake message: %w", err) + } + if hsIn.Version != wire.CurrentVersion { + return fmt.Errorf("wrong version number, want: %d, got, %d", wire.CurrentVersion, hsIn.Version) + } + + hsOut := pb.Handshake{Version: client.handler.Version()} + out, err := proto.Marshal(&hsOut) + if err != nil { + return fmt.Errorf("marshalling handshake message: %w", err) + } + if _, err := client.socket.Write(out); err != nil { + return fmt.Errorf("sending handshake message: %w", err) + } + return nil +} + +func (s *CommonServer) handleClient(client client) { + defer s.closeClient(client) + + var buf = make([]byte, 1024*1024) + for { + read, err := client.socket.Read(buf) + if err != nil { + if errors.Is(err, io.EOF) || errors.Is(err, unix.EBADF) { + // Both errors indicate that the socket has been closed. + return + } + panic(err) + } + if read < wire.HeaderStructSize { + panic("message too small") + } + hdr := wire.Header{} + hdr.UnmarshalUnsafe(buf[0:wire.HeaderStructSize]) + if read < int(hdr.HeaderSize) { + panic(fmt.Sprintf("message truncated, header size: %d, read: %d", hdr.HeaderSize, read)) + } + if err := client.handler.Message(buf[:read], hdr, buf[hdr.HeaderSize:read]); err != nil { + panic(err) + } + } +} + +func (s *CommonServer) closeClient(client client) { + client.close() + + // Stop tracking this client. + s.cond.L.Lock() + for i, c := range s.clients { + if c == client { + s.clients = append(s.clients[:i], s.clients[i+1:]...) + break + } + } + s.cond.Broadcast() + s.cond.L.Unlock() +} + +// Close stops listening and closes all connections. +func (s *CommonServer) Close() { + if s.socket != nil { + _ = s.socket.Close() + } + s.cond.L.Lock() + for _, client := range s.clients { + client.close() + } + s.clients = nil + s.cond.Broadcast() + s.cond.L.Unlock() + _ = os.Remove(s.Endpoint) +} + +// WaitForNoClients waits until the number of clients connected reaches 0. +func (s *CommonServer) WaitForNoClients() { + s.cond.L.Lock() + defer s.cond.L.Unlock() + for len(s.clients) > 0 { + s.cond.Wait() + } +} diff --git a/pkg/sentry/seccheck/checkers/remote/test/BUILD b/pkg/sentry/seccheck/checkers/remote/test/BUILD index 2e3fb6666..1d02d47b5 100644 --- a/pkg/sentry/seccheck/checkers/remote/test/BUILD +++ b/pkg/sentry/seccheck/checkers/remote/test/BUILD @@ -8,13 +8,9 @@ go_library( srcs = ["server.go"], visibility = ["//:sandbox"], deps = [ - "//pkg/cleanup", - "//pkg/log", + "//pkg/sentry/seccheck/checkers/remote/server", "//pkg/sentry/seccheck/checkers/remote/wire", "//pkg/sentry/seccheck/points:points_go_proto", "//pkg/sync", - "//pkg/unet", - "@org_golang_google_protobuf//proto:go_default_library", - "@org_golang_x_sys//unix:go_default_library", ], ) diff --git a/pkg/sentry/seccheck/checkers/remote/test/server.go b/pkg/sentry/seccheck/checkers/remote/test/server.go index bbe687c3e..e73443883 100644 --- a/pkg/sentry/seccheck/checkers/remote/test/server.go +++ b/pkg/sentry/seccheck/checkers/remote/test/server.go @@ -16,33 +16,23 @@ package test import ( - "errors" - "fmt" "io/ioutil" "os" "path/filepath" - "golang.org/x/sys/unix" - "google.golang.org/protobuf/proto" - "gvisor.dev/gvisor/pkg/cleanup" - "gvisor.dev/gvisor/pkg/log" + "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/server" "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire" pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" "gvisor.dev/gvisor/pkg/sync" - "gvisor.dev/gvisor/pkg/unet" ) // Server is the counterpart to the checkers.Remote. It receives connections // remote checkers and stores all points that it receives. type Server struct { - Path string - socket *unet.ServerSocket + server.CommonServer cond sync.Cond - // +checklocks:cond.L - clients []*unet.Socket - // +checklocks:cond.L points []Message @@ -67,147 +57,21 @@ func NewServer() (*Server, error) { if err != nil { return nil, err } - server, err := newServerPath(filepath.Join(dir, "remote.sock")) - if err != nil { - _ = os.RemoveAll(dir) - return nil, err - } - return server, nil -} - -func newServerPath(path string) (*Server, error) { - socket, err := unix.Socket(unix.AF_UNIX, unix.SOCK_SEQPACKET, 0) - if err != nil { - return nil, fmt.Errorf("socket(AF_UNIX, SOCK_SEQPACKET, 0): %w", err) - } - cu := cleanup.Make(func() { - _ = unix.Close(socket) - }) - defer cu.Clean() - - sa := &unix.SockaddrUnix{Name: path} - if err := unix.Bind(socket, sa); err != nil { - return nil, fmt.Errorf("bind(%q): %w", path, err) - } - - ss, err := unet.NewServerSocket(socket) - if err != nil { - return nil, err - } - cu.Add(func() { ss.Close() }) - - if err := ss.Listen(); err != nil { - return nil, err - } - - server := &Server{ - Path: path, - socket: ss, + s := &Server{ version: wire.CurrentVersion, cond: sync.Cond{L: &sync.Mutex{}}, } - go server.run() - cu.Release() - return server, nil + s.CommonServer.Init(filepath.Join(dir, "remote.sock"), s) + if err := s.CommonServer.Start(); err != nil { + _ = os.RemoveAll(dir) + return nil, err + } + return s, nil } -func (s *Server) run() { - for { - client, err := s.socket.Accept() - if err != nil { - // EBADF returns when the socket closes. - if !errors.Is(err, unix.EBADF) { - log.Warningf("socket.Accept(): %v", err) - } - return - } - if err := s.handshake(client); err != nil { - log.Warningf(err.Error()) - _ = client.Close() - continue - } - s.cond.L.Lock() - s.clients = append(s.clients, client) - s.cond.Broadcast() - s.cond.L.Unlock() - go s.handleClient(client) - } -} - -// handshake performs version exchange with client. See common.proto for details -// about the protocol. -func (s *Server) handshake(client *unet.Socket) error { - var in [1024]byte - read, err := client.Read(in[:]) - if err != nil { - return fmt.Errorf("reading handshake message: %w", err) - } - hsIn := pb.Handshake{} - if err := proto.Unmarshal(in[:read], &hsIn); err != nil { - return fmt.Errorf("unmarshalling handshake message: %w", err) - } - if hsIn.Version != wire.CurrentVersion { - return fmt.Errorf("wrong version number, want: %d, got, %d", wire.CurrentVersion, hsIn.Version) - } - - s.mu.Lock() - v := s.version - s.mu.Unlock() - hsOut := pb.Handshake{Version: v} - out, err := proto.Marshal(&hsOut) - if err != nil { - return fmt.Errorf("marshalling handshake message: %w", err) - } - if _, err := client.Write(out); err != nil { - return fmt.Errorf("sending handshake message: %w", err) - } - return nil -} - -func (s *Server) handleClient(client *unet.Socket) { - defer func() { - s.cond.L.Lock() - for i, c := range s.clients { - if c == client { - s.clients = append(s.clients[:i], s.clients[i+1:]...) - break - } - } - s.cond.Broadcast() - s.cond.L.Unlock() - _ = client.Close() - }() - - var buf = make([]byte, 1024*1024) - for { - read, err := client.Read(buf) - if err != nil { - return - } - if read == 0 { - return - } - if read < wire.HeaderStructSize { - panic("invalid message") - } - hdr := wire.Header{} - hdr.UnmarshalUnsafe(buf[0:wire.HeaderStructSize]) - if read < int(hdr.HeaderSize) { - panic(fmt.Sprintf("message truncated, header size: %d, readL %d", hdr.HeaderSize, read)) - } - - msgSize := read - int(hdr.HeaderSize) - msg := Message{ - MsgType: pb.MessageType(hdr.MessageType), - Msg: make([]byte, msgSize), - } - copy(msg.Msg, buf[hdr.HeaderSize:read]) - - s.cond.L.Lock() - s.points = append(s.points, msg) - s.cond.Broadcast() - s.cond.L.Unlock() - } +// NewClient returns a new MessageHandler to process messages. +func (s *Server) NewClient() (server.MessageHandler, error) { + return &msgHandler{owner: s}, nil } // Count return the number of points it has received. @@ -236,19 +100,6 @@ func (s *Server) GetPoints() []Message { return cpy } -// Close stops listenning and closes all connections. -func (s *Server) Close() { - _ = s.socket.Close() - s.cond.L.Lock() - for _, client := range s.clients { - _ = client.Close() - } - s.clients = nil - s.cond.Broadcast() - s.cond.L.Unlock() - _ = os.Remove(s.Path) -} - // WaitForCount waits for the number of points to reach the desired number. func (s *Server) WaitForCount(count int) { s.cond.L.Lock() @@ -259,18 +110,38 @@ func (s *Server) WaitForCount(count int) { return } -// WaitForNoClients waits until the number of clients connected reaches 0. -func (s *Server) WaitForNoClients() { - s.cond.L.Lock() - defer s.cond.L.Unlock() - for len(s.clients) > 0 { - s.cond.Wait() - } -} - // SetVersion sets the version to be used in handshake. func (s *Server) SetVersion(newVersion uint32) { s.mu.Lock() defer s.mu.Unlock() s.version = newVersion } + +type msgHandler struct { + owner *Server +} + +// Message stores the message type and payload. +func (m *msgHandler) Message(_ []byte, hdr wire.Header, payload []byte) error { + msg := Message{ + MsgType: pb.MessageType(hdr.MessageType), + Msg: make([]byte, len(payload)), + } + copy(msg.Msg, payload) + + m.owner.cond.L.Lock() + defer m.owner.cond.L.Unlock() + m.owner.points = append(m.owner.points, msg) + m.owner.cond.Broadcast() + return nil +} + +// Version returns the wire version supported or overriden by SetVersion. +func (m *msgHandler) Version() uint32 { + m.owner.mu.Lock() + defer m.owner.mu.Unlock() + return m.owner.version +} + +// Close implements server.MessageHandler. +func (m *msgHandler) Close() {} diff --git a/runsc/container/trace_test.go b/runsc/container/trace_test.go index 9a98af330..a2df2d5a5 100644 --- a/runsc/container/trace_test.go +++ b/runsc/container/trace_test.go @@ -69,7 +69,7 @@ func TestTraceStartup(t *testing.T) { ContextFields: []string{"container_id"}, }, }, - Sinks: []seccheck.SinkConfig{remoteSinkConfig(server.Path)}, + Sinks: []seccheck.SinkConfig{remoteSinkConfig(server.Endpoint)}, }, } encoder := json.NewEncoder(podInitConfig) @@ -148,7 +148,7 @@ func TestTraceLifecycle(t *testing.T) { ContextFields: []string{"container_id"}, }, }, - Sinks: []seccheck.SinkConfig{remoteSinkConfig(server.Path)}, + Sinks: []seccheck.SinkConfig{remoteSinkConfig(server.Endpoint)}, } if err := cont.Sandbox.CreateTraceSession(&session, false); err != nil { t.Fatalf("CreateTraceSession(): %v", err) @@ -248,7 +248,7 @@ func TestTraceForceCreate(t *testing.T) { Points: []seccheck.PointConfig{ {Name: "sentry/exit_notify_parent"}, }, - Sinks: []seccheck.SinkConfig{remoteSinkConfig(server.Path)}, + Sinks: []seccheck.SinkConfig{remoteSinkConfig(server.Endpoint)}, } if err := cont.Sandbox.CreateTraceSession(&session, false); err != nil { t.Fatalf("CreateTraceSession(): %v", err) @@ -277,7 +277,7 @@ func TestTraceForceCreate(t *testing.T) { Points: []seccheck.PointConfig{ {Name: "sentry/task_exit"}, }, - Sinks: []seccheck.SinkConfig{remoteSinkConfig(server.Path)}, + Sinks: []seccheck.SinkConfig{remoteSinkConfig(server.Endpoint)}, } if err := cont.Sandbox.CreateTraceSession(&session, true); err != nil { t.Fatalf("CreateTraceSession(force): %v", err) diff --git a/test/trace/trace_test.go b/test/trace/trace_test.go index cd5de6ef8..76f370270 100644 --- a/test/trace/trace_test.go +++ b/test/trace/trace_test.go @@ -48,7 +48,7 @@ func TestAll(t *testing.T) { if err != nil { t.Fatal(err) } - cfg, err := buildPodConfig(runsc, server.Path) + cfg, err := buildPodConfig(runsc, server.Endpoint) if err != nil { t.Fatal(err) } diff --git a/tools/tracereplay/BUILD b/tools/tracereplay/BUILD new file mode 100644 index 000000000..54d131568 --- /dev/null +++ b/tools/tracereplay/BUILD @@ -0,0 +1,36 @@ +load("//tools:defs.bzl", "go_library", "go_test") + +package(licenses = ["notice"]) + +go_library( + name = "tracereplay", + srcs = [ + "replay.go", + "save.go", + "tracereplay.go", + ], + visibility = [ + "//tools/tracereplay:__subpackages__", + ], + deps = [ + "//pkg/atomicbitops", + "//pkg/log", + "//pkg/sentry/seccheck/checkers/remote/server", + "//pkg/sentry/seccheck/checkers/remote/wire", + "//pkg/sentry/seccheck/points:points_go_proto", + "@org_golang_google_protobuf//proto:go_default_library", + "@org_golang_x_sys//unix:go_default_library", + ], +) + +go_test( + name = "tracereplay_test", + srcs = ["tracereplay_test.go"], + data = [ + "testdata/client-0001", + ], + library = ":tracereplay", + deps = [ + "//pkg/test/testutil", + ], +) diff --git a/tools/tracereplay/README.md b/tools/tracereplay/README.md new file mode 100644 index 000000000..964f89666 --- /dev/null +++ b/tools/tracereplay/README.md @@ -0,0 +1,77 @@ +# What is it? + +The `tracereplay` tool can save `runsc trace` sessions to a file, and later +replay the same sequence of messages. This can be used to run tests that rely on +the messages without the need to setup runsc, configure trace sessions, and run +specific workloads. + +# How to use it? + +The `tracereplay save` command starts a server that listens to new connections +from runsc and creates a trace file for each runsc instance that connects to it. +The command below starts a server on listening on `/tmp/gvisor_events.sock` and +writes trace files to `/tmp/trace` directory: + +```shell +$ tracereplay save --endpoint=/tmp/gvisor_events.sock --out=/tmp/trace +``` + +When you execute runsc configured with a trace session using the `remote` sink +connecting to `/tmp/gvisor_events.sock`, all messages will be saved to a file +under `/tmp/trace`. For example, if you run the following commands, runsc will +connect to the server above and all trace points triggered by the workload will +be stored in the save file: + +```shell +$ cat > /tmp/pod_init.json < id: "runsc-865139" cwd: "/home/fvoznika" args: "/bin/true" +Connection closed +``` diff --git a/tools/tracereplay/main/BUILD b/tools/tracereplay/main/BUILD new file mode 100644 index 000000000..1e114b7da --- /dev/null +++ b/tools/tracereplay/main/BUILD @@ -0,0 +1,15 @@ +load("//tools:defs.bzl", "go_binary") + +package(licenses = ["notice"]) + +go_binary( + name = "tracereplay", + srcs = [ + "main.go", + ], + deps = [ + "//runsc/flag", + "//tools/tracereplay", + "@com_github_google_subcommands//:go_default_library", + ], +) diff --git a/tools/tracereplay/main/main.go b/tools/tracereplay/main/main.go new file mode 100644 index 000000000..979aeefc6 --- /dev/null +++ b/tools/tracereplay/main/main.go @@ -0,0 +1,152 @@ +// 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 main implements a tool that can save and replay messages from +// issued from remote.Remote. +package main + +import ( + "context" + "fmt" + "os" + "os/signal" + + "github.com/google/subcommands" + "gvisor.dev/gvisor/runsc/flag" + "gvisor.dev/gvisor/tools/tracereplay" +) + +func main() { + subcommands.Register(subcommands.HelpCommand(), "") + subcommands.Register(subcommands.FlagsCommand(), "") + subcommands.Register(&saveCmd{}, "") + subcommands.Register(&replayCmd{}, "") + flag.CommandLine.Parse(os.Args[1:]) + os.Exit(int(subcommands.Execute(context.Background()))) +} + +// saveCmd implements subcommands.Command for the "save" command. +type saveCmd struct { + endpoint string + out string + prefix string +} + +// Name implements subcommands.Command. +func (*saveCmd) Name() string { + return "save" +} + +// Synopsis implements subcommands.Command. +func (*saveCmd) Synopsis() string { + return "save trace sessions to files" +} + +// Usage implements subcommands.Command. +func (*saveCmd) Usage() string { + return `save [flags] - save trace sessions to files +` +} + +// SetFlags implements subcommands.Command. +func (c *saveCmd) SetFlags(f *flag.FlagSet) { + f.StringVar(&c.endpoint, "endpoint", "", "path to trace server endpoint to connect") + f.StringVar(&c.out, "out", "./replay", "path to a directory where trace files will be saved") + f.StringVar(&c.prefix, "prefix", "client-", "name to be prefixed to each trace file") +} + +// Execute implements subcommands.Command. +func (c *saveCmd) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) subcommands.ExitStatus { + if f.NArg() > 0 { + fmt.Fprintf(os.Stderr, "unexpected argument: %s\n", f.Args()) + return subcommands.ExitUsageError + } + if len(c.endpoint) == 0 { + fmt.Fprintf(os.Stderr, "--endpoint is required\n") + return subcommands.ExitUsageError + } + _ = os.Remove(c.endpoint) + + server := tracereplay.NewSave(c.endpoint, c.out, c.prefix) + defer server.Close() + + if err := server.Start(); err != nil { + fmt.Fprintf(os.Stderr, "starting server: %v\n", err) + return subcommands.ExitFailure + } + + ch := make(chan os.Signal) + signal.Notify(ch, os.Interrupt) + + done := make(chan struct{}) + go func() { + <-ch + fmt.Printf("Ctrl-C pressed, stopping.\n") + done <- struct{}{} + }() + + fmt.Printf("Listening on %q. Press ctrl-C to stop...\n", c.endpoint) + <-done + return subcommands.ExitSuccess +} + +// replayCmd implements subcommands.Command for the "replay" command. +type replayCmd struct { + endpoint string + in string +} + +// Name implements subcommands.Command. +func (*replayCmd) Name() string { + return "replay" +} + +// Synopsis implements subcommands.Command. +func (*replayCmd) Synopsis() string { + return "replay a trace session from a file" +} + +// Usage implements subcommands.Command. +func (*replayCmd) Usage() string { + return `replay [flags] - replay a trace session from a file +` +} + +// SetFlags implements subcommands.Command. +func (c *replayCmd) SetFlags(f *flag.FlagSet) { + f.StringVar(&c.endpoint, "endpoint", "", "path to trace server endpoint to connect") + f.StringVar(&c.in, "in", "", "path to trace file containing messages to be replayed") +} + +// Execute implements subcommands.Command. +func (c *replayCmd) Execute(_ context.Context, f *flag.FlagSet, _ ...interface{}) subcommands.ExitStatus { + if f.NArg() > 0 { + fmt.Fprintf(os.Stderr, "unexpected argument: %s\n", f.Args()) + return subcommands.ExitUsageError + } + if len(c.in) == 0 { + fmt.Fprintf(os.Stderr, "--in is required\n") + return subcommands.ExitUsageError + } + + r := tracereplay.Replay{ + Endpoint: c.endpoint, + In: c.in, + } + if err := r.Execute(); err != nil { + fmt.Fprintln(os.Stderr, err) + return subcommands.ExitFailure + } + return subcommands.ExitSuccess +} diff --git a/tools/tracereplay/replay.go b/tools/tracereplay/replay.go new file mode 100644 index 000000000..3a2fc7b16 --- /dev/null +++ b/tools/tracereplay/replay.go @@ -0,0 +1,133 @@ +// 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 tracereplay + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "os" + + "golang.org/x/sys/unix" + "google.golang.org/protobuf/proto" + "gvisor.dev/gvisor/pkg/log" + pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" +) + +// Replay implements the functionality required for the "replay" command. +type Replay struct { + Endpoint string + In string +} + +// Execute connects to the remote endpoint and replays all messages stored in +// the `In` file. +func (r *Replay) Execute() error { + socket, err := connect(r.Endpoint) + if err != nil { + return err + } + defer socket.Close() + + f, err := os.Open(r.In) + if err != nil { + return err + } + defer f.Close() + + hdr := make([]byte, len(signature)) + if err := readFull(f, hdr); err != nil { + return err + } + if string(hdr) != signature { + return fmt.Errorf("%q is not a replay file", r.In) + } + + cfgJSON, err := readWithSize(f) + if err != nil { + return err + } + cfg := Config{} + if err := json.Unmarshal(cfgJSON, &cfg); err != nil { + return err + } + if err := handshake(socket, cfg.Version); err != nil { + return err + } + fmt.Printf("Handshake completed\n") + + for count := 1; ; count++ { + bytes, err := readWithSize(f) + if err != nil { + if errors.Is(err, io.EOF) { + break + } + return err + } + fmt.Printf("\rReplaying message: %d", count) + if _, err := socket.Write(bytes); err != nil { + return err + } + } + fmt.Printf("\nDone\n") + return nil +} + +func connect(endpoint string) (*os.File, error) { + log.Debugf("Connecting to %q", endpoint) + socket, err := unix.Socket(unix.AF_UNIX, unix.SOCK_SEQPACKET, 0) + if err != nil { + return nil, fmt.Errorf("socket(AF_UNIX, SOCK_SEQPACKET, 0): %w", err) + } + f := os.NewFile(uintptr(socket), endpoint) + + addr := unix.SockaddrUnix{Name: endpoint} + if err := unix.Connect(int(f.Fd()), &addr); err != nil { + _ = f.Close() + return nil, fmt.Errorf("connect(%q): %w", endpoint, err) + } + return f, nil +} + +// See common.proto for details about the handshake protocol. +func handshake(socket *os.File, version uint32) error { + hsOut := pb.Handshake{Version: version} + out, err := proto.Marshal(&hsOut) + if err != nil { + return err + } + if _, err := socket.Write(out); err != nil { + return fmt.Errorf("sending handshake message: %w", err) + } + + in := make([]byte, 10240) + read, err := socket.Read(in) + if err != nil && !errors.Is(err, io.EOF) { + return fmt.Errorf("reading handshake message: %w", err) + } + // Protect against the handshake becoming larger than the buffer. + if read == len(in) { + return fmt.Errorf("handshake message too big") + } + hsIn := pb.Handshake{} + if err := proto.Unmarshal(in[:read], &hsIn); err != nil { + return fmt.Errorf("unmarshalling handshake message: %w", err) + } + + // Just validate that the message can unmarshall and accept any version from + // the server. Will try to replay and see what happens... + return nil +} diff --git a/tools/tracereplay/save.go b/tools/tracereplay/save.go new file mode 100644 index 000000000..b12a00a62 --- /dev/null +++ b/tools/tracereplay/save.go @@ -0,0 +1,111 @@ +// 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 tracereplay + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + + "gvisor.dev/gvisor/pkg/atomicbitops" + "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/server" + "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/wire" +) + +// Save implements the functionality required for the "save" command. +type Save struct { + server.CommonServer + dir string + prefix string + clientCount atomicbitops.Uint64 +} + +var _ server.ClientHandler = (*Save)(nil) + +// NewSave creates a new Save instance. +func NewSave(endpoint, dir, prefix string) *Save { + s := &Save{dir: dir, prefix: prefix} + s.CommonServer.Init(endpoint, s) + return s +} + +// Start starts the server. +func (s *Save) Start() error { + if err := os.MkdirAll(s.dir, 0755); err != nil { + return err + } + return s.CommonServer.Start() +} + +// NewClient creates a new file for the client and writes messages to it. +// +// The file format starts with a string signature to make it easy to check that +// it's a trace file. The signature is followed by a JSON configuration that +// contains information required to process the file. Next, there are a sequence +// of messages. Both JSON and messages are prefixed by an uint64 with their +// size. +// +// Ex: +// signature Config JSON [message]* +func (s *Save) NewClient() (server.MessageHandler, error) { + seq := s.clientCount.Add(1) + filename := filepath.Join(s.dir, fmt.Sprintf("%s%04d", s.prefix, seq)) + fmt.Printf("New client connected, writing to: %q\n", filename) + + out, err := os.Create(filename) + if err != nil { + return nil, err + } + if _, err := out.Write([]byte(signature)); err != nil { + return nil, err + } + + handler := &msgHandler{out: out} + + cfg, err := json.Marshal(Config{Version: handler.Version()}) + if err != nil { + return nil, err + } + if err := writeWithSize(out, cfg); err != nil { + return nil, err + } + + return handler, nil +} + +type msgHandler struct { + out *os.File + messageCount atomicbitops.Uint64 +} + +var _ server.MessageHandler = (*msgHandler)(nil) + +// Version implements server.MessageHandler. +func (m *msgHandler) Version() uint32 { + return wire.CurrentVersion +} + +// Message saves the message to the client file. +func (m *msgHandler) Message(raw []byte, _ wire.Header, _ []byte) error { + m.messageCount.Add(1) + return writeWithSize(m.out, raw) +} + +// Close closes the client file. +func (m *msgHandler) Close() { + fmt.Printf("Closing client, wrote %d messages to %q\n", m.messageCount.Load(), m.out.Name()) + _ = m.out.Close() +} diff --git a/tools/tracereplay/testdata/client-0001 b/tools/tracereplay/testdata/client-0001 new file mode 100644 index 0000000000000000000000000000000000000000..cdcc767302a7522b9e7e357fc9080921c426f52a GIT binary patch literal 804 zcmXRZN=!~IN-fAqtW-$L%t_^CfPiYHvecsD%=|nhE5q6dh!_J010$Fvb46iu3i7 z6O%JiEgBjW48ST8dW4ubn2FJ6jiwJxkGy_PW|Dq|g;~6riEd72UTKAHdR{3=Fd1aM z8OUmTG$XKBO^h*(aAP>ZEG|EeWqYn}-`lkOmzV&f5yfGwz^tVNayWM1=o;x68|oG3 z8ysL00$UFXN{B;*m^luxDKHxxU@L^nuz^`zJ{)`Af8GOjA{89SsKIFP;pqG0J9dj& pusC!+g~fOPV=34=kdxpRKbzLDmr523F>$ac;I^CtZaFKM1pt{E;IRMz literal 0 HcmV?d00001 diff --git a/tools/tracereplay/tracereplay.go b/tools/tracereplay/tracereplay.go new file mode 100644 index 000000000..eef449c05 --- /dev/null +++ b/tools/tracereplay/tracereplay.go @@ -0,0 +1,83 @@ +// 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 tracereplay implements a tool that can save and replay messages +// issued from remote.Remote. +package tracereplay + +import ( + "encoding/binary" + "fmt" + "io" + "os" +) + +const signature = "tracereplay file" + +func writeSize(w io.Writer, val int) error { + var bin [8]byte + binary.LittleEndian.PutUint64(bin[:], uint64(val)) + _, err := w.Write(bin[:]) + return err +} + +func readSize(r io.Reader) (int, error) { + var bin [8]byte + if read, err := r.Read(bin[:]); err != nil { + return 0, err + } else if read != 8 { + return 0, fmt.Errorf("truncated read (%d bytes)", read) + } + size := int(binary.LittleEndian.Uint64(bin[:])) + // Prevent returning a too large size to avoid OOMs. + if size > 1024*1024 { + return 0, fmt.Errorf("size is too big: %d", size) + } + return size, nil +} + +func writeWithSize(f *os.File, buf []byte) error { + if err := writeSize(f, len(buf)); err != nil { + return err + } + _, err := f.Write(buf) + return err +} + +func readWithSize(r io.Reader) ([]byte, error) { + size, err := readSize(r) + if err != nil { + return nil, err + } + bytes := make([]byte, size) + if err := readFull(r, bytes); err != nil { + return nil, err + } + return bytes, nil +} + +func readFull(r io.Reader, dest []byte) error { + if read, err := r.Read(dest); err != nil { + return err + } else if read < len(dest) { + return fmt.Errorf("truncated read. Read %d bytes, expected %d bytes", read, len(dest)) + } + return nil +} + +// Config contains information required to replay messages from a file. +type Config struct { + // Version is the wire format saved in the file. + Version uint32 `json:"version"` +} diff --git a/tools/tracereplay/tracereplay_test.go b/tools/tracereplay/tracereplay_test.go new file mode 100644 index 000000000..f3756b001 --- /dev/null +++ b/tools/tracereplay/tracereplay_test.go @@ -0,0 +1,76 @@ +// 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 tracereplay + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "gvisor.dev/gvisor/pkg/test/testutil" +) + +// TestBasic uses a pre-generated file that is replayed into a save process. +// Then verifies that the generated file looks exactly the same as the original. +// In other words, it is doing `replay original | save new`, then checking if +// `original == new`. +func TestBasic(t *testing.T) { + dir, err := os.MkdirTemp(testutil.TmpDir(), "tracereplay") + if err != nil { + t.Fatal(err) + } + endpoint := filepath.Join(dir, "tracereplay.sock") + + // Start a new save server to store the replayed file. This tests that save + // communicates with clients correctly and generates a valid file. + s := NewSave(endpoint, filepath.Join(dir, "out"), "test-") + defer s.Close() + + if err := s.Start(); err != nil { + t.Fatal(err) + } + + // Then replay the re-generated file. This tests that replay can connect to + // a server and process the generated file. + r := Replay{} + r.Endpoint = endpoint + + const testdata = "tools/tracereplay/testdata/client-0001" + r.In, err = testutil.FindFile(testdata) + if err != nil { + t.Fatalf("FindFile(%q): %v", testdata, err) + } + + if err := r.Execute(); err != nil { + t.Fatal(err) + } + + // Wait until all messages are processed and client disconnects. + s.WaitForNoClients() + + // The generated file must be an exact copy of the original file. + want, err := os.ReadFile(r.In) + if err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(filepath.Join(dir, "out", "test-0001")) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(want, got) { + t.Errorf("files don't match\nwant: %s\ngot: %s", want, got) + } +} From 0301bb07efabc1b6e441727814fce25cf8a847ca Mon Sep 17 00:00:00 2001 From: Zach Koopmans Date: Mon, 13 Jun 2022 10:48:51 -0700 Subject: [PATCH 02/29] [bugs] Don't hold root taskset mutex when starting kernel tasks. Holding the kernel.taskSetRWMutex isn't necessary while calling t.Start() and doing so can cause a nested locking error. We only need to make sure start is called on each task once. PiperOrigin-RevId: 454648122 --- pkg/sentry/kernel/kernel.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index 623e96ba4..c2973adcc 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/pkg/sentry/kernel/kernel.go @@ -1122,11 +1122,18 @@ func (k *Kernel) Start() error { // Kernel.SaveTo and need to be resumed. If k was created by NewKernel, // this is a no-op. k.resumeTimeLocked(k.SupervisorContext()) - // Start task goroutines. k.tasks.mu.RLock() - defer k.tasks.mu.RUnlock() - for t, tid := range k.tasks.Root.tids { - t.Start(tid) + ts := make([]*Task, 0, len(k.tasks.Root.tids)) + for t := range k.tasks.Root.tids { + ts = append(ts, t) + } + k.tasks.mu.RUnlock() + // Start task goroutines. + // NOTE(b/235349091): We don't actually need the TaskSet mutex, we just + // need to make sure we only call t.Start() once for each task. Holding the + // mutex for each task start may cause a nested locking error. + for _, t := range ts { + t.Start(t.ThreadID()) } return nil } From 2610918ed43d26c5116c2245c87b31e0c8a40a68 Mon Sep 17 00:00:00 2001 From: Nayana Bidari Date: Mon, 13 Jun 2022 10:54:30 -0700 Subject: [PATCH 03/29] Run two containers This change will allow the gVisor sandbox to run multiple containers. - Added new functions to validate the SentryMultiContainerSpec and the container mounts. - Changes to set the target path in container mounts for eg: root target will be "/container0" and the submounts target will be "/container0/dev".. etc in multi-container mode. - Added new fields in the protos: container name in ContainerSpec and MultiContainerConfig in SentryConfig. - Updated the sentry to create the mount namespaces for all the containers during the sentry start as we have all the container mount configs. These mount namespaces are passed to the control server in a map with the container name as the key. - Updated changes in StartContainer(): - Use ContainerSpec instead of SandboxRunSpec. - No longer create the mount namespaces here and use the ones created by the sentry. - Added test to run two containers in multi-container mode using the SentryMultiContainerSpec. PiperOrigin-RevId: 454649646 --- pkg/sentry/control/lifecycle.go | 42 +++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/pkg/sentry/control/lifecycle.go b/pkg/sentry/control/lifecycle.go index 52b44babc..071d893f3 100644 --- a/pkg/sentry/control/lifecycle.go +++ b/pkg/sentry/control/lifecycle.go @@ -26,6 +26,7 @@ import ( "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/limits" "gvisor.dev/gvisor/pkg/sentry/vfs" + "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/pkg/urpc" ) @@ -34,13 +35,19 @@ type Lifecycle struct { // Kernel is the kernel where the tasks belong to. Kernel *kernel.Kernel - // Sends a message to the sentry that the task has been started. + // StartedCh is the channel used to send a message to the sentry that + // all the containers in the sandbox have been started. StartedCh chan struct{} - // TODO(b/202052732): Root mount namespace. When running multiple - // containers, create the mount namespace using the mount spec in - // the StartContainerArgs. - MountNamespaceVFS2 *vfs.MountNamespace + // mu protects the fields below. + mu sync.RWMutex + + // containersStarted is the number of containers started in the sandbox. + containersStarted int32 + + // MountNamespacesMap is a map of container id/names and the mount + // namespaces. + MountNamespacesMap map[string]*vfs.MountNamespace } // StartContainerArgs is the set of arguments to start a container. @@ -127,12 +134,22 @@ func (l *Lifecycle) StartContainer(args *StartContainerArgs, _ *uint32) error { AbstractSocketNamespace: l.Kernel.RootAbstractSocketNamespace(), ContainerID: args.ContainerID, PIDNamespace: l.Kernel.RootPIDNamespace(), - MountNamespaceVFS2: l.MountNamespaceVFS2, } ctx := initArgs.NewContext(l.Kernel) defer fdTable.DecRef(ctx) + // VFS2 is supported in multi-container mode by default. + l.mu.RLock() + mntns, ok := l.MountNamespacesMap[initArgs.ContainerID] + if !ok { + l.mu.RUnlock() + return fmt.Errorf("mount namespace is nil for %s", initArgs.ContainerID) + } + initArgs.MountNamespaceVFS2 = mntns + l.mu.RUnlock() + initArgs.MountNamespaceVFS2.IncRef() + resolved, err := user.ResolveExecutablePath(ctx, &initArgs) if err != nil { return err @@ -154,12 +171,17 @@ func (l *Lifecycle) StartContainer(args *StartContainerArgs, _ *uint32) error { return err } + l.mu.Lock() + numContainers := int32(len(l.MountNamespacesMap)) + // Start the newly created process. l.Kernel.StartProcess(tg) - - log.Infof("Started the new container") - - l.StartedCh <- struct{}{} + log.Infof("Started the new container %v ", l.containersStarted) + l.containersStarted++ + if numContainers == l.containersStarted { + l.StartedCh <- struct{}{} + } + l.mu.Unlock() return nil } From 5ffcc1f799e31eba3a95d7e2f251ee111656520c Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Mon, 13 Jun 2022 15:02:23 -0700 Subject: [PATCH 04/29] Don't leak network namespaces PiperOrigin-RevId: 454707336 --- pkg/sentry/fsimpl/testutil/kernel.go | 1 + pkg/sentry/inet/BUILD | 14 ++++++++++++++ pkg/sentry/inet/inet.go | 3 +++ pkg/sentry/inet/namespace.go | 16 +++++++++++++++- pkg/sentry/inet/test_stack.go | 4 ++++ pkg/sentry/kernel/kernel.go | 2 ++ pkg/sentry/kernel/task_clone.go | 10 ++++++++++ pkg/sentry/kernel/task_exit.go | 2 ++ pkg/sentry/kernel/task_start.go | 1 + pkg/sentry/socket/hostinet/stack.go | 4 ++++ pkg/sentry/socket/netstack/stack.go | 5 +++++ 11 files changed, 61 insertions(+), 1 deletion(-) diff --git a/pkg/sentry/fsimpl/testutil/kernel.go b/pkg/sentry/fsimpl/testutil/kernel.go index 2df2501ee..e0fd2ff39 100644 --- a/pkg/sentry/fsimpl/testutil/kernel.go +++ b/pkg/sentry/fsimpl/testutil/kernel.go @@ -147,6 +147,7 @@ func CreateTask(ctx context.Context, name string, tc *kernel.ThreadGroup, mntns FDTable: k.NewFDTable(), UserCounters: k.GetUserCounters(creds.RealKUID), } + config.NetworkNamespace.IncRef() t, err := k.TaskSet().NewTask(ctx, config) if err != nil { config.ThreadGroup.Release(ctx) diff --git a/pkg/sentry/inet/BUILD b/pkg/sentry/inet/BUILD index 3bd141b89..156addaf2 100644 --- a/pkg/sentry/inet/BUILD +++ b/pkg/sentry/inet/BUILD @@ -6,6 +6,17 @@ package( licenses = ["notice"], ) +go_template_instance( + name = "namespace_refs", + out = "namespace_refs.go", + package = "inet", + prefix = "namespace", + template = "//pkg/refsvfs2:refs_template", + types = { + "T": "Namespace", + }, +) + go_template_instance( name = "atomicptr_netns", out = "atomicptr_netns_unsafe.go", @@ -24,11 +35,14 @@ go_library( "context.go", "inet.go", "namespace.go", + "namespace_refs.go", "test_stack.go", ], deps = [ "//pkg/abi/linux", + "//pkg/atomicbitops", "//pkg/context", + "//pkg/refsvfs2", "//pkg/tcpip", "//pkg/tcpip/stack", ], diff --git a/pkg/sentry/inet/inet.go b/pkg/sentry/inet/inet.go index b80e07679..5daa76b1f 100644 --- a/pkg/sentry/inet/inet.go +++ b/pkg/sentry/inet/inet.go @@ -85,6 +85,9 @@ type Stack interface { // Resume restarts the network stack after restore. Resume() + // Destroy the network stack. + Destroy() + // RegisteredEndpoints returns all endpoints which are currently registered. RegisteredEndpoints() []stack.TransportEndpoint diff --git a/pkg/sentry/inet/namespace.go b/pkg/sentry/inet/namespace.go index 029af3025..e66dbaa9f 100644 --- a/pkg/sentry/inet/namespace.go +++ b/pkg/sentry/inet/namespace.go @@ -18,6 +18,8 @@ package inet // // +stateify savable type Namespace struct { + namespaceRefs + // stack is the network stack implementation of this network namespace. stack Stack `state:"nosave"` @@ -36,11 +38,13 @@ type Namespace struct { // allowing new network namespaces to be created. If creator is nil, no // networking will function if the network is namespaced. func NewRootNamespace(stack Stack, creator NetworkStackCreator) *Namespace { - return &Namespace{ + n := &Namespace{ stack: stack, creator: creator, isRoot: true, } + n.InitRefs() + return n } // NewNamespace creates a new network namespace from the root. @@ -49,9 +53,19 @@ func NewNamespace(root *Namespace) *Namespace { creator: root.creator, } n.init() + n.InitRefs() return n } +// DecRef decrements the Namespace's refcount. +func (n *Namespace) DecRef() { + n.namespaceRefs.DecRef(func() { + if s := n.Stack(); s != nil { + s.Destroy() + } + }) +} + // Stack returns the network stack of n. Stack may return nil if no network // stack is configured. func (n *Namespace) Stack() Stack { diff --git a/pkg/sentry/inet/test_stack.go b/pkg/sentry/inet/test_stack.go index fef7391b9..e04f2c144 100644 --- a/pkg/sentry/inet/test_stack.go +++ b/pkg/sentry/inet/test_stack.go @@ -50,6 +50,10 @@ func (s *TestStack) Interfaces() map[int32]Interface { return s.InterfacesMap } +// Destroy implements Stack. +func (s *TestStack) Destroy() { +} + // RemoveInterface implements Stack. func (s *TestStack) RemoveInterface(idx int32) error { delete(s.InterfacesMap, idx) diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index c2973adcc..7ec524d2a 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/pkg/sentry/kernel/kernel.go @@ -1079,6 +1079,7 @@ func (k *Kernel) CreateProcess(args CreateProcessArgs) (*ThreadGroup, ThreadID, ContainerID: args.ContainerID, UserCounters: k.GetUserCounters(args.Credentials.RealKUID), } + config.NetworkNamespace.IncRef() t, err := k.tasks.NewTask(ctx, config) if err != nil { return nil, 0, err @@ -1853,6 +1854,7 @@ func (k *Kernel) Release() { } k.timekeeper.Destroy() k.vdso.Release(ctx) + k.RootNetworkNamespace().DecRef() } // PopulateNewCgroupHierarchy moves all tasks into a newly created cgroup diff --git a/pkg/sentry/kernel/task_clone.go b/pkg/sentry/kernel/task_clone.go index ca3684829..d5b3b181b 100644 --- a/pkg/sentry/kernel/task_clone.go +++ b/pkg/sentry/kernel/task_clone.go @@ -117,7 +117,12 @@ func (t *Task) Clone(args *linux.CloneArgs) (ThreadID, *SyscallControl, error) { netns := t.NetworkNamespace() if args.Flags&linux.CLONE_NEWNET != 0 { netns = inet.NewNamespace(netns) + } else { + netns.IncRef() } + cu.Add(func() { + netns.DecRef() + }) // TODO(b/63601033): Implement CLONE_NEWNS. mntnsVFS2 := t.mountNamespaceVFS2 @@ -454,11 +459,13 @@ func (t *Task) Unshare(flags int32) error { } t.mu.Lock() // Can't defer unlock: DecRefs must occur without holding t.mu. + var oldNETNS *inet.Namespace if flags&linux.CLONE_NEWNET != 0 { if !haveCapSysAdmin { t.mu.Unlock() return linuxerr.EPERM } + oldNETNS = t.netns.Load() t.netns.Store(inet.NewNamespace(t.netns.Load())) } if flags&linux.CLONE_NEWUTS != 0 { @@ -498,6 +505,9 @@ func (t *Task) Unshare(flags int32) error { if oldIPCNS != nil { oldIPCNS.DecRef(t) } + if oldNETNS != nil { + oldNETNS.DecRef() + } if oldFDTable != nil { oldFDTable.DecRef(t) } diff --git a/pkg/sentry/kernel/task_exit.go b/pkg/sentry/kernel/task_exit.go index 717c2b29f..f0eebda40 100644 --- a/pkg/sentry/kernel/task_exit.go +++ b/pkg/sentry/kernel/task_exit.go @@ -272,11 +272,13 @@ func (*runExitMain) execute(t *Task) taskRunState { mntns := t.mountNamespaceVFS2 t.mountNamespaceVFS2 = nil ipcns := t.ipcns + netns := t.NetworkNamespace() t.mu.Unlock() if mntns != nil { mntns.DecRef(t) } ipcns.DecRef(t) + netns.DecRef() // If this is the last task to exit from the thread group, release the // thread group's resources. diff --git a/pkg/sentry/kernel/task_start.go b/pkg/sentry/kernel/task_start.go index 2904398ad..b7497563b 100644 --- a/pkg/sentry/kernel/task_start.go +++ b/pkg/sentry/kernel/task_start.go @@ -115,6 +115,7 @@ func (ts *TaskSet) NewTask(ctx context.Context, cfg *TaskConfig) (*Task, error) cfg.FSContext.DecRef(ctx) cfg.FDTable.DecRef(ctx) cfg.IPCNamespace.DecRef(ctx) + cfg.NetworkNamespace.DecRef() if cfg.MountNamespaceVFS2 != nil { cfg.MountNamespaceVFS2.DecRef(ctx) } diff --git a/pkg/sentry/socket/hostinet/stack.go b/pkg/sentry/socket/hostinet/stack.go index 3e176d80c..f64827bd6 100644 --- a/pkg/sentry/socket/hostinet/stack.go +++ b/pkg/sentry/socket/hostinet/stack.go @@ -65,6 +65,10 @@ type Stack struct { netSNMPFile *os.File } +// Destroy implements inet.Stack.Destroy. +func (*Stack) Destroy() { +} + // NewStack returns an empty Stack containing no configuration. func NewStack() *Stack { return &Stack{ diff --git a/pkg/sentry/socket/netstack/stack.go b/pkg/sentry/socket/netstack/stack.go index 36f377edf..bf2b0741a 100644 --- a/pkg/sentry/socket/netstack/stack.go +++ b/pkg/sentry/socket/netstack/stack.go @@ -37,6 +37,11 @@ type Stack struct { Stack *stack.Stack `state:"manual"` } +// Destroy implements inet.Stack.Destroy. +func (s *Stack) Destroy() { + s.Stack.Close() +} + // SupportsIPv6 implements Stack.SupportsIPv6. func (s *Stack) SupportsIPv6() bool { return s.Stack.CheckNetworkProtocol(ipv6.ProtocolNumber) From 82498d087ef8fa85ba4537406522a500a7b92a26 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 13 Jun 2022 18:11:30 -0700 Subject: [PATCH 05/29] Don't hold MM.activeMu when calling MM.vmaMapsEntryLocked(). Cloned from CL 446797388 by 'hg patch'. PiperOrigin-RevId: 454742145 --- pkg/sentry/mm/debug.go | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/pkg/sentry/mm/debug.go b/pkg/sentry/mm/debug.go index c273c982e..d927b1702 100644 --- a/pkg/sentry/mm/debug.go +++ b/pkg/sentry/mm/debug.go @@ -40,24 +40,22 @@ func (mm *MemoryManager) String() string { // DebugString returns a string containing information about mm for debugging. func (mm *MemoryManager) DebugString(ctx context.Context) string { + var b bytes.Buffer + mm.mappingMu.RLock() defer mm.mappingMu.RUnlock() - mm.activeMu.RLock() - defer mm.activeMu.RUnlock() - return mm.debugStringLocked(ctx) -} - -// Preconditions: mm.mappingMu and mm.activeMu must be locked. -func (mm *MemoryManager) debugStringLocked(ctx context.Context) string { - var b bytes.Buffer b.WriteString("VMAs:\n") for vseg := mm.vmas.FirstSegment(); vseg.Ok(); vseg = vseg.NextSegment() { b.Write(mm.vmaMapsEntryLocked(ctx, vseg)) } + + mm.activeMu.RLock() + defer mm.activeMu.RUnlock() b.WriteString("PMAs:\n") for pseg := mm.pmas.FirstSegment(); pseg.Ok(); pseg = pseg.NextSegment() { b.Write(pseg.debugStringEntryLocked()) } + return string(b.Bytes()) } From fa7c3d9c5142a3b9e05bf64a2e60624e5e6ff5cf Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Tue, 14 Jun 2022 10:32:49 -0700 Subject: [PATCH 06/29] Support specifying local IPv6 adddress/interface ...for outgoing packets. This is the first step in supporting IPV6_PKTINFO in the send path. Note that the implementation enforces strong host semantics as gVisor currently follows the strong host model. This change does not support IPV6_PKTINFO in the send path from Linux applications (via CMSGS) yet as there is no need. The bindings to enable the IPV6_PKTINFO CMSG in the sendpath can be done in a followup. Bug: https://fxbug.dev/102222 PiperOrigin-RevId: 454898862 --- pkg/tcpip/tcpip.go | 6 + pkg/tcpip/transport/BUILD | 2 + pkg/tcpip/transport/datagram_test.go | 450 ++++++++++++++++++ .../transport/internal/network/endpoint.go | 129 +++-- 4 files changed, 560 insertions(+), 27 deletions(-) diff --git a/pkg/tcpip/tcpip.go b/pkg/tcpip/tcpip.go index 81013e522..dd3236add 100644 --- a/pkg/tcpip/tcpip.go +++ b/pkg/tcpip/tcpip.go @@ -436,6 +436,12 @@ type SendableControlMessages struct { // HopLimit is the IPv6 Hop Limit of the associated packet. HopLimit uint8 + + // HasIPv6PacketInfo indicates whether IPv6PacketInfo is set. + HasIPv6PacketInfo bool + + // IPv6PacketInfo holds interface and address data on an incoming packet. + IPv6PacketInfo IPv6PacketInfo } // ReceivableControlMessages contains socket control messages that can be diff --git a/pkg/tcpip/transport/BUILD b/pkg/tcpip/transport/BUILD index 889317964..33e6557b5 100644 --- a/pkg/tcpip/transport/BUILD +++ b/pkg/tcpip/transport/BUILD @@ -19,7 +19,9 @@ go_test( deps = [ ":transport", "//pkg/tcpip", + "//pkg/tcpip/checker", "//pkg/tcpip/header", + "//pkg/tcpip/link/channel", "//pkg/tcpip/link/loopback", "//pkg/tcpip/network/ipv4", "//pkg/tcpip/network/ipv6", diff --git a/pkg/tcpip/transport/datagram_test.go b/pkg/tcpip/transport/datagram_test.go index b80912102..9618b7447 100644 --- a/pkg/tcpip/transport/datagram_test.go +++ b/pkg/tcpip/transport/datagram_test.go @@ -23,7 +23,9 @@ import ( "github.com/google/go-cmp/cmp" "gvisor.dev/gvisor/pkg/tcpip" + "gvisor.dev/gvisor/pkg/tcpip/checker" "gvisor.dev/gvisor/pkg/tcpip/header" + "gvisor.dev/gvisor/pkg/tcpip/link/channel" "gvisor.dev/gvisor/pkg/tcpip/link/loopback" "gvisor.dev/gvisor/pkg/tcpip/network/ipv4" "gvisor.dev/gvisor/pkg/tcpip/network/ipv6" @@ -664,3 +666,451 @@ func TestMulticastLoop(t *testing.T) { }) } } + +func TestIPv6PacketInfo(t *testing.T) { + const ( + nicID1 = 1 + nicID2 = 2 + port = 12345 + ) + + type localNICAddr struct { + nicID tcpip.NICID + addr tcpip.AddressWithPrefix + } + + type testCase struct { + name string + boundNICID tcpip.NICID + bindAddr tcpip.FullAddress + connectAddr tcpip.FullAddress + toAddr tcpip.FullAddress + pktInfo tcpip.IPv6PacketInfo + + expectedErr tcpip.Error + expectedLocalAddr tcpip.Address + expectedRemoteAddr tcpip.Address + } + + ipv6Addr1 := testutil.MustParse6("1::1") + ipv6Addr2 := testutil.MustParse6("1::2") + ipv6RemoteAddr1 := testutil.MustParse6("2::1") + ipv6RemoteAddr2 := testutil.MustParse6("2::2") + + localAddrs := []localNICAddr{ + { + nicID: nicID1, + addr: ipv6Addr1.WithPrefix(), + }, + { + nicID: nicID2, + addr: ipv6Addr2.WithPrefix(), + }, + } + + tests := []testCase{ + // Bind and SendTo + { + name: "Bind wildcard & SendTo with packet info NIC", + bindAddr: tcpip.FullAddress{ + Addr: "", + Port: port, + }, + toAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedLocalAddr: ipv6Addr1, + expectedRemoteAddr: ipv6RemoteAddr1, + }, + { + name: "BindToDevice & Bind wildcard & SendTo with packet info NIC not matching", + boundNICID: nicID2, + bindAddr: tcpip.FullAddress{ + Addr: "", + Port: port, + }, + toAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedErr: &tcpip.ErrNoRoute{}, + }, + { + name: "Bind wildcard and NIC & SendTo with packet info NIC matching", + bindAddr: tcpip.FullAddress{ + NIC: nicID1, + Addr: "", + Port: port, + }, + toAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedLocalAddr: ipv6Addr1, + expectedRemoteAddr: ipv6RemoteAddr1, + }, + { + name: "Bind wildcard and NIC & SendTo with packet info NIC not matching", + bindAddr: tcpip.FullAddress{ + NIC: nicID2, + Addr: "", + Port: port, + }, + toAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedErr: &tcpip.ErrNoRoute{}, + }, + { + name: "Bind specified & SendTo with packet info NIC not matching bound addr", + bindAddr: tcpip.FullAddress{ + Addr: ipv6Addr2, + Port: port, + }, + toAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedErr: &tcpip.ErrBadLocalAddress{}, + }, + { + name: "Bind specified and NIC & SendTo with packet info NIC not matching but local addr specified", + bindAddr: tcpip.FullAddress{ + NIC: nicID2, + Addr: ipv6Addr2, + Port: port, + }, + toAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + Addr: ipv6Addr1, + }, + expectedLocalAddr: ipv6Addr1, + expectedRemoteAddr: ipv6RemoteAddr1, + }, + + // Bind and Connect + { + name: "Bind wildcard & Connect then Send with packet info NIC", + bindAddr: tcpip.FullAddress{ + Addr: "", + Port: port, + }, + connectAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedLocalAddr: ipv6Addr1, + expectedRemoteAddr: ipv6RemoteAddr1, + }, + { + name: "Bind wildcard and NIC & Connect then Send with packet info NIC matching", + bindAddr: tcpip.FullAddress{ + NIC: nicID1, + Addr: "", + Port: port, + }, + connectAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedLocalAddr: ipv6Addr1, + expectedRemoteAddr: ipv6RemoteAddr1, + }, + { + name: "Bind wildcard and NIC & Connect then Send with packet info NIC not matching", + bindAddr: tcpip.FullAddress{ + NIC: nicID2, + Addr: "", + Port: port, + }, + connectAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedErr: &tcpip.ErrNoRoute{}, + }, + { + name: "Bind wildcard & Connect with NIC then Send with packet info NIC matching", + bindAddr: tcpip.FullAddress{ + Addr: "", + Port: port, + }, + connectAddr: tcpip.FullAddress{ + NIC: nicID1, + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedLocalAddr: ipv6Addr1, + expectedRemoteAddr: ipv6RemoteAddr1, + }, + { + name: "Bind wildcard & Connect with NIC then Send with packet info NIC not matching", + bindAddr: tcpip.FullAddress{ + Addr: "", + Port: port, + }, + connectAddr: tcpip.FullAddress{ + NIC: nicID2, + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedErr: &tcpip.ErrNoRoute{}, + }, + { + name: "Bind specified & Connect then Send with packet info NIC not matching but local addr specified", + bindAddr: tcpip.FullAddress{ + NIC: nicID2, + Addr: ipv6Addr2, + Port: port, + }, + connectAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + Addr: ipv6Addr1, + }, + expectedErr: &tcpip.ErrNoRoute{}, + }, + + // Connect + { + name: "Connect with NIC then Send with packet info NIC matching", + connectAddr: tcpip.FullAddress{ + NIC: nicID1, + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedLocalAddr: ipv6Addr1, + expectedRemoteAddr: ipv6RemoteAddr1, + }, + { + // Because NIC2 is preferred over NIC1 for route selection, we pick a + // local address on NIC2. Since the pktinfo does not specify a local + // address but requests the packet to be sent out through NIC1 we fail + // with err bad local address because NIC2's local address is not + // available on NIC1. + name: "Connect then Send with packet info NIC not matching", + connectAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedErr: &tcpip.ErrBadLocalAddress{}, + }, + { + name: "BindToDevice & Connect then Send with packet info NIC matching", + boundNICID: nicID2, + connectAddr: tcpip.FullAddress{ + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedErr: &tcpip.ErrNoRoute{}, + }, + { + name: "Connect then Send with packet info NIC not matching", + connectAddr: tcpip.FullAddress{ + NIC: nicID2, + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + NIC: nicID1, + }, + expectedErr: &tcpip.ErrNoRoute{}, + }, + + // Connect and SendTo + { + name: "Connect with NIC then SendTo with different NIC with packet info NIC matching SendTo NIC", + connectAddr: tcpip.FullAddress{ + NIC: nicID2, + Addr: ipv6RemoteAddr2, + Port: port, + }, + toAddr: tcpip.FullAddress{ + NIC: nicID1, + Addr: ipv6RemoteAddr1, + Port: port, + }, + pktInfo: tcpip.IPv6PacketInfo{ + Addr: ipv6Addr1, + NIC: nicID1, + }, + expectedLocalAddr: ipv6Addr1, + expectedRemoteAddr: ipv6RemoteAddr1, + }, + } + + for _, transProto := range []struct { + name string + createEndpoint func(*stack.Stack, *waiter.Queue) (tcpip.Endpoint, error) + }{ + { + name: "UDP", + createEndpoint: func(s *stack.Stack, wq *waiter.Queue) (tcpip.Endpoint, error) { + ep, err := s.NewEndpoint(udp.ProtocolNumber, header.IPv6ProtocolNumber, wq) + if err != nil { + return nil, fmt.Errorf("s.NewEndpoint(%d, %d, _) failed: %s", udp.ProtocolNumber, header.IPv6ProtocolNumber, err) + } + return ep, nil + }, + }, + { + name: "RAW", + createEndpoint: func(s *stack.Stack, wq *waiter.Queue) (tcpip.Endpoint, error) { + ep, err := s.NewRawEndpoint(udp.ProtocolNumber, header.IPv6ProtocolNumber, wq, true /* associated */) + if err != nil { + return nil, fmt.Errorf("s.NewRawEndpoint(%d, %d, _, true) failed: %s", udp.ProtocolNumber, header.IPv6ProtocolNumber, err) + } + return ep, nil + }, + }, + } { + t.Run(transProto.name, func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + s := stack.New(stack.Options{ + NetworkProtocols: []stack.NetworkProtocolFactory{ipv6.NewProtocol, ipv4.NewProtocol}, + TransportProtocols: []stack.TransportProtocolFactory{udp.NewProtocol}, + RawFactory: &raw.EndpointFactory{}, + }) + e1 := channel.New(1, header.IPv6MinimumMTU, "") + if err := s.CreateNIC(nicID1, e1); err != nil { + t.Fatalf("s.CreateNIC(%d, _) failed: %s", nicID1, err) + } + e2 := channel.New(1, header.IPv6MinimumMTU, "") + if err := s.CreateNIC(nicID2, e2); err != nil { + t.Fatalf("s.CreateNIC(%d, _) failed: %s", nicID2, err) + } + + for _, localAddr := range localAddrs { + addr := tcpip.ProtocolAddress{ + Protocol: header.IPv6ProtocolNumber, + AddressWithPrefix: localAddr.addr, + } + if err := s.AddProtocolAddress(localAddr.nicID, addr, stack.AddressProperties{}); err != nil { + t.Fatalf("AddProtocolAddress(%d, %#v, {}): %s", localAddr.nicID, addr, err) + } + } + s.SetRouteTable([]tcpip.Route{ + // NIC2 before NIC1 to let NIC2 have preference. + { + Destination: header.IPv6EmptySubnet, + NIC: nicID2, + }, + { + Destination: header.IPv6EmptySubnet, + NIC: nicID1, + }, + }) + + var wq waiter.Queue + ep, err := transProto.createEndpoint(s, &wq) + if err != nil { + t.Fatalf("transProto.createEndpoint(_) failed: %s", err) + } + defer ep.Close() + + if err := ep.SocketOptions().SetBindToDevice(int32(test.boundNICID)); err != nil { + t.Fatalf("ep.SocketOptions().SetBindToDevice(int32(%d)): %s", test.boundNICID, err) + } + + if test.bindAddr != (tcpip.FullAddress{}) { + if err := ep.Bind(test.bindAddr); err != nil { + t.Fatalf("ep.Bind(%#v): %s", test.bindAddr, err) + } + } + + if test.connectAddr != (tcpip.FullAddress{}) { + if err := ep.Connect(test.connectAddr); err != nil { + t.Fatalf("ep.Connect(%#v): %s", test.connectAddr, err) + } + } + + buf := [...]byte{1, 2, 3, 4} + var r bytes.Reader + r.Reset(buf[:]) + opts := tcpip.WriteOptions{ + ControlMessages: tcpip.SendableControlMessages{ + HasIPv6PacketInfo: true, + IPv6PacketInfo: test.pktInfo, + }, + } + if test.toAddr != (tcpip.FullAddress{}) { + opts.To = &test.toAddr + } + + if n, err := ep.Write(&r, opts); !cmp.Equal(test.expectedErr, err) { + t.Fatalf("got Write(_, %#v) = %s, want = %s", opts, err, test.expectedErr) + } else if test.expectedErr != nil { + return + } else if want := int64(len(buf)); n != want { + t.Fatalf("got Write(_, %#v) = %d, want = %d", opts, n, want) + } + + { + p := e1.Read() + if p == nil { + t.Fatal("packet didn't arrive at ep1") + } + + checker.IPv6(t, stack.PayloadSince(p.NetworkHeader()), + checker.SrcAddr(test.expectedLocalAddr), + checker.DstAddr(test.expectedRemoteAddr), + ) + } + + if p := e2.Read(); p != nil { + t.Errorf("unexpected packet from ep2 = %#v", p) + } + }) + } + }) + } +} diff --git a/pkg/tcpip/transport/internal/network/endpoint.go b/pkg/tcpip/transport/internal/network/endpoint.go index b70080b1c..724114ba0 100644 --- a/pkg/tcpip/transport/internal/network/endpoint.go +++ b/pkg/tcpip/transport/internal/network/endpoint.go @@ -392,40 +392,113 @@ func (e *Endpoint) AcquireContextForWrite(opts tcpip.WriteOptions) (WriteContext return WriteContext{}, &tcpip.ErrClosedForSend{} } + ipv6PktInfoValid := e.effectiveNetProto == header.IPv6ProtocolNumber && opts.ControlMessages.HasIPv6PacketInfo + route := e.connectedRoute - if opts.To == nil { + to := opts.To + info := e.Info() + switch { + case to == nil: // If the user doesn't specify a destination, they should have // connected to another address. if e.State() != transport.DatagramEndpointStateConnected { return WriteContext{}, &tcpip.ErrDestinationRequired{} } - route.Acquire() - } else { + if !ipv6PktInfoValid { + route.Acquire() + break + } + + // We are connected and the caller did not specify the destination but + // we have an IPv6 packet info structure which may change our local + // interface/address used to send the packet so we need to construct + // a new route instead of using the connected route. + // + // Contruct a destination matching the remote the endpoint is connected + // to. + to = &tcpip.FullAddress{ + // RegisterNICID is set when the endpoint is connected. It is usually + // only set for link-local addresses or multicast addresses if the + // multicast interface was specified (see e.multicastNICID, + // e.connectRouteRLocked and e.ConnectAndThen). + NIC: info.RegisterNICID, + Addr: info.ID.RemoteAddress, + } + fallthrough + default: // Reject destination address if it goes through a different // NIC than the endpoint was bound to. - nicID := opts.To.NIC + nicID := to.NIC if nicID == 0 { nicID = tcpip.NICID(e.ops.GetBindToDevice()) } - info := e.Info() - if info.BindNICID != 0 { - if nicID != 0 && nicID != info.BindNICID { - return WriteContext{}, &tcpip.ErrNoRoute{} + + var localAddr tcpip.Address + if ipv6PktInfoValid { + // Uphold strong-host semantics since (as of writing) the stack follows + // the strong host model. + + pktInfoNICID := opts.ControlMessages.IPv6PacketInfo.NIC + pktInfoAddr := opts.ControlMessages.IPv6PacketInfo.Addr + + if pktInfoNICID != 0 { + // If we are bound to an interface or specified the destination + // interface (usually when using link-local addresses), make sure the + // interface matches the specified local interface. + if nicID != 0 && nicID != pktInfoNICID { + return WriteContext{}, &tcpip.ErrNoRoute{} + } + + // If a local address is not specified, then we need to make sure the + // bound address belongs to the specified local interface. + if len(pktInfoAddr) == 0 { + // If the bound interface is different from the specified local + // interface, the bound address obviously does not belong to the + // specified local interface. + // + // The bound interface is usually only set for link-local addresses. + if info.BindNICID != 0 && info.BindNICID != pktInfoNICID { + return WriteContext{}, &tcpip.ErrNoRoute{} + } + if len(info.ID.LocalAddress) != 0 && e.stack.CheckLocalAddress(pktInfoNICID, header.IPv6ProtocolNumber, info.ID.LocalAddress) == 0 { + return WriteContext{}, &tcpip.ErrBadLocalAddress{} + } + } + + nicID = pktInfoNICID } - nicID = info.BindNICID - } - if nicID == 0 { - nicID = info.RegisterNICID + if len(pktInfoAddr) != 0 { + // The local address must belong to the stack. If an outgoing interface + // is specified as a result of binding the endpoint to a device, or + // specifying the outgoing interface in the destination address/pkt info + // structure, the address must belong to that interface. + if e.stack.CheckLocalAddress(nicID, header.IPv6ProtocolNumber, pktInfoAddr) == 0 { + return WriteContext{}, &tcpip.ErrBadLocalAddress{} + } + + localAddr = pktInfoAddr + } + } else { + if info.BindNICID != 0 { + if nicID != 0 && nicID != info.BindNICID { + return WriteContext{}, &tcpip.ErrNoRoute{} + } + + nicID = info.BindNICID + } + if nicID == 0 { + nicID = info.RegisterNICID + } } - dst, netProto, err := e.checkV4Mapped(*opts.To) + dst, netProto, err := e.checkV4Mapped(*to) if err != nil { return WriteContext{}, err } - route, _, err = e.connectRouteRLocked(nicID, dst, netProto) + route, _, err = e.connectRouteRLocked(nicID, localAddr, dst, netProto) if err != nil { return WriteContext{}, err } @@ -496,19 +569,21 @@ func (e *Endpoint) Disconnect() { // specified address is a multicast address. // // +checklocksread:e.mu -func (e *Endpoint) connectRouteRLocked(nicID tcpip.NICID, addr tcpip.FullAddress, netProto tcpip.NetworkProtocolNumber) (*stack.Route, tcpip.NICID, tcpip.Error) { - localAddr := e.Info().ID.LocalAddress - if e.isBroadcastOrMulticast(nicID, netProto, localAddr) { - // A packet can only originate from a unicast address (i.e., an interface). - localAddr = "" - } - - if header.IsV4MulticastAddress(addr.Addr) || header.IsV6MulticastAddress(addr.Addr) { - if nicID == 0 { - nicID = e.multicastNICID +func (e *Endpoint) connectRouteRLocked(nicID tcpip.NICID, localAddr tcpip.Address, addr tcpip.FullAddress, netProto tcpip.NetworkProtocolNumber) (*stack.Route, tcpip.NICID, tcpip.Error) { + if len(localAddr) == 0 { + localAddr = e.Info().ID.LocalAddress + if e.isBroadcastOrMulticast(nicID, netProto, localAddr) { + // A packet can only originate from a unicast address (i.e., an interface). + localAddr = "" } - if localAddr == "" && nicID == 0 { - localAddr = e.multicastAddr + + if header.IsV4MulticastAddress(addr.Addr) || header.IsV6MulticastAddress(addr.Addr) { + if nicID == 0 { + nicID = e.multicastNICID + } + if localAddr == "" && nicID == 0 { + localAddr = e.multicastAddr + } } } @@ -563,7 +638,7 @@ func (e *Endpoint) ConnectAndThen(addr tcpip.FullAddress, f func(netProto tcpip. return err } - r, nicID, err := e.connectRouteRLocked(nicID, addr, netProto) + r, nicID, err := e.connectRouteRLocked(nicID, "", addr, netProto) if err != nil { return err } From 758a425bbc2e40cf789c521024533574ca1770e5 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Tue, 14 Jun 2022 11:58:26 -0700 Subject: [PATCH 07/29] shrink DataTransferStressTest PiperOrigin-RevId: 454921228 --- test/syscalls/linux/socket_generic_stress.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/syscalls/linux/socket_generic_stress.cc b/test/syscalls/linux/socket_generic_stress.cc index c08e4d99e..51901a3a0 100644 --- a/test/syscalls/linux/socket_generic_stress.cc +++ b/test/syscalls/linux/socket_generic_stress.cc @@ -193,9 +193,6 @@ INSTANTIATE_TEST_SUITE_P( using DataTransferStressTest = SocketPairTest; TEST_P(DataTransferStressTest, BigDataTransfer) { - // TODO(b/165912341): These are too slow on KVM platform with nested virt. - SKIP_IF(GvisorPlatform() == Platform::kKVM); - const std::unique_ptr sockets = ASSERT_NO_ERRNO_AND_VALUE(NewSocketPair()); int client_fd = sockets->first_fd(); @@ -218,9 +215,12 @@ TEST_P(DataTransferStressTest, BigDataTransfer) { ASSERT_THAT(shutdown(server_fd, SHUT_WR), SyscallSucceeds()); }); + // Tests can be prohibitively slow on the KVM platform with nested virt. + const int kShift = GvisorPlatform() == Platform::kKVM ? 10 : 20; + const std::string chunk = "Though this upload be but little, it is fierce."; std::string big_string; - while (big_string.size() < 31 << 20) { + while (big_string.size() < 31 << kShift) { big_string += chunk; } absl::string_view data = big_string; @@ -236,7 +236,7 @@ TEST_P(DataTransferStressTest, BigDataTransfer) { }); std::string buf; - buf.resize(1 << 20); + buf.resize(1 << kShift); while (!data.empty()) { ssize_t n = read(client_fd, buf.data(), buf.size()); ASSERT_GE(n, 0); From ebadfe7702fe745bf451c5538be72eacfc7f397c Mon Sep 17 00:00:00 2001 From: Ghanan Gowripalan Date: Tue, 14 Jun 2022 12:03:43 -0700 Subject: [PATCH 08/29] Allow IPv6 only stack to use UDP endpoints Check if stack supports IPv4 before registering an IPv6 endpoint as dual-stack. PiperOrigin-RevId: 454922614 --- pkg/tcpip/transport/datagram_test.go | 2 +- pkg/tcpip/transport/udp/endpoint.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/tcpip/transport/datagram_test.go b/pkg/tcpip/transport/datagram_test.go index 9618b7447..bbcf8cf74 100644 --- a/pkg/tcpip/transport/datagram_test.go +++ b/pkg/tcpip/transport/datagram_test.go @@ -1016,7 +1016,7 @@ func TestIPv6PacketInfo(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { s := stack.New(stack.Options{ - NetworkProtocols: []stack.NetworkProtocolFactory{ipv6.NewProtocol, ipv4.NewProtocol}, + NetworkProtocols: []stack.NetworkProtocolFactory{ipv6.NewProtocol}, TransportProtocols: []stack.TransportProtocolFactory{udp.NewProtocol}, RawFactory: &raw.EndpointFactory{}, }) diff --git a/pkg/tcpip/transport/udp/endpoint.go b/pkg/tcpip/transport/udp/endpoint.go index e6e45b8c0..d71c67c79 100644 --- a/pkg/tcpip/transport/udp/endpoint.go +++ b/pkg/tcpip/transport/udp/endpoint.go @@ -653,7 +653,7 @@ func (e *endpoint) Connect(addr tcpip.FullAddress) tcpip.Error { // packets on a different network protocol, so we register both even if // v6only is set to false and this is an ipv6 endpoint. netProtos := []tcpip.NetworkProtocolNumber{netProto} - if netProto == header.IPv6ProtocolNumber && !e.ops.GetV6Only() { + if netProto == header.IPv6ProtocolNumber && !e.ops.GetV6Only() && e.stack.CheckNetworkProtocol(header.IPv4ProtocolNumber) { netProtos = []tcpip.NetworkProtocolNumber{ header.IPv4ProtocolNumber, header.IPv6ProtocolNumber, @@ -790,7 +790,7 @@ func (e *endpoint) bindLocked(addr tcpip.FullAddress) tcpip.Error { // wildcard (empty) address, and this is an IPv6 endpoint with v6only // set to false. netProtos := []tcpip.NetworkProtocolNumber{boundNetProto} - if boundNetProto == header.IPv6ProtocolNumber && !e.ops.GetV6Only() && boundAddr == "" { + if boundNetProto == header.IPv6ProtocolNumber && !e.ops.GetV6Only() && boundAddr == "" && e.stack.CheckNetworkProtocol(header.IPv4ProtocolNumber) { netProtos = []tcpip.NetworkProtocolNumber{ header.IPv6ProtocolNumber, header.IPv4ProtocolNumber, From 6e662d0262c7827ab6fff7373e563f2c1bab45a1 Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Tue, 14 Jun 2022 14:45:20 -0700 Subject: [PATCH 09/29] buildkite: move "Build everything" upper in the list It runs for a long time and so it would be good to run it earlier. PiperOrigin-RevId: 454960320 --- .buildkite/pipeline.yaml | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/.buildkite/pipeline.yaml b/.buildkite/pipeline.yaml index d829c2efa..1cc0481dd 100644 --- a/.buildkite/pipeline.yaml +++ b/.buildkite/pipeline.yaml @@ -59,6 +59,13 @@ steps: label: ":fire: Smoke race tests" command: make smoke-race-tests + # Build everything. + - <<: *common + label: ":world_map: Build everything" + command: "make build OPTIONS=--build_tag_filters=-nogo TARGETS=//..." + agents: + arch: "amd64" + # Check that the Go branch builds. This is not technically required, as this build is maintained # as a GitHub action in order to preserve this maintaince across forks. However, providing the # action here may provide easier debuggability and diagnosis on failure. @@ -364,13 +371,6 @@ steps: arch: "amd64" os: "ubuntu" - # Build everything. - - <<: *common - label: ":world_map: Build everything" - command: "make build OPTIONS=--build_tag_filters=-nogo TARGETS=//..." - agents: - arch: "amd64" - # Run basic benchmarks smoke tests (no upload). - <<: *common label: ":fire: Benchmarks smoke test" From 9a78b8267c7a3d1544d36d16e8ca6cbfeeb607fd Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Tue, 14 Jun 2022 18:19:27 -0700 Subject: [PATCH 10/29] Add new buffer implementation. PiperOrigin-RevId: 455003647 --- pkg/bufferv2/BUILD | 65 +++ pkg/bufferv2/buffer.go | 608 +++++++++++++++++++++++++++ pkg/bufferv2/buffer_state.go | 26 ++ pkg/bufferv2/buffer_test.go | 792 +++++++++++++++++++++++++++++++++++ pkg/bufferv2/chunk.go | 113 +++++ pkg/bufferv2/view.go | 232 ++++++++++ pkg/bufferv2/view_test.go | 174 ++++++++ pkg/bufferv2/view_unsafe.go | 32 ++ 8 files changed, 2042 insertions(+) create mode 100644 pkg/bufferv2/BUILD create mode 100644 pkg/bufferv2/buffer.go create mode 100644 pkg/bufferv2/buffer_state.go create mode 100644 pkg/bufferv2/buffer_test.go create mode 100644 pkg/bufferv2/chunk.go create mode 100644 pkg/bufferv2/view.go create mode 100644 pkg/bufferv2/view_test.go create mode 100644 pkg/bufferv2/view_unsafe.go diff --git a/pkg/bufferv2/BUILD b/pkg/bufferv2/BUILD new file mode 100644 index 000000000..632e89584 --- /dev/null +++ b/pkg/bufferv2/BUILD @@ -0,0 +1,65 @@ +load("//tools:defs.bzl", "go_library", "go_test") +load("//tools/go_generics:defs.bzl", "go_template_instance") + +package(licenses = ["notice"]) + +go_template_instance( + name = "chunk_refs", + out = "chunk_refs.go", + package = "buffer", + prefix = "chunk", + template = "//pkg/refsvfs2:refs_template", + types = { + "T": "chunk", + }, +) + +go_template_instance( + name = "view_list", + out = "view_list.go", + package = "buffer", + prefix = "view", + template = "//pkg/ilist:generic_list", + types = { + "Element": "*View", + "Linker": "*View", + }, +) + +go_library( + name = "buffer", + srcs = [ + "buffer.go", + "buffer_state.go", + "chunk.go", + "chunk_refs.go", + "view.go", + "view_list.go", + "view_unsafe.go", + ], + visibility = ["//visibility:public"], + deps = [ + "//pkg/atomicbitops", + "//pkg/bits", + "//pkg/context", + "//pkg/ilist", + "//pkg/log", + "//pkg/pool", + "//pkg/refsvfs2", + "//pkg/sync", + ], +) + +go_test( + name = "buffer_test", + size = "small", + srcs = [ + "buffer_test.go", + "view_test.go", + ], + library = ":buffer", + deps = [ + "//pkg/state", + "@com_github_google_go_cmp//cmp:go_default_library", + ], +) diff --git a/pkg/bufferv2/buffer.go b/pkg/bufferv2/buffer.go new file mode 100644 index 000000000..ea961ab18 --- /dev/null +++ b/pkg/bufferv2/buffer.go @@ -0,0 +1,608 @@ +// 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 buffer provides the implementation of a non-contiguous buffer. +// +// A buffer is an flexible buffer, supporting the safecopy operations natively +// as well as the ability to grow via either prepend or append, as well as shrink. +package buffer + +import ( + "fmt" + "io" +) + +// Buffer is a non-linear buffer. +// +// +stateify savable +type Buffer struct { + data viewList `state:".([]byte)"` + size int64 +} + +func (b *Buffer) removeView(v *View) { + b.data.Remove(v) + v.Release() +} + +// MakeWithData creates a new Buffer initialized with given data. This function +// should be used with caution to avoid unnecessary []byte allocations. When in +// doubt use NewWithView to maximize chunk reuse. +func MakeWithData(b []byte) Buffer { + buf := Buffer{} + if len(b) == 0 { + return buf + } + v := NewViewWithData(b) + buf.Append(v) + return buf +} + +// MakeWithView creates a new Buffer initialized with given view. This function +// takes ownership of v. +func MakeWithView(v *View) Buffer { + if v == nil { + return Buffer{} + } + b := Buffer{ + size: int64(v.Size()), + } + if b.size == 0 { + v.Release() + return b + } + b.data.PushBack(v) + return b +} + +// Release frees all resources held by b. +func (b *Buffer) Release() { + for v := b.data.Front(); v != nil; v = b.data.Front() { + b.removeView(v) + } + b.size = 0 +} + +// TrimFront removes the first count bytes from the buffer. +func (b *Buffer) TrimFront(count int64) { + if count >= b.size { + b.advanceRead(b.size) + } else { + b.advanceRead(count) + } +} + +// ReadAt implements io.ReaderAt.ReadAt. +func (b *Buffer) ReadAt(p []byte, offset int64) (int, error) { + var ( + skipped int64 + done int64 + ) + for v := b.data.Front(); v != nil && done < int64(len(p)); v = v.Next() { + needToSkip := int(offset - skipped) + if sz := v.Size(); sz <= needToSkip { + skipped += int64(sz) + continue + } + + // Actually read data. + n := copy(p[done:], v.AsSlice()[needToSkip:]) + skipped += int64(needToSkip) + done += int64(n) + } + if int(done) < len(p) || offset+done == b.size { + return int(done), io.EOF + } + return int(done), nil +} + +// advanceRead advances the Buffer's read index. +// +// Precondition: there must be sufficient bytes in the buffer. +func (b *Buffer) advanceRead(count int64) { + for v := b.data.Front(); v != nil && count > 0; { + sz := int64(v.Size()) + if sz > count { + // There is still data for reading. + v.TrimFront(int(count)) + b.size -= count + count = 0 + return + } + + // Consume the whole view. + oldView := v + v = v.Next() // Iterate. + b.removeView(oldView) + + // Update counts. + count -= sz + b.size -= sz + } + if count > 0 { + panic(fmt.Sprintf("advanceRead still has %d bytes remaining", count)) + } +} + +// Truncate truncates the Buffer to the given length. +// +// This will not grow the Buffer, only shrink it. If a length is passed that is +// greater than the current size of the Buffer, then nothing will happen. +// +// Precondition: length must be >= 0. +func (b *Buffer) Truncate(length int64) { + if length < 0 { + panic("negative length provided") + } + if length >= b.size { + return // Nothing to do. + } + for v := b.data.Back(); v != nil && b.size > length; v = b.data.Back() { + sz := int64(v.Size()) + if after := b.size - sz; after < length { + // Truncate the buffer locally. + left := (length - after) + v.write = v.read + int(left) + b.size = length + break + } + + // Drop the buffer completely; see above. + b.removeView(v) + b.size -= sz + } +} + +// GrowTo grows the given Buffer to the number of bytes, which will be appended. +// If zero is true, all these bytes will be zero. If zero is false, then this is +// the caller's responsibility. +// +// Precondition: length must be >= 0. +func (b *Buffer) GrowTo(length int64, zero bool) { + if length < 0 { + panic("negative length provided") + } + for b.size < length { + v := b.data.Back() + + // Is there some space in the last buffer? + if v.Full() { + v = NewView(int(length - b.size)) + b.data.PushBack(v) + } + + // Write up to length bytes. + sz := v.AvailableSize() + if int64(sz) > length-b.size { + sz = int(length - b.size) + } + + // Zero the written section; note that this pattern is + // specifically recognized and optimized by the compiler. + if zero { + for i := v.write; i < v.write+sz; i++ { + v.chunk.data[i] = 0 + } + } + + // Advance the index. + v.Grow(sz) + b.size += int64(sz) + } +} + +// Prepend prepends the given data. Prepend takes ownership of src. +func (b *Buffer) Prepend(src *View) error { + if src == nil { + return nil + } + // If the first buffer does not have room just prepend the view. + v := b.data.Front() + if v == nil || v.read == 0 { + b.prependOwned(src) + return nil + } + + // If there's room at the front and we won't incur a copy by writing to this + // view, fill in the extra room first. + if !v.sharesChunk() { + avail := v.read + vStart := 0 + srcStart := src.Size() - avail + if avail > src.Size() { + vStart = avail - src.Size() + srcStart = 0 + } + // Save the write index and restore it after. + old := v.write + v.read = vStart + n, err := v.WriteAt(src.AsSlice()[srcStart:], 0) + if err != nil { + return fmt.Errorf("could not write to view during append: %w", err) + } + b.size += int64(n) + v.write = old + src.write = srcStart + + // If there's no more to be written, then we're done. + if src.Size() == 0 { + src.Release() + return nil + } + } + + // Otherwise, just prepend the view. + b.prependOwned(src) + return nil +} + +// Append appends the given data. Append takes ownership of src. +func (b *Buffer) Append(src *View) error { + if src == nil { + return nil + } + // If the last buffer is full, just append the view. + v := b.data.Back() + if v.Full() { + b.appendOwned(src) + return nil + } + + // If a write won't incur a copy, then fill the back of the existing last + // chunk. + if !v.sharesChunk() { + writeSz := src.Size() + if src.Size() > v.AvailableSize() { + writeSz = v.AvailableSize() + } + done, err := v.Write(src.AsSlice()[:writeSz]) + if err != nil { + return fmt.Errorf("could not write to view during append: %w", err) + } + src.TrimFront(done) + b.size += int64(done) + if src.Size() == 0 { + src.Release() + return nil + } + } + + // If there is still data left just append the src. + b.appendOwned(src) + return nil +} + +func (b *Buffer) appendOwned(v *View) { + b.data.PushBack(v) + b.size += int64(v.Size()) +} + +func (b *Buffer) prependOwned(v *View) { + b.data.PushFront(v) + b.size += int64(v.Size()) +} + +// PullUp makes the specified range contiguous and returns the backing memory. +func (b *Buffer) PullUp(offset, length int) (*View, bool) { + if length == 0 { + return nil, true + } + tgt := Range{begin: offset, end: offset + length} + if tgt.Intersect(Range{end: int(b.size)}).Len() != length { + return nil, false + } + + curr := Range{} + v := b.data.Front() + for ; v != nil; v = v.Next() { + origLen := v.Size() + curr.end = curr.begin + origLen + + if x := curr.Intersect(tgt); x.Len() == tgt.Len() { + // buf covers the whole requested target range. + sub := x.Offset(-curr.begin) + new := viewPool.Get().(*View) + new.read = sub.begin + new.write = sub.end + // Don't increment the reference count of the underlying chunk. Views + // returned by PullUp are explicitly unowned and read only + new.chunk = v.chunk + return new, true + } else if x.Len() > 0 { + // buf is pointing at the starting buffer we want to merge. + break + } + + curr.begin += origLen + } + + // Calculate the total merged length. + totLen := 0 + for n := v; n != nil; n = n.Next() { + totLen += n.Size() + if curr.begin+totLen >= tgt.end { + break + } + } + + // Merge the buffers. + merged := NewViewSize(totLen) + off := 0 + for n := v; n != nil && off < totLen; { + merged.WriteAt(n.AsSlice(), off) + off += n.Size() + + // Remove buffers except for the first one, which will be reused. + if n == v { + n = n.Next() + } else { + old := n + n = n.Next() + b.removeView(old) + } + } + // Make data the first buffer. + b.data.InsertBefore(v, merged) + b.removeView(v) + + r := tgt.Offset(-curr.begin) + pulled := viewPool.Get().(*View) + pulled.read = r.begin + pulled.write = r.end + pulled.chunk = merged.chunk + return pulled, true +} + +// Flatten returns a flattened copy of this data. +// +// This method should not be used in any performance-sensitive paths. It may +// allocate a fresh byte slice sufficiently large to contain all the data in +// the buffer. This is principally for debugging. +// +// N.B. Tee data still belongs to this Buffer, as if there is a single buffer +// present, then it will be returned directly. This should be used for +// temporary use only, and a reference to the given slice should not be held. +func (b *Buffer) Flatten() []byte { + if v := b.data.Front(); v == nil { + return nil // No data at all. + } else if v.Next() == nil { + return v.AsSlice() // Only one buffer. + } + data := make([]byte, 0, b.size) // Need to flatten. + for v := b.data.Front(); v != nil; v = v.Next() { + // Copy to the allocated slice. + data = append(data, v.AsSlice()...) + } + return data +} + +// Size indicates the total amount of data available in this Buffer. +func (b *Buffer) Size() int64 { + return b.size +} + +// Clone creates a copy-on-write clone of b. The underlying chunks are shared +// until they are written to. +func (b *Buffer) Clone() Buffer { + other := Buffer{ + size: b.size, + } + for v := b.data.Front(); v != nil; v = v.Next() { + newView := v.Clone() + other.data.PushBack(newView) + } + return other +} + +// Apply applies the given function across all valid data. +func (b *Buffer) Apply(fn func(*View)) { + for v := b.data.Front(); v != nil; v = v.Next() { + d := v.Clone() + fn(d) + d.Release() + } +} + +// SubApply applies fn to a given range of data in b. Any part of the range +// outside of b is ignored. +func (b *Buffer) SubApply(offset, length int, fn func(*View)) { + for v := b.data.Front(); length > 0 && v != nil; v = v.Next() { + d := v.Clone() + if offset >= d.Size() { + offset -= d.Size() + continue + } + if offset > 0 { + d.TrimFront(offset) + offset = 0 + } + if length < d.Size() { + d.write = d.read + length + } + fn(d) + length -= d.Size() + d.Release() + } +} + +// Merge merges the provided Buffer with this one. +// +// The other Buffer will be appended to v, and other will be empty after this +// operation completes. +func (b *Buffer) Merge(other *Buffer) { + // Copy over all buffers. + for v := other.data.Front(); v != nil; v = other.data.Front() { + b.Append(v.Clone()) + other.removeView(v) + } + + // Adjust sizes. + other.size = 0 +} + +// WriteFromReader writes to the buffer from an io.Reader. +// +// A minimum read size equal to unsafe.Sizeof(unintptr) is enforced, +// provided that count is greater than or equal to unsafe.Sizeof(uintptr). +func (b *Buffer) WriteFromReader(r io.Reader, count int64) (int64, error) { + var ( + done int64 + n int + err error + ) + for done < count { + view := b.data.Back() + + // Ensure we have an empty buffer. + if view.Full() { + view = NewView(int(count - done)) + b.data.PushBack(view) + } + + // Is this less than the minimum batch? + if view.AvailableSize() < minBatch && (count-done) >= int64(minBatch) { + tmp := NewView(minBatch) + n, err = r.Read(tmp.availableSlice()) + tmp.Grow(n) + b.Append(tmp) + done += int64(n) + if err != nil { + break + } + continue + } + + // Limit the read, if necessary. + sz := view.AvailableSize() + if left := count - done; int64(sz) > left { + sz = int(left) + } + + // Pass the relevant portion of the buffer. + n, err = r.Read(view.availableSlice()[:sz]) + view.Grow(n) + done += int64(n) + b.size += int64(n) + if err == io.EOF { + err = nil // Short write allowed. + break + } else if err != nil { + break + } + } + return done, err +} + +// ReadToWriter reads from the buffer into an io.Writer. +// +// N.B. This does not consume the bytes read. TrimFront should +// be called appropriately after this call in order to do so. +// +// A minimum write size equal to unsafe.Sizeof(unintptr) is enforced, +// provided that count is greater than or equal to unsafe.Sizeof(uintptr). +func (b *Buffer) ReadToWriter(w io.Writer, count int64) (int64, error) { + var ( + done int64 + n int + err error + ) + offset := 0 // Spill-over for batching. + for view := b.data.Front(); view != nil && done < count; view = view.Next() { + // Has this been consumed? Skip it. + sz := view.Size() + if sz <= offset { + offset -= sz + continue + } + sz -= offset + + // Is this less than the minimum batch? + left := count - done + if sz < minBatch && left >= int64(minBatch) && (b.size-done) >= int64(minBatch) { + tmp := NewView(minBatch) + n, err = b.ReadAt(tmp.availableSlice()[:minBatch], done) + tmp.Grow(n) + w.Write(tmp.AsSlice()) + tmp.Release() + done += int64(n) + offset = n - sz // Reset below. + if err != nil { + break + } + continue + } + + // Limit the write if necessary. + if int64(sz) >= left { + sz = int(left) + } + + // Perform the actual write. + n, err = w.Write(view.AsSlice()[offset : offset+sz]) + done += int64(n) + if err != nil { + break + } + + // Reset spill-over. + offset = 0 + } + return done, err +} + +// AsSlices returns a list of each of Buffer's underlying Views as Slices. +// The slices returned should not be modifed. +func (b *Buffer) AsSlices() [][]byte { + slices := make([][]byte, 0, b.data.Len()) + for v := b.data.Front(); v != nil; v = v.Next() { + slices = append(slices, v.AsSlice()) + } + return slices +} + +// Range specifies a range of buffer. +type Range struct { + begin int + end int +} + +// Intersect returns the intersection of x and y. +func (x Range) Intersect(y Range) Range { + if x.begin < y.begin { + x.begin = y.begin + } + if x.end > y.end { + x.end = y.end + } + if x.begin >= x.end { + return Range{} + } + return x +} + +// Offset returns x offset by off. +func (x Range) Offset(off int) Range { + x.begin += off + x.end += off + return x +} + +// Len returns the length of x. +func (x Range) Len() int { + l := x.end - x.begin + if l < 0 { + l = 0 + } + return l +} diff --git a/pkg/bufferv2/buffer_state.go b/pkg/bufferv2/buffer_state.go new file mode 100644 index 000000000..447dfe389 --- /dev/null +++ b/pkg/bufferv2/buffer_state.go @@ -0,0 +1,26 @@ +// 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 buffer + +// saveBuf is invoked by stateify. +func (b *Buffer) saveData() []byte { + return b.Flatten() +} + +// loadBuf is invoked by stateify. +func (b *Buffer) loadData(data []byte) { + v := NewViewWithData(data) + b.Append(v) +} diff --git a/pkg/bufferv2/buffer_test.go b/pkg/bufferv2/buffer_test.go new file mode 100644 index 000000000..a5a0a2ea1 --- /dev/null +++ b/pkg/bufferv2/buffer_test.go @@ -0,0 +1,792 @@ +// Copyright 2021 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 buffer + +import ( + "bytes" + "context" + "fmt" + "io" + "reflect" + "strings" + "testing" + + "gvisor.dev/gvisor/pkg/state" +) + +func BenchmarkReadAt(b *testing.B) { + b.ReportAllocs() + var buf Buffer + buf.Append(NewView(100)) + defer buf.Release() + + bytes := make([]byte, 10) + for i := 0; i < b.N; i++ { + buf.ReadAt(bytes, 0) + } +} + +func BenchmarkWriteRead(b *testing.B) { + b.ReportAllocs() + var buf Buffer + defer buf.Release() + sz := 1000 + rbuf := bytes.NewBuffer(make([]byte, sz)) + for i := 0; i < b.N; i++ { + buf.Append(NewView(sz)) + rbuf.Reset() + buf.ReadToWriter(rbuf, int64(sz)) + } +} + +func fillAppend(b *Buffer, data []byte) { + b.Append(NewViewWithData(data)) +} + +func fillAppendEnd(b *Buffer, data []byte) { + b.GrowTo(baseChunkSize-1, false) + b.Append(NewViewWithData(data)) + b.TrimFront(baseChunkSize - 1) +} + +func fillWriteFromReader(b *Buffer, data []byte) { + buf := bytes.NewBuffer(data) + b.WriteFromReader(buf, int64(len(data))) +} + +func fillWriteFromReaderEnd(b *Buffer, data []byte) { + b.GrowTo(baseChunkSize-1, false) + buf := bytes.NewBuffer(data) + b.WriteFromReader(buf, int64(len(data))) + b.TrimFront(baseChunkSize - 1) +} + +var fillFuncs = map[string]func(*Buffer, []byte){ + "append": fillAppend, + "appendEnd": fillAppendEnd, + "writeFromReader": fillWriteFromReader, + "writeFromReaderEnd": fillWriteFromReaderEnd, +} + +func testReadAt(t *testing.T, b *Buffer, offset int64, n int, wantStr string, wantErr error) { + t.Helper() + d := make([]byte, n) + n, err := b.ReadAt(d, offset) + if n != len(wantStr) { + t.Errorf("got %d, want %d", n, len(wantStr)) + } + if err != wantErr { + t.Errorf("got err %v, want %v", err, wantErr) + } + if !bytes.Equal(d[:n], []byte(wantStr)) { + t.Errorf("got %q, want %q", string(d[:n]), wantStr) + } +} + +func TestBuffer(t *testing.T) { + testCases := []struct { + name string + input string + output string + op func(*testing.T, *Buffer) + }{ + // Preconditions. + { + name: "truncate-check", + input: "hello", + output: "hello", // Not touched. + op: func(t *testing.T, b *Buffer) { + defer func() { + if r := recover(); r == nil { + t.Errorf("Truncate(-1) did not panic") + } + }() + b.Truncate(-1) + }, + }, + { + name: "growto-check", + input: "hello", + output: "hello", // Not touched. + op: func(t *testing.T, b *Buffer) { + defer func() { + if r := recover(); r == nil { + t.Errorf("GrowTo(-1) did not panic") + } + }() + b.GrowTo(-1, false) + }, + }, + { + name: "advance-check", + input: "hello", + output: "", // Consumed. + op: func(t *testing.T, b *Buffer) { + defer func() { + if r := recover(); r == nil { + t.Errorf("advanceRead(Size()+1) did not panic") + } + }() + b.advanceRead(b.Size() + 1) + }, + }, + + // Prepend. + { + name: "prepend", + input: "world", + output: "hello world", + op: func(t *testing.T, b *Buffer) { + b.Prepend(NewViewWithData([]byte("hello "))) + }, + }, + { + name: "prepend-backfill-full", + input: "hello world", + output: "jello world", + op: func(t *testing.T, b *Buffer) { + b.TrimFront(1) + b.Prepend(NewViewWithData([]byte("j"))) + }, + }, + { + name: "prepend-backfill-under", + input: "hello world", + output: "hola world", + op: func(t *testing.T, b *Buffer) { + b.TrimFront(5) + b.Prepend(NewViewWithData([]byte("hola"))) + }, + }, + { + name: "prepend-backfill-over", + input: "hello world", + output: "smello world", + op: func(t *testing.T, b *Buffer) { + b.TrimFront(1) + b.Prepend(NewViewWithData([]byte("sm"))) + }, + }, + { + name: "prepend-fill", + input: strings.Repeat("1", baseChunkSize-1), + output: "0" + strings.Repeat("1", baseChunkSize-1), + op: func(t *testing.T, b *Buffer) { + b.Prepend(NewViewWithData([]byte("0"))) + }, + }, + { + name: "prepend-overflow", + input: strings.Repeat("1", baseChunkSize), + output: "0" + strings.Repeat("1", baseChunkSize), + op: func(t *testing.T, b *Buffer) { + b.Prepend(NewViewWithData([]byte("0"))) + }, + }, + { + name: "prepend-multiple-buffers", + input: strings.Repeat("1", baseChunkSize-1), + output: strings.Repeat("0", baseChunkSize*3) + strings.Repeat("1", baseChunkSize-1), + op: func(t *testing.T, b *Buffer) { + b.Prepend(NewViewWithData([]byte(strings.Repeat("0", baseChunkSize)))) + b.Prepend(NewViewWithData([]byte(strings.Repeat("0", baseChunkSize)))) + b.Prepend(NewViewWithData([]byte(strings.Repeat("0", baseChunkSize)))) + }, + }, + + // Append and write. + { + name: "append", + input: "hello", + output: "hello world", + op: func(t *testing.T, b *Buffer) { + b.Append(NewViewWithData([]byte(" world"))) + }, + }, + { + name: "append-fill", + input: strings.Repeat("1", baseChunkSize-1), + output: strings.Repeat("1", baseChunkSize-1) + "0", + op: func(t *testing.T, b *Buffer) { + b.Append(NewViewWithData([]byte("0"))) + }, + }, + { + name: "append-overflow", + input: strings.Repeat("1", baseChunkSize), + output: strings.Repeat("1", baseChunkSize) + "0", + op: func(t *testing.T, b *Buffer) { + b.Append(NewViewWithData([]byte("0"))) + }, + }, + { + name: "append-multiple-views", + input: strings.Repeat("1", baseChunkSize-1), + output: strings.Repeat("1", baseChunkSize-1) + strings.Repeat("0", baseChunkSize*3), + op: func(t *testing.T, b *Buffer) { + b.Append(NewViewWithData([]byte(strings.Repeat("0", baseChunkSize)))) + b.Append(NewViewWithData([]byte(strings.Repeat("0", baseChunkSize)))) + b.Append(NewViewWithData([]byte(strings.Repeat("0", baseChunkSize)))) + }, + }, + + // AppendOwned. + { + name: "append-owned", + input: "hello", + output: "hello world", + op: func(t *testing.T, b *Buffer) { + v := NewViewWithData([]byte("Xworld")) + // Appending to a buffer that has extra references means this will + // degrade into an "appendOwned" for the chunk being added. + c := b.Clone() + defer c.Release() + b.Append(v) + v.chunk.data[0] = ' ' + }, + }, + + // Truncate. + { + name: "truncate", + input: "hello world", + output: "hello", + op: func(t *testing.T, b *Buffer) { + b.Truncate(5) + }, + }, + { + name: "truncate-noop", + input: "hello world", + output: "hello world", + op: func(t *testing.T, b *Buffer) { + b.Truncate(b.Size() + 1) + }, + }, + { + name: "truncate-multiple-buffers", + input: strings.Repeat("1", baseChunkSize), + output: strings.Repeat("1", baseChunkSize*2-1), + op: func(t *testing.T, b *Buffer) { + b.Append(NewViewWithData([]byte(strings.Repeat("1", baseChunkSize)))) + b.Truncate(baseChunkSize*2 - 1) + }, + }, + { + name: "truncate-multiple-buffers-to-one", + input: strings.Repeat("1", baseChunkSize), + output: "11111", + op: func(t *testing.T, b *Buffer) { + b.Append(NewViewWithData([]byte(strings.Repeat("1", baseChunkSize)))) + b.Truncate(5) + }, + }, + + // TrimFront. + { + name: "trim", + input: "hello world", + output: "world", + op: func(t *testing.T, b *Buffer) { + b.TrimFront(6) + }, + }, + { + name: "trim-too-large", + input: "hello world", + output: "", + op: func(t *testing.T, b *Buffer) { + b.TrimFront(b.Size() + 1) + }, + }, + { + name: "trim-multiple-buffers", + input: strings.Repeat("1", baseChunkSize), + output: strings.Repeat("1", baseChunkSize*2-1), + op: func(t *testing.T, b *Buffer) { + b.Append(NewViewWithData([]byte(strings.Repeat("1", baseChunkSize)))) + b.TrimFront(1) + }, + }, + { + name: "trim-multiple-buffers-to-one-buffer", + input: strings.Repeat("1", baseChunkSize), + output: "1", + op: func(t *testing.T, b *Buffer) { + b.Append(NewViewWithData([]byte(strings.Repeat("1", baseChunkSize)))) + b.TrimFront(baseChunkSize*2 - 1) + }, + }, + + // GrowTo. + { + name: "growto", + input: "hello world", + output: "hello world", + op: func(t *testing.T, b *Buffer) { + b.GrowTo(1, true) + }, + }, + { + name: "growto-from-zero", + output: strings.Repeat("\x00", 1024), + op: func(t *testing.T, b *Buffer) { + b.GrowTo(1024, true) + }, + }, + { + name: "growto-from-non-zero", + input: strings.Repeat("1", baseChunkSize), + output: strings.Repeat("1", baseChunkSize) + strings.Repeat("\x00", baseChunkSize), + op: func(t *testing.T, b *Buffer) { + b.GrowTo(baseChunkSize*2, true) + }, + }, + + // Clone. + { + name: "clone", + input: "hello", + output: "hello", + op: func(t *testing.T, b *Buffer) { + other := b.Clone() + bs := other.Flatten() + want := []byte("hello") + if !bytes.Equal(bs, want) { + t.Errorf("expected %v, got %v", want, bs) + } + }, + }, + { + name: "copy-large", + input: strings.Repeat("1", baseChunkSize+1), + output: strings.Repeat("1", baseChunkSize+1), + op: func(t *testing.T, b *Buffer) { + other := b.Clone() + bs := other.Flatten() + want := []byte(strings.Repeat("1", baseChunkSize+1)) + if !bytes.Equal(bs, want) { + t.Errorf("expected %v, got %v", want, bs) + } + }, + }, + + // Merge. + { + name: "merge", + input: "hello", + output: "hello world", + op: func(t *testing.T, b *Buffer) { + var other Buffer + other.Append(NewViewWithData([]byte(" world"))) + b.Merge(&other) + if sz := other.Size(); sz != 0 { + t.Errorf("expected 0, got %d", sz) + } + }, + }, + { + name: "merge-large", + input: strings.Repeat("1", baseChunkSize+1), + output: strings.Repeat("1", baseChunkSize+1) + strings.Repeat("0", baseChunkSize+1), + op: func(t *testing.T, b *Buffer) { + var other Buffer + other.Append(NewViewWithData(([]byte(strings.Repeat("0", baseChunkSize+1))))) + b.Merge(&other) + if sz := other.Size(); sz != 0 { + t.Errorf("expected 0, got %d", sz) + } + }, + }, + + // ReadAt. + { + name: "readat", + input: "hello", + output: "hello", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, 0, 6, "hello", io.EOF) }, + }, + { + name: "readat-long", + input: "hello", + output: "hello", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, 0, 8, "hello", io.EOF) }, + }, + { + name: "readat-short", + input: "hello", + output: "hello", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, 0, 3, "hel", nil) }, + }, + { + name: "readat-offset", + input: "hello", + output: "hello", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, 2, 3, "llo", io.EOF) }, + }, + { + name: "readat-long-offset", + input: "hello", + output: "hello", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, 2, 8, "llo", io.EOF) }, + }, + { + name: "readat-short-offset", + input: "hello", + output: "hello", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, 2, 2, "ll", nil) }, + }, + { + name: "readat-skip-all", + input: "hello", + output: "hello", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, baseChunkSize+1, 1, "", io.EOF) }, + }, + { + name: "readat-second-view", + input: strings.Repeat("0", baseChunkSize+1) + "12", + output: strings.Repeat("0", baseChunkSize+1) + "12", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, baseChunkSize+1, 1, "1", nil) }, + }, + { + name: "readat-second-buffer-end", + input: strings.Repeat("0", baseChunkSize+1) + "12", + output: strings.Repeat("0", baseChunkSize+1) + "12", + op: func(t *testing.T, b *Buffer) { testReadAt(t, b, baseChunkSize+1, 2, "12", io.EOF) }, + }, + } + + for _, tc := range testCases { + for fillName, fn := range fillFuncs { + t.Run(fillName+"/"+tc.name, func(t *testing.T) { + // Construct & fill the view. + var buf Buffer + fn(&buf, []byte(tc.input)) + + // Run the operation. + if tc.op != nil { + tc.op(t, &buf) + } + + // Flatten and validate. + out := buf.Flatten() + if !bytes.Equal([]byte(tc.output), out) { + t.Errorf("expected %q, got %q", tc.output, string(out)) + } + + // Ensure the size is correct. + if len(out) != int(buf.Size()) { + t.Errorf("size is wrong: expected %d, got %d", len(out), buf.Size()) + } + + // Calculate contents via apply. + var appliedOut []byte + buf.Apply(func(v *View) { + appliedOut = append(appliedOut, v.AsSlice()...) + }) + if len(appliedOut) != len(out) { + t.Errorf("expected %d, got %d", len(out), len(appliedOut)) + } + if !bytes.Equal(appliedOut, out) { + t.Errorf("expected %v, got %v", out, appliedOut) + } + + // Calculate contents via ReadToWriter. + var b bytes.Buffer + n, err := buf.ReadToWriter(&b, int64(len(out))) + if n != int64(len(out)) { + t.Errorf("expected %d, got %d", len(out), n) + } + if err != nil { + t.Errorf("expected nil, got %v", err) + } + if !bytes.Equal(b.Bytes(), out) { + t.Errorf("expected %v, got %v", out, b.Bytes()) + } + }) + } + } +} + +func TestBufferPullUp(t *testing.T) { + for _, tc := range []struct { + desc string + inputs []string + offset int + length int + output string + failed bool + // lengths is the lengths of each buffer node after the pull up. + lengths []int + }{ + { + desc: "whole empty view", + }, + { + desc: "zero pull", + inputs: []string{"hello", " world"}, + lengths: []int{5, 6}, + }, + { + desc: "whole view", + inputs: []string{"hello", " world"}, + offset: 0, + length: 11, + output: "hello world", + lengths: []int{11}, + }, + { + desc: "middle to end aligned", + inputs: []string{"0123", "45678", "9abcd"}, + offset: 4, + length: 10, + output: "456789abcd", + lengths: []int{4, 10}, + }, + { + desc: "middle to end unaligned", + inputs: []string{"0123", "45678", "9abcd"}, + offset: 6, + length: 8, + output: "6789abcd", + lengths: []int{4, 10}, + }, + { + desc: "middle aligned", + inputs: []string{"0123", "45678", "9abcd", "efgh"}, + offset: 6, + length: 5, + output: "6789a", + lengths: []int{4, 10, 4}, + }, + + // Failed cases. + { + desc: "empty view - length too long", + offset: 0, + length: 1, + failed: true, + }, + { + desc: "empty view - offset too large", + offset: 1, + length: 1, + failed: true, + }, + { + desc: "length too long", + inputs: []string{"0123", "45678", "9abcd"}, + offset: 4, + length: 100, + failed: true, + lengths: []int{4, 5, 5}, + }, + { + desc: "offset too large", + inputs: []string{"0123", "45678", "9abcd"}, + offset: 100, + length: 1, + failed: true, + lengths: []int{4, 5, 5}, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + var b Buffer + defer b.Release() + for _, s := range tc.inputs { + v := NewViewWithData([]byte(s)) + b.appendOwned(v) + } + + got, gotOk := b.PullUp(tc.offset, tc.length) + want, wantOk := []byte(tc.output), !tc.failed + if gotOk == wantOk && got == nil && len(want) == 0 { + return + } + if gotOk != wantOk || !bytes.Equal(got.AsSlice(), want) { + t.Errorf("v.PullUp(%d, %d) = %q, %t; %q, %t", tc.offset, tc.length, got.AsSlice(), gotOk, want, wantOk) + } + + var gotLengths []int + for v := b.data.Front(); v != nil; v = v.Next() { + gotLengths = append(gotLengths, v.Size()) + } + if !reflect.DeepEqual(gotLengths, tc.lengths) { + t.Errorf("lengths = %v; want %v", gotLengths, tc.lengths) + } + }) + } +} + +func TestBufferClone(t *testing.T) { + const ( + originalSize = 90 + bytesToDelete = 30 + ) + b := MakeWithData(bytes.Repeat([]byte{originalSize}, originalSize)) + clonedB := b.Clone() + b.TrimFront(bytesToDelete) + + if got, want := int(b.Size()), originalSize-bytesToDelete; got != want { + t.Errorf("original buffer was not changed: size expected = %d, got = %d", want, got) + } + if got := clonedB.Size(); got != originalSize { + t.Errorf("cloned buffer should not be modified: expected size = %d, got = %d", originalSize, got) + } +} + +func TestBufferSubApply(t *testing.T) { + var b Buffer + defer b.Release() + b.appendOwned(NewViewWithData([]byte("0123"))) + b.appendOwned(NewViewWithData([]byte("45678"))) + b.appendOwned(NewViewWithData([]byte("9abcd"))) + data := []byte("0123456789abcd") + + for i := 0; i <= len(data); i++ { + for j := i; j <= len(data); j++ { + t.Run(fmt.Sprintf("SubApply(%d,%d)", i, j), func(t *testing.T) { + var got []byte + b.SubApply(i, j-i, func(v *View) { + got = append(got, v.AsSlice()...) + }) + if want := data[i:j]; !bytes.Equal(got, want) { + t.Errorf("got = %q; want %q", got, want) + } + }) + } + } +} + +func doSaveAndLoad(t *testing.T, toSave, toLoad *Buffer) { + t.Helper() + var buf bytes.Buffer + ctx := context.Background() + if _, err := state.Save(ctx, &buf, toSave); err != nil { + t.Fatal("state.Save:", err) + } + if _, err := state.Load(ctx, bytes.NewReader(buf.Bytes()), toLoad); err != nil { + t.Fatal("state.Load:", err) + } +} + +func TestSaveRestoreBufferEmpty(t *testing.T) { + var toSave Buffer + var b Buffer + doSaveAndLoad(t, &toSave, &b) + + if got := b.Flatten(); len(got) != 0 { + t.Errorf("v.Flatten() = %x, want []", got) + } +} + +func TestSaveRestoreBuffer(t *testing.T) { + // Create data that fits slots. + data := bytes.Join([][]byte{ + bytes.Repeat([]byte{1, 2}, baseChunkSize), + }, nil) + + var toSave Buffer + toSave.appendOwned(NewViewWithData(data)) + + var b Buffer + doSaveAndLoad(t, &toSave, &b) + + // Next available slot at index 3; 0-2 slot are used. + if got := b.Flatten(); !bytes.Equal(got, data) { + t.Errorf("v.Flatten() = %x, want %x", got, data) + } +} + +func TestRangeIntersect(t *testing.T) { + for _, tc := range []struct { + desc string + x, y, want Range + }{ + { + desc: "empty intersects empty", + }, + { + desc: "empty intersection", + x: Range{end: 10}, + y: Range{begin: 10, end: 20}, + }, + { + desc: "some intersection", + x: Range{begin: 5, end: 20}, + y: Range{end: 10}, + want: Range{begin: 5, end: 10}, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + if got := tc.x.Intersect(tc.y); got != tc.want { + t.Errorf("(%#v).Intersect(%#v) = %#v; want %#v", tc.x, tc.y, got, tc.want) + } + if got := tc.y.Intersect(tc.x); got != tc.want { + t.Errorf("(%#v).Intersect(%#v) = %#v; want %#v", tc.y, tc.x, got, tc.want) + } + }) + } +} + +func TestRangeOffset(t *testing.T) { + for _, tc := range []struct { + input Range + offset int + output Range + }{ + { + input: Range{}, + offset: 0, + output: Range{}, + }, + { + input: Range{}, + offset: -1, + output: Range{begin: -1, end: -1}, + }, + { + input: Range{begin: 10, end: 20}, + offset: -1, + output: Range{begin: 9, end: 19}, + }, + { + input: Range{begin: 10, end: 20}, + offset: 2, + output: Range{begin: 12, end: 22}, + }, + } { + if got := tc.input.Offset(tc.offset); got != tc.output { + t.Errorf("(%#v).Offset(%d) = %#v, want %#v", tc.input, tc.offset, got, tc.output) + } + } +} + +func TestRangeLen(t *testing.T) { + for _, tc := range []struct { + r Range + want int + }{ + {r: Range{}, want: 0}, + {r: Range{begin: 1, end: 1}, want: 0}, + {r: Range{begin: -1, end: -1}, want: 0}, + {r: Range{end: 10}, want: 10}, + {r: Range{begin: 5, end: 10}, want: 5}, + } { + if got := tc.r.Len(); got != tc.want { + t.Errorf("(%#v).Len() = %d, want %d", tc.r, got, tc.want) + } + } +} diff --git a/pkg/bufferv2/chunk.go b/pkg/bufferv2/chunk.go new file mode 100644 index 000000000..8cab3cba0 --- /dev/null +++ b/pkg/bufferv2/chunk.go @@ -0,0 +1,113 @@ +// 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 buffer + +import ( + "fmt" + + "gvisor.dev/gvisor/pkg/bits" + "gvisor.dev/gvisor/pkg/sync" +) + +const ( + // This is log2(baseChunkSize). This number is used to calculate which pool + // to use for a payload size by right shifting the payload size by this + // number and passing the result to MostSignificantOne64. + baseChunkSizeLog2 = 6 + + // This is the size of the buffers in the first pool. Each subsquent pool + // creates payloads 2^(pool index) times larger than the first pool's + // payloads. + baseChunkSize = 1 << baseChunkSizeLog2 // 64 + + // The largest payload size that we pool. Payloads larger than this will + // allocated from the heap and garbage collected as normal. + maxChunkSize = baseChunkSize << (numPools - 1) // 65536 + + // The number of chunk pools we have for use. + numPools = 11 +) + +// chunkPools is a collection of pools for payloads of different sizes. The +// size of the payloads doubles in each successive pool. +var chunkPools [numPools]sync.Pool + +func init() { + for i := 0; i < numPools; i++ { + chunkSize := baseChunkSize * (1 << i) + chunkPools[i].New = func() interface{} { + return &chunk{ + data: make([]byte, chunkSize), + } + } + } +} + +// Precondition: 0 <= size <= maxChunkSize +func getChunkPool(size int) *sync.Pool { + idx := 0 + if size > baseChunkSize { + idx = bits.MostSignificantOne64(uint64(size) >> baseChunkSizeLog2) + if size > 1<<(idx+baseChunkSizeLog2) { + idx++ + } + } + if idx >= numPools { + panic(fmt.Sprintf("pool for chunk size %d does not exist", size)) + } + return &chunkPools[idx] +} + +// Chunk represents a slice of pooled memory. +type chunk struct { + chunkRefs + data []byte +} + +func newChunk(size int) *chunk { + var c *chunk + if size > maxChunkSize { + c = &chunk{ + data: make([]byte, size), + } + } else { + pool := getChunkPool(size) + c = pool.Get().(*chunk) + for i := range c.data { + c.data[i] = 0 + } + } + c.InitRefs() + return c +} + +func (c *chunk) destroy() { + if len(c.data) > maxChunkSize { + c.data = nil + return + } + pool := getChunkPool(len(c.data)) + pool.Put(c) +} + +func (c *chunk) DecRef() { + c.chunkRefs.DecRef(c.destroy) +} + +func (c *chunk) Clone() *chunk { + cpy := newChunk(len(c.data)) + copy(cpy.data, c.data) + return cpy +} diff --git a/pkg/bufferv2/view.go b/pkg/bufferv2/view.go new file mode 100644 index 000000000..659493bcc --- /dev/null +++ b/pkg/bufferv2/view.go @@ -0,0 +1,232 @@ +// 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 buffer + +import ( + "fmt" + "io" + + "gvisor.dev/gvisor/pkg/sync" +) + +var viewPool = sync.Pool{ + New: func() interface{} { + return &View{} + }, +} + +// View is a window into a shared chunk. Views are held by Buffers in +// viewLists to represent contiguous memory. +// +// A View must be created with NewView, NewViewWithData, or Clone. Owners are +// responsible for maintaining ownership over their views. When Views need to be +// shared or copied, the owner should create a new View with Clone. Clone must +// only ever be called on a owned View, not a borrowed one. +// +// Users are responsible for calling Release when finished with their View so +// that its resources can be returned to the pool. +// +// Users must not write directly to slices returned by AsSlice. Instead, they +// must use Write/WriteAt/CopyIn to modify the underlying View. This preserves +// the safety guarantees of copy-on-write. +type View struct { + sync.NoCopy + + viewEntry + read int + write int + chunk *chunk +} + +// NewView creates a new view with capacity at least as big as cap. It is +// analogous to make([]byte, 0, cap). +func NewView(cap int) *View { + c := newChunk(cap) + v := viewPool.Get().(*View) + *v = View{chunk: c} + return v +} + +// NewViewSize creates a new view with capacity at least as big as size and +// length that is exactly size. It is analogous to make([]byte, size). +func NewViewSize(size int) *View { + v := NewView(size) + v.Grow(size) + return v +} + +// NewViewWithData creates a new view and initializes it with data. This +// function should be used with caution to avoid unnecessary []byte allocations. +// When in doubt use NewWithView to maximize chunk reuse in production +// environments. +func NewViewWithData(data []byte) *View { + c := newChunk(len(data)) + v := viewPool.Get().(*View) + *v = View{chunk: c} + v.Write(data) + return v +} + +// Clone creates a shallow clone of v where the underlying chunk is shared. +// +// The caller must own the View to call Clone. It is not safe to call Clone +// on a borrowed or shared View because it can race with other View methods. +func (v *View) Clone() *View { + v.chunk.IncRef() + newV := viewPool.Get().(*View) + newV.chunk = v.chunk + newV.read = v.read + newV.write = v.write + return newV +} + +// Release releases the chunk held by v and returns v to the pool. +func (v *View) Release() { + v.chunk.DecRef() + *v = View{} + viewPool.Put(v) +} + +func (v *View) sharesChunk() bool { + return v.chunk.refCount.Load() > 1 +} + +// Full indicates the chunk is full. +// +// This indicates there is no capacity left to write. +func (v *View) Full() bool { + return v == nil || v.write == len(v.chunk.data) +} + +// Capacity returns the total size of this view's chunk. +func (v *View) Capacity() int { + return len(v.chunk.data) +} + +// Size returns the size of data written to the view. +func (v *View) Size() int { + return v.write - v.read +} + +// TrimFront advances the read index by the given amount. +func (v *View) TrimFront(n int) { + v.read += n +} + +// AsSlice returns a slice of the data written to this view. +func (v *View) AsSlice() []byte { + if v == nil { + return nil + } + return v.chunk.data[v.read:v.write] +} + +// AvailableSize returns the number of bytes available for writing. +func (v *View) AvailableSize() int { + return len(v.chunk.data) - v.write +} + +// Read reads v's data into p. +// +// Implements the io.Reader interface. +func (v *View) Read(p []byte) (int, error) { + if len(p) == 0 { + return 0, nil + } + if v.Size() == 0 { + return 0, io.EOF + } + n := copy(p, v.AsSlice()) + v.TrimFront(n) + return n, nil +} + +// ReadAt reads data to the p starting at offset. +// +// Implements the io.ReaderAt interface. +func (v *View) ReadAt(p []byte, off int) (int, error) { + if off < 0 || off > v.Size() { + return 0, fmt.Errorf("ReadAt(): offset out of bounds: want 0 < off < %d, got off=%d", v.Size(), off) + } + n := copy(p, v.AsSlice()[off:]) + return n, nil +} + +// Write writes data to the view's chunk starting at the v.write index. If the +// view's chunk has a reference count greater than 1, the chunk is copied first +// and then written to. +// +// Implements the io.Writer interface. +func (v *View) Write(p []byte) (int, error) { + if v.sharesChunk() { + defer v.chunk.DecRef() + v.chunk = v.chunk.Clone() + } + n := copy(v.chunk.data[v.write:], p) + v.write += n + if n < len(p) { + return n, fmt.Errorf("could not finish write: want len(p) <= v.AvailableSize(), got len(p)=%d, v.AvailableSize()=%d", len(p), v.AvailableSize()) + } + return n, nil +} + +// WriteAt writes data to the views's chunk starting at start. If the +// view's chunk has a reference count greater than 1, the chunk is copied first +// and then written to. +// +// Implements the io.WriterAt interface. +func (v *View) WriteAt(p []byte, off int) (int, error) { + if off < 0 || off > v.Size() { + return 0, fmt.Errorf("write offset out of bounds: want 0 < off < %d, got off=%d", v.Size(), off) + } + if v.sharesChunk() { + defer v.chunk.DecRef() + v.chunk = v.chunk.Clone() + } + n := copy(v.AsSlice()[off:], p) + if n < len(p) { + return n, fmt.Errorf("could not finish write: want off + len(p) < v.Capacity(), got off=%d, len(p)=%d ,v.Size() = %d", off, len(p), v.Size()) + } + return n, nil +} + +// Grow advances the write index by the given amount. +func (v *View) Grow(n int) { + if n+v.write > v.Capacity() { + panic("cannot grow view past capacity") + } + v.write += n +} + +// CapLength caps the length of the view's read slice to n. If n > v.Size(), +// the function is a no-op. +func (v *View) CapLength(n int) { + if n < 0 { + panic("n must be >= 0") + } + if n > v.Size() { + n = v.Size() + } + v.write = v.read + n +} + +func (v *View) availableSlice() []byte { + if v.sharesChunk() { + defer v.chunk.DecRef() + c := v.chunk.Clone() + v.chunk = c + } + return v.chunk.data[v.write:] +} diff --git a/pkg/bufferv2/view_test.go b/pkg/bufferv2/view_test.go new file mode 100644 index 000000000..25eda9e22 --- /dev/null +++ b/pkg/bufferv2/view_test.go @@ -0,0 +1,174 @@ +// 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 buffer + +import ( + "math/rand" + "testing" + + "github.com/google/go-cmp/cmp" +) + +func TestNewView(t *testing.T) { + for sz := baseChunkSize; sz < maxChunkSize; sz <<= 1 { + v := NewView(sz - 1) + defer v.Release() + + if v.Capacity() != sz { + t.Errorf("v.Capacity() = %d, want %d", v.Capacity(), sz) + } + if v.AvailableSize() != sz { + t.Errorf("v.WriteSize() = %d, want %d", v.AvailableSize(), sz) + } + if v.Size() != 0 { + t.Errorf("v.ReadSize() = %d, want %d", v.Size(), 0) + } + + v1 := NewView(sz) + defer v1.Release() + + if v1.Capacity() != sz { + t.Errorf("v.Capacity() = %d, want %d", v.Capacity(), sz) + } + if v1.AvailableSize() != sz { + t.Errorf("v.WriteSize() = %d, want %d", v.AvailableSize(), sz) + } + if v1.Size() != 0 { + t.Errorf("v.ReadSize() = %d, want %d", v.Size(), 0) + } + } + + // Allocating from heap should produce a chunk with the exact size requested + // instead of a chunk where the size is contingent on the pool it came from. + viewSize := maxChunkSize + 1 + v := NewView(viewSize) + defer v.Release() + if v.Capacity() != viewSize { + t.Errorf("v.Capacity() = %d, want %d", v.Capacity(), viewSize) + } +} + +func TestClone(t *testing.T) { + orig := NewView(100) + clone := orig.Clone() + if orig.chunk != clone.chunk { + t.Errorf("orig.Clone().chunk = %p, want %p", clone.chunk, orig.chunk) + } + if orig.chunk.refCount.Load() != 2 { + t.Errorf("got orig.chunk.chunkRefs.Load() = %d, want 2", orig.chunk.refCount.Load()) + } + orig.Release() + if clone.chunk.refCount.Load() != 1 { + t.Errorf("got clone.chunk.chunkRefs.Load() = %d, want 1", clone.chunk.refCount.Load()) + } + clone.Release() +} + +func TestWrite(t *testing.T) { + for _, tc := range []struct { + name string + view *View + initData []byte + writeSize int + }{ + { + name: "empty view", + view: NewView(100), + writeSize: 50, + }, + { + name: "full view", + view: NewView(100), + initData: make([]byte, 100), + writeSize: 50, + }, + { + name: "full write to partially full view", + view: NewView(100), + initData: make([]byte, 20), + writeSize: 50, + }, + { + name: "partial write to partially full view", + view: NewView(100), + initData: make([]byte, 80), + writeSize: 50, + }, + } { + t.Run(tc.name, func(t *testing.T) { + tc.view.Write(tc.initData) + defer tc.view.Release() + origWriteSize := tc.view.AvailableSize() + + var orig []byte + orig = append(orig, tc.view.AsSlice()...) + toWrite := make([]byte, tc.writeSize) + rand.Read(toWrite) + + n, _ := tc.view.Write(toWrite) + + if n > origWriteSize { + t.Errorf("got tc.view.Write() = %d, want <=%d", n, origWriteSize) + } + if tc.writeSize > origWriteSize { + toWrite = toWrite[:origWriteSize] + } + if tc.view.AvailableSize() != tc.view.Capacity()-(len(toWrite)+len(orig)) { + t.Errorf("got tc.view.WriteSize() = %d, want %d", tc.view.AvailableSize(), tc.view.Capacity()-(len(toWrite)+len(orig))) + } + if !cmp.Equal(tc.view.AsSlice(), append(orig, toWrite...)) { + t.Errorf("got tc.view.ReadSlice() = %d, want %d", tc.view.AsSlice(), toWrite) + } + }) + } +} + +func TestWriteToCloned(t *testing.T) { + orig := NewView(100) + toWrite := make([]byte, 20) + rand.Read(toWrite) + orig.Write(toWrite) + + clone := orig.Clone() + clone.Write(toWrite) + + if !cmp.Equal(orig.AsSlice(), toWrite) { + t.Errorf("got orig.ReadSlice() = %v, want %v", orig.AsSlice(), toWrite) + } + + toWrite = append(toWrite, toWrite...) + if !cmp.Equal(clone.AsSlice(), toWrite) { + t.Errorf("got clone.ReadSlice() = %v, want %v", clone.AsSlice(), toWrite) + } +} + +func TestWriteAt(t *testing.T) { + size := 10 + off := 5 + v := NewViewSize(size) + p := make([]byte, 20) + rand.Read(p) + orig := v.Clone() + + if n, _ := v.WriteAt(p, off); n != size-off { + t.Errorf("got v.CopyIn()= %v, want %v", n, size-off) + } + if !cmp.Equal(v.AsSlice()[off:], p[:size-off]) { + t.Errorf("got v.AsSlice()[off:] = %v, want %v", v.AsSlice()[off:], p[off:size]) + } + if !cmp.Equal(v.AsSlice()[:off], orig.AsSlice()[:off]) { + t.Errorf("got v.AsSlice()[:off] = %v, want %v", v.AsSlice()[:off], orig.AsSlice()[:off]) + } +} diff --git a/pkg/bufferv2/view_unsafe.go b/pkg/bufferv2/view_unsafe.go new file mode 100644 index 000000000..19c99bcbf --- /dev/null +++ b/pkg/bufferv2/view_unsafe.go @@ -0,0 +1,32 @@ +// Copyright 2020 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 buffer + +import ( + "reflect" + "unsafe" +) + +// minBatch is the smallest Read or Write operation that the +// WriteFromReader and ReadToWriter functions will use. +// +// This is defined as the size of a native pointer. +const minBatch = int(unsafe.Sizeof(uintptr(0))) + +// BasePtr returns a pointer to the view's chunk. +func (v *View) BasePtr() *byte { + hdr := (*reflect.SliceHeader)(unsafe.Pointer(&v.chunk.data)) + return (*byte)(unsafe.Pointer(hdr.Data)) +} From c54948f3c1940071f1690651b533e22977481de4 Mon Sep 17 00:00:00 2001 From: Kevin Krakauer Date: Tue, 14 Jun 2022 20:01:58 -0700 Subject: [PATCH 11/29] notify users of common reasons for failing to create a raw socket Fixes #199 PiperOrigin-RevId: 455018292 --- pkg/log/BUILD | 2 + pkg/log/rate_limited.go | 63 +++++++++++++++++++++ pkg/sentry/socket/netstack/provider.go | 6 ++ pkg/sentry/socket/netstack/provider_vfs2.go | 1 + pkg/tcpip/stack/stack.go | 4 ++ 5 files changed, 76 insertions(+) create mode 100644 pkg/log/rate_limited.go diff --git a/pkg/log/BUILD b/pkg/log/BUILD index 3ed6aba5c..7a48f92c7 100644 --- a/pkg/log/BUILD +++ b/pkg/log/BUILD @@ -9,6 +9,7 @@ go_library( "json.go", "json_k8s.go", "log.go", + "rate_limited.go", ], marshal = False, stateify = False, @@ -18,6 +19,7 @@ go_library( deps = [ "//pkg/linewriter", "//pkg/sync", + "@org_golang_x_time//rate:go_default_library", ], ) diff --git a/pkg/log/rate_limited.go b/pkg/log/rate_limited.go new file mode 100644 index 000000000..285d9b26d --- /dev/null +++ b/pkg/log/rate_limited.go @@ -0,0 +1,63 @@ +// 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 log + +import ( + "time" + + "golang.org/x/time/rate" +) + +type rateLimitedLogger struct { + logger Logger + limit *rate.Limiter +} + +func (rl *rateLimitedLogger) Debugf(format string, v ...interface{}) { + if rl.limit.Allow() { + rl.logger.Debugf(format, v...) + } +} + +func (rl *rateLimitedLogger) Infof(format string, v ...interface{}) { + if rl.limit.Allow() { + rl.logger.Infof(format, v...) + } +} + +func (rl *rateLimitedLogger) Warningf(format string, v ...interface{}) { + if rl.limit.Allow() { + rl.logger.Warningf(format, v...) + } +} + +func (rl *rateLimitedLogger) IsLogging(level Level) bool { + return rl.logger.IsLogging(level) +} + +// BasicRateLimitedLogger returns a Logger that logs to the global logger no +// more than once per the provided duration. +func BasicRateLimitedLogger(every time.Duration) Logger { + return RateLimitedLogger(Log(), every) +} + +// RateLimitedLogger returns a Logger that logs to the provided logger no more +// than once per the provided duration. +func RateLimitedLogger(logger Logger, every time.Duration) Logger { + return &rateLimitedLogger{ + logger: logger, + limit: rate.NewLimiter(rate.Every(every), 1), + } +} diff --git a/pkg/sentry/socket/netstack/provider.go b/pkg/sentry/socket/netstack/provider.go index 8605ad507..e675db199 100644 --- a/pkg/sentry/socket/netstack/provider.go +++ b/pkg/sentry/socket/netstack/provider.go @@ -15,9 +15,12 @@ package netstack import ( + "time" + "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/context" + "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/sentry/fs" "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" @@ -40,6 +43,8 @@ type provider struct { netProto tcpip.NetworkProtocolNumber } +var rawMissingLogger = log.BasicRateLimitedLogger(time.Minute) + // getTransportProtocol figures out transport protocol. Currently only TCP, // UDP, and ICMP are supported. The bool return value is true when this socket // is associated with a transport protocol. This is only false for SOCK_RAW, @@ -66,6 +71,7 @@ func getTransportProtocol(ctx context.Context, stype linux.SockType, protocol in // Raw sockets require CAP_NET_RAW. creds := auth.CredentialsFromContext(ctx) if !creds.HasCapability(linux.CAP_NET_RAW) { + rawMissingLogger.Infof("A process tried to create a raw socket without CAP_NET_RAW. Should the container config enable CAP_NET_RAW?") return 0, true, syserr.ErrNotPermitted } diff --git a/pkg/sentry/socket/netstack/provider_vfs2.go b/pkg/sentry/socket/netstack/provider_vfs2.go index ba1cc79e9..f2ebb233f 100644 --- a/pkg/sentry/socket/netstack/provider_vfs2.go +++ b/pkg/sentry/socket/netstack/provider_vfs2.go @@ -86,6 +86,7 @@ func packetSocketVFS2(t *kernel.Task, epStack *Stack, stype linux.SockType, prot // Packet sockets require CAP_NET_RAW. creds := auth.CredentialsFromContext(t) if !creds.HasCapability(linux.CAP_NET_RAW) { + rawMissingLogger.Infof("A process tried to create a raw socket without CAP_NET_RAW. Should the container config enable CAP_NET_RAW?") return nil, syserr.ErrNotPermitted } diff --git a/pkg/tcpip/stack/stack.go b/pkg/tcpip/stack/stack.go index 7836934da..a741f0251 100644 --- a/pkg/tcpip/stack/stack.go +++ b/pkg/tcpip/stack/stack.go @@ -30,6 +30,7 @@ import ( "golang.org/x/time/rate" "gvisor.dev/gvisor/pkg/atomicbitops" "gvisor.dev/gvisor/pkg/buffer" + "gvisor.dev/gvisor/pkg/log" cryptorand "gvisor.dev/gvisor/pkg/rand" "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/pkg/tcpip" @@ -64,6 +65,8 @@ func (u *uniqueIDGenerator) UniqueID() uint64 { return ((*atomicbitops.Uint64)(u)).Add(1) } +var netRawMissingLogger = log.BasicRateLimitedLogger(time.Minute) + // Stack is a networking stack, with all supported protocols, NICs, and route // table. // @@ -712,6 +715,7 @@ func (s *Stack) NewEndpoint(transport tcpip.TransportProtocolNumber, network tcp // of address. func (s *Stack) NewRawEndpoint(transport tcpip.TransportProtocolNumber, network tcpip.NetworkProtocolNumber, waiterQueue *waiter.Queue, associated bool) (tcpip.Endpoint, tcpip.Error) { if s.rawFactory == nil { + netRawMissingLogger.Infof("A process tried to create a raw socket, but --net-raw was not specified. Should runsc be run with --net-raw?") return nil, &tcpip.ErrNotPermitted{} } From 1ff543e17ee355943abfba33be59ad771bb88095 Mon Sep 17 00:00:00 2001 From: Adin Scannell Date: Tue, 14 Jun 2022 21:20:35 -0700 Subject: [PATCH 12/29] Handle cross-package global guards. Updates #7721 PiperOrigin-RevId: 455029306 --- tools/checklocks/analysis.go | 14 +++++++++--- tools/checklocks/facts.go | 14 +++++++++--- tools/checklocks/test/BUILD | 1 + tools/checklocks/test/crosspkg/BUILD | 12 ++++++++++ tools/checklocks/test/crosspkg/crosspkg.go | 26 ++++++++++++++++++++++ tools/checklocks/test/globals.go | 12 ++++++++++ 6 files changed, 73 insertions(+), 6 deletions(-) create mode 100644 tools/checklocks/test/crosspkg/BUILD create mode 100644 tools/checklocks/test/crosspkg/crosspkg.go diff --git a/tools/checklocks/analysis.go b/tools/checklocks/analysis.go index 9b880d7db..8a8f18ea5 100644 --- a/tools/checklocks/analysis.go +++ b/tools/checklocks/analysis.go @@ -322,9 +322,17 @@ func (pc *passContext) checkFieldAccess(inst almostInst, structObj ssa.Value, fi pc.checkGuards(inst, structObj, fieldObj, ls, isWrite) } +// noReferrers wraps an instruction as an almostInst. +type noReferrers struct { + ssa.Instruction +} + +// Referrers implements almostInst.Referrers. +func (noReferrers) Referrers() *[]ssa.Instruction { return nil } + // checkGlobalAccess checks the validity of a global access. -func (pc *passContext) checkGlobalAccess(g *ssa.Global, ls *lockState, isWrite bool) { - pc.checkGuards(g, g, g.Object(), ls, isWrite) +func (pc *passContext) checkGlobalAccess(inst ssa.Instruction, g *ssa.Global, ls *lockState, isWrite bool) { + pc.checkGuards(noReferrers{inst}, g, g.Object(), ls, isWrite) } func (pc *passContext) checkCall(call callCommon, lff *lockFunctionFacts, ls *lockState) { @@ -592,7 +600,7 @@ func (pc *passContext) checkInstruction(inst ssa.Instruction, lff *lockFunctionF continue } _, isWrite := inst.(*ssa.Store) - pc.checkGlobalAccess(g, ls, isWrite) + pc.checkGlobalAccess(inst, g, ls, isWrite) } // Process the instruction. diff --git a/tools/checklocks/facts.go b/tools/checklocks/facts.go index cdac713a9..2d9d6380a 100644 --- a/tools/checklocks/facts.go +++ b/tools/checklocks/facts.go @@ -167,6 +167,9 @@ type globalGuard struct { // ObjectName indicates the object from which resolution should occur. ObjectName string + // PackageName is the package where the object lives. + PackageName string + // FieldList is the traversal path from object. FieldList fieldList } @@ -179,7 +182,11 @@ type ssaPackager interface { // resolveCommon implements resolution for all cases. func (g *globalGuard) resolveCommon(pc *passContext, ls *lockState) resolvedValue { state := pc.pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA) - v := state.Pkg.Members[g.ObjectName].(ssa.Value) + pkg := state.Pkg + if g.PackageName != "" && g.PackageName != state.Pkg.Pkg.Path() { + pkg = state.Pkg.Prog.ImportedPackage(g.PackageName) + } + v := pkg.Members[g.ObjectName].(ssa.Value) return makeResolvedValue(v, g.FieldList) } @@ -627,8 +634,9 @@ func (pc *passContext) findGlobalGuard(pos token.Pos, guardName string) (*global return nil, false } return &globalGuard{ - ObjectName: parts[0], - FieldList: fl, + ObjectName: parts[0], + PackageName: pc.pass.Pkg.Path(), + FieldList: fl, }, true } diff --git a/tools/checklocks/test/BUILD b/tools/checklocks/test/BUILD index 21a68fbdf..9ef6a0a1b 100644 --- a/tools/checklocks/test/BUILD +++ b/tools/checklocks/test/BUILD @@ -27,4 +27,5 @@ go_library( # control expected failures for analysis. marshal = False, stateify = False, + deps = ["//tools/checklocks/test/crosspkg"], ) diff --git a/tools/checklocks/test/crosspkg/BUILD b/tools/checklocks/test/crosspkg/BUILD new file mode 100644 index 000000000..03fa92297 --- /dev/null +++ b/tools/checklocks/test/crosspkg/BUILD @@ -0,0 +1,12 @@ +load("//tools:defs.bzl", "go_library") + +package(licenses = ["notice"]) + +go_library( + name = "crosspkg", + srcs = ["crosspkg.go"], + # See next level up. + marshal = False, + stateify = False, + visibility = ["//tools/checklocks/test:__pkg__"], +) diff --git a/tools/checklocks/test/crosspkg/crosspkg.go b/tools/checklocks/test/crosspkg/crosspkg.go new file mode 100644 index 000000000..ccc0d6bad --- /dev/null +++ b/tools/checklocks/test/crosspkg/crosspkg.go @@ -0,0 +1,26 @@ +// 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 crosspkg is a second package for testing. +package crosspkg + +import ( + "sync" +) + +var ( + // +checklocks:FooMu + Foo int + FooMu sync.Mutex +) diff --git a/tools/checklocks/test/globals.go b/tools/checklocks/test/globals.go index 656b0c9a3..03c7473d6 100644 --- a/tools/checklocks/test/globals.go +++ b/tools/checklocks/test/globals.go @@ -16,6 +16,8 @@ package test import ( "sync" + + "gvisor.dev/gvisor/tools/checklocks/test/crosspkg" ) var ( @@ -83,3 +85,13 @@ func testGlobalInvalid() { otherStruct.guardedField2 = 1 // +checklocksfail otherStruct.guardedField3 = 1 // +checklocksfail } + +func testCrosspkgGlobalValid() { + crosspkg.FooMu.Lock() + crosspkg.Foo = 1 + crosspkg.FooMu.Unlock() +} + +func testCrosspkgGlobalInvalid() { + crosspkg.Foo = 1 // +checklocksfail +} From 7d3a75fa60f83bc211f194ebffb0a182f092a8bb Mon Sep 17 00:00:00 2001 From: Nate Hurley Date: Wed, 15 Jun 2022 09:58:35 -0700 Subject: [PATCH 13/29] Enable multicast forwarding per protocol. After this change, multicast packets will only be forwarded if multicast forwarding is enabled on the relevant NIC AND it is enabled for the relevant protocol (IPv4 or IPv6). Multicast forwarding at the protocol level will be tied to the lifetime of the IPv4/IPv6RoutingTableControllers. In particular, this change enables us to satisfy the following requirements: https://cs.opensource.google/fuchsia/fuchsia/+/main:sdk/fidl/fuchsia.net.multicast.admin/ipv4.fidl;l=20-21;drc=acd6519f50c92e30f18cc1816bd1c400759b533c Updates #7338. PiperOrigin-RevId: 455155793 --- .../network/internal/multicast/route_table.go | 10 + .../internal/multicast/route_table_test.go | 38 +++ pkg/tcpip/network/ipv4/ipv4.go | 89 ++++-- pkg/tcpip/network/ipv4/ipv4_test.go | 21 ++ pkg/tcpip/network/ipv6/icmp_test.go | 11 +- pkg/tcpip/network/ipv6/ipv6.go | 86 +++-- pkg/tcpip/network/ipv6/ipv6_test.go | 13 + pkg/tcpip/stack/registration.go | 10 + pkg/tcpip/stack/stack.go | 46 +++ pkg/tcpip/stack/stack_test.go | 141 +++++++++ .../integration/multicast_forward_test.go | 298 +++++++++++++----- 11 files changed, 638 insertions(+), 125 deletions(-) diff --git a/pkg/tcpip/network/internal/multicast/route_table.go b/pkg/tcpip/network/internal/multicast/route_table.go index 3fe307664..41227e6ed 100644 --- a/pkg/tcpip/network/internal/multicast/route_table.go +++ b/pkg/tcpip/network/internal/multicast/route_table.go @@ -413,6 +413,16 @@ func (r *RouteTable) RemoveInstalledRoute(key stack.UnicastSourceAndMulticastDes return false } +// RemoveAllInstalledRoutes removes all installed routes from the table. +func (r *RouteTable) RemoveAllInstalledRoutes() { + r.installedMu.Lock() + defer r.installedMu.Unlock() + + for key := range r.installedRoutes { + delete(r.installedRoutes, key) + } +} + // GetLastUsedTimestamp returns a monotonic timestamp that represents the last // time the route that matches the provided key was used or updated. // diff --git a/pkg/tcpip/network/internal/multicast/route_table_test.go b/pkg/tcpip/network/internal/multicast/route_table_test.go index ae1d1fd91..6adedad19 100644 --- a/pkg/tcpip/network/internal/multicast/route_table_test.go +++ b/pkg/tcpip/network/internal/multicast/route_table_test.go @@ -407,6 +407,44 @@ func TestRemoveInstalledRouteWithNoMatchingRoute(t *testing.T) { } } +func TestRemoveAllInstalledRoutes(t *testing.T) { + otherAddress := testutil.MustParse4("192.168.2.1") + + table := RouteTable{} + defer table.Close() + config := defaultConfig() + if err := table.Init(config); err != nil { + t.Fatalf("table.Init(%#v): %s", config, err) + } + + routes := map[stack.UnicastSourceAndMulticastDestination]stack.MulticastRoute{ + defaultRouteKey: defaultRoute, + stack.UnicastSourceAndMulticastDestination{otherAddress, otherAddress}: defaultRoute, + } + + for key, route := range routes { + installedRoute := table.NewInstalledRoute(route) + table.AddInstalledRoute(key, installedRoute) + } + + table.RemoveAllInstalledRoutes() + + for key := range routes { + pkt := newPacketBuffer("hello") + defer pkt.DecRef() + + result, hasBufferSpace := table.GetRouteOrInsertPending(key, pkt) + + if !hasBufferSpace { + t.Fatalf("table.GetRouteOrInsertPending(%#v, %#v): false", key, pkt) + } + + if result.InstalledRoute != nil { + t.Errorf("result.InstalledRoute = %v, want = nil", result.InstalledRoute) + } + } +} + func TestGetLastUsedTimestampWithNoMatchingRoute(t *testing.T) { table := RouteTable{} defer table.Close() diff --git a/pkg/tcpip/network/ipv4/ipv4.go b/pkg/tcpip/network/ipv4/ipv4.go index ad3ab738a..481a170dc 100644 --- a/pkg/tcpip/network/ipv4/ipv4.go +++ b/pkg/tcpip/network/ipv4/ipv4.go @@ -366,16 +366,15 @@ func (e *endpoint) disableLocked() { } } -// multicastEventDispatcher returns the multicast forwarding event dispatcher. -// -// Panics if a multicast forwarding event dispatcher does not exist. This -// indicates that multicast forwarding is enabled, but no dispatcher was -// provided. -func (e *endpoint) multicastEventDispatcher() stack.MulticastForwardingEventDispatcher { - if mcastDisp := e.protocol.options.MulticastForwardingDisp; mcastDisp != nil { - return mcastDisp +// emitMulticastEvent emits a multicast forwarding event using the provided +// generator if a valid event dispatcher exists. +func (e *endpoint) emitMulticastEvent(eventGenerator func(stack.MulticastForwardingEventDispatcher)) { + e.protocol.mu.RLock() + defer e.protocol.mu.RUnlock() + + if mcastDisp := e.protocol.multicastForwardingDisp; mcastDisp != nil { + eventGenerator(mcastDisp) } - panic("e.procotol.options.MulticastForwardingDisp unexpectedly nil") } // DefaultTTL is the default time-to-live value for this endpoint. @@ -902,9 +901,11 @@ func (e *endpoint) forwardMulticastPacket(h header.IPv4, pkt *stack.PacketBuffer // Attempt to forward the pkt using an existing route. return e.forwardValidatedMulticastPacket(pkt, result.InstalledRoute) case multicast.NoRouteFoundAndPendingInserted: - e.multicastEventDispatcher().OnMissingRoute(stack.MulticastPacketContext{ - stack.UnicastSourceAndMulticastDestination{h.SourceAddress(), h.DestinationAddress()}, - e.nic.ID(), + e.emitMulticastEvent(func(disp stack.MulticastForwardingEventDispatcher) { + disp.OnMissingRoute(stack.MulticastPacketContext{ + stack.UnicastSourceAndMulticastDestination{h.SourceAddress(), h.DestinationAddress()}, + e.nic.ID(), + }) }) case multicast.PacketQueuedInPendingRoute: default: @@ -957,10 +958,12 @@ func (e *endpoint) forwardValidatedMulticastPacket(pkt *stack.PacketBuffer, inst // dropped silently. if e.nic.ID() != installedRoute.ExpectedInputInterface { h := header.IPv4(pkt.NetworkHeader().View()) - e.multicastEventDispatcher().OnUnexpectedInputInterface(stack.MulticastPacketContext{ - stack.UnicastSourceAndMulticastDestination{h.SourceAddress(), h.DestinationAddress()}, - e.nic.ID(), - }, installedRoute.ExpectedInputInterface) + e.emitMulticastEvent(func(disp stack.MulticastForwardingEventDispatcher) { + disp.OnUnexpectedInputInterface(stack.MulticastPacketContext{ + stack.UnicastSourceAndMulticastDestination{h.SourceAddress(), h.DestinationAddress()}, + e.nic.ID(), + }, installedRoute.ExpectedInputInterface) + }) return &ip.ErrUnexpectedMulticastInputInterface{} } @@ -1046,7 +1049,7 @@ func (e *endpoint) handleValidatedPacket(h header.IPv4, pkt *stack.PacketBuffer, // RFC 1812 section 5.2.3 for details regarding the forwarding/local // delivery decision. - multicastForwarding := e.MulticastForwarding() + multicastForwarding := e.MulticastForwarding() && e.protocol.multicastForwarding() if multicastForwarding { e.handleForwardingError(e.forwardMulticastPacket(h, pkt)) @@ -1432,6 +1435,11 @@ type protocol struct { options Options multicastRouteTable multicast.RouteTable + // multicastForwardingDisp is the multicast forwarding event dispatcher that + // an integrator can provide to receive multicast forwarding events. Note + // that multicast packets will only be forwarded if this is non-nil. + // +checklocks:mu + multicastForwardingDisp stack.MulticastForwardingEventDispatcher } // Number returns the ipv4 protocol number. @@ -1505,6 +1513,12 @@ func (p *protocol) validateUnicastSourceAndMulticastDestination(addresses stack. return nil } +func (p *protocol) multicastForwarding() bool { + p.mu.RLock() + defer p.mu.RUnlock() + return p.multicastForwardingDisp != nil +} + func (p *protocol) newInstalledRoute(route stack.MulticastRoute) (*multicast.InstalledRoute, tcpip.Error) { if len(route.OutgoingInterfaces) == 0 { return nil, &tcpip.ErrMissingRequiredFields{} @@ -1528,6 +1542,10 @@ func (p *protocol) newInstalledRoute(route stack.MulticastRoute) (*multicast.Ins // AddMulticastRoute implements stack.MulticastForwardingNetworkProtocol. func (p *protocol) AddMulticastRoute(addresses stack.UnicastSourceAndMulticastDestination, route stack.MulticastRoute) tcpip.Error { + if !p.multicastForwarding() { + return &tcpip.ErrNotPermitted{} + } + if err := p.validateUnicastSourceAndMulticastDestination(addresses); err != nil { return err } @@ -1559,6 +1577,34 @@ func (p *protocol) RemoveMulticastRoute(addresses stack.UnicastSourceAndMulticas return nil } +// EnableMulticastForwarding implements +// stack.MulticastForwardingNetworkProtocol.EnableMulticastForwarding. +func (p *protocol) EnableMulticastForwarding(disp stack.MulticastForwardingEventDispatcher) (bool, tcpip.Error) { + p.mu.Lock() + defer p.mu.Unlock() + + if p.multicastForwardingDisp != nil { + return true, nil + } + + if disp == nil { + return false, &tcpip.ErrInvalidOptionValue{} + } + + p.multicastForwardingDisp = disp + return false, nil +} + +// DisableMulticastForwarding implements +// stack.MulticastForwardingNetworkProtocol.DisableMulticastForwarding. +func (p *protocol) DisableMulticastForwarding() { + p.mu.Lock() + defer p.mu.Unlock() + + p.multicastForwardingDisp = nil + p.multicastRouteTable.RemoveAllInstalledRoutes() +} + // MulticastRouteLastUsedTime implements // stack.MulticastForwardingNetworkProtocol. func (p *protocol) MulticastRouteLastUsedTime(addresses stack.UnicastSourceAndMulticastDestination) (tcpip.MonotonicTime, tcpip.Error) { @@ -1589,6 +1635,11 @@ func (p *protocol) forwardPendingMulticastPacket(pkt *stack.PacketBuffer, instal // drop the pkt. return } + + if !ep.MulticastForwarding() { + return + } + ep.handleForwardingError(ep.forwardValidatedMulticastPacket(pkt, installedRoute)) } @@ -1771,10 +1822,6 @@ type Options struct { // AllowExternalLoopbackTraffic indicates that inbound loopback packets (i.e. // martian loopback packets) should be accepted. AllowExternalLoopbackTraffic bool - - // MulticastForwardingDisp is the multicast forwarding event dispatcher that - // an integrator can provide to receive multicast forwarding events. - MulticastForwardingDisp stack.MulticastForwardingEventDispatcher } // NewProtocolWithOptions returns an IPv4 network protocol. diff --git a/pkg/tcpip/network/ipv4/ipv4_test.go b/pkg/tcpip/network/ipv4/ipv4_test.go index b067d1199..4d19d1df9 100644 --- a/pkg/tcpip/network/ipv4/ipv4_test.go +++ b/pkg/tcpip/network/ipv4/ipv4_test.go @@ -58,6 +58,15 @@ type testContext struct { clock *faketime.ManualClock } +var _ stack.MulticastForwardingEventDispatcher = (*fakeMulticastEventDispatcher)(nil) + +type fakeMulticastEventDispatcher struct{} + +func (m *fakeMulticastEventDispatcher) OnMissingRoute(context stack.MulticastPacketContext) {} + +func (m *fakeMulticastEventDispatcher) OnUnexpectedInputInterface(context stack.MulticastPacketContext, expectedInputInterface tcpip.NICID) { +} + func newTestContext() testContext { clock := faketime.NewManualClock() s := stack.New(stack.Options{ @@ -203,6 +212,10 @@ func TestAddMulticastRouteIPv4Errors(t *testing.T) { } } + if _, err := s.EnableMulticastForwardingForProtocol(ipv4.ProtocolNumber, &fakeMulticastEventDispatcher{}); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, _): (_, %s)", ipv4.ProtocolNumber, err) + } + outgoingInterfaces := []stack.MulticastRouteOutgoingInterface{{ID: outgoingNICID, MinTTL: 1}} addresses := stack.UnicastSourceAndMulticastDestination{ @@ -814,6 +827,10 @@ func TestMulticastFragmentForwarding(t *testing.T) { defer ctx.cleanup() s := ctx.s + if _, err := s.EnableMulticastForwardingForProtocol(ipv4.ProtocolNumber, &fakeMulticastEventDispatcher{}); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, _): (_, %s)", ipv4.ProtocolNumber, err) + } + endpoints := make(map[tcpip.NICID]*channel.Endpoint) for nicID, addr := range defaultEndpointConfigs { // For the input interface, we expect at most a single packet in @@ -983,6 +1000,10 @@ func TestMulticastForwardingOptions(t *testing.T) { // it give a more recognisable signature than 00,00,00,00. clock.Advance(time.Millisecond * randomTimeOffset) + if _, err := s.EnableMulticastForwardingForProtocol(ipv4.ProtocolNumber, &fakeMulticastEventDispatcher{}); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, _): (_, %s)", ipv4.ProtocolNumber, err) + } + endpoints := make(map[tcpip.NICID]*channel.Endpoint) for nicID, addr := range defaultEndpointConfigs { ep := channel.New(1, ipv4.MaxTotalSize, "") diff --git a/pkg/tcpip/network/ipv6/icmp_test.go b/pkg/tcpip/network/ipv6/icmp_test.go index 897b21ba0..c58818bcf 100644 --- a/pkg/tcpip/network/ipv6/icmp_test.go +++ b/pkg/tcpip/network/ipv6/icmp_test.go @@ -199,15 +199,6 @@ func handleICMPInIPv6(ep stack.NetworkEndpoint, src, dst tcpip.Address, icmp hea pkt.DecRef() } -var _ stack.MulticastForwardingEventDispatcher = (*fakeMulticastEventDispatcher)(nil) - -type fakeMulticastEventDispatcher struct{} - -func (m *fakeMulticastEventDispatcher) OnMissingRoute(context stack.MulticastPacketContext) {} - -func (m *fakeMulticastEventDispatcher) OnUnexpectedInputInterface(context stack.MulticastPacketContext, expectedInputInterface tcpip.NICID) { -} - type testContext struct { s *stack.Stack clock *faketime.ManualClock @@ -216,7 +207,7 @@ type testContext struct { func newTestContext() testContext { clock := faketime.NewManualClock() s := stack.New(stack.Options{ - NetworkProtocols: []stack.NetworkProtocolFactory{NewProtocolWithOptions(Options{MulticastForwardingDisp: &fakeMulticastEventDispatcher{}})}, + NetworkProtocols: []stack.NetworkProtocolFactory{NewProtocol}, TransportProtocols: []stack.TransportProtocolFactory{icmp.NewProtocol6, udp.NewProtocol}, Clock: clock, }) diff --git a/pkg/tcpip/network/ipv6/ipv6.go b/pkg/tcpip/network/ipv6/ipv6.go index fe6469da5..40d6bd511 100644 --- a/pkg/tcpip/network/ipv6/ipv6.go +++ b/pkg/tcpip/network/ipv6/ipv6.go @@ -1128,9 +1128,11 @@ func (e *endpoint) forwardMulticastPacket(h header.IPv6, pkt *stack.PacketBuffer // Attempt to forward the pkt using an existing route. return e.forwardValidatedMulticastPacket(pkt, result.InstalledRoute) case multicast.NoRouteFoundAndPendingInserted: - e.multicastEventDispatcher().OnMissingRoute(stack.MulticastPacketContext{ - stack.UnicastSourceAndMulticastDestination{h.SourceAddress(), h.DestinationAddress()}, - e.nic.ID(), + e.emitMulticastEvent(func(disp stack.MulticastForwardingEventDispatcher) { + disp.OnMissingRoute(stack.MulticastPacketContext{ + stack.UnicastSourceAndMulticastDestination{h.SourceAddress(), h.DestinationAddress()}, + e.nic.ID(), + }) }) case multicast.PacketQueuedInPendingRoute: default: @@ -1152,10 +1154,12 @@ func (e *endpoint) forwardValidatedMulticastPacket(pkt *stack.PacketBuffer, inst // dropped silently. if e.nic.ID() != installedRoute.ExpectedInputInterface { h := header.IPv6(pkt.NetworkHeader().View()) - e.multicastEventDispatcher().OnUnexpectedInputInterface(stack.MulticastPacketContext{ - stack.UnicastSourceAndMulticastDestination{h.SourceAddress(), h.DestinationAddress()}, - e.nic.ID(), - }, installedRoute.ExpectedInputInterface) + e.emitMulticastEvent(func(disp stack.MulticastForwardingEventDispatcher) { + disp.OnUnexpectedInputInterface(stack.MulticastPacketContext{ + stack.UnicastSourceAndMulticastDestination{h.SourceAddress(), h.DestinationAddress()}, + e.nic.ID(), + }, installedRoute.ExpectedInputInterface) + }) return &ip.ErrUnexpectedMulticastInputInterface{} } @@ -1258,7 +1262,7 @@ func (e *endpoint) handleValidatedPacket(h header.IPv6, pkt *stack.PacketBuffer, // RFC 1812 section 5.2.3 for details regarding the forwarding/local // delivery decision. - multicastForwading := e.MulticastForwarding() + multicastForwading := e.MulticastForwarding() && e.protocol.multicastForwarding() if multicastForwading { e.handleForwardingError(e.forwardMulticastPacket(h, pkt)) @@ -2126,6 +2130,11 @@ type protocol struct { // ICMP types for which the stack's global rate limiting must apply. icmpRateLimitedTypes map[header.ICMPv6Type]struct{} + + // multicastForwardingDisp is the multicast forwarding event dispatcher that + // an integrator can provide to receive multicast forwarding events. Note + // that multicast packets will only be forwarded if this is non-nil. + multicastForwardingDisp stack.MulticastForwardingEventDispatcher } ids []atomicbitops.Uint32 @@ -2267,16 +2276,14 @@ func (p *protocol) DefaultTTL() uint8 { return uint8(p.defaultTTL.Load()) } -// multicastEventDispatcher returns the multicast forwarding event dispatcher. -// -// Panics if a multicast forwarding event dispatcher does not exist. This -// indicates that multicast forwarding is enabled, but no dispatcher was -// provided. -func (e *endpoint) multicastEventDispatcher() stack.MulticastForwardingEventDispatcher { - if mcastDisp := e.protocol.options.MulticastForwardingDisp; mcastDisp != nil { - return mcastDisp +// emitMulticastEvent emits a multicast forwarding event using the provided +// generator if a valid event dispatcher exists. +func (e *endpoint) emitMulticastEvent(eventGenerator func(stack.MulticastForwardingEventDispatcher)) { + e.protocol.mu.RLock() + defer e.protocol.mu.RUnlock() + if mcastDisp := e.protocol.mu.multicastForwardingDisp; mcastDisp != nil { + eventGenerator(mcastDisp) } - panic("e.procotol.options.MulticastForwardingDisp unexpectedly nil") } // Close implements stack.TransportProtocol. @@ -2297,6 +2304,12 @@ func validateUnicastSourceAndMulticastDestination(addresses stack.UnicastSourceA return nil } +func (p *protocol) multicastForwarding() bool { + p.mu.RLock() + defer p.mu.RUnlock() + return p.mu.multicastForwardingDisp != nil +} + func (p *protocol) newInstalledRoute(route stack.MulticastRoute) (*multicast.InstalledRoute, tcpip.Error) { if len(route.OutgoingInterfaces) == 0 { return nil, &tcpip.ErrMissingRequiredFields{} @@ -2320,6 +2333,10 @@ func (p *protocol) newInstalledRoute(route stack.MulticastRoute) (*multicast.Ins // AddMulticastRoute implements stack.MulticastForwardingNetworkProtocol. func (p *protocol) AddMulticastRoute(addresses stack.UnicastSourceAndMulticastDestination, route stack.MulticastRoute) tcpip.Error { + if !p.multicastForwarding() { + return &tcpip.ErrNotPermitted{} + } + if err := validateUnicastSourceAndMulticastDestination(addresses); err != nil { return err } @@ -2367,6 +2384,33 @@ func (p *protocol) MulticastRouteLastUsedTime(addresses stack.UnicastSourceAndMu return timestamp, nil } +// EnableMulticastForwarding implements +// stack.MulticastForwardingNetworkProtocol.EnableMulticastForwarding. +func (p *protocol) EnableMulticastForwarding(disp stack.MulticastForwardingEventDispatcher) (bool, tcpip.Error) { + p.mu.Lock() + defer p.mu.Unlock() + + if p.mu.multicastForwardingDisp != nil { + return true, nil + } + + if disp == nil { + return false, &tcpip.ErrInvalidOptionValue{} + } + + p.mu.multicastForwardingDisp = disp + return false, nil +} + +// DisableMulticastForwarding implements +// stack.MulticastForwardingNetworkProtocol.DisableMulticastForwarding. +func (p *protocol) DisableMulticastForwarding() { + p.mu.Lock() + defer p.mu.Unlock() + p.mu.multicastForwardingDisp = nil + p.multicastRouteTable.RemoveAllInstalledRoutes() +} + func (p *protocol) forwardPendingMulticastPacket(pkt *stack.PacketBuffer, installedRoute *multicast.InstalledRoute) { defer pkt.DecRef() @@ -2382,6 +2426,10 @@ func (p *protocol) forwardPendingMulticastPacket(pkt *stack.PacketBuffer, instal return } + if !ep.MulticastForwarding() { + return + } + ep.handleForwardingError(ep.forwardValidatedMulticastPacket(pkt, installedRoute)) } @@ -2543,10 +2591,6 @@ type Options struct { // AllowExternalLoopbackTraffic indicates that inbound loopback packets (i.e. // martian loopback packets) should be accepted. AllowExternalLoopbackTraffic bool - - // MulticastForwardingDisp is the multicast forwarding event dispatcher that - // an integrator can provide to receive multicast forwarding events. - MulticastForwardingDisp stack.MulticastForwardingEventDispatcher } // NewProtocolWithOptions returns an IPv6 network protocol. diff --git a/pkg/tcpip/network/ipv6/ipv6_test.go b/pkg/tcpip/network/ipv6/ipv6_test.go index 11ff88fdb..910833dd8 100644 --- a/pkg/tcpip/network/ipv6/ipv6_test.go +++ b/pkg/tcpip/network/ipv6/ipv6_test.go @@ -59,6 +59,15 @@ const ( extraHeaderReserve = 50 ) +var _ stack.MulticastForwardingEventDispatcher = (*fakeMulticastEventDispatcher)(nil) + +type fakeMulticastEventDispatcher struct{} + +func (m *fakeMulticastEventDispatcher) OnMissingRoute(context stack.MulticastPacketContext) {} + +func (m *fakeMulticastEventDispatcher) OnUnexpectedInputInterface(context stack.MulticastPacketContext, expectedInputInterface tcpip.NICID) { +} + // testReceiveICMP tests receiving an ICMP packet from src to dst. want is the // expected Neighbor Advertisement received count after receiving the packet. func testReceiveICMP(t *testing.T, s *stack.Stack, e *channel.Endpoint, src, dst tcpip.Address, want uint64) { @@ -3440,6 +3449,10 @@ func TestMulticastForwarding(t *testing.T) { defer c.cleanup() s := c.s + if _, err := s.EnableMulticastForwardingForProtocol(ProtocolNumber, &fakeMulticastEventDispatcher{}); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, _): (_, %s)", ProtocolNumber, err) + } + endpoints := make(map[tcpip.NICID]*channel.Endpoint) for nicID, addr := range defaultEndpointConfigs { ep := channel.New(1, header.IPv6MinimumMTU, "") diff --git a/pkg/tcpip/stack/registration.go b/pkg/tcpip/stack/registration.go index a08d1c899..64a438993 100644 --- a/pkg/tcpip/stack/registration.go +++ b/pkg/tcpip/stack/registration.go @@ -816,6 +816,16 @@ type MulticastForwardingNetworkProtocol interface { // Returns an error if the addresses are invalid or a matching route was not // found. MulticastRouteLastUsedTime(UnicastSourceAndMulticastDestination) (tcpip.MonotonicTime, tcpip.Error) + + // EnableMulticastForwarding enables multicast forwarding for the protocol. + // + // Returns an error if the provided multicast forwarding event dispatcher is + // nil. Otherwise, returns true if the multicast forwarding was already + // enabled. + EnableMulticastForwarding(MulticastForwardingEventDispatcher) (bool, tcpip.Error) + + // DisableMulticastForwarding disables multicast forwarding for the protocol. + DisableMulticastForwarding() } // MulticastPacketContext is the context in which a multicast packet triggered diff --git a/pkg/tcpip/stack/stack.go b/pkg/tcpip/stack/stack.go index a741f0251..91cef1f23 100644 --- a/pkg/tcpip/stack/stack.go +++ b/pkg/tcpip/stack/stack.go @@ -616,6 +616,52 @@ func (s *Stack) MulticastRouteLastUsedTime(protocol tcpip.NetworkProtocolNumber, return forwardingNetProto.MulticastRouteLastUsedTime(addresses) } +// EnableMulticastForwardingForProtocol enables multicast forwarding for the +// provided protocol. +// +// Returns true if forwarding was already enabled on the protocol. +// Additionally, returns an error if: +// +// - The protocol is not found. +// - The protocol doesn't support multicast forwarding. +// - The multicast forwarding event dispatcher is nil. +// +// If successful, future multicast forwarding events will be sent to the +// provided event dispatcher. +func (s *Stack) EnableMulticastForwardingForProtocol(protocol tcpip.NetworkProtocolNumber, disp MulticastForwardingEventDispatcher) (bool, tcpip.Error) { + netProto, ok := s.networkProtocols[protocol] + if !ok { + return false, &tcpip.ErrUnknownProtocol{} + } + + forwardingNetProto, ok := netProto.(MulticastForwardingNetworkProtocol) + if !ok { + return false, &tcpip.ErrNotSupported{} + } + + return forwardingNetProto.EnableMulticastForwarding(disp) +} + +// DisableMulticastForwardingForProtocol disables multicast forwarding for the +// provided protocol. +// +// Returns an error if the provided protocol is not found or if it does not +// support multicast forwarding. +func (s *Stack) DisableMulticastForwardingForProtocol(protocol tcpip.NetworkProtocolNumber) tcpip.Error { + netProto, ok := s.networkProtocols[protocol] + if !ok { + return &tcpip.ErrUnknownProtocol{} + } + + forwardingNetProto, ok := netProto.(MulticastForwardingNetworkProtocol) + if !ok { + return &tcpip.ErrNotSupported{} + } + + forwardingNetProto.DisableMulticastForwarding() + return nil +} + // SetNICMulticastForwarding enables or disables multicast packet forwarding on // the specified NIC for the passed protocol. // diff --git a/pkg/tcpip/stack/stack_test.go b/pkg/tcpip/stack/stack_test.go index f04c93557..e8d289942 100644 --- a/pkg/tcpip/stack/stack_test.go +++ b/pkg/tcpip/stack/stack_test.go @@ -231,6 +231,11 @@ type addMulticastRouteData struct { route stack.MulticastRoute } +type enableMulticastForwardingForProtocolResult struct { + AlreadyEnabled bool + Err tcpip.Error +} + // fakeNetworkProtocol is a network-layer protocol descriptor. It aggregates the // number of packets sent and received via endpoints of this protocol. The index // where packets are added is given by the packet's destination address MOD 10. @@ -244,6 +249,9 @@ type fakeNetworkProtocol struct { addMulticastRouteData addMulticastRouteData multicastRouteLastUsedTimeData stack.UnicastSourceAndMulticastDestination removeMulticastRouteData stack.UnicastSourceAndMulticastDestination + + enableMulticastForwardingForProtocolResult enableMulticastForwardingForProtocolResult + disableMulticastForwardingForProtocolCalled bool } func (*fakeNetworkProtocol) Number() tcpip.NetworkProtocolNumber { @@ -329,6 +337,18 @@ func (f *fakeNetworkProtocol) MulticastRouteLastUsedTime(addresses stack.Unicast return tcpip.MonotonicTime{}, nil } +// EnableMulticastForwarding implements +// MulticastForwardingNetworkProtocol.EnableMulticastForwarding. +func (f *fakeNetworkProtocol) EnableMulticastForwarding(stack.MulticastForwardingEventDispatcher) (bool, tcpip.Error) { + return f.enableMulticastForwardingForProtocolResult.AlreadyEnabled, f.enableMulticastForwardingForProtocolResult.Err +} + +// DisableMulticastForwarding implements +// MulticastForwardingNetworkProtocol.DisableMulticastForwarding. +func (f *fakeNetworkProtocol) DisableMulticastForwarding() { + f.disableMulticastForwardingForProtocolCalled = true +} + // Forwarding implements stack.ForwardingNetworkEndpoint. func (f *fakeNetworkEndpoint) Forwarding() bool { f.mu.RLock() @@ -382,6 +402,17 @@ func (l *linkEPWithMockedAttach) isAttached() bool { return l.attached } +var _ stack.MulticastForwardingEventDispatcher = (*fakeMulticastEventDispatcher)(nil) + +type fakeMulticastEventDispatcher struct { +} + +func (m *fakeMulticastEventDispatcher) OnMissingRoute(context stack.MulticastPacketContext) { +} + +func (m *fakeMulticastEventDispatcher) OnUnexpectedInputInterface(context stack.MulticastPacketContext, expectedInputInterface tcpip.NICID) { +} + // Checks to see if list contains an address. func containsAddr(list []tcpip.ProtocolAddress, item tcpip.ProtocolAddress) bool { for _, i := range list { @@ -4860,6 +4891,116 @@ func TestMulticastRouteLastUsedTime(t *testing.T) { } } +func TestEnableMulticastForwardingForProtocol(t *testing.T) { + tests := []struct { + name string + netProto tcpip.NetworkProtocolNumber + factory stack.NetworkProtocolFactory + delegateOutput enableMulticastForwardingForProtocolResult + wantResult enableMulticastForwardingForProtocolResult + }{ + { + name: "impl returns previously enabled", + netProto: fakeNetNumber, + factory: fakeNetFactory, + delegateOutput: enableMulticastForwardingForProtocolResult{true, nil}, + wantResult: enableMulticastForwardingForProtocolResult{true, nil}, + }, + { + name: "impl returns previously disabled", + netProto: fakeNetNumber, + factory: fakeNetFactory, + delegateOutput: enableMulticastForwardingForProtocolResult{false, nil}, + wantResult: enableMulticastForwardingForProtocolResult{false, nil}, + }, + { + name: "impl returns error", + netProto: fakeNetNumber, + factory: fakeNetFactory, + delegateOutput: enableMulticastForwardingForProtocolResult{false, &tcpip.ErrUnknownDevice{}}, + wantResult: enableMulticastForwardingForProtocolResult{false, &tcpip.ErrUnknownDevice{}}, + }, + { + name: "unknown protocol", + factory: fakeNetFactory, + netProto: arp.ProtocolNumber, + wantResult: enableMulticastForwardingForProtocolResult{false, &tcpip.ErrUnknownProtocol{}}, + }, + { + name: "not supported", + factory: arp.NewProtocol, + netProto: arp.ProtocolNumber, + wantResult: enableMulticastForwardingForProtocolResult{false, &tcpip.ErrNotSupported{}}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + s := stack.New(stack.Options{ + NetworkProtocols: []stack.NetworkProtocolFactory{test.factory}, + }) + + if test.netProto == fakeNetNumber { + fakeNet := s.NetworkProtocolInstance(fakeNetNumber).(*fakeNetworkProtocol) + fakeNet.enableMulticastForwardingForProtocolResult = test.delegateOutput + } + + alreadyEnabled, err := s.EnableMulticastForwardingForProtocol(test.netProto, &fakeMulticastEventDispatcher{}) + + if !cmp.Equal(enableMulticastForwardingForProtocolResult{alreadyEnabled, err}, test.wantResult, cmpopts.EquateErrors()) { + t.Errorf("s.EnableMulticastForwardingForProtocol(%d, _) = (%t, %s), want = (%t, %s)", test.netProto, alreadyEnabled, err, test.wantResult.AlreadyEnabled, test.wantResult.Err) + } + }) + } +} + +func TestDisableMulticastForwardingForProtocol(t *testing.T) { + tests := []struct { + name string + netProto tcpip.NetworkProtocolNumber + factory stack.NetworkProtocolFactory + wantErr tcpip.Error + }{ + { + name: "valid", + netProto: fakeNetNumber, + factory: fakeNetFactory, + wantErr: nil, + }, + { + name: "unknown protocol", + factory: fakeNetFactory, + netProto: arp.ProtocolNumber, + wantErr: &tcpip.ErrUnknownProtocol{}, + }, + { + name: "not supported", + factory: arp.NewProtocol, + netProto: arp.ProtocolNumber, + wantErr: &tcpip.ErrNotSupported{}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + s := stack.New(stack.Options{ + NetworkProtocols: []stack.NetworkProtocolFactory{test.factory}, + }) + + err := s.DisableMulticastForwardingForProtocol(test.netProto) + + if !cmp.Equal(err, test.wantErr, cmpopts.EquateErrors()) { + t.Errorf("s.DisableMulticastForwardingForProtocol(%d) = %s, want = %s", test.netProto, err, test.wantErr) + } + + if err == nil { + fakeNet := s.NetworkProtocolInstance(fakeNetNumber).(*fakeNetworkProtocol) + if !fakeNet.disableMulticastForwardingForProtocolCalled { + t.Errorf("fakeNet.disableMulticastForwardingForProtocolCalled = false, want = true") + } + } + }) + } +} + func TestNICForwarding(t *testing.T) { const nicID = 1 diff --git a/pkg/tcpip/tests/integration/multicast_forward_test.go b/pkg/tcpip/tests/integration/multicast_forward_test.go index eb6edf7d0..33559b5b9 100644 --- a/pkg/tcpip/tests/integration/multicast_forward_test.go +++ b/pkg/tcpip/tests/integration/multicast_forward_test.go @@ -231,15 +231,26 @@ func TestAddMulticastRoute(t *testing.T) { otherNICID: otherEndpointAddr, } + type multicastForwardingEvent int + const ( + enabledForProtocol multicastForwardingEvent = iota + enabledForNIC + injectPendingPacket + ) + + type multicastForwardingStateBeforeAddRouteCalled struct { + multicastForwardingEvents []multicastForwardingEvent + } + tests := []struct { - name string - srcAddr, dstAddr addrType - routeIncomingNICID tcpip.NICID - routeOutgoingNICID tcpip.NICID - omitOutgoingInterfaces bool - injectPendingPacket bool - expectForward bool - wantErr tcpip.Error + name string + srcAddr, dstAddr addrType + routeIncomingNICID tcpip.NICID + routeOutgoingNICID tcpip.NICID + omitOutgoingInterfaces bool + multicastForwardingEventsBeforeAddRouteCalled []multicastForwardingEvent + expectForward bool + wantErr tcpip.Error }{ { name: "no pending packets", @@ -247,16 +258,26 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, - wantErr: nil, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: nil, }, { - name: "pending packet forwarded", - srcAddr: remoteUnicastAddr, - dstAddr: multicastAddr, - routeIncomingNICID: incomingNICID, - routeOutgoingNICID: outgoingNICID, - injectPendingPacket: true, - expectForward: true, + name: "packet arrived after forwarding enabled but before add route called", + srcAddr: remoteUnicastAddr, + dstAddr: multicastAddr, + routeIncomingNICID: incomingNICID, + routeOutgoingNICID: outgoingNICID, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol, injectPendingPacket}, + expectForward: true, + }, + { + name: "packet arrived before multicast forwarding enabled", + srcAddr: remoteUnicastAddr, + dstAddr: multicastAddr, + routeIncomingNICID: incomingNICID, + routeOutgoingNICID: outgoingNICID, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, injectPendingPacket, enabledForProtocol}, + expectForward: false, }, { name: "unexpected input interface", @@ -264,9 +285,28 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, // The added route's incoming NICID does not match the pending packet's // incoming NICID. As a result, the packet should not be forwarded. - routeIncomingNICID: otherNICID, - routeOutgoingNICID: outgoingNICID, - injectPendingPacket: true, + routeIncomingNICID: otherNICID, + routeOutgoingNICID: outgoingNICID, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + }, + { + name: "multicast forwarding disabled for NIC", + srcAddr: remoteUnicastAddr, + dstAddr: multicastAddr, + routeIncomingNICID: incomingNICID, + routeOutgoingNICID: outgoingNICID, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForProtocol}, + expectForward: false, + wantErr: nil, + }, + { + name: "multicast forwarding disabled for protocol", + srcAddr: remoteUnicastAddr, + dstAddr: multicastAddr, + routeIncomingNICID: incomingNICID, + routeOutgoingNICID: outgoingNICID, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC}, + wantErr: &tcpip.ErrNotPermitted{}, }, { name: "multicast source", @@ -274,7 +314,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, - wantErr: &tcpip.ErrBadAddress{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrBadAddress{}, }, { name: "any source", @@ -282,7 +323,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, - wantErr: &tcpip.ErrBadAddress{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrBadAddress{}, }, { name: "link-local unicast source", @@ -290,7 +332,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, - wantErr: &tcpip.ErrBadAddress{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrBadAddress{}, }, { name: "empty source", @@ -298,7 +341,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, - wantErr: &tcpip.ErrBadAddress{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrBadAddress{}, }, { name: "unicast destination", @@ -306,7 +350,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: remoteUnicastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, - wantErr: &tcpip.ErrBadAddress{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrBadAddress{}, }, { name: "empty destination", @@ -314,7 +359,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: emptyAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, - wantErr: &tcpip.ErrBadAddress{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrBadAddress{}, }, { name: "link-local multicast destination", @@ -322,7 +368,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: linkLocalMulticastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, - wantErr: &tcpip.ErrBadAddress{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrBadAddress{}, }, { name: "unknown input NICID", @@ -330,7 +377,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, routeIncomingNICID: unknownNICID, routeOutgoingNICID: outgoingNICID, - wantErr: &tcpip.ErrUnknownNICID{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrUnknownNICID{}, }, { name: "unknown output NICID", @@ -338,7 +386,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: unknownNICID, - wantErr: &tcpip.ErrUnknownNICID{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrUnknownNICID{}, }, { name: "input NIC matches output NIC", @@ -346,7 +395,8 @@ func TestAddMulticastRoute(t *testing.T) { dstAddr: multicastAddr, routeIncomingNICID: incomingNICID, routeOutgoingNICID: incomingNICID, - wantErr: &tcpip.ErrMulticastInputCannotBeOutput{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrMulticastInputCannotBeOutput{}, }, { name: "empty outgoing interfaces", @@ -355,7 +405,8 @@ func TestAddMulticastRoute(t *testing.T) { routeIncomingNICID: incomingNICID, routeOutgoingNICID: outgoingNICID, omitOutgoingInterfaces: true, - wantErr: &tcpip.ErrMissingRequiredFields{}, + multicastForwardingEventsBeforeAddRouteCalled: []multicastForwardingEvent{enabledForNIC, enabledForProtocol}, + wantErr: &tcpip.ErrMissingRequiredFields{}, }, } @@ -364,10 +415,7 @@ func TestAddMulticastRoute(t *testing.T) { t.Run(fmt.Sprintf("%s %d", test.name, protocol), func(t *testing.T) { eventDispatcher := &fakeMulticastEventDispatcher{} s := stack.New(stack.Options{ - NetworkProtocols: []stack.NetworkProtocolFactory{ - ipv4.NewProtocolWithOptions(ipv4.Options{MulticastForwardingDisp: eventDispatcher}), - ipv6.NewProtocolWithOptions(ipv6.Options{MulticastForwardingDisp: eventDispatcher}), - }, + NetworkProtocols: []stack.NetworkProtocolFactory{ipv4.NewProtocol, ipv6.NewProtocol}, }) defer s.Close() @@ -386,25 +434,37 @@ func TestAddMulticastRoute(t *testing.T) { if err := s.AddProtocolAddress(nicID, addr, stack.AddressProperties{}); err != nil { t.Fatalf("s.AddProtocolAddress(%d, %#v, {}): %s", nicID, addr, err) } - s.SetNICMulticastForwarding(nicID, protocol, true /* enabled */) endpoints[nicID] = ep } srcAddr := getAddr(protocol, test.srcAddr) dstAddr := getAddr(protocol, test.dstAddr) - if test.injectPendingPacket { - incomingEp, ok := endpoints[incomingNICID] - if !ok { - t.Fatalf("got endpoints[%d] = (_, false), want (_, true)", incomingNICID) - } + for _, event := range test.multicastForwardingEventsBeforeAddRouteCalled { + switch event { + case enabledForNIC: + for nicID := range endpoints { + s.SetNICMulticastForwarding(nicID, protocol, true /* enable */) + } + case enabledForProtocol: + if _, err := s.EnableMulticastForwardingForProtocol(protocol, eventDispatcher); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, _): (_, %s)", protocol, err) + } + case injectPendingPacket: + incomingEp, ok := endpoints[incomingNICID] + if !ok { + t.Fatalf("got endpoints[%d] = (_, false), want (_, true)", incomingNICID) + } - injectPacket(incomingEp, protocol, srcAddr, dstAddr, packetTTL) - p := incomingEp.Read() + injectPacket(incomingEp, protocol, srcAddr, dstAddr, packetTTL) + p := incomingEp.Read() - if p != nil { - // An ICMP error should never be sent in response to a multicast packet. - t.Fatalf("got incomingEp.Read() = %#v, want = nil", p) + if p != nil { + // An ICMP error should never be sent in response to a multicast packet. + t.Fatalf("got incomingEp.Read() = %#v, want = nil", p) + } + default: + panic(fmt.Sprintf("unsupported multicastForwardingEvent: %d", event)) } } @@ -451,6 +511,56 @@ func TestAddMulticastRoute(t *testing.T) { } } +func TestEnableMulticastForwardingE(t *testing.T) { + eventDispatcher := &fakeMulticastEventDispatcher{} + + type enableMulticastForwardingResult struct { + AlreadyEnabled bool + Err tcpip.Error + } + + tests := []struct { + name string + eventDispatcher stack.MulticastForwardingEventDispatcher + wantResult []enableMulticastForwardingResult + }{ + { + name: "success", + eventDispatcher: eventDispatcher, + wantResult: []enableMulticastForwardingResult{{false, nil}}, + }, + { + name: "already enabled", + eventDispatcher: eventDispatcher, + wantResult: []enableMulticastForwardingResult{{false, nil}, {true, nil}}, + }, + { + name: "invalid event dispatcher", + eventDispatcher: nil, + wantResult: []enableMulticastForwardingResult{{false, &tcpip.ErrInvalidOptionValue{}}}, + }, + } + for _, test := range tests { + for _, protocol := range []tcpip.NetworkProtocolNumber{ipv4.ProtocolNumber, ipv6.ProtocolNumber} { + t.Run(fmt.Sprintf("%s %d", test.name, protocol), func(t *testing.T) { + s := stack.New(stack.Options{ + NetworkProtocols: []stack.NetworkProtocolFactory{ipv4.NewProtocol, ipv6.NewProtocol}, + TransportProtocols: []stack.TransportProtocolFactory{udp.NewProtocol}, + }) + defer s.Close() + + for _, wantResult := range test.wantResult { + alreadyEnabled, err := s.EnableMulticastForwardingForProtocol(protocol, test.eventDispatcher) + result := enableMulticastForwardingResult{alreadyEnabled, err} + if !cmp.Equal(result, wantResult, cmpopts.EquateErrors()) { + t.Errorf("s.EnableMulticastForwardingForProtocol(%d, %#v) = (%t, %s), want = (%t, %s)", protocol, test.eventDispatcher, alreadyEnabled, err, wantResult.AlreadyEnabled, wantResult.Err) + } + } + }) + } + } +} + func TestMulticastRouteLastUsedTime(t *testing.T) { endpointConfigs := map[tcpip.NICID]endpointAddrType{ incomingNICID: incomingEndpointAddr, @@ -530,6 +640,10 @@ func TestMulticastRouteLastUsedTime(t *testing.T) { }) defer s.Close() + if _, err := s.EnableMulticastForwardingForProtocol(protocol, &fakeMulticastEventDispatcher{}); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, _): (_, %s)", protocol, err) + } + endpoints := make(map[tcpip.NICID]*channel.Endpoint) for nicID, addrType := range endpointConfigs { ep := channel.New(1, ipv4.MaxTotalSize, "") @@ -676,16 +790,16 @@ func TestRemoveMulticastRoute(t *testing.T) { for _, test := range tests { for _, protocol := range []tcpip.NetworkProtocolNumber{ipv4.ProtocolNumber, ipv6.ProtocolNumber} { t.Run(fmt.Sprintf("%s %d", test.name, protocol), func(t *testing.T) { - eventDispatcher := &fakeMulticastEventDispatcher{} s := stack.New(stack.Options{ - NetworkProtocols: []stack.NetworkProtocolFactory{ - ipv4.NewProtocolWithOptions(ipv4.Options{MulticastForwardingDisp: eventDispatcher}), - ipv6.NewProtocolWithOptions(ipv6.Options{MulticastForwardingDisp: eventDispatcher}), - }, + NetworkProtocols: []stack.NetworkProtocolFactory{ipv4.NewProtocol, ipv6.NewProtocol}, TransportProtocols: []stack.TransportProtocolFactory{udp.NewProtocol}, }) defer s.Close() + if _, err := s.EnableMulticastForwardingForProtocol(protocol, &fakeMulticastEventDispatcher{}); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, _): (_, %s)", protocol, err) + } + endpoints := make(map[tcpip.NICID]*channel.Endpoint) for nicID, addrType := range endpointConfigs { ep := channel.New(1, ipv4.MaxTotalSize, "") @@ -791,16 +905,17 @@ func TestMulticastForwarding(t *testing.T) { } tests := []struct { - name string - dstAddr addrType - ttl uint8 - routeInputInterface tcpip.NICID - disableMulticastForwarding bool - removeOutputInterface tcpip.NICID - expectMissingRouteEvent bool - expectUnexpectedInputInterfaceEvent bool - joinMulticastGroup bool - expectedForwardingInterfaces []tcpip.NICID + name string + dstAddr addrType + ttl uint8 + routeInputInterface tcpip.NICID + disableMulticastForwardingForNIC bool + updateMulticastForwardingForProtocol func(*testing.T, *stack.Stack, tcpip.NetworkProtocolNumber, stack.MulticastForwardingEventDispatcher) + removeOutputInterface tcpip.NICID + expectMissingRouteEvent bool + expectUnexpectedInputInterfaceEvent bool + joinMulticastGroup bool + expectedForwardingInterfaces []tcpip.NICID }{ { name: "forward only", @@ -826,13 +941,39 @@ func TestMulticastForwarding(t *testing.T) { expectedForwardingInterfaces: []tcpip.NICID{}, }, { - name: "multicast forwarding disabled", - disableMulticastForwarding: true, - dstAddr: multicastAddr, + name: "multicast forwarding disabled for NIC", + disableMulticastForwardingForNIC: true, + dstAddr: multicastAddr, + ttl: packetTTL, + routeInputInterface: incomingNICID, + expectedForwardingInterfaces: []tcpip.NICID{}, + }, + { + name: "multicast forwarding disabled for protocol", + dstAddr: multicastAddr, + updateMulticastForwardingForProtocol: func(t *testing.T, s *stack.Stack, protocol tcpip.NetworkProtocolNumber, disp stack.MulticastForwardingEventDispatcher) { + s.DisableMulticastForwardingForProtocol(protocol) + }, ttl: packetTTL, routeInputInterface: incomingNICID, expectedForwardingInterfaces: []tcpip.NICID{}, }, + { + name: "route table cleared after multicast forwarding disabled for protocol", + dstAddr: multicastAddr, + updateMulticastForwardingForProtocol: func(t *testing.T, s *stack.Stack, protocol tcpip.NetworkProtocolNumber, disp stack.MulticastForwardingEventDispatcher) { + t.Helper() + + s.DisableMulticastForwardingForProtocol(protocol) + if _, err := s.EnableMulticastForwardingForProtocol(protocol, disp); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, _): (_, %s)", protocol, err) + } + }, + ttl: packetTTL, + routeInputInterface: incomingNICID, + expectMissingRouteEvent: true, + expectedForwardingInterfaces: []tcpip.NICID{}, + }, { name: "unexpected input interface", dstAddr: multicastAddr, @@ -892,14 +1033,20 @@ func TestMulticastForwarding(t *testing.T) { t.Run(fmt.Sprintf("%s %d", test.name, protocol), func(t *testing.T) { s := stack.New(stack.Options{ - NetworkProtocols: []stack.NetworkProtocolFactory{ - ipv4.NewProtocolWithOptions(ipv4.Options{MulticastForwardingDisp: ipv4EventDispatcher}), - ipv6.NewProtocolWithOptions(ipv6.Options{MulticastForwardingDisp: ipv6EventDispatcher}), - }, + NetworkProtocols: []stack.NetworkProtocolFactory{ipv4.NewProtocol, ipv6.NewProtocol}, TransportProtocols: []stack.TransportProtocolFactory{udp.NewProtocol}, }) defer s.Close() + eventDispatcher, ok := eventDispatchers[protocol] + if !ok { + t.Fatalf("eventDispatchers[%d] = (_, false), want (_, true)", protocol) + } + + if _, err := s.EnableMulticastForwardingForProtocol(protocol, eventDispatcher); err != nil { + t.Fatalf("s.EnableMulticastForwardingForProtocol(%d, %#v): (_, %s)", protocol, eventDispatcher, err) + } + endpoints := make(map[tcpip.NICID]*channel.Endpoint) for nicID, addrType := range endpointConfigs { ep := channel.New(1, ipv4.MaxTotalSize, "") @@ -916,7 +1063,7 @@ func TestMulticastForwarding(t *testing.T) { t.Fatalf("s.AddProtocolAddress(%d, %+v, {}): %s", nicID, addr, err) } - s.SetNICMulticastForwarding(nicID, protocol, !test.disableMulticastForwarding) + s.SetNICMulticastForwarding(nicID, protocol, true /* enable */) endpoints[nicID] = ep } @@ -945,6 +1092,16 @@ func TestMulticastForwarding(t *testing.T) { t.Fatalf("AddMulticastRoute(%d, %#v, %#v): %s", protocol, addresses, route, err) } + if test.disableMulticastForwardingForNIC { + for nicID := range endpoints { + s.SetNICMulticastForwarding(nicID, protocol, false /* enable */) + } + } + + if test.updateMulticastForwardingForProtocol != nil { + test.updateMulticastForwardingForProtocol(t, s, protocol, eventDispatcher) + } + if test.removeOutputInterface != 0 { if err := s.RemoveNIC(test.removeOutputInterface); err != nil { t.Fatalf("RemoveNIC(%d): %s", test.removeOutputInterface, err) @@ -1024,11 +1181,6 @@ func TestMulticastForwarding(t *testing.T) { p.DecRef() } - eventDispatcher, ok := eventDispatchers[protocol] - if !ok { - t.Fatalf("eventDispatchers[%d] = (_, false), want (_, true)", protocol) - } - wantUnexpectedInputInterfaceEvent := func() *onUnexpectedInputInterfaceData { if test.expectUnexpectedInputInterfaceEvent { return &onUnexpectedInputInterfaceData{stack.MulticastPacketContext{stack.UnicastSourceAndMulticastDestination{srcAddr, dstAddr}, incomingNICID}, test.routeInputInterface} From 2df29e11ddb07c1283751274ef5b230bec5409b7 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 15 Jun 2022 12:03:28 -0700 Subject: [PATCH 14/29] Deflake MsgqueueTest.MsgCtlIpcStat test. This test flakes with <1% rate. There is a relevant comment explaining that "timestamps only have a resolution of seconds" and so we need to sleep for a second to actually see the timestamps change. However, currently the tests sleeps *before* calculating local time. Which beats the point. Move the local time declaration before the sleep. absl::ToTimeT() rounds down to the nearest second, discarding nanoseconds field completely. I think the host kernel might be rounding up the `msqid_ds` time fields. Tested: 1000 runs of this test pass natively. PiperOrigin-RevId: 455187650 --- test/syscalls/linux/msgqueue.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/test/syscalls/linux/msgqueue.cc b/test/syscalls/linux/msgqueue.cc index c4761eba8..1b43f000f 100644 --- a/test/syscalls/linux/msgqueue.cc +++ b/test/syscalls/linux/msgqueue.cc @@ -697,7 +697,10 @@ TEST(MsgqueueTest, InterruptSend) { // Test msgctl with IPC_STAT option. TEST(MsgqueueTest, MsgCtlIpcStat) { + // The timestamps only have a resolution of seconds; slow down so we actually + // see the timestamps change. auto start = absl::Now(); + absl::SleepFor(absl::Milliseconds(1010)); Queue queue(msgget(IPC_PRIVATE, 0600)); ASSERT_THAT(queue.get(), SyscallSucceeds()); @@ -726,10 +729,8 @@ TEST(MsgqueueTest, MsgCtlIpcStat) { EXPECT_EQ(ds.msg_lspid, 0); EXPECT_EQ(ds.msg_lrpid, 0); - // The timestamps only have a resolution of seconds; slow down so we actually - // see the timestamps change. - absl::SleepFor(absl::Seconds(1)); auto pre_send = absl::Now(); + absl::SleepFor(absl::Milliseconds(1010)); msgbuf buf{1, "A message."}; ASSERT_THAT(msgsnd(queue.get(), &buf, sizeof(buf.mtext), 0), @@ -747,8 +748,8 @@ TEST(MsgqueueTest, MsgCtlIpcStat) { EXPECT_EQ(ds.msg_lspid, pid); EXPECT_EQ(ds.msg_lrpid, 0); - absl::SleepFor(absl::Seconds(1)); auto pre_receive = absl::Now(); + absl::SleepFor(absl::Milliseconds(1010)); ASSERT_THAT(msgrcv(queue.get(), &buf, sizeof(buf.mtext), 0, 0), SyscallSucceedsWithValue(msgSize)); From 6258291815e823034218fc617c3fc70978d7137b Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Wed, 15 Jun 2022 13:37:13 -0700 Subject: [PATCH 15/29] Small fixes to trace points Don't collect `cwd` when task root is not present anymore, e.g. zombie. Limit the size allocated for address in `connect(2)`. Updates #4805 PiperOrigin-RevId: 455208465 --- pkg/sentry/kernel/seccheck.go | 14 ++++++++------ pkg/sentry/syscalls/linux/points.go | 5 +---- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/sentry/kernel/seccheck.go b/pkg/sentry/kernel/seccheck.go index 9afb7c130..9d680235e 100644 --- a/pkg/sentry/kernel/seccheck.go +++ b/pkg/sentry/kernel/seccheck.go @@ -49,12 +49,14 @@ func LoadSeccheckDataLocked(t *Task, mask seccheck.FieldMask, info *pb.ContextDa info.ContainerId = t.tg.leader.ContainerID() } if mask.Contains(seccheck.FieldCtxtCwd) { - root := t.FSContext().RootDirectoryVFS2() - defer root.DecRef(t) - wd := t.FSContext().WorkingDirectoryVFS2() - defer wd.DecRef(t) - vfsObj := root.Mount().Filesystem().VirtualFilesystem() - info.Cwd, _ = vfsObj.PathnameWithDeleted(t, root, wd) + if root := t.FSContext().RootDirectoryVFS2(); root.Ok() { + defer root.DecRef(t) + if wd := t.FSContext().WorkingDirectoryVFS2(); wd.Ok() { + defer wd.DecRef(t) + vfsObj := root.Mount().Filesystem().VirtualFilesystem() + info.Cwd, _ = vfsObj.PathnameWithDeleted(t, root, wd) + } + } } if mask.Contains(seccheck.FieldCtxtProcessName) { info.ProcessName = t.Name() diff --git a/pkg/sentry/syscalls/linux/points.go b/pkg/sentry/syscalls/linux/points.go index bedfeb899..6b7a3956c 100644 --- a/pkg/sentry/syscalls/linux/points.go +++ b/pkg/sentry/syscalls/linux/points.go @@ -191,10 +191,7 @@ func PointConnect(t *kernel.Task, fields seccheck.FieldSet, cxtData *pb.ContextD addr := info.Args[1].Pointer() addrlen := info.Args[2].Uint() - if addr > 0 { - p.Address = make([]byte, addrlen) - _, _ = t.CopyInBytes(addr, p.Address) - } + p.Address, _ = CaptureAddress(t, addr, addrlen) if fields.Local.Contains(seccheck.FieldSyscallPath) { p.FdPath = getFilePath(t, int32(p.Fd)) From 2f315ea39d8b87776bb803fd139b7a37448619cd Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Wed, 15 Jun 2022 14:44:03 -0700 Subject: [PATCH 16/29] Suppress the known lock issue It is triggered too often in syzkaller jobs. PiperOrigin-RevId: 455223977 --- pkg/sentry/mm/debug.go | 6 ++++-- pkg/sentry/mm/procfs.go | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/pkg/sentry/mm/debug.go b/pkg/sentry/mm/debug.go index d927b1702..3fff52daf 100644 --- a/pkg/sentry/mm/debug.go +++ b/pkg/sentry/mm/debug.go @@ -42,8 +42,10 @@ func (mm *MemoryManager) String() string { func (mm *MemoryManager) DebugString(ctx context.Context) string { var b bytes.Buffer - mm.mappingMu.RLock() - defer mm.mappingMu.RUnlock() + // FIXME(b/235153601): Need to replace RLockBypass with RLockBypass + // after fixing b/235153601. + mm.mappingMu.RLockBypass() + defer mm.mappingMu.RUnlockBypass() b.WriteString("VMAs:\n") for vseg := mm.vmas.FirstSegment(); vseg.Ok(); vseg = vseg.NextSegment() { b.Write(mm.vmaMapsEntryLocked(ctx, vseg)) diff --git a/pkg/sentry/mm/procfs.go b/pkg/sentry/mm/procfs.go index f1440e884..f4331239c 100644 --- a/pkg/sentry/mm/procfs.go +++ b/pkg/sentry/mm/procfs.go @@ -85,8 +85,10 @@ func (mm *MemoryManager) ReadMapsDataInto(ctx context.Context, buf *bytes.Buffer // ReadMapsSeqFileData is called by fs/proc.mapsData.ReadSeqFileData to // implement /proc/[pid]/maps. func (mm *MemoryManager) ReadMapsSeqFileData(ctx context.Context, handle seqfile.SeqHandle) ([]seqfile.SeqData, int64) { - mm.mappingMu.RLock() - defer mm.mappingMu.RUnlock() + // FIXME(b/235153601): Need to replace RLockBypass with RLockBypass + // after fixing b/235153601. + mm.mappingMu.RLockBypass() + defer mm.mappingMu.RUnlockBypass() var data []seqfile.SeqData var start hostarch.Addr if handle != nil { From 21c757b60f22d9a570ac92b24b27fafede3b0224 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Wed, 15 Jun 2022 15:19:33 -0700 Subject: [PATCH 17/29] Enable all trace points for syscall tests Enable all trace points while running syscall tests to catch possible crashes and bugs that may exist. These are not verifying that the data in the points are correct though. Since all trace points are platform agnostic for now, only enable them for ptrace. Updates #4805 PiperOrigin-RevId: 455232093 --- pkg/sentry/kernel/task_syscall.go | 2 +- pkg/sentry/seccheck/checkers/null/BUILD | 13 +++ pkg/sentry/seccheck/checkers/null/null.go | 40 ++++++++ pkg/sentry/seccheck/syscall.go | 4 + runsc/boot/BUILD | 1 + runsc/boot/seccheck.go | 1 + test/runner/BUILD | 2 + test/runner/defs.bzl | 4 + test/runner/main.go | 32 +++++++ test/trace/BUILD | 2 +- test/trace/config/BUILD | 14 +++ test/trace/config/config.go | 110 ++++++++++++++++++++++ test/trace/trace_test.go | 101 +++----------------- 13 files changed, 235 insertions(+), 91 deletions(-) create mode 100644 pkg/sentry/seccheck/checkers/null/BUILD create mode 100644 pkg/sentry/seccheck/checkers/null/null.go create mode 100644 test/trace/config/BUILD create mode 100644 test/trace/config/config.go diff --git a/pkg/sentry/kernel/task_syscall.go b/pkg/sentry/kernel/task_syscall.go index edd36e519..48be595f2 100644 --- a/pkg/sentry/kernel/task_syscall.go +++ b/pkg/sentry/kernel/task_syscall.go @@ -182,7 +182,6 @@ func (t *Task) executeSyscall(sysno uintptr, args arch.SyscallArguments) (rval u }) } if seccheck.Global.SyscallEnabled(seccheck.SyscallExit, sysno) { - cb := t.SyscallTable().LookupSyscallToProto(sysno) fields := seccheck.Global.GetFieldSet(seccheck.GetPointForSyscall(seccheck.SyscallExit, sysno)) var ctxData *pb.ContextData if !fields.Context.Empty() { @@ -196,6 +195,7 @@ func (t *Task) executeSyscall(sysno uintptr, args arch.SyscallArguments) (rval u Rval: rval, Errno: ExtractErrno(err, int(sysno)), } + cb := t.SyscallTable().LookupSyscallToProto(sysno) msg, msgType := cb(t, fields, ctxData, info) seccheck.Global.SendToCheckers(func(c seccheck.Checker) error { return c.Syscall(t, fields, ctxData, msgType, msg) diff --git a/pkg/sentry/seccheck/checkers/null/BUILD b/pkg/sentry/seccheck/checkers/null/BUILD new file mode 100644 index 000000000..1136dc5bc --- /dev/null +++ b/pkg/sentry/seccheck/checkers/null/BUILD @@ -0,0 +1,13 @@ +load("//tools:defs.bzl", "go_library") + +package(licenses = ["notice"]) + +go_library( + name = "null", + srcs = ["null.go"], + visibility = ["//:sandbox"], + deps = [ + "//pkg/fd", + "//pkg/sentry/seccheck", + ], +) diff --git a/pkg/sentry/seccheck/checkers/null/null.go b/pkg/sentry/seccheck/checkers/null/null.go new file mode 100644 index 000000000..9837952cd --- /dev/null +++ b/pkg/sentry/seccheck/checkers/null/null.go @@ -0,0 +1,40 @@ +// Copyright 2021 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 null defines a seccheck.Checker that does nothing with the trace +// points, akin to /dev/null. +package null + +import ( + "gvisor.dev/gvisor/pkg/fd" + "gvisor.dev/gvisor/pkg/sentry/seccheck" +) + +func init() { + seccheck.RegisterSink(seccheck.SinkDesc{ + Name: "null", + New: new, + }) +} + +// null is a checker that does nothing with the trace points. +type null struct { + seccheck.CheckerDefaults +} + +var _ seccheck.Checker = (*null)(nil) + +func new(_ map[string]interface{}, _ *fd.FD) (seccheck.Checker, error) { + return &null{}, nil +} diff --git a/pkg/sentry/seccheck/syscall.go b/pkg/sentry/seccheck/syscall.go index 43d0cce84..4dea53546 100644 --- a/pkg/sentry/seccheck/syscall.go +++ b/pkg/sentry/seccheck/syscall.go @@ -63,5 +63,9 @@ func GetPointForSyscall(typ SyscallType, sysno uintptr) Point { // SyscallEnabled checks if the corresponding point for the syscall is enabled. func (s *State) SyscallEnabled(typ SyscallType, sysno uintptr) bool { + // Prevent overflow. + if sysno >= syscallsMax { + return false + } return s.Enabled(GetPointForSyscall(typ, sysno)) } diff --git a/runsc/boot/BUILD b/runsc/boot/BUILD index 5a7782c97..912c2faac 100644 --- a/runsc/boot/BUILD +++ b/runsc/boot/BUILD @@ -73,6 +73,7 @@ go_library( "//pkg/sentry/pgalloc", "//pkg/sentry/platform", "//pkg/sentry/seccheck", + "//pkg/sentry/seccheck/checkers/null", "//pkg/sentry/seccheck/checkers/remote", "//pkg/sentry/seccheck/points:points_go_proto", "//pkg/sentry/socket/hostinet", diff --git a/runsc/boot/seccheck.go b/runsc/boot/seccheck.go index df4a49ff0..5d9363820 100644 --- a/runsc/boot/seccheck.go +++ b/runsc/boot/seccheck.go @@ -23,6 +23,7 @@ import ( "gvisor.dev/gvisor/pkg/sentry/seccheck" // Register supported of checkers. + _ "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/null" _ "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote" ) diff --git a/test/runner/BUILD b/test/runner/BUILD index 7c6bebd3b..084cdbf5a 100644 --- a/test/runner/BUILD +++ b/test/runner/BUILD @@ -13,9 +13,11 @@ go_binary( visibility = ["//:sandbox"], deps = [ "//pkg/log", + "//pkg/sentry/seccheck", "//pkg/test/testutil", "//runsc/specutils", "//test/runner/gtest", + "//test/trace/config", "//test/uds", "@com_github_opencontainers_runtime_spec//specs-go:go_default_library", "@com_github_syndtr_gocapability//capability:go_default_library", diff --git a/test/runner/defs.bzl b/test/runner/defs.bzl index 7cbae734d..a14818c15 100644 --- a/test/runner/defs.bzl +++ b/test/runner/defs.bzl @@ -145,6 +145,10 @@ def _syscall_test( "--container=" + str(container), ] + # Trace points are platform agnostic, so enable them for ptrace only. + if platform == "ptrace": + runner_args.append("--trace") + # Call the rule above. _runner_test( name = name, diff --git a/test/runner/main.go b/test/runner/main.go index 313d9939f..97f8b96a4 100644 --- a/test/runner/main.go +++ b/test/runner/main.go @@ -33,9 +33,11 @@ import ( "github.com/syndtr/gocapability/capability" "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/log" + "gvisor.dev/gvisor/pkg/sentry/seccheck" "gvisor.dev/gvisor/pkg/test/testutil" "gvisor.dev/gvisor/runsc/specutils" "gvisor.dev/gvisor/test/runner/gtest" + "gvisor.dev/gvisor/test/trace/config" "gvisor.dev/gvisor/test/uds" ) @@ -52,6 +54,7 @@ var ( lisafs = flag.Bool("lisafs", false, "enable lisafs protocol if vfs2 is also enabled") container = flag.Bool("container", false, "run tests in their own namespaces (user ns, network ns, etc), pretending to be root. Implicitly enabled if network=host, or if using network namespaces") setupContainerPath = flag.String("setup-container", "", "path to setup_container binary (for use with --container)") + trace = flag.Bool("trace", false, "enables all trace points") addUDSTree = flag.Bool("add-uds-tree", false, "expose a tree of UDS utilities for use in tests") // TODO(gvisor.dev/issue/4572): properly support leak checking for runsc, and @@ -217,6 +220,14 @@ func runRunsc(tc gtest.TestCase, spec *specs.Spec) error { if *leakCheck { args = append(args, "-ref-leak-mode=log-names") } + if *trace { + flag, err := enableAllTraces(rootDir) + if err != nil { + return fmt.Errorf("enabling all traces: %w", err) + } + log.Infof("Enabling all trace points: %s", flag) + args = append(args, flag) + } testLogDir := "" if undeclaredOutputsDir, ok := unix.Getenv("TEST_UNDECLARED_OUTPUTS_DIR"); ok { @@ -562,3 +573,24 @@ func main() { testing.Main(matchString, tests, nil, nil) } + +func enableAllTraces(dir string) (string, error) { + builder := config.Builder{} + if err := builder.LoadAllPoints(specutils.ExePath); err != nil { + return "", err + } + builder.AddSink(seccheck.SinkConfig{ + Name: "null", + }) + path := filepath.Join(dir, "pod_init.json") + cfgFile, err := os.Create(path) + if err != nil { + return "", err + } + defer cfgFile.Close() + + if err := builder.WriteInitConfig(cfgFile); err != nil { + return "", fmt.Errorf("writing config file: %w", err) + } + return "--pod-init-config=" + path, nil +} diff --git a/test/trace/BUILD b/test/trace/BUILD index bd3564de8..74597ab6d 100644 --- a/test/trace/BUILD +++ b/test/trace/BUILD @@ -20,7 +20,7 @@ go_test( "//pkg/sentry/seccheck/checkers/remote/test", "//pkg/sentry/seccheck/points:points_go_proto", "//pkg/test/testutil", - "//runsc/boot", + "//test/trace/config", "@org_golang_google_protobuf//proto:go_default_library", ], ) diff --git a/test/trace/config/BUILD b/test/trace/config/BUILD new file mode 100644 index 000000000..b87bf4b53 --- /dev/null +++ b/test/trace/config/BUILD @@ -0,0 +1,14 @@ +load("//tools:defs.bzl", "go_library") + +package(licenses = ["notice"]) + +go_library( + name = "config", + testonly = 1, + srcs = ["config.go"], + visibility = ["//:sandbox"], + deps = [ + "//pkg/sentry/seccheck", + "//runsc/boot", + ], +) diff --git a/test/trace/config/config.go b/test/trace/config/config.go new file mode 100644 index 000000000..c3f4343d7 --- /dev/null +++ b/test/trace/config/config.go @@ -0,0 +1,110 @@ +// 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 config providides helper functions to configure trace sessions. +package config + +import ( + "bufio" + "bytes" + "encoding/json" + "fmt" + "io" + "os/exec" + "strings" + + "gvisor.dev/gvisor/pkg/sentry/seccheck" + "gvisor.dev/gvisor/runsc/boot" +) + +// Builder helps with building of trace session configuration. +type Builder struct { + points []seccheck.PointConfig + sinks []seccheck.SinkConfig +} + +// WriteInitConfig writes the current configuration in a format compatible with +// the flag --pod-init-config. +func (b *Builder) WriteInitConfig(w io.Writer) error { + init := &boot.InitConfig{ + TraceSession: seccheck.SessionConfig{ + Name: seccheck.DefaultSessionName, + Points: b.points, + Sinks: b.sinks, + }, + } + + encoder := json.NewEncoder(w) + return encoder.Encode(&init) +} + +// LoadAllPoints enables all points together with all optional and context +// fields. +func (b *Builder) LoadAllPoints(runscPath string) error { + cmd := exec.Command(runscPath, "trace", "metadata") + out, err := cmd.CombinedOutput() + if err != nil { + return err + } + + // The command above produces an output like the following: + // POINTS (907) + // Name: container/start, optional fields: [], context fields: [time|thread_id] + scanner := bufio.NewScanner(bytes.NewReader(out)) + if !scanner.Scan() { + return fmt.Errorf("%q returned empty", cmd) + } + if !scanner.Scan() { + return fmt.Errorf("%q returned empty", cmd) + } + for line := scanner.Text(); scanner.Scan(); line = scanner.Text() { + elems := strings.Split(line, ",") + if len(elems) != 3 { + return fmt.Errorf("invalid line: %q", line) + } + name := strings.TrimPrefix(elems[0], "Name: ") + optFields, err := parseFields(elems[1], "optional fields: ") + if err != nil { + return err + } + ctxFields, err := parseFields(elems[2], "context fields: ") + if err != nil { + return err + } + b.points = append(b.points, seccheck.PointConfig{ + Name: name, + OptionalFields: optFields, + ContextFields: ctxFields, + }) + } + return scanner.Err() +} + +func parseFields(elem, prefix string) ([]string, error) { + stripped := strings.TrimPrefix(strings.TrimSpace(elem), prefix) + switch { + case len(stripped) < 2: + return nil, fmt.Errorf("invalid %s format: %q", prefix, elem) + case len(stripped) == 2: + return nil, nil + } + // Remove [] from `stripped`. + clean := stripped[1 : len(stripped)-1] + return strings.Split(clean, "|"), nil +} + +// AddSink adds the sink to the configuration. +func (b *Builder) AddSink(sink seccheck.SinkConfig) { + b.sinks = append(b.sinks, sink) +} diff --git a/test/trace/trace_test.go b/test/trace/trace_test.go index 76f370270..617c7f0b8 100644 --- a/test/trace/trace_test.go +++ b/test/trace/trace_test.go @@ -16,11 +16,7 @@ package trace import ( - "bufio" - "bytes" - "encoding/json" "fmt" - "io/ioutil" "os" "os/exec" "strings" @@ -32,7 +28,7 @@ import ( "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/test" pb "gvisor.dev/gvisor/pkg/sentry/seccheck/points/points_go_proto" "gvisor.dev/gvisor/pkg/test/testutil" - "gvisor.dev/gvisor/runsc/boot" + "gvisor.dev/gvisor/test/trace/config" ) // TestAll enabled all trace points in the system with all optional and context @@ -48,19 +44,24 @@ func TestAll(t *testing.T) { if err != nil { t.Fatal(err) } - cfg, err := buildPodConfig(runsc, server.Endpoint) - if err != nil { + builder := config.Builder{} + if err := builder.LoadAllPoints(runsc); err != nil { t.Fatal(err) } + builder.AddSink(seccheck.SinkConfig{ + Name: "remote", + Config: map[string]interface{}{ + "endpoint": server.Endpoint, + }, + }) - cfgFile, err := ioutil.TempFile(testutil.TmpDir(), "config") + cfgFile, err := os.CreateTemp(testutil.TmpDir(), "config") if err != nil { t.Fatalf("error creating tmp file: %v", err) } defer cfgFile.Close() - encoder := json.NewEncoder(cfgFile) - if err := encoder.Encode(&cfg); err != nil { - t.Fatalf("JSON encode: %v", err) + if err := builder.WriteInitConfig(cfgFile); err != nil { + t.Fatalf("writing config file: %v", err) } workload, err := testutil.FindFile("test/trace/workload/workload") @@ -84,84 +85,6 @@ func TestAll(t *testing.T) { matchPoints(t, server.GetPoints()) } -func buildPodConfig(runscPath, endpoint string) (*boot.InitConfig, error) { - pts, err := allPoints(runscPath) - if err != nil { - return nil, err - } - return &boot.InitConfig{ - TraceSession: seccheck.SessionConfig{ - Name: seccheck.DefaultSessionName, - Points: pts, - Sinks: []seccheck.SinkConfig{ - { - Name: "remote", - Config: map[string]interface{}{ - "endpoint": endpoint, - }, - }, - }, - }, - }, nil -} - -func allPoints(runscPath string) ([]seccheck.PointConfig, error) { - cmd := exec.Command(runscPath, "trace", "metadata") - out, err := cmd.CombinedOutput() - if err != nil { - return nil, err - } - - // The command above produces an output like the following: - // POINTS (907) - // Name: container/start, optional fields: [], context fields: [time|thread_id] - scanner := bufio.NewScanner(bytes.NewReader(out)) - if !scanner.Scan() { - return nil, fmt.Errorf("%q returned empty", cmd) - } - if !scanner.Scan() { - return nil, fmt.Errorf("%q returned empty", cmd) - } - var points []seccheck.PointConfig - for line := scanner.Text(); scanner.Scan(); line = scanner.Text() { - elems := strings.Split(line, ",") - if len(elems) != 3 { - return nil, fmt.Errorf("invalid line: %q", line) - } - name := strings.TrimPrefix(elems[0], "Name: ") - optFields, err := parseFields(elems[1], "optional fields: ") - if err != nil { - return nil, err - } - ctxFields, err := parseFields(elems[2], "context fields: ") - if err != nil { - return nil, err - } - points = append(points, seccheck.PointConfig{ - Name: name, - OptionalFields: optFields, - ContextFields: ctxFields, - }) - } - if scanner.Err() != nil { - return nil, scanner.Err() - } - return points, nil -} - -func parseFields(elem, prefix string) ([]string, error) { - stripped := strings.TrimPrefix(strings.TrimSpace(elem), prefix) - switch { - case len(stripped) < 2: - return nil, fmt.Errorf("invalid %s format: %q", prefix, elem) - case len(stripped) == 2: - return nil, nil - } - // Remove [] from `stripped`. - clean := stripped[1 : len(stripped)-1] - return strings.Split(clean, "|"), nil -} - func matchPoints(t *testing.T, msgs []test.Message) { // Register functions that verify each available point. matchers := map[pb.MessageType]*struct { From b252543133681c44bccdabf682b3afc447fd4702 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Wed, 15 Jun 2022 16:10:10 -0700 Subject: [PATCH 18/29] Restrict checkers.remote visibility Updates #4805 PiperOrigin-RevId: 455242654 --- pkg/sentry/seccheck/checkers/remote/remote.go | 38 +++++++++---------- .../seccheck/checkers/remote/remote_test.go | 6 +-- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/pkg/sentry/seccheck/checkers/remote/remote.go b/pkg/sentry/seccheck/checkers/remote/remote.go index a12cee250..aadbd40f9 100644 --- a/pkg/sentry/seccheck/checkers/remote/remote.go +++ b/pkg/sentry/seccheck/checkers/remote/remote.go @@ -36,26 +36,26 @@ import ( func init() { seccheck.RegisterSink(seccheck.SinkDesc{ Name: "remote", - Setup: Setup, - New: New, + Setup: setupSink, + New: new, }) } -// Remote sends a serialized point to a remote process asynchronously over a +// remote sends a serialized point to a remote process asynchronously over a // SOCK_SEQPACKET Unix-domain socket. Each message corresponds to a single // serialized point proto, preceded by a standard header. If the point cannot // be sent, e.g. buffer full, the point is dropped on the floor to avoid // delaying/hanging indefinitely the application. -type Remote struct { +type remote struct { endpoint *fd.FD } -var _ seccheck.Checker = (*Remote)(nil) +var _ seccheck.Checker = (*remote)(nil) -// Setup starts the connection to the remote process and returns a file that +// setupSink starts the connection to the remote process and returns a file that // can be used to communicate with it. The caller is responsible to close to // file. -func Setup(config map[string]interface{}) (*os.File, error) { +func setupSink(config map[string]interface{}) (*os.File, error) { addrOpaque, ok := config["endpoint"] if !ok { return nil, fmt.Errorf("endpoint not present in configuration") @@ -119,16 +119,16 @@ func setup(path string) (*os.File, error) { return f, nil } -// New creates a new Remote checker. -func New(_ map[string]interface{}, endpoint *fd.FD) (seccheck.Checker, error) { +// new creates a new Remote checker. +func new(_ map[string]interface{}, endpoint *fd.FD) (seccheck.Checker, error) { if endpoint == nil { return nil, fmt.Errorf("remote sink requires an endpoint") } - return &Remote{endpoint: endpoint}, nil + return &remote{endpoint: endpoint}, nil } // Stop implements seccheck.Checker. -func (r *Remote) Stop() { +func (r *remote) Stop() { if r.endpoint != nil { // It's possible to race with Point firing, but in the worst case they will // simply fail to be delivered. @@ -136,7 +136,7 @@ func (r *Remote) Stop() { } } -func (r *Remote) write(msg proto.Message, msgType pb.MessageType) { +func (r *remote) write(msg proto.Message, msgType pb.MessageType) { out, err := proto.Marshal(msg) if err != nil { log.Debugf("Marshal(%+v): %v", msg, err) @@ -158,43 +158,43 @@ func (r *Remote) write(msg proto.Message, msgType pb.MessageType) { } // Clone implements seccheck.Checker. -func (r *Remote) Clone(_ context.Context, _ seccheck.FieldSet, info *pb.CloneInfo) error { +func (r *remote) Clone(_ context.Context, _ seccheck.FieldSet, info *pb.CloneInfo) error { r.write(info, pb.MessageType_MESSAGE_SENTRY_CLONE) return nil } // Execve implements seccheck.Checker. -func (r *Remote) Execve(_ context.Context, _ seccheck.FieldSet, info *pb.ExecveInfo) error { +func (r *remote) Execve(_ context.Context, _ seccheck.FieldSet, info *pb.ExecveInfo) error { r.write(info, pb.MessageType_MESSAGE_SENTRY_EXEC) return nil } // ExitNotifyParent implements seccheck.Checker. -func (r *Remote) ExitNotifyParent(_ context.Context, _ seccheck.FieldSet, info *pb.ExitNotifyParentInfo) error { +func (r *remote) ExitNotifyParent(_ context.Context, _ seccheck.FieldSet, info *pb.ExitNotifyParentInfo) error { r.write(info, pb.MessageType_MESSAGE_SENTRY_EXIT_NOTIFY_PARENT) return nil } // TaskExit implements seccheck.Checker. -func (r *Remote) TaskExit(_ context.Context, _ seccheck.FieldSet, info *pb.TaskExit) error { +func (r *remote) TaskExit(_ context.Context, _ seccheck.FieldSet, info *pb.TaskExit) error { r.write(info, pb.MessageType_MESSAGE_SENTRY_TASK_EXIT) return nil } // ContainerStart implements seccheck.Checker. -func (r *Remote) ContainerStart(_ context.Context, _ seccheck.FieldSet, info *pb.Start) error { +func (r *remote) ContainerStart(_ context.Context, _ seccheck.FieldSet, info *pb.Start) error { r.write(info, pb.MessageType_MESSAGE_CONTAINER_START) return nil } // RawSyscall implements seccheck.Checker. -func (r *Remote) RawSyscall(_ context.Context, _ seccheck.FieldSet, info *pb.Syscall) error { +func (r *remote) RawSyscall(_ context.Context, _ seccheck.FieldSet, info *pb.Syscall) error { r.write(info, pb.MessageType_MESSAGE_SYSCALL_RAW) return nil } // Syscall implements seccheck.Checker. -func (r *Remote) Syscall(ctx context.Context, fields seccheck.FieldSet, ctxData *pb.ContextData, msgType pb.MessageType, msg proto.Message) error { +func (r *remote) Syscall(ctx context.Context, fields seccheck.FieldSet, ctxData *pb.ContextData, msgType pb.MessageType, msg proto.Message) error { r.write(msg, msgType) return nil } diff --git a/pkg/sentry/seccheck/checkers/remote/remote_test.go b/pkg/sentry/seccheck/checkers/remote/remote_test.go index 57aa2b80f..d7c3bd69e 100644 --- a/pkg/sentry/seccheck/checkers/remote/remote_test.go +++ b/pkg/sentry/seccheck/checkers/remote/remote_test.go @@ -126,7 +126,7 @@ func TestBasic(t *testing.T) { } _ = endpoint.Close() - r, err := New(nil, endpointFD) + r, err := new(nil, endpointFD) if err != nil { t.Fatalf("New(): %v", err) } @@ -205,7 +205,7 @@ func TestExample(t *testing.T) { } _ = endpoint.Close() - r, err := New(nil, endpointFD) + r, err := new(nil, endpointFD) if err != nil { t.Fatalf("New(): %v", err) } @@ -247,7 +247,7 @@ func BenchmarkSmall(t *testing.B) { } _ = endpoint.Close() - r, err := New(nil, endpointFD) + r, err := new(nil, endpointFD) if err != nil { t.Fatalf("New(): %v", err) } From 34d2ad28d0bee859d06498773833c1c28b836d29 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 15 Jun 2022 17:25:49 -0700 Subject: [PATCH 19/29] Cache children names in directory dentry on getdents64(2). When the gofer mount has an authoritative cache (when the remote filesystem is not changing under our feet), it caches all dirents in the directory dentry on a successful getdents64(2) application syscall. This change also additionally caches all the children names in the dentry. This is so that on walks to non-existent files, if the dirents cache is available, we can return ENOENT without having to make the round trip to the gofer. This is especially helpful for applications that exhaustively search all directories in a certain environment variable for loading code. For example, $LOAD_PATH and $CLASSPATH is used respectively by Ruby and Java. This is more memory intensive. For sure. Ruby example on KVM platform sees ~10% reduction in load time. Before: ``` Finished in 1.39 seconds (files took 5.91 seconds to load) ``` After: ``` Finished in 1.23 seconds (files took 5.24 seconds to load) ``` PiperOrigin-RevId: 455257590 --- pkg/sentry/fsimpl/gofer/directory.go | 11 ++++++++ pkg/sentry/fsimpl/gofer/filesystem.go | 37 ++++++++++++++++++++++----- pkg/sentry/fsimpl/gofer/gofer.go | 7 +++-- pkg/sentry/fsimpl/gofer/revalidate.go | 2 +- 4 files changed, 48 insertions(+), 9 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/directory.go b/pkg/sentry/fsimpl/gofer/directory.go index f02253761..70ef06409 100644 --- a/pkg/sentry/fsimpl/gofer/directory.go +++ b/pkg/sentry/fsimpl/gofer/directory.go @@ -146,6 +146,13 @@ func (d *dentry) createSyntheticChildLocked(opts *createSyntheticOpts) { d.syntheticChildren++ } +// Preconditions: +// - d.dirMu must be locked. +func (d *dentry) clearDirentsLocked() { + d.dirents = nil + d.childrenSet = nil +} + // +stateify savable type directoryFD struct { fileDescription @@ -346,6 +353,10 @@ func (d *dentry) getDirents(ctx context.Context) ([]vfs.Dirent, error) { // Cache dirents for future directoryFDs if permitted. if d.cachedMetadataAuthoritative() { d.dirents = dirents + d.childrenSet = make(map[string]struct{}, len(dirents)) + for _, dirent := range d.dirents { + d.childrenSet[dirent.Name] = struct{}{} + } } return dirents, nil } diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index 69e55ad20..9c44f0506 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -276,6 +276,13 @@ func (fs *filesystem) getChildAndWalkPathLocked(ctx context.Context, parent *den return child, nil } + if parent.childrenSet != nil { + // Is the first child even there? Don't make RPC if not. + if _, ok := parent.childrenSet[first]; !ok { + return nil, linuxerr.ENOENT + } + } + // Walk as much of the path as possible in 1 RPC. names := []string{first} for pit = pit.Next(); pit.Ok(); pit = pit.Next() { @@ -369,6 +376,13 @@ func (fs *filesystem) getChildLocked(ctx context.Context, parent *dentry, name s return child, nil } + if parent.childrenSet != nil { + // Is the child even there? Don't make RPC if not. + if _, ok := parent.childrenSet[name]; !ok { + return nil, linuxerr.ENOENT + } + } + var child *dentry if fs.opts.lisaEnabled { childInode, err := parent.controlFDLisa.Walk(ctx, name) @@ -512,6 +526,11 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir if child, ok := parent.children[name]; ok && child != nil { return linuxerr.EEXIST } + if parent.childrenSet != nil { + if _, ok := parent.childrenSet[name]; ok { + return linuxerr.EEXIST + } + } checkExistence := func() error { if child, err := fs.getChildLocked(ctx, parent, name, &ds); err != nil && !linuxerr.Equals(linuxerr.ENOENT, err) { return err @@ -549,7 +568,7 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir return err } parent.touchCMtime() - parent.dirents = nil + parent.clearDirentsLocked() ev := linux.IN_CREATE if dir { ev |= linux.IN_ISDIR @@ -569,7 +588,7 @@ func (fs *filesystem) doCreateAt(ctx context.Context, rp *vfs.ResolvingPath, dir delete(parent.children, name) } parent.touchCMtime() - parent.dirents = nil + parent.clearDirentsLocked() } ev := linux.IN_CREATE if dir { @@ -622,6 +641,12 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b parent.dirMu.Lock() defer parent.dirMu.Unlock() + if parent.childrenSet != nil { + if _, ok := parent.childrenSet[name]; !ok { + return linuxerr.ENOENT + } + } + // Load child if sticky bit is set because we need to determine whether // deletion is allowed. var child *dentry @@ -745,7 +770,7 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b } parent.cacheNegativeLookupLocked(name) if parent.cachedMetadataAuthoritative() { - parent.dirents = nil + parent.clearDirentsLocked() parent.touchCMtime() if dir { parent.decLinks() @@ -1391,7 +1416,7 @@ func (d *dentry) createAndOpenChildLocked(ctx context.Context, rp *vfs.Resolving appendNewChildDentry(ds, d, child) if d.cachedMetadataAuthoritative() { d.touchCMtime() - d.dirents = nil + d.clearDirentsLocked() } // Finally, construct a file description representing the created file. @@ -1623,14 +1648,14 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa renamed.touchCtime() } if oldParent.cachedMetadataAuthoritative() { - oldParent.dirents = nil + oldParent.clearDirentsLocked() oldParent.touchCMtime() if renamed.isDir() { oldParent.decLinks() } } if newParent.cachedMetadataAuthoritative() { - newParent.dirents = nil + newParent.clearDirentsLocked() newParent.touchCMtime() if renamed.isDir() && (replaced == nil || !replaced.isDir()) { // Increase the link count if we did not replace another directory. diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index 6717eb308..fc72d8602 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -850,8 +850,11 @@ type dentry struct { // If this dentry represents a directory, // dentry.cachedMetadataAuthoritative() == true, and dirents is not nil, it // is a cache of all entries in the directory, in the order they were - // returned by the server. dirents is protected by dirMu. - dirents []vfs.Dirent + // returned by the server. childrenSet just stores the `Name` field of all + // dirents in a set for fast query. dirents and childrenSet are protected by + // dirMu and share the same lifecycle. + dirents []vfs.Dirent + childrenSet map[string]struct{} // Cached metadata; protected by metadataMu. // To access: diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go index 08136e441..782f0d02d 100644 --- a/pkg/sentry/fsimpl/gofer/revalidate.go +++ b/pkg/sentry/fsimpl/gofer/revalidate.go @@ -318,7 +318,7 @@ func (fs *filesystem) revalidateHelper(ctx context.Context, vfsObj *vfs.VirtualF *ds = appendDentry(*ds, d) d.parent.syntheticChildren-- - d.parent.dirents = nil + d.parent.clearDirentsLocked() } // Since the dirMu was released and reacquired, re-check that the From 8011b8d6d298cb9bbdd3656a6f0d9ba4c5b1f955 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 15 Jun 2022 19:04:50 -0700 Subject: [PATCH 20/29] Make anonfs return ENOTDIR for open(O_DIRECTORY) calls. All anonfs dentries are non-directories. So all FilesystemImpl methods that would require an anonDentry to be a directory should return ENOTDIR. PiperOrigin-RevId: 455271653 --- pkg/sentry/vfs/anonfs.go | 22 ++++++------ pkg/sentry/vfs/resolving_path.go | 2 +- test/syscalls/linux/BUILD | 1 + test/syscalls/linux/proc.cc | 59 ++++++++++++++++++-------------- 4 files changed, 47 insertions(+), 37 deletions(-) diff --git a/pkg/sentry/vfs/anonfs.go b/pkg/sentry/vfs/anonfs.go index 255d3992e..f34770c77 100644 --- a/pkg/sentry/vfs/anonfs.go +++ b/pkg/sentry/vfs/anonfs.go @@ -100,7 +100,7 @@ func (fs *anonFilesystem) Sync(ctx context.Context) error { // AccessAt implements vfs.Filesystem.Impl.AccessAt. func (fs *anonFilesystem) AccessAt(ctx context.Context, rp *ResolvingPath, creds *auth.Credentials, ats AccessTypes) error { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return linuxerr.ENOTDIR } return GenericCheckPermissions(creds, ats, anonFileMode, anonFileUID, anonFileGID) @@ -108,7 +108,7 @@ func (fs *anonFilesystem) AccessAt(ctx context.Context, rp *ResolvingPath, creds // GetDentryAt implements FilesystemImpl.GetDentryAt. func (fs *anonFilesystem) GetDentryAt(ctx context.Context, rp *ResolvingPath, opts GetDentryOptions) (*Dentry, error) { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return nil, linuxerr.ENOTDIR } if opts.CheckSearchable { @@ -153,7 +153,7 @@ func (fs *anonFilesystem) MknodAt(ctx context.Context, rp *ResolvingPath, opts M // OpenAt implements FilesystemImpl.OpenAt. func (fs *anonFilesystem) OpenAt(ctx context.Context, rp *ResolvingPath, opts OpenOptions) (*FileDescription, error) { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return nil, linuxerr.ENOTDIR } return nil, linuxerr.ENODEV @@ -161,7 +161,7 @@ func (fs *anonFilesystem) OpenAt(ctx context.Context, rp *ResolvingPath, opts Op // ReadlinkAt implements FilesystemImpl.ReadlinkAt. func (fs *anonFilesystem) ReadlinkAt(ctx context.Context, rp *ResolvingPath) (string, error) { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return "", linuxerr.ENOTDIR } return "", linuxerr.EINVAL @@ -185,7 +185,7 @@ func (fs *anonFilesystem) RmdirAt(ctx context.Context, rp *ResolvingPath) error // SetStatAt implements FilesystemImpl.SetStatAt. func (fs *anonFilesystem) SetStatAt(ctx context.Context, rp *ResolvingPath, opts SetStatOptions) error { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return linuxerr.ENOTDIR } // Linux actually permits anon_inode_inode's metadata to be set, which is @@ -196,7 +196,7 @@ func (fs *anonFilesystem) SetStatAt(ctx context.Context, rp *ResolvingPath, opts // StatAt implements FilesystemImpl.StatAt. func (fs *anonFilesystem) StatAt(ctx context.Context, rp *ResolvingPath, opts StatOptions) (linux.Statx, error) { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return linux.Statx{}, linuxerr.ENOTDIR } // See fs/anon_inodes.c:anon_inode_init() => fs/libfs.c:alloc_anon_inode(). @@ -217,7 +217,7 @@ func (fs *anonFilesystem) StatAt(ctx context.Context, rp *ResolvingPath, opts St // StatFSAt implements FilesystemImpl.StatFSAt. func (fs *anonFilesystem) StatFSAt(ctx context.Context, rp *ResolvingPath) (linux.Statfs, error) { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return linux.Statfs{}, linuxerr.ENOTDIR } return linux.Statfs{ @@ -255,7 +255,7 @@ func (fs *anonFilesystem) BoundEndpointAt(ctx context.Context, rp *ResolvingPath // ListXattrAt implements FilesystemImpl.ListXattrAt. func (fs *anonFilesystem) ListXattrAt(ctx context.Context, rp *ResolvingPath, size uint64) ([]string, error) { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return nil, linuxerr.ENOTDIR } return nil, nil @@ -263,7 +263,7 @@ func (fs *anonFilesystem) ListXattrAt(ctx context.Context, rp *ResolvingPath, si // GetXattrAt implements FilesystemImpl.GetXattrAt. func (fs *anonFilesystem) GetXattrAt(ctx context.Context, rp *ResolvingPath, opts GetXattrOptions) (string, error) { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return "", linuxerr.ENOTDIR } return "", linuxerr.ENOTSUP @@ -271,7 +271,7 @@ func (fs *anonFilesystem) GetXattrAt(ctx context.Context, rp *ResolvingPath, opt // SetXattrAt implements FilesystemImpl.SetXattrAt. func (fs *anonFilesystem) SetXattrAt(ctx context.Context, rp *ResolvingPath, opts SetXattrOptions) error { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return linuxerr.ENOTDIR } return linuxerr.EPERM @@ -279,7 +279,7 @@ func (fs *anonFilesystem) SetXattrAt(ctx context.Context, rp *ResolvingPath, opt // RemoveXattrAt implements FilesystemImpl.RemoveXattrAt. func (fs *anonFilesystem) RemoveXattrAt(ctx context.Context, rp *ResolvingPath, name string) error { - if !rp.Done() { + if !rp.Done() || rp.MustBeDir() { return linuxerr.ENOTDIR } return linuxerr.EPERM diff --git a/pkg/sentry/vfs/resolving_path.go b/pkg/sentry/vfs/resolving_path.go index 028801956..128c66fcf 100644 --- a/pkg/sentry/vfs/resolving_path.go +++ b/pkg/sentry/vfs/resolving_path.go @@ -377,7 +377,7 @@ func (rp *ResolvingPath) relpathPrepend(path fspath.Path) { // HandleJump is called when the current path component is a "magic" link to // the given VirtualDentry, like /proc/[pid]/fd/[fd]. If the calling Filesystem -// method should continue path traversal, HandleMagicSymlink updates the path +// method should continue path traversal, HandleJump updates the path // component stream to reflect the magic link target and returns nil. Otherwise // it returns a non-nil error. // diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index 9919c710d..f496864d5 100644 --- a/test/syscalls/linux/BUILD +++ b/test/syscalls/linux/BUILD @@ -1777,6 +1777,7 @@ cc_binary( "@com_google_absl//absl/synchronization", "@com_google_absl//absl/time", gtest, + "//test/util:eventfd_util", "//test/util:memory_util", "//test/util:multiprocess_util", "//test/util:posix_error", diff --git a/test/syscalls/linux/proc.cc b/test/syscalls/linux/proc.cc index 7a8190dda..52921334e 100644 --- a/test/syscalls/linux/proc.cc +++ b/test/syscalls/linux/proc.cc @@ -63,6 +63,7 @@ #include "absl/time/time.h" #include "test/util/capability_util.h" #include "test/util/cleanup.h" +#include "test/util/eventfd_util.h" #include "test/util/file_descriptor.h" #include "test/util/fs_util.h" #include "test/util/memory_util.h" @@ -1662,32 +1663,30 @@ TEST(ProcPidStatusTest, HasBasicFields) { Pair("PPid", absl::StrCat(getppid())), })); - uid_t ruid, euid, suid; - ASSERT_THAT(getresuid(&ruid, &euid, &suid), SyscallSucceeds()); - gid_t rgid, egid, sgid; - ASSERT_THAT(getresgid(&rgid, &egid, &sgid), SyscallSucceeds()); - std::vector supplementary_gids; - int ngids = getgroups(0, nullptr); - supplementary_gids.resize(ngids); - ASSERT_THAT(getgroups(ngids, supplementary_gids.data()), - SyscallSucceeds()); + uid_t ruid, euid, suid; + ASSERT_THAT(getresuid(&ruid, &euid, &suid), SyscallSucceeds()); + gid_t rgid, egid, sgid; + ASSERT_THAT(getresgid(&rgid, &egid, &sgid), SyscallSucceeds()); + std::vector supplementary_gids; + int ngids = getgroups(0, nullptr); + supplementary_gids.resize(ngids); + ASSERT_THAT(getgroups(ngids, supplementary_gids.data()), SyscallSucceeds()); - EXPECT_THAT( - status, - IsSupersetOf(std::vector< - ::testing::Matcher>>{ - // gVisor doesn't support fsuid/gid, and even if it did there is - // no getfsuid/getfsgid(). - Pair("Uid", StartsWith(absl::StrFormat("%d\t%d\t%d\t", ruid, euid, - suid))), - Pair("Gid", StartsWith(absl::StrFormat("%d\t%d\t%d\t", rgid, egid, - sgid))), - // ParseProcStatus strips leading whitespace for each value, - // so if the Groups line is empty then the trailing space is - // stripped. - Pair("Groups", - StartsWith(absl::StrJoin(supplementary_gids, " "))), - })); + EXPECT_THAT( + status, + IsSupersetOf(std::vector< + ::testing::Matcher>>{ + // gVisor doesn't support fsuid/gid, and even if it did there is + // no getfsuid/getfsgid(). + Pair("Uid", + StartsWith(absl::StrFormat("%d\t%d\t%d\t", ruid, euid, suid))), + Pair("Gid", + StartsWith(absl::StrFormat("%d\t%d\t%d\t", rgid, egid, sgid))), + // ParseProcStatus strips leading whitespace for each value, + // so if the Groups line is empty then the trailing space is + // stripped. + Pair("Groups", StartsWith(absl::StrJoin(supplementary_gids, " "))), + })); }); } @@ -2734,6 +2733,16 @@ TEST(Proc, ResolveSymlinkToProc) { EXPECT_EQ(target, JoinPath("/proc/", absl::StrCat(getpid()), "/cmdline")); } +// NOTE(b/236035339): Tests that opening /proc/[pid]/fd/[eventFDNum] with +// O_DIRECTORY leads to ENOTDIR. +TEST(Proc, RegressionTestB236035339) { + FileDescriptor efd = + ASSERT_NO_ERRNO_AND_VALUE(NewEventFD(0, EFD_NONBLOCK | EFD_CLOEXEC)); + const auto path = JoinPath("/proc/self/fd/", absl::StrCat(efd.get())); + EXPECT_THAT(open(path.c_str(), O_RDONLY | O_CLOEXEC | O_DIRECTORY), + SyscallFailsWithErrno(ENOTDIR)); +} + } // namespace } // namespace testing } // namespace gvisor From 9dfa65ebdb74f35c3bfbdebf6ee87af6073733c0 Mon Sep 17 00:00:00 2001 From: Adin Scannell Date: Thu, 16 Jun 2022 08:56:19 -0700 Subject: [PATCH 21/29] checklocks: make behavior configurable. Defaults are the same, but certain behaviors can now be disabled to more easily use the analyzer on its own. Now wrappers, lock inferrence and atomic default analysis can be disabled. For example: ``` go vet -vettool=$HOME/go/bin/checklocks -wrappers=false -inferred=false -atomic=false ./... ``` Fixes #7721 PiperOrigin-RevId: 455393957 --- tools/checklocks/analysis.go | 4 ++++ tools/checklocks/annotations.go | 3 +++ tools/checklocks/checklocks.go | 16 +++++++++++++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/tools/checklocks/analysis.go b/tools/checklocks/analysis.go index 8a8f18ea5..f1ab91c47 100644 --- a/tools/checklocks/analysis.go +++ b/tools/checklocks/analysis.go @@ -268,6 +268,10 @@ func (pc *passContext) checkGuards(inst almostInst, from ssa.Value, accessObj ty pc.maybeFail(inst.Pos(), "non-atomic write of field %s, writes must still be atomic with locks held (locks: %s)", accessObj.Name(), ls.String()) } case atomicDisallow: + // If atomic analysis is not enabled, skip. + if !enableAtomic { + break + } // Check that this is *not* used atomically. if refs := inst.Referrers(); refs != nil { for _, otherInst := range *refs { diff --git a/tools/checklocks/annotations.go b/tools/checklocks/annotations.go index 950168ee1..9e328edbb 100644 --- a/tools/checklocks/annotations.go +++ b/tools/checklocks/annotations.go @@ -91,6 +91,9 @@ func (pc *passContext) maybeFail(pos token.Pos, fmtStr string, args ...interface if _, ok := pc.exemptions[pc.positionKey(pos)]; ok { return // Ignored, not counted. } + if !enableWrappers && !pos.IsValid() { + return // Ignored, implicit. + } pc.pass.Reportf(pos, fmtStr, args...) } diff --git a/tools/checklocks/checklocks.go b/tools/checklocks/checklocks.go index 939af4239..70921a15c 100644 --- a/tools/checklocks/checklocks.go +++ b/tools/checklocks/checklocks.go @@ -41,6 +41,18 @@ var Analyzer = &analysis.Analyzer{ }, } +var ( + enableInferred = true + enableAtomic = true + enableWrappers = true +) + +func init() { + Analyzer.Flags.BoolVar(&enableInferred, "inferred", true, "enable inferred locks") + Analyzer.Flags.BoolVar(&enableAtomic, "atomic", true, "enable atomic checks") + Analyzer.Flags.BoolVar(&enableWrappers, "wrappers", true, "enable analysis of wrappers") +} + // objectObservations tracks lock correlations. type objectObservations struct { counts map[types.Object]int @@ -187,7 +199,9 @@ func run(pass *analysis.Pass) (interface{}, error) { } // Check for inferred checklocks annotations. - pc.checkInferred() + if enableInferred { + pc.checkInferred() + } // Check for expected failures. pc.checkFailures() From 5dc4c42f8f91f5b604bef187bb5e507739e04b12 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Thu, 16 Jun 2022 10:02:36 -0700 Subject: [PATCH 22/29] Add more trace point integration tests Updates #4805 PiperOrigin-RevId: 455408137 --- test/trace/BUILD | 1 + test/trace/trace_test.go | 201 +++++++++++++++++++++++++++++--- test/trace/workload/BUILD | 7 ++ test/trace/workload/workload.cc | 114 +++++++++++++++++- 4 files changed, 306 insertions(+), 17 deletions(-) diff --git a/test/trace/BUILD b/test/trace/BUILD index 74597ab6d..bac34d6b2 100644 --- a/test/trace/BUILD +++ b/test/trace/BUILD @@ -22,6 +22,7 @@ go_test( "//pkg/test/testutil", "//test/trace/config", "@org_golang_google_protobuf//proto:go_default_library", + "@org_golang_x_sys//unix:go_default_library", ], ) diff --git a/test/trace/trace_test.go b/test/trace/trace_test.go index 617c7f0b8..ebd3846eb 100644 --- a/test/trace/trace_test.go +++ b/test/trace/trace_test.go @@ -23,6 +23,7 @@ import ( "testing" "time" + "golang.org/x/sys/unix" "google.golang.org/protobuf/proto" "gvisor.dev/gvisor/pkg/sentry/seccheck" "gvisor.dev/gvisor/pkg/sentry/seccheck/checkers/remote/test" @@ -31,6 +32,8 @@ import ( "gvisor.dev/gvisor/test/trace/config" ) +var cutoffTime time.Time + // TestAll enabled all trace points in the system with all optional and context // fields enabled. Then it runs a workload that will trigger those points and // run some basic validation over the points generated. @@ -68,6 +71,8 @@ func TestAll(t *testing.T) { if err != nil { t.Fatal(err) } + // No trace point should have a time lesser than this. + cutoffTime = time.Now() cmd := exec.Command( runsc, "--debug", "--alsologtostderr", // Debug logging for troubleshooting @@ -75,10 +80,10 @@ func TestAll(t *testing.T) { "--pod-init-config", cfgFile.Name(), "do", workload) out, err := cmd.CombinedOutput() + t.Log(string(out)) if err != nil { t.Fatalf("runsc do: %v", err) } - t.Log(string(out)) // Wait until the sandbox disconnects to ensure all points were gathered. server.WaitForNoClients() @@ -91,12 +96,18 @@ func matchPoints(t *testing.T, msgs []test.Message) { checker func(test.Message) error count int }{ - pb.MessageType_MESSAGE_CONTAINER_START: {checker: checkContainerStart}, - pb.MessageType_MESSAGE_SENTRY_TASK_EXIT: {checker: checkSentryTaskExit}, - pb.MessageType_MESSAGE_SYSCALL_RAW: {checker: checkSyscallRaw}, - pb.MessageType_MESSAGE_SYSCALL_OPEN: {checker: checkSyscallOpen}, - pb.MessageType_MESSAGE_SYSCALL_CLOSE: {checker: checkSyscallClose}, - pb.MessageType_MESSAGE_SYSCALL_READ: {checker: checkSyscallRead}, + pb.MessageType_MESSAGE_CONTAINER_START: {checker: checkContainerStart}, + pb.MessageType_MESSAGE_SENTRY_CLONE: {checker: checkSentryClone}, + pb.MessageType_MESSAGE_SENTRY_EXEC: {checker: checkSentryExec}, + pb.MessageType_MESSAGE_SENTRY_EXIT_NOTIFY_PARENT: {checker: checkSentryExitNotifyParent}, + pb.MessageType_MESSAGE_SENTRY_TASK_EXIT: {checker: checkSentryTaskExit}, + pb.MessageType_MESSAGE_SYSCALL_CLOSE: {checker: checkSyscallClose}, + pb.MessageType_MESSAGE_SYSCALL_CONNECT: {checker: checkSyscallConnect}, + pb.MessageType_MESSAGE_SYSCALL_EXECVE: {checker: checkSyscallExecve}, + pb.MessageType_MESSAGE_SYSCALL_OPEN: {checker: checkSyscallOpen}, + pb.MessageType_MESSAGE_SYSCALL_RAW: {checker: checkSyscallRaw}, + pb.MessageType_MESSAGE_SYSCALL_READ: {checker: checkSyscallRead}, + pb.MessageType_MESSAGE_SYSCALL_SOCKET: {checker: checkSyscallSocket}, } for _, msg := range msgs { t.Logf("Processing message type %v", msg.MsgType) @@ -120,7 +131,22 @@ func matchPoints(t *testing.T, msgs []test.Message) { } } +func checkTimeNs(ns int64) error { + if ns <= int64(cutoffTime.Nanosecond()) { + return fmt.Errorf("time should not be less than %d (%v), got: %d (%v)", cutoffTime.Nanosecond(), cutoffTime, ns, time.Unix(0, ns)) + } + return nil +} + +type contextDataOpts struct { + skipCwd bool +} + func checkContextData(data *pb.ContextData) error { + return checkContextDataOpts(data, contextDataOpts{}) +} + +func checkContextDataOpts(data *pb.ContextData, opts contextDataOpts) error { if data == nil { return fmt.Errorf("ContextData should not be nil") } @@ -128,18 +154,17 @@ func checkContextData(data *pb.ContextData) error { return fmt.Errorf("invalid container ID %q", data.ContainerId) } - cutoff := time.Now().Add(-time.Minute) - if data.TimeNs <= int64(cutoff.Nanosecond()) { - return fmt.Errorf("time should not be less than %d (%v), got: %d (%v)", cutoff.Nanosecond(), cutoff, data.TimeNs, time.Unix(0, data.TimeNs)) + if err := checkTimeNs(data.TimeNs); err != nil { + return err } - if data.ThreadStartTimeNs <= int64(cutoff.Nanosecond()) { - return fmt.Errorf("thread_start_time should not be less than %d (%v), got: %d (%v)", cutoff.Nanosecond(), cutoff, data.ThreadStartTimeNs, time.Unix(0, data.ThreadStartTimeNs)) + if err := checkTimeNs(data.ThreadStartTimeNs); err != nil { + return err } if data.ThreadStartTimeNs > data.TimeNs { return fmt.Errorf("thread_start_time should not be greater than point time: %d (%v), got: %d (%v)", data.TimeNs, time.Unix(0, data.TimeNs), data.ThreadStartTimeNs, time.Unix(0, data.ThreadStartTimeNs)) } - if data.ThreadGroupStartTimeNs <= int64(cutoff.Nanosecond()) { - return fmt.Errorf("thread_group_start_time should not be less than %d (%v), got: %d (%v)", cutoff.Nanosecond(), cutoff, data.ThreadGroupStartTimeNs, time.Unix(0, data.ThreadGroupStartTimeNs)) + if err := checkTimeNs(data.ThreadGroupStartTimeNs); err != nil { + return err } if data.ThreadGroupStartTimeNs > data.TimeNs { return fmt.Errorf("thread_group_start_time should not be greater than point time: %d (%v), got: %d (%v)", data.TimeNs, time.Unix(0, data.TimeNs), data.ThreadGroupStartTimeNs, time.Unix(0, data.ThreadGroupStartTimeNs)) @@ -151,7 +176,7 @@ func checkContextData(data *pb.ContextData) error { if data.ThreadGroupId <= 0 { return fmt.Errorf("invalid thread_group_id: %v", data.ThreadGroupId) } - if len(data.Cwd) == 0 { + if !opts.skipCwd && len(data.Cwd) == 0 { return fmt.Errorf("invalid cwd: %v", data.Cwd) } if len(data.ProcessName) == 0 { @@ -262,3 +287,149 @@ func checkSyscallRead(msg test.Message) error { } return nil } + +func checkSentryClone(msg test.Message) error { + p := pb.CloneInfo{} + if err := proto.Unmarshal(msg.Msg, &p); err != nil { + return err + } + if err := checkContextData(p.ContextData); err != nil { + return err + } + if p.CreatedThreadId < 0 { + return fmt.Errorf("invalid TID: %d", p.CreatedThreadId) + } + if p.CreatedThreadGroupId < 0 { + return fmt.Errorf("invalid TGID: %d", p.CreatedThreadGroupId) + } + if p.CreatedThreadStartTimeNs < 0 { + return fmt.Errorf("invalid TID: %d", p.CreatedThreadId) + } + return checkTimeNs(p.CreatedThreadStartTimeNs) +} + +func checkSentryExec(msg test.Message) error { + p := pb.ExecveInfo{} + if err := proto.Unmarshal(msg.Msg, &p); err != nil { + return err + } + if err := checkContextData(p.ContextData); err != nil { + return err + } + if want := "/bin/true"; want != p.BinaryPath { + return fmt.Errorf("wrong BinaryPath, want: %q, got: %q", want, p.BinaryPath) + } + if len(p.Argv) == 0 { + return fmt.Errorf("empty Argv") + } + if p.Argv[0] != p.BinaryPath { + return fmt.Errorf("wrong Argv[0], want: %q, got: %q", p.BinaryPath, p.Argv[0]) + } + if len(p.Env) == 0 { + return fmt.Errorf("empty Env") + } + if want := "TEST=123"; want != p.Env[0] { + return fmt.Errorf("wrong Env[0], want: %q, got: %q", want, p.Env[0]) + } + if (p.BinaryMode & 0111) == 0 { + return fmt.Errorf("executing non-executable file, mode: %#o (%#x)", p.BinaryMode, p.BinaryMode) + } + const nobody = 65534 + if p.BinaryUid != nobody { + return fmt.Errorf("BinaryUid, want: %d, got: %d", nobody, p.BinaryUid) + } + if p.BinaryGid != nobody { + return fmt.Errorf("BinaryGid, want: %d, got: %d", nobody, p.BinaryGid) + } + return nil +} + +func checkSyscallExecve(msg test.Message) error { + p := pb.Execve{} + if err := proto.Unmarshal(msg.Msg, &p); err != nil { + return err + } + if err := checkContextData(p.ContextData); err != nil { + return err + } + if p.Fd < 3 { + return fmt.Errorf("execve invalid FD: %d", p.Fd) + } + if want := "/"; want != p.FdPath { + return fmt.Errorf("wrong FdPath, want: %q, got: %q", want, p.FdPath) + } + if want := "/bin/true"; want != p.Pathname { + return fmt.Errorf("wrong Pathname, want: %q, got: %q", want, p.Pathname) + } + if len(p.Argv) == 0 { + return fmt.Errorf("empty Argv") + } + if p.Argv[0] != p.Pathname { + return fmt.Errorf("wrong Argv[0], want: %q, got: %q", p.Pathname, p.Argv[0]) + } + if len(p.Envv) == 0 { + return fmt.Errorf("empty Envv") + } + if want := "TEST=123"; want != p.Envv[0] { + return fmt.Errorf("wrong Envv[0], want: %q, got: %q", want, p.Envv[0]) + } + return nil +} + +func checkSentryExitNotifyParent(msg test.Message) error { + p := pb.ExitNotifyParentInfo{} + if err := proto.Unmarshal(msg.Msg, &p); err != nil { + return err + } + // cwd is empty because the task has already been destroyed when the point + // fires. + opts := contextDataOpts{skipCwd: true} + if err := checkContextDataOpts(p.ContextData, opts); err != nil { + return err + } + if p.ExitStatus != 0 { + return fmt.Errorf("wrong ExitStatus, want: 0, got: %d", p.ExitStatus) + } + return nil +} + +func checkSyscallConnect(msg test.Message) error { + p := pb.Connect{} + if err := proto.Unmarshal(msg.Msg, &p); err != nil { + return err + } + if err := checkContextData(p.ContextData); err != nil { + return err + } + if p.Fd < 3 { + return fmt.Errorf("invalid FD: %d", p.Fd) + } + if want := "socket:"; !strings.HasPrefix(p.FdPath, want) { + return fmt.Errorf("FdPath should start with %q, got: %q", want, p.FdPath) + } + if len(p.Address) == 0 { + return fmt.Errorf("empty address: %q", string(p.Address)) + } + + return nil +} + +func checkSyscallSocket(msg test.Message) error { + p := pb.Socket{} + if err := proto.Unmarshal(msg.Msg, &p); err != nil { + return err + } + if err := checkContextData(p.ContextData); err != nil { + return err + } + if want := unix.AF_UNIX; int32(want) != p.Domain { + return fmt.Errorf("wrong Domain, want: %v, got: %v", want, p.Domain) + } + if want := unix.SOCK_STREAM; int32(want) != p.Type { + return fmt.Errorf("wrong Type, want: %v, got: %v", want, p.Type) + } + if want := int32(0); want != p.Protocol { + return fmt.Errorf("wrong Protocol, want: %v, got: %v", want, p.Protocol) + } + return nil +} diff --git a/test/trace/workload/BUILD b/test/trace/workload/BUILD index fcca7b93b..7391ac2c8 100644 --- a/test/trace/workload/BUILD +++ b/test/trace/workload/BUILD @@ -10,5 +10,12 @@ cc_binary( ], visibility = ["//test/trace:__pkg__"], deps = [ + "//test/util:file_descriptor", + "//test/util:multiprocess_util", + "//test/util:posix_error", + "//test/util:test_util", + "@com_google_absl//absl/cleanup", + "@com_google_absl//absl/strings", + "@com_google_absl//absl/time", ], ) diff --git a/test/trace/workload/workload.cc b/test/trace/workload/workload.cc index 72eee9bc6..457cafb5b 100644 --- a/test/trace/workload/workload.cc +++ b/test/trace/workload/workload.cc @@ -12,5 +12,115 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Empty for now. Actual workload will be added as more points are covered. -int main(int argc, char** argv) { return 0; } +#include +#include +#include +#include + +#include "absl/cleanup/cleanup.h" +#include "absl/strings/str_cat.h" +#include "absl/time/clock.h" +#include "test/util/file_descriptor.h" +#include "test/util/multiprocess_util.h" +#include "test/util/posix_error.h" +#include "test/util/test_util.h" + +namespace gvisor { +namespace testing { + +void runForkExecve() { + auto root_or_error = Open("/", O_RDONLY, 0); + auto& root = root_or_error.ValueOrDie(); + + pid_t child; + int execve_errno; + ExecveArray argv = {"/bin/true"}; + ExecveArray envv = {"TEST=123"}; + auto kill_or_error = ForkAndExecveat(root.get(), "/bin/true", argv, envv, 0, + nullptr, &child, &execve_errno); + ASSERT_EQ(0, execve_errno); + + // Don't kill child, just wait for gracefully exit. + kill_or_error.ValueOrDie().Release(); + RetryEINTR(waitpid)(child, nullptr, 0); +} + +// Creates a simple UDS in the abstract namespace and send one byte from the +// client to the server. +void runSocket() { + auto path = absl::StrCat(std::string("\0", 1), "trace_test.", getpid(), + absl::GetCurrentTimeNanos()); + + struct sockaddr_un addr; + addr.sun_family = AF_UNIX; + strncpy(addr.sun_path, path.c_str(), path.size() + 1); + + int parent_sock = socket(AF_UNIX, SOCK_STREAM, 0); + if (parent_sock < 0) { + err(1, "socket"); + } + auto sock_closer = absl::MakeCleanup([parent_sock] { close(parent_sock); }); + + if (bind(parent_sock, reinterpret_cast(&addr), + sizeof(addr))) { + err(1, "bind"); + } + if (listen(parent_sock, 5) < 0) { + err(1, "listen"); + } + + pid_t pid = fork(); + if (pid < 0) { + // Fork error. + err(1, "fork"); + + } else if (pid == 0) { + // Child. + close(parent_sock); // ensure it's not mistakely used in child. + + int server = socket(AF_UNIX, SOCK_STREAM, 0); + if (server < 0) { + err(1, "socket"); + } + auto server_closer = absl::MakeCleanup([server] { close(server); }); + + if (connect(server, reinterpret_cast(&addr), + sizeof(addr)) < 0) { + err(1, "connect"); + } + + char buf = 'A'; + int bytes = write(server, &buf, sizeof(buf)); + if (bytes != 1) { + err(1, "write: %d", bytes); + } + exit(0); + + } else { + // Parent. + int client = RetryEINTR(accept)(parent_sock, nullptr, nullptr); + if (client < 0) { + err(1, "accept"); + } + auto client_closer = absl::MakeCleanup([client] { close(client); }); + + char buf; + int bytes = read(client, &buf, sizeof(buf)); + if (bytes != 1) { + err(1, "read: %d", bytes); + } + + // Wait to reap the child. + RetryEINTR(waitpid)(pid, nullptr, 0); + } +} + +} // namespace testing +} // namespace gvisor + +int main(int argc, char** argv) { + ::gvisor::testing::runForkExecve(); + ::gvisor::testing::runSocket(); + + return 0; +} From 72295623b015b0a73d02af0b39cba7d11b29b177 Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Thu, 16 Jun 2022 10:36:04 -0700 Subject: [PATCH 23/29] `bazel.mk`: Set Docker hostnames for all containers. This makes it easier to tell where each command is being run than a random-generated string. PiperOrigin-RevId: 455416726 --- tools/bazel.mk | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tools/bazel.mk b/tools/bazel.mk index bbbfbc9da..37d9ea9de 100644 --- a/tools/bazel.mk +++ b/tools/bazel.mk @@ -24,6 +24,7 @@ ## USER - The in-container user. ## DOCKER_RUN_OPTIONS - Options for the container (default: --privileged, required for tests). ## DOCKER_NAME - The container name (default: gvisor-bazel-HASH). +## DOCKER_HOSTNAME - The container name (default: same as DOCKER_NAME). ## DOCKER_PRIVILEGED - Docker privileged flags (default: --privileged). ## PRE_BAZEL_INIT - If set, run this command with bash outside the Bazel ## server container. @@ -50,7 +51,9 @@ RACE_FLAGS := --@io_bazel_rules_go//go/config:race USER := $(shell whoami) HASH := $(shell realpath -m $(CURDIR) | md5sum | cut -c1-8) BUILDER_NAME := gvisor-builder-$(HASH)-$(ARCH) +BUILDER_HOSTNAME := $(BUILDER_NAME) DOCKER_NAME := gvisor-bazel-$(HASH)-$(ARCH) +DOCKER_HOSTNAME := $(DOCKER_NAME) DOCKER_PRIVILEGED := --privileged BAZEL_CACHE := $(HOME)/.cache/bazel/ GCLOUD_CONFIG := $(HOME)/.config/gcloud/ @@ -182,7 +185,9 @@ bazel-alias: ## Emits an alias that can be used within the shell. bazel-image: load-default ## Ensures that the local builder exists. @$(call header,DOCKER BUILD) @docker rm -f $(BUILDER_NAME) 2>/dev/null || true - @docker run --user 0:0 --entrypoint "" --name $(BUILDER_NAME) gvisor.dev/images/default \ + @docker run --user 0:0 --entrypoint "" \ + --name $(BUILDER_NAME) --hostname $(BUILDER_HOSTNAME) \ + gvisor.dev/images/default \ bash -c "$(GROUPADD_DOCKER) $(USERADD_DOCKER) if test -e /dev/kvm; then chmod a+rw /dev/kvm; fi" >&2 @docker commit $(BUILDER_NAME) gvisor.dev/images/builder >&2 .PHONY: bazel-image @@ -197,7 +202,7 @@ endif @docker rm -f $(DOCKER_NAME) 2>/dev/null || true @mkdir -p $(BAZEL_CACHE) @mkdir -p $(GCLOUD_CONFIG) - @docker run -d --name $(DOCKER_NAME) \ + @docker run -d --name $(DOCKER_NAME) --hostname $(DOCKER_HOSTNAME) \ -v "$(CURDIR):$(CURDIR)" \ --workdir "$(CURDIR)" \ $(DOCKER_RUN_OPTIONS) \ From 1b604ce01053e0acae0138b7e345da50afe2007f Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Thu, 16 Jun 2022 10:43:42 -0700 Subject: [PATCH 24/29] Pass on systemd-related files to Docker containers if present. This allows control of the Docker container via `systemd` from within the containers themselves. This is useful for tests that require restarting Docker, e.g. after installing a new runtime or enabling Docker's `experimental` mode. PiperOrigin-RevId: 455418499 --- .buildkite/hooks/pre-command | 21 ++++++++++++++++----- tools/bazel.mk | 10 ++++++++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/.buildkite/hooks/pre-command b/.buildkite/hooks/pre-command index 53225aeaf..a64a82e73 100644 --- a/.buildkite/hooks/pre-command +++ b/.buildkite/hooks/pre-command @@ -13,8 +13,8 @@ function install_pkgs() { done } install_pkgs make linux-libc-dev graphviz jq curl binutils gnupg gnupg-agent \ - gcc pkg-config apt-transport-https ca-certificates software-properties-common \ - rsync kmod + gcc pkg-config apt-transport-https ca-certificates \ + software-properties-common rsync kmod systemd # Install headers, only if available. if test -n "$(apt-cache search --names-only "^linux-headers-$(uname -r)$")"; then @@ -34,9 +34,20 @@ export TOTAL_PARTITIONS=${BUILDKITE_PARALLEL_JOB_COUNT:-1} export RUNTIME="${BUILDKITE_BRANCH}-${BUILDKITE_BUILD_ID}" # Ensure Docker has experimental enabled. -EXPERIMENTAL=$(sudo docker version --format='{{.Server.Experimental}}') -make sudo TARGETS=//runsc:runsc \ - ARGS="install --experimental=true --runtime=${RUNTIME} -- ${RUNTIME_ARGS:-}" +if [[ -n "${STAGED_BINARIES:-}" ]]; then + # Used `runsc` from STAGED_BINARIES instead of building it from scratch. + tmpdir="$(mktemp -d)" + gsutil cat "$(STAGED_BINARIES)" | tar -C "$tmpdir" -zxvf - runsc + chmod +x "$tmpdir/runsc" + "$tmpdir/runsc" install --experimental=true --runtime="${RUNTIME}" \ + -- "${RUNTIME_ARGS:-}" + rm -rf "$tmpdir" +else + make sudo TARGETS=//runsc:runsc \ + ARGS="install --experimental=true --runtime=${RUNTIME} -- ${RUNTIME_ARGS:-}" +fi +# WARNING: We may be running in a container when this command executes. +# This only makes sense if Docker's `live-restore` feature is enabled. sudo systemctl restart docker # Helper for benchmarks, based on the branch. diff --git a/tools/bazel.mk b/tools/bazel.mk index 37d9ea9de..4d3a71bc8 100644 --- a/tools/bazel.mk +++ b/tools/bazel.mk @@ -111,6 +111,16 @@ DOCKER_RUN_OPTIONS += -v "$(KERNEL_HEADERS_DIR_LINKED):$(KERNEL_HEADERS_DIR_LINK endif endif +# Same for systemd-related files and directories. This allows control of systemd +# from within the container, which is useful for tests that need to e.g. restart +# docker. +ifneq (,$(wildcard /run/systemd/system)) +DOCKER_RUN_OPTIONS += -v "/run/systemd/system:/run/systemd/system" +endif +ifneq (,$(wildcard /var/run/dbus/system_bus_socket)) +DOCKER_RUN_OPTIONS += -v "/var/run/dbus/system_bus_socket:/var/run/dbus/system_bus_socket" +endif + # Add basic UID/GID options. # # Note that USERADD_DOCKER and GROUPADD_DOCKER are both defined as "deferred" From 7ef9f97f41251705afce3b4bf448c50b9c17e8d1 Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Thu, 16 Jun 2022 11:37:43 -0700 Subject: [PATCH 25/29] Suppress the known lock issue It is triggered too often in syzkaller jobs. PiperOrigin-RevId: 455431173 --- pkg/sentry/mm/procfs.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/pkg/sentry/mm/procfs.go b/pkg/sentry/mm/procfs.go index f4331239c..6358e9c14 100644 --- a/pkg/sentry/mm/procfs.go +++ b/pkg/sentry/mm/procfs.go @@ -60,8 +60,10 @@ func (mm *MemoryManager) NeedsUpdate(generation int64) bool { // ReadMapsDataInto is called by fsimpl/proc.mapsData.Generate to // implement /proc/[pid]/maps. func (mm *MemoryManager) ReadMapsDataInto(ctx context.Context, buf *bytes.Buffer) { - mm.mappingMu.RLock() - defer mm.mappingMu.RUnlock() + // FIXME(b/235153601): Need to replace RLockBypass with RLockBypass + // after fixing b/235153601. + mm.mappingMu.RLockBypass() + defer mm.mappingMu.RUnlockBypass() var start hostarch.Addr for vseg := mm.vmas.LowerBoundSegment(start); vseg.Ok(); vseg = vseg.NextSegment() { @@ -177,8 +179,10 @@ func (mm *MemoryManager) appendVMAMapsEntryLocked(ctx context.Context, vseg vmaI // ReadSmapsDataInto is called by fsimpl/proc.smapsData.Generate to // implement /proc/[pid]/maps. func (mm *MemoryManager) ReadSmapsDataInto(ctx context.Context, buf *bytes.Buffer) { - mm.mappingMu.RLock() - defer mm.mappingMu.RUnlock() + // FIXME(b/235153601): Need to replace RLockBypass with RLockBypass + // after fixing b/235153601. + mm.mappingMu.RLockBypass() + defer mm.mappingMu.RUnlockBypass() var start hostarch.Addr for vseg := mm.vmas.LowerBoundSegment(start); vseg.Ok(); vseg = vseg.NextSegment() { @@ -195,8 +199,10 @@ func (mm *MemoryManager) ReadSmapsDataInto(ctx context.Context, buf *bytes.Buffe // ReadSmapsSeqFileData is called by fs/proc.smapsData.ReadSeqFileData to // implement /proc/[pid]/smaps. func (mm *MemoryManager) ReadSmapsSeqFileData(ctx context.Context, handle seqfile.SeqHandle) ([]seqfile.SeqData, int64) { - mm.mappingMu.RLock() - defer mm.mappingMu.RUnlock() + // FIXME(b/235153601): Need to replace RLockBypass with RLockBypass + // after fixing b/235153601. + mm.mappingMu.RLockBypass() + defer mm.mappingMu.RUnlockBypass() var data []seqfile.SeqData var start hostarch.Addr if handle != nil { From 35227fb84fd0d9a35ece6d43834ce3099d3944fc Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Thu, 16 Jun 2022 13:33:01 -0700 Subject: [PATCH 26/29] Add a panic that prints information about the sender before calling splitSeg. Calling splitSeg with a negative value panics anyway, we need to know why. PiperOrigin-RevId: 455455562 --- pkg/tcpip/transport/tcp/snd.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/tcpip/transport/tcp/snd.go b/pkg/tcpip/transport/tcp/snd.go index e429a7209..99700c2ca 100644 --- a/pkg/tcpip/transport/tcp/snd.go +++ b/pkg/tcpip/transport/tcp/snd.go @@ -859,6 +859,13 @@ func (s *sender) maybeSendSegment(seg *segment, limit int, end seqnum.Value) (se } if seg.payloadSize() > available { + // A negative value causes splitSeg to panic anyways, so just panic + // earlier to get more information about the cause. + // TOOD(b/236090764): Remove this panic once the cause of negative values + // of "available" is understood. + if available < 0 { + panic(fmt.Sprintf("got available=%d, want available>=0. limit %d, s.MaxPayloadSize %d, seg.payloadSize() %d, gso.MaxSize %d, gso.MSS %d", available, limit, s.MaxPayloadSize, seg.payloadSize(), s.ep.gso.MaxSize, s.ep.gso.MSS)) + } s.splitSeg(seg, available) } From d5a04e338eef531108e9f6266e67e09f63ae4155 Mon Sep 17 00:00:00 2001 From: Rahat Mahmood Date: Thu, 16 Jun 2022 13:54:09 -0700 Subject: [PATCH 27/29] cgroupfs: Don't copy in with cgroups locks held. CopyIn acquires mm.mappingMu, which is ordered before cgroups locks. Reported-by: syzbot+97a5960840a8aeb25e6a@syzkaller.appspotmail.com PiperOrigin-RevId: 455460140 --- pkg/sentry/fsimpl/cgroupfs/pids.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/sentry/fsimpl/cgroupfs/pids.go b/pkg/sentry/fsimpl/cgroupfs/pids.go index ea17a45fd..29aa491f8 100644 --- a/pkg/sentry/fsimpl/cgroupfs/pids.go +++ b/pkg/sentry/fsimpl/cgroupfs/pids.go @@ -268,9 +268,6 @@ func (d *pidsMaxData) Generate(ctx context.Context, buf *bytes.Buffer) error { // Write implements vfs.WritableDynamicBytesSource.Write. func (d *pidsMaxData) Write(ctx context.Context, _ *vfs.FileDescription, src usermem.IOSequence, offset int64) (int64, error) { - d.c.mu.Lock() - defer d.c.mu.Unlock() - t := kernel.TaskFromContext(ctx) buf := t.CopyScratchBuffer(hostarch.PageSize) ncpy, err := src.CopyIn(ctx, buf) @@ -278,6 +275,8 @@ func (d *pidsMaxData) Write(ctx context.Context, _ *vfs.FileDescription, src use return 0, err } if strings.TrimSpace(string(buf)) == "max" { + d.c.mu.Lock() + defer d.c.mu.Unlock() d.c.max = pidLimitUnlimited return int64(ncpy), nil } @@ -290,6 +289,8 @@ func (d *pidsMaxData) Write(ctx context.Context, _ *vfs.FileDescription, src use return 0, linuxerr.EINVAL } + d.c.mu.Lock() + defer d.c.mu.Unlock() d.c.max = val return int64(n), nil } From 9f7d89e7d96635215f3589c98813e42711f1ce5b Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Thu, 16 Jun 2022 15:20:40 -0700 Subject: [PATCH 28/29] overlay: use lockdep mutexes PiperOrigin-RevId: 455478722 --- pkg/sentry/fsimpl/overlay/BUILD | 66 +++++++++++++++++++++++ pkg/sentry/fsimpl/overlay/directory.go | 3 +- pkg/sentry/fsimpl/overlay/filesystem.go | 12 ++--- pkg/sentry/fsimpl/overlay/overlay.go | 12 ++--- pkg/sentry/fsimpl/overlay/regular_file.go | 3 +- 5 files changed, 80 insertions(+), 16 deletions(-) diff --git a/pkg/sentry/fsimpl/overlay/BUILD b/pkg/sentry/fsimpl/overlay/BUILD index d1912dcd4..98b60a483 100644 --- a/pkg/sentry/fsimpl/overlay/BUILD +++ b/pkg/sentry/fsimpl/overlay/BUILD @@ -1,8 +1,65 @@ load("//tools:defs.bzl", "go_library") load("//tools/go_generics:defs.bzl", "go_template_instance") +load("//pkg/sync/locking:locking.bzl", "declare_mutex", "declare_rwmutex") licenses(["notice"]) +declare_mutex( + name = "dir_mutex", + out = "dir_mutex.go", + package = "overlay", + prefix = "dir", +) + +declare_mutex( + name = "dev_mutex", + out = "dev_mutex.go", + package = "overlay", + prefix = "dev", +) + +declare_mutex( + name = "dir_cache_mutex", + out = "dir_cache_mutex.go", + package = "overlay", + prefix = "dirInoCache", +) + +declare_mutex( + name = "reg_file_fd_mutex", + out = "req_file_fd_mutex.go", + package = "overlay", + prefix = "regularFileFD", +) + +declare_mutex( + name = "dir_fd_mutex", + out = "dir_fd_mutex.go", + package = "overlay", + prefix = "directoryFD", +) + +declare_rwmutex( + name = "rename_rwmutex", + out = "rename_rwmutex.go", + package = "overlay", + prefix = "rename", +) + +declare_rwmutex( + name = "data_rwmutex", + out = "data_rwmutex.go", + package = "overlay", + prefix = "data", +) + +declare_mutex( + name = "maps_mutex", + out = "maps_mutex.go", + package = "overlay", + prefix = "maps", +) + go_template_instance( name = "fstree", out = "fstree.go", @@ -18,11 +75,19 @@ go_library( name = "overlay", srcs = [ "copy_up.go", + "data_rwmutex.go", + "dev_mutex.go", + "dir_cache_mutex", + "dir_fd_mutex.go", + "dir_mutex.go", "directory.go", "filesystem.go", "fstree.go", + "maps_mutex.go", "overlay.go", "regular_file.go", + "rename_rwmutex.go", + "req_file_fd_mutex.go", "save_restore.go", ], visibility = ["//pkg/sentry:internal"], @@ -43,6 +108,7 @@ go_library( "//pkg/sentry/socket/unix/transport", "//pkg/sentry/vfs", "//pkg/sync", + "//pkg/sync/locking", "//pkg/usermem", "//pkg/waiter", ], diff --git a/pkg/sentry/fsimpl/overlay/directory.go b/pkg/sentry/fsimpl/overlay/directory.go index 8091f718a..2ff25e170 100644 --- a/pkg/sentry/fsimpl/overlay/directory.go +++ b/pkg/sentry/fsimpl/overlay/directory.go @@ -20,7 +20,6 @@ import ( "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/fspath" "gvisor.dev/gvisor/pkg/sentry/vfs" - "gvisor.dev/gvisor/pkg/sync" ) func (d *dentry) isDir() bool { @@ -104,7 +103,7 @@ type directoryFD struct { vfs.DirectoryFileDescriptionDefaultImpl vfs.DentryMetadataFileDescriptionImpl - mu sync.Mutex `state:"nosave"` + mu directoryFDMutex `state:"nosave"` off int64 dirents []vfs.Dirent } diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go index 80f58a00f..496b0ca1b 100644 --- a/pkg/sentry/fsimpl/overlay/filesystem.go +++ b/pkg/sentry/fsimpl/overlay/filesystem.go @@ -1117,8 +1117,8 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa if err := newParent.checkPermissions(creds, vfs.MayWrite|vfs.MayExec); err != nil { return err } - newParent.dirMu.Lock() - defer newParent.dirMu.Unlock() + newParent.dirMu.NestedLock() + defer newParent.dirMu.NestedUnlock() } if newParent.vfsd.IsDead() { return linuxerr.ENOENT @@ -1145,8 +1145,8 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa if genericIsAncestorDentry(replaced, renamed) { return linuxerr.ENOTEMPTY } - replaced.dirMu.Lock() - defer replaced.dirMu.Unlock() + replaced.dirMu.NestedLock() + defer replaced.dirMu.NestedUnlock() whiteouts, err = replaced.collectWhiteoutsForRmdirLocked(ctx) if err != nil { return err @@ -1350,8 +1350,8 @@ func (fs *filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error if err := parent.mayDelete(rp.Credentials(), child); err != nil { return err } - child.dirMu.Lock() - defer child.dirMu.Unlock() + child.dirMu.NestedLock() + defer child.dirMu.NestedUnlock() whiteouts, err := child.collectWhiteoutsForRmdirLocked(ctx) if err != nil { return err diff --git a/pkg/sentry/fsimpl/overlay/overlay.go b/pkg/sentry/fsimpl/overlay/overlay.go index 1ec755df8..462cf51f8 100644 --- a/pkg/sentry/fsimpl/overlay/overlay.go +++ b/pkg/sentry/fsimpl/overlay/overlay.go @@ -108,18 +108,18 @@ type filesystem struct { // distinguishable because they will diverge after copy-up; this isn't true // for non-directory files already on the upper layer.) lowerDevMinors is // protected by devMu. - devMu sync.Mutex `state:"nosave"` + devMu devMutex `state:"nosave"` lowerDevMinors map[layerDevNumber]uint32 // renameMu synchronizes renaming with non-renaming operations in order to // ensure consistent lock ordering between dentry.dirMu in different // dentries. - renameMu sync.RWMutex `state:"nosave"` + renameMu renameRWMutex `state:"nosave"` // dirInoCache caches overlay-private directory inode numbers by mapped // topmost device numbers and inode number. dirInoCache is protected by // dirInoCacheMu. - dirInoCacheMu sync.Mutex `state:"nosave"` + dirInoCacheMu dirInoCacheMutex `state:"nosave"` dirInoCache map[layerDevNoAndIno]uint64 // lastDirIno is the last inode number assigned to a directory. lastDirIno @@ -452,7 +452,7 @@ type dentry struct { // and dirents (if not nil) is a cache of dirents as returned by // directoryFDs representing this directory. children is protected by // dirMu. - dirMu sync.Mutex `state:"nosave"` + dirMu dirMutex `state:"nosave"` children map[string]*dentry dirents []vfs.Dirent @@ -499,9 +499,9 @@ type dentry struct { // // - isMappable is non-zero iff wrappedMappable is non-nil. isMappable is // accessed using atomic memory operations. - mapsMu sync.Mutex `state:"nosave"` + mapsMu mapsMutex `state:"nosave"` lowerMappings memmap.MappingSet - dataMu sync.RWMutex `state:"nosave"` + dataMu dataRWMutex `state:"nosave"` wrappedMappable memmap.Mappable isMappable atomicbitops.Uint32 diff --git a/pkg/sentry/fsimpl/overlay/regular_file.go b/pkg/sentry/fsimpl/overlay/regular_file.go index 54204f0b8..ce56b48e8 100644 --- a/pkg/sentry/fsimpl/overlay/regular_file.go +++ b/pkg/sentry/fsimpl/overlay/regular_file.go @@ -24,7 +24,6 @@ import ( "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/memmap" "gvisor.dev/gvisor/pkg/sentry/vfs" - "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/pkg/usermem" "gvisor.dev/gvisor/pkg/waiter" ) @@ -54,7 +53,7 @@ type regularFileFD struct { // fileDescription.dentry().upperVD. cachedFlags is the last known value of // cachedFD.StatusFlags(). copiedUp, cachedFD, and cachedFlags are // protected by mu. - mu sync.Mutex `state:"nosave"` + mu regularFileFDMutex `state:"nosave"` copiedUp bool cachedFD *vfs.FileDescription cachedFlags uint32 From c28cb583025ad75dab0c106d078687a7dce694b6 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Thu, 16 Jun 2022 16:17:22 -0700 Subject: [PATCH 29/29] Do not do searchable check while setting the working directory. This is akin to what docker does. Docker allows you to run a non-root container with a working directory that the non-root user does not have access to. This is also consistent with VFS1. Some applications rely on this behavior. Added a regression test for this. PiperOrigin-RevId: 455490226 --- pkg/sentry/kernel/kernel.go | 11 ++++++--- test/e2e/integration_test.go | 46 ++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index 7ec524d2a..59d04d93b 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/pkg/sentry/kernel/kernel.go @@ -971,10 +971,15 @@ func (k *Kernel) CreateProcess(args CreateProcessArgs) (*ThreadGroup, ThreadID, Path: fspath.Parse(args.WorkingDirectory), FollowFinalSymlink: true, } + // NOTE(b/236028361): Do not set CheckSearchable flag to true. + // Application is allowed to start with a working directory that it can + // not access/search. This is consistent with Docker and VFS1. Runc + // explicitly allows for this in 6ce2d63a5db6 ("libct/init_linux: retry + // chdir to fix EPERM"). As described in the commit, runc unintentionally + // allowed this behavior in a couple of releases and applications started + // relying on it. So they decided to allow it for backward compatibility. var err error - wd, err = k.VFS().GetDentryAt(ctx, args.Credentials, &pop, &vfs.GetDentryOptions{ - CheckSearchable: true, - }) + wd, err = k.VFS().GetDentryAt(ctx, args.Credentials, &pop, &vfs.GetDentryOptions{}) if err != nil { return nil, 0, fmt.Errorf("failed to find initial working directory %q: %v", args.WorkingDirectory, err) } diff --git a/test/e2e/integration_test.go b/test/e2e/integration_test.go index 42f5d699f..285620715 100644 --- a/test/e2e/integration_test.go +++ b/test/e2e/integration_test.go @@ -951,3 +951,49 @@ func TestTmpMountWithSize(t *testing.T) { t.Errorf("unexpected echo error:Expected: %v, Got: %v", wantErr, echoOutput) } } + +// NOTE(b/236028361): Regression test. Check we can handle a working directory +// without execute permissions. See comment in +// pkg/sentry/kernel/kernel.go:CreateProcess() for more context. +func TestNonSearchableWorkingDirectory(t *testing.T) { + dir, err := os.MkdirTemp(testutil.TmpDir(), "tmp-mount") + if err != nil { + t.Fatalf("MkdirTemp() failed: %v", err) + } + defer os.RemoveAll(dir) + + // The container will run as a non-root user. Make dir not searchable by + // others by removing execute bit for others. + if err := os.Chmod(dir, 0766); err != nil { + t.Fatalf("Chmod() failed: %v", err) + } + ctx := context.Background() + d := dockerutil.MakeContainer(ctx, t) + defer d.CleanUp(ctx) + + targetMount := "/foo" + opts := dockerutil.RunOpts{ + Image: "basic/alpine", + Mounts: []mount.Mount{ + { + Type: mount.TypeBind, + Source: dir, + Target: targetMount, + }, + }, + WorkDir: targetMount, + User: "nobody", + } + + echoPhrase := "All izz well" + got, err := d.Run(ctx, opts, "sh", "-c", "echo "+echoPhrase+" && (ls || true)") + if err != nil { + t.Fatalf("docker run failed: %v", err) + } + if !strings.Contains(got, echoPhrase) { + t.Errorf("echo output not found, want: %q, got: %q", echoPhrase, got) + } + if wantErrorMsg := "Permission denied"; !strings.Contains(got, wantErrorMsg) { + t.Errorf("ls error message not found, want: %q, got: %q", wantErrorMsg, got) + } +}