Repository: cilium/ebpf
Pull request #834 (2022-10-31 20:40:56)

prog: populate VerifierError.Truncate when LogLevel > 0
Truncate would only be set in case LogLevel was left to the default value of 0, and would only consider the return code of the retried prog load. Prog loads with a non-zero LogLevel would return ENOSPC all the way to the caller, but would not have the Truncated flag set on the VerifierError.

Since this code is and has been a pain in the rear, codify the behaviour in TestProgramVerifierLogTruncated, with a clear overview of when the Truncated flag is expected to be set, and explicitly validate all cases.

cc @arthurfabre 


Patch:
diff --git a/asm/instruction.go b/asm/instruction.go
index f17d88b..19c5b64 100644
--- a/asm/instruction.go
+++ b/asm/instruction.go
@@ -354,6 +354,13 @@ func (ins Instruction) Size() uint64 {
 	return uint64(InstructionSize * ins.OpCode.rawInstructions())
 }
 
+// WithMetadata sets the given Metadata on the Instruction. e.g. to copy
+// Metadata from another Instruction when replacing it.
+func (ins Instruction) WithMetadata(meta Metadata) Instruction {
+	ins.Metadata = meta
+	return ins
+}
+
 type symbolMeta struct{}
 
 // WithSymbol marks the Instruction as a Symbol, which other Instructions
diff --git a/prog.go b/prog.go
index cb496b0..06e91e9 100644
--- a/prog.go
+++ b/prog.go
@@ -306,9 +306,7 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions) (*Program, er
 		opts.LogSize = DefaultVerifierLogSize
 	}
 
-	// The caller provided a specific verifier log level. Immediately load
-	// the program with the given log level and buffer size, and skip retrying
-	// with a different level / size later.
+	// The caller requested a specific verifier log level. Set up the log buffer.
 	var logBuf []byte
 	if !opts.LogDisabled && opts.LogLevel != 0 {
 		logBuf = make([]byte, opts.LogSize)
@@ -322,17 +320,19 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions) (*Program, er
 		return &Program{unix.ByteSliceToString(logBuf), fd, spec.Name, "", spec.Type}, nil
 	}
 
-	// A verifier error occurred, but the caller did not specify a log level.
-	// Re-run with branch-level verifier logs enabled to obtain more info.
-	var truncated bool
+	// An error occurred loading the program, but the caller did not explicitly
+	// enable the verifier log. Re-run with branch-level verifier logs enabled to
+	// obtain more info. Preserve the original error to return it to the caller.
+	// An undersized log buffer will result in ENOSPC regardless of the underlying
+	// cause.
+	var err2 error
 	if !opts.LogDisabled && opts.LogLevel == 0 {
 		logBuf = make([]byte, opts.LogSize)
 		attr.LogLevel = LogLevelBranch
 		attr.LogSize = uint32(len(logBuf))
 		attr.LogBuf = sys.NewSlicePointer(logBuf)
 
-		_, ve := sys.ProgLoad(attr)
-		truncated = errors.Is(ve, unix.ENOSPC)
+		_, err2 = sys.ProgLoad(attr)
 	}
 
 	switch {
@@ -357,6 +357,7 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions) (*Program, er
 		}
 	}
 
+	truncated := errors.Is(err, unix.ENOSPC) || errors.Is(err2, unix.ENOSPC)
 	err = internal.ErrorWithLog(err, logBuf, truncated)
 	if btfDisabled {
 		return nil, fmt.Errorf("load program: %w (kernel without BTF support)", err)


Test patch:
diff --git a/asm/instruction_test.go b/asm/instruction_test.go
index ef3a37d..4eded25 100644
--- a/asm/instruction_test.go
+++ b/asm/instruction_test.go
@@ -177,6 +177,19 @@ func TestInstructionsRewriteMapPtr(t *testing.T) {
 	}
 }
 
+func TestInstructionWithMetadata(t *testing.T) {
+	ins := LoadImm(R0, 123, DWord).WithSymbol("abc")
+	ins2 := LoadImm(R0, 567, DWord).WithMetadata(ins.Metadata)
+
+	if want, got := "abc", ins2.Symbol(); want != got {
+		t.Fatalf("unexpected Symbol value on ins2: want: %s, got: %s", want, got)
+	}
+
+	if want, got := ins.Metadata, ins2.Metadata; want != got {
+		t.Fatal("expected ins and isn2 Metadata to match")
+	}
+}
+
 // You can use format flags to change the way an eBPF
 // program is stringified.
 func ExampleInstructions_Format() {
diff --git a/prog_test.go b/prog_test.go
index b98f0f6..dfc2eb1 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -422,6 +422,96 @@ func TestProgramVerifierOutput(t *testing.T) {
 	}
 }
 
+// Test all scenarios where the VerifierError.Truncated flag is expected to be
+// true, marked with an x. LL means ProgramOption.LogLevel.
+//
+// |      | Valid | Invalid |
+// |------|-------|---------|
+// | LL=0 |       |    x    |
+// | LL>0 |   x   |    x    |
+func TestProgramVerifierLogTruncated(t *testing.T) {
+	// Make the buffer intentionally small to coerce ENOSPC.
+	// 128 bytes is the smallest the kernel will accept.
+	logSize := 128
+
+	check := func(t *testing.T, err error) {
+		if err == nil {
+			t.Fatal("Expected an error")
+		}
+		var ve *internal.VerifierError
+		if !errors.As(err, &ve) {
+			t.Error("Error is not a VerifierError")
+		}
+		if !ve.Truncated {
+			t.Errorf("VerifierError is not truncated: %+v", ve)
+		}
+	}
+
+	// Generate a base program of sufficient size whose verifier log does not fit
+	// a 128-byte buffer. This should always result in ENOSPC, setting the
+	// VerifierError.Truncated flag.
+	base := func() (out asm.Instructions) {
+		for i := 0; i < 32; i++ {
+			out = append(out, asm.Mov.Reg(asm.R0, asm.R1))
+		}
+		return
+	}()
+
+	invalid := func() (out asm.Instructions) {
+		out = base
+		// Touch R10 (read-only frame pointer) to reliably force a verifier error.
+		out = append(out, asm.Mov.Reg(asm.R10, asm.R0))
+		out = append(out, asm.Return())
+		return
+	}()
+
+	valid := func() (out asm.Instructions) {
+		out = base
+		out = append(out, asm.Return())
+		return
+	}()
+
+	// Start out with testing against the invalid program.
+	spec := &ProgramSpec{
+		Type:         SocketFilter,
+		License:      "MIT",
+		Instructions: invalid,
+	}
+
+	// Set an undersized log buffer without explicitly requesting a verifier log
+	// for an invalid program.
+	_, err := NewProgramWithOptions(spec, ProgramOptions{LogSize: logSize})
+	check(t, err)
+
+	// Explicitly request a verifier log for an invalid program.
+	_, err = NewProgramWithOptions(spec, ProgramOptions{
+		LogSize:  logSize,
+		LogLevel: LogLevelInstruction,
+	})
+	check(t, err)
+
+	// Run tests against a valid program from here on out.
+	spec.Instructions = valid
+
+	// Don't request a verifier log, only set LogSize. Expect the valid program to
+	// be created without errors.
+	prog, err := NewProgramWithOptions(spec, ProgramOptions{
+		LogSize: logSize,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	prog.Close()
+
+	// Explicitly request verifier log for a valid program. If a log is requested
+	// and the buffer is too small, ENOSPC occurs even for valid programs.
+	_, err = NewProgramWithOptions(spec, ProgramOptions{
+		LogSize:  logSize,
+		LogLevel: LogLevelInstruction,
+	})
+	check(t, err)
+}
+
 func TestProgramWithUnsatisfiedMap(t *testing.T) {
 	coll, err := LoadCollectionSpec("testdata/loader-el.elf")
 	if err != nil {