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 process_execute_failed only for supported new kernels #3881

Closed
wants to merge 3 commits into from
Closed

Fix process_execute_failed only for supported new kernels #3881

wants to merge 3 commits into from

Conversation

AlonZivony
Copy link
Collaborator

@AlonZivony AlonZivony commented Feb 21, 2024 •
edited

1. Explain what the PR does

9fc0ae6 fix(events): change process_execute_failed probes
e9094a9 feat(events): add probe relevance attribute

Fix #3356

2. Explain how to test it

3. Other comments

@AlonZivony AlonZivony mentioned this pull request Feb 21, 2024
Closed
NDStrahilevitz
NDStrahilevitz reviewed Feb 21, 2024
View reviewed changes
Copy link
Collaborator

@NDStrahilevitz NDStrahilevitz left a comment

Choose a reason for hiding this comment

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

Much better IMO. Thanks for addressing.

pkg/events/core.go Show resolved Hide resolved
pkg/events/derive/process_execute_failed.go Outdated Show resolved Hide resolved
pkg/events/definition_dependencies.go Outdated Show resolved Hide resolved
@NDStrahilevitz
Copy link
Collaborator

@josedonizetti or @geyslan I think one of you should review this as well due to the new kernel compatibility check.

@geyslan
Copy link
Member

geyslan commented Feb 21, 2024

@josedonizetti or @geyslan I think one of you should review this as well due to the new kernel compatibility check.

@AlonZivony when you move it for review, ping us. 👍🏼

@AlonZivony AlonZivony force-pushed the bugfix/exec-failed-usermode-kernel-dep branch from 419d325 to 009b60e Compare February 22, 2024 14:54
@AlonZivony AlonZivony marked this pull request as ready for review February 22, 2024 14:54
@AlonZivony AlonZivony force-pushed the bugfix/exec-failed-usermode-kernel-dep branch from 009b60e to 9fc0ae6 Compare February 22, 2024 14:56
@AlonZivony
Copy link
Collaborator Author

@josedonizetti or @geyslan I think one of you should review this as well due to the new kernel compatibility check.

@AlonZivony when you move it for review, ping us. 👍🏼

This is ready for review :)

@geyslan
Copy link
Member

geyslan commented Feb 22, 2024

@josedonizetti or @geyslan I think one of you should review this as well due to the new kernel compatibility check.

@AlonZivony when you move it for review, ping us. 👍🏼

This is ready for review :)

Is this a fix, a feature or a chore? A bit of all? 😀

If a fix, please open one issue to link it exposing the reasons. Tks.

I'm reviewing it now. 👍🏼

geyslan
geyslan requested changes Feb 22, 2024
View reviewed changes
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM when addressed a few things.

docs/docs/events/builtin/extra/process_execute_failed.md Outdated Show resolved Hide resolved
docs/docs/events/builtin/extra/process_execute_failed.md Outdated Show resolved Hide resolved
tests/e2e-inst-test.sh Outdated Show resolved Hide resolved
tests/e2e-inst-signatures/scripts/process_execute_failed.sh Show resolved Hide resolved
tests/e2e-inst-signatures/scripts/process_execute_failed.sh Outdated Show resolved Hide resolved
tests/e2e-inst-signatures/e2e-process_execute_failed.go Outdated Show resolved Hide resolved
tests/e2e-inst-signatures/e2e-process_execute_failed.go Show resolved Hide resolved
pkg/events/core.go Outdated Show resolved Hide resolved
@geyslan
Copy link
Member

geyslan commented Feb 23, 2024

LGTM when addressed a few things.

@AlonZivony I have this PR #3848 that touches on some parts that you also touched on (e.g. attaching probe). I think it's worth taking a look just to catch on upcoming changes.

@AlonZivony AlonZivony force-pushed the bugfix/exec-failed-usermode-kernel-dep branch from 9fc0ae6 to 379a507 Compare February 25, 2024 11:54
@AlonZivony AlonZivony force-pushed the bugfix/exec-failed-usermode-kernel-dep branch 7 times, most recently from a84af2d to 878a200 Compare February 28, 2024 12:05
@AlonZivony AlonZivony marked this pull request as draft February 28, 2024 12:13
@AlonZivony
Copy link
Collaborator Author

@geyslan I will give you a notice when the PR is ready again.
I have some issues with older kernels, I hope that I will soon make it work properly.

@AlonZivony AlonZivony force-pushed the bugfix/exec-failed-usermode-kernel-dep branch from 878a200 to 5479759 Compare February 28, 2024 14:07
@AlonZivony AlonZivony marked this pull request as ready for review February 28, 2024 14:07
@AlonZivony
Copy link
Collaborator Author

@geyslan ok now it passes all the tests and ready for review :)

@AlonZivony AlonZivony force-pushed the bugfix/exec-failed-usermode-kernel-dep branch from 5479759 to 3105f86 Compare February 29, 2024 11:22
@yanivagman yanivagman added this to the v0.21.0 milestone Mar 10, 2024
@AlonZivony
feat(events): add probe relevance attribute
6266321 
Add to each probe the option to determine its relevance according to
the OS version.
If a probe is irrelevant, an attempt to load it won't be initiated.
This allows to have different probes for events according to OS
version.
@AlonZivony
fix(events): create usermode events for submit
1ad33e2 
Fix an issue that usermode events are only created if they should be
emitted.
@AlonZivony
fix(events): change process_execute_failed probes
51177a2 
The previous probe was missing from different distros and kernels.
The new probes are safer, but only exist starting from v5.8 of the kernel.
@AlonZivony AlonZivony force-pushed the bugfix/exec-failed-usermode-kernel-dep branch from 3105f86 to 51177a2 Compare March 11, 2024 16:21
geyslan
geyslan reviewed Mar 11, 2024
View reviewed changes
tests/e2e-inst-test.sh
@@ -129,7 +129,7 @@ for TEST in $TESTS; do
--output option:parse-arguments \
--log file:$SCRIPT_TMP_DIR/tracee-log-$$ \
--signatures-dir "$SIG_DIR" \
--scope comm=echo,mv,ls,tracee,proctreetester,ping,ds_writer \
--scope comm=echo,mv,ls,tracee,proctreetester,ping,ds_writer,process_execute,tracee-ebpf \
Copy link
Member

Choose a reason for hiding this comment

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

Why tracee-ebpf is required here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feared that the init_namespaces event might fail to be submitted in the future because of this filter.
Currently it doesn't matter as scope filters are not applied to it.
Just thought it was a better practice.

geyslan
geyslan approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@geyslan geyslan left a comment

Choose a reason for hiding this comment

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

LGTM

yanivagman
yanivagman requested changes Mar 14, 2024
View reviewed changes
Copy link
Collaborator

@yanivagman yanivagman left a comment

Choose a reason for hiding this comment

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

Relying on kernel version to check if a symbol exists or not is not a good approach.
Commits can be backported (e.g. in RHEL), so we need a more robust way for these kind of checks.

@geyslan geyslan removed this from the v0.21.0 milestone Apr 16, 2024
@AlonZivony AlonZivony mentioned this pull request Apr 17, 2024
Merged
@AlonZivony
Copy link
Collaborator Author

I opened #3983 to fix it quickly for now, as it is not using any choosing mechanism of probes.
When the fallback mechanism will be available we will be able to change the event to use it.

@AlonZivony AlonZivony closed this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NDStrahilevitz NDStrahilevitz NDStrahilevitz left review comments

@yanivagman yanivagman yanivagman requested changes

@geyslan geyslan geyslan approved these changes

Assignees

@AlonZivony AlonZivony

Labels
area/build area/ebpf area/events area/testing kind/documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing symbols on newer kernels (> ~6.3)
4 participants
@AlonZivony @NDStrahilevitz @geyslan @yanivagman