cleanup: drop `capture_enabled` flag from driver by FedeDP · Pull Request #709 · falcosecurity/libs

FedeDP · 2022-11-10T11:35:45Z

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area API-version
/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
/area libscap-engine-bpf
/area libscap-engine-kmod
/area libscap-engine-modern-bpf
/area libscap
/area libpman
/area tests

Does this PR require a change in the driver versions?

/version driver-API-version-major

We changed how libscap starts the capture for the driver.

What this PR does / why we need it:

This PR drops useless capture_enabled, by leveraging new runtime tracepoint management feature.
Basically, instead of stopping the capture by setting the flag, but keeping everything running (so, every tracepoint is still called but it just checks if capture is enabled and leaves), we detach every tracepoint.

Moreover, made tracepoint dynamic management per-consumer in kmod.
Tracepoints will be attached using a ref-counted method, so that a tracepoint that is not requested by any consumer, gets detached.
Moreover, now kmod follows other drivers behavior: as soon as it is started, no tracepoint is attached. Instead, tracepoints must be manually set by userspace consumer, using the PPM_IOCTL_MANAGE_TP ioctl.
As a side effect of the above, this also means that an inserted kmod won't steal any resource until any consumer is actually using it, in a way much similar to other drivers.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

poiana · 2022-11-10T11:35:55Z

@FedeDP: The label(s) area/api-version cannot be applied, because the repository doesn't have them.

Details

In response to this:

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area API-version
/area driver-kmod
/area driver-bpf
/area driver-modern-bpf
/area libscap-engine-bpf
/area libscap-engine-kmod
/area libscap-engine-modern-bpf
/area libscap
/area libpman
/area tests

Does this PR require a change in the driver versions?

/version driver-API-version-major

We changed how libscap start the capture for the driver.
TO BE DONE!

What this PR does / why we need it:

This PR drops useless capture_enabled, by leveraging new runtime tracepoint management feature.
Basically, instead of stopping the capture by setting the flag, but keeping everything running (so, every tracepoint is still called but it just checks if capture is enabled and leaves), we detach every tracepoint.

Moreover, made tracepoint dynamic management per-consumer in kmod.
Tracepoints will be attached using a ref-counted method, so that a tracepoint that is not requested by any consumer, gets detached.
Moreover, now kmod follows other drivers behavior: as soon as it is started, no tracepoint is attached. Instead, tracepoints must be manually set by userspace consumer, using the PPM_IOCTL_MANAGE_TP ioctl.
All of the above, also means that an inserted kmod won't steal any resource until any consumer is actually using it, in a way much similar to other drivers.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
NONE

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

FedeDP · 2022-11-10T14:00:52Z

driver/API_VERSION

@@ -1 +1 @@
-2.0.0
+3.0.0


We changed how scap tells drivers to start/stop the capture.

FedeDP · 2022-11-10T14:01:31Z

driver/bpf/plumbing_helpers.h

-					struct scap_bpf_settings *settings,
 					enum syscall_flags drop_flags)
 {
+	struct scap_bpf_settings *settings;


Moved the retrieval of bpf settings down to call_filler as it is now unused in attached programs.

FedeDP · 2022-11-10T14:02:32Z

driver/ppm.h

 	uint16_t statsd_port;
 	unsigned long buffer_bytes_dim; /* Every consumer will have its per-CPU buffer dim in bytes. */
 	DECLARE_BITMAP(events_mask, PPM_EVENT_MAX);
+	u32 tracepoints_attached;


Every consumer owns its own tracepoint mask.

FedeDP · 2022-11-10T14:02:52Z

driver/ppm.h


 #ifndef UDIG
 struct ppm_consumer_t {
+	unsigned int id; // numeric id for the consumer (ie: registration index)


This is the consumer index (ie: the first added will have 0, the second 1...).

FedeDP · 2022-11-10T14:03:37Z

test/modern_bpf/event_class/event_class.cpp

 	m_event_type = event_type;
-	switch(event_type)
+	m_event_direction = BOTH_EVENT;
+	switch(m_event_type)


Now it is enable_capture job to attach required tracepoints.

FedeDP · 2022-11-10T14:33:09Z

driver/main.c

 		event_data.event_info.context_data.sched_next = (void *)0;

-		record_event_consumer(consumer, PPME_SCAPEVENT_E, UF_NEVER_DROP, ppm_nsecs(), &event_data);
+		record_event_consumer(consumer, PPME_SCAPEVENT_E, UF_NEVER_DROP, ppm_nsecs(), &event_data, TP_VAL_INTERNAL);


Internal event; use canary value for tp.

FedeDP · 2022-11-10T14:33:23Z

driver/main.c

 	struct event_data_t event_data = {0};

-	if (record_event_consumer(consumer, PPME_DROP_E, UF_NEVER_DROP, ns, &event_data) == 0) {
+	if (record_event_consumer(consumer, PPME_DROP_E, UF_NEVER_DROP, ns, &event_data, TP_VAL_INTERNAL) == 0) {


Internal event; use canary value for tp.

FedeDP · 2022-11-10T14:33:26Z

driver/main.c

 	struct event_data_t event_data = {0};

-	if (record_event_consumer(consumer, PPME_DROP_X, UF_NEVER_DROP, ns, &event_data) == 0) {
+	if (record_event_consumer(consumer, PPME_DROP_X, UF_NEVER_DROP, ns, &event_data, TP_VAL_INTERNAL) == 0) {


Internal event; use canary value for tp.

FedeDP · 2022-11-10T14:33:46Z

driver/main.c

 	int32_t cbres = PPM_SUCCESS;
 	int cpu;

+	if (tp_type < TP_VAL_INTERNAL && !(consumer->tracepoints_attached & (1 << tp_type)))


Check if the consumer is attached to the tracepoint, or if the tracepoint is a canary value.

FedeDP · 2022-11-10T14:34:13Z

driver/main.c

 		event_data.event_info.context_data.sched_prev = (void *)cpu;
 		event_data.event_info.context_data.sched_next = (void *)sd_action;
-		record_event_all_consumers(PPME_CPU_HOTPLUG_E, UF_NEVER_DROP, &event_data);
+		record_event_all_consumers(PPME_CPU_HOTPLUG_E, UF_NEVER_DROP, &event_data, TP_VAL_INTERNAL);


Internal event; use canary value for tp.

FedeDP · 2022-11-10T15:22:23Z

Most of the interesting stuff (and the locs) is in kmod; i highlighted relevant changes in the self-review.

FedeDP · 2022-11-14T09:51:47Z

Updated kmod code:
It now can't have desynced g_tracepoints_attached and g_tracepoints_refs.
Only thing that can happens is that when destroying a consumer, that is the only one present, thus triggering a tracepoint detach, for some reason not all tracepoints can be detached; so we end up with some refs/attached tracepoints
that are not properly cleaned, even if no consumer exists.

This just means that we won't be able to disable them anymore because we will have some zombie references to those tracepoints.
Not a big deal, and i think it should really never happen.

/cc @gnosek @Andreagit97

Andreagit97 · 2022-11-17T12:13:39Z

driver/main.c

 	u32 idx;
 	u32 new_val;
 	u32 curr_val;
-	int ret = 0;
-
-	if (new_tp_set == g_tracepoints_attached)
-	{
-		// ok already equal
-		return ret;
-	}
+	int cpu;
+	int ret;


I would initialize all these variables just to be future proof

We can't mix declaration and code in c89/90 :) I mean, we actually can but then any added variable must be initialized.
We try to avoid doing that in kmod!

Andreagit97

/approve

…dropped `capture_enabled` from modern bpf probe. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

…er in headers that can be included by other engines. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

Moreover, made tracepoint dynamic management per-consumer. Tracepoints will be attached using a ref-counted method, so that a tracepoint that is not requested by any consumer, gets detached. Moreover, now kmod follows other drivers behavior: as soon as it is started, no tracepoint is attached. Instead, tracepoints must be manually set by userspace consumer, using the PPM_IOCTL_MANAGE_TP ioctl. All of the above, also means that an inserted kmod won't steal any resource until any consumer is actually using it, in a way much similar to other drivers. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

…nimum driver api version. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

…heir original position. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

It now **can't** have desynced `g_tracepoints_attached` and `g_tracepoints_refs`. Only thing that can happens is that when destroying a consumer (that is the only one present), for any reason, not all tracepoints can be detached; so we end up with some refs/attached tracepoints that are not properly cleaned, even if no consumer exists. This just means that we won't be able to disable them anymore because we will have some zombie references to those tracepoints. Not a big deal, and i think it should really never happen. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

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

FedeDP · 2022-11-22T08:40:39Z

Rebased on top of master.

Andreagit97

/approve

poiana · 2022-11-22T10:38:23Z

LGTM label has been added.

Details

Git tree hash: 11e47c318daf26b0f0d946b8d0e56d33564f127b

poiana · 2022-11-22T11:03:21Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Andreagit97,FedeDP,leogr]

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

FedeDP · 2022-12-02T13:53:39Z

/milestone 4.0.0+driver

poiana added release-note-none kind/cleanup dco-signoff: yes area/driver-kmod area/driver-bpf area/driver-modern-bpf area/libscap-engine-bpf area/libscap-engine-kmod area/libscap-engine-modern-bpf area/libscap area/libpman area/tests labels Nov 10, 2022

poiana added approved size/XXL labels Nov 10, 2022

poiana requested review from Molter73 and hbrueckner November 10, 2022 11:36

FedeDP changed the title ~~cleanup: drop capture_enabled flag from driver~~ wip: cleanup: drop capture_enabled flag from driver Nov 10, 2022

poiana added the do-not-merge/work-in-progress label Nov 10, 2022

FedeDP changed the title ~~wip: cleanup: drop capture_enabled flag from driver~~ cleanup: drop capture_enabled flag from driver Nov 10, 2022

poiana removed the do-not-merge/work-in-progress label Nov 10, 2022

FedeDP commented Nov 10, 2022

View reviewed changes

Andreagit97 added this to the 0.10.0 milestone Nov 10, 2022

poiana requested review from Andreagit97 and gnosek November 14, 2022 09:51

Andreagit97 reviewed Nov 17, 2022

View reviewed changes

Andreagit97 previously approved these changes Nov 21, 2022

View reviewed changes

poiana assigned Andreagit97 Nov 21, 2022

FedeDP and others added 12 commits November 22, 2022 09:39

cleanup(driver/modern_bpf,test,userspace/libscap,userspace/libpman): …

536f8da

…dropped `capture_enabled` from modern bpf probe. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

cleanup(userspace/libscap): define SCAP_HANDLE_T on source files, nev…

0f974e3

…er in headers that can be included by other engines. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

fix(userspace/libscap): fixed dynamic tracepoints for bpf engine.

a97a843

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

fix(test/modern_bpf): fixed modern bpf tests.

07f8a6c

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

update(driver,userspace/libscap): bump driver API_VERSION and scap mi…

10637bf

…nimum driver api version. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

fix(driver): fix consumer numeric id retrieval.

4cde43f

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

chore(libscap): be safer while destroying ELF data in bpf engine.

060fb1c

Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

chore(driver): moved back force_tp_set and compat_set_tracepoint to t…

ab9ad3f

…heir original position. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

update: refactor tests

6a3e413

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

cleanup: remove some compilation warning

7e391c5

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

FedeDP dismissed Andreagit97’s stale review via 7e391c5 November 22, 2022 08:40

FedeDP force-pushed the cleanup/drop_capture_enabled branch from 8227fb4 to 7e391c5 Compare November 22, 2022 08:40

poiana removed the lgtm label Nov 22, 2022

poiana requested a review from Andreagit97 November 22, 2022 08:40

Andreagit97 approved these changes Nov 22, 2022

View reviewed changes

poiana added the lgtm label Nov 22, 2022

leogr approved these changes Nov 22, 2022

View reviewed changes

poiana assigned leogr Nov 22, 2022

poiana merged commit c98f08b into master Nov 22, 2022

poiana deleted the cleanup/drop_capture_enabled branch November 22, 2022 11:03

FedeDP mentioned this pull request Nov 23, 2022

cleanup(driver,userspace/libscap): let kmod use syscalls mask instead of events mask, like bpf and modern bpf #728

Merged

poiana modified the milestones: 0.10.0, 4.0.0+driver Dec 2, 2022

FedeDP mentioned this pull request Dec 2, 2022

[TRACKING] Libs Release #761

Closed

7 tasks

		@@ -1 +1 @@
		2.0.0
		3.0.0

Conversation

FedeDP commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poiana commented Nov 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Nov 10, 2022

Uh oh!

FedeDP commented Nov 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Nov 22, 2022

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Nov 22, 2022

Uh oh!

poiana commented Nov 22, 2022

Uh oh!

FedeDP commented Dec 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

FedeDP commented Nov 10, 2022 •

edited

Loading