ebpf icon indicating copy to clipboard operation
ebpf copied to clipboard

link: implement kprobe.multi link

Open mmat11 opened this issue 2 years ago • 6 comments

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

mmat11 avatar Jun 16 '22 20:06 mmat11

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?

lmb avatar Jun 24 '22 12:06 lmb

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 (https://github.com/libbpf/libbpf/commit/c84815ee376b32d344f2dadf436912138c91255c), I can recompile them once https://github.com/cilium/ebpf/pull/668 is merged

mmat11 avatar Jun 24 '22 15:06 mmat11

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.

ti-mo avatar Jun 28 '22 15:06 ti-mo

The following make transparent fallback difficult:

https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

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/[email protected]/

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.

lmb avatar Jul 05 '22 17:07 lmb

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 avatar Jul 06 '22 07:07 olsajiri

I made first draft in here https://github.com/cilium/tetragon/pull/79/commits/ebccee5eb063c6e0e313acc12cdc41754fe58493 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 avatar Jul 14 '22 12:07 olsajiri

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

olsajiri avatar Aug 13 '22 13:08 olsajiri

@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

mmat11 avatar Aug 13 '22 17:08 mmat11

@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

olsajiri avatar Aug 14 '22 09:08 olsajiri

@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.

ti-mo avatar Aug 14 '22 09:08 ti-mo

we are going to use kprobe_multi link instead of standard kprobe, when there's support detected, like in https://github.com/cilium/tetragon/pull/79/commits/1065b26879f10ee25d79a5cb3d0c28dee228da9f :

	// 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 avatar Aug 14 '22 14:08 olsajiri

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

olsajiri avatar Aug 14 '22 14:08 olsajiri

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!

mmat11 avatar Sep 06 '22 15:09 mmat11