link: implement kprobe.multi link #716

mmat11 · 2022-06-16T20:27:14Z

Leaving as draft until CI supports 5.18.
Also, support for pattern matching and addresses array (which libbpf already supports) is to be evaluated

Edit: I've added support for addresses array, I left "pattern matching" out for now since it's not part of the link_create API

lmb · 2022-06-24T12:01:04Z

Do you have a link to the corresponding upstream patch set?

Should we support transparent multi attach, with a fallback for old kernels instead of a separate multi attach API? How does libbpf expose this?

mmat11 · 2022-06-24T15:33:10Z

Do you have a link to the corresponding upstream patch set?

https://patchwork.kernel.org/project/netdevbpf/list/?series=623878&state=*

I think there are also some bugfixes which are not part of that set

Should we support transparent multi attach, with a fallback for old kernels instead of a separate multi attach API? How does libbpf expose this?

From what I can see, libbpf doesn't do fallback; I think it doesn't make much sense as the underlying API is different from the one of classic kprobes

p.s. I think we need to enable CONFIG_FPROBE=y in ci-kernels (libbpf/libbpf@c84815e), I can recompile them once #668 is merged

ti-mo · 2022-06-28T15:47:13Z

Thanks for picking this up! (cc @olsajiri)

Should we support transparent multi attach, with a fallback for old kernels instead of a separate multi attach API?

I've been mulling this over for a while, was thinking the same. There's no straightforward way to make K(ret)probe() variadic since symbol is the first arg. This kind of addition justifies breaking API in my book. KprobeOptions and KprobeMultiOptions are too similar to be separate IMO.

What about unifying them and something like func Kprobe(prog *ebpf.Program, opts *KprobeOptions, symbols ...string) (Link, error)? Also parsing strings for symbols vs. addresses.

lmb · 2022-07-05T17:13:49Z

The following make transparent fallback difficult:

https://patchwork.kernel.org/project/netdevbpf/patch/20220316122419.933957-4-jolsa@kernel.org/

The fprobe API allows to attach probe on multiple functions at once
very fast, because it works on top of ftrace. On the other hand this
limits the probe point to the function entry or return.

This is incompatible with KprobeOptions.Offset. Possible to work around by returning an error I guess.

https://patchwork.kernel.org/project/netdevbpf/patch/20220316122419.933957-10-jolsa@kernel.org/

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 1ca520a29fdb..f3a31478e23b 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c 
@@ -8621,6 +8622,8 @@  static const struct bpf_sec_def section_defs[] = {
 	SEC_DEF("uprobe/",		KPROBE,	0, SEC_NONE),
 	SEC_DEF("kretprobe/",		KPROBE, 0, SEC_NONE, attach_kprobe),
 	SEC_DEF("uretprobe/",		KPROBE, 0, SEC_NONE),
+	SEC_DEF("kprobe.multi/",	KPROBE,	BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
+	SEC_DEF("kretprobe.multi/",	KPROBE,	BPF_TRACE_KPROBE_MULTI, SEC_NONE, attach_kprobe_multi),
 	SEC_DEF("tc",			SCHED_CLS, 0, SEC_NONE),
 	SEC_DEF("classifier",		SCHED_CLS, 0, SEC_NONE | SEC_SLOPPY_PFX | SEC_DEPRECATED),
 	SEC_DEF("action",		SCHED_ACT, 0, SEC_NONE | SEC_SLOPPY_PFX),

Multi kprobes have a distinct expected_attach_type. This significantly reduces the likelihood / usefulness of a fallback, since you can't just take a regular kprobe and attach it to to multiple functions.

olsajiri · 2022-07-06T07:44:25Z

not sure fallback is possible.. attaching all symbols by separate kprobe, but that would be real slow, I'd just return error

it's looks good, I'm out this week, but next week I'll try to rebase my multi_kprobe tetragon code on top of this and test, thanks

olsajiri · 2022-07-14T12:35:52Z

I made first draft in here cilium/tetragon@ebccee5 and I'm able to use the interface in tetragon, there are many loose ends I need to take care of, but the interface is fine for tetragon for now ;-) thanks

olsajiri · 2022-08-13T13:06:17Z

@mmat11 @ti-mo is there easy way to use the multi kprobe detection check from user code?

mmat11 · 2022-08-13T17:28:55Z

@mmat11 @ti-mo is there easy way to use the multi kprobe detection check from user code?

I could expose HaveKprobeMultiLink in the features package

olsajiri · 2022-08-14T09:01:25Z

@mmat11 @ti-mo is there easy way to use the multi kprobe detection check from user code?

I could expose HaveKprobeMultiLink in the features package

that would work for me, thanks

ti-mo · 2022-08-14T09:14:42Z

@mmat11 @ti-mo is there easy way to use the multi kprobe detection check from user code?

I could expose HaveKprobeMultiLink in the features package

that would work for me, thanks

Before we do that, could you give an example of how that would be used and why? (as opposed to just trying the multi-attach and falling back to perf attach when it fails?)

Probably better to make the multi attach itself return ErrNotSupported instead of adding arbitrary feature probes. Ask for forgiveness, not permission etc.

olsajiri · 2022-08-14T14:49:31Z

we are going to use kprobe_multi link instead of standard kprobe,
when there's support detected, like in cilium/tetragon@1065b26 :

	// use multi kprobe only if there's support and multiple kprobes defined
	useMulti = bpf.HasKprobeMulti() && len(kprobes) > 1

I'd call HaveKprobeMultiLink instead of HasKprobeMulti

olsajiri · 2022-08-14T14:56:23Z

Probably better to make the multi attach itself return ErrNotSupported instead of adding arbitrary feature probes. Ask for forgiveness, not permission etc.

there's lot of different configuration code for standard and multi kprobes,
and the link attachment comes as last, so I'd like to know before all that

internal/cmd/gentypes/main.go

ti-mo

As discussed offline, leaving this review to highlight the main points I'd liked to see differently while hacking on this code. Mostly cleaned up the middle part of kprobeMulti and moved all input validation to the top. Will push my changes shortly.

link/kprobe_multi.go

link/kprobe_multi_test.go

link/link.go

link/kprobe_multi.go

mmat11 · 2022-09-06T15:57:46Z

As discussed offline, leaving this review to highlight the main points I'd liked to see differently while hacking on this code. Mostly cleaned up the middle part of kprobeMulti and moved all input validation to the top. Will push my changes shortly.

Thanks, looks better!

lmb · 2022-09-15T11:07:21Z

link/kprobe_multi.go

+var _ Link = (*kprobeMultiLink)(nil)
+
+func (kml *kprobeMultiLink) Update(prog *ebpf.Program) error {
+	return fmt.Errorf("update kprobe_multi: %w", ErrNotSupported)


Hardcoding ErrNotSupported here for each link type really sucks, we should change the kernel to return a sensible errno.

link/kprobe_multi.go

lmb · 2022-09-15T11:08:46Z

link/kprobe_multi.go

+	}
+	defer prog.Close()
+
+	fd, err := sys.LinkCreateKprobeMulti(&sys.LinkCreateKprobeMultiAttr{


Isn't it enough to check that AttachTraceKprobeMulti is accepted by the kernel?

seems like it's accepted even when not supported

link/kprobe_multi_test.go

lmb · 2022-09-15T11:11:55Z

internal/sys/types.go

+	ProgFd           uint32
+	TargetFd         uint32
+	AttachType       AttachType
+	Flags            uint32


@joamaki somewhat OT, but why does kprobe_multi not use the existing flags field?

@lmb I think you mean @olsajiri 😉

Signed-off-by: Timo Beckers <timo@isovalent.com>

Having both a switch statement to initialize the metadata struct pointer as well as a list of link types was a bit redundant. Signed-off-by: Timo Beckers <timo@isovalent.com>

Co-authored-by: Lorenz Bauer <i@lmb.io>

As of Linux 5.18, or commit 5a5c11ee3e65 ("Merge branch 'bpf: Add kprobe multi link'"), attaching multiple k(ret)probes using a single system call is now possible. This commit adds support for this through the KprobeMulti() and KretprobeMulti() APIs in package link. Co-authored-by: Timo Beckers <timo@isovalent.com>

ti-mo

Thanks!

mmat11 mentioned this pull request Jul 13, 2022

rebuild all kernels with CONFIG_FPROBE cilium/ci-kernels#29

Merged

mmat11 force-pushed the matt/multikp branch from 0fedd4d to 7f252c5 Compare July 13, 2022 15:29

mmat11 force-pushed the matt/multikp branch 4 times, most recently from 9e00e33 to 2a39fd8 Compare July 20, 2022 15:29

mmat11 marked this pull request as ready for review July 20, 2022 15:32

mmat11 force-pushed the matt/multikp branch from 2a39fd8 to e70993a Compare July 21, 2022 09:33

mmat11 force-pushed the matt/multikp branch from e70993a to 230ccc2 Compare July 30, 2022 07:59

olsajiri mentioned this pull request Aug 13, 2022

tetragon: Add support for kprobe multi interface cilium/tetragon#79

Merged

mmat11 force-pushed the matt/multikp branch 2 times, most recently from 0ee4bc5 to 3e3109e Compare August 30, 2022 08:51

ti-mo reviewed Sep 1, 2022

View reviewed changes

internal/cmd/gentypes/main.go Outdated Show resolved Hide resolved

ti-mo requested changes Sep 5, 2022

View reviewed changes

ti-mo force-pushed the matt/multikp branch from 3e3109e to 69a3664 Compare September 5, 2022 10:43

ti-mo requested a review from lmb September 5, 2022 10:44

ti-mo force-pushed the matt/multikp branch from 69a3664 to 0b0defc Compare September 5, 2022 11:26

brb mentioned this pull request Sep 6, 2022

Use multi-link kprobe attachment cilium/pwru#87

Closed

mmat11 commented Sep 6, 2022

View reviewed changes

link/kprobe_multi.go Show resolved Hide resolved

lmb force-pushed the matt/multikp branch 2 times, most recently from 1894450 to 586b751 Compare September 15, 2022 11:14

lmb requested changes Sep 15, 2022

View reviewed changes

ti-mo and others added 3 commits September 15, 2022 13:41

Bump x/sys for new BPF_F_KPROBE_MULTI_RETURN constant

23bc595

Signed-off-by: Timo Beckers <timo@isovalent.com>

link: deduplicate extra metadata reading logic

0929477

Having both a switch statement to initialize the metadata struct pointer as well as a list of link types was a bit redundant. Signed-off-by: Timo Beckers <timo@isovalent.com>

internal/sys: generate kprobe_multi link create attr

ff198e7

Co-authored-by: Lorenz Bauer <i@lmb.io>

mmat11 force-pushed the matt/multikp branch 2 times, most recently from b7b10a6 to 9208bf5 Compare September 15, 2022 11:55

mmat11 force-pushed the matt/multikp branch from 9208bf5 to efd936a Compare September 15, 2022 12:01

lmb approved these changes Sep 15, 2022

View reviewed changes

ti-mo approved these changes Sep 15, 2022

View reviewed changes

ti-mo merged commit aaa9af2 into cilium:master Sep 15, 2022

mmat11 deleted the matt/multikp branch November 1, 2022 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

link: implement kprobe.multi link #716

link: implement kprobe.multi link #716

mmat11 commented Jun 16, 2022 •

edited

lmb commented Jun 24, 2022

mmat11 commented Jun 24, 2022

ti-mo commented Jun 28, 2022 •

edited

lmb commented Jul 5, 2022

olsajiri commented Jul 6, 2022

olsajiri commented Jul 14, 2022

olsajiri commented Aug 13, 2022

mmat11 commented Aug 13, 2022

olsajiri commented Aug 14, 2022

ti-mo commented Aug 14, 2022 •

edited

olsajiri commented Aug 14, 2022 •

edited

olsajiri commented Aug 14, 2022 •

edited

ti-mo left a comment

mmat11 commented Sep 6, 2022

lmb Sep 15, 2022

lmb Sep 15, 2022

mmat11 Sep 15, 2022

lmb Sep 15, 2022

lmb Sep 15, 2022

ti-mo Sep 15, 2022

ti-mo left a comment

link: implement kprobe.multi link #716

link: implement kprobe.multi link #716

Conversation

mmat11 commented Jun 16, 2022 • edited

lmb commented Jun 24, 2022

mmat11 commented Jun 24, 2022

ti-mo commented Jun 28, 2022 • edited

lmb commented Jul 5, 2022

olsajiri commented Jul 6, 2022

olsajiri commented Jul 14, 2022

olsajiri commented Aug 13, 2022

mmat11 commented Aug 13, 2022

olsajiri commented Aug 14, 2022

ti-mo commented Aug 14, 2022 • edited

olsajiri commented Aug 14, 2022 • edited

olsajiri commented Aug 14, 2022 • edited

ti-mo left a comment

Choose a reason for hiding this comment

mmat11 commented Sep 6, 2022

lmb Sep 15, 2022

Choose a reason for hiding this comment

lmb Sep 15, 2022

Choose a reason for hiding this comment

mmat11 Sep 15, 2022

Choose a reason for hiding this comment

lmb Sep 15, 2022

Choose a reason for hiding this comment

lmb Sep 15, 2022

Choose a reason for hiding this comment

ti-mo Sep 15, 2022

Choose a reason for hiding this comment

ti-mo left a comment

Choose a reason for hiding this comment

mmat11 commented Jun 16, 2022 •

edited

ti-mo commented Jun 28, 2022 •

edited

ti-mo commented Aug 14, 2022 •

edited

olsajiri commented Aug 14, 2022 •

edited

olsajiri commented Aug 14, 2022 •

edited