Declare contents of tail call maps in map declarations

[ti-mo]

Now #24876 is merged, we no longer need to worry about retaining compatibility with iproute2's ELF loader.

Currently, there are 4 remaining prog arrays that use the legacy struct bpf_elf_map:

• POLICY_CALL_MAP
• POLICY_EGRESSCALL_MAP
• CALLS_MAP
• CUSTOM_CALLS_MAP

This issue is for:

• Update these map definitions to use BTF map definitions. Values can be declared using typical C syntax instead: https://github.com/cilium/ebpf/blob/5b305b240a5aeea9d473631ed50b1c4c5d3af76d/testdata/btf_map_init.c#L19-L23.
• Remove the need for CILIUM_CALL_* macros with manually-allocated identifiers. This could be done using a fn->id lookup function that contains a jump table populated using a COUNT macro.
• Update ep_tail_call() to take the above changes into account. Ideally, it would be called using the actual function reference, like ep_tail_call(ctx, tail_handle_ipv6) instead of ep_tail_call(ctx, CILIUM_CALL_IPV6_FROM_NETDEV).
• Remove hooking up tail calls to prog arrays from pkg/bpf.iproute2Compat().
• Remove struct bpf_elf_map.

[lmb]

Your outline doesn't account for gems such as __section_tail(CILIUM_MAP_POLICY, TEMPLATE_HOST_EP_ID) where we rely on changing the HOST_EP_ID to update a different slot for each ELF load. Any idea how to tackle that?

[ti-mo]

One option could be declaring POLICY_CALL_MAP as

struct {
	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
	__type(key, uint32_t);
	__type(value, uint32_t);
	__uint(max_entries, POLICY_PROG_MAP_SIZE);
	__array(values, int());
} POLICY_CALL_MAP __section(".maps") = {
.values =
	{
		[TEMPLATE_HOST_EP_ID] = &handle_lxc_traffic,
	},
}

(the downside of this approach is that we'll need FWDs for all tail calls anyway)

The resulting MapSpec.Contents will have a single MapKV{Key: 0xffff, Value: prog_fd} entry that will be inserted when the map is loaded from a pin during collection load. Each ELF will result in a different .Contents based on the value of TEMPLATE_HOST_EP_ID at compile time.

[Jack-R-lantern]

It's going to take me a long time to fix this, is it okay if I'm assigned to it?

[ti-mo]

Hey @Jack-R-lantern, thanks for the interest! This one is going to be a bit tricky even for committers, but of course you're free to work on it. With the proposal I put forward in the OP, the order of tail calls in the prog arrays might change, so #20691 would be a requirement for the tail call map conversion to land. Would you be willing to pick this up instead?

[Jack-R-lantern]

Sure! thanks.

[ti-mo]

Your outline doesn't account for gems such as __section_tail(CILIUM_MAP_POLICY, TEMPLATE_HOST_EP_ID) where we rely on changing the HOST_EP_ID to update a different slot for each ELF load. Any idea how to tackle that?

Closing the loop on this with #29307. ELFs should be very careful inserting entries into shared prog arrays, as doing so means making code reachable from the rest of the system. Adding an entry to such a global prog array is basically the equivalent of attaching an entrypoint. It should not be done as part of loading an ELF, as the control flow of its various programs is only guaranteed to be sane after all (internal) tail call maps have been populated.

As such, inserting these programs should be delegated to the code responsible for loading Endpoints. That code already has knowledge of the endpoint ID, so it can easily insert a prog into policy call map(s).

Tracking this in #29331.

[github-actions]

This issue has been automatically marked as stale because it has not
had recent activity. It will be closed if no further activity occurs.

[github-actions]

This issue has not seen any activity since it was marked stale.
Closing.