Add support for freplace programs #288

zeffron · 2021-04-19T17:35:35Z

Test base program, extension program, loader, and associated vmlinux.h can be found in this gist

Key changes:

FreplaceLink and associated AttachFreplace are added to the link package
A BTF id is now returned with Program.Info
btf.Specs can be constructed from loaded BTF
There were some syscall things I needed to reimplement in the btf package because they’re implemented in the ebpf package and I couldn’t create an import cycle.
AttachTarget was added to ProgramSpec as extension programs need to specify both the file handle of the intended (or a template) attach program and provide the name of the function to attach so BTF ID can be located.

ti-mo · 2021-04-20T11:36:01Z

Thank you! This change should probably be broken up into smaller PRs so they can be reviewed separately. (for example, the new APIs in internal/btf: NewProgram, *Handle* etc.)

[...] I couldn’t create an import cycle. I’d like to lift those into internal. (In fact, I wonder if all non-exported, non-specific use-case syscall wrappers should maybe be lifted into internal.

Agreed, this will be necessary for #235 as well and should also go into a separate PR. Perhaps this should be left to @lmb, but not sure about his current time budget.

I'll mark this as a draft for now.

lmb

Thanks for investing so much work already :) Here are my takeaways:

Freplace itself seems not too complicated. The worst bit is that we need a BTF ID to attach to. We don't have a good way of specifying that right now.
The current code has problems with resource ownership / lifetime management. ProgramInfo mustn't contain *btf.Handle. We shouldn't use a plain int for AttachTarget.
We can drop the feature test and with that BTF writing. It's nice if a user gets a decent error, but the cost of maintaining BTF writing isn't worth it.

info.go

internal/btf/btf.go

internal/btf/info.go

lmb · 2021-04-20T15:46:18Z

internal/btf/strings.go

@@ -8,9 +8,12 @@ import (
 	"io/ioutil"
 )

-type stringTable []byte


We should keep stringTable a read-only structure and add a stringTableBuilder instead. We only need to modify it when building BTF on the fly, which is rare.

Will keep in mind for a future PR to reintroduce this functionality, but, as discussed, this is not needed for the functionality this PR is trying to introduce so it has been removed.

internal/btf/types.go

prog.go

lmb · 2021-04-21T09:41:31Z

internal/btf/btf.go

+// Returns ErrNotExist, if there is no BTF with the given id.
+// Returns ErrNotSupported if BTF is not supported.
+func NewHandleFromID(id TypeID) (*Handle, error) {
+	if err := haveBTF(); err != nil {


Not necessary I think.

I could make a function that goes directly from ID to Spec, but it seemed likely that once the BTF library is exported that there will be desire for going from a BTF ID to a Handle in other use cases as well.

It's been a while, but I think I meant the call to haveBTF is probably superfluous here.

Ah, yes. I think I had that for when it was possible to construct BTF specs from a consumer of the library (when I had all of the type changes) since this could have been the first entry to BTF in the kernel in that case. It is not needed now that all of that is gone. Will remove.

Addressed in b87cf4a

internal/btf/info.go

lmb · 2021-04-21T09:46:16Z

internal/btf/info.go

+		return nil, err
+	}
+
+	rawTypes, rawStrings, err := parseBTF(bytes.NewReader(btfBuffer), internal.NativeEndian)


can this call loadNakedSpec instead?

Ah, maybe! I saw that loadNakedSpec took section information. Didn't realize it was valid to pass nil for that.

Addressed in 3df3789

link/freplace.go

zeffron · 2021-06-28T02:39:12Z

My apologies for having to put this down for so long. I appreciate all of the excellent feedback and am going through it now.

zeffron · 2021-06-30T04:48:12Z

PR updated. Original version is maintained at zeffron/freplace.old in my fork for reference for the BTF types code.
I've left this as a draft as I believe there's still some polishing work to be done. Also because it needs unit tests.

zeffron · 2021-06-30T05:38:02Z

I believe the TestLibBPFCompat/test_trace_ext.o is failing because the 5.10 kernel does not have freplace support. Without the probe, I could use internal.KernelVersion() to check for the minimum kernel version, but it does seem like feature probes are preferred. Thoughts?

zeffron · 2021-06-30T06:07:41Z

Ah. I misunderstood the failure situation. They weren't failing the attach, they were failing the load because they require a second program to be attached to. I'll have to figure out how to update the unit tests for that.

zeffron · 2021-07-01T01:05:50Z

And I misunderstood again. Those object files wouldn't be there if they couldn't be loaded, so the kernel version isn't the issue. Which means it's just the missing attach target part that needs to be worked out.

link/link_test.go

elf_reader_test.go

internal/btf/btf.go

internal/btf/info.go

lmb · 2021-07-15T09:53:48Z

link/freplace.go

+		return nil, fmt.Errorf("eBPF program type %s is not an Extension: %w", prog.Type(), errInvalidInput)
+	}
+
+	info, err := targetProg.Info()


This is the same logic as in resolveBTFType? What happens if the name (and ultimately the BTF ID) here is different than what was passed at load time? It's a shame that we need this twice.

This prompted me to go through a deeper dive on how libbpf and the kernel handles this. I think there's a way to avoid this duplicate execution in the common case (but not the duplicate code). I should have an updated PR sometime this week.

To answer your question more specifically, it looks like libbpf and the kernel optionally only actually do this lookup once. If it's done at load time, it gets cached on the kernel program object. If the at link time, no program and function name are provided, then the cached values are used. If a program and function name are provided, they override the cached values (in which case the name and program provided at load time are completely wasted). As long as the final target function (supplied at load time or link time) has a matching signature to the extension function, the link is successfully created.

Agreed that it's a shame to need it twice. We could potentially create a function to get BTF from a program and put it into a package that's accessible from both link and ebpf. Maybe put it into the btf package?

Just re-discovered that extension programs MUST provide attachment information at load time. It can still be overridden at link time (this allows replacing two different functions with the same program, for example).

lmb · 2021-07-15T09:58:16Z

link/link.go

@@ -49,6 +50,8 @@ type RawLinkOptions struct {
 	Program *ebpf.Program
 	// Attach must match the attach type of Program.
 	Attach ebpf.AttachType
+	// BTF is the BTF of the attachment target (or 0).
+	BTF btf.Type


This should be btf.TypeID I think.

Why make this a btf.TypeID instead of a btf.Type? Other fields here use the Go native type instead of the ID, such as Program.
(The comment about it being 0 is incorrect based on current implementation. It should have said nil, but I think it also doesn't need to be said.)

My thinking was this (if I remember correctly): most of the code we have constructs a *Func to pass here, only for the Go type to be discarded. To look at it another way: since this is a low-level interface (hence the Raw in the name) it's better to stay close to the kernel interface.

info.go

internal/btf/btf.go

lmb

I think this is looking really good! Left some nits, the only open question for me is the behaviour of CO-RE relocations as mentioned in one of the comments.

link/freplace_test.go

link/link_test.go

prog.go

lmb · 2021-07-26T09:23:06Z

prog.go

@@ -185,6 +209,25 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions, handles *hand
 		if err != nil {
 			return nil, fmt.Errorf("load target BTF: %w", err)
 		}
+	} else if spec.AttachTarget != nil {


I think this else if branch needs to move into spec.AttachTo != "".

opts.TargetBTF specifies the location of the vmlinux BTF (for environments where that isn't available from a convenient location)

Even if opts.TargetBTF is specified, we need to use spec.AttachTarget to resolve the type ID for freplace programs.

targetBTF = nil if opts.TargetBTF { targetBTF = ... } if spec.BTF { btf.ProgramFixups(spec.BTF, targetBTF) } if spec.AttachTo { if spec.AttachTarget { targetBTF = ... } resolveBTFType(targetBTF) }

btf.ProgramFixups is always invoked with the kernel BTF or nil

resolveBTFType is invoked with the kernel BTF, the AttachTarget BTF or nil

The implication is that freplace programs are CO-RE relocated against the kernel, not against the program they are attaching to. Is that the correct behaviour?

prog.go

link/freplace_test.go

The btf package needs access to BPF_BTF_GET_FD_BY_ID, which is implemented via bpfObjGetFDByID which lives in the root ebpf package. The btf cannot access it there without creating an import cycle, so it is now moved into the internal collection of syscalls to allow access from the btf package.

Starting with the 5.10 kernel it is possible to replace functions (with some limitations) using eBPF. This is useful for XDP multi-attach. This commit adds a new link type to support that functionality.

This adds a unit test that loads a program with a subprogram, then replaces the subprogram. The resulting link is tested with the common link tests, excluding the update test because tracing links (the kind used by freplace) don't support link updates.

lmb · 2021-08-02T15:39:31Z

Thank you for your hard work, and sorry the review took so long!

zeffron · 2021-08-09T19:42:32Z

Thank you for the quality review. I think we ended up in a pretty good place. 😀

ti-mo changed the title ~~[WIP] Freplace~~ Support for freplace programs Apr 20, 2021

ti-mo marked this pull request as draft April 20, 2021 11:36

lmb reviewed Apr 21, 2021

View reviewed changes

zeffron force-pushed the zeffron/freplace branch from 1235b3b to ae43156 Compare June 30, 2021 04:47

zeffron force-pushed the zeffron/freplace branch 2 times, most recently from 8ed2747 to 32d5c3e Compare June 30, 2021 04:57

zeffron force-pushed the zeffron/freplace branch 3 times, most recently from 1dbfcba to eddfca3 Compare July 4, 2021 20:12

zeffron marked this pull request as ready for review July 4, 2021 20:19

zeffron changed the title ~~Support for freplace programs~~ Add support for freplace programs Jul 6, 2021

zeffron requested a review from lmb July 12, 2021 17:43

lmb requested changes Jul 15, 2021

View reviewed changes

zeffron force-pushed the zeffron/freplace branch 3 times, most recently from 15b1981 to d4a98bd Compare July 22, 2021 21:26

zeffron requested a review from lmb July 22, 2021 21:30

lmb requested changes Jul 26, 2021

View reviewed changes

zeffron force-pushed the zeffron/freplace branch 3 times, most recently from 24f14e4 to e7812fa Compare July 26, 2021 19:23

zeffron requested a review from lmb July 26, 2021 19:29

lmb approved these changes Aug 2, 2021

View reviewed changes

zeffron added 2 commits August 2, 2021 17:24

Add the FreplaceLink type for replacing functions

7209ef0

Starting with the 5.10 kernel it is possible to replace functions (with some limitations) using eBPF. This is useful for XDP multi-attach. This commit adds a new link type to support that functionality.

Add unit tests for FreplaceLink

ff5d96d

This adds a unit test that loads a program with a subprogram, then replaces the subprogram. The resulting link is tested with the common link tests, excluding the update test because tracing links (the kind used by freplace) don't support link updates.

lmb force-pushed the zeffron/freplace branch from 9445141 to ff5d96d Compare August 2, 2021 15:28

lmb merged commit 7ef0d0f into cilium:master Aug 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for freplace programs #288

Add support for freplace programs #288

zeffron commented Apr 19, 2021 •

edited

ti-mo commented Apr 20, 2021

lmb left a comment

lmb Apr 20, 2021

zeffron Jun 30, 2021

lmb Apr 21, 2021

zeffron Jun 30, 2021

lmb Jun 30, 2021

zeffron Jun 30, 2021

zeffron Jul 2, 2021

lmb Apr 21, 2021

zeffron Jun 30, 2021

zeffron Jun 30, 2021

zeffron commented Jun 28, 2021

zeffron commented Jun 30, 2021 •

edited

zeffron commented Jun 30, 2021

zeffron commented Jun 30, 2021

zeffron commented Jul 1, 2021

lmb Jul 15, 2021

zeffron Jul 19, 2021

zeffron Jul 21, 2021

zeffron Jul 21, 2021

zeffron Jul 21, 2021

lmb Jul 15, 2021

zeffron Jul 22, 2021

lmb Jul 26, 2021

lmb left a comment

lmb Jul 26, 2021

lmb commented Aug 2, 2021

zeffron commented Aug 9, 2021

Add support for freplace programs #288

Add support for freplace programs #288

Conversation

zeffron commented Apr 19, 2021 • edited

ti-mo commented Apr 20, 2021

lmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeffron commented Jun 28, 2021

zeffron commented Jun 30, 2021 • edited

zeffron commented Jun 30, 2021

zeffron commented Jun 30, 2021

zeffron commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmb commented Aug 2, 2021

zeffron commented Aug 9, 2021

zeffron commented Apr 19, 2021 •

edited

zeffron commented Jun 30, 2021 •

edited