-
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
Overlay FS: Add fields proc.is_exe_lower_layer, fd.is_upper_layer and fd.is_lower_layer #1936
Overlay FS: Add fields proc.is_exe_lower_layer, fd.is_upper_layer and fd.is_lower_layer #1936
Conversation
Please double check driver/API_VERSION file. See versioning. /hold |
Very nice 🚀 @eddyduer-sysdig! @loresuso could you help with the review since you worked on the upper layer flag? Thanks! |
Woah this is cool! I'm gonna review it soon 👀 🚀 |
5dc3187
to
de57f2d
Compare
Perf diff from master - unit tests
Heap diff from master - unit tests
Heap diff from master - scap file
|
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.
Thank you for this! I left some comments mainly on the driver side!
Maybe we could add some tests both driver side and userspace side:
- driver side (you can use something like this as a reference):
TEST(SyscallExit, execveX_upperlayer_success) - userspace side (you can check that the new fields are correctly populated after the execve/open/... events and that the new helpers
set_overlay_upper
work well) : https://github.com/falcosecurity/libs/blob/master/userspace/libsinsp/test/parsers/parse_clone.cpp
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/open.bpf.c
Outdated
Show resolved
Hide resolved
driver/modern_bpf/programs/attached/events/sched_process_exec.bpf.c
Outdated
Show resolved
Hide resolved
de57f2d
to
704eead
Compare
Perf diff from master - unit tests
Perf diff from master - scap file
Heap diff from master - unit tests
Heap diff from master - scap file
|
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.
Great work! i will check why the CI on ARM64 is failing. Some side comments:
- It would be great to add some drivers test for checking we are correctly getting these new params from the kernel
- it would be great to unify the extraction approach between the 3 drivers as suggested in the below comments.
- it would be great to add some tests in userspace as suggested by codecov
Here you can find some references for drivers_tests and sinsp tests #1936 (review)
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/open_by_handle_at.bpf.c
Outdated
Show resolved
Hide resolved
driver/modern_bpf/programs/tail_called/events/syscall_dispatched_events/open_by_handle_at.bpf.c
Outdated
Show resolved
Hide resolved
@@ -265,6 +265,7 @@ typedef struct scap_threadinfo | |||
char exepath[SCAP_MAX_PATH_SIZE+1]; ///< full executable path | |||
bool exe_writable; ///< true if the original executable is writable by the same user that spawned it. | |||
bool exe_upper_layer; //< True if the original executable belongs to upper layer in overlayfs | |||
bool exe_lower_layer; //< True if the original executable belongs to lower layer in overlayfs |
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.
For myself: I need to check if we missed something with scap-files
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.
I think that we need to append the new value to captures here: https://github.com/falcosecurity/libs/pull/1936/files#diff-7b4e2af897ce4b8645f4f862762f68425d8394805c45c194255730e99078767dR701
instead of putting it in the middle and also when reading it: https://github.com/falcosecurity/libs/pull/1936/files#diff-f62df5aa6a14a8687e897847637228c047695a25049b0b095769c4f7579f618fR688 we should read it as last param otherwise we would break existing scap files.
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 seems this is also causing an issue with sanitizer builds scap files related tests: https://github.com/falcosecurity/libs/actions/runs/10563275899/job/29263411886?pr=1936
driver/bpf/fillers.h
Outdated
bpf_probe_read_kernel(&upper_dentry, sizeof(upper_dentry), (char *)vfs_inode + sizeof(struct inode)); | ||
if(!upper_dentry) | ||
{ | ||
return PPM_OVERLAY_LOWER; |
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.
Not 100% sure but I don't think that !upper_dentry
-> PPM_OVERLAY_LOWER
WDYT?
return PPM_OVERLAY_LOWER; | |
return PPM_NOT_OVERLAY_FS; |
Hey @Molter73 I see that e2e tests are failing but the report is not uploaded in CI https://github.com/falcosecurity/libs/actions/runs/10307297656/job/28555325360?pr=1936#step:8:8 |
704eead
to
9066a9b
Compare
Fixed comments from code review and added some tests |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1936 +/- ##
==========================================
+ Coverage 74.20% 74.25% +0.04%
==========================================
Files 253 253
Lines 30832 30895 +63
Branches 5411 5410 -1
==========================================
+ Hits 22880 22941 +61
+ Misses 7952 7935 -17
- Partials 0 19 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
27e124d
to
7f0b654
Compare
/milestone next-driver |
… fd.is_lower_layer Signed-off-by: Eddy Duer <[email protected]>
Signed-off-by: Eddy Duer <[email protected]>
… syscall family Signed-off-by: Eddy Duer <[email protected]>
Signed-off-by: Eddy Duer <[email protected]>
Signed-off-by: Andrea Terzolo <[email protected]>
7f0b654
to
7a6bcd4
Compare
Signed-off-by: Andrea Terzolo <[email protected]>
7a6bcd4
to
a9546c9
Compare
Signed-off-by: Andrea Terzolo <[email protected]>
624fa1c
to
a332c03
Compare
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.
LGTM, thanks! I just pushed some commits to fix the scap-file handling and the e2e sinsp tests
Results of kernel tests: https://github.com/falcosecurity/libs/actions/runs/10575083303 x86
arm64
|
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.
/approve
LGTM label has been added. Git tree hash: dbd6247baa518ba2d5473ef695fa5edc7c0a3215
|
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Andreagit97, eddyduer-sysdig, 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 |
/unhold |
Overlay FS: Add fields proc.is_exe_lower_layer, fd.is_upper_layer and fd.is_lower_layer
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 libsinsp
Does this PR require a change in the driver versions?
/version driver-API-version-minor
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: