Add a few program attach variants #1115
Conversation
atenart
changed the title
Looks like some of the added tests are failing while being triggered in Github actions. They worked locally and there might be some incompatibilities. Note that no previously existing test is failing AFAICT. |
There was a problem hiding this comment.
Overall this seems great, thanks! Left a few mostly minor comments. As for the test failures, let's ignore them for the time being, as it seems GitHub Actions kernel is just not there yet or is compiled without support.
--- libbpf-rs/tests/test.rs
+++ libbpf-rs/tests/test.rs
@@ -1653,6 +1653,7 @@ fn test_object_kprobe_with_opts() {
/// kprobe_multi.
#[tag(root)]
#[test]
+#[ignore = "requires kernel with kprobe support"]
fn test_object_kprobe_multi() {
let mut open_obj = open_test_object("kprobe.bpf.o");
open_objOr so. I checked and they pass on my local system.
Thanks for the prompt review! Yes, looks like the kernel on Github actions is not supporting kprobe multi and access to getting the BPF cookie from raw tp (that last part isn't surprising as it was only added a year ago, I just checked). As you suggested, I added an As a side note looks like the raw tracepoint opt addition might need to take into account #1114, or the other way around, depending on merge order. |
libbpf-rs/src/program.rs
Outdated
| // Calling `CString::into_raw` requires the CString to be reconstituted | ||
| // using `CString::from_raw` to be properly deallocated. | ||
| let _ = syms.drain(..).map(|s| unsafe { CString::from_raw(s) }); |
There was a problem hiding this comment.
I am confused about this line.
There was a problem hiding this comment.
CString::into_raw transfers ownership and to avoid memory leak the above is required. This is explained in the doc, https://doc.rust-lang.org/std/ffi/struct.CString.html#method.into_raw.
There was a problem hiding this comment.
It makes sense, but it's kind of error prone. The next person may adjust the attach call above to return eagerly and we end up leaking again (even more easy to do because the link is actually a Result<Link> and not Link as seems to be implied). Personally I'd prefer we keep the Vec<CString> around and then another Vec comprised of a mapping using the non-consuming https://doc.rust-lang.org/std/ffi/struct.CString.html#method.as_ptr
I don't feel strongly, though.
There was a problem hiding this comment.
Actually, we may leak even now: if part of
let mut syms = symbols
.iter()
.map(|s| util::str_to_cstring(s.as_ref()).map(|s| s.into_raw()))fails midway through then I think we loose the first part that has already been converted to raw pointers, right? So we should probably fix that up.
There was a problem hiding this comment.
Good point; you're right choosing into_raw instead of as_ptr to avoid keeping a Vec<CString> around led to leak memory on error. Changing this as you suggested, keeping an extra Vec around.
atenart
force-pushed
the
attach-variants
branch
2 times, most recently
from
5f01140 to
20fa95b
Compare
The kprobe multi API allows to attach a kprobe to multiple symbols at once, speeding up attach time significantly. Two new attach functions are added, attach_kprobe_multi and attach_kprobe_multi_with_opts to allow attaching kprobe and kretprobe programs to multiple symbols at once. The former is a simplification of the underlying API when no complex option is required while the second one exposes the full underlying API. The 'pattern' parameter is not exposed by this patch. It is incompatible with using symbols name explicitly and if needed one day could have its own libbpf-rs API, e.g. `attach_kprobe_multi_pattern`. Signed-off-by: Antoine Tenart <[email protected]>
Using the additional options provided at attach time, cookies can be passed to raw tracepoints. Signed-off-by: Antoine Tenart <[email protected]>
Using the additional options provided at attach time, cookies can be passed to kprobes. Signed-off-by: Antoine Tenart <[email protected]>
Add a test for attaching a raw tracepoint, with and without opts. The existing tracepoint is reused and the same tests are performed, with the slight modification of changing the program type. As raw tracepoints cannot be attached to individual syscalls, attach them to `sys_enter`. Signed-off-by: Antoine Tenart <[email protected]>
Adding kprobe tests is not so easy as it's not guaranteed the functions we attach to will be stable over time or not inlined by a compiler. This is an attempt at adding basic tests (just checking we can probe functions using kprobe(multi) with and without opts). Not ideal but the hope is this would be better than nothing. Signed-off-by: Antoine Tenart <[email protected]>
Signed-off-by: Antoine Tenart <[email protected]>
The following variants are added to attach a program:
The main motivation is 1) kprobe multi has significant performance benefits and 2) using additional options allows to use BPF cookies in probes.
In addition tests for raw tracepoints are added (for the existing support and for the new variant added), as well as basic tests for all kprobe variants (excluding kretprobes).
A few things to note wrt. to reviews:
attach_kprobe_with_optskept thekretprobeflag as an argument even though it is part of the underlyinglibbpf-sysoptions struct. It felt more natural IMO to keep this flag visible and in sync with the normal kprobe API, but this can be changed easily.