Repository: cilium/ebpf

Issue:
pin: WalkDir makes it too easy to leak objects
> It's very easy to accidentally leak objects when using WalkDir, and kind of awkward to do the right thing (because obj can be nil?) This is why https://pkg.go.dev/github.com/cilium/ebpf/btf#HandleIterator.Take exists for handles. It might be good to do another pass over this API.

_Originally posted by @lmb in https://github.com/cilium/ebpf/issues/1651#issuecomment-2605231732_
            
More context from Mahe:
>Mmmh I'm slightly confused now, this it the typical use we have in Tetragon:
>
>    https://github.com/cilium/tetragon/blob/d72f98aefb8e88c06caec7cfbc687bbeabf97a5f/cmd/tetra/debug/progs.go#L408-L445
>    https://github.com/cilium/tetragon/blob/d72f98aefb8e88c06caec7cfbc687bbeabf97a5f/pkg/bugtool/maps.go#L148-L176
>
> Maybe having an example in the doc would make thing more clear?

As far as  I can tell both of these examples leak objects. Not a big deal in these cases since the processes are short lived (I think?) but an indication that the API is bug prone. My gut feeling is that this is because of wrapping `WalkDir` which in itself is a really clunky API.

Ideas:
1. Drop Walk outright and have callers do the call to Load themselves. That makes it clearer that they are reponsible for cleaning up.
2. Only invoke Walk for valid objects so that an unconditional `defer obj.Close()` at the top of the function works.

Since 1.24 is around the corner it might also be interesting to consider what it would look like to turn this API into an iterator.

Hints from issue comments:
@ti-mo: As far as I can tell, Go iterators are also callback-based and would require something similar to `HandleIterator.Take` if we decide to always Close() under the covers.

> Drop Walk outright and have callers do the call to Load themselves. That makes it clearer that they are reponsible for cleaning up.

The API aims to unburden the caller from this exact pain. The library already does the (runtime) work of opening the pin and determining its type. Closing it and requiring the caller to repeat the same work feels very inefficient; doubly so if these are short-lived objects.

I think this is a documentation issue; this isn't much different from any other omission of `Close()`. I'm of course open to other API forms if there are better suggestions, but iirc Go iterators lack the feedback mechanism that WalkDir provides, e.g. skipping directories to avoid unnecessary tree visits.

Haven't really heard about people taking issue with `filepath.WalkDir()`, I think it's just okay personally.

I think a `pin.Iterator` would be ideal, that would give us the most flexibility.

> Only invoke Walk for valid objects so that an unconditional defer obj.Close() at the top of the function works.

Not sure what you mean by invalid objects, but there's a stat() call at the beginning that makes sure it's called on a bpffs instance. All nodes visited by WalkDir are valid bpf objects, currently meaning pin, prog or map. btf can't be pinned (yet).

Solution patch:
diff --git a/pin/load.go b/pin/load.go
index 817e177..b3d2fe7 100644
--- a/pin/load.go
+++ b/pin/load.go
@@ -2,6 +2,7 @@ package pin
 
 import (
 	"fmt"
+	"io"
 
 	"github.com/cilium/ebpf"
 	"github.com/cilium/ebpf/internal/sys"
@@ -11,6 +12,7 @@ import (
 // Pinner is an interface implemented by all eBPF objects that support pinning
 // to a bpf virtual filesystem.
 type Pinner interface {
+	io.Closer
 	Pin(string) error
 }
 



Test patch:
diff --git a/pin/load_test.go b/pin/load_test.go
index a16b794..742355c 100644
--- a/pin/load_test.go
+++ b/pin/load_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/cilium/ebpf"
 	"github.com/cilium/ebpf/asm"
 	"github.com/cilium/ebpf/internal/testutils"
+	"github.com/cilium/ebpf/internal/testutils/fdtrace"
 )
 
 func mustPinnedProgram(t *testing.T, path string) *ebpf.Program {
@@ -77,9 +78,15 @@ func TestLoad(t *testing.T) {
 
 	m, err := Load(mpath, nil)
 	qt.Assert(t, qt.IsNil(err))
+	defer m.Close()
 	qt.Assert(t, qt.Satisfies(m, testutils.Contains[*ebpf.Map]))
 
 	p, err := Load(ppath, nil)
 	qt.Assert(t, qt.IsNil(err))
+	defer p.Close()
 	qt.Assert(t, qt.Satisfies(p, testutils.Contains[*ebpf.Program]))
 }
+
+func TestMain(m *testing.M) {
+	fdtrace.TestMain(m)
+}
diff --git a/pin/walk_test.go b/pin/walk_test.go
index bde193e..13bf164 100644
--- a/pin/walk_test.go
+++ b/pin/walk_test.go
@@ -27,6 +27,10 @@ func TestWalkDir(t *testing.T) {
 	bpffn := func(path string, d fs.DirEntry, obj Pinner, err error) error {
 		qt.Assert(t, qt.IsNil(err))
 
+		if obj != nil {
+			defer obj.Close()
+		}
+
 		if path == "." {
 			return nil
 		}