-
Notifications
You must be signed in to change notification settings - Fork 164
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
fix(driver)!: create dedicated events for umount2
syscall and rename them to "umount2"
#944
fix(driver)!: create dedicated events for umount2
syscall and rename them to "umount2"
#944
Conversation
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!
Thank you!
/approve
LGTM label has been added. Git tree hash: b8c53e06e94bc1b472039562ba633b4fabc90873
|
LGTM! /hold until we formalize agreement around non-shared event names across the board, see #936 (comment). |
I need to rebase it on master, after #937 |
ed8d512
to
922e269
Compare
adapted using the new |
static std::unordered_set<std::string> sc_set_names_truth = {"accept", | ||
"accept4", "execve", "syncfs", "eventfd", "eventfd2", "umount", "umount2", | ||
"accept4", "execve", "syncfs", "eventfd", "eventfd2", "umount", |
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'd add umount2
in the tmp_sc_set
isntead of removing it from here.
Or, we could also completely drop umount2
from here since it is not a corner case anymore.
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.
Interestingly, the test is also failing now:
Expected equality of these values:
std::set<std::string>(sc_set_names_truth.begin(), sc_set_names_truth.end())
Which is: { "accept", "accept4", "eventfd", "eventfd2", "execve", "pipe", "pipe2", "signalfd", "signalfd4", "syncfs", "umount" }
std::set<std::string>(sc_set_names.begin(), sc_set_names.end())
Which is: { "accept", "accept4", "eventfd", "eventfd2", "execve", "pipe", "pipe2", "signalfd", "signalfd4", "syncfs", "umount", "umount2" }
Didn't expect that; why is it retrieving "umount2" in the sc_set_names
?
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.
Oh of course, because [PPME_SYSCALL_UMOUNT_E] = (ppm_sc_code[]){PPM_SC_UMOUNT2, -1},
is mapped to UMOUNT2 here.
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.
See #944 (comment)
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.
Found a couple of minor issues, otherwise this LGTM!
Hey, folks here there is something strange.. we are using
|
after the rebase the CI is failing on ARM64 i suppose that there is something strange under the hood, I will take a look 👀 |
It is the expected current behavior given shared event names across syscalls, e.g. Falco rule calls Then perhaps we should first have another PR that changes this for other shared event cases, e.g. the re Last step -> rebasing this PR one more time and the confusion and hiccups should have gone away by then. |
I think we actually need a schema minor version bump. I'm going to do that in #975 |
9a74925
to
79e70df
Compare
TEST(interesting_syscalls, names_sc_set_names_corner_cases) | ||
{ | ||
/* INCONSISTENCY: `names_to_sc_set` is converting event names to ppm_sc, but this was not its original scope, the original scope was to convert sc_names -> to sc_set */ | ||
std::unordered_set<std::string> event_names{"accept", "execve", "syncfs", "eventfd", "umount", "pipe", "signalfd"}; |
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.
We now have umount2 as event name and system call name so old tests should pass? Would prefer not purging umount2 from the tests.
Agreed on revisiting all tests after all new APIs are finalized.
We already put this on
/hold until after 2023-03-15 community call formal agreement, see falcosecurity/falco#2443
plus need to decide which one to merge first and which one needs subsequent rebasing <-> #975
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.
now this test is correctly retrieving only PPM_SC_UMOUNT from the umount
name, I have to purge "PPM_SC_UMOUNT2" from the expected sc_set otherwise the test won't pass 🤔
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've added again UMOUNT_2
and its associated PPM_SC_UMOUNT2
and obviously tests are green again :)
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>
79e70df
to
0c03f22
Compare
} | ||
|
||
end: | ||
ASSERT_EQ(event_num, PPM_EVENT_MAX); | ||
} | ||
|
||
TEST(events, check_event_names) |
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.
just to avoid other "accept/accept4" cases in the next future 😆
Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com>
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.
Thanks a lot @Andreagit97 !!!
Also quickly tested it with some dummy setup:
-- Filter event types names: mount, umount2
-- Start capture
{"evt.dir":">","evt.type":"mount","proc.cmdline":"mount --bind foo2 foo","proc.name":"mount"}
{"evt.dir":"<","evt.type":"mount","proc.cmdline":"mount --bind foo2 foo","proc.name":"mount"}
{"evt.dir":">","evt.type":"umount2","proc.cmdline":"umount foo","proc.name":"umount"}
{"evt.dir":"<","evt.type":"umount2","proc.cmdline":"umount foo","proc.name":"umount"}
{"evt.dir":">","evt.type":"umount2","proc.cmdline":"systemd --user","proc.name":"systemd"}
{"evt.dir":"<","evt.type":"umount2","proc.cmdline":"systemd --user","proc.name":"systemd"}
{"evt.dir":">","evt.type":"umount2","proc.cmdline":"systemd rhgb --switched-root --system --deserialize 35","proc.name":"systemd"}
{"evt.dir":"<","evt.type":"umount2","proc.cmdline":"systemd rhgb --switched-root --system --deserialize 35","proc.name":"systemd"}
/approve
/unhold
LGTM label has been added. Git tree hash: 3f534979c4f5549e3a6bf168f6d91b0da3703167
|
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, FedeDP, incertum 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 |
What type of PR is this?
/kind bug
/kind cleanup
Any specific area of the project related to this PR?
/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
/area libscap-engine-bpf
/area libscap-engine-kmod
/area libscap-engine-modern-bpf
/area libsinsp
/area tests
Does this PR require a change in the driver versions?
No
What this PR does / why we need it:
This PR addresses part of the #911 issue.
Before this patch,
umount2
was associated with event pairPPME_SYSCALL_UMOUNT_E, PPME_SYSCALL_UMOUNT_X
and "umount" name. Every syscall should have its own events pair, with the right name (in this case "umount2"). This PR is the first step in that direction!Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?: