[WIP] Testing framework shared between all the drivers #783

Andreagit97 · 2022-12-09T11:14:22Z

What type of PR is this?

/kind cleanup

/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 tests

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

Finally, we have the testing framework shared between all the 3 drivers! Right now it supports only the syscalls implemented in the modern probe but every time we add a new syscall in the modern and so a new test we can check that this test runs also for the other 2 drivers.
I know this PR is quite huge, and sorry for that, when I started I didn't imagine it would become something like this 😟 BTW we can think of splitting it in some way not so easy to do but we can find something.

One of the most widespread issues is the conversion from 32-bit signed numbers to 64-bit signed numbers. What we usually do
is:

take a 32-bit signed number like -1 (FFFFFFFF)
put it into an unsigned long (00000000FFFFFFFF)
push it to userspace, which expects an int64 so -1 should be (FFFFFFFFFFFFFFFF)

We have other issues with syscalls that receive data like (recvfrom, recvmsg), because we rely on userspace structs that could be empty so in some cases, we are not able to extract the desired data (you can find more info in every single commit).

When a syscall doesn't behave as we expect I've chosen to skip the test case, but I don't skip it a priori, before doing that, I check that the event sent to userspace is correct, and only at the end I skip the test case, this is the reason why in the old probe or in the kmod you could see some tests SKIPPED

The idea is to run a sort of matrix in CI (I've still to check if all works well in CI), right now I would to run them on an ubuntu 22.04 machine and an ubuntu 20.04 one (only x86, unfortunately, we cannot run these tests in CI with aarch64 and s390x and that is really a pity since I tried locally and they work also on these architectures :( )

Which issue(s) this PR fixes:

Special notes for your reviewer:

I suggest reviewing it commit by commit, every commit should address a particular issue.
The PR is still not ready, I've to check the CI part, but I've tested it locally on the following machines:

5.11.0-051100 (x86)
5.15.0-1023 (x86)
4.14.276 (x86)
5.15.0-1019 (aarch64)
5.11.0-051100 (s390x)

Does this PR introduce a user-facing change?:

NONE

poiana · 2022-12-09T11:14:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97

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]

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

Andreagit97 · 2022-12-09T11:16:56Z

and yes, I didn't change 202 files I've just renamed the external folder, and now we can build tests also without the modern bpf probe, I wrote everything in the README so please take a look :)

Andreagit97 · 2022-12-09T11:45:43Z

Cool it seems that gtest doesn't define GTEST_SKIP() when we use system deps 🙄

hbrueckner · 2022-12-09T11:59:30Z

Hi @Andreagit97,

the PR reads awesome ❤️ ! And my thanks for also testing it on s390x! The s390 CI topic is still on my todo list ;-)

and yes, I didn't change 202 files I've just renamed the external folder, and now we can build tests also without the modern bpf probe, I wrote everything in the README so please take a look :)

I will have a look at it ;-)

Andreagit97 · 2022-12-09T12:44:36Z

The question about build-modern-bpf-arm64 and build-modern-bpf-s390x(point [2] #782) is quite clear here we are using 2 jobs just to build the cpp tests without running them :/

Andreagit97 · 2022-12-09T12:47:40Z

I supposed that ubuntu-20.04 and ubuntu-22.04 runners were different but they run on the same kernel so it's completely useless to have both of them I will remove ubuntu-20.04

Andreagit97

A sort of little self-review

Andreagit97 · 2022-12-09T14:04:14Z

.github/workflows/ci.yml

-  build-and-test-modern-bpf-x86:
-    name: build-and-test-modern-bpf-x86 😇 (bundled_deps)
+  # This job checks that we correctly run `scap-open` for all the 3 drivers.
+  test-scap-open-x86:


This is a test to check all drivers correctly start (only x86, it would be amazing to test also other archs)

Andreagit97 · 2022-12-09T14:05:53Z

CMakeLists.txt

@@ -38,6 +38,7 @@ option(USE_BUNDLED_DEPS "Enable bundled dependencies instead of using the system
 option(MINIMAL_BUILD "Produce a minimal build with only the essential features (no eBPF probe driver, no kubernetes, no mesos, no marathon and no container metadata)" OFF)
 option(MUSL_OPTIMIZED_BUILD "Enable if you want a musl optimized build" OFF)
 option(USE_BUNDLED_DRIVER "Use the driver/ subdirectory in the build process (only available in Linux)" ON)
+option(ENABLE_DRIVERS_TESTS "Enable driver tests (bpf, kernel module, modern bpf)" OFF)


we have to disable it by default otherwise some CI jobs that use system deps will break since the gtest version is different

Andreagit97 · 2022-12-09T14:07:43Z

driver/bpf/fillers.h

@@ -1227,7 +1215,8 @@ FILLER(sys_socketpair_x, true)
 {
 	struct unix_sock *us = NULL;
 	struct sock *speer = NULL;
-	int fds[2] = { 0 };
+	/* In case of failure we send invalid fd (-1) */
+	int fds[2] = {-1, -1};


like in the modern probe

Andreagit97 · 2022-12-09T14:08:56Z

driver/bpf/fillers.h

+	/* Levels different from `SOL_SOCKET` are not supported
+	 * right now.
+	 */
+	if(level != SOL_SOCKET)


the logic is the same as before, i've just removed a nested if

Andreagit97 · 2022-12-09T14:10:48Z

driver/bpf/fillers.h

+		case SO_RCVTIMEO:
+#endif
+#if (defined(SO_RCVTIMEO_OLD) && !defined(SO_RCVTIMEO)) || (defined(SO_RCVTIMEO_OLD) && (SO_RCVTIMEO_OLD != SO_RCVTIMEO))
+		case SO_RCVTIMEO_OLD:
+#endif			
+#if (defined(SO_RCVTIMEO_NEW) && !defined(SO_RCVTIMEO)) || (defined(SO_RCVTIMEO_NEW) && (SO_RCVTIMEO_NEW != SO_RCVTIMEO)) 
+		case SO_RCVTIMEO_NEW:
 #endif
 #ifdef SO_SNDTIMEO
-			case SO_SNDTIMEO:
+		case SO_SNDTIMEO:
 #endif
-				if (bpf_probe_read(&u.tv, sizeof(u.tv), optval))
-					return PPM_FAILURE_INVALID_USER_MEMORY;
-				return bpf_val_to_ring_dyn(data, u.tv.tv_sec * 1000000000 + u.tv.tv_usec * 1000, PT_RELTIME, PPM_SOCKOPT_IDX_TIMEVAL);
+#if (defined(SO_SNDTIMEO_OLD) && !defined(SO_SNDTIMEO)) || (defined(SO_SNDTIMEO_OLD) && (SO_SNDTIMEO_OLD != SO_SNDTIMEO))
+		case SO_SNDTIMEO_OLD:
+#endif
+#if (defined(SO_SNDTIMEO_NEW) && !defined(SO_SNDTIMEO)) || (defined(SO_SNDTIMEO_NEW) && (SO_SNDTIMEO_NEW != SO_SNDTIMEO))
+		case SO_SNDTIMEO_NEW:
+#endif


This is a workaround to avoid 2 case with the same value, for example:
if SO_SNDTIMEO is defined its value will be SO_SNDTIMEO_OLD or SO_SNDTIMEO_NEW so we cannot have a case for each of them

Andreagit97 · 2022-12-09T16:02:02Z

test/drivers/test_suites/syscall_exit_suite/bpf_x.cpp

@@ -0,0 +1,79 @@
+#include "../../event_class/event_class.h"


again this is not a new test case!

Andreagit97 · 2022-12-09T16:04:08Z

test/drivers/test_suites/syscall_exit_suite/bpf_x.cpp

+	if(ret_pid == 0)
+	{
+		/* In this way in the father we know if the call was successful or not. */
+		if(syscall(__NR_bpf, cmd, attr, size) == -1)
+		{
+			/* SUCCESS because we want the call to fail */
+			exit(EXIT_SUCCESS);
+		}
+		else
+		{
+			exit(EXIT_FAILURE);
+		}
+	}


I had to call the bpf syscall from the child because the main process throws lots of bpf and sometimes we caught the wrong event from the ring buffer. With this workaround, we have just one bpf syscall produced by the child

Same for ioctl since the kmod use many ioctl calls

Andreagit97 · 2022-12-09T16:07:15Z

test/drivers/test_suites/syscall_exit_suite/clone3_x.cpp

+	/* Here we scan the parent just to obtain some info for the child */
+	struct proc_info info = {0};
+	pid_t pid = ::getpid();
+	if(!get_proc_info(pid, &info))
+	{
+		FAIL() << "Unable to get all the info from proc" << std::endl;
+	}
+


I have changed the approach in all process syscalls. Now the father collects some data and asserts some parameters of the child. In the previous approach, the child asserted itself but the problem is that now we use a scap_handle and this should not be shared between more than one process for this reason i chose this new method :)

Same for all other process syscalls

Andreagit97 · 2022-12-09T16:09:22Z

test/drivers/test_suites/syscall_exit_suite/ioctl_x.cpp

@@ -0,0 +1,83 @@
+#include "../../event_class/event_class.h"


not a new test!

Andreagit97 · 2022-12-09T16:09:39Z

test/drivers/test_suites/syscall_exit_suite/ioctl_x.cpp

+	uint64_t request = SIOCGIFCOUNT;
+	char* argp = NULL;
+
+	/* Here we need to call the `ioctl` from a child because the main process throws lots of


same approach described in bpf syscall

Andreagit97 · 2022-12-09T16:18:41Z

/hold

Andreagit97 · 2022-12-09T16:18:56Z

/milestone next-driver

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

Andreagit97 · 2023-01-24T16:44:59Z

Probably this is too complex to review in one shot, I will keep this PR open just to show you the final result, but I will split its content into smaller PRs

We merged all the required PRs I think we can close this one :)

hbrueckner · 2023-02-01T10:21:27Z

Hi @Andreagit97 ,
with rebase of #809, I started running the driver_test against the BPF probe. There is yet a different behavior for sys_accept_x where modern BPF tests for successful return and BPF driver always provides information. I would add below to #809 unless you like to have a separate PR for it. Wdyt?

--- a/driver/bpf/fillers.h
+++ b/driver/bpf/fillers.h
@@ -2867,25 +2867,32 @@ FILLER(sys_accept_x, true)
        int res = bpf_val_to_ring_type(data, (s64)fd, PT_FD);
        CHECK_RES(res);
 
-       /* Parameter 2: tuple (type: PT_SOCKTUPLE) */
-       long size = bpf_fd_to_socktuple(data, fd, NULL, 0, false, true, data->tmp_scratch);
-       data->curarg_already_on_frame = true;
-       res = __bpf_val_to_ring(data, 0, size, PT_SOCKTUPLE, -1, false, KERNEL);
-       CHECK_RES(res);
-
        u32 queuelen = 0;
        u32 queuemax = 0;
        u8 queuepct = 0;
 
-       /* Get the listening socket (first syscall parameter) */
-       s32 listening_fd = (s32)bpf_syscall_get_argument(data, 0);
-       struct socket * sock = bpf_sockfd_lookup(data, listening_fd);
-       struct sock *sk = _READ(sock->sk);
-       queuelen = _READ(sk->sk_ack_backlog);
-       queuemax = _READ(sk->sk_max_ack_backlog);
-       if(queuelen && queuemax)
+       if (fd >= 0)
+       {
+               /* Parameter 2: tuple (type: PT_SOCKTUPLE) */
+               long size = bpf_fd_to_socktuple(data, fd, NULL, 0, false, true, data->tmp_scratch);
+               data->curarg_already_on_frame = true;
+               res = __bpf_val_to_ring(data, 0, size, PT_SOCKTUPLE, -1, false, KERNEL);
+               CHECK_RES(res);
+
+               /* Get the listening socket (first syscall parameter) */
+               s32 listening_fd = (s32)bpf_syscall_get_argument(data, 0);
+               struct socket * sock = bpf_sockfd_lookup(data, listening_fd);
+               struct sock *sk = _READ(sock->sk);
+               queuelen = _READ(sk->sk_ack_backlog);
+               queuemax = _READ(sk->sk_max_ack_backlog);
+               if(queuelen && queuemax)
+               {
+                       queuepct = (u8)((u64)queuelen * 100 / queuemax);
+               }
+       }
+       else
        {
-               queuepct = (u8)((u64)queuelen * 100 / queuemax);
+               bpf_push_empty_param(data);
        }
 
        /* Parameter 3: queuepct (type: PT_UINT8) */

Andreagit97 · 2023-02-01T18:02:05Z

Hey @hbrueckner thank you for that! feel free to add these changes into #809 :)

poiana added do-not-merge/work-in-progress kind/cleanup release-note-none kind/feature New feature or request dco-signoff: yes area/build labels Dec 9, 2022

poiana added area/CI approved area/driver-kmod area/driver-bpf area/driver-modern-bpf area/tests size/XXL labels Dec 9, 2022

poiana requested review from gnosek and hbrueckner December 9, 2022 11:15

Andreagit97 force-pushed the test_framework branch from 0cee1dc to 5cbecb1 Compare December 9, 2022 11:35

Andreagit97 force-pushed the test_framework branch 2 times, most recently from 1b17109 to ede6d73 Compare December 9, 2022 12:32

Andreagit97 force-pushed the test_framework branch from ede6d73 to a4ee3f3 Compare December 9, 2022 12:54

Andreagit97 commented Dec 9, 2022

View reviewed changes

poiana added the do-not-merge/hold label Dec 9, 2022

poiana added this to the next-driver milestone Dec 9, 2022

Andreagit97 added 15 commits December 15, 2022 16:56

fix: adapt tests after rebase

4250180

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

fix: quotactl enter event

6852247

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

tests: add a new test to test reading phase from buffer

cfdeb1d

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

tests: add check for unknown engine

eac71cd

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

cleanup: rename test folder

8b4b545

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

chore: adapt cmake to run tests also without the modern bpf probe

c8471d0

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

ci: support drivers tests in CI

71f1aab

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

new: add an helper for pushing empty params

180482a

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

fix: add a missing header for epoll_create1

088802a

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

fix: prlimit64 enter event

c1d40a1

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

tests: fix BPF tests on amazon linux 4.14

57b416c

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

new(ci): test drivers also on ubuntu 20.04

298c51f

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

fix(ci): add a cmake option to compile out drivers tests

4ede2bf

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

doc: fix README and a comment

c63a53d

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

cleanup

14c1dc7

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

Andreagit97 force-pushed the test_framework branch from d2c8a68 to 14c1dc7 Compare December 15, 2022 15:58

fix: reintroduce cmake option after rebase

34dd4e8

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

FedeDP mentioned this pull request Jan 12, 2023

sync: 0.10.1 #827

Merged

hbrueckner mentioned this pull request Jan 13, 2023

new(bpf-driver): Introduce bpf_probe_read_user and bpf_probe_read_kernel variants #809

Merged

Andreagit97 mentioned this pull request Jan 16, 2023

fix(driver): extend modern bpf test framework to all drivers (part 5) #832

Merged

Andreagit97 closed this Jan 24, 2023

Andreagit97 mentioned this pull request Jun 14, 2023

Feat: Support for pidfd_getfd syscall #1145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Testing framework shared between all the drivers #783

[WIP] Testing framework shared between all the drivers #783

Andreagit97 commented Dec 9, 2022

poiana commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

hbrueckner commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 left a comment

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Jan 24, 2023

hbrueckner commented Feb 1, 2023

Andreagit97 commented Feb 1, 2023

[WIP] Testing framework shared between all the drivers #783

[WIP] Testing framework shared between all the drivers #783

Conversation

Andreagit97 commented Dec 9, 2022

poiana commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

hbrueckner commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 left a 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 commented Dec 9, 2022

Andreagit97 commented Dec 9, 2022

Andreagit97 commented Jan 24, 2023

hbrueckner commented Feb 1, 2023

Andreagit97 commented Feb 1, 2023