Repository: cilium/ebpf
Pull request #571 (2022-02-16 15:05:03)

features: centralize error semantics, remove maxMiscType, etc.
Best reviewed per commit.

These are some changes I'd liked to see in #541, but did not have the bandwidth to review at the time. apologies.

The function documentation was getting a bit repetitive to my liking, I think it fits better in the package doc, with any exceptions (currently none) documented on the function in question.

I don't feel very strongly about `ErrUnsatisfiedProgramReference`, but it's potentially confusing during troubleshooting, so IMO it's better to be specific.


Patch:
diff --git a/asm/instruction.go b/asm/instruction.go
index 8c6d502..1d37a73 100644
--- a/asm/instruction.go
+++ b/asm/instruction.go
@@ -20,7 +20,7 @@ const InstructionSize = 8
 type RawInstructionOffset uint64
 
 var ErrUnsatisfiedMapReference = errors.New("unsatisfied map reference")
-var ErrUnsatisfiedReference = errors.New("unsatisfied reference")
+var ErrUnsatisfiedProgramReference = errors.New("unsatisfied program reference")
 
 // Bytes returns the offset of an instruction in bytes.
 func (rio RawInstructionOffset) Bytes() uint64 {
@@ -180,17 +180,6 @@ func (ins *Instruction) RewriteMapOffset(offset uint32) error {
 	return nil
 }
 
-// RewriteJumpOffset sets the offset for a jump operation.
-//
-// Returns an error if the instruction is not a jump operation.
-func (ins *Instruction) RewriteJumpOffset(offset int16) error {
-	if ins.OpCode.JumpOp() == InvalidJumpOp {
-		return errors.New("not a jump operation")
-	}
-	ins.Offset = offset
-	return nil
-}
-
 func (ins *Instruction) mapOffset() uint32 {
 	return uint32(uint64(ins.Constant) >> 32)
 }
@@ -571,31 +560,26 @@ func (insns Instructions) FixupReferences() error {
 			continue
 		}
 
-		symOffset, ok := symbolOffsets[ins.Reference]
 		switch {
 		case ins.IsFunctionReference() && ins.Constant == -1:
+			symOffset, ok := symbolOffsets[ins.Reference]
 			if !ok {
-				break
+				return fmt.Errorf("%s at insn %d: symbol %q: %w", ins.OpCode, i, ins.Reference, ErrUnsatisfiedProgramReference)
 			}
 
 			ins.Constant = int64(symOffset - offset - 1)
-			continue
 
 		case ins.OpCode.Class().IsJump() && ins.Offset == -1:
+			symOffset, ok := symbolOffsets[ins.Reference]
 			if !ok {
-				break
+				return fmt.Errorf("%s at insn %d: symbol %q: %w", ins.OpCode, i, ins.Reference, ErrUnsatisfiedProgramReference)
 			}
 
 			ins.Offset = int16(symOffset - offset - 1)
-			continue
 
 		case ins.IsLoadFromMap() && ins.MapPtr() == -1:
 			return fmt.Errorf("map %s: %w", ins.Reference, ErrUnsatisfiedMapReference)
-		default:
-			// no fixup needed
-			continue
 		}
-		return fmt.Errorf("%s at insn %d: symbol %q: %w", ins.OpCode, i, ins.Reference, ErrUnsatisfiedReference)
 	}
 	return nil
 }
diff --git a/asm/opcode.go b/asm/opcode.go
index f6d8e06..b11917e 100644
--- a/asm/opcode.go
+++ b/asm/opcode.go
@@ -18,22 +18,23 @@ type Class uint8
 const classMask OpCode = 0x07
 
 const (
-	// LdClass load memory
+	// LdClass loads immediate values into registers.
+	// Also used for non-standard load operations from cBPF.
 	LdClass Class = 0x00
-	// LdXClass load memory from constant
+	// LdXClass loads memory into registers.
 	LdXClass Class = 0x01
-	// StClass load register from memory
+	// StClass stores immediate values to memory.
 	StClass Class = 0x02
-	// StXClass load register from constant
+	// StXClass stores registers to memory.
 	StXClass Class = 0x03
-	// ALUClass arithmetic operators
+	// ALUClass describes arithmetic operators.
 	ALUClass Class = 0x04
-	// JumpClass jump operators
+	// JumpClass describes jump operators.
 	JumpClass Class = 0x05
-	// Jump32Class jump operators with 32 bit comparaisons
-	// Requires kernel 5.1
+	// Jump32Class describes jump operators with 32-bit comparisons.
+	// Requires kernel 5.1.
 	Jump32Class Class = 0x06
-	// ALU64Class arithmetic in 64 bit mode
+	// ALU64Class describes arithmetic operators in 64-bit mode.
 	ALU64Class Class = 0x07
 )
 
diff --git a/features/doc.go b/features/doc.go
index f445101..723c7ae 100644
--- a/features/doc.go
+++ b/features/doc.go
@@ -1,2 +1,19 @@
 // Package features allows probing for BPF features available to the calling process.
+//
+// In general, the error return values from feature probes in this package
+// all have the following semantics unless otherwise specified:
+//
+//   err == nil: The feature is available.
+//   errors.Is(err, ebpf.ErrNotSupported): The feature is not available.
+//   err != nil: Any errors encountered during probe execution, wrapped.
+//
+// Note that the latter case may include false negatives, and that resource
+// creation may succeed despite an error being returned. For example, some
+// map and program types cannot reliably be probed and will return an
+// inconclusive error.
+//
+// As a rule, only `nil` and `ebpf.ErrNotSupported` are conclusive.
+//
+// Probe results are cached by the library and persist throughout any changes
+// to the process' environment, like capability changes.
 package features
diff --git a/features/map.go b/features/map.go
index 29a8742..eab39bf 100644
--- a/features/map.go
+++ b/features/map.go
@@ -96,17 +96,8 @@ func createMapTypeAttr(mt ebpf.MapType) *sys.MapCreateAttr {
 }
 
 // HaveMapType probes the running kernel for the availability of the specified map type.
-// Return values have the following semantics:
 //
-//   err == nil: The feature is available.
-//   errors.Is(err, ebpf.ErrNotSupported): The feature is not available.
-//   err != nil: Any errors encountered during probe execution, wrapped.
-//
-// Note that the latter case may include false negatives, and that map creation may succeed
-// despite an error being returned. Some map types cannot reliably be probed and will also
-// return error. Only `nil` and `ebpf.ErrNotSupported` are conclusive.
-//
-// Probe results are cached and persist throughout any process capability changes.
+// See the package documentation for the meaning of the error return value.
 func HaveMapType(mt ebpf.MapType) error {
 	if err := validateMaptype(mt); err != nil {
 		return err
diff --git a/features/misc.go b/features/misc.go
index 0934652..c9eff09 100644
--- a/features/misc.go
+++ b/features/misc.go
@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
-	"os"
 	"sync"
 
 	"github.com/cilium/ebpf"
@@ -15,7 +14,7 @@ import (
 )
 
 func init() {
-	miscs.miscTypes = make(map[miscType]error, maxMiscType)
+	miscs.miscTypes = make(map[miscType]error)
 }
 
 var (
@@ -29,11 +28,6 @@ type miscCache struct {
 
 type miscType uint32
 
-// Max returns the latest supported MiscType.
-func (_ miscType) max() miscType {
-	return maxMiscType - 1
-}
-
 const (
 	// largeInsn support introduced in
 	// commit c04c0d2b968ac45d6ef020316808ef6c82325a82
@@ -47,8 +41,6 @@ const (
 	// v3ISA support introduced in
 	// commit 092ed0968bb648cd18e8a0430cd0a8a71727315c
 	v3ISA
-	// maxMiscType - Bound enum of FeatureTypes, has to be last in enum.
-	maxMiscType
 )
 
 const (
@@ -57,76 +49,36 @@ const (
 
 // HaveLargeInstructions probes the running kernel if more than 4096 instructions
 // per program are supported.
-// Return values have the following semantics:
-//
-//   err == nil: The feature is available.
-//   errors.Is(err, ebpf.ErrNotSupported): The feature is not available.
-//   err != nil: Any errors encountered during probe execution, wrapped.
 //
-// Note that the latter case may include false negatives, and that program creation may
-// succeed despite an error being returned. Some program types cannot reliably be probed and
-// will also return error. Only `nil` and `ebpf.ErrNotSupported` are conclusive.
-//
-// Probe results are cached and persist throughout any process capability changes.
+// See the package documentation for the meaning of the error return value.
 func HaveLargeInstructions() error {
 	return probeMisc(largeInsn)
 }
 
 // HaveBoundedLoops probes the running kernel if bounded loops are supported.
-// Return values have the following semantics:
-//
-//   err == nil: The feature is available.
-//   errors.Is(err, ebpf.ErrNotSupported): The feature is not available.
-//   err != nil: Any errors encountered during probe execution, wrapped.
-//
-// Note that the latter case may include false negatives, and that program creation may
-// succeed despite an error being returned. Some program types cannot reliably be probed and
-// will also return error. Only `nil` and `ebpf.ErrNotSupported` are conclusive.
 //
-// Probe results are cached and persist throughout any process capability changes.
+// See the package documentation for the meaning of the error return value.
 func HaveBoundedLoops() error {
 	return probeMisc(boundedLoops)
 }
 
 // HaveV2ISA probes the running kernel if instructions of the v2 ISA are supported.
-// Return values have the following semantics:
 //
-//   err == nil: The feature is available.
-//   errors.Is(err, ebpf.ErrNotSupported): The feature is not available.
-//   err != nil: Any errors encountered during probe execution, wrapped.
-//
-// Note that the latter case may include false negatives, and that program creation may
-// succeed despite an error being returned. Some program types cannot reliably be probed and
-// will also return error. Only `nil` and `ebpf.ErrNotSupported` are conclusive.
-//
-// Probe results are cached and persist throughout any process capability changes.
+// See the package documentation for the meaning of the error return value.
 func HaveV2ISA() error {
 	return probeMisc(v2ISA)
 }
 
 // HaveV3ISA probes the running kernel if instructions of the v3 ISA are supported.
-// Return values have the following semantics:
-//
-//   err == nil: The feature is available.
-//   errors.Is(err, ebpf.ErrNotSupported): The feature is not available.
-//   err != nil: Any errors encountered during probe execution, wrapped.
 //
-// Note that the latter case may include false negatives, and that program creation may
-// succeed despite an error being returned. Some program types cannot reliably be probed and
-// will also return error. Only `nil` and `ebpf.ErrNotSupported` are conclusive.
-//
-// Probe results are cached and persist throughout any process capability changes.
+// See the package documentation for the meaning of the error return value.
 func HaveV3ISA() error {
 	return probeMisc(v3ISA)
 }
 
 // probeMisc checks the kernel for a given supported misc by creating
 // a specialized program probe and loading it.
-// Results are cached and persist throughout any process capability changes.
 func probeMisc(mt miscType) error {
-	if mt > mt.max() {
-		return os.ErrInvalid
-	}
 	mc.Lock()
 	defer mc.Unlock()
 	err, ok := miscs.miscTypes[mt]
@@ -195,10 +147,8 @@ func createMiscProbeAttr(mt miscType) (*sys.ProgLoadAttr, error) {
 			asm.Return(),
 		}
 		// To test the v2 ISA we need a dedicated jump offset other
-		// than the one we would get from Instruction.FixupReferences().
-		if err := insns[1].RewriteJumpOffset(1); err != nil {
-			return nil, err
-		}
+		// than the one we would get from Instructions.FixupReferences().
+		insns[1].Offset = 1
 	case v3ISA:
 		label = "v3isa"
 		insns = asm.Instructions{
diff --git a/features/prog.go b/features/prog.go
index 6cfd091..d09b957 100644
--- a/features/prog.go
+++ b/features/prog.go
@@ -79,17 +79,8 @@ func createProgLoadAttr(pt ebpf.ProgramType) (*sys.ProgLoadAttr, error) {
 }
 
 // HaveProgType probes the running kernel for the availability of the specified program type.
-// Return values have the following semantics:
 //
-//   err == nil: The feature is available.
-//   errors.Is(err, ebpf.ErrNotSupported): The feature is not available.
-//   err != nil: Any errors encountered during probe execution, wrapped.
-//
-// Note that the latter case may include false negatives, and that program creation may
-// succeed despite an error being returned. Some program types cannot reliably be probed and
-// will also return error. Only `nil` and `ebpf.ErrNotSupported` are conclusive.
-//
-// Probe results are cached and persist throughout any process capability changes.
+// See the package documentation for the meaning of the error return value.
 func HaveProgType(pt ebpf.ProgramType) error {
 	if err := validateProgType(pt); err != nil {
 		return err


Test patch:
diff --git a/features/misc_test.go b/features/misc_test.go
index 207dd64..4822fdb 100644
--- a/features/misc_test.go
+++ b/features/misc_test.go
@@ -1,21 +1,12 @@
 package features
 
 import (
-	"errors"
 	"fmt"
-	"math"
-	"os"
 	"testing"
 
 	"github.com/cilium/ebpf/internal/testutils"
 )
 
-func TestInvalidMisc(t *testing.T) {
-	if err := probeMisc(miscType(math.MaxUint32)); !errors.Is(err, os.ErrInvalid) {
-		t.Fatalf("Expected os.ErrInvalid but was: %v", err)
-	}
-}
-
 func TestHaveMisc(t *testing.T) {
 	tests := map[miscType]struct {
 		probe     func() error
diff --git a/linker_test.go b/linker_test.go
index bcb7635..34e028d 100644
--- a/linker_test.go
+++ b/linker_test.go
@@ -74,8 +74,8 @@ func TestForwardFunctionDeclaration(t *testing.T) {
 
 		// This program calls an unimplemented forward function declaration.
 		_, err = NewProgram(spec)
-		if !errors.Is(err, asm.ErrUnsatisfiedReference) {
-			t.Fatal("Expected an error wrapping errUnsatisfiedProgram, got:", err)
+		if !errors.Is(err, asm.ErrUnsatisfiedProgramReference) {
+			t.Fatal("Expected an error wrapping ErrUnsatisfiedProgramReference, got:", err)
 		}
 
 		// Append the implementation of fwd().
diff --git a/prog_test.go b/prog_test.go
index 448fa0a..2f846ca 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -375,7 +375,7 @@ func TestProgramWithUnsatisfiedMap(t *testing.T) {
 
 	_, err = NewProgram(progSpec)
 	if !errors.Is(err, asm.ErrUnsatisfiedMapReference) {
-		t.Fatal("Expected an error wrapping errUnsatisfiedMapReference, got", err)
+		t.Fatal("Expected an error wrapping asm.ErrUnsatisfiedMapReference, got", err)
 	}
 	t.Log(err)
 }