Repository: cilium/ebpf
Pull request #937 (2023-02-16 18:22:06)

btf: ensure that TypesIterator is not affected by Spec.Add
Spec.Add will grow Spec.types, which means that an iterator will see newly added types. Instead take a copy of the original slice to ensure that iteration doesn't reach added types.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>


Patch:
diff --git a/btf/btf.go b/btf/btf.go
index 2561202..4f214ce 100644
--- a/btf/btf.go
+++ b/btf/btf.go
@@ -698,7 +698,7 @@ func LoadSplitSpecFromReader(r io.ReaderAt, base *Spec) (*Spec, error) {
 
 // TypesIterator iterates over types of a given spec.
 type TypesIterator struct {
-	spec  *Spec
+	types []Type
 	index int
 	// The last visited type in the spec.
 	Type Type
@@ -706,16 +706,18 @@ type TypesIterator struct {
 
 // Iterate returns the types iterator.
 func (s *Spec) Iterate() *TypesIterator {
-	return &TypesIterator{spec: s, index: 0}
+	// We share the backing array of types with the Spec. This is safe since
+	// we don't allow deletion or shuffling of types.
+	return &TypesIterator{types: s.types, index: 0}
 }
 
 // Next returns true as long as there are any remaining types.
 func (iter *TypesIterator) Next() bool {
-	if len(iter.spec.types) <= iter.index {
+	if len(iter.types) <= iter.index {
 		return false
 	}
 
-	iter.Type = iter.spec.types[iter.index]
+	iter.Type = iter.types[iter.index]
 	iter.index++
 	return true
 }
diff --git a/internal/pinning.go b/internal/pinning.go
index c711353..c4afb0f 100644
--- a/internal/pinning.go
+++ b/internal/pinning.go
@@ -6,15 +6,12 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
-	"unsafe"
 
 	"github.com/cilium/ebpf/internal/sys"
 	"github.com/cilium/ebpf/internal/unix"
 )
 
 func Pin(currentPath, newPath string, fd *sys.FD) error {
-	const bpfFSType = 0xcafe4a11
-
 	if newPath == "" {
 		return errors.New("given pinning path cannot be empty")
 	}
@@ -22,20 +19,11 @@ func Pin(currentPath, newPath string, fd *sys.FD) error {
 		return nil
 	}
 
-	var statfs unix.Statfs_t
-	if err := unix.Statfs(filepath.Dir(newPath), &statfs); err != nil {
+	fsType, err := FSType(filepath.Dir(newPath))
+	if err != nil {
 		return err
 	}
-
-	fsType := int64(statfs.Type)
-	if unsafe.Sizeof(statfs.Type) == 4 {
-		// We're on a 32 bit arch, where statfs.Type is int32. bpfFSType is a
-		// negative number when interpreted as int32 so we need to cast via
-		// uint32 to avoid sign extension.
-		fsType = int64(uint32(statfs.Type))
-	}
-
-	if fsType != bpfFSType {
+	if fsType != BpfFSType {
 		return fmt.Errorf("%s is not on a bpf filesystem", newPath)
 	}
 
@@ -50,7 +38,7 @@ func Pin(currentPath, newPath string, fd *sys.FD) error {
 
 	// Renameat2 is used instead of os.Rename to disallow the new path replacing
 	// an existing path.
-	err := unix.Renameat2(unix.AT_FDCWD, currentPath, unix.AT_FDCWD, newPath, unix.RENAME_NOREPLACE)
+	err = unix.Renameat2(unix.AT_FDCWD, currentPath, unix.AT_FDCWD, newPath, unix.RENAME_NOREPLACE)
 	if err == nil {
 		// Object is now moved to the new pinning path.
 		return nil
diff --git a/internal/statfs.go b/internal/statfs.go
new file mode 100644
index 0000000..c45f0b3
--- /dev/null
+++ b/internal/statfs.go
@@ -0,0 +1,29 @@
+package internal
+
+import (
+	"unsafe"
+
+	"github.com/cilium/ebpf/internal/unix"
+)
+
+const (
+	BpfFSType   = 0xcafe4a11
+	TraceFSType = 0x74726163
+	DebugFSType = 0x64626720
+)
+
+func FSType(path string) (int64, error) {
+	var statfs unix.Statfs_t
+	if err := unix.Statfs(path, &statfs); err != nil {
+		return 0, err
+	}
+
+	fsType := int64(statfs.Type)
+	if unsafe.Sizeof(statfs.Type) == 4 {
+		// We're on a 32 bit arch, where statfs.Type is int32. bpfFSType is a
+		// negative number when interpreted as int32 so we need to cast via
+		// uint32 to avoid sign extension.
+		fsType = int64(uint32(statfs.Type))
+	}
+	return fsType, nil
+}
diff --git a/link/perf_event.go b/link/perf_event.go
index 525d0e9..28f2dab 100644
--- a/link/perf_event.go
+++ b/link/perf_event.go
@@ -445,14 +445,13 @@ func isValidTraceID(s string) bool {
 // The available tracefs paths will depends on distribution choices.
 var getTracefsPath = internal.Memoize(func() (string, error) {
 	for _, p := range []struct {
-		file, path string
+		path   string
+		fsType int64
 	}{
-		// It's not enough to stat /sys/kernel/tracing, since some distros have
-		// a directory without a filesystem there.
-		{"/sys/kernel/tracing/kprobe_events", "/sys/kernel/tracing"},
-		{"/sys/kernel/debug/tracing", "/sys/kernel/debug/tracing"},
+		{"/sys/kernel/tracing", internal.TraceFSType},
+		{"/sys/kernel/debug/tracing", internal.DebugFSType},
 	} {
-		if _, err := os.Stat(p.file); err == nil {
+		if fsType, err := internal.FSType(p.path); err == nil && fsType == p.fsType {
 			return p.path, nil
 		}
 	}


Test patch:
diff --git a/btf/btf_test.go b/btf/btf_test.go
index 8482867..06e4f4e 100644
--- a/btf/btf_test.go
+++ b/btf/btf_test.go
@@ -447,34 +447,40 @@ func ExampleSpec_TypeByName() {
 }
 
 func TestTypesIterator(t *testing.T) {
-	spec := vmlinuxTestdataSpec(t)
+	spec := NewSpec()
 
-	if len(spec.types) < 1 {
-		t.Fatal("Not enough types")
-	}
+	types := []Type{(*Void)(nil), &Int{Size: 4}, &Int{Size: 2}}
+	mustSpecAdd(t, spec, types...)
 
-	// Assertion that 'iphdr' type exists within the spec
-	_, err := spec.AnyTypeByName("iphdr")
-	if err != nil {
-		t.Fatalf("Failed to find 'iphdr' type by name: %s", err)
-	}
+	iter := spec.Iterate()
 
-	found := false
-	count := 0
+	// Add a type after calling Iterate() to make sure the iteration
+	// below doesn't pick it up.
+	mustSpecAdd(t, spec, &Const{types[0]})
 
-	iter := spec.Iterate()
-	for iter.Next() {
-		if !found && iter.Type.TypeName() == "iphdr" {
-			found = true
+	for i, typ := range types {
+		if !iter.Next() {
+			t.Fatal("Iterator ended early at item", i)
+		}
+
+		if iter.Type != typ {
+			t.Fatalf("Expected %p to match %p (%[1]T)", iter.Type, typ)
 		}
-		count += 1
 	}
 
-	if l := len(spec.types); l != count {
-		t.Fatalf("Failed to iterate over all types (%d vs %d)", l, count)
+	if iter.Next() {
+		t.Fatalf("Iterator yielded too many items: %p (%[1]T)", iter.Type)
 	}
-	if !found {
-		t.Fatal("Cannot find 'iphdr' type")
+}
+
+func mustSpecAdd(t *testing.T, s *Spec, types ...Type) {
+	t.Helper()
+
+	for _, typ := range types {
+		_, err := s.Add(typ)
+		if err != nil {
+			t.Fatal(err)
+		}
 	}
 }
 
diff --git a/btf/traversal_test.go b/btf/traversal_test.go
index 05d48f2..96bfa6d 100644
--- a/btf/traversal_test.go
+++ b/btf/traversal_test.go
@@ -11,7 +11,7 @@ import (
 
 func init() {
 	seed := time.Now().UnixMicro()
-	fmt.Println("seed:", seed)
+	fmt.Println("Seed is", seed)
 	rand.Seed(seed)
 }