Skip to content

fix(userspace/libpman): fix modern ebpf failures in master kernel-testing #2282

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 17, 2025
Merged

fix(userspace/libpman): fix modern ebpf failures in master kernel-testing #2282

merged 1 commit into from
Feb 17, 2025

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Feb 14, 2025 •
edited
Loading

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area driver-modern-bpf
/area libpman

Does this PR require a change in the driver versions?

What this PR does / why we need it:

Master kernel-testing did break on multiple modern ebpf kernels since #2255. See the comment on that PR: #2255 (comment)
Checking the edits on that comment, it seems like the culprit commit is 3e0e122.

Here the issue i think lies in the usage of BPF_PROG_TYPE_TRACING; indeed in pman_check_support we have a fallback for it:

        res = libbpf_probe_bpf_prog_type(BPF_PROG_TYPE_TRACING, NULL) > 0;
	if(!res) {
		// The above function checks for the `BPF_TRACE_FENTRY` attach type presence, while we need
		// to check for the `BPF_TRACE_RAW_TP` one. If `BPF_TRACE_FENTRY` is defined we are
		// sure `BPF_TRACE_RAW_TP` is defined as well, in all other cases, we need to search
		// for it in the `vmlinux` file.
		res = probe_BPF_TRACE_RAW_TP_type();
		if(!res) {
			// Clear the errno for `pman_print_error`
			errno = 0;
			pman_print_error("prog 'BPF_TRACE_RAW_TP' is not supported");
			return res;
		}
	}

I think that what's happening is that the check:

libbpf_probe_bpf_helper(BPF_PROG_TYPE_TRACING, progs[idx].feat, NULL) == 0)

in lifecycle.c is failing because it cannot find BPF_PROG_TYPE_TRACING (and we don't have any fallback in that case).
Since it works fine too using BPF_PROG_TYPE_RAW_TRACEPOINT, revert to use it instead.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@poiana
Copy link
Contributor

poiana commented Feb 14, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FedeDP

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 14, 2025 •
edited
Loading

Perf diff from master - unit tests

     4.94%     +0.51%  [.] sinsp_evt::get_type
     4.09%     -0.43%  [.] next_event_from_file
    18.27%     -0.38%  [.] sinsp_threadinfo::get_main_thread
     2.06%     -0.33%  [.] sinsp_thread_manager::get_thread_ref
     9.47%     +0.32%  [.] sinsp_thread_manager::create_thread_dependencies
     7.14%     -0.25%  [.] sinsp::next
     0.78%     +0.20%  [.] sinsp::fetch_next_event
     1.36%     +0.17%  [.] is_conversion_needed
     0.39%     +0.15%  [.] sinsp_parser::parse_context_switch
     0.25%     +0.14%  [.] copy_and_sanitize_path

Heap diff from master - unit tests

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Benchmarks diff from master

Comparing gbench_data.json to /root/actions-runner/_work/libs/libs/build/gbench_data.json
Benchmark                                                         Time             CPU      Time Old      Time New       CPU Old       CPU New
----------------------------------------------------------------------------------------------------------------------------------------------
BM_sinsp_split_mean                                            +0.0215         +0.0214           147           150           147           150
BM_sinsp_split_median                                          +0.0183         +0.0183           147           150           147           150
BM_sinsp_split_stddev                                          +0.1729         +0.1737             2             2             2             2
BM_sinsp_split_cv                                              +0.1482         +0.1490             0             0             0             0
BM_sinsp_concatenate_paths_relative_path_mean                  +0.0683         +0.0682            56            60            56            60
BM_sinsp_concatenate_paths_relative_path_median                +0.0618         +0.0616            56            59            56            59
BM_sinsp_concatenate_paths_relative_path_stddev                +2.4285         +2.4279             0             1             0             1
BM_sinsp_concatenate_paths_relative_path_cv                    +2.2093         +2.2090             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_mean                     +0.0096         +0.0096            24            25            24            25
BM_sinsp_concatenate_paths_empty_path_median                   +0.0105         +0.0105            24            25            24            25
BM_sinsp_concatenate_paths_empty_path_stddev                   -0.3639         -0.3641             0             0             0             0
BM_sinsp_concatenate_paths_empty_path_cv                       -0.3700         -0.3701             0             0             0             0
BM_sinsp_concatenate_paths_absolute_path_mean                  +0.1055         +0.1055            57            63            57            63
BM_sinsp_concatenate_paths_absolute_path_median                +0.1124         +0.1124            57            63            57            63
BM_sinsp_concatenate_paths_absolute_path_stddev                +5.6968         +5.7061             0             2             0             2
BM_sinsp_concatenate_paths_absolute_path_cv                    +5.0575         +5.0662             0             0             0             0
BM_sinsp_split_container_image_mean                            +0.0407         +0.0406           385           401           385           400
BM_sinsp_split_container_image_median                          +0.0446         +0.0445           384           401           384           401
BM_sinsp_split_container_image_stddev                          -0.4150         -0.4154             3             2             3             2
BM_sinsp_split_container_image_cv                              -0.4379         -0.4382             0             0             0             0

@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 14, 2025

I'll wait for the kernel CI to amend the wip commit message and un-wip this one.

@FedeDP
fix(userspace/libpman): do not use BPF_PROG_TYPE_TRACING that may o…
451e9cc 
…r may not be checkable in `pman_prepare_progs_before_loading`.

Instead, use `BPF_PROG_TYPE_RAW_TRACEPOINT` that works fine for the bpf helper probing.

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 14, 2025

/milestone 0.21.0

@poiana poiana added this to the 0.21.0 milestone Feb 14, 2025
@FedeDP FedeDP changed the title wip: fix(driver/modern_bpf): fix modern ebpf failures in master kernel-testing wip: fix(userspace/libpman): fix modern ebpf failures in master kernel-testing Feb 14, 2025
@FedeDP FedeDP mentioned this pull request Feb 14, 2025
Merged
@codecov Codecov
Copy link

codecov bot commented Feb 14, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.32%. Comparing base (c7b8dba) to head (451e9cc).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2282      +/-   ##
==========================================
- Coverage   75.33%   75.32%   -0.01%     
==========================================
  Files         280      280              
  Lines       34556    34556              
  Branches     5901     5902       +1     
==========================================
- Hits        26032    26031       -1     
- Misses       8524     8525       +1
Flag Coverage Δ
libsinsp 75.32% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FedeDP FedeDP changed the title wip: fix(userspace/libpman): fix modern ebpf failures in master kernel-testing fix(userspace/libpman): fix modern ebpf failures in master kernel-testing Feb 14, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 14, 2025 •
edited
Loading

X64 kernel testing matrix

KERNEL CMAKE-CONFIGURE KMOD BUILD KMOD SCAP-OPEN BPF-PROBE BUILD BPF-PROBE SCAP-OPEN MODERN-BPF SCAP-OPEN
amazonlinux2-4.19 🟢 🟢 🟢 🟢 🟢 🟡
amazonlinux2-5.10 🟢 🟢 🟢 🟢 🟢 🟢
amazonlinux2-5.15 🟢 🟢 🟢 🟢 🟢 🟢
amazonlinux2-5.4 🟢 🟢 🟢 🟢 🟢 🟡
amazonlinux2022-5.15 🟢 🟢 🟢 🟢 🟢 🟢
amazonlinux2023-6.1 🟢 🟢 🟢 🟢 🟢 🟢
archlinux-6.0 🟢 🟢 🟢 🟢 🟢 🟢
archlinux-6.7 🟢 🟢 🟢 🟢 🟢 🟢
centos-3.10 🟢 🟢 🟢 🟡 🟡 🟡
centos-4.18 🟢 🟢 🟢 🟢 🟢 🟢
centos-5.14 🟢 🟢 🟢 🟢 🟢 🟢
fedora-5.17 🟢 🟢 🟢 🟢 🟢 🟢
fedora-5.8 🟢 🟢 🟢 🟢 🟢 🟢
fedora-6.2 🟢 🟢 🟢 🟢 🟢 🟢
oraclelinux-3.10 🟢 🟢 🟢 🟡 🟡 🟡
oraclelinux-4.14 🟢 🟢 🟢 🟢 🟢 🟡
oraclelinux-5.15 🟢 🟢 🟢 🟢 🟢 🟢
oraclelinux-5.4 🟢 🟢 🟢 🟢 🟢 🟡
ubuntu-4.15 🟢 🟢 🟢 🟢 🟢 🟡
ubuntu-5.8 🟢 🟢 🟢 🟢 🟢 🟡
ubuntu-6.5 🟢 🟢 🟢 🟢 🟢 🟢

ARM64 kernel testing matrix

KERNEL CMAKE-CONFIGURE KMOD BUILD KMOD SCAP-OPEN BPF-PROBE BUILD BPF-PROBE SCAP-OPEN MODERN-BPF SCAP-OPEN
amazonlinux2-5.4 🟢 🟢 🟢 🟢 🟢 🟡
amazonlinux2022-5.15 🟢 🟢 🟢 🟢 🟢 🟢
fedora-6.2 🟢 🟢 🟢 🟢 🟢 🟢
oraclelinux-4.14 🟢 🟢 🟢 🟡 🟡 🟡
oraclelinux-5.15 🟢 🟢 🟢 🟢 🟢 🟢
ubuntu-6.5 🟢 🟢 🟢 🟢 🟢 🟢

@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 14, 2025 •
edited
Loading

I manually downloaded matrixes from the CI and the issue is fixed. To avoid losing time (s390x build...), i already force-pushed the branch and now we can wait for the kernel-testing comment too :)

@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 14, 2025

/cc @Molter73 @Andreagit97

Andreagit97
Andreagit97 approved these changes Feb 14, 2025
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it seems fair, the only issue that we can face with this approach is that we have an helper that is supported by BPF_PROG_TYPE_TRACING programs but it's not supported by BPF_PROG_TYPE_RAW_TRACEPOINT. This could lead to a false negatives since we indeed use BPF_PROG_TYPE_TRACING programs.

BTW the above case is quite unlikely so this could be a great workaround! Maybe just leave a comment so that we don't forget again about it

@poiana poiana added the lgtm label Feb 14, 2025
@FedeDP
Copy link
Contributor Author

FedeDP commented Feb 14, 2025

We'll have the linked PR in the blame 😆
Thanks andre!

@poiana poiana merged commit 618da03 into master Feb 17, 2025
48 of 50 checks passed
@poiana poiana deleted the fix/modern_ebpf branch February 17, 2025 09:28
@bevanjkay bevanjkay mentioned this pull request May 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasondellaluce jasondellaluce jasondellaluce approved these changes

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@hbrueckner hbrueckner Awaiting requested review from hbrueckner

@incertum incertum Awaiting requested review from incertum

@Molter73 Molter73 Awaiting requested review from Molter73

Assignees

@jasondellaluce jasondellaluce

@Andreagit97 Andreagit97

Labels
approved area/driver-modern-bpf area/libpman dco-signoff: yes kind/bug Something isn't working lgtm release-note-none size/XS
Projects
Falco Roadmap
Archived in project
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.
4 participants
@FedeDP @poiana @jasondellaluce @Andreagit97