new(driver): add ppc64le support to old bpf and kmod plus CI job #1497

mdafsanhossain · 2023-11-22T06:14:34Z

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libsinsp

/area tests

What this PR does / why we need it:

This PR adds GHA job to build kmod,bpf,modern-bpf on ppc64le (using QEMU) and also run libsinsp unit tests. It is similar to what is currently enabled for s390x here.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

FedeDP · 2023-11-22T07:59:32Z

driver/bpf/CMakeLists.txt

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


I am pretty sure that eBPF support will also need same fixes as modern eBPF, ie:

https://github.com/falcosecurity/libs/blob/master/driver/bpf/plumbing_helpers.h#L125

https://github.com/falcosecurity/libs/blob/master/driver/bpf/plumbing_helpers.h#L167

https://github.com/falcosecurity/libs/blob/master/driver/bpf/plumbing_helpers.h#L286

I also noticed that we skipped an include here: https://github.com/falcosecurity/libs/blob/master/userspace/libscap/engine/gvisor/parsers.cpp#L35.
I am not sure whether it is needed, but i guess so given we added aarch64 and s390x too (btw gvisor should only be supported on x86_64 atm, since the option is only available on it: https://github.com/falcosecurity/libs/blob/master/cmake/modules/engine_config.cmake#L26)

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.

@FedeDP Btw, I noticed these 2 lines point to incorrect links for each architecture.

libs/driver/bpf/plumbing_helpers.h

Line 268 in f61e7ff

* https://github.com/libbpf/libbpf/blob/master/src/bpf_tracing.h#L166-L178

libs/driver/bpf/plumbing_helpers.h

Line 289 in f61e7ff

* https://github.com/libbpf/libbpf/blob/master/src/bpf_tracing.h#L132-L144

I think libbpf touched those files (we should've linked a fixed hash instead of master). No problem anyway!

FedeDP · 2023-11-22T08:01:13Z

driver/CMakeLists.txt

@@ -26,6 +27,7 @@ else()
 	set(kmod_min_kver_map_aarch64 3.16)
 	set(kmod_min_kver_map_s390x 2.6)
 	set(kmod_min_kver_map_riscv64 5.0)
+	set(kmod_min_kver_map_ppc64le 2.6)


Like for bpf, kmod too needs some small work:

https://github.com/falcosecurity/libs/blob/master/driver/main.c#L2144

https://github.com/falcosecurity/libs/blob/master/driver/ppm.h#L81

@FedeDP I was going through this

libs/driver/feature_gates.h

Line 64 in c2fd308

#if defined(CONFIG_ARM64) || defined(CONFIG_S390) || defined(CONFIG_RISCV)

We need this patch for ppc64 too?

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

uhm it seems like ppc64le behaves like x86 since looking at failed tests here #1475 (comment) execve and clone test are passing.

BTW always looking at the tests there is another issue:

[ FAILED ] SyscallExit.execveatX_correct_exit [ FAILED ] SyscallExit.execveatX_execve_exit [ FAILED ] SyscallExit.execveatX_success_memfd

it seems that ppc64le correctly returns an execveat exit event when an execveat syscall is called (like s390x and riscv)

libs/test/drivers/test_suites/syscall_exit_suite/execveat_x.cpp

Line 214 in c2fd308

#if defined(__s390x__) || defined(__riscv)

Other architectures like x86 instead return an execve instead of an execveat exit event.

Thanks @FedeDP @Andreagit97. I reran the test and yes clone3_x is passing so we won't need the patch.

[ RUN ] SyscallExit.clone3X_father [ OK ] SyscallExit.clone3X_father (2780 ms) [ RUN ] SyscallExit.clone3X_child [ OK ] SyscallExit.clone3X_child (1139 ms) [ RUN ] SyscallExit.clone3X_create_child_with_2_threads [ OK ] SyscallExit.clone3X_create_child_with_2_threads (1659 ms) [ RUN ] SyscallExit.clone3X_child_clone_parent_flag [ OK ] SyscallExit.clone3X_child_clone_parent_flag (899 ms) [ RUN ] SyscallExit.clone3X_child_new_namespace_from_child [ OK ] SyscallExit.clone3X_child_new_namespace_from_child (1389 ms) [ RUN ] SyscallExit.clone3X_child_new_namespace_from_caller [ OK ] SyscallExit.clone3X_child_new_namespace_from_caller (3079 ms) [ RUN ] SyscallExit.clone3X_child_new_namespace_create_thread [ OK ] SyscallExit.clone3X_child_new_namespace_create_thread (2649 ms) [ RUN ] SyscallExit.cloneX_father [ OK ] SyscallExit.cloneX_father (2619 ms) [ RUN ] SyscallExit.cloneX_child [ OK ] SyscallExit.cloneX_child (1279 ms)

For the execveat , I will add a condition for ppc64le but should that be done with a separate PR?

Tried it. That seems to be the offending line

Thanks for the extensive tests!

Uhm it seems strange

struct list_head *head = &(task->children); struct list_head *next_child = (struct list_head *)_READ(head->next); if(next_child != head) { /* We have at least one child, so we need a reaper for it */ reaper_pid = find_new_reaper_pid(data, task); }

if head!= null, next_child cannot be null because in the worst case, it would be next_child==head, BTW we never dereference next_child so it shouldn't be an issue at all 🤔

Yep the issue is within find_new_reaper_pid :/

I opened the issue to track the legacy bpf verifier failure: #1521

FedeDP · 2023-11-22T08:01:55Z

.github/workflows/ci.yml

@@ -362,6 +362,44 @@ jobs:
            KERNELDIR=/lib/modules/$(ls /lib/modules)/build make scap-open driver bpf unit-test-libsinsp -j6
            ./libsinsp/test/unit-test-libsinsp

+  build-libs-ppc64le:


This is a nice idea! Can we use a matrix to merge together this job with the build-libs-s390x one?

Yes we can. I will try it out.

this is a great point, moreover, I just thought that the s390x job takes almost 1 h to run but in the end, we want to check the build of our drivers (modern_bpf, kmod, bpf) why do we need to build sinsp and run tests? In order to not slow down our CI too much I would propose just building the drivers in this CI job, and maybe we can do the same for the s390x, WDYT?

Big +1 from me!

Do you recommend a separate job for testing sinsp or do we stick to the driver builds for now?

I think sticking with the drvers build for now is fine; higher level code rarely needs adjustments for new architecture, therefore we are pretty confident that we cannot break the build of scap/sinsp for a given architecture.

yep let's say that breaking the build of the userspace part should never happen and a 1h job at every build seems a high price to pay

Makes sense. I will modify for both archs. @hbrueckner let me know if this is ok for s390x

HI @mdafsanhossain

Makes sense. I will modify for both archs. @hbrueckner let me know if this is ok for s390x

@Andreagit97 @FedeDP let's reduce the build time on s390x by focusing on the drivers only.

FedeDP · 2023-11-24T14:23:13Z

I updated the title to be more precise!
Let's try this for
/milestone next-driver

FedeDP · 2023-11-28T10:34:04Z

Failing tests for each driver:
kmod:

[  FAILED  ] 9 tests, listed below:
[  FAILED  ] SyscallExit.getrlimitX_success
[  FAILED  ] SyscallExit.prlimit64X
[  FAILED  ] SyscallExit.socketcall_sendmsgX_fail
[  FAILED  ] SyscallExit.socketcall_wrong_code_socketcall_interesting
[  FAILED  ] SyscallExit.socketcall_wrong_code_socketcall_not_interesting
[  FAILED  ] SyscallExit.socketcall_null_pointer_and_wrong_code_socketcall_interesting
[  FAILED  ] SyscallEnter.socketcall_wrong_code_socketcall_interesting
[  FAILED  ] SyscallEnter.socketcall_wrong_code_socketcall_not_interesting
[  FAILED  ] SyscallEnter.socketcall_null_pointer_and_wrong_code_socketcall_interesting

bpf: (without reaper_pid part of procexit filler though...)

[  FAILED  ] 8 tests, listed below:
[  FAILED  ] SyscallExit.getrlimitX_success
[  FAILED  ] SyscallExit.prlimit64X
[  FAILED  ] SyscallExit.socketcall_sendmsgX_fail
[  FAILED  ] SyscallExit.socketcall_wrong_code_socketcall_not_interesting
[  FAILED  ] SyscallEnter.socketcall_wrong_code_socketcall_not_interesting
[  FAILED  ] GenericTracepoints.sched_proc_exit_prctl_subreaper
[  FAILED  ] GenericTracepoints.sched_proc_exit_child_namespace_reaper
[  FAILED  ] GenericTracepoints.sched_proc_exit_reaper_in_the_same_group

modern-bpf:

[  FAILED  ] 14 tests, listed below:
[  FAILED  ] SyscallExit.getrlimitX_success
[  FAILED  ] SyscallExit.getsockoptX_SO_RCVTIMEO
[  FAILED  ] SyscallExit.getsockoptX_SO_PASSCRED
[  FAILED  ] SyscallExit.io_uring_setupX
[  FAILED  ] SyscallExit.mlockallX
[  FAILED  ] SyscallExit.prlimit64X
[  FAILED  ] SyscallExit.sendmsgX_fail
[  FAILED  ] SyscallExit.setsockoptX_SO_RCVTIMEO
[  FAILED  ] SyscallExit.setsockoptX_SO_PASSCRED
[  FAILED  ] SyscallExit.socketcall_sendmsgX_fail
[  FAILED  ] SyscallExit.socketcall_getsockoptX_SO_RCVTIMEO
[  FAILED  ] SyscallExit.socketcall_getsockoptX_SO_PASSCRED
[  FAILED  ] SyscallExit.socketcall_setsockoptX_SO_RCVTIMEO
[  FAILED  ] SyscallExit.socketcall_setsockoptX_SO_PASSCRED

FedeDP · 2023-11-28T10:57:57Z

The syscall 'getrlimit' must be successful. Errno: 38 err_message: Function not implemented

Getrlimit is not implemented and test should be skipped. Weird thing is that the __NR_ code exists though.
prlimit64 has the same issue because it calls getrlimit.
I think we should find a way to dynamically skip tests for errno=38 (ENOSYS); i am trying a patch:

--- a/test/drivers/event_class/event_class.cpp
+++ b/test/drivers/event_class/event_class.cpp
@@ -51,6 +51,12 @@ void assert_syscall_state(int syscall_state, const char* syscall_name, long sysc
 {
        bool match = false;

+       if (errno == ENOSYS)
+       {
+               GTEST_SKIP() << "Syscall " << syscall_name << " not implemented"  << std::endl;
+               return;
+       }
+

FedeDP · 2023-11-28T11:05:24Z

Seems to work reliably; added the PR: #1517

mdafsanhossain · 2023-11-28T12:53:37Z

Seems to work reliably; added the PR: #1517

Thanks for the patch

FedeDP · 2023-11-29T09:24:35Z

So, what we need here is the CI part, right? Ie: the matrix merging s390x and ppc64le (and stopping at drivers build without testing libsinsp unit tests).

mdafsanhossain · 2023-11-29T09:34:28Z

So, what we need here is the CI part, right? Ie: the matrix merging s390x and ppc64le (and stopping at drivers build without testing libsinsp unit tests).

Yes. I am testing it. I will commit in some time

FedeDP · 2023-11-29T09:48:24Z

Yes. I am testing it. I will commit in some time

No problem, it was just to recap :) take your time! And thank you!

poiana · 2023-11-29T14:16:34Z

LGTM label has been added.

Git tree hash: 0aebaf20cc5a04cab1334f71d39b1a8dfc11e95f

Andreagit97

unfortunately, the build is really slow also without the sinsp tests :/ BTW we can think of something

/approve

Andreagit97 · 2023-11-29T14:18:41Z

/hold

Andreagit97 · 2023-11-29T14:18:55Z

waiting for the CI to complete

mdafsanhossain · 2023-11-29T14:20:57Z

Let me add fail-fast: false as the job strategy before merging

FedeDP · 2023-11-29T14:22:07Z

While you are here @mdafsanhossain care to update naming of the jobs from "build-libs-" to build-drivers-? Thanks :D
Wasn't going to bother you with this little change, but since you are already there...

mdafsanhossain · 2023-11-29T14:23:12Z

While you are here @mdafsanhossain care to update naming of the jobs from "build-libs-" to build-drivers-? Thanks :D Wasn't going to bother you with this little change, but since you are already there...

Will do it. No issues :)

Signed-off-by: mdafsanhossain <[email protected]>

FedeDP

/approve

poiana · 2023-11-29T14:48:23Z

LGTM label has been added.

Git tree hash: 92174aa30ca74ec5bdcc889534d01267c54e8b7c

FedeDP · 2023-11-29T15:28:15Z

/unhold

FedeDP · 2023-11-29T15:28:30Z

/hold

FedeDP · 2023-11-29T15:28:48Z

@Andreagit97 didn't see your comment, sorry!

FedeDP · 2023-11-29T15:58:56Z

/unhold

Andreagit97

/approve

poiana · 2023-11-29T16:19:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, mdafsanhossain

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:

~~OWNERS~~ [Andreagit97,FedeDP]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

poiana added kind/feature New feature or request release-note-none dco-signoff: yes area/build area/CI labels Nov 22, 2023

poiana requested review from gnosek and hbrueckner November 22, 2023 06:14

poiana added area/driver-kmod area/driver-bpf area/driver-modern-bpf area/libsinsp area/tests size/M labels Nov 22, 2023

FedeDP reviewed Nov 22, 2023

View reviewed changes

FedeDP changed the title ~~new(CI): Job to build libs on ppc64le arch~~ new(CI): add ppc64le support to old bpf and kmod plus CI job Nov 24, 2023

poiana added this to the next-driver milestone Nov 24, 2023

poiana added size/L and removed size/M labels Nov 28, 2023

FedeDP mentioned this pull request Nov 28, 2023

fix(test/drivers): skip unimplemented syscalls tests. #1517

Merged

mdafsanhossain force-pushed the ci-ppc64le branch from dbd31ac to 716ed99 Compare November 28, 2023 12:56

poiana added the approved label Nov 29, 2023

Andreagit97 previously approved these changes Nov 29, 2023

View reviewed changes

poiana assigned Andreagit97 Nov 29, 2023

poiana added the do-not-merge/hold label Nov 29, 2023

CI:Set fail-fast to false and change s390x/ppc64le job name

5079dae

Signed-off-by: mdafsanhossain <[email protected]>

mdafsanhossain dismissed stale reviews from Andreagit97 and FedeDP via 5079dae November 29, 2023 14:40

poiana removed the lgtm label Nov 29, 2023

poiana requested review from Andreagit97 and FedeDP November 29, 2023 14:40

FedeDP approved these changes Nov 29, 2023

View reviewed changes

poiana added the lgtm label Nov 29, 2023

poiana removed the do-not-merge/hold label Nov 29, 2023

poiana added the do-not-merge/hold label Nov 29, 2023

poiana removed the do-not-merge/hold label Nov 29, 2023

Andreagit97 approved these changes Nov 29, 2023

View reviewed changes

poiana merged commit 9677b66 into falcosecurity:master Nov 29, 2023
30 checks passed

Andreagit97 changed the title ~~new(CI): add ppc64le support to old bpf and kmod plus CI job~~ new(driver): add ppc64le support to old bpf and kmod plus CI job Jan 8, 2024

new(driver): add ppc64le support to old bpf and kmod plus CI job #1497

new(driver): add ppc64le support to old bpf and kmod plus CI job #1497

Conversation

mdafsanhossain commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andreagit97 Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP commented Nov 24, 2023

FedeDP commented Nov 28, 2023

FedeDP commented Nov 28, 2023 • edited Loading

FedeDP commented Nov 28, 2023

mdafsanhossain commented Nov 28, 2023 • edited Loading

FedeDP commented Nov 29, 2023

mdafsanhossain commented Nov 29, 2023

FedeDP commented Nov 29, 2023

poiana commented Nov 29, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

Andreagit97 commented Nov 29, 2023

Andreagit97 commented Nov 29, 2023

mdafsanhossain commented Nov 29, 2023

FedeDP commented Nov 29, 2023 • edited Loading

mdafsanhossain commented Nov 29, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Nov 29, 2023

FedeDP commented Nov 29, 2023

FedeDP commented Nov 29, 2023

FedeDP commented Nov 29, 2023

FedeDP commented Nov 29, 2023

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Nov 29, 2023

Andreagit97 Nov 22, 2023 •

edited

Loading

FedeDP commented Nov 28, 2023 •

edited

Loading

mdafsanhossain commented Nov 28, 2023 •

edited

Loading

FedeDP commented Nov 29, 2023 •

edited

Loading