Skip to content
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

fix(modern): perform an exact check on BPF_TRACE_RAW_TP attach type #1404

Merged
merged 3 commits into from Oct 18, 2023
Merged

fix(modern): perform an exact check on BPF_TRACE_RAW_TP attach type #1404

merged 3 commits into from Oct 18, 2023

Conversation

Andreagit97
Copy link
Member

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area libscap-engine-modern-bpf

Does this PR require a change in the driver versions?

No

What this PR does / why we need it:

This PR performs a more detailed check on the BPF_TRACE_RAW_TP attach type. Actual libbpf_probe_bpf_prog_type(BPF_PROG_TYPE_TRACING, NULL) 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.

Which issue(s) this PR fixes:

See falcosecurity/falco#2792

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

fix(modern): perform an exact check on `BPF_TRACE_RAW_TP` attach type

@Andreagit97 Andreagit97 added this to the 0.13.2 milestone Oct 11, 2023
FedeDP
FedeDP previously approved these changes Oct 11, 2023
View reviewed changes
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Oct 11, 2023
@poiana
Copy link
Contributor

poiana commented Oct 11, 2023

LGTM label has been added.

Git tree hash: 1b6caf1ce9f3dd308f1b7988ad6a69ceadda9f07

@Andreagit97
Copy link
Member Author

/hold

Molter73
Molter73 reviewed Oct 11, 2023
View reviewed changes
userspace/libpman/src/configuration.c Outdated Show resolved Hide resolved
userspace/libpman/src/configuration.c Outdated Show resolved Hide resolved
@poiana poiana removed the lgtm label Oct 12, 2023
@poiana poiana requested a review from FedeDP October 12, 2023 09:29
@Andreagit97
Copy link
Member Author

I have addressed the suggestions (thank you @Molter73). The actual solution checks for /sys/kernel/btf/vmlinux as a first fallback (IMO 99% of the cases), in the unlucky case in which the vmlinux is not found the user could be asked to mount /boot, /lib and /usr inside the container, the same requirement of libbpf (https://elixir.bootlin.com/linux/latest/source/tools/lib/bpf/btf.c#L4786). Let me know if you have other suggestions/ideas :)

FedeDP
FedeDP previously approved these changes Oct 12, 2023
View reviewed changes
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Oct 12, 2023
@poiana
Copy link
Contributor

poiana commented Oct 12, 2023

LGTM label has been added.

Git tree hash: 83335278ad86413233f815bc588b02043156afc4

@Andreagit97
Copy link
Member Author

/unhold

@Andreagit97
Copy link
Member Author

Since we didn't find an agreement in time I would move it to the next release

/milestone 0.14.0

@poiana poiana removed this from the 0.13.2 milestone Oct 13, 2023
@poiana poiana added this to the 0.14.0 milestone Oct 13, 2023
Molter73
Molter73 reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

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

Thanks for refactoring @Andreagit97! Code looks much cleaner this way IMO! Just left one more comment and would like to resolve the comment on pathing before merging, so waiting on @incertum for that one.

userspace/libpman/src/configuration.c Outdated Show resolved Hide resolved
userspace/libpman/src/configuration.c Show resolved Hide resolved
Molter73
Molter73 previously approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

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

/lgtm

@FedeDP
Copy link
Contributor

FedeDP commented Oct 17, 2023

@Andreagit97 small conflict and we are good to go!

Andreagit97 and others added 3 commits October 18, 2023 11:30
@Andreagit97
fix(modern): perform an exact check on BPF_TRACE_RAW_TP attach type
c416ba2 
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>
@Andreagit97 @Molter73
cleanup
4585417 
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>
Co-authored-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
@Andreagit97 @Molter73
cleanup
79f06b8 
Signed-off-by: Andrea Terzolo <andreaterzolo3@gmail.com>
Co-authored-by: Mauro Ezequiel Moltrasio <mmoltras@redhat.com>
@Andreagit97 Andreagit97 dismissed stale reviews from Molter73 and FedeDP via 79f06b8 October 18, 2023 09:34
@poiana poiana removed the lgtm label Oct 18, 2023
Molter73
Molter73 approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@Molter73 Molter73 left a comment

Choose a reason for hiding this comment

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

/lgtm

FedeDP
FedeDP approved these changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Oct 18, 2023
@poiana
Copy link
Contributor

poiana commented Oct 18, 2023

LGTM label has been added.

Git tree hash: 751cb33f7112c0c139664d637e88d390490bfd97

@poiana
Copy link
Contributor

poiana commented Oct 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

@poiana poiana merged commit 7d7a052 into falcosecurity:master Oct 18, 2023
25 checks passed
@FedeDP FedeDP mentioned this pull request Oct 23, 2023
Merged
@FedeDP
Copy link
Contributor

FedeDP commented Oct 23, 2023

/milestone 0.13.3

@poiana poiana modified the milestones: 0.14.0, 0.13.3 Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FedeDP FedeDP FedeDP approved these changes

@Molter73 Molter73 Molter73 approved these changes

@hbrueckner hbrueckner Awaiting requested review from hbrueckner

@incertum incertum Awaiting requested review from incertum

Assignees

@FedeDP FedeDP

@Molter73 Molter73

Labels
approved area/libscap-engine-modern-bpf dco-signoff: yes kind/bug Something isn't working lgtm release-note size/L
Projects
None yet
Milestone
0.13.3
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Andreagit97 @poiana @FedeDP @Molter73 @incertum