-
Notifications
You must be signed in to change notification settings - Fork 164
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: implement some syscalls in the modern probe (1/n) #968
new: implement some syscalls in the modern probe (1/n) #968
Conversation
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
/* Parameter 1: res (type: PT_ERRNO) */ | ||
long retval = bpf_syscall_get_retval(data->ctx); | ||
int res = bpf_val_to_ring_type(data, retval, PT_ERRNO); | ||
CHECK_RES(res); | ||
|
||
/* | ||
* data | ||
/* Parameter 2: data (type: PT_BYTEBUF) */ | ||
/* If the syscall doesn't fail we use the return value as `size` | ||
* otherwise we need to rely on the syscall parameter provided by the user. | ||
*/ | ||
if (retval < 0) { | ||
/* | ||
* The operation failed, return an empty buffer | ||
*/ | ||
val = 0; | ||
bufsize = 0; | ||
} else { | ||
val = bpf_syscall_get_argument(data, 1); | ||
|
||
/* | ||
* The return value can be lower than the value provided by the user, | ||
* and we take that into account. | ||
*/ | ||
bufsize = retval; | ||
} | ||
|
||
unsigned long bytes_to_read = retval > 0 ? retval : bpf_syscall_get_argument(data, 2); | ||
unsigned long sent_data_pointer = bpf_syscall_get_argument(data, 1); | ||
data->fd = bpf_syscall_get_argument(data, 0); | ||
res = __bpf_val_to_ring(data, val, bufsize, PT_BYTEBUF, -1, true, USER); | ||
|
||
return res; | ||
return __bpf_val_to_ring(data, sent_data_pointer, bytes_to_read, PT_BYTEBUF, -1, true, USER); |
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.
Refactor to follow the new approach outlined here #942
"write-like" syscalls should always send data to userspace even when they fail
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.
Read through #942, makes sense to me, thanks everyone for sharing the details!
/*=============================== COLLECT PARAMETERS ===========================*/ | ||
|
||
/* Parameter 1: ret (type: PT_UINT64) */ | ||
/* the return value is the program break */ |
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 return value is not the classic errno, it is the address of program break
else | ||
{ | ||
/* Parameter 2: data (type: PT_BYTEBUF) */ | ||
auxmap__store_empty_param(auxmap); | ||
} |
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.
we are in a "read-like" syscall so we send data only in case of success
unsigned long sent_data_pointer = args[1]; | ||
auxmap__store_bytebuf_param(auxmap, sent_data_pointer, bytes_to_read, USER); |
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.
"write-like" syscall, we always send data!
if (unlikely(res != PPM_SUCCESS)) | ||
return res; | ||
res = val_to_ring(args, sent_data_pointer, bufsize, true, 0); | ||
CHECK_RES(res); |
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.
Dual of the bpf refactor
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.
@hbrueckner could you double-check on all the socketcall refactor, ty!
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.
@Andreagit97 @incertum reviewed this PR and socketcall changes look good.
#include "../../event_class/event_class.h" | ||
|
||
#ifdef __NR_recv | ||
TEST(SyscallExit, recvX_fail) |
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.
this syscall is never called in our supported architectures, I've added just one test to check if the filler is correctly called. If one day we will support an arch with this syscall we could add further tests.
Same for send
syscall
/* Please note: | ||
* the syscall `ugetrlimit` is mapped to `PPME_SYSCALL_GETRLIMIT_E` event | ||
* like `getrlimit`. The same BPF program will be used for both the syscalls. | ||
*/ |
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.
right now we don't have a dedicated event pair for ugetrlimit
so we use the same fillers of getrlimit
. This issue is tracked here #911
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.
LGTM!
BTW I honestly think that the drivers test suites is possibly the coolest thing that happened in the last 6 months! Kudos @Andreagit97
That plus the fact that we now enforce that every syscall added must be added with its own tests, it's an incredible step forward.
/approve
LGTM label has been added. Git tree hash: 2cb4d6a91d314858894cd4bb4abefa10c58397e2
|
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.
Hi @Andreagit97
excellent work as always! Thanks for taking care on the socketcall integration and respective tests for recv
and send
.
/approve
if (unlikely(res != PPM_SUCCESS)) | ||
return res; | ||
res = val_to_ring(args, sent_data_pointer, bufsize, true, 0); | ||
CHECK_RES(res); |
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.
@Andreagit97 @incertum reviewed this PR and socketcall changes look good.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, FedeDP, hbrueckner 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 |
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
This PR is part of a series that aims to address 👉 #723
More in detail this PR implements:
brk
getrlimit
ugetrlimit
(used only on arm 32 bit)send
(used only on arm 32 bit)recv
(used only on arm 32 bit)Which issue(s) this PR fixes:
Part of #723
Special notes for your reviewer:
Does this PR introduce a user-facing change?: