fix(driver): manage syscalls only defined with socketcall #1128

Andreagit97 · 2023-05-31T09:42:24Z

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area driver-kmod

/area tests

Does this PR require a change in the driver versions?

Already bumped both patch versions

What this PR does / why we need it:

Please note: the following changes regard only the kernel module, at least at the moment, since it is the unique driver which supports 32-bit syscall. We will need to find a valid solution for all the drivers in the next future:

As explained in the code there are cases in which given a specific socketcall code we are not able to obtain the corresponding syscall code. There are 2 possibilities:

The user provided the wrong socket call code. In this case, we will send a generic event.
The socket call code is defined but the corresponding syscall call is not defined in the system. For example on s390x machines SYS_ACCEPT is defined but __NR_accept is not. In this case, instead of converting to the syscall code, we will convert to the corresponding event. The only difference in converting to the event is that we lose the capability to use the simple consumer (the simple consumer logic is syscall driven...).
Let's imagine the case in which we receive an SYS_ACCEPT socketcall code on s390x. We will try to convert it to __NR_accept but this is not defined in the system. For this reason, we will covert it to PPME_SOCKET_ACCEPT_5_E/X. Note that the syscall id is not changed to __NR_accept, we cannot do that as we said, so we keep the __NR_socketcall id. Now when we face the simple consumer logic we will decide to keep this event looking at the __NR_socketcall code. If it is interesting we will keep this event, if not we will drop the event.
TL;DR; in these corner cases in which we need to convert the socketcall code to an event code we use the __NR_socketcall code as a knob to keep or drop these events.

Known cases in which the socket call code is defined but the corresponding syscall code is not:

s390x

SYS_ACCEPT is defined but __NR_accept is not defined

x86 with CONFIG_IA32_EMULATION

SYS_ACCEPT is defined but __NR_accept is not defined
SYS_SEND is defined but __NR_send is not defined
SYS_RECV is defined but __NR_recv is not defined

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(driver): manage syscalls only defined with socketcall

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Federico Di Pierro <[email protected]>

Signed-off-by: Andrea Terzolo <[email protected]>

FedeDP · 2023-05-31T10:05:41Z

driver/main.c

+			/* we need to use `return_code + 1` because return_code
+			 * is the enter event.
+			 */
+			record_event_all_consumers(return_code + 1, return_code == PPME_GENERIC_E ? UF_ALWAYS_DROP : UF_USED, &event_data, KMOD_PROG_SYS_EXIT);


Missing return statement?

Btw i'd avoid this at all, just setting data to be used by the record_event_all_consumers at the end of the function. Same for sys enter.

Great catch!

FedeDP · 2023-05-31T10:06:46Z

driver/socketcall_to_syscall.c

+#define SYS_RECVMMSG 19	  /* sys_recvmmsg(2)		*/
+#define SYS_SENDMMSG 20	  /* sys_sendmmsg(2)		*/
+
+int socketcall_code_to_syscall_code(int socketcall_code, bool* is_syscall_return)


This feels weird; still, we will need to come back at this impl later, therefore no blocker for me.

agree this is just a patch needed because we are near the release, we need to find something better for all the engines

FedeDP · 2023-05-31T10:10:26Z

test/drivers/test_suites/syscall_enter_suite/socketcall_e.cpp

@@ -362,17 +370,21 @@ TEST(SyscallEnter, socketcall_acceptE)
 	evt_test->assert_header();

 #ifdef __s390x__
-	/* socketcall uses accept4 event for SYS_ACCEPT */
+	if(!evt_test->is_kmod_engine())


We are not testing kmod on s390x here, right?

yep, we are testing it, in the else case of this ifdef

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Federico Di Pierro <[email protected]>

FedeDP

/approve

poiana · 2023-05-31T12:53:32Z

LGTM label has been added.

Git tree hash: 144068c418c4d878c9bdcc84e441398c1a1574ec

LucaGuerra · 2023-05-31T12:54:46Z

driver/socketcall_to_syscall.c

+		return PPME_SOCKET_ACCEPT_5_E;
+
+	case SYS_ACCEPT4:
+		return PPME_SOCKET_ACCEPT4_5_E;


ACCEPT4_5 or ACCEPT4_6 or it makes no difference?

Great catch!

Great catch Luca !

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Luca Guerra <[email protected]>

LucaGuerra

LGTM! Thanks for this fix!

poiana · 2023-05-31T13:16:35Z

LGTM label has been added.

Git tree hash: 5dc27973805355895a8209ec8ff748418b015534

FedeDP

/approve

poiana · 2023-05-31T13:18:15Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,LucaGuerra]

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

poiana added release-note kind/bug Something isn't working dco-signoff: yes area/driver-kmod area/tests approved labels May 31, 2023

poiana requested review from hbrueckner and Molter73 May 31, 2023 09:42

poiana added the size/XXL label May 31, 2023

Andreagit97 and others added 5 commits May 31, 2023 11:42

fix: socketcall on 32 bits

3a7d102

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Federico Di Pierro <[email protected]>

cleanup

890ceb3

Signed-off-by: Andrea Terzolo <[email protected]>

fix: check first the 32 bit mode

1e75499

Signed-off-by: Andrea Terzolo <[email protected]>

tests: update socketcall tests

7542d67

Signed-off-by: Andrea Terzolo <[email protected]>

cleanup

87bed4b

Signed-off-by: Andrea Terzolo <[email protected]>

Andreagit97 force-pushed the fix_32_socketcalls_2 branch from f690ff8 to 87bed4b Compare May 31, 2023 09:42

Andreagit97 modified the milestones: 0.11.0, 5.0.1+driver May 31, 2023

FedeDP reviewed May 31, 2023

View reviewed changes

Andreagit97 and others added 2 commits May 31, 2023 13:29

fix: add a missing return

837b64d

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Federico Di Pierro <[email protected]>

update: add socketcall to simple consumer logic

99afd71

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Federico Di Pierro <[email protected]>

FedeDP previously approved these changes May 31, 2023

View reviewed changes

poiana assigned FedeDP May 31, 2023

poiana added the lgtm label May 31, 2023

LucaGuerra reviewed May 31, 2023

View reviewed changes

fix: ACCEPT4_6 code

aaac442

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Luca Guerra <[email protected]>

Andreagit97 dismissed FedeDP’s stale review via aaac442 May 31, 2023 12:58

poiana removed the lgtm label May 31, 2023

poiana requested a review from FedeDP May 31, 2023 12:58

LucaGuerra approved these changes May 31, 2023

View reviewed changes

poiana assigned LucaGuerra May 31, 2023

poiana added the lgtm label May 31, 2023

FedeDP approved these changes May 31, 2023

View reviewed changes

poiana merged commit 6396838 into master May 31, 2023

poiana deleted the fix_32_socketcalls_2 branch May 31, 2023 13:23

This was referenced May 31, 2023

chore: sync release branch 0.11.x #1129

Merged

[TRACKING] Libs Release (0.11.1+5.0.1+driver) #1092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(driver): manage syscalls only defined with socketcall #1128

fix(driver): manage syscalls only defined with socketcall #1128

Andreagit97 commented May 31, 2023

FedeDP May 31, 2023

FedeDP May 31, 2023

Andreagit97 May 31, 2023

FedeDP May 31, 2023

Andreagit97 May 31, 2023

FedeDP May 31, 2023

Andreagit97 May 31, 2023

FedeDP left a comment

poiana commented May 31, 2023

LucaGuerra May 31, 2023

Andreagit97 May 31, 2023

FedeDP May 31, 2023

LucaGuerra left a comment

poiana commented May 31, 2023

FedeDP left a comment

poiana commented May 31, 2023

fix(driver): manage syscalls only defined with socketcall #1128

fix(driver): manage syscalls only defined with socketcall #1128

Conversation

Andreagit97 commented May 31, 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

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented May 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaGuerra left a comment

Choose a reason for hiding this comment

poiana commented May 31, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented May 31, 2023