-
Notifications
You must be signed in to change notification settings - Fork 165
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): ia32 emulation bpf drivers support #1196
Conversation
Please double check driver/SCHEMA_VERSION file. See versioning. /hold |
@@ -1339,50 +1339,4 @@ static __always_inline int bpf_val_to_ring_type(struct filler_data *data, | |||
return __bpf_val_to_ring(data, val, 0, type, -1, false, param_type_to_mem(type)); | |||
} | |||
|
|||
static __always_inline bool bpf_in_ia32_syscall() |
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.
Just moved to plumbing_helpers
@@ -7,6 +7,10 @@ | |||
|
|||
#include <helpers/interfaces/syscalls_dispatcher.h> | |||
#include <helpers/interfaces/attached_programs.h> | |||
#include <bpf/bpf_helpers.h> | |||
|
|||
#define X86_64_NR_EXECVE 59 |
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.
Can we use the __NR_*
macros here?
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.
assuming building for 64bit, then I think you can use them.
|
||
#include "ppm_events_public.h" | ||
|
||
const int g_ia32_64_map[SYSCALL_TABLE_SIZE] = { |
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 file contains the mapping between a 32bit syscall number (array index) and its 64bit value.
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.
It is autogenerated by syscalls-bumper; i can add a comment explaining what the map does ;)
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.
Added the comment.
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.
Also, as discussed privately with @Andreagit97 , in the future we might push a generic syscall32
event when the 32bit syscalls have no 64bit mapping (the -1
values in the array).
Or we could push a syscall
generic event adding a new bool is32bit
parameter.
I'll add this to the PR body future work.
07a43f2
to
baab5a7
Compare
Fixed old
(I added an |
driver/API_VERSION
Outdated
@@ -1 +1 @@ | |||
4.0.2 | |||
5.0.0 |
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 added a new map for bpf drivers. This is a maj break.
driver/bpf/plumbing_helpers.h
Outdated
args_pointer = bpf_syscall_get_argument_from_ctx(ctx, 1); | ||
bpf_probe_read_user(&arg, sizeof(unsigned long), (void*)(args_pointer + (idx * sizeof(unsigned long)))); | ||
bpf_probe_read_user(&arg, size, (void*)(args_pointer + (idx * size))); |
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.
Adjust read size if we are on a 32bit syscall.
driver/bpf/plumbing_helpers.h
Outdated
#ifdef CAPTURE_SOCKETCALL | ||
if(bpf_syscall_get_nr(data->ctx) == __NR_socketcall) | ||
#if defined(CAPTURE_SOCKETCALL) || (defined(CONFIG_X86_64) && defined(CONFIG_IA32_EMULATION)) | ||
if(bpf_syscall_get_nr(data->ctx) == data->state->tail_ctx.socketcall_syscall_id) |
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 can be __NR_socketcall
or __NR_ia32_socketcall
.
@@ -634,6 +725,8 @@ static __always_inline void call_filler(void *ctx, | |||
|
|||
++state->n_evts; | |||
|
|||
state->tail_ctx.socketcall_syscall_id = socketcall_syscall_id; |
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.
Store this in our tail_ctx
so that it is made available to bpf_syscall_get_argument
.
Implemented x86 ia32 support for
|
Ok. 😆 |
14a5b12
to
5daacdc
Compare
Signed-off-by: Federico Di Pierro <[email protected]>
…an exit event. Signed-off-by: Federico Di Pierro <[email protected]>
…since it may have different values. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
847b05b
to
31a142b
Compare
Signed-off-by: Federico Di Pierro <[email protected]>
[1] re
hmmm yes a bit mysterious https://docs.kernel.org/bpf/verifier.html#register-value-tracking [2] re below, do we still somehow index an array or so without checking bounds first?
[STATUS] FAILED /libs/test/vm/build/driver/clang-12/5.10.9-1.el7.elrepo.x86_64 [3] re
Can we split? |
Yep, we have already been doing that; no luck for now :(
No idea :(
Yep, i'd do that in a follow up PR though! @Andreagit97 is going to push the fix for the modern bpf verifier issue soon! We are just left with the old bpf verifier issue; IMHO we could even track it in an issue and go on, but i am willing to invest a little bit of time still to try to fix it. |
Moving `extract__network_args` before the ringbuf initialization keeps the `ringbuf` variable into BPF registers. Signed-off-by: Andrea Terzolo <[email protected]>
…ts are not available. This limitation was already set in stone on master. Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
…xes. Signed-off-by: Federico Di Pierro <[email protected]>
Me and @Andreagit97 fixed old bpf verifier and modern bpf verifier issues!! 🚀 Final matrix (same as on master):
Since i mentioned before, here is the list of syscalls that are ia32 only (ie; are unmapped on x86_64 and we will discard them):
I updated syscalls-bumper PR to properly add the syscall name for unmapped syscalls in the new table; moreover, since |
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.
Amazing work! If the kernel testing is green I think we can merge it!
LGTM, I have some commit here so I would avoid approving!
Kernel testing is super 🟢 :D |
The SCHEMA_VERSION failure is a false positive here |
👍 /hold cancel |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP, hbrueckner, jasondellaluce, leogr 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
/area libscap-engine-bpf
/area libscap-engine-modern-bpf
Does this PR require a change in the driver versions?
/version driver-API-version-major
What this PR does / why we need it:
This PR introduces support for x86_64 ia32 emulation for bpf drivers.
Note: we only really supported compat executables for x86_64; this is now more explicit in the code.
This PR aims at feature parity for bpf drivers with kmod.
Future work, that won't be addressed by this PR:
Add compat support for other supported architectures (? is this needed at all?)Find a way to run drivers test as compat executables-> see new(driver): ia32 emulation bpf drivers support #1196 (comment)syscall32
generic event when the 32bit syscall has no 64bit mapping (the-1
values in the mapping array). Or add a newis32bit bool
parameter to genericsyscall
event.Still TODO:
socketcall
for x86_64 ia32 for bpf drivers. This is unfortunately much harder than i thoughtWhich issue(s) this PR fixes:
Fixes #279
Special notes for your reviewer:
Most of the line changes are from the new table (driver/syscall_ia32_64_map.c) that is ~500 locs.
The PR is not really big, main idea is to convert the 32bit syscalls to 64bit ones, so that we can normally manage them without the need of a 32bit syscall_table (as we used to do in our kmod).
As a side effect, we now support filtering interesting syscalls for 32bit syscalls too, with no changes (given that they are translated to 64bit syscalls and our current interesting syscalls impl uses 64bit syscalls!)
To test (both normal syscall and socketcall), please use this small C script:
Built with
gcc ia32.c -o ia32 -m32
.Then run
sinsp-example
like:Example output for
kmod:
modern bpf:
bpf:
As you can see, only kmod receives the ia32 socketcall. As i said previously,socketcall
is very hard because it is not present on x86_64, but it is present on x86_32; therefore:we are not able to map the 32bit to 64bit through our table and we need to manage it manually (see kmod main.c enter and exit probes as an example)CurrentlyCAPTURE_SOCKETCALL
(feature_gates.h) is only enabled for s390x, and it is used by bpf and modern bpf. Enabling it for x86_64 too breaks the build, because x86_64 has no__NR_socketcall
syscall(SOCKETCALL SUPPORT WAS INDEED IMPLEMENTED IN ALL 3 DRIVERS for x86. See below comments:
Does this PR introduce a user-facing change?: