Repository: cilium/ebpf
Pull request #1558 (2024-09-03 15:04:48)

map: remove MapSpec.Freeze field
This has been an ambiguous flag since its inception. Internally, we would allow maps to be created with BPF_F_RDONLY_PROG, but not freeze them afterwards. Conversely, we would allow MapSpecs with Freeze set to 'true', but without the rdonly map flag set.

The latter case has caused subtle bugs in the past, since BPF_MAP_FREEZE only blocks further modifications from user space, but doesn't actually mark the map as readonly for the verifier. This will prevent code pruning and other optimizations from taking place.

This commit removes the Freeze field in favor of the map flag and adds a helper to keep existing call sites simple. Sourcegraph code search yields no references to this field in open-source code.


Patch:
diff --git a/elf_reader.go b/elf_reader.go
index a7b844a..cf7a31d 100644
--- a/elf_reader.go
+++ b/elf_reader.go
@@ -200,6 +200,10 @@ func loadVersion(sec *elf.Section, bo binary.ByteOrder) (uint32, error) {
 	return version, nil
 }
 
+func isConstantDataSection(name string) bool {
+	return strings.HasPrefix(name, ".rodata")
+}
+
 type elfSectionKind int
 
 const (
@@ -1137,9 +1141,8 @@ func (ec *elfCode) loadDataSections() error {
 			}
 		}
 
-		if strings.HasPrefix(sec.Name, ".rodata") {
+		if isConstantDataSection(sec.Name) {
 			mapSpec.Flags = sys.BPF_F_RDONLY_PROG
-			mapSpec.Freeze = true
 		}
 
 		ec.maps[sec.Name] = mapSpec
@@ -1175,7 +1178,6 @@ func (ec *elfCode) loadKconfigSection() error {
 		ValueSize:  ds.Size,
 		MaxEntries: 1,
 		Flags:      sys.BPF_F_RDONLY_PROG,
-		Freeze:     true,
 		Key:        &btf.Int{Size: 4},
 		Value:      ds,
 	}
diff --git a/map.go b/map.go
index d0ea05d..36fe274 100644
--- a/map.go
+++ b/map.go
@@ -73,9 +73,6 @@ type MapSpec struct {
 	// The initial contents of the map. May be nil.
 	Contents []MapKV
 
-	// Whether to freeze a map after setting its initial contents.
-	Freeze bool
-
 	// InnerMap is used as a template for ArrayOfMaps and HashOfMaps
 	InnerMap *MapSpec
 
@@ -190,6 +187,10 @@ func (ms *MapSpec) dataSection() ([]byte, *btf.Datasec, error) {
 	return value, ds, nil
 }
 
+func (ms *MapSpec) readOnly() bool {
+	return (ms.Flags & sys.BPF_F_RDONLY_PROG) > 0
+}
+
 // MapKV is used to initialize the contents of a Map.
 type MapKV struct {
 	Key   interface{}
@@ -484,7 +485,7 @@ func handleMapCreateError(attr sys.MapCreateAttr, spec *MapSpec, err error) erro
 			return fmt.Errorf("map create: %w", haveFeatErr)
 		}
 	}
-	if spec.Flags&(sys.BPF_F_RDONLY_PROG|sys.BPF_F_WRONLY_PROG) > 0 || spec.Freeze {
+	if spec.Flags&(sys.BPF_F_RDONLY_PROG|sys.BPF_F_WRONLY_PROG) > 0 {
 		if haveFeatErr := haveMapMutabilityModifiers(); haveFeatErr != nil {
 			return fmt.Errorf("map create: %w", haveFeatErr)
 		}
@@ -1400,7 +1401,7 @@ func (m *Map) finalize(spec *MapSpec) error {
 		}
 	}
 
-	if spec.Freeze {
+	if spec.readOnly() {
 		if err := m.Freeze(); err != nil {
 			return fmt.Errorf("freezing map: %w", err)
 		}


Test patch:
diff --git a/elf_reader_test.go b/elf_reader_test.go
index 2457afe..089c457 100644
--- a/elf_reader_test.go
+++ b/elf_reader_test.go
@@ -491,7 +491,7 @@ func TestStringSection(t *testing.T) {
 		t.Fatal("Unable to find map '.rodata.str1.1' in loaded collection")
 	}
 
-	if !strMap.Freeze {
+	if !strMap.readOnly() {
 		t.Fatal("Read only data maps should be frozen")
 	}
 
diff --git a/map_test.go b/map_test.go
index b8dd30f..93dd764 100644
--- a/map_test.go
+++ b/map_test.go
@@ -104,8 +104,7 @@ func TestMapSpecCopy(t *testing.T) {
 		PinByName,
 		1,
 		[]MapKV{{1, 2}}, // Can't copy Contents, use value types
-		true,
-		nil, // InnerMap
+		nil,             // InnerMap
 		bytes.NewReader(nil),
 		&btf.Int{},
 		&btf.Int{},