-
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): add ppc64le support to old bpf and kmod plus CI job #1497
Changes from all commits
9dd72f5
5d012ef
737247e
1b38114
7a3b8af
201397d
f57237d
32fe4a9
c61230f
28a356d
2056fb8
5079dae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -15,6 +15,7 @@ if(BUILD_BPF) | |||||
set(bpf_min_kver_map_x86_64 4.14) | ||||||
set(bpf_min_kver_map_aarch64 4.17) | ||||||
set(bpf_min_kver_map_s390x 5.5) | ||||||
set(bpf_min_kver_map_ppc64le 4.18) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am pretty sure that eBPF support will also need same fixes as modern eBPF, ie:
I also noticed that we skipped an include here: https://github.com/falcosecurity/libs/blob/master/userspace/libscap/engine/gvisor/parsers.cpp#L35. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. I was able to build bpf and kmod out of the box without any changes, so I assumed I did not need to make any other modifications. I will add the fixes you suggested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FedeDP Btw, I noticed these 2 lines point to incorrect links for each architecture. libs/driver/bpf/plumbing_helpers.h Line 268 in f61e7ff
libs/driver/bpf/plumbing_helpers.h Line 289 in f61e7ff
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||||||
if (LINUX_KERNEL_VERSION VERSION_LESS ${bpf_min_kver_map_${TARGET_ARCH}}) | ||||||
message(WARNING "[BPF] To run this driver you need a Linux kernel version >= ${bpf_min_kver_map_${TARGET_ARCH}} but actual kernel version is: ${UNAME_RESULT}") | ||||||
endif() | ||||||
|
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.
Like for bpf, kmod too needs some small work:
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.
@FedeDP I was going through this
libs/driver/feature_gates.h
Line 64 in c2fd308
We need this patch for ppc64 too?
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.
I am not sure! That depends if
ppc64le
is hit by the kernel behavior of not sending the clone exit event for the child; at the moment, all supported architectures but x86_64 are hit.We have drivers tests for it though, eg: https://github.com/falcosecurity/libs/blob/master/test/drivers/test_suites/syscall_exit_suite/clone3_x.cpp#L191
So, if this test is passing fine running on ppc64le, we should not need the CAPTURE_SCHED_PROC_FORK enabled.
The same goes for CAPTURE_SCHED_PROC_EXEC.
cc @Andreagit97
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.
uhm it seems like
ppc64le
behaves likex86
since looking at failed tests here #1475 (comment)execve
andclone
test are passing.BTW always looking at the tests there is another issue:
it seems that
ppc64le
correctly returns anexecveat
exit event when anexecveat
syscall is called (like s390x and riscv)libs/test/drivers/test_suites/syscall_exit_suite/execveat_x.cpp
Line 214 in c2fd308
Other architectures like x86 instead return an
execve
instead of anexecveat
exit event.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.
Thanks @FedeDP @Andreagit97. I reran the test and yes
clone3_x
is passing so we won't need the patch.For the
execveat
, I will add a condition forppc64le
but should that be done with a separate PR?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.
Tried it. That seems to be the offending line
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.
Thanks for the extensive tests!
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.
Uhm it seems strange
if
head!= null
,next_child
cannot benull
because in the worst case, it would benext_child==head
, BTW we never dereferencenext_child
so it shouldn't be an issue at all 🤔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.
Yep the issue is within
find_new_reaper_pid
:/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.
I opened the issue to track the legacy bpf verifier failure: #1521