From d4973670c3c9822163e2a24942fce34e7c8c8acf Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Tue, 24 Oct 2023 20:45:36 -0700 Subject: [PATCH] `bpf`: Remove `Input` interface and simply use bytes as the input. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This removes the interface indirection from BPF evaluation, which is a very hot path (runs for every application syscall for seccomp'd containers) and simplifies the code in general. ``` │ initial │ hey_look_no_interfaces │ │ sec/op │ sec/op vs base │ Interpreter 26.31n ± 0% 21.12n ± 0% -19.73% (p=0.000 n=21+20) ``` PiperOrigin-RevId: 576375947 --- pkg/bpf/input_bytes.go | 24 ++++++++++++++------ pkg/bpf/interpreter.go | 29 ------------------------- pkg/bpf/interpreter_test.go | 14 +++--------- pkg/seccomp/BUILD | 2 +- pkg/seccomp/seccomp.go | 15 +++++++++++++ pkg/seccomp/seccomp_test.go | 18 +++++---------- pkg/sentry/kernel/seccomp.go | 2 +- runsc/specutils/seccomp/BUILD | 3 +-- runsc/specutils/seccomp/seccomp_test.go | 16 +++++--------- test/secbench/BUILD | 2 -- test/secbench/secbench.go | 15 +++++-------- 11 files changed, 54 insertions(+), 86 deletions(-) diff --git a/pkg/bpf/input_bytes.go b/pkg/bpf/input_bytes.go index 86b216cfc..2ccbe368c 100644 --- a/pkg/bpf/input_bytes.go +++ b/pkg/bpf/input_bytes.go @@ -18,9 +18,11 @@ import ( "encoding/binary" ) -// InputBytes implements the Input interface by providing access to a byte -// slice. Unaligned loads are supported. -type InputBytes struct { +// Input represents a source of input data for a BPF program. (BPF +// documentation sometimes refers to the input data as the "packet" due to its +// origins as a packet processing DSL.) +// Unaligned loads are supported. +type Input struct { // Data is the data accessed through the Input interface. Data []byte @@ -29,7 +31,9 @@ type InputBytes struct { } // Load32 implements Input.Load32. -func (i InputBytes) Load32(off uint32) (uint32, bool) { +// +//go:nosplit +func (i *Input) Load32(off uint32) (uint32, bool) { if uint64(off)+4 > uint64(len(i.Data)) { return 0, false } @@ -37,7 +41,9 @@ func (i InputBytes) Load32(off uint32) (uint32, bool) { } // Load16 implements Input.Load16. -func (i InputBytes) Load16(off uint32) (uint16, bool) { +// +//go:nosplit +func (i *Input) Load16(off uint32) (uint16, bool) { if uint64(off)+2 > uint64(len(i.Data)) { return 0, false } @@ -45,7 +51,9 @@ func (i InputBytes) Load16(off uint32) (uint16, bool) { } // Load8 implements Input.Load8. -func (i InputBytes) Load8(off uint32) (uint8, bool) { +// +//go:nosplit +func (i *Input) Load8(off uint32) (uint8, bool) { if uint64(off)+1 > uint64(len(i.Data)) { return 0, false } @@ -53,6 +61,8 @@ func (i InputBytes) Load8(off uint32) (uint8, bool) { } // Length implements Input.Length. -func (i InputBytes) Length() uint32 { +// +//go:nosplit +func (i *Input) Length() uint32 { return uint32(len(i.Data)) } diff --git a/pkg/bpf/interpreter.go b/pkg/bpf/interpreter.go index cde771154..ec7ccb257 100644 --- a/pkg/bpf/interpreter.go +++ b/pkg/bpf/interpreter.go @@ -217,35 +217,6 @@ func Compile(insns []Instruction) (Program, error) { return Program{Optimize(insns)}, nil } -// Input represents a source of input data for a BPF program. (BPF -// documentation sometimes refers to the input data as the "packet" due to its -// origins as a packet processing DSL.) -// -// For all of Input's Load methods: -// -// - The second (bool) return value is true if the load succeeded and false -// otherwise. -// -// - Inputs should not assume that the loaded range falls within the input -// data's length. Inputs should return false if the load falls outside of the -// input data. -// -// - Inputs should not assume that the offset is correctly aligned. Inputs may -// choose to service or reject loads to unaligned addresses. -type Input interface { - // Load32 reads 32 bits from the input starting at the given byte offset. - Load32(off uint32) (uint32, bool) - - // Load16 reads 16 bits from the input starting at the given byte offset. - Load16(off uint32) (uint16, bool) - - // Load8 reads 8 bits from the input starting at the given byte offset. - Load8(off uint32) (uint8, bool) - - // Length returns the length of the input in bytes. - Length() uint32 -} - // machine represents the state of a BPF virtual machine. type machine struct { A uint32 diff --git a/pkg/bpf/interpreter_test.go b/pkg/bpf/interpreter_test.go index b17685669..c15b2000c 100644 --- a/pkg/bpf/interpreter_test.go +++ b/pkg/bpf/interpreter_test.go @@ -150,7 +150,7 @@ func TestExecErrors(t *testing.T) { t.Errorf("%s: unexpected compilation error: %v", test.desc, err) continue } - ret, err := Exec(p, InputBytes{nil, binary.BigEndian}) + ret, err := Exec(p, Input{nil, binary.BigEndian}) if err != test.expectedErr { t.Errorf("%s: expected execution error %q, got (%d, %v)", test.desc, test.expectedErr, ret, err) } @@ -724,7 +724,7 @@ func TestValidInstructions(t *testing.T) { t.Errorf("%s: unexpected compilation error: %v", test.desc, err) continue } - ret, err := Exec(p, InputBytes{test.input, binary.BigEndian}) + ret, err := Exec(p, Input{test.input, binary.BigEndian}) if err != nil { t.Errorf("%s: expected return value of %d, got execution error: %v", test.desc, test.expectedRet, err) continue @@ -798,17 +798,9 @@ func TestSimpleFilter(t *testing.T) { } } -// seccompData is equivalent to struct seccomp_data. -type seccompData struct { - nr uint32 - arch uint32 - instructionPointer uint64 - args [6]uint64 -} - // asInput converts a seccompData to a bpf.Input. func dataAsInput(data *linux.SeccompData) Input { - return InputBytes{marshal.Marshal(data), hostarch.ByteOrder} + return Input{marshal.Marshal(data), hostarch.ByteOrder} } // BenchmarkInterpreter benchmarks the execution of the sample filter diff --git a/pkg/seccomp/BUILD b/pkg/seccomp/BUILD index 014d1a6de..44630fd30 100644 --- a/pkg/seccomp/BUILD +++ b/pkg/seccomp/BUILD @@ -18,6 +18,7 @@ go_library( deps = [ "//pkg/abi/linux", "//pkg/bpf", + "//pkg/hostarch", "//pkg/log", "@org_golang_x_sys//unix:go_default_library", ], @@ -36,6 +37,5 @@ go_test( deps = [ "//pkg/abi/linux", "//pkg/bpf", - "//pkg/hostarch", ], ) diff --git a/pkg/seccomp/seccomp.go b/pkg/seccomp/seccomp.go index e73930b3c..771202bda 100644 --- a/pkg/seccomp/seccomp.go +++ b/pkg/seccomp/seccomp.go @@ -23,6 +23,7 @@ import ( "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/bpf" + "gvisor.dev/gvisor/pkg/hostarch" "gvisor.dev/gvisor/pkg/log" ) @@ -491,3 +492,17 @@ func (n *node) traverse(fn traverseFunc, rules []RuleSet, program *syscallProgra } return n.right.traverse(fn, rules, program) } + +// DataAsBPFInput converts a linux.SeccompData to a bpf.Input. +// It uses `buf` as scratch buffer; this buffer must be wide enough +// to accommodate a mashaled version of `d`. +func DataAsBPFInput(d *linux.SeccompData, buf []byte) bpf.Input { + if len(buf) < d.SizeBytes() { + panic(fmt.Sprintf("buffer must be at least %d bytes long", d.SizeBytes())) + } + d.MarshalUnsafe(buf) + return bpf.Input{ + Data: buf, + Order: hostarch.ByteOrder, + } +} diff --git a/pkg/seccomp/seccomp_test.go b/pkg/seccomp/seccomp_test.go index 2675a2074..59c689ea9 100644 --- a/pkg/seccomp/seccomp_test.go +++ b/pkg/seccomp/seccomp_test.go @@ -31,7 +31,6 @@ import ( "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/bpf" - "gvisor.dev/gvisor/pkg/hostarch" ) //go:embed victim @@ -56,17 +55,9 @@ func newVictim() (string, error) { return path, nil } -// dataAsInput converts a linux.SeccompData to a bpf.Input. -func dataAsInput(d *linux.SeccompData) bpf.Input { - buf := make([]byte, d.SizeBytes()) - d.MarshalUnsafe(buf) - return bpf.InputBytes{ - Data: buf, - Order: hostarch.ByteOrder, - } -} - func TestBasic(t *testing.T) { + buf := make([]byte, (&linux.SeccompData{}).SizeBytes()) + type spec struct { // desc is the test's description. desc string @@ -886,7 +877,7 @@ func TestBasic(t *testing.T) { t.Fatalf("bpf.Compile() got error: %v", err) } for _, spec := range test.specs { - got, err := bpf.Exec(p, dataAsInput(&spec.data)) + got, err := bpf.Exec(p, DataAsBPFInput(&spec.data, buf)) if err != nil { t.Fatalf("%s: bpf.Exec() got error: %v", spec.desc, err) } @@ -926,9 +917,10 @@ func TestRandom(t *testing.T) { if err != nil { t.Fatalf("bpf.Compile() got error: %v", err) } + buf := make([]byte, (&linux.SeccompData{}).SizeBytes()) for i := uint32(0); i < 200; i++ { data := linux.SeccompData{Nr: int32(i), Arch: LINUX_AUDIT_ARCH} - got, err := bpf.Exec(p, dataAsInput(&data)) + got, err := bpf.Exec(p, DataAsBPFInput(&data, buf)) if err != nil { t.Errorf("bpf.Exec() got error: %v, for syscall %d", err, i) continue diff --git a/pkg/sentry/kernel/seccomp.go b/pkg/sentry/kernel/seccomp.go index 0d66648c3..bd9b1dd13 100644 --- a/pkg/sentry/kernel/seccomp.go +++ b/pkg/sentry/kernel/seccomp.go @@ -32,7 +32,7 @@ const maxSyscallFilterInstructions = 1 << 15 func dataAsBPFInput(t *Task, d *linux.SeccompData) bpf.Input { buf := t.CopyScratchBuffer(d.SizeBytes()) d.MarshalUnsafe(buf) - return bpf.InputBytes{ + return bpf.Input{ Data: buf, // Go-marshal always uses the native byte order. Order: hostarch.ByteOrder, diff --git a/runsc/specutils/seccomp/BUILD b/runsc/specutils/seccomp/BUILD index f3c6cfb2c..e4eed09da 100644 --- a/runsc/specutils/seccomp/BUILD +++ b/runsc/specutils/seccomp/BUILD @@ -33,8 +33,7 @@ go_test( deps = [ "//pkg/abi/linux", "//pkg/bpf", - "//pkg/hostarch", - "//pkg/marshal", + "//pkg/seccomp", "@com_github_opencontainers_runtime_spec//specs-go:go_default_library", "@org_golang_x_sys//unix:go_default_library", ], diff --git a/runsc/specutils/seccomp/seccomp_test.go b/runsc/specutils/seccomp/seccomp_test.go index 20796bf14..d06e025e3 100644 --- a/runsc/specutils/seccomp/seccomp_test.go +++ b/runsc/specutils/seccomp/seccomp_test.go @@ -22,15 +22,9 @@ import ( "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/bpf" - "gvisor.dev/gvisor/pkg/hostarch" - "gvisor.dev/gvisor/pkg/marshal" + "gvisor.dev/gvisor/pkg/seccomp" ) -// asInput converts a linux.SeccompData to a bpf.Input. -func asInput(d *linux.SeccompData) bpf.Input { - return bpf.InputBytes{marshal.Marshal(d), hostarch.ByteOrder} -} - // testInput creates an Input struct with given seccomp input values. func testInput(arch uint32, syscallName string, args *[6]uint64) bpf.Input { syscallNo, err := lookupSyscallNo(arch, syscallName) @@ -49,8 +43,7 @@ func testInput(arch uint32, syscallName string, args *[6]uint64) bpf.Input { Arch: arch, Args: *args, } - - return asInput(&data) + return seccomp.DataAsBPFInput(&data, make([]byte, data.SizeBytes())) } // testCase holds a seccomp test case. @@ -95,7 +88,10 @@ var ( }, // Syscall matches but the arch is AUDIT_ARCH_X86 so the return // value is the bad arch action. - input: asInput(&linux.SeccompData{Nr: 183, Arch: 0x40000003}), // + input: seccomp.DataAsBPFInput( + &linux.SeccompData{Nr: 183, Arch: 0x40000003}, + make([]byte, (&linux.SeccompData{}).SizeBytes()), + ), expected: uint32(killThreadAction), }, { diff --git a/test/secbench/BUILD b/test/secbench/BUILD index 44dbdd6fa..4a66a1080 100644 --- a/test/secbench/BUILD +++ b/test/secbench/BUILD @@ -18,8 +18,6 @@ go_library( deps = [ "//pkg/abi/linux", "//pkg/bpf", - "//pkg/hostarch", - "//pkg/marshal", "//pkg/seccomp", "//pkg/test/testutil", "//test/secbench/secbenchdef", diff --git a/test/secbench/secbench.go b/test/secbench/secbench.go index a41230d77..3cbe9f31f 100644 --- a/test/secbench/secbench.go +++ b/test/secbench/secbench.go @@ -27,8 +27,6 @@ import ( "golang.org/x/sys/unix" "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/bpf" - "gvisor.dev/gvisor/pkg/hostarch" - "gvisor.dev/gvisor/pkg/marshal" "gvisor.dev/gvisor/pkg/seccomp" "gvisor.dev/gvisor/pkg/test/testutil" "gvisor.dev/gvisor/test/secbench/secbenchdef" @@ -107,8 +105,8 @@ func runRequest(runReq secbenchdef.BenchRunRequest) (secbenchdef.BenchRunRespons return runResp, nil } -func evalSyscall(program bpf.Program, arch uint32, sc secbenchdef.Syscall) (uint32, error) { - scData := &linux.SeccompData{ +func evalSyscall(program bpf.Program, arch uint32, sc secbenchdef.Syscall, buf []byte) (uint32, error) { + return bpf.Exec(program, seccomp.DataAsBPFInput(&linux.SeccompData{ Nr: int32(sc.Sysno), Arch: arch, Args: [6]uint64{ @@ -119,11 +117,7 @@ func evalSyscall(program bpf.Program, arch uint32, sc secbenchdef.Syscall) (uint uint64(sc.Args[4]), uint64(sc.Args[5]), }, - } - return bpf.Exec(program, bpf.InputBytes{ - Data: marshal.Marshal(scData), - Order: hostarch.ByteOrder, - }) + }, buf)) } // Number of times we scale b.N by. @@ -139,6 +133,7 @@ func RunBench(b *testing.B, bn secbenchdef.Bench) { randSeed := time.Now().UnixNano() b.Logf("Running with %d iterations (scaled by %dx), random seed %d...", b.N, iterationScaleFactor, randSeed) iterations := uint64(b.N * iterationScaleFactor) + buf := make([]byte, (&linux.SeccompData{}).SizeBytes()) // Check if there are any sequences where the syscall will be approved. // If there is any, we will need to run the runner twice: Once with the @@ -160,7 +155,7 @@ func RunBench(b *testing.B, bn secbenchdef.Bench) { for i, seq := range bn.Profile.Sequences { result := int64(-1) for _, sc := range seq.Syscalls { - scResult, err := evalSyscall(program, bn.Profile.Arch, sc) + scResult, err := evalSyscall(program, bn.Profile.Arch, sc, buf) if err != nil { b.Fatalf("cannot eval program with syscall %v: %v", sc, err) }