LoadAndAssign fails after RewriteMaps

[mauriciovasquezbernal]

Describe the bug

LoadAndAssign fails when RewriteMaps and bpf2go are used with a missing map error.

To Reproduce
There is a reproducer available at https://github.com/mauriciovasquezbernal/ebpfissue/tree/1bfab92fea1c988c032d6b5715fdfee9ed80c1ce

$ go version
go version go1.16.3 linux/amd64
$ uname -r
5.11.0-41-generic
$ clang --version
Ubuntu clang version 12.0.0-3ubuntu1~20.04.4
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

Expected behavior
The object should load fine and IMO the rewritten map should be assigned to fooMaps.MountNsSet

Note: I added a call to runtime.KeepAlive to avoid hitting #515.

[lmb]

I can see why you'd like behaviour like this, but it's hard to make RewriteMaps work the way you want it to. We'd need a new API as pointed out by timo #515 (comment)

FWIW, you can just set pinning on mount_ns_net and have the library handle pinning for you (example here). Why is that not an option here?

[mauriciovasquezbernal]

FWIW, you can just set pinning on mount_ns_net and have the library handle pinning for you (example here). Why is that not an option here?

I ended up doing exactly that. My only reason to prefer a RewriteMaps like approach is that I don't have to pin the map if it's not already pinned. (If I'm creating the map or if I already have a reference to it).

I'm fine with always using the pinning, but I think RewriteMaps should also work in this case.

[lmb]

Hmm. Why does pinning / no pinning change dynamically for you? I don't fully understand the use case, sorry.

I'm fine with always using the pinning

You shouldn't have to if you don't care about persisting the state. You're saying you want to persist without pinning?

[mauriciovasquezbernal]

Hmm. Why does pinning / no pinning change dynamically for you? I don't fully understand the use case, sorry.

I have a golang application that creates and pins a given BPF map. I'm developing an extension to that application that loads an eBPF program that should use that map, so I have two options: (1) pass the map directly as a golang pointer (*ebpf.Map) and use RewriteMaps or (2) pass the pinned path (string), set m.Pinning = PinByName and let the library do the work. I tried the first one but hit these two issues, now I'm sticking with the second one.

I'm fine with always using the pinning

I meant, these two issues are not hard blockers for me as I found a workaround using the pinning.

[lmb]

Makes sense, thanks for the context.

[ti-mo]

@mauriciovasquezbernal The changes merged in #567 should address this issue as well as #515. To my best understanding, the root cause is the same: the lifetime of Maps loaded from bpffs pins.

Closing this for now, but please do relay your feedback here if you get the time. Thanks for the help!

[ti-mo]

Note: looking at your reproducer, that one still fails due to an unsupported interaction between RewriteMaps and your use of LoadAndAssign. We're currently unlikely to change this behaviour.

As documented on RewriteMaps, it removes any maps it rewrites from the CollectionSpec to defend against accidentally creating new instances of them, as they won't end up being actually used by the programs.

I see two options:

• Avoid LoadAndAssigning fooMapSpecs, as mount_ns_set will be gone from the CollectionSpec. Load the map manually from a pin if you want to interact with it from userspace.
• Set the map's pinning flag (either in the ELF or in Go in your 'extension' app) and run with MapOptions.PinPath. This way, you won't have to deal with the plumbing as you've mentioned earlier.

[mauriciovasquezbernal]

We're currently unlikely to change this behaviour.

I'm wondering what the reason not to change it. I think it's straightforward to avoid creating a new map in RewriteMaps but at the same time assigning it when using LoadAndAssign. Could something like inspektor-gadget@c0a840d work? Is it missing important cases?

Set the map's pinning flag (either in the ELF or in Go in your 'extension' app) and run with MapOptions.PinPath. This way, you won't have to deal with the plumbing as you've mentioned earlier.

I'll stick to this solution for now.

[d0u9]

Hi @mauriciovasquezbernal , I just met this problem and I figured out that it is due to the misuse of the object type.

The object you used here has a type of fooObjects, in which both programs and maps are included:

https://github.com/mauriciovasquezbernal/ebpfissue/blob/1bfab92fea1c988c032d6b5715fdfee9ed80c1ce/foo.go#L26

https://github.com/mauriciovasquezbernal/ebpfissue/blob/1bfab92fea1c988c032d6b5715fdfee9ed80c1ce/foo_bpfel.go#L69-L72

However, the CollectionSpec is rewritten by .RewriteMaps() and from which the mount_ns_set is removed. This is the reason for the problem that I have encountered.

To fix it, don't use fooObjects. Instead, use fooPrograms as the object. In other words, we only load programs and ignore maps.

var objs fooPrograms
if err := spec.LoadAndAssign(&objs, nil); err != nil {
	log.Fatalf("loading objects: %v", err)
}

The downside is that we have to load maps manually one by one to eliminate the pinned one which we have loaded before.

[ti-mo]

@mauriciovasquezbernal Your suggestion would of course be the straightforward way to fix it, though adding a *Map to a MapSpec introduces more complexity than I'm comfortable with. IMO we should consider passing a map[string]*Map as part of MapOptions.

• It adds another point where 'live' kernel resources get spliced into Specs. Already happens in the current incarnation and introduces fun bugs like Map file descriptor is closed after RewriteMaps #515.
• Also an issue in the current incarnation: the lifetime of a MapSpec is potentially longer than a Map. If the caller uses RewriteMaps() and then Map.Close(), the underlying fd gets closed and could potentially get re-used for a (theoretically) compatible map. This would cause bpf progs to operate on a wrong map.
• Spec resources can (and do) get copied, so copying m should at least be included in MapSpec.Copy()
• It allows for inconsistency between what's in m and the MapSpec itself. At least MapSpec.checkCompatibility(m) should be called in RewriteMaps() (this is actually something we can add to avoid surprises). The caller can still modify MapSpec afterwards, which might invalidate some assumptions we made elsewhere in the code.

I would consider the MapOptions idea after all. @lmb WDYT?

[mauriciovasquezbernal]

I'm investigating this again because it turns out that in systemd < v238 bpffs is not mounted by default, hence passing a pinned map doesn't work. Having a way to replace a map and using LoadAndAssign() would be nice for us.

IMO we should consider passing a map[string]*Map as part of MapOptions.

I did a quick try at inspektor-gadget@f46bf3e and it works fine, if you agree I could open a PR with that change.

[lmb]

I just did a refresh of why RewriteMaps removes rewritten maps from the collection spec: if we don't do it, NewCollection will create new maps of the same name and return them in Collection. This is potentially confusing to users.

Regarding inspektor-gadget@f46bf3e: it's on the right track, but I think MapReplacements should be in CollectionOptions. We can then use CollectionOptions.MapReplacements to prime collectionLoader.maps in newCollectionLoader. When priming the collectionLoader.maps we should call MapSpec.checkCompatibility on replacements, and return an error if a replacement is given for a non-existant map.

Note that this solution would never pin a rewritten map, since we completely bypass that logic in newMapWithOptions. Probably ok.

Given this we could consider deprecating CollectionSpec.RewriteMaps I think.

[d0u9]

I did a quick try at kinvolk@f46bf3e and it works fine, if you agree I could open a PR with that change.

I don't agree with this change. I think the fields in MapOptions should not depend on any other Maps from the view of code abstraction. The options are used by the loader to make decisions on how to handle Map loading. They are the properties of maps, not pointers to other maps.

In my view, the best location for replacing a map with another is at loading time. All the ingredients are well prepared and can be used by the loader directly to replace FDs in BPF instructions.

[mauriciovasquezbernal]

I don't agree with this change. I think the fields in MapOptions should not depend on any other Maps from the view of code abstraction. The options are used by the loader to make decisions on how to handle Map loading. They are the properties of maps, not pointers to other maps.

I think having this field on CollectionOptions (as suggested by @lmb above) would solve this issue.

I think MapReplacements should be in CollectionOptions