Repository: cilium/ebpf
Pull request #356 (2021-07-29 14:10:52)

Assorted patches in preparation of BTF map auto-population
Best reviewed per commit. These are a few things I encountered while working on prog array auto-population from ELF/BTF.

Making separate PRs here to (hopefully) make reviewing a bit easier.


Patch:
diff --git a/Makefile b/Makefile
index 5dd342c..1ed14b7 100644
--- a/Makefile
+++ b/Makefile
@@ -21,7 +21,7 @@ TARGETS := \
 	testdata/invalid_map \
 	testdata/raw_tracepoint \
 	testdata/invalid_map_static \
-	testdata/initialized_btf_map \
+	testdata/invalid_btf_map_init \
 	testdata/strings \
 	internal/btf/testdata/relocs
 
diff --git a/collection.go b/collection.go
index 17cc694..1ea77f7 100644
--- a/collection.go
+++ b/collection.go
@@ -1,6 +1,7 @@
 package ebpf
 
 import (
+	"encoding/binary"
 	"errors"
 	"fmt"
 	"io"
@@ -25,6 +26,8 @@ type CollectionOptions struct {
 type CollectionSpec struct {
 	Maps     map[string]*MapSpec
 	Programs map[string]*ProgramSpec
+
+	byteOrder binary.ByteOrder
 }
 
 // Copy returns a recursive copy of the spec.
@@ -34,8 +37,9 @@ func (cs *CollectionSpec) Copy() *CollectionSpec {
 	}
 
 	cpy := CollectionSpec{
-		Maps:     make(map[string]*MapSpec, len(cs.Maps)),
-		Programs: make(map[string]*ProgramSpec, len(cs.Programs)),
+		Maps:      make(map[string]*MapSpec, len(cs.Maps)),
+		Programs:  make(map[string]*ProgramSpec, len(cs.Programs)),
+		byteOrder: cs.byteOrder,
 	}
 
 	for name, spec := range cs.Maps {
@@ -314,8 +318,6 @@ func (hc handleCache) close() {
 	for _, handle := range hc.btfHandles {
 		handle.Close()
 	}
-	hc.btfHandles = nil
-	hc.btfSpecs = nil
 }
 
 func lazyLoadCollection(coll *CollectionSpec, opts *CollectionOptions) (
diff --git a/elf_reader.go b/elf_reader.go
index ad5c8e1..4713c2f 100644
--- a/elf_reader.go
+++ b/elf_reader.go
@@ -20,7 +20,7 @@ import (
 )
 
 // elfCode is a convenience to reduce the amount of arguments that have to
-// be passed around explicitly. You should treat it's contents as immutable.
+// be passed around explicitly. You should treat its contents as immutable.
 type elfCode struct {
 	*internal.SafeELFFile
 	sections map[elf.SectionIndex]*elfSection
@@ -189,7 +189,7 @@ func LoadCollectionSpecFromReader(rd io.ReaderAt) (*CollectionSpec, error) {
 		return nil, fmt.Errorf("load programs: %w", err)
 	}
 
-	return &CollectionSpec{maps, progs}, nil
+	return &CollectionSpec{maps, progs, ec.ByteOrder}, nil
 }
 
 func loadLicense(sec *elf.Section) (string, error) {
@@ -620,21 +620,23 @@ func (ec *elfCode) loadBTFMaps(maps map[string]*MapSpec) error {
 func mapSpecFromBTF(name string, def *btf.Struct, inner bool, spec *btf.Spec) (*MapSpec, error) {
 
 	var (
-		key, value                 btf.Type
-		keySize, valueSize         uint32
-		mapType, flags, maxEntries uint32
-		pinType                    PinType
-		innerMapSpec               *MapSpec
-		err                        error
+		key, value         btf.Type
+		keySize, valueSize uint32
+		mapType            MapType
+		flags, maxEntries  uint32
+		pinType            PinType
+		innerMapSpec       *MapSpec
+		err                error
 	)
 
 	for i, member := range def.Members {
 		switch member.Name {
 		case "type":
-			mapType, err = uintFromBTF(member.Type)
+			mt, err := uintFromBTF(member.Type)
 			if err != nil {
 				return nil, fmt.Errorf("can't get type: %w", err)
 			}
+			mapType = MapType(mt)
 
 		case "map_flags":
 			flags, err = uintFromBTF(member.Type)
@@ -744,7 +746,7 @@ func mapSpecFromBTF(name string, def *btf.Struct, inner bool, spec *btf.Spec) (*
 			case *btf.Struct:
 				// The values member pointing to an array of structs means we're expecting
 				// a map-in-map declaration.
-				if MapType(mapType) != ArrayOfMaps && MapType(mapType) != HashOfMaps {
+				if mapType != ArrayOfMaps && mapType != HashOfMaps {
 					return nil, errors.New("outer map needs to be an array or a hash of maps")
 				}
 				if inner {
diff --git a/internal/btf/types.go b/internal/btf/types.go
index 6e7083d..baec67f 100644
--- a/internal/btf/types.go
+++ b/internal/btf/types.go
@@ -238,7 +238,7 @@ var (
 type Member struct {
 	Name
 	Type Type
-	// Offset is the bit offset of this member
+	// Offset is the bit offset of this member.
 	Offset       uint32
 	BitfieldSize uint32
 }
@@ -475,7 +475,7 @@ func (ds *Datasec) copy() Type {
 	return &cpy
 }
 
-// VarSecinfo describes variable in a Datasec
+// VarSecinfo describes variable in a Datasec.
 //
 // It is not a valid Type.
 type VarSecinfo struct {
diff --git a/marshalers.go b/marshalers.go
index f2610ef..348ae4b 100644
--- a/marshalers.go
+++ b/marshalers.go
@@ -39,6 +39,10 @@ func marshalPtr(data interface{}, length int) (internal.Pointer, error) {
 // Returns an error if the given value isn't representable in exactly
 // length bytes.
 func marshalBytes(data interface{}, length int) (buf []byte, err error) {
+	if data == nil {
+		return nil, errors.New("can't marshal a nil value")
+	}
+
 	switch value := data.(type) {
 	case encoding.BinaryMarshaler:
 		buf, err = value.MarshalBinary()
diff --git a/prog.go b/prog.go
index 13bdb6d..75a968a 100644
--- a/prog.go
+++ b/prog.go
@@ -57,16 +57,20 @@ type ProgramSpec struct {
 	// Name is passed to the kernel as a debug aid. Must only contain
 	// alpha numeric and '_' characters.
 	Name string
+
 	// Type determines at which hook in the kernel a program will run.
 	Type       ProgramType
 	AttachType AttachType
-	// Name of a kernel data structure to attach to. It's interpretation
+
+	// Name of a kernel data structure to attach to. Its interpretation
 	// depends on Type and AttachType.
 	AttachTo     string
 	Instructions asm.Instructions
+
 	// Flags is passed to the kernel and specifies additional program
 	// load attributes.
 	Flags uint32
+
 	// License of the program. Some helpers are only available if
 	// the license is deemed compatible with the GPL.
 	//
diff --git a/syscalls.go b/syscalls.go
index f5a3854..d01cad2 100644
--- a/syscalls.go
+++ b/syscalls.go
@@ -372,6 +372,10 @@ func wrapMapError(err error) error {
 		return internal.SyscallError(ErrNotSupported, unix.ENOTSUPP)
 	}
 
+	if errors.Is(err, unix.E2BIG) {
+		return fmt.Errorf("key too big for map: %w", err)
+	}
+
 	return err
 }
 


Test patch:
diff --git a/elf_reader_test.go b/elf_reader_test.go
index bd85348..cfde042 100644
--- a/elf_reader_test.go
+++ b/elf_reader_test.go
@@ -118,6 +118,7 @@ func TestLoadCollectionSpec(t *testing.T) {
 
 	defaultOpts := cmp.Options{
 		cmpopts.IgnoreTypes(new(btf.Map), new(btf.Program)),
+		cmpopts.IgnoreFields(CollectionSpec{}, "byteOrder"),
 		cmpopts.IgnoreFields(ProgramSpec{}, "Instructions", "ByteOrder"),
 		cmpopts.IgnoreMapEntries(func(key string, _ *MapSpec) bool {
 			switch key {
@@ -165,7 +166,7 @@ func TestLoadCollectionSpec(t *testing.T) {
 			t.Errorf("MapSpec mismatch (-want +got):\n%s", diff)
 		}
 
-		if have.Programs["xdp_prog"].ByteOrder != internal.NativeEndian {
+		if have.byteOrder != internal.NativeEndian {
 			return
 		}
 
@@ -304,7 +305,7 @@ func TestLoadInvalidMapMissingSymbol(t *testing.T) {
 }
 
 func TestLoadInitializedBTFMap(t *testing.T) {
-	testutils.Files(t, testutils.Glob(t, "testdata/initialized_btf_map-*.elf"), func(t *testing.T, file string) {
+	testutils.Files(t, testutils.Glob(t, "testdata/invalid_btf_map_init-*.elf"), func(t *testing.T, file string) {
 		_, err := LoadCollectionSpec(file)
 		t.Log(err)
 		if !errors.Is(err, internal.ErrNotSupported) {
@@ -332,7 +333,7 @@ func TestLoadRawTracepoint(t *testing.T) {
 			t.Fatal("Can't parse ELF:", err)
 		}
 
-		if spec.Programs["sched_process_exec"].ByteOrder != internal.NativeEndian {
+		if spec.byteOrder != internal.NativeEndian {
 			return
 		}
 
diff --git a/prog_test.go b/prog_test.go
index e9a07f6..1d76a20 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -643,7 +643,7 @@ func TestProgramTypeLSM(t *testing.T) {
 func TestProgramTargetBTF(t *testing.T) {
 	// Load a file that contains valid BTF, but doesn't contain the types
 	// we need for bpf_iter.
-	fh, err := os.Open("testdata/initialized_btf_map-el.elf")
+	fh, err := os.Open("testdata/invalid_btf_map_init-el.elf")
 	if err != nil {
 		t.Fatal(err)
 	}