Map file descriptor is closed after RewriteMaps

[mauriciovasquezbernal]

Describe the bug

The file descriptor of an eBPF map uses in RewriteMaps is closed before loading the program, causing a failure when loading the program.

To Reproduce

I prepared a repository with the reproducer of this problem -> https://github.com/mauriciovasquezbernal/ebpfissue.

$ git clone https://github.com/mauriciovasquezbernal/ebpfissue
...
$ cd ebpfissue
$ make 
...
$ sudo bpftool map create /sys/fs/bpf/mount_ns_set type hash key 8 value 4 entries 1024 name mount_ns_set
...
$ sudo ./foo
2021/12/02 18:34:34 loading objects: field VfsReadEntry: program vfs_read_entry: load program: invalid argument: fd 3 is not pointing to valid bpf_map
processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 

I see the problem is that the file descriptor of the map is being closed before loading the program:

$ sudo strace -f -e signal=SIGINT -e trace=close,bpf ./foo
close(3)                                = 0
strace: Process 466748 attached
strace: Process 466749 attached
strace: Process 466750 attached
strace: Process 466751 attached
[pid 466747] bpf(BPF_MAP_CREATE, {map_type=BPF_MAP_TYPE_ARRAY, key_size=4, value_size=4, max_entries=1, map_flags=0, inner_map_fd=0, map_name="", map_ifindex=0, btf_fd=0, btf_key_type_id=0, btf_value_type_id=0}, 60) = 3
[pid 466747] close(3)                   = 0
[pid 466747] bpf(BPF_MAP_CREATE, {map_type=BPF_MAP_TYPE_ARRAY, key_size=4, value_size=4, max_entries=1, map_flags=0, inner_map_fd=0, map_name="feature_test", map_ifindex=0, btf_fd=0, btf_key_type_id=0, btf_value_type_id=0}, 60) = 7
[pid 466747] close(7)                   = 0
[pid 466747] bpf(BPF_MAP_CREATE, {map_type=BPF_MAP_TYPE_ARRAY, key_size=4, value_size=4, max_entries=1, map_flags=0, inner_map_fd=0, map_name=".test", map_ifindex=0, btf_fd=0, btf_key_type_id=0, btf_value_type_id=0}, 60) = 7
[pid 466747] close(7)                   = 0
[pid 466747] close(3)                   = 0

---> fd 3 is returned here

[pid 466747] bpf(BPF_OBJ_GET, {pathname="/sys/fs/bpf/mount_ns_set", bpf_fd=0, file_flags=0}, 16) = 3
[pid 466747] bpf(BPF_OBJ_GET_INFO_BY_FD, {info={bpf_fd=3, info_len=80, info=0xc00001c320}}, 16) = 0
[pid 466747] close(7)                   = 0
strace: Process 466752 attached
strace: Process 466753 attached

---> but it's closed here before BPF_PROG_LOAD!!!

[pid 466752] close(3)                   = 0
strace: Process 466754 attached
strace: Process 466755 attached
strace: Process 466756 attached
[pid 466755] close(7)                   = 0
[pid 466755] bpf(BPF_BTF_LOAD, {btf="\237\353\1\0\30\0\0\0\0\0\0\0\34\0\0\0\34\0\0\0\3\0\0\0\0\0\0\0\0\0\0\2"..., btf_log_buf=NULL, btf_size=55, btf_log_size=0, btf_log_level=0}, 32) = 3
[pid 466755] close(3)                   = 0
[pid 466755] bpf(BPF_BTF_LOAD, {btf="\237\353\1\0\30\0\0\0\0\0\0\0\30\0\0\0\30\0\0\0\3\0\0\0\0\0\0\0\0\0\0\r"..., btf_log_buf=NULL, btf_size=51, btf_log_size=0, btf_log_level=0}, 32) = 3
[pid 466755] close(3)                   = 0
[pid 466755] bpf(BPF_BTF_LOAD, {btf="\237\353\1\0\30\0\0\0\0\0\0\0\244D\0\0\244D\0\0\373-\0\0\0\0\0\0\0\0\0\2"..., btf_log_buf=NULL, btf_size=29367, btf_log_size=0, btf_log_level=0}, 32) = 3
[pid 466755] bpf(BPF_PROG_LOAD, {prog_type=BPF_PROG_TYPE_KPROBE, insn_cnt=6, insns=0xc001011590, license="GPL", log_level=0, log_size=0, log_buf=NULL, kern_version=KERNEL_VERSION(0, 0, 0), prog_flags=0, prog_name="", prog_ifindex=0, expected_attach_type=BPF_CGROUP_INET_INGRESS, prog_btf_fd=0, func_info_rec_size=0, func_info=NULL, func_info_cnt=0, line_info_rec_size=0, line_info=NULL, line_info_cnt=0, attach_btf_id=0, attach_prog_fd=0}, 120) = 7
[pid 466755] close(7)                   = 0
[pid 466755] bpf(BPF_PROG_LOAD, {prog_type=BPF_PROG_TYPE_KPROBE, insn_cnt=33, insns=0xc000c90240, license="Dual BSD/GPL", log_level=0, log_size=0, log_buf=NULL, kern_version=KERNEL_VERSION(5, 11, 22), prog_flags=0, prog_name="vfs_read_entry", prog_ifindex=0, expected_attach_type=BPF_CGROUP_INET_INGRESS, prog_btf_fd=3, func_info_rec_size=8, func_info=0xc0026e2a78, func_info_cnt=1, line_info_rec_size=16, line_info=0xc000ae8540, line_info_cnt=12, attach_btf_id=0, attach_prog_fd=0}, 120) = -1 EINVAL (Invalid argument)
[pid 466755] bpf(BPF_PROG_LOAD, {prog_type=BPF_PROG_TYPE_KPROBE, insn_cnt=33, insns=0xc000c90240, license="Dual BSD/GPL", log_level=1, log_size=65536, log_buf="", kern_version=KERNEL_VERSION(5, 11, 22), prog_flags=0, prog_name="vfs_read_entry", prog_ifindex=0, expected_attach_type=BPF_CGROUP_INET_INGRESS, prog_btf_fd=3, func_info_rec_size=8, func_info=0xc0026e2a78, func_info_cnt=1, line_info_rec_size=16, line_info=0xc000ae8540, line_info_cnt=12, attach_btf_id=0, attach_prog_fd=0}, 120) = -1 EINVAL (Invalid argument)
[pid 466755] close(3)                   = 0
[pid 466755] close(3)                   = 0
2021/12/02 18:34:44 loading objects: field VfsReadEntry: program vfs_read_entry: load program: invalid argument: fd 3 is not pointing to valid bpf_map
processed 0 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0
[pid 466756] +++ exited with 1 +++
[pid 466755] +++ exited with 1 +++
[pid 466752] +++ exited with 1 +++
[pid 466754] +++ exited with 1 +++
[pid 466749] +++ exited with 1 +++
[pid 466753] +++ exited with 1 +++
[pid 466748] +++ exited with 1 +++
[pid 466751] +++ exited with 1 +++
[pid 466750] +++ exited with 1 +++
+++ exited with 1 +++

It's somehow related to the garbage collection, if I comment out the following line it works fine:

ebpf/internal/sys/fd.go

Line 21 in f40ce35

runtime.SetFinalizer(fd, (*FD).Close)

Two additional details:

1. It only happens (in this case) when there is a core relocation. (commenting out https://github.com/mauriciovasquezbernal/ebpfissue/blob/05a7074afb97b833836bc02e7c0e8df79d54a888/bpf/foo.bpf.c#L9 makes it work)
2. Doesn't happen when the map is created instead of loaded from a pinned one (https://github.com/mauriciovasquezbernal/ebpfissue/blob/05a7074afb97b833836bc02e7c0e8df79d54a888/foo.go#L33)

My environment is:

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

I hope I gave enough information to track this down.

/cc @alban

[lmb]

You're right, this is most likely the GC freeing the map since it's not referenced by the Go program anymore. Thanks for providing a reproducer, I'll take a look.

[lmb]

Here is a snippet from your reproducer:

	spec.RewriteMaps(map[string]*ebpf.Map{
		"mount_ns_set": mount_ns_set,
	})

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

1. After RewriteMaps(), the bytecode of spec.Program["vfs_read_entry"] uses fd 3
2. However, there is no further reference to mount_ns_set in the program after this, so the GC may free the object.
3. LoadAndAssign invokes the CO-RE logic, which allocates memory and triggers a GC run, which frees mount_ns_set and invoked the finalizer which does a close().
4. Loading the byte code into the kernel fails since fd 3 has been closed.

You can observe a similar behaviour by removing the CO-RE relocation and inserting runtime.GC() between RewriteMaps and LoadAndAssign. As a workaround you can call runtime.KeepAlive(mount_ns_set) after LoadAndAssign which will keep the map alive for long enough for the load to succeed.

This is a problem in the API, since it doesn't enforce that the map is kept alive until the map isn't necessary anymore. Fixing this might be complicated however. The most "correct" solution would be to add a pointer to asm.Instruction which references the map, but this adds a big overhead to every single instruction. Not sure what to do here yet.

[ti-mo]

I think RewriteMaps() might simply be the wrong abstraction as it allows splicing existing kernel resources into a spec type.

What about deprecating it (or rather, making it unexported) in favor of passing a map[string]*ebpf.Map using a ProgramOptions? This is already plumbed into LoadAndAssign and friends using CollectionOptions.

[lmb]

That might also work. I think the API came about due to xdpcap which has unusual requirements.

@arthurfabre could you make xdcap work with an API like this? An alternative would be to add xdpcap.FromMap() which doesn't deal with any of the pinning stuff. Arguably the latter is more appropriate now that the library handles pinning better.

P.S.: I think the API would have to be CollectionOptions.ReplacementMaps or something. Judging from #517 users will expect that the replacements to feed into LoadAndAssign as well?