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

Signed-off-by: Timo Beckers <timo@isovalent.com>

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)

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{

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)
 		}