Repository: cilium/ebpf
Pull request #1655 (2025-01-24 10:16:04)

map: check MapReplacements compatibility after applying BPF_F_MMAPABLE
There were 2 problems with the existing implementation:
- cl.loadMap would mutate the original MapSpec before loading
- map compatibility was checked before cl.loadMap applied the BPF_F_MMAPABLE flag

To address this, the following changes were made:
- MapSpec.Copy() is called in loadMap, to follow suit with cl.loadProgram
- the compatibility check on MapReplacements was deferred to loadMap, after flags are mutated
- Added a superficial CollectionSpec mutation test after loading, using the same logic as TestLoadCollectionSpec. testutils.IsDeepCopy() isn't usable here since MapSpec.Copy() doesn't deepcopy .Contents.
- Added a regression test on using .data and .rodata in MapReplacements

Fixes issues seen in https://github.com/cilium/pwru/pull/484. cc @brb @Asphaltt 


Patch:
diff --git a/collection.go b/collection.go
index 1bda110..a7b6cb3 100644
--- a/collection.go
+++ b/collection.go
@@ -411,15 +411,10 @@ func newCollectionLoader(coll *CollectionSpec, opts *CollectionOptions) (*collec
 	}
 
 	// Check for existing MapSpecs in the CollectionSpec for all provided replacement maps.
-	for name, m := range opts.MapReplacements {
-		spec, ok := coll.Maps[name]
-		if !ok {
+	for name := range opts.MapReplacements {
+		if _, ok := coll.Maps[name]; !ok {
 			return nil, fmt.Errorf("replacement map %s not found in CollectionSpec", name)
 		}
-
-		if err := spec.Compatible(m); err != nil {
-			return nil, fmt.Errorf("using replacement map %s: %w", spec.Name, err)
-		}
 	}
 
 	if err := populateKallsyms(coll.Programs); err != nil {
@@ -494,7 +489,22 @@ func (cl *collectionLoader) loadMap(mapName string) (*Map, error) {
 		return nil, fmt.Errorf("missing map %s", mapName)
 	}
 
+	mapSpec = mapSpec.Copy()
+
+	// Defer setting the mmapable flag on maps until load time. This avoids the
+	// MapSpec having different flags on some kernel versions. Also avoid running
+	// syscalls during ELF loading, so platforms like wasm can also parse an ELF.
+	if isDataSection(mapSpec.Name) && haveMmapableMaps() == nil {
+		mapSpec.Flags |= sys.BPF_F_MMAPABLE
+	}
+
 	if replaceMap, ok := cl.opts.MapReplacements[mapName]; ok {
+		// Check compatibility with the replacement map after setting
+		// feature-dependent map flags.
+		if err := mapSpec.Compatible(replaceMap); err != nil {
+			return nil, fmt.Errorf("using replacement map %s: %w", mapSpec.Name, err)
+		}
+
 		// Clone the map to avoid closing user's map later on.
 		m, err := replaceMap.Clone()
 		if err != nil {
@@ -505,13 +515,6 @@ func (cl *collectionLoader) loadMap(mapName string) (*Map, error) {
 		return m, nil
 	}
 
-	// Defer setting the mmapable flag on maps until load time. This avoids the
-	// MapSpec having different flags on some kernel versions. Also avoid running
-	// syscalls during ELF loading, so platforms like wasm can also parse an ELF.
-	if isDataSection(mapSpec.Name) && haveMmapableMaps() == nil {
-		mapSpec.Flags |= sys.BPF_F_MMAPABLE
-	}
-
 	m, err := newMapWithOptions(mapSpec, cl.opts.Maps)
 	if err != nil {
 		return nil, fmt.Errorf("map %s: %w", mapName, err)
diff --git a/linker.go b/linker.go
index 6f97af2..024b72b 100644
--- a/linker.go
+++ b/linker.go
@@ -207,13 +207,19 @@ func flattenPrograms(progs map[string]*ProgramSpec, names []string) {
 // dependencies of each program.
 func flattenInstructions(name string, progs map[string]*ProgramSpec, refs map[*ProgramSpec][]string) asm.Instructions {
 	prog := progs[name]
+	progRefs := refs[prog]
+
+	if len(progRefs) == 0 {
+		// No references, nothing to do.
+		return prog.Instructions
+	}
 
 	insns := make(asm.Instructions, len(prog.Instructions))
 	copy(insns, prog.Instructions)
 
 	// Add all direct references of prog to the list of to be linked programs.
-	pending := make([]string, len(refs[prog]))
-	copy(pending, refs[prog])
+	pending := make([]string, len(progRefs))
+	copy(pending, progRefs)
 
 	// All references for which we've appended instructions.
 	linked := make(map[string]bool)
diff --git a/pin/load.go b/pin/load.go
index 817e177..b3d2fe7 100644
--- a/pin/load.go
+++ b/pin/load.go
@@ -2,6 +2,7 @@ package pin
 
 import (
 	"fmt"
+	"io"
 
 	"github.com/cilium/ebpf"
 	"github.com/cilium/ebpf/internal/sys"
@@ -11,6 +12,7 @@ import (
 // Pinner is an interface implemented by all eBPF objects that support pinning
 // to a bpf virtual filesystem.
 type Pinner interface {
+	io.Closer
 	Pin(string) error
 }
 


Test patch:
diff --git a/collection_test.go b/collection_test.go
index b59b360..7f79cba 100644
--- a/collection_test.go
+++ b/collection_test.go
@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"github.com/go-quicktest/qt"
+	"github.com/google/go-cmp/cmp"
 
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/btf"
@@ -119,6 +120,29 @@ func TestCollectionSpecLoadCopy(t *testing.T) {
 	defer objs.Prog.Close()
 }
 
+func TestCollectionSpecLoadMutate(t *testing.T) {
+	file := testutils.NativeFile(t, "testdata/loader-%s.elf")
+	spec, err := LoadCollectionSpec(file)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	orig := spec.Copy()
+
+	var objs struct {
+		Prog *Program `ebpf:"xdp_prog"`
+	}
+
+	err = spec.LoadAndAssign(&objs, nil)
+	testutils.SkipIfNotSupported(t, err)
+	qt.Assert(t, qt.IsNil(err))
+	defer objs.Prog.Close()
+
+	if diff := cmp.Diff(orig, spec, csCmpOpts...); diff != "" {
+		t.Errorf("CollectionSpec was mutated after loading (-want +got):\n%s", diff)
+	}
+}
+
 func TestCollectionSpecRewriteMaps(t *testing.T) {
 	insns := asm.Instructions{
 		// R1 map
@@ -264,6 +288,7 @@ func TestCollectionSpecMapReplacements(t *testing.T) {
 		t.Fatalf("failed to update replaced map: %s", err)
 	}
 }
+
 func TestCollectionSpecMapReplacements_NonExistingMap(t *testing.T) {
 	cs := &CollectionSpec{
 		Maps: map[string]*MapSpec{
@@ -333,6 +358,38 @@ func TestCollectionSpecMapReplacements_SpecMismatch(t *testing.T) {
 	}
 }
 
+func TestMapReplacementsDataSections(t *testing.T) {
+	// In some circumstances, it can be useful to share data sections between
+	// Collections, for example to hold a ready/pause flag or some metrics.
+	// Test read-only maps for good measure.
+	file := testutils.NativeFile(t, "testdata/loader-%s.elf")
+	spec, err := LoadCollectionSpec(file)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var objs struct {
+		Data   *Map `ebpf:".data"`
+		ROData *Map `ebpf:".rodata"`
+	}
+
+	err = spec.LoadAndAssign(&objs, nil)
+	testutils.SkipIfNotSupported(t, err)
+	qt.Assert(t, qt.IsNil(err))
+	defer objs.Data.Close()
+	defer objs.ROData.Close()
+
+	qt.Assert(t, qt.IsNil(
+		spec.LoadAndAssign(&objs, &CollectionOptions{
+			MapReplacements: map[string]*Map{
+				".data":   objs.Data,
+				".rodata": objs.ROData,
+			},
+		})))
+	defer objs.Data.Close()
+	defer objs.ROData.Close()
+}
+
 func TestCollectionSpec_LoadAndAssign_LazyLoading(t *testing.T) {
 	spec := &CollectionSpec{
 		Maps: map[string]*MapSpec{
diff --git a/elf_reader_test.go b/elf_reader_test.go
index 06fb132..07711dd 100644
--- a/elf_reader_test.go
+++ b/elf_reader_test.go
@@ -24,6 +24,27 @@ import (
 	"github.com/go-quicktest/qt"
 )
 
+var csCmpOpts = cmp.Options{
+	// Dummy Comparer that works with empty readers to support test cases.
+	cmp.Comparer(func(a, b bytes.Reader) bool {
+		if a.Len() == 0 && b.Len() == 0 {
+			return true
+		}
+		return false
+	}),
+	cmp.Comparer(func(a, b *VariableSpec) bool {
+		if a.name != b.name || a.offset != b.offset || a.size != b.size {
+			return false
+		}
+		return true
+	}),
+	cmpopts.IgnoreTypes(btf.Spec{}),
+	cmpopts.IgnoreFields(CollectionSpec{}, "ByteOrder", "Types"),
+	cmpopts.IgnoreFields(ProgramSpec{}, "Instructions", "ByteOrder"),
+	cmpopts.IgnoreFields(MapSpec{}, "Key", "Value", "Contents"),
+	cmpopts.IgnoreUnexported(ProgramSpec{}),
+}
+
 func TestLoadCollectionSpec(t *testing.T) {
 	coll := &CollectionSpec{
 		Maps: map[string]*MapSpec{
@@ -198,27 +219,6 @@ func TestLoadCollectionSpec(t *testing.T) {
 		},
 	}
 
-	cmpOpts := cmp.Options{
-		// Dummy Comparer that works with empty readers to support test cases.
-		cmp.Comparer(func(a, b bytes.Reader) bool {
-			if a.Len() == 0 && b.Len() == 0 {
-				return true
-			}
-			return false
-		}),
-		cmp.Comparer(func(a, b *VariableSpec) bool {
-			if a.name != b.name || a.offset != b.offset || a.size != b.size {
-				return false
-			}
-			return true
-		}),
-		cmpopts.IgnoreTypes(new(btf.Spec)),
-		cmpopts.IgnoreFields(CollectionSpec{}, "ByteOrder", "Types"),
-		cmpopts.IgnoreFields(ProgramSpec{}, "Instructions", "ByteOrder"),
-		cmpopts.IgnoreFields(MapSpec{}, "Key", "Value", "Contents"),
-		cmpopts.IgnoreUnexported(ProgramSpec{}),
-	}
-
 	testutils.Files(t, testutils.Glob(t, "testdata/loader-*.elf"), func(t *testing.T, file string) {
 		have, err := LoadCollectionSpec(file)
 		if err != nil {
@@ -250,7 +250,7 @@ func TestLoadCollectionSpec(t *testing.T) {
 		qt.Assert(t, qt.Equals(have.Maps["perf_event_array"].ValueSize, 0))
 		qt.Assert(t, qt.IsNotNil(have.Maps["perf_event_array"].Value))
 
-		if diff := cmp.Diff(coll, have, cmpOpts...); diff != "" {
+		if diff := cmp.Diff(coll, have, csCmpOpts); diff != "" {
 			t.Errorf("MapSpec mismatch (-want +got):\n%s", diff)
 		}
 
diff --git a/linker_test.go b/linker_test.go
index 39b01cf..cbe31d6 100644
--- a/linker_test.go
+++ b/linker_test.go
@@ -159,3 +159,24 @@ func TestSplitSymbols(t *testing.T) {
 	qt.Assert(t, qt.HasLen(m["sym3"], 3))
 	qt.Assert(t, qt.HasLen(m["sym4"], 4))
 }
+
+func TestFlattenInstructionsAllocations(t *testing.T) {
+	name := "entrypoint"
+	instructions := asm.Instructions{
+		asm.LoadImm(asm.R0, 0, asm.DWord),
+		asm.Return(),
+	}
+	prog := &ProgramSpec{
+		Name:         name,
+		Instructions: instructions,
+	}
+	progs := map[string]*ProgramSpec{name: prog}
+	refs := make(map[*ProgramSpec][]string)
+
+	// ensure that flattenInstructions does not allocate memory
+	// if there is no reference for the given program.
+	allocs := testing.AllocsPerRun(5, func() {
+		_ = flattenInstructions(name, progs, refs)
+	})
+	qt.Assert(t, qt.Equals(allocs, float64(0)))
+}
diff --git a/pin/load_test.go b/pin/load_test.go
index a16b794..742355c 100644
--- a/pin/load_test.go
+++ b/pin/load_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/cilium/ebpf"
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/internal/testutils"
+	"github.com/cilium/ebpf/internal/testutils/fdtrace"
 )
 
 func mustPinnedProgram(t *testing.T, path string) *ebpf.Program {
@@ -77,9 +78,15 @@ func TestLoad(t *testing.T) {
 
 	m, err := Load(mpath, nil)
 	qt.Assert(t, qt.IsNil(err))
+	defer m.Close()
 	qt.Assert(t, qt.Satisfies(m, testutils.Contains[*ebpf.Map]))
 
 	p, err := Load(ppath, nil)
 	qt.Assert(t, qt.IsNil(err))
+	defer p.Close()
 	qt.Assert(t, qt.Satisfies(p, testutils.Contains[*ebpf.Program]))
 }
+
+func TestMain(m *testing.M) {
+	fdtrace.TestMain(m)
+}
diff --git a/pin/walk_test.go b/pin/walk_test.go
index bde193e..13bf164 100644
--- a/pin/walk_test.go
+++ b/pin/walk_test.go
@@ -27,6 +27,10 @@ func TestWalkDir(t *testing.T) {
 	bpffn := func(path string, d fs.DirEntry, obj Pinner, err error) error {
 		qt.Assert(t, qt.IsNil(err))
 
+		if obj != nil {
+			defer obj.Close()
+		}
+
 		if path == "." {
 			return nil
 		}