cleanup(driver,userspace/libscap,userspace/libsinsp): dropped simpleconsumer mode and simple driver concepts by FedeDP · Pull Request #521 · falcosecurity/libs

FedeDP · 2022-08-01T10:32:03Z

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area driver-kmod

/area driver-bpf

/area libscap-engine-bpf

/area libscap-engine-kmod

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

We'd need to bump the API maj version, but it was already bumped during this release cycle.

What this PR does / why we need it:

Dropped simpleconsumer mode and simple driver concepts.

Instead, clients should always push interesting syscalls set using open sinsp APIs for supported engines (kmod, bpf, modern_bpf) parameters.
Clients can also dynamically switch on/off syscalls at runtime, by using:

void mark_ppm_sc_of_interest(uint32_t ppm_sc, bool enabled = true);

Moreover, moved bpf interesting syscall check as soon as possible.
A new map was created to hold interesting syscalls for bpf.

Dropped EF_DROP_SIMPLE_CONS and UF_SIMPLEDRIVER_KEEP flags too.

Moreover, added a new interesting_tp_set field to scap_open_args, to allow scap clients to specify a set of tracepoints to be attached.
When NULL, all tracepoints will be attached (default value). Updated interesting_ppm_sc_codeset to match same behavior when NULL. Libbscap scap-open example now has 2 more options:--tpand--ppm_sc`, that allow to:

mark a single (or multiple, if option is passed multiple times) tracepoints as interesting
mark a single (or multiple, if option is passed multiple times) syscalls as interesting

Note however that the new interesting_tp_set is actually unused by sinsp, ie: it is only available to scap clients, not sinsp ones.
IMHO this is a somewhat debug feature that should not be exposed to libs clients.

Example:

sudo ./libscap/examples/01-open/scap-open --bpf driver/bpf/probe.o --tp 1 --ppm_sc 27 --ppm_sc 28

---------------------- SCAP SOURCE ----------------------
* BPF probe: 'driver/bpf/probe.o'
-----------------------------------------------------------

---------------------- CONFIGURATIONS ----------------------
* Print single event type: -1 (`-1` means no event to print).
* Run until '18446744073709551615' events are catched.
--------------------------------------------------------------

* OK! BPF probe correctly loaded: NO VERIFIER ISSUES :)
* Live capture in progress...
* Press CTRL+C to stop the capture
^C
---------------------- STATS -----------------------
events captured: 2
seen by driver: 2

Where ppm_sc 27 and 28 are mkdir and rmdir, and i've got another terminal that runs mkdir x; rmdir x.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

FedeDP · 2022-08-01T10:32:17Z

driver/bpf/maps.h

 };
 #endif

+struct bpf_map_def __bpf_section("maps") interesting_syscalls_table = {


New interesting syscalls map for bpf.

FedeDP · 2022-08-01T10:33:21Z

driver/bpf/types.h

 	SCAP_TMP_SCRATCH_MAP = 7,
 	SCAP_SETTINGS_MAP = 8,
 	SCAP_LOCAL_STATE_MAP = 9,
+	SCAP_INTERESTING_SYSCALLS_TABLE = 10,


SCAP_STASH_MAP should always stay last because it can be missing when BPF_SUPPORTS_RAW_TRACEPOINTS is enabled, therefore all below idx would be shifted by one.

Hmm how didn't we have this table before?

We passed a sentinel value while filling the syscall_table map instead; see old impl of populate_syscall_table_map() in scap_bpf.c

FedeDP · 2022-08-01T10:33:42Z

driver/main.c

-			/*
-			 * While we're here, disable simple mode if it's active
-			 */
-			g_simple_mode_enabled = false;


Dropped simple mode for kmod.

FedeDP · 2022-08-01T10:34:18Z

driver/ppm_events_public.h

 	EF_SKIPPARSERESET = (1 << 8), /* This event shouldn't pollute the parser lastevent state tracker. */
 	EF_OLD_VERSION = (1 << 9), /* This event is kept for backward compatibility */
-	EF_DROP_SIMPLE_CONS = (1 << 10), /* This event can be skipped by consumers that privilege low overhead to full event capture */
+	// EF_DROP_SIMPLE_CONS = (1 << 10), /* This event can be skipped by consumers that privilege low overhead to full event capture */ SUPPORT DROPPED


Didn't remove the flag to avoid having a weird jump between
1 << 9 and 1 << 11

FedeDP · 2022-08-01T10:34:48Z

driver/ppm_events_public.h

 #define PPM_IOCTL_GET_PROCLIST _IO(PPM_IOCTL_MAGIC, 16)
 #define PPM_IOCTL_SET_TRACERS_CAPTURE _IO(PPM_IOCTL_MAGIC, 17)
-#define PPM_IOCTL_SET_SIMPLE_MODE _IO(PPM_IOCTL_MAGIC, 18)
+// #define PPM_IOCTL_SET_SIMPLE_MODE _IO(PPM_IOCTL_MAGIC, 18) Support dropped


Didn't remove the flag to avoid having a weird jump between
(PPM_IOCTL_MAGIC, 17) and (PPM_IOCTL_MAGIC, 19)

FedeDP · 2022-08-01T10:35:04Z

driver/ppm_events_public.h

 	UF_NEVER_DROP = (1 << 1),
 	UF_ALWAYS_DROP = (1 << 2),
-	UF_SIMPLEDRIVER_KEEP = (1 << 3), ///< Mark a syscall to be kept in simpledriver mode, see scap_enable_simpledriver_mode()
+	// UF_SIMPLEDRIVER_KEEP = (1 << 3), ///< Mark a syscall to be kept in simpledriver mode, see scap_enable_simpledriver_mode() -> SUPPORT DROPPED


Didn't remove the flag to avoid having a weird jump between
1 << 2 and 1 << 4

FedeDP · 2022-08-01T10:35:32Z

userspace/libscap/engine/bpf/scap_bpf.c

 	{
 		const struct syscall_evt_pair *p = &g_syscall_table[j];
-		if (!handle->m_syscalls_of_interest[j])
+		if(bpf_map_update_elem(handle->m_bpf_map_fds[SCAP_SYSCALL_TABLE], &j, p, BPF_ANY) != 0)


Syscall table map will now always have full info about syscalls, because we will use the new interesting_syscalls_map.

FedeDP · 2022-08-01T10:35:45Z

userspace/libscap/engine/bpf/scap_bpf.c

+	}

-		if(bpf_map_update_elem(handle->m_bpf_map_fds[SCAP_SYSCALL_TABLE], &j, p, BPF_ANY) != 0)
+	return bpf_map_freeze(handle->m_bpf_map_fds[SCAP_SYSCALL_TABLE]);


Given it is start time constant, we can even freeze the map.

FedeDP · 2022-08-01T10:36:27Z

userspace/libscap/examples/01-open/scap_open.c

 	#define MODERN_BPF_OPTION "--modern_bpf"
 #endif
 #define SCAP_FILE_OPTION "--scap_file"
-#define SIMPLE_CONSUMER_OPTION "--simple_consumer"


No more simpleconsumer mode exists.

FedeDP · 2022-08-01T10:37:01Z

userspace/libscap/scap_engine_util.h

 #pragma once

-void fill_syscalls_of_interest(interesting_ppm_sc_set *ppm_sc_of_interest, bool (*syscalls_of_interest)[SYSCALL_TABLE_SIZE]);
+void fill_syscalls_of_interest(interesting_ppm_sc_set *ppm_sc_of_interest, bool *syscalls_of_interest);


Avoid passing a weird double pointer here :)

FedeDP · 2022-08-01T10:49:16Z

/cc @gnosek

incertum · 2022-08-02T17:14:53Z

❤️

Could we include a patch to scap-open ?

Add summary of syscall events (or any event) in form of a set() showing distinct syscall ids seen (sanity check for perf studies). Also transform to human readable list of syscall names if possible.

Edited: Perhaps also add start and end timestamp, duration of run and average event rate per second to scap-open summary stats? @FedeDP

gnosek

Looks fine at a quick glance :)

driver/bpf/plumbing_helpers.h

gnosek · 2022-08-03T11:38:07Z

driver/bpf/types.h

 	SCAP_TMP_SCRATCH_MAP = 7,
 	SCAP_SETTINGS_MAP = 8,
 	SCAP_LOCAL_STATE_MAP = 9,
+	SCAP_INTERESTING_SYSCALLS_TABLE = 10,


Hmm how didn't we have this table before?

userspace/libscap/engine/modern_bpf/modern_bpf.c

LucaGuerra · 2022-09-01T14:58:57Z

userspace/libscap/scap.c

+			{
+				if (g_syscall_table[j].exit_event_type == i || g_syscall_table[j].enter_event_type == i)
+				{
+					uint32_t ppm_sc_code = g_syscall_code_routing_table[i];


As per conversations with @FedeDP this might need to be j instead of i since it iterates on SYSCALL_TABLE_SIZE

Moreover, give more meaningful names to idx. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Luca Guerra <luca@guerra.sh>

incertum · 2022-09-01T21:49:38Z

Pulled latest changes and simulated #521 (comment) in the sinsp-example and everything seems to work as expected.
Simulation was using the new APIs to enforce syscalls of interest, but adding fchmodat which can be important to subscribe to through Falco rules.

std::unordered_set<uint32_t> ppm_sc = inspector.enforce_sinsp_state_ppm_sc();

open_engine(inspector);

inspector.mark_ppm_sc_of_interest(207); // fchmodat

FedeDP · 2022-09-02T09:22:12Z

/kind feature

This is the same API we introduced for runtime ppm_sc management in #521, but for tracepoints. It is implemented for kmod, bpf and modern_bpf. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

leogr

Approved again. I just left some minor comments (no blocker anyway).

leogr · 2022-09-02T14:58:54Z

driver/bpf/types.h

+	SCAP_INTERESTING_SYSCALLS_TABLE = 10,
 #ifndef BPF_SUPPORTS_RAW_TRACEPOINTS
-	SCAP_STASH_MAP = 10,
+	SCAP_STASH_MAP = 11,


I'd remove the ifndef wrapping and always keep SCAP_STASH_MAP (even when unnecessary).
That would make the addition of other items easier.

Indeed it is unused 😆 https://github.com/falcosecurity/libs/search?q=SCAP_STASH_MAP

driver/ppm_events_public.h

leogr · 2022-09-02T15:08:27Z

userspace/libscap/engine/kmod/scap_kmod.c

 	return 0;
 }

+/// TODO: we need to pass directly the system syscall number not the `ppm_sc` here.


Suggested change

/// TODO: we need to pass directly the system syscall number not the `ppm_sc` here.

// TODO: we need to pass directly the system syscall number not the `ppm_sc` here.

Not sure I understood this comment 🤔

userspace/libscap/engine/modern_bpf/scap_modern_bpf.c

userspace/libscap/engine/udig/udig_public.h

poiana · 2022-09-02T15:15:43Z

LGTM label has been added.

Details

Git tree hash: 6c59ae5cc9de6205a3ee4711110f3496cac7e098

LucaGuerra

Great work! This will be instrumental to tune performance depending on the lib client's needs!

/approve

poiana · 2022-09-06T14:10:59Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [FedeDP,LucaGuerra,leogr]

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

This is the same API we introduced for runtime ppm_sc management in #521, but for tracepoints. It is implemented for kmod, bpf and modern_bpf. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

This is the same API we introduced for runtime ppm_sc management in falcosecurity#521, but for tracepoints. It is implemented for kmod, bpf and modern_bpf. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

poiana added release-note-none kind/cleanup dco-signoff: yes area/driver-kmod labels Aug 1, 2022

poiana requested review from gnosek and mstemm August 1, 2022 10:32

poiana added area/driver-bpf area/libscap-engine-bpf approved area/libscap-engine-kmod labels Aug 1, 2022

FedeDP commented Aug 1, 2022

View reviewed changes

poiana added the size/XXL label Aug 1, 2022

FedeDP commented Aug 1, 2022

View reviewed changes

Andreagit97 changed the title ~~cleanup(driver,userspace/libscap,userspace/libsinsp): dropped simpleconsumer mode and simple driver concepts~~ [WIP] cleanup(driver,userspace/libscap,userspace/libsinsp): dropped simpleconsumer mode and simple driver concepts Aug 1, 2022

poiana added the do-not-merge/work-in-progress label Aug 1, 2022

FedeDP force-pushed the cleanup/unify_simpleconsumer_simpledriver branch from 8e85033 to 95dce9e Compare August 1, 2022 10:37

Andreagit97 added this to the 0.8.0 milestone Aug 2, 2022

FedeDP force-pushed the cleanup/unify_simpleconsumer_simpledriver branch from 32a896c to dc5f533 Compare August 3, 2022 10:24

gnosek reviewed Aug 3, 2022

View reviewed changes

FedeDP force-pushed the cleanup/unify_simpleconsumer_simpledriver branch from dc5f533 to cf088ec Compare August 3, 2022 12:59

leogr requested a review from LucaGuerra September 1, 2022 12:49

LucaGuerra reviewed Sep 1, 2022

View reviewed changes

fix(userspace/libscap): use correct idx when accessing routing table.

3d2d1ca

Moreover, give more meaningful names to idx. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Luca Guerra <luca@guerra.sh>

FedeDP dismissed leogr’s stale review via 3d2d1ca September 1, 2022 15:01

poiana removed the lgtm label Sep 1, 2022

poiana requested a review from leogr September 1, 2022 15:01

poiana added the kind/feature New feature or request label Sep 2, 2022

FedeDP mentioned this pull request Sep 2, 2022

wip: new: runtime tracepoints management #577

Closed

leogr approved these changes Sep 2, 2022

View reviewed changes

poiana added the lgtm label Sep 2, 2022

LucaGuerra approved these changes Sep 6, 2022

View reviewed changes

poiana assigned LucaGuerra Sep 6, 2022

poiana merged commit 61a5712 into master Sep 6, 2022

poiana deleted the cleanup/unify_simpleconsumer_simpledriver branch September 6, 2022 14:11

FedeDP mentioned this pull request Sep 6, 2022

new: runtime tracepoints management #582

Merged

	/// TODO: we need to pass directly the system syscall number not the `ppm_sc` here.
	// TODO: we need to pass directly the system syscall number not the `ppm_sc` here.

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FedeDP commented Aug 1, 2022

Uh oh!

incertum commented Aug 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnosek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

incertum commented Sep 1, 2022

Uh oh!

FedeDP commented Sep 2, 2022

Uh oh!

leogr left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

poiana commented Sep 2, 2022

Uh oh!

LucaGuerra left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Sep 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

FedeDP commented Aug 1, 2022 •

edited

Loading

incertum commented Aug 2, 2022 •

edited

Loading