update(modern_bpf): reduce the `execve` instrumentation time with new APIs #648

Andreagit97 · 2022-10-02T17:35:15Z

What type of PR is this?

/kind cleanup

/kind feature

Any specific area of the project related to this PR?

/area driver-modern-bpf

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

Playing with https://github.com/Andreagit97/BPF-perftool we noticed that the execveat instrumentation time was strangely high compared to the old probe one:

old probe     new probe

12140 ns      16347 ns

This overhead seems related to the use of extract__charbuf_pointer_from_array inline function, for some reason clang increases the complexity of the programs causing overhead in the prog execution. With the new APIs auxmap__store_execve_exe and auxmap__store_execve_args I've recorded these new data:

old probe     new probe

12140 ns      10518 ns

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

FedeDP

Cool!
/approve

poiana · 2022-10-03T06:46:49Z

LGTM label has been added.

Git tree hash: 6210ba75c3a3c29c0dbbf33c60a4e419f49a50ee

hbrueckner · 2022-10-04T15:57:12Z

@Andreagit97 , very nice tooling for measurements. I will have a look at it. I left a comment on #633 regarding the args/env max discussion. So far, you change here looks nice.

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c

Andreagit97 · 2022-10-15T14:06:49Z

Thanks to @incertum fixes I've added also 2 tests for execve and execveat success cases to avoid future issues like the one reported here #633

incertum

Thanks @Andreagit97 LGTM

driver/modern_bpf/helpers/store/auxmap_store_params.h

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c

driver/modern_bpf/helpers/store/auxmap_store_params.h

hbrueckner

Hi @Andreagit97 , I have spot some places for reviewing the right constant. I will give this PR a try later this week. Thanks.

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/fsconfig.bpf.c

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/mount.bpf.c

test/modern_bpf/test_suites/syscall_exit_suite/execveat_x.cpp

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

Andreagit97 · 2022-10-24T15:00:32Z

hey @hbrueckner the problem is even worst, the kmod is not able to catch the clone child event while the modern bpf probe yes, but the tracepoint is exactly the same 🤔

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

hbrueckner · 2022-10-25T09:25:29Z

Hi @Andreagit97

hey @hbrueckner the problem is even worst, the kmod is not able to catch the clone child event while the modern bpf probe yes, but the tracepoint is exactly the same thinking

This indeed looks very strange. Also had a look add kernel driver and kernel sources, and agree they basically use the same tracepoint_probe_register() functions. Regarding the modern bpf and related EFAULT, one option would be to try a sleepable BPF program for clone family and use bpf_copy_from_user() which might sleep (not sure if that will work or perform at all). However, I did not catch the right place to add the BPF_F_SLEEPABLE program attribute.

hbrueckner · 2022-10-25T09:33:47Z

Hi @Andreagit97,

thanks for the update and, yeah, also agree on sticking with sched_exec_fork for now until we have more clarity on the strange behavior. Perhaps, worth to open a separate issue to track it? wdyt?

Andreagit97 · 2022-10-25T09:34:39Z

Hi @Andreagit97

hey @hbrueckner the problem is even worst, the kmod is not able to catch the clone child event while the modern bpf probe yes, but the tracepoint is exactly the same thinking

This indeed looks very strange. Also had a look add kernel driver and kernel sources, and agree they basically use the same tracepoint_probe_register() functions. Regarding the modern bpf and related EFAULT, one option would be to try a sleepable BPF program for clone family and use bpf_copy_from_user() which might sleep (not sure if that will work or perform at all). However, I did not catch the right place to add the BPF_F_SLEEPABLE program attribute.

I've just pushed a possible solution. In ARM we already need to catch the clone/clone3 child events from another tracepoint (sched_process_fork) since there is an issue in the sys_exit_tracepoint, in the past, I've seen this issue also in old s390x machines for this reason we have this workaround also in our kmod that supports s390x. So I propose to use also for s390x this different tracepoint since we already need it for arm64. Now all the tests should pass and we should be able to collect all fork/clone/clone3/vfork events without EFAULT problems since the child event is generated by the parent process. Let me know if you have some doubts or other suggestions @FedeDP @hbrueckner

Sorry for the PR dimension this is quite huge but there are a lot of tests inside... I still need to address this comment #648 (comment)

Hi @Andreagit97,

thanks for the update and, yeah, also agree on sticking with sched_exec_fork for now until we have more clarity on the strange behavior. Perhaps, worth to open a separate issue to track it? wdyt?

Agree on opening the issue, the best solution would be to find another way to take the args from the kernel but i would address it in another PR probably as you suggested :)

FedeDP · 2022-10-25T09:38:09Z

've just pushed a possible solution. In ARM we already need to catch the clone/clone3 child events from another tracepoint (sched_process_fork) since there is an issue in the sys_exit_tracepoint, in the past, I've seen this issue also in old s390x machines for this reason we have this workaround also in our kmod that supports s390x. So I propose to use also for s390x this different tracepoint since we already need it for arm64. Now all the tests should pass and we should be able to collect all fork/clone/clone3/vfork events without EFAULT problems since the child event is generated by the parent process. Let me know if you have some doubts or other suggestions @FedeDP @hbrueckner

Fully agree with the solution; simple and effective!

Andreagit97 · 2022-10-25T09:40:03Z

the good news is that this PR should be the last one to have a full working implementation of the modern bpf probe 🥳

hbrueckner · 2022-10-25T09:44:38Z

've just pushed a possible solution. In ARM we already need to catch the clone/clone3 child events from another tracepoint (sched_process_fork) since there is an issue in the sys_exit_tracepoint, in the past, I've seen this issue also in old s390x machines for this reason we have this workaround also in our kmod that supports s390x. So I propose to use also for s390x this different tracepoint since we already need it for arm64. Now all the tests should pass and we should be able to collect all fork/clone/clone3/vfork events without EFAULT problems since the child event is generated by the parent process. Let me know if you have some doubts or other suggestions @FedeDP @hbrueckner

Fully agree with the solution; simple and effective!

Agreed too! Thanks a lot @Andreagit97 !

hbrueckner · 2022-10-25T09:46:37Z

Hi @Andreagit97

Sorry for the PR dimension this is quite huge but there are a lot of tests inside... I still need to address this comment #648 (comment)

No problem ... I will need some time for a deeper review on the recent changes ... and, awesome that we now have a full working modern bpf probe!

hbrueckner · 2022-10-25T13:54:03Z

driver/modern_bpf/helpers/store/auxmap_store_params.h

@@ -39,6 +39,9 @@
 /* Proc name */
 #define MAX_PROC_EXE 4096

+/* Task comm */
+#define MAX_TASK_COMM 16


Hi @Andreagit97 ,
for reference, here is the link to the kernel source as background: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/sched.h#n286

Reading it a bit more carefully, the len has been redefined as enum to make it available to BPF programs with commit:

commit 3087c61ed2c48548b74dd343a5209b87082c682d Author: Yafang Shao <[email protected]> Date: Wed Jan 19 18:08:40 2022 -0800 tools/testing/selftests/bpf: replace open-coded 16 with TASK_COMM_LEN As the sched:sched_switch tracepoint args are derived from the kernel, we'd better make it same with the kernel. So the macro TASK_COMM_LEN is converted to type enum, then all the BPF programs can get it through BTF. The BPF program which wants to use TASK_COMM_LEN should include the header vmlinux.h. Regarding the test_stacktrace_map and test_tracepoint, as the type defined in linux/bpf.h are also defined in vmlinux.h, so we don't need to include linux/bpf.h again.

For s390x, it is already included in the generated driver/modern_bpf/definitions/vmlinux.h definitions. So question here is whether we keep it that way or update the definitions and use directly the enum? Wdyt?

P.S. the remaining changes looks good to me!

yes I think we can add it manually to x86 and aarch64 without updating all the vmlinux files and use this already defined enum, btw great catch this saves us an additional macro :)

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Hendrik Brueckner <[email protected]>

Andreagit97 · 2022-10-25T14:46:33Z

Tested on all three architectures (x86, aarch64, s390x) and it should work!

hbrueckner · 2022-10-25T15:15:08Z

Hi @Andreagit97 ,

Tested on all three architectures (x86, aarch64, s390x) and it should work!

Confirmed for s390x:

[----------] Global test environment tear-down
[==========] 205 tests from 3 test suites ran. (131 ms total)
[  PASSED  ] 205 tests.

Further, I went through your commits and they are fine!

@Andreagit97 Incredible and awesome work as usual!

/lgtm

poiana · 2022-10-25T15:15:14Z

LGTM label has been added.

Git tree hash: 37fc8e72aaa5b99bbcef12263541fdaf6a073d6b

leogr

LGTM

FedeDP

/approve

poiana · 2022-10-25T15:41:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, hbrueckner, 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:

~~OWNERS~~ [Andreagit97,FedeDP,leogr]

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

Andreagit97 · 2022-10-26T15:13:57Z

This PR completes #513

FedeDP · 2022-12-02T13:54:30Z

/milestone 4.0.0+driver

poiana added kind/cleanup release-note-none dco-signoff: yes kind/feature New feature or request area/driver-modern-bpf labels Oct 2, 2022

poiana requested review from hbrueckner and Molter73 October 2, 2022 17:35

poiana added approved size/L labels Oct 2, 2022

Andreagit97 added this to the 0.10.0 milestone Oct 2, 2022

FedeDP previously approved these changes Oct 3, 2022

View reviewed changes

poiana assigned FedeDP Oct 3, 2022

poiana added the lgtm label Oct 3, 2022

hbrueckner mentioned this pull request Oct 4, 2022

fix(driver-modern-bpf): fix execve USER vs MEMORY reads + cleanup + enforce upper bounds in push__bytebuf #633

Merged

hbrueckner reviewed Oct 5, 2022

View reviewed changes

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c Show resolved Hide resolved

Andreagit97 dismissed FedeDP’s stale review via c9388df October 15, 2022 13:58

Andreagit97 force-pushed the execve_instrumentation_time branch from 3bcbd53 to c9388df Compare October 15, 2022 13:58

poiana removed the lgtm label Oct 15, 2022

poiana requested a review from FedeDP October 15, 2022 13:58

poiana added size/XL and removed size/L labels Oct 15, 2022

incertum reviewed Oct 16, 2022

View reviewed changes

driver/modern_bpf/helpers/store/auxmap_store_params.h Show resolved Hide resolved

driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/execve.bpf.c Show resolved Hide resolved

hbrueckner reviewed Oct 18, 2022

View reviewed changes

driver/modern_bpf/helpers/store/auxmap_store_params.h Outdated Show resolved Hide resolved

hbrueckner reviewed Oct 18, 2022

View reviewed changes

test/modern_bpf/test_suites/syscall_exit_suite/execveat_x.cpp Outdated Show resolved Hide resolved

Andreagit97 mentioned this pull request Oct 20, 2022

[Tracking] Params inconsistencies in our drivers #515

Open

59 tasks

update(modern_bpf): reduce the execve instrumentation time with new…

0159103

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

Andreagit97 added 3 commits October 24, 2022 20:59

new(driver): implement CAPTURE_SCHED_PROC_EXEC logic

5eb1b66

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

new(driver): implement CAPTURE_SCHED_PROC_FORK logic

145b691

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

update(userspace): support execveat exit event for s390x

9288fb8

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

hbrueckner reviewed Oct 25, 2022

View reviewed changes

update(driver): add the TASK_COMM_LEN enum in vmlinux (x86, aarch64)

ecd21b0

Signed-off-by: Andrea Terzolo <[email protected]> Co-authored-by: Hendrik Brueckner <[email protected]>

Andreagit97 force-pushed the execve_instrumentation_time branch from 542524d to ecd21b0 Compare October 25, 2022 14:37

poiana assigned hbrueckner Oct 25, 2022

poiana added the lgtm label Oct 25, 2022

leogr approved these changes Oct 25, 2022

View reviewed changes

poiana assigned leogr Oct 25, 2022

FedeDP approved these changes Oct 25, 2022

View reviewed changes

poiana merged commit fbc44ec into master Oct 25, 2022

poiana deleted the execve_instrumentation_time branch October 25, 2022 15:42

poiana modified the milestones: 0.10.0, 4.0.0+driver Dec 2, 2022

hbrueckner mentioned this pull request Feb 6, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update(modern_bpf): reduce the `execve` instrumentation time with new APIs #648

update(modern_bpf): reduce the `execve` instrumentation time with new APIs #648

Andreagit97 commented Oct 2, 2022 •

edited

Loading

FedeDP left a comment

poiana commented Oct 3, 2022

hbrueckner commented Oct 4, 2022

Andreagit97 commented Oct 15, 2022 •

edited

Loading

incertum left a comment

hbrueckner left a comment

Andreagit97 commented Oct 24, 2022

hbrueckner commented Oct 25, 2022

hbrueckner commented Oct 25, 2022

Andreagit97 commented Oct 25, 2022 •

edited

Loading

FedeDP commented Oct 25, 2022

Andreagit97 commented Oct 25, 2022

hbrueckner commented Oct 25, 2022

hbrueckner commented Oct 25, 2022

hbrueckner Oct 25, 2022

Andreagit97 Oct 25, 2022

Andreagit97 Oct 25, 2022

Andreagit97 commented Oct 25, 2022

hbrueckner commented Oct 25, 2022

poiana commented Oct 25, 2022

leogr left a comment

FedeDP left a comment

poiana commented Oct 25, 2022

Andreagit97 commented Oct 26, 2022

FedeDP commented Dec 2, 2022

update(modern_bpf): reduce the execve instrumentation time with new APIs #648

update(modern_bpf): reduce the execve instrumentation time with new APIs #648

Conversation

Andreagit97 commented Oct 2, 2022 • edited Loading

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Oct 3, 2022

hbrueckner commented Oct 4, 2022

Andreagit97 commented Oct 15, 2022 • edited Loading

incertum left a comment

Choose a reason for hiding this comment

hbrueckner left a comment

Choose a reason for hiding this comment

Andreagit97 commented Oct 24, 2022

hbrueckner commented Oct 25, 2022

hbrueckner commented Oct 25, 2022

Andreagit97 commented Oct 25, 2022 • edited Loading

FedeDP commented Oct 25, 2022

Andreagit97 commented Oct 25, 2022

hbrueckner commented Oct 25, 2022

hbrueckner commented Oct 25, 2022

hbrueckner Oct 25, 2022

Choose a reason for hiding this comment

Andreagit97 Oct 25, 2022

Choose a reason for hiding this comment

Andreagit97 Oct 25, 2022

Choose a reason for hiding this comment

Andreagit97 commented Oct 25, 2022

hbrueckner commented Oct 25, 2022

poiana commented Oct 25, 2022

leogr left a comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Oct 25, 2022

Andreagit97 commented Oct 26, 2022

FedeDP commented Dec 2, 2022

update(modern_bpf): reduce the `execve` instrumentation time with new APIs #648

update(modern_bpf): reduce the `execve` instrumentation time with new APIs #648

Andreagit97 commented Oct 2, 2022 •

edited

Loading

Andreagit97 commented Oct 15, 2022 •

edited

Loading

Andreagit97 commented Oct 25, 2022 •

edited

Loading