Repository: cilium/ebpf
Pull request #1094 (2023-07-13 09:32:47)

elf: don't clamp PerfEventArray.MaxEntries at parse time
elf: don't clamp PerfEventArray.MaxEntries at parse time

    One key idea of the ELF reader is that it is deterministic: it should always
    produce the same output regardless of which system it runs on. The
    PerfEventArray clamping logic violates this since it substitutes the number
    of possible CPUs.

    Move the logic into map creation and adjust MapSpec.Compatible to do the
    same fixup.

    Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

map: fix MapSpec.Compatible for PerfEventArray with zero key or value size

    MapSpec.Compatible currently returns an error when comparing a
    PerfEventArray created from a MapSpec without explicit key or value sizes.
    This is because we only do the fixup just before executing the
    BPF_MAP_CREATE syscall.

    Move the fixup logic into a separate function so that we can invoke it from 
    MapSpec.Compatible.

    Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

map: fix buggy TestMapLoadReusePinned

    The test calls t.Fatal on the wrong (parent) t since it reuses qt.New from
    the parent scope. Remove qt.New.

    Signed-off-by: Lorenz Bauer <lmb@isovalent.com>



Patch:
diff --git a/elf_reader.go b/elf_reader.go
index 8d92672..c777917 100644
--- a/elf_reader.go
+++ b/elf_reader.go
@@ -694,10 +694,6 @@ func (ec *elfCode) loadMaps() error {
 				spec.Extra = bytes.NewReader(extra)
 			}
 
-			if err := spec.clampPerfEventArraySize(); err != nil {
-				return fmt.Errorf("map %s: %w", mapName, err)
-			}
-
 			ec.maps[mapName] = &spec
 		}
 	}
@@ -762,10 +758,6 @@ func (ec *elfCode) loadBTFMaps() error {
 				return fmt.Errorf("map %v: %w", name, err)
 			}
 
-			if err := mapSpec.clampPerfEventArraySize(); err != nil {
-				return fmt.Errorf("map %v: %w", name, err)
-			}
-
 			ec.maps[name] = mapSpec
 		}
 
diff --git a/map.go b/map.go
index 4d16e6f..828c24a 100644
--- a/map.go
+++ b/map.go
@@ -102,26 +102,55 @@ func (ms *MapSpec) Copy() *MapSpec {
 	return &cpy
 }
 
-func (ms *MapSpec) clampPerfEventArraySize() error {
-	if ms.Type != PerfEventArray {
-		return nil
-	}
+// fixupMagicFields fills fields of MapSpec which are usually
+// left empty in ELF or which depend on runtime information.
+//
+// The method doesn't modify Spec, instead returning a copy.
+// The copy is only performed if fixups are necessary, so callers mustn't mutate
+// the returned spec.
+func (spec *MapSpec) fixupMagicFields() (*MapSpec, error) {
+	switch spec.Type {
+	case ArrayOfMaps, HashOfMaps:
+		if spec.ValueSize != 0 && spec.ValueSize != 4 {
+			return nil, errors.New("ValueSize must be zero or four for map of map")
+		}
 
-	n, err := internal.PossibleCPUs()
-	if err != nil {
-		return fmt.Errorf("perf event array: %w", err)
-	}
+		spec = spec.Copy()
+		spec.ValueSize = 4
+
+	case PerfEventArray:
+		if spec.KeySize != 0 && spec.KeySize != 4 {
+			return nil, errors.New("KeySize must be zero or four for perf event array")
+		}
 
-	if n := uint32(n); ms.MaxEntries > n {
-		ms.MaxEntries = n
+		if spec.ValueSize != 0 && spec.ValueSize != 4 {
+			return nil, errors.New("ValueSize must be zero or four for perf event array")
+		}
+
+		spec = spec.Copy()
+		spec.KeySize = 4
+		spec.ValueSize = 4
+
+		n, err := internal.PossibleCPUs()
+		if err != nil {
+			return nil, fmt.Errorf("fixup perf event array: %w", err)
+		}
+
+		if n := uint32(n); spec.MaxEntries == 0 || spec.MaxEntries > n {
+			// MaxEntries should be zero most of the time, but there is code
+			// out there which hardcodes large constants. Clamp the number
+			// of entries to the number of CPUs at most. Allow creating maps with
+			// less than n items since some kernel selftests relied on this
+			// behaviour in the past.
+			spec.MaxEntries = n
+		}
 	}
 
-	return nil
+	return spec, nil
 }
 
 // dataSection returns the contents and BTF Datasec descriptor of the spec.
 func (ms *MapSpec) dataSection() ([]byte, *btf.Datasec, error) {
-
 	if ms.Value == nil {
 		return nil, nil, errMapNoBTFValue
 	}
@@ -155,6 +184,11 @@ type MapKV struct {
 //
 // Returns an error wrapping [ErrMapIncompatible] otherwise.
 func (ms *MapSpec) Compatible(m *Map) error {
+	ms, err := ms.fixupMagicFields()
+	if err != nil {
+		return err
+	}
+
 	switch {
 	case m.typ != ms.Type:
 		return fmt.Errorf("expected type %v, got %v: %w", ms.Type, m.typ, ErrMapIncompatible)
@@ -165,8 +199,7 @@ func (ms *MapSpec) Compatible(m *Map) error {
 	case m.valueSize != ms.ValueSize:
 		return fmt.Errorf("expected value size %v, got %v: %w", ms.ValueSize, m.valueSize, ErrMapIncompatible)
 
-	case !(ms.Type == PerfEventArray && ms.MaxEntries == 0) &&
-		m.maxEntries != ms.MaxEntries:
+	case m.maxEntries != ms.MaxEntries:
 		return fmt.Errorf("expected max entries %v, got %v: %w", ms.MaxEntries, m.maxEntries, ErrMapIncompatible)
 
 	// BPF_F_RDONLY_PROG is set unconditionally for devmaps. Explicitly allow
@@ -350,39 +383,9 @@ func (spec *MapSpec) createMap(inner *sys.FD, opts MapOptions) (_ *Map, err erro
 		}
 	}
 
-	switch spec.Type {
-	case ArrayOfMaps, HashOfMaps:
-		if err := haveNestedMaps(); err != nil {
-			return nil, err
-		}
-
-		if spec.ValueSize != 0 && spec.ValueSize != 4 {
-			return nil, errors.New("ValueSize must be zero or four for map of map")
-		}
-
-		spec = spec.Copy()
-		spec.ValueSize = 4
-
-	case PerfEventArray:
-		if spec.KeySize != 0 && spec.KeySize != 4 {
-			return nil, errors.New("KeySize must be zero or four for perf event array")
-		}
-
-		if spec.ValueSize != 0 && spec.ValueSize != 4 {
-			return nil, errors.New("ValueSize must be zero or four for perf event array")
-		}
-
-		spec = spec.Copy()
-		spec.KeySize = 4
-		spec.ValueSize = 4
-
-		if spec.MaxEntries == 0 {
-			n, err := internal.PossibleCPUs()
-			if err != nil {
-				return nil, fmt.Errorf("perf event array: %w", err)
-			}
-			spec.MaxEntries = uint32(n)
-		}
+	spec, err = spec.fixupMagicFields()
+	if err != nil {
+		return nil, err
 	}
 
 	attr := sys.MapCreateAttr{
@@ -437,6 +440,13 @@ func (spec *MapSpec) createMap(inner *sys.FD, opts MapOptions) (_ *Map, err erro
 			return nil, fmt.Errorf("map create: cannot use type %s", UnspecifiedMap)
 		}
 
+		switch spec.Type {
+		case ArrayOfMaps, HashOfMaps:
+			if haveFeatErr := haveNestedMaps(); haveFeatErr != nil {
+				return nil, fmt.Errorf("map create: %w", haveFeatErr)
+			}
+		}
+
 		if spec.Flags&(unix.BPF_F_RDONLY_PROG|unix.BPF_F_WRONLY_PROG) > 0 || spec.Freeze {
 			if haveFeatErr := haveMapMutabilityModifiers(); haveFeatErr != nil {
 				return nil, fmt.Errorf("map create: %w", haveFeatErr)


Test patch:
diff --git a/elf_reader_test.go b/elf_reader_test.go
index d168939..093a512 100644
--- a/elf_reader_test.go
+++ b/elf_reader_test.go
@@ -24,11 +24,6 @@ import (
 )
 
 func TestLoadCollectionSpec(t *testing.T) {
-	cpus, err := internal.PossibleCPUs()
-	if err != nil {
-		t.Fatal(err)
-	}
-
 	coll := &CollectionSpec{
 		Maps: map[string]*MapSpec{
 			"hash_map": {
@@ -55,7 +50,7 @@ func TestLoadCollectionSpec(t *testing.T) {
 			"perf_event_array": {
 				Name:       "perf_event_array",
 				Type:       PerfEventArray,
-				MaxEntries: uint32(cpus),
+				MaxEntries: 4096,
 			},
 			// Maps prefixed by btf_ are ignored when testing ELFs
 			// that don't have BTF info embedded. (clang<9)
diff --git a/map_test.go b/map_test.go
index 7c5e195..cb4fd0c 100644
--- a/map_test.go
+++ b/map_test.go
@@ -637,8 +637,6 @@ func TestMapLoadPinned(t *testing.T) {
 }
 
 func TestMapLoadReusePinned(t *testing.T) {
-	c := qt.New(t)
-
 	for _, typ := range []MapType{Array, Hash, DevMap, DevMapHash} {
 		t.Run(typ.String(), func(t *testing.T) {
 			if typ == DevMap {
@@ -658,11 +656,11 @@ func TestMapLoadReusePinned(t *testing.T) {
 			}
 
 			m1, err := NewMapWithOptions(spec, MapOptions{PinPath: tmp})
-			c.Assert(err, qt.IsNil)
+			qt.Assert(t, err, qt.IsNil)
 			defer m1.Close()
 
 			m2, err := NewMapWithOptions(spec, MapOptions{PinPath: tmp})
-			c.Assert(err, qt.IsNil)
+			qt.Assert(t, err, qt.IsNil)
 			defer m2.Close()
 		})
 	}
@@ -1734,6 +1732,21 @@ func TestMapPinning(t *testing.T) {
 	}
 }
 
+func TestPerfEventArrayCompatible(t *testing.T) {
+	ms := &MapSpec{
+		Type: PerfEventArray,
+	}
+
+	m, err := NewMap(ms)
+	qt.Assert(t, err, qt.IsNil)
+	defer m.Close()
+
+	qt.Assert(t, ms.Compatible(m), qt.IsNil)
+
+	ms.MaxEntries = m.MaxEntries() - 1
+	qt.Assert(t, ms.Compatible(m), qt.IsNotNil)
+}
+
 type benchValue struct {
 	ID      uint32
 	Val16   uint16