From 0c1e672d28c6e1fb9a05e2ad6271d11fa555abe1 Mon Sep 17 00:00:00 2001 From: Timo Beckers Date: Wed, 16 Feb 2022 13:17:19 +0100 Subject: [PATCH 1/5] asm: remove Instruction.RewriteJumpOffset() As this function is a relatively lean wrapper around setting ins.Offset and doesn't do any particular sanity checking to see if e.g. the offset is within bounds, it's not clear what benefit this provides being part of the exported API. As its only use is within the test suite where the instruction stream is known up front, remove the function for now. Signed-off-by: Timo Beckers --- asm/instruction.go | 11 ----------- features/misc.go | 6 ++---- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/asm/instruction.go b/asm/instruction.go index 8c6d502be..d1cfe0f04 100644 --- a/asm/instruction.go +++ b/asm/instruction.go @@ -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) } diff --git a/features/misc.go b/features/misc.go index 093465289..b14f220f6 100644 --- a/features/misc.go +++ b/features/misc.go @@ -195,10 +195,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{ From d9de07e8abbe683e99e21e8a093105e50e06e5e0 Mon Sep 17 00:00:00 2001 From: Timo Beckers Date: Wed, 16 Feb 2022 15:39:35 +0100 Subject: [PATCH 2/5] features: remove maxMiscType and miscType.max() As miscTypes cannot be specified by the caller, there's no strong reason to input check these arguments. Calls to createMiscProbeAttr() will fail when the miscTyps is not implemented anyway. Signed-off-by: Timo Beckers --- features/misc.go | 13 +------------ features/misc_test.go | 9 --------- 2 files changed, 1 insertion(+), 21 deletions(-) diff --git a/features/misc.go b/features/misc.go index b14f220f6..07f9ac5a7 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 ( @@ -124,9 +116,6 @@ func HaveV3ISA() error { // 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] diff --git a/features/misc_test.go b/features/misc_test.go index 207dd64da..4822fdbd8 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 From c6010554c332b40db93f559d774d9ba4a5fd7802 Mon Sep 17 00:00:00 2001 From: Timo Beckers Date: Wed, 16 Feb 2022 15:53:04 +0100 Subject: [PATCH 3/5] features: define probe error return semantics in package doc Signed-off-by: Timo Beckers --- features/doc.go | 17 +++++++++++++++++ features/map.go | 11 +---------- features/misc.go | 45 ++++----------------------------------------- features/prog.go | 11 +---------- 4 files changed, 23 insertions(+), 61 deletions(-) diff --git a/features/doc.go b/features/doc.go index f44510145..723c7ae72 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 29a874279..eab39bfa9 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 07f9ac5a7..c9eff0957 100644 --- a/features/misc.go +++ b/features/misc.go @@ -49,72 +49,35 @@ 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 { mc.Lock() defer mc.Unlock() diff --git a/features/prog.go b/features/prog.go index 6cfd091f1..d09b95751 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 From a27ae6d6f4438b557f3a7234fe8cf369dc3be44e Mon Sep 17 00:00:00 2001 From: Timo Beckers Date: Wed, 16 Feb 2022 15:54:04 +0100 Subject: [PATCH 4/5] asm: rename ErrUnsatisfied{,Program}Reference The error string was changed when it was exported in #541 and is potentially ambiguous. Signed-off-by: Timo Beckers --- asm/instruction.go | 4 ++-- linker_test.go | 4 ++-- prog_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/asm/instruction.go b/asm/instruction.go index d1cfe0f04..b1fae989d 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 { @@ -584,7 +584,7 @@ func (insns Instructions) FixupReferences() error { // no fixup needed continue } - return fmt.Errorf("%s at insn %d: symbol %q: %w", ins.OpCode, i, ins.Reference, ErrUnsatisfiedReference) + return fmt.Errorf("%s at insn %d: symbol %q: %w", ins.OpCode, i, ins.Reference, ErrUnsatisfiedProgramReference) } return nil } diff --git a/linker_test.go b/linker_test.go index bcb763525..34e028de9 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 448fa0a38..2f846ca75 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) } From 3880052a663a028fedeb5d4f3a0697201d629d97 Mon Sep 17 00:00:00 2001 From: Lorenz Bauer Date: Thu, 17 Feb 2022 11:26:09 +0000 Subject: [PATCH 5/5] asm: remove fallthrough from FixupReferences I keep reading the switch in FixupReferences wrong: I always assume that any instruction with a reference will trigger the unsatisfied reference error. This isn't the case since only the program reference switch cases execute a break. Make the whole thing easier to understand by copying the error message into two places. --- asm/instruction.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/asm/instruction.go b/asm/instruction.go index b1fae989d..1d37a73c5 100644 --- a/asm/instruction.go +++ b/asm/instruction.go @@ -560,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, ErrUnsatisfiedProgramReference) } return nil }