Repository: cilium/ebpf
Pull request #732 (2022-07-07 10:17:07)

sys: add fd leak tracing instrumentation
In an effort to improve the library's fd hygiene, this patch adds optional
metadata to sys.FD to hold a name and a stack trace of where the fd was
created, as well as a simple subscription mechanism for packages to
get notified when a live fd gets garbage collected due to losing its reference.

Metadata is only collected when there are active tracers, so they must be
registered before creating the first sys.FD.

This has shaken out a few minor bugs so far. Most of them from the test suite,
but also a few where we forgot to close resources that were no longer needed
before returning from LoadAndAssign and some other internal APIs

All test packages now also include a TestMain that sets up leak tracing and
starts a GC round before finishing the test, so we can catch these kinds of
errors more easily in the future.

The idea is to export the sys package over time, which would make this
instrumentation available to all, after it has been given some time to mature.

ti-mo: Thanks for the feedback. :pray: 

Something else I considered is making `sys.FD` an interface, since it's already mostly consumed from outside the package through a clean API, but I don't think that's an improvement performance-wise if weighed against adding a single `*metadata` pointer in the struct. I would suggest we carry the field initially, maybe we can store metadata in a dedicated lookaside map later on.
lmb: > Something else I considered is making sys.FD an interface, since it's already mostly consumed from outside the package through a clean API, but I don't think that's an improvement performance-wise if weighed against adding a single *metadata pointer in the struct.

Agreed. sys.FD as interface means additional allocation and it also prevents function inlining, which it'd be good to avoid.

> I would suggest we carry the field initially, maybe we can store metadata in a dedicated lookaside map later on.

Metadata would have to become `any` for that to work I guess?
ti-mo: @lmb Needs a bit of polish, but swapped this out for a much simpler design instead of the pub-sub thing. sys.FD no longer has a metadata pointer, all metadata is memoized in a package global.

We keep merging leaks, so we should at least consider running this internally. For working on Cilium itself, I can keep a local branch with some setters for `sys.{Create,Close,Finalize}`.
ti-mo: Re-spun this a bit. We've concluded in offline discussion that this kind of API is best kept internal for now. Cilium can do its own resource tracking for higher-level objects.

In the last version, I've:
- merged metadata and FDTrace
- protected accesses to fdmap and made it accessible through methods on `sys.FDs`
- changed the default frame count to 32
- removed all hooks except for `sys.Finalize`

Looking for more ideas for simplification. We need an fd registry to be able to query open FDs at the end of a test run, since any reliance on GC for this purpose will flake/race. Adding a metadata pointer to sys.FD is not enough and incurs memory overhead on all users, however limited it may be.

Marking this as ready to review.

Patch:
diff --git a/internal/sys/fd.go b/internal/sys/fd.go
index 65517d4..f5ae7b0 100644
--- a/internal/sys/fd.go
+++ b/internal/sys/fd.go
@@ -17,11 +17,33 @@ type FD struct {
 }
 
 func newFD(value int) *FD {
+	if onLeakFD != nil {
+		fds.Store(value, callersFrames())
+	}
+
 	fd := &FD{value}
-	runtime.SetFinalizer(fd, (*FD).Close)
+	runtime.SetFinalizer(fd, (*FD).finalize)
 	return fd
 }
 
+// finalize is set as the FD's runtime finalizer and
+// sends a leak trace before calling FD.Close().
+func (fd *FD) finalize() {
+	if fd.raw < 0 {
+		return
+	}
+
+	// Invoke the fd leak callback. Calls LoadAndDelete to guarantee the callback
+	// is invoked at most once for one sys.FD allocation, runtime.Frames can only
+	// be unwound once.
+	f, ok := fds.LoadAndDelete(fd.Int())
+	if ok && onLeakFD != nil {
+		onLeakFD(f.(*runtime.Frames))
+	}
+
+	_ = fd.Close()
+}
+
 // NewFD wraps a raw fd with a finalizer.
 //
 // You must not use the raw fd after calling this function, since the underlying
@@ -67,6 +89,8 @@ func (fd *FD) Close() error {
 	value := int(fd.raw)
 	fd.raw = -1
 
+	fds.Delete(value)
+
 	fd.Forget()
 	return unix.Close(value)
 }
diff --git a/internal/sys/fd_trace.go b/internal/sys/fd_trace.go
new file mode 100644
index 0000000..ed2f32c
--- /dev/null
+++ b/internal/sys/fd_trace.go
@@ -0,0 +1,78 @@
+package sys
+
+import (
+	"runtime"
+	"sync"
+)
+
+// OnLeakFD controls tracing [FD] lifetime to detect resources that are not
+// closed by Close().
+//
+// If fn is not nil, tracing is enabled for all FDs created going forward. fn is
+// invoked for all FDs that are closed by the garbage collector instead of an
+// explicit Close() by a caller. Calling OnLeakFD twice with a non-nil fn
+// (without disabling tracing in the meantime) will cause a panic.
+//
+// If fn is nil, tracing will be disabled. Any FDs that have not been closed are
+// considered to be leaked, fn will be invoked for them, and the process will be
+// terminated.
+//
+// fn will be invoked at most once for every unique sys.FD allocation since a
+// runtime.Frames can only be unwound once.
+func OnLeakFD(fn func(*runtime.Frames)) {
+	// Enable leak tracing if new fn is provided.
+	if fn != nil {
+		if onLeakFD != nil {
+			panic("OnLeakFD called twice with non-nil fn")
+		}
+
+		onLeakFD = fn
+		return
+	}
+
+	// fn is nil past this point.
+
+	if onLeakFD == nil {
+		return
+	}
+
+	// Call onLeakFD for all open fds.
+	if fs := flushFrames(); len(fs) != 0 {
+		for _, f := range fs {
+			onLeakFD(f)
+		}
+	}
+
+	onLeakFD = nil
+}
+
+var onLeakFD func(*runtime.Frames)
+
+// fds is a registry of all file descriptors wrapped into sys.fds that were
+// created while an fd tracer was active.
+var fds sync.Map // map[int]*runtime.Frames
+
+// flushFrames removes all elements from fds and returns them as a slice. This
+// deals with the fact that a runtime.Frames can only be unwound once using
+// Next().
+func flushFrames() []*runtime.Frames {
+	var frames []*runtime.Frames
+	fds.Range(func(key, value any) bool {
+		frames = append(frames, value.(*runtime.Frames))
+		fds.Delete(key)
+		return true
+	})
+	return frames
+}
+
+func callersFrames() *runtime.Frames {
+	c := make([]uintptr, 32)
+
+	// Skip runtime.Callers and this function.
+	i := runtime.Callers(2, c)
+	if i == 0 {
+		return nil
+	}
+
+	return runtime.CallersFrames(c)
+}
diff --git a/link/cgroup.go b/link/cgroup.go
index bfad1cc..a685fe0 100644
--- a/link/cgroup.go
+++ b/link/cgroup.go
@@ -41,6 +41,13 @@ func AttachCgroup(opts CgroupOptions) (Link, error) {
 
 	var cg Link
 	cg, err = newLinkCgroup(cgroup, opts.Attach, clone)
+	if err == nil {
+		cgroup.Close()
+		clone.Close()
+		return cg, nil
+	}
+
+	// cgroup and clone are retained by progAttachCgroup.
 	if errors.Is(err, ErrNotSupported) {
 		cg, err = newProgAttachCgroup(cgroup, opts.Attach, clone, flagAllowMulti)
 	}
@@ -67,6 +74,8 @@ var _ Link = (*progAttachCgroup)(nil)
 
 func (cg *progAttachCgroup) isLink() {}
 
+// newProgAttachCgroup attaches prog to cgroup using BPF_PROG_ATTACH.
+// cgroup and prog are retained by [progAttachCgroup].
 func newProgAttachCgroup(cgroup *os.File, attach ebpf.AttachType, prog *ebpf.Program, flags cgroupAttachFlags) (*progAttachCgroup, error) {
 	if flags&flagAllowMulti > 0 {
 		if err := haveProgAttachReplace(); err != nil {
@@ -151,6 +160,7 @@ type linkCgroup struct {
 
 var _ Link = (*linkCgroup)(nil)
 
+// newLinkCgroup attaches prog to cgroup using BPF_LINK_CREATE.
 func newLinkCgroup(cgroup *os.File, attach ebpf.AttachType, prog *ebpf.Program) (*linkCgroup, error) {
 	link, err := AttachRawLink(RawLinkOptions{
 		Target:  int(cgroup.Fd()),


Test patch:
diff --git a/collection_test.go b/collection_test.go
index a0a179c..ccd67c1 100644
--- a/collection_test.go
+++ b/collection_test.go
@@ -10,9 +10,14 @@ import (
 	"github.com/cilium/ebpf/btf"
 	"github.com/cilium/ebpf/internal"
 	"github.com/cilium/ebpf/internal/testutils"
+	"github.com/cilium/ebpf/internal/testutils/fdtrace"
 	qt "github.com/frankban/quicktest"
 )
 
+func TestMain(m *testing.M) {
+	fdtrace.TestMain(m)
+}
+
 func TestCollectionSpecNotModified(t *testing.T) {
 	cs := CollectionSpec{
 		Maps: map[string]*MapSpec{
@@ -586,6 +591,10 @@ func TestIncompleteLoadAndAssign(t *testing.T) {
 		t.Fatal("expected error loading invalid ProgramSpec")
 	}
 
+	if s.Valid == nil {
+		t.Fatal("expected valid prog to be non-nil")
+	}
+
 	if fd := s.Valid.FD(); fd != -1 {
 		t.Fatal("expected valid prog to have closed fd -1, got:", fd)
 	}
diff --git a/elf_reader_test.go b/elf_reader_test.go
index 8399f51..008e35f 100644
--- a/elf_reader_test.go
+++ b/elf_reader_test.go
@@ -479,6 +479,7 @@ func TestStringSection(t *testing.T) {
 	if err != nil {
 		t.Fatalf("new collection: %s", err)
 	}
+	defer coll.Close()
 
 	prog := coll.Programs["filter"]
 	if prog == nil {
diff --git a/features/prog_test.go b/features/prog_test.go
index c512c37..54cd44a 100644
--- a/features/prog_test.go
+++ b/features/prog_test.go
@@ -10,8 +10,13 @@ import (
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/internal"
 	"github.com/cilium/ebpf/internal/testutils"
+	"github.com/cilium/ebpf/internal/testutils/fdtrace"
 )
 
+func TestMain(m *testing.M) {
+	fdtrace.TestMain(m)
+}
+
 func TestHaveProgramType(t *testing.T) {
 	testutils.CheckFeatureMatrix(t, haveProgramTypeMatrix)
 }
diff --git a/internal/feature_test.go b/internal/feature_test.go
index 4039256..f477819 100644
--- a/internal/feature_test.go
+++ b/internal/feature_test.go
@@ -4,8 +4,14 @@ import (
 	"errors"
 	"strings"
 	"testing"
+
+	"github.com/cilium/ebpf/internal/testutils/fdtrace"
 )
 
+func TestMain(m *testing.M) {
+	fdtrace.TestMain(m)
+}
+
 func TestFeatureTest(t *testing.T) {
 	var called bool
 
diff --git a/internal/testutils/fdtrace/fd.go b/internal/testutils/fdtrace/fd.go
new file mode 100644
index 0000000..612d32d
--- /dev/null
+++ b/internal/testutils/fdtrace/fd.go
@@ -0,0 +1,47 @@
+package fdtrace
+
+import (
+	"bytes"
+	"fmt"
+	"os"
+	"runtime"
+	"testing"
+
+	"github.com/cilium/ebpf/internal/sys"
+)
+
+// TestMain runs m with sys.FD leak tracing enabled.
+func TestMain(m *testing.M) {
+	// fn can either be invoked asynchronously by the gc or during disabling of
+	// the leak tracer below. Don't terminate the program immediately, instead
+	// capture a boolean that will be used to set the exit code. This avoids races
+	// and gives all events the chance to be written to stderr.
+	var leak bool
+	sys.OnLeakFD(func(fs *runtime.Frames) {
+		fmt.Fprintln(os.Stderr, "leaked fd created at:")
+		fmt.Fprintln(os.Stderr, formatFrames(fs))
+		leak = true
+	})
+
+	ret := m.Run()
+
+	sys.OnLeakFD(nil)
+
+	if leak {
+		ret = 99
+	}
+
+	os.Exit(ret)
+}
+
+func formatFrames(fs *runtime.Frames) string {
+	var b bytes.Buffer
+	for {
+		f, more := fs.Next()
+		b.WriteString(fmt.Sprintf("\t%s+%#x\n\t\t%s:%d\n", f.Function, f.PC-f.Entry, f.File, f.Line))
+		if !more {
+			break
+		}
+	}
+	return b.String()
+}
diff --git a/link/cgroup_test.go b/link/cgroup_test.go
index d18ec9c..7bcbde5 100644
--- a/link/cgroup_test.go
+++ b/link/cgroup_test.go
@@ -19,6 +19,7 @@ func TestAttachCgroup(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	defer link.Close()
 
 	if haveBPFLink() == nil {
 		if _, ok := link.(*linkCgroup); !ok {
diff --git a/link/kprobe_multi_test.go b/link/kprobe_multi_test.go
index e6b64bc..aceb347 100644
--- a/link/kprobe_multi_test.go
+++ b/link/kprobe_multi_test.go
@@ -80,12 +80,14 @@ func TestKprobeMultiCookie(t *testing.T) {
 
 	prog := mustLoadProgram(t, ebpf.Kprobe, ebpf.AttachTraceKprobeMulti, "")
 
-	if _, err := KprobeMulti(prog, KprobeMultiOptions{
+	km, err := KprobeMulti(prog, KprobeMultiOptions{
 		Symbols: kprobeMultiSyms,
 		Cookies: []uint64{0, 1},
-	}); err != nil {
+	})
+	if err != nil {
 		t.Fatal(err)
 	}
+	_ = km.Close()
 }
 
 func TestKprobeMultiProgramCall(t *testing.T) {
diff --git a/link/link_test.go b/link/link_test.go
index a00156d..f1d600e 100644
--- a/link/link_test.go
+++ b/link/link_test.go
@@ -12,11 +12,16 @@ import (
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/internal/sys"
 	"github.com/cilium/ebpf/internal/testutils"
+	"github.com/cilium/ebpf/internal/testutils/fdtrace"
 	"github.com/cilium/ebpf/internal/unix"
 
 	qt "github.com/frankban/quicktest"
 )
 
+func TestMain(m *testing.M) {
+	fdtrace.TestMain(m)
+}
+
 func TestRawLink(t *testing.T) {
 	cgroup, prog := mustCgroupFixtures(t)
 
@@ -55,11 +60,11 @@ func TestRawLink(t *testing.T) {
 func TestUnpinRawLink(t *testing.T) {
 	cgroup, prog := mustCgroupFixtures(t)
 	link, _ := newPinnedRawLink(t, cgroup, prog)
+	defer link.Close()
 
 	qt.Assert(t, link.IsPinned(), qt.IsTrue)
 
-	err := link.Unpin()
-	if err != nil {
+	if err := link.Unpin(); err != nil {
 		t.Fatal(err)
 	}
 
@@ -69,6 +74,7 @@ func TestUnpinRawLink(t *testing.T) {
 func TestRawLinkLoadPinnedWithOptions(t *testing.T) {
 	cgroup, prog := mustCgroupFixtures(t)
 	link, path := newPinnedRawLink(t, cgroup, prog)
+	defer link.Close()
 
 	qt.Assert(t, link.IsPinned(), qt.IsTrue)
 
diff --git a/map_test.go b/map_test.go
index a655a45..d283aaf 100644
--- a/map_test.go
+++ b/map_test.go
@@ -31,6 +31,21 @@ var (
 	}
 )
 
+// newHash returns a new Map of type Hash. Cleanup is handled automatically.
+func newHash(t *testing.T) *Map {
+	hash, err := NewMap(&MapSpec{
+		Type:       Hash,
+		KeySize:    5,
+		ValueSize:  4,
+		MaxEntries: 10,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { hash.Close() })
+	return hash
+}
+
 func TestMap(t *testing.T) {
 	m := createArray(t)
 	defer m.Close()
@@ -1214,8 +1229,7 @@ func TestMapGuessNonExistentKey(t *testing.T) {
 }
 
 func TestNotExist(t *testing.T) {
-	hash := createHash()
-	defer hash.Close()
+	hash := newHash(t)
 
 	var tmp uint32
 	err := hash.Lookup("hello", &tmp)
@@ -1246,8 +1260,7 @@ func TestNotExist(t *testing.T) {
 }
 
 func TestExist(t *testing.T) {
-	hash := createHash()
-	defer hash.Close()
+	hash := newHash(t)
 
 	if err := hash.Put("hello", uint32(21)); err != nil {
 		t.Errorf("Failed to put key/value pair into hash: %v", err)
@@ -1611,8 +1624,8 @@ func TestMapGetNextID(t *testing.T) {
 	var next MapID
 	var err error
 
-	hash := createHash()
-	defer hash.Close()
+	// Ensure there is at least one map on the system.
+	_ = newHash(t)
 
 	if next, err = MapGetNextID(MapID(0)); err != nil {
 		t.Fatal("Can't get next ID:", err)
@@ -1638,8 +1651,7 @@ func TestMapGetNextID(t *testing.T) {
 }
 
 func TestNewMapFromID(t *testing.T) {
-	hash := createHash()
-	defer hash.Close()
+	hash := newHash(t)
 
 	info, err := hash.Info()
 	testutils.SkipIfNotSupported(t, err)
@@ -1985,7 +1997,15 @@ func ExampleMap_perCPU() {
 // Note that using unsafe.Pointer is only marginally faster than
 // implementing Marshaler on the type.
 func ExampleMap_zeroCopy() {
-	hash := createHash()
+	hash, err := NewMap(&MapSpec{
+		Type:       Hash,
+		KeySize:    5,
+		ValueSize:  4,
+		MaxEntries: 10,
+	})
+	if err != nil {
+		panic(err)
+	}
 	defer hash.Close()
 
 	key := [5]byte{'h', 'e', 'l', 'l', 'o'}
@@ -2004,7 +2024,7 @@ func ExampleMap_zeroCopy() {
 	// Output: The value is: 23
 }
 
-func createHash() *Map {
+func ExampleMap_NextKey() {
 	hash, err := NewMap(&MapSpec{
 		Type:       Hash,
 		KeySize:    5,
@@ -2014,11 +2034,6 @@ func createHash() *Map {
 	if err != nil {
 		panic(err)
 	}
-	return hash
-}
-
-func ExampleMap_NextKey() {
-	hash := createHash()
 	defer hash.Close()
 
 	if err := hash.Put("hello", uint32(21)); err != nil {
@@ -2045,7 +2060,15 @@ func ExampleMap_NextKey() {
 // ExampleMap_Iterate demonstrates how to iterate over all entries
 // in a map.
 func ExampleMap_Iterate() {
-	hash := createHash()
+	hash, err := NewMap(&MapSpec{
+		Type:       Hash,
+		KeySize:    5,
+		ValueSize:  4,
+		MaxEntries: 10,
+	})
+	if err != nil {
+		panic(err)
+	}
 	defer hash.Close()
 
 	if err := hash.Put("hello", uint32(21)); err != nil {
diff --git a/marshaler_example_test.go b/marshaler_example_test.go
index 335b0c7..6228ac8 100644
--- a/marshaler_example_test.go
+++ b/marshaler_example_test.go
@@ -27,7 +27,15 @@ func (ce *customEncoding) UnmarshalBinary(buf []byte) error {
 
 // ExampleMarshaler shows how to use custom encoding with map methods.
 func Example_customMarshaler() {
-	hash := createHash()
+	hash, err := NewMap(&MapSpec{
+		Type:       Hash,
+		KeySize:    5,
+		ValueSize:  4,
+		MaxEntries: 10,
+	})
+	if err != nil {
+		panic(err)
+	}
 	defer hash.Close()
 
 	if err := hash.Put(&customEncoding{"hello"}, uint32(111)); err != nil {
diff --git a/perf/reader_test.go b/perf/reader_test.go
index f419551..b412cde 100644
--- a/perf/reader_test.go
+++ b/perf/reader_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/internal"
 	"github.com/cilium/ebpf/internal/testutils"
+	"github.com/cilium/ebpf/internal/testutils/fdtrace"
 	"github.com/cilium/ebpf/internal/unix"
 
 	qt "github.com/frankban/quicktest"
@@ -23,6 +24,10 @@ var (
 	readTimeout = 250 * time.Millisecond
 )
 
+func TestMain(m *testing.M) {
+	fdtrace.TestMain(m)
+}
+
 func TestPerfReader(t *testing.T) {
 	prog, events := mustOutputSamplesProg(t, 5)
 
diff --git a/prog_test.go b/prog_test.go
index 31466df..0d951f7 100644
--- a/prog_test.go
+++ b/prog_test.go
@@ -441,7 +441,7 @@ func TestProgramVerifierLogTruncated(t *testing.T) {
 		}
 		var ve *internal.VerifierError
 		if !errors.As(err, &ve) {
-			t.Error("Error is not a VerifierError")
+			t.Fatal("Error is not a VerifierError")
 		}
 		if !ve.Truncated {
 			t.Errorf("VerifierError is not truncated: %+v", ve)
diff --git a/ringbuf/reader_test.go b/ringbuf/reader_test.go
index d6633de..964bd28 100644
--- a/ringbuf/reader_test.go
+++ b/ringbuf/reader_test.go
@@ -11,9 +11,14 @@ import (
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/internal"
 	"github.com/cilium/ebpf/internal/testutils"
+	"github.com/cilium/ebpf/internal/testutils/fdtrace"
 	"github.com/google/go-cmp/cmp"
 )
 
+func TestMain(m *testing.M) {
+	fdtrace.TestMain(m)
+}
+
 func TestRingbufReader(t *testing.T) {
 	testutils.SkipOnOldKernel(t, "5.8", "BPF ring buffer")