ROX-10009: implementation of collector-specific custom eBPF probe#14
ROX-10009: implementation of collector-specific custom eBPF probe#14Stringy wants to merge 12 commits into
Conversation
… rest of the platforms
…es workarounds in the wider plumbing
stashed, resulting in invalid memory later and subsequent loss of events.
| COMMAND "${CMAKE_COMMAND}" -E copy_if_different probe.o "${CMAKE_CURRENT_BINARY_DIR}" | ||
| WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}" | ||
| VERBATIM) | ||
|
|
Molter73
left a comment
Molter73
left a comment
There was a problem hiding this comment.
This is awesome! Really looking forward to performance measuring to see how much of an improvement this is.
| @rm -f *~ | ||
|
|
||
| $(obj)/probe.o: $(src)/probe.c \ | ||
| $(obj)/probe.o: $(src)/collector_probe.c \ |
There was a problem hiding this comment.
This is OK for testing, but is there any chance we could instead move compilation of our probe into the collector repo itself?
| #include <linux/sched.h> | ||
| #include <uapi/linux/bpf.h> | ||
|
|
||
| // Unfortunately include order is important here, so turn clang-format |
| struct sysdig_bpf_settings* settings; | ||
| enum ppm_event_type evt_type = PPME_GENERIC_E; | ||
| int drop_flags = UF_ALWAYS_DROP; | ||
| struct sys_enter_args stack_ctx = {.id = id, {0}}; |
There was a problem hiding this comment.
I don't think we need the {0}, partially initializing structures defaults uninitialized fields to 0.
| struct sys_enter_args stack_ctx = {.id = id, {0}}; | |
| struct sys_enter_args stack_ctx = {.id = id}; |
| if (!settings) { | ||
| return 0; | ||
| } | ||
|
|
||
| if (!settings->capture_enabled) { |
There was a problem hiding this comment.
This is an EXTREMELY big nitpick, but I would prefer if checking for null pointers was expressed as ptr == NULL and boolean falseness using !bool. I know it's silly and the end result is exactly the same, but when you have multiple comparisons like these one after the other is makes it a little easier to follow which variables are pointers and which booleans. Food for thought.
| sc_evt = get_syscall_info(id); | ||
| if (!sc_evt) { | ||
| return 0; | ||
| } | ||
|
|
||
| if (sc_evt->flags & UF_USED) { | ||
| evt_type = sc_evt->enter_event_type; | ||
| drop_flags = sc_evt->flags; | ||
| } else { | ||
| // early indicator that this event is not needed/wanted, so just exit. | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Do we need this block still? I thought we were going to only attach used syscalls and now I'm confused on why we are checking this.
There was a problem hiding this comment.
The only reason I kept it in was because we have userspace access to these settings and could still disable certain syscalls at runtime if required. Obviously we don't really support that now, but for the sake of this small check, it might be worth keeping in? what do you think?
There was a problem hiding this comment.
If we are not using it right now, maybe we can put it behind an #ifdef? That way we keep the code but don't execute it for the time being.
There was a problem hiding this comment.
I've decided to just remove it for now, we can always add it back in later if that requirement comes up, and without it the function is a little simpler
| sc_evt = get_syscall_info(id); | ||
| if (!sc_evt) { | ||
| return 0; | ||
| } | ||
|
|
||
| if (sc_evt->flags & UF_USED) { | ||
| evt_type = sc_evt->exit_event_type; | ||
| drop_flags = sc_evt->flags; | ||
| } else { | ||
| // early indicator that this event is not needed/wanted, so just exit. | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Same as above, do we still need this?
|
The custom probe has been moved into the collector repository and as a result, this PR is now redundant. See stackrox/collector#670 for the other PR |
1 similar comment
|
The custom probe has been moved into the collector repository and as a result, this PR is now redundant. See stackrox/collector#670 for the other PR |
2 participants
This PR introduces a custom eBPF probe for collector that traces only the syscalls that we are currently interested in. It conforms to the same fillers and event types as the existing falco probe, and therefore interfaces with the userspace components seamlessly.
The intention is for this to replace the eBPF probes in collector, for significant performance gains.
The sister PR in the collector repo is here: stackrox/collector#670