From 75efc6828d7eff0ddaebbcf465ade7ac30f7670e Mon Sep 17 00:00:00 2001 From: Timo Beckers Date: Fri, 24 Jan 2025 10:57:28 +0100 Subject: [PATCH] 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 --- collection.go | 31 +++++++++++++------------ collection_test.go | 57 ++++++++++++++++++++++++++++++++++++++++++++++ elf_reader_test.go | 44 +++++++++++++++++------------------ 3 files changed, 96 insertions(+), 36 deletions(-) diff --git a/collection.go b/collection.go index 1bda110a4..a7b6cb31b 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/collection_test.go b/collection_test.go index b59b360cb..7f79cbaca 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 06fb132c2..07711dd87 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) }