-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
new(driver/modern_bpf,userspace/libpman): support multiple programs for each event #2255
base: master
Are you sure you want to change the base?
Conversation
Perf diff from master - unit tests
Heap diff from master - unit tests
Heap diff from master - scap file
Benchmarks diff from master
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2255 +/- ##
==========================================
+ Coverage 75.16% 75.29% +0.12%
==========================================
Files 278 279 +1
Lines 34478 34389 -89
Branches 5922 5878 -44
==========================================
- Hits 25916 25893 -23
+ Misses 8562 8496 -66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
X64 kernel testing matrix
ARM64 kernel testing matrix
|
6096d33
to
92df24d
Compare
Please double check driver/API_VERSION file. See versioning. /hold |
3bbfcfd
to
aa354b6
Compare
…or each event. Try to inject each of them until success. This allows us to inject `bpf_loop` sendmmsg and recvmmsg programs where supported, and fallback at just sending first message where it isn't. Signed-off-by: Federico Di Pierro <[email protected]>
aa354b6
to
24581f6
Compare
Removed
This seems like a bug in the verifier since i cannot repro it locally, neither in arm64 CI, neither in kernel testing matrix. |
In the last commit i tried to split Note also that the exact same code for the 2 bpf programs wasn't failing some weeks ago (ie: at this commit: 67975da). |
0323c41
to
24581f6
Compare
/milestone next-driver |
b000c6f
to
d930932
Compare
Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
bb4321f
to
ffb1207
Compare
Oh i should test with same clang version we use in the CI: |
Hi, I've rebased my #1842 PR on this branch, and tried to build it with Clang 13 in the environment. I'm getting an undeclared error:
|
Which libbpf version are you using? |
These are the dependencies I tested with:
|
That's weird, you are using correct versions; it seems like the build is using an older libbpf version though since its headers do not contain |
…back. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
…_dynamic_snaplen at each bpf_loop iteration for sendmmsg and recvmmsg. This also fixes a verifier issue on clang 14, related to stack length. Signed-off-by: Federico Di Pierro <[email protected]>
e83a441
to
3ab30fc
Compare
Signed-off-by: Federico Di Pierro <[email protected]>
cdd8980
to
acbd21c
Compare
@@ -55,7 +55,7 @@ jobs: | |||
kernelrelease: 6.4.1-1.el9.elrepo.aarch64 | |||
target: centos | |||
kernelurls: https://download.falco.org/fixtures/libs/kernel-ml-devel-6.4.1-1.el9.elrepo.aarch64.rpm | |||
runs-on: ubuntu-latest | |||
runs-on: ubuntu-24.04-arm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improvement: run driverkit arm64 CI on arm64 gh runners.
@@ -220,8 +220,9 @@ jobs: | |||
cd src && make install | |||
cd ../../ | |||
git clone https://github.com/libbpf/libbpf.git --branch v1.3.0 --single-branch | |||
cd libbpf/src && BUILD_STATIC_ONLY=y DESTDIR=/ make install | |||
cd libbpf/src && BUILD_STATIC_ONLY=y DESTDIR=/ make install install_uapi_headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix: s390x was using system libbpf headers, not the one built by us in the CI.
@@ -676,6 +676,7 @@ static __always_inline void auxmap__store_socktuple_param(struct auxiliary_map * | |||
switch(socket_family) { | |||
case AF_INET: { | |||
struct inet_sock *inet = (struct inet_sock *)sk; | |||
struct sockaddr_in usrsockaddr_in = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple similar fixes: do not keep a reference to an out-of-scope variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe i'm missing something but why is this a fix? it seems to be used only inside the below if to read some data from the kernel
@@ -1556,6 +1558,13 @@ static __always_inline void apply_dynamic_snaplen(struct pt_regs *regs, | |||
*/ | |||
unsigned long args[5] = {0}; | |||
struct sockaddr *sockaddr = NULL; | |||
typedef union { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple similar improvements: don't waste lots of stack space while we just need one of these.
// in dynamic_snaplen_args. | ||
// This also gives a small perf boost while using `bpf_loop` because we don't need | ||
// to re-fetch first 3 syscall args at every iteration. | ||
__builtin_memcpy(args, input_args->mm_args, 3 * sizeof(unsigned long)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sendmmsg/recvmmsg
improvement: since we are going to call apply_dynamic_snaplen
for each bpf_loop
iteration, just call extract__network_args
once (in the sendmmsg/recvmmsg
main program) and then reference it.
This also fixes a verifier issue on clang-14 about stack size too large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @FedeDP! PR is looking really good! Just have a couple minor comments and questions.
Use anonymous unions in modern bpf driver. Moreover, add some debug prints to `pman_prepare_progs_before_loading`, and always disable all unused programs autoload. Signed-off-by: Federico Di Pierro <[email protected]> Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
Ehy @Molter73 should've addressed everything! Let me know :) Thanks for the in-depth review btw! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's just one more tiny comment, but it is more of a readability thing, everything else looks good.
/lgtm
progs[idx].name, | ||
progs[idx].feat); | ||
pman_print_msg(FALCOSECURITY_LOG_SEV_DEBUG, (const char *)msg); | ||
chosen_idx = idx; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just break
out of the inner loop here and use idx
directly in the next block? It would also mean we don't need should_disable
because the final block in the loop will only be reached from the other branch of this if-else block.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, Molter73 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, thanks!
@@ -676,6 +676,7 @@ static __always_inline void auxmap__store_socktuple_param(struct auxiliary_map * | |||
switch(socket_family) { | |||
case AF_INET: { | |||
struct inet_sock *inet = (struct inet_sock *)sk; | |||
struct sockaddr_in usrsockaddr_in = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe i'm missing something but why is this a fix? it seems to be used only inside the below if to read some data from the kernel
bool should_disable = chosen_idx != -1; | ||
if(!should_disable) { | ||
if(progs[idx].feat > 0 && | ||
libbpf_probe_bpf_helper(BPF_PROG_TYPE_RAW_TRACEPOINT, progs[idx].feat, NULL) == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual programs we use in modern ebpf is BPF_PROG_TYPE_TRACING
even if it shouldn't change too much
libbpf_probe_bpf_helper(BPF_PROG_TYPE_RAW_TRACEPOINT, progs[idx].feat, NULL) == | |
libbpf_probe_bpf_helper(BPF_PROG_TYPE_TRACING, progs[idx].feat, NULL) == |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area driver-modern-bpf
/area libpman
Does this PR require a change in the driver versions?
What this PR does / why we need it:
Allow to specify multiple program names for each event type and try to inject each of them until success.
This allows us to inject
bpf_loop
sendmmsg and recvmmsg programs where supported, and fallback at a program just sending first message where it isn't.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This superseedes #2233
Does this PR introduce a user-facing change?: