pin: WalkDir makes it too easy to leak objects

[lmb]

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 #1651 (comment)

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.

[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).