Repository: cilium/ebpf
Pull request #657 (2022-05-04 08:02:07)

Probe kernel bpf2bpf support
```
The adjustments performed by fixupAndValidate() don't rely on CO-RE being
applied first. Execute it earlier in the chain to potentially avoid loading
BTF and executing CO-RE when we could bail out instead.

Also avoids having to touch test cases due to changes in a subsequent commit.
```

```
When trying to load a program containing bpf2bpf calls on a kernel that
doesn't support it, we bubble up the verifier error to the caller:

  program xdp_prog: load program: invalid argument: unreachable insn 28

This is clear, but doesn't allow skipping tests when loading e.g. loader.c
on older kernels due to it containing bpf2bpf calls.

Implement a feature probe for bpf2bpf, return an unsupported error when
trying to load a program with multiple subprogs on an older kernel.

Update the test suite to skip unsupported features instead of hardcoding
a skip on 4.16 and earlier.
```

- Provide clear(er) and earlier error feedback about the kernel's bpf2bpf support
- Allow test skips when loading a bpf2bpf prog on a kernel that doesn't support it
- Remove hardcoded feature gates for bpf2bpf on kernel 4.16

lmb: Thanks, checking after loading is nicer!
ti-mo: @lmb I've found a bug introduced in 9b5a039e00c6df7f25e76eac13fede067b0a48a1 and made some changes to `ErrorWithLog`. PTAL :pray: 

I've also added `hasReferences()` and looked at `err` instead of `logErr`.

Patch:
diff --git a/internal/errors.go b/internal/errors.go
index 411043a..5ff158e 100644
--- a/internal/errors.go
+++ b/internal/errors.go
@@ -14,7 +14,13 @@ import (
 // logErr should be the error returned by the syscall that generated
 // the log. It is used to check for truncation of the output.
 func ErrorWithLog(err error, log []byte, logErr error) error {
-	logStr := unix.ByteSliceToString(bytes.Trim(log, "\t\r\n "))
+	// Convert verifier log C string by truncating it on the first 0 byte
+	// and trimming trailing whitespace before interpreting as a Go string.
+	if i := bytes.IndexByte(log, 0); i != -1 {
+		log = log[:i]
+	}
+	logStr := string(bytes.Trim(log, "\t\r\n "))
+
 	if errors.Is(logErr, unix.ENOSPC) {
 		logStr += " (truncated...)"
 	}
diff --git a/linker.go b/linker.go
index 60cb7a6..6bc00f9 100644
--- a/linker.go
+++ b/linker.go
@@ -110,6 +110,17 @@ func findReferences(progs map[string]*ProgramSpec) error {
 	return nil
 }
 
+// hasReferences returns true if insns contains one or more bpf2bpf
+// function references.
+func hasReferences(insns asm.Instructions) bool {
+	for _, i := range insns {
+		if i.IsFunctionReference() {
+			return true
+		}
+	}
+	return false
+}
+
 // applyRelocations collects and applies any CO-RE relocations in insns.
 //
 // Passing a nil target will relocate against the running kernel. insns are
diff --git a/prog.go b/prog.go
index b72c0f5..6e024e4 100644
--- a/prog.go
+++ b/prog.go
@@ -361,6 +361,12 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions, handles *hand
 		}
 	}
 
+	if (errors.Is(err, unix.EINVAL) || errors.Is(err, unix.EPERM)) && hasReferences(spec.Instructions) {
+		if err := haveBPFToBPFCalls(); err != nil {
+			return nil, fmt.Errorf("load program: %w", internal.ErrorWithLog(err, logBuf, logErr))
+		}
+	}
+
 	if errors.Is(logErr, unix.EPERM) && len(logBuf) > 0 && logBuf[0] == 0 {
 		// EPERM due to RLIMIT_MEMLOCK happens before the verifier, so we can
 		// check that the log is empty to reduce false positives.
diff --git a/syscalls.go b/syscalls.go
index ccbbe09..b90ff7b 100644
--- a/syscalls.go
+++ b/syscalls.go
@@ -38,6 +38,21 @@ func invalidBPFObjNameChar(char rune) bool {
 	}
 }
 
+func progLoad(insns asm.Instructions, typ ProgramType, license string) (*sys.FD, error) {
+	buf := bytes.NewBuffer(make([]byte, 0, insns.Size()))
+	if err := insns.Marshal(buf, internal.NativeEndian); err != nil {
+		return nil, err
+	}
+	bytecode := buf.Bytes()
+
+	return sys.ProgLoad(&sys.ProgLoadAttr{
+		ProgType: sys.ProgType(typ),
+		License:  sys.NewStringPointer(license),
+		Insns:    sys.NewSlicePointer(bytecode),
+		InsnCnt:  uint32(len(bytecode) / asm.InstructionSize),
+	})
+}
+
 var haveNestedMaps = internal.FeatureTest("nested maps", "4.12", func() error {
 	_, err := sys.MapCreate(&sys.MapCreateAttr{
 		MapType:    sys.MapType(ArrayOfMaps),
@@ -226,21 +241,30 @@ var haveProbeReadKernel = internal.FeatureTest("bpf_probe_read_kernel", "5.5", f
 		asm.FnProbeReadKernel.Call(),
 		asm.Return(),
 	}
-	buf := bytes.NewBuffer(make([]byte, 0, insns.Size()))
-	if err := insns.Marshal(buf, internal.NativeEndian); err != nil {
-		return err
-	}
-	bytecode := buf.Bytes()
 
-	fd, err := sys.ProgLoad(&sys.ProgLoadAttr{
-		ProgType: sys.ProgType(Kprobe),
-		License:  sys.NewStringPointer("GPL"),
-		Insns:    sys.NewSlicePointer(bytecode),
-		InsnCnt:  uint32(len(bytecode) / asm.InstructionSize),
-	})
+	fd, err := progLoad(insns, Kprobe, "GPL")
 	if err != nil {
 		return internal.ErrNotSupported
 	}
 	_ = fd.Close()
 	return nil
 })
+
+var haveBPFToBPFCalls = internal.FeatureTest("bpf2bpf calls", "4.16", func() error {
+	insns := asm.Instructions{
+		asm.Call.Label("prog2").WithSymbol("prog1"),
+		asm.Return(),
+		asm.Mov.Imm(asm.R0, 0).WithSymbol("prog2"),
+		asm.Return(),
+	}
+
+	fd, err := progLoad(insns, SocketFilter, "MIT")
+	if errors.Is(err, unix.EINVAL) {
+		return internal.ErrNotSupported
+	}
+	if err != nil {
+		return err
+	}
+	_ = fd.Close()
+	return nil
+})


Test patch:
diff --git a/internal/btf/core_reloc_test.go b/internal/btf/core_reloc_test.go
index b254efc..c150c39 100644
--- a/internal/btf/core_reloc_test.go
+++ b/internal/btf/core_reloc_test.go
@@ -90,8 +90,6 @@ func TestCORERelocationRead(t *testing.T) {
 		}
 		defer tgt.Close()
 
-		testutils.SkipOnOldKernel(t, "4.16", "bpf2bpf calls")
-
 		for _, progSpec := range spec.Programs {
 			t.Run(progSpec.Name, func(t *testing.T) {
 				if _, err := tgt.Seek(0, io.SeekStart); err != nil {
@@ -101,6 +99,7 @@ func TestCORERelocationRead(t *testing.T) {
 				prog, err := ebpf.NewProgramWithOptions(progSpec, ebpf.ProgramOptions{
 					TargetBTF: tgt,
 				})
+				testutils.SkipIfNotSupported(t, err)
 				if err != nil {
 					t.Fatal("Load program:", err)
 				}
diff --git a/internal/errors_test.go b/internal/errors_test.go
new file mode 100644
index 0000000..e853b34
--- /dev/null
+++ b/internal/errors_test.go
@@ -0,0 +1,30 @@
+package internal
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/cilium/ebpf/internal/unix"
+)
+
+func TestErrorWithLog(t *testing.T) {
+	b := []byte("unreachable insn 28")
+	b = append(b,
+		0xa,  // \n
+		0xd,  // \r
+		0x9,  // \t
+		0x20, // space
+		0, 0, // trailing NUL bytes
+	)
+
+	err := ErrorWithLog(errors.New("test"), b, unix.ENOSPC)
+
+	want := "test: unreachable insn 28 (truncated...)"
+	got := err.Error()
+
+	t.Log(got)
+
+	if want != got {
+		t.Fatalf("\nwant: %s\ngot: %s", want, got)
+	}
+}
diff --git a/linker_test.go b/linker_test.go
index 283a842..51bdf8d 100644
--- a/linker_test.go
+++ b/linker_test.go
@@ -42,9 +42,8 @@ func TestFindReferences(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	testutils.SkipOnOldKernel(t, "4.16", "bpf2bpf calls")
-
 	prog, err := NewProgram(progs["entrypoint"])
+	testutils.SkipIfNotSupported(t, err)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -66,8 +65,6 @@ func TestForwardFunctionDeclaration(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		testutils.SkipOnOldKernel(t, "4.16", "bpf2bpf calls")
-
 		if coll.ByteOrder != internal.NativeEndian {
 			return
 		}
@@ -91,6 +88,7 @@ func TestForwardFunctionDeclaration(t *testing.T) {
 		spec.BTF = nil
 
 		prog, err := NewProgram(spec)
+		testutils.SkipIfNotSupported(t, err)
 		if err != nil {
 			t.Fatal(err)
 		}
diff --git a/prog_test.go b/prog_test.go
index 52e2400..147c38a 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -374,6 +374,7 @@ func TestProgramWithUnsatisfiedMap(t *testing.T) {
 	progSpec.ByteOrder = nil
 
 	_, err = NewProgram(progSpec)
+	testutils.SkipIfNotSupported(t, err)
 	if !errors.Is(err, asm.ErrUnsatisfiedMapReference) {
 		t.Fatal("Expected an error wrapping asm.ErrUnsatisfiedMapReference, got", err)
 	}
diff --git a/syscalls_test.go b/syscalls_test.go
index 7ad03c0..e9735b7 100644
--- a/syscalls_test.go
+++ b/syscalls_test.go
@@ -54,3 +54,7 @@ func TestHaveInnerMaps(t *testing.T) {
 func TestHaveProbeReadKernel(t *testing.T) {
 	testutils.CheckFeatureTest(t, haveProbeReadKernel)
 }
+
+func TestHaveBPFToBPFCalls(t *testing.T) {
+	testutils.CheckFeatureTest(t, haveBPFToBPFCalls)
+}