fix(driver)!: create dedicated events for `umount2` syscall and rename them to "umount2" #944

Andreagit97 · 2023-03-07T16:30:35Z

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 pair PPME_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?:

fix(driver)!: create dedicated events for `umount2` syscall and rename them to "umount2"

FedeDP

LGTM!
Thank you!
/approve

poiana · 2023-03-07T17:31:31Z

LGTM label has been added.

Git tree hash: b8c53e06e94bc1b472039562ba633b4fabc90873

incertum · 2023-03-07T19:31:46Z

LGTM!

/hold until we formalize agreement around non-shared event names across the board, see #936 (comment).

Andreagit97 · 2023-03-08T10:17:21Z

I need to rebase it on master, after #937

Andreagit97 · 2023-03-08T11:41:41Z

adapted using the new ppm_sc table

userspace/libscap/linux/scap_ppm_sc.c

FedeDP · 2023-03-08T11:47:36Z

userspace/libsinsp/test/public_sinsp_API/interesting_syscalls.cpp

 	static std::unordered_set<std::string> sc_set_names_truth = {"accept",
-	"accept4", "execve", "syncfs", "eventfd", "eventfd2", "umount", "umount2",
+	"accept4", "execve", "syncfs", "eventfd", "eventfd2", "umount",


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.

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?

Oh of course, because [PPME_SYSCALL_UMOUNT_E] = (ppm_sc_code[]){PPM_SC_UMOUNT2, -1}, is mapped to UMOUNT2 here.

See #944 (comment)

FedeDP

Found a couple of minor issues, otherwise this LGTM!

Andreagit97 · 2023-03-08T15:29:26Z

Hey, folks here there is something strange.. we are using libsinsp::events::names_to_sc_set() API to convert event_names to sc_set while its dual libsinsp::events::sc_set_to_names convert sc_set to sc_names :/ this is quite confusing in my opinion. Right now we can keep it as it is in this PR but my proposal would be:

create 2 methods that convert event_names -> sc_set (what is doing names_to_sc_set) and vice-versa sc_set -> event_names
restore original behavior of libsinsp::events::names_to_sc_set() so sc_names -> sc_set
rename all methods like names_to_sc_set in something more meaningful like sc_names_to_sc_set, "names" alone is too generic IMHO

@FedeDP @jasondellaluce @incertum

Andreagit97 · 2023-03-08T15:36:22Z

after the rebase the CI is failing on ARM64 i suppose that there is something strange under the hood, I will take a look 👀

incertum · 2023-03-08T21:12:47Z

Hey, folks here there is something strange.. we are using libsinsp::events::names_to_sc_set() API to convert event_names to sc_set while its dual libsinsp::events::sc_set_to_names convert sc_set to sc_names :/ this is quite confusing in my opinion. Right now we can keep it as it is in this PR but my proposal would be:
1. create 2 methods that convert `event_names` -> `sc_set` (what is doing `names_to_sc_set`) and vice-versa `sc_set` -> `event_names`

2. restore original behavior of `libsinsp::events::names_to_sc_set()`  so `sc_names` -> `sc_set`

3. rename all methods like `names_to_sc_set` in something more meaningful like `sc_names_to_sc_set`, "names" alone is too generic IMHO
@FedeDP @jasondellaluce @incertum

It is the expected current behavior given shared event names across syscalls, e.g. Falco rule calls accept -> activate both accept, accept4. Would propose to hold this PR or any further back and forth until @jasondellaluce formalizes agreement on "requiring Falco to call the real syscall string name in the rules at all times in case of a syscall event".

Then perhaps we should first have another PR that changes this for other shared event cases, e.g. the accept -> activate both accept, accept4 case and similar ones. The current workaround in names_to_sc_set can then be removed again in that very same PR as it will then be redundant, but it's a big ! for all Falco end users.

re names_to_sc_set, I think it is fine to keep the name, but we could rename it (on board either way).

Last step -> rebasing this PR one more time and the confusion and hiccups should have gone away by then.

FedeDP · 2023-03-14T14:54:00Z

Does this PR require a change in the driver versions?

No

I think we actually need a schema minor version bump. I'm going to do that in #975

FedeDP · 2023-03-14T14:59:53Z

I think we actually need a schema minor version bump. I'm going to do that in #975

Minor SCHEMA version was already bumped since last release in dae8bc2

incertum · 2023-03-14T18:04:37Z

userspace/libsinsp/test/public_sinsp_API/interesting_syscalls.cpp

+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"};


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

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 🤔

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>

Andreagit97 · 2023-03-15T18:19:25Z

userspace/libsinsp/test/table/event_table.cpp

 	}

 end:
 	ASSERT_EQ(event_num, PPM_EVENT_MAX);
 }
+
+TEST(events, check_event_names)


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>

incertum

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

poiana · 2023-03-16T03:42:41Z

LGTM label has been added.

Git tree hash: 3f534979c4f5549e3a6bf168f6d91b0da3703167

FedeDP

/approve

poiana · 2023-03-16T07:20:47Z

[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:

~~OWNERS~~ [Andreagit97,FedeDP,incertum]

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

poiana added release-note kind/bug Something isn't working dco-signoff: yes kind/cleanup area/driver-kmod area/driver-bpf approved area/driver-modern-bpf area/libscap-engine-bpf labels Mar 7, 2023

poiana requested review from hbrueckner and LucaGuerra March 7, 2023 16:30

poiana added area/libscap-engine-kmod area/libscap-engine-modern-bpf area/libsinsp area/tests labels Mar 7, 2023

Andreagit97 added this to the 0.11.0 milestone Mar 7, 2023

poiana added the size/L label Mar 7, 2023

Andreagit97 modified the milestones: 0.11.0, next-driver Mar 7, 2023

FedeDP previously approved these changes Mar 7, 2023

View reviewed changes

poiana assigned FedeDP Mar 7, 2023

poiana added the lgtm label Mar 7, 2023

poiana added the do-not-merge/hold label Mar 7, 2023

Andreagit97 dismissed FedeDP’s stale review via 922e269 March 8, 2023 11:40

Andreagit97 force-pushed the address_umount2_issue branch from ed8d512 to 922e269 Compare March 8, 2023 11:40

poiana removed the lgtm label Mar 8, 2023

poiana requested a review from FedeDP March 8, 2023 11:40

FedeDP reviewed Mar 8, 2023

View reviewed changes

userspace/libscap/linux/scap_ppm_sc.c Show resolved Hide resolved

FedeDP reviewed Mar 8, 2023

View reviewed changes

Andreagit97 force-pushed the address_umount2_issue branch from 9a74925 to 79e70df Compare March 14, 2023 16:31

incertum reviewed Mar 14, 2023

View reviewed changes

Andreagit97 added 5 commits March 15, 2023 18:23

update: support new umount2 event pair

07dfcf9

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

tests: adapt tests to new umount2 events

e23a89f

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

update: adapt ppm_sc table

405f8d9

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

tests: fix broken test names_sc_set_names_corner_cases

29fe699

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

tests: add 2 new tests

0c03f22

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it>

Andreagit97 force-pushed the address_umount2_issue branch from 79e70df to 0c03f22 Compare March 15, 2023 18:16

Andreagit97 commented Mar 15, 2023

View reviewed changes

tests: revert a previous test change

2bf1c3a

Signed-off-by: Andrea Terzolo <andrea.terzolo@polito.it> Co-authored-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum approved these changes Mar 16, 2023

View reviewed changes

poiana removed the do-not-merge/hold label Mar 16, 2023

poiana assigned incertum Mar 16, 2023

poiana added the lgtm label Mar 16, 2023

FedeDP approved these changes Mar 16, 2023

View reviewed changes

poiana merged commit b2dcb51 into falcosecurity:master Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Andreagit97 commented Mar 7, 2023

FedeDP left a comment

poiana commented Mar 7, 2023

incertum commented Mar 7, 2023

Andreagit97 commented Mar 8, 2023

Andreagit97 commented Mar 8, 2023

FedeDP Mar 8, 2023

FedeDP Mar 8, 2023

FedeDP Mar 8, 2023

Andreagit97 Mar 8, 2023

FedeDP left a comment

Andreagit97 commented Mar 8, 2023

Andreagit97 commented Mar 8, 2023 •

edited

Loading

incertum commented Mar 8, 2023 •

edited

Loading

FedeDP commented Mar 14, 2023

FedeDP commented Mar 14, 2023

incertum Mar 14, 2023

Andreagit97 Mar 15, 2023

Andreagit97 Mar 15, 2023

Andreagit97 Mar 15, 2023

incertum left a comment

poiana commented Mar 16, 2023

FedeDP left a comment

poiana commented Mar 16, 2023

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

Andreagit97 commented Mar 7, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Mar 7, 2023

incertum commented Mar 7, 2023

Andreagit97 commented Mar 8, 2023

Andreagit97 commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FedeDP left a comment

Choose a reason for hiding this comment

Andreagit97 commented Mar 8, 2023

Andreagit97 commented Mar 8, 2023 • edited Loading

incertum commented Mar 8, 2023 • edited Loading

FedeDP commented Mar 14, 2023

FedeDP commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum left a comment

Choose a reason for hiding this comment

poiana commented Mar 16, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Mar 16, 2023

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

Andreagit97 commented Mar 8, 2023 •

edited

Loading

incertum commented Mar 8, 2023 •

edited

Loading