Implementing StructOps Support

[takehaya]

StructOps are used in sched_ext, mptcp, and tcp congestion control. It appears that cilium/ebpf does not support them. Why hasn't this been addressed?

If possible, I would like to work on this. Could you guide me on where to start?

[lmb]

I'm happy to help, the problem is that I don't really know how struct_ops work. Maybe you can explain them to me?

• Which program type do they use?
• How are they attached?

[ti-mo]

Could you please share some error output so we're sure we're talking about the same thing? It's hard to help without any real information.

[shun159]

The following is an error message when loading a program that includes references to .rodata.

2024/10/01 21:59:42 loading objects: field SimpleSched1: map simple_sched1: failed to load struct_ops program: simple_enable cannot load program without loading its whole collection: instruction 0: unsatisfied map reference

However, I may have figured out the cause. It's probably that the program should be loaded from NewCollectionWithOptions(), but I'm not doing that now.

[ti-mo]

It's probably that the program should be loaded from NewCollectionWithOptions(), but I'm not doing that now.

Exactly. 🙂 This is not an error related to StructOps, it goes for all ELFs. See https://ebpf-go.dev/concepts/loader/#loadandassign if you're only interested in a subset of map or program handles.

[shun159]

Thank you!

I will reconsider the implementation a bit while referring to the documentation. (I didn't know there was such a good resource available 😄)

[shun159]

Thanks. For now, I implemented it according to the document and your advise, confirmed that it works, and submitted a draft PR. #1603

[shun159]

@takehaya could you reopen here? I'd like to continue discussing how to proceed.

[shun159]

First, we'll set our current goal as enabling linking of .struct_ops.link.
The requirements for supporting StructOps can be broadly categorized into two:

1. ELF reading process
2. Loading process

For 1, the expected scope of work includes classification of structOpsSection, generation of MapSpec, and association of ProgramSpec and MapSpec. Also, we will consider the points you pointed out below. I'm thinking of placing necessary helper functions and StructOpsSpec in internal/struct_ops.

MapSpec and btf.Spec are important pieces of exported API and the bar is high for extending them. Avoid using them for storing arbitrary state that isn't useful for the caller.
Likewise, avoid trivial, overly-specific helpers on btf.Spec. The ones added here are basically struct ops business logic. Conder making them free functions in a separate package, e.g. internal/structops.

Question:
I'm wondering how to pass the state processed by LoadCollectionSpecFromReader() to LoadAndAssign(). While it might be possible to create some mapping by extending CollectionSpec, what do you think about extending CollectionSpec? For example, like adding a map to CollectionSpec to map StructOpsSpec and MapSpec that hold things like a list of pointers to ProgramSpec and btfID, as shown below:

+type StructOpsSpec struct {
+	ProgramSpecs []*ProgramSpec
+	KernFuncOff  []uint32
+	/* e.g. struct tcp_congestion_ops in bpf_prog's btf format */
+	Data []byte
+	/* e.g. struct bpf_struct_ops_tcp_congestion_ops in
+	 *      btf_vmlinux's format.
+	 * struct bpf_struct_ops_tcp_congestion_ops {
+	 *	[... some other kernel fields ...]
+	 *	struct tcp_congestion_ops data;
+	 * }
+	 * kern_vdata-size == sizeof(struct bpf_struct_ops_tcp_congestion_ops)
+	 * bpf_map__init_kern_struct_ops() will populate the "kern_vdata"
+	 * from "data".
+	 */
+	KernVData []byte
+	TypeId    btf.TypeID
+	Btf       *btf.Spec
+}

// CollectionSpec describes a collection.
type CollectionSpec struct {
	Maps     map[string]*MapSpec
	Programs map[string]*ProgramSpec

	// Variables refer to global variables declared in the ELF. They can be read
	// and modified freely before loading the Collection. Modifying them after
	// loading has no effect on a running eBPF program.
	Variables map[string]*VariableSpec

	// Types holds type information about Maps and Programs.
	// Modifications to Types are currently undefined behaviour.
	Types *btf.Spec

	// ByteOrder specifies whether the ELF was compiled for
	// big-endian or little-endian architectures.
	ByteOrder binary.ByteOrder
	
+	StructOpsSpecs map[string]*StructOpsSpec
}

[shun159]

for 2, basically after the program and map are loaded, we expect to need processing that generates kern_vdata and writes kern_vdata as a value to the map when attaching.
Whether to link using AttachRawLink like in the draft PR, or prepare a dedicated function like AttachStructOps and perform kern_vdata writing and Link at that time, can be decided later.
(It might be better to prepare a StructOps-specific Attach function)

[shun159]

I plan to split the PR into at least 4-5 parts. We will add helpers in each PR as needed. (Please let me submit doc/ and example/ in the final PR)

@ti-mo
I'm looking forward to your comment. have a good weekend 😄

[ti-mo]

Hi @shun159, I haven't forgotten, I'll try to allocate some time next week.

[shun159]

I'd like to resume this if that sounds good to everyone.
please let me know if there are any questions. thx

[lmb]

@shun159 thanks for pinging. Seems like this is going to be quite an involved feature, doing it in small PRs is definitely the way to go.

Some feedback upfront from looking at the draft PR:

• The ELF reader must not depend on the environment. Think of it as a pure function from ELF to *Spec. createStructOpsMap violates this by looking at the kernel types. The part that needs kernel types has to happen when creating the map.
• Like Timo said, avoid modifying exported API at all costs. You may be able to stuff the metadata you need into MapSpec.Contents. Something like MapSpec.Contents[uint32(0)] = structOpsMeta{}.
• I think this is doable without changing the exported API.

Can we figure out together in which order it would make sense to land things? As far as I understand, getting struct ops into the kernel involves the following:

User:

1. Declare a struct type in their .c which includes a bunch of function pointers. The struct may also contain other, non-function pointer data.
2. Load that collection
3. Take the map fd (?) and attach that via RawLink.

Library:

1. Discover struct ops from __section(".struct_ops.link") in the ELF. Each variable in that section is a separate map of type StructOpsMap.
2. Associate programs with the map. This seems to work similarly to how ProgramArrays are populated.
3. Load the struct ops map, which in turn loads necessary programs.
4. Insert programs into the map. The difference to ProgramArray is that program fds are just stuffed into a []byte instead of being inserted via map_update_elem (?). There is some trickiness here because we also need to account for differences between the kernel and the user type (so things might be at distinct offsets, etc.)

If that is correct the following order / PRs would make sense to me.

1. Create a StructOps map from a hand-crafted MapSpec. Assume that the user type == kernel type, so no complicated copying around based on BTF.
2. Support user type != kernel type, by implementing something like func extendType(sourceData []byte, source, dest Type) ([]byte, error).
3. Load a StructOpsMap from a CollectionSpec via collectionLoader.
4. Read StructOps MapSpec from ELF and output into CollectionSpec.

[shun159]

@lmb Would it be easier for you if I open a PR with the patch in its current state, so we can start the review and discussion?

[lmb]

Hi @shun159, I didn't see your message, sorry. Yes, if you want us to review something the way to go is to create a PR.

Please be aware that I won't be able to spend much time on this until September.

[shun159]

Thanks, @lmb.
Since the change is quite large, I’ll submit a series of MRs. First, I’ll modify link.go so that it accepts the ProgramFd from a struct_ops map.
cilium:ebpf:main...shun159:ebpf:feature/struct-ops-map_1

[shun159]

I've submitted a PR for link so that it accept programFd option from struct_ops map.
#1844

[shun159]

also, here is the PR so that create a "hand-crafted" StructOpsMap MapSpec with metadata
#1845