Skip to content

cleanup(driver,userspace/libscap,userspace/libsinsp): dropped simpleconsumer mode and simple driver concepts #521

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.

Sign up for GitHub

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

Merged
poiana merged 29 commits into from
Sep 6, 2022
Merged

cleanup(driver,userspace/libscap,userspace/libsinsp): dropped simpleconsumer mode and simple driver concepts #521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
a4262fe
cleanup(driver,userspace/libscap,userspace/libsinsp): dropped simplec…
FedeDP Aug 1, 2022
9b993d3
new(driver, userspace/libscap): add a new scap_open argument to choos…
FedeDP Aug 3, 2022
db62a72
chore(driver,userspace/libscap,userspace/libsinsp): refactored kmod m…
FedeDP Aug 4, 2022
1b89ed5
chore(driver): restore old behavior in ppm_open: do not automatically…
FedeDP Aug 4, 2022
046001c
cleanup(driver,userspace/libscap,userspace/libsinsp): completely drop…
FedeDP Aug 4, 2022
9c0298c
update(scap-open): improve scap-open logging level
Andreagit97 Aug 5, 2022
b978031
update: disable all syscalls with `--ppm_sc -1`
Andreagit97 Aug 10, 2022
fdc58a9
fix(userspace/libscap,userspace/libsinsp): multiple fixes to SCAP_EVE…
FedeDP Aug 25, 2022
db79e54
fix(driver): fix build after rebase on top of master.
FedeDP Aug 25, 2022
42ca2f2
new(userspace/libsinsp,userspace/libscap): added a new sinsp::enforce…
FedeDP Aug 25, 2022
9993fae
fix(userspace/libscap): fix windows build.
FedeDP Aug 25, 2022
a049e16
docs(userspace/libsinsp): properly state risks of playing with intere…
FedeDP Aug 25, 2022
1e75573
update(libsinsp)!: refactor the APIs to use with syscalls and tracepo…
Andreagit97 Aug 26, 2022
a46471c
update(userspace): complete the rebase from master after #540.
FedeDP Aug 26, 2022
e5689fd
update(userspace): some small fixes after rebase.
FedeDP Aug 26, 2022
9417dbb
fix(userspace): fix non-linux and modern-bpf builds.
FedeDP Aug 26, 2022
cb3ba23
cleanup(libsinsp): remove unused API
Andreagit97 Aug 26, 2022
76f9581
cleanup(libsinsp): change signature of some methods
Andreagit97 Aug 26, 2022
5db8905
update(scap-open): add some `print` helpers
Andreagit97 Aug 27, 2022
6155d52
update(libsinsp): remove `should_consider` API
Andreagit97 Aug 27, 2022
3a1b968
cleanup(bpf-engine/kmod-engine): initialize tracepoints and syscalls
Andreagit97 Aug 27, 2022
340c061
refactor(libsinsp): use `uint32_t` instead of `string` for tracepoints
Andreagit97 Aug 28, 2022
f642963
cleanup(bpf-engine/kmod-engine): remove unused tables from the engines
Andreagit97 Aug 28, 2022
d9e1900
fix(libscap/libsinsp): allow examples to work correctly
Andreagit97 Aug 28, 2022
4ce017f
update(modern_bpf_engine): support interesting syscalls/tracepoints
Andreagit97 Aug 28, 2022
dfecb83
cleanup(bpf_engine): remove useless checks
Andreagit97 Aug 28, 2022
6645f62
update(engines): support `SCAP_EVENTMASK_ZERO` in all the engines
Andreagit97 Aug 29, 2022
e2710fe
chore: fix multiple small nits.
FedeDP Aug 31, 2022
3d2d1ca
fix(userspace/libscap): use correct idx when accessing routing table.
FedeDP Sep 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions driver/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ set(DRIVER_SOURCES
ppm_compat_unistd_32.h
ppm_version.h
systype_compat.h
ppm_tp.h
tp_table.c
)

foreach(FILENAME IN LISTS DRIVER_SOURCES)
Expand Down
2 changes: 1 addition & 1 deletion driver/Makefile.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
# MIT.txt or GPL.txt for full copies of the license.
#

@DRIVER_NAME@-y += main.o dynamic_params_table.o fillers_table.o flags_table.o ppm_events.o ppm_fillers.o event_table.o syscall_table.o ppm_cputime.o
@DRIVER_NAME@-y += main.o dynamic_params_table.o fillers_table.o flags_table.o ppm_events.o ppm_fillers.o event_table.o syscall_table.o ppm_cputime.o tp_table.o
obj-m += @DRIVER_NAME@.o
ccflags-y := @KBUILD_FLAGS@

Expand Down
7 changes: 7 additions & 0 deletions driver/bpf/maps.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,13 @@ struct bpf_map_def __bpf_section("maps") stash_map = {
};
#endif

struct bpf_map_def __bpf_section("maps") interesting_syscalls_table = {
Copy link
Copy Markdown
Contributor Author

@FedeDP FedeDP Aug 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New interesting syscalls map for bpf.

.type = BPF_MAP_TYPE_ARRAY,
.key_size = sizeof(u32),
.value_size = sizeof(bool),
.max_entries = SYSCALL_TABLE_SIZE,
};

#endif // __KERNEL__

#endif
13 changes: 13 additions & 0 deletions driver/bpf/plumbing_helpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,19 @@ static __always_inline const struct syscall_evt_pair *get_syscall_info(int id)
return p;
}

static __always_inline bool is_syscall_interesting(int id)
{
bool *enabled = bpf_map_lookup_elem(&interesting_syscalls_table, &id);

if (!enabled)
{
bpf_printk("no syscall_info for %d\n", id);
return false;
}

return *enabled;
}

static __always_inline const struct ppm_event_info *get_event_info(enum ppm_event_type event_type)
{
const struct ppm_event_info *e =
Expand Down
23 changes: 14 additions & 9 deletions driver/bpf/probe.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ BPF_PROBE("raw_syscalls/", sys_enter, sys_enter_args)
enum ppm_event_type evt_type;
int drop_flags;
long id;
bool enabled;

if (bpf_in_ia32_syscall())
return 0;
Expand All @@ -48,6 +49,12 @@ BPF_PROBE("raw_syscalls/", sys_enter, sys_enter_args)
if (id < 0 || id >= SYSCALL_TABLE_SIZE)
return 0;

enabled = is_syscall_interesting(id);
if (enabled == false)
{
return 0;
}

settings = get_bpf_settings();
if (!settings)
return 0;
Expand All @@ -59,9 +66,6 @@ BPF_PROBE("raw_syscalls/", sys_enter, sys_enter_args)
if (!sc_evt)
return 0;

if (sc_evt->flags & UF_UNINTERESTING)
return 0;

if (sc_evt->flags & UF_USED) {
Copy link
Copy Markdown
Contributor

@incertum incertum Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure about the use case for these settings that are probably legacy leftovers?

libs/driver/bpf/probe.c

Lines 65 to 71 in fb22f8e

if (sc_evt->flags & UF_USED) {
evt_type = sc_evt->enter_event_type;
drop_flags = sc_evt->flags;
} else {
evt_type = PPME_GENERIC_E;
drop_flags = UF_ALWAYS_DROP;
}

Would it be possible to remove all of the code related to dropping_mode and kernel side random sampling?

libs/userspace/libscap/engine/bpf/scap_bpf.c

Line 1421 in fb22f8e

settings.dropping_mode = false;

For every bpf tail call in sys enter and exit for the interesting syscalls we make an unnecessary call to drop_event

libs/driver/bpf/plumbing_helpers.h

Line 545 in fb22f8e

if (drop_event(stack_ctx, state, evt_type, settings, drop_flags))

Copy link
Copy Markdown
Contributor

@gnosek gnosek Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, we do need kernel side sampling and UF_ALWAYS_DROP/UF_NEVER_DROP (or equivalent functionality provided by some other mechanism)

Copy link
Copy Markdown
Contributor Author

@FedeDP FedeDP Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, agree with @gnosek .
My aim is for something like #521 (comment) for the future, ie: removing the dropping logic from the kernel drivers, and instead allow userspace to take full control.
This means:

  • much more customizable logic for clients
  • much more powerful logic, given that kernel drivers are a much more constrained environment
  • much cleaner driver behavior, ie: nothing is hidden to userspace: every event is sent to userspace unless userspace decides it is under pressure and asks sinsp->scap->drivers to set some syscalls/tracepoints as uninteresting

Copy link
Copy Markdown
Member

@Andreagit97 Andreagit97 Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah agree with @FedeDP we cannot remove all this stuff in just one step, we will do it in little steps to avoid breaking all 🤣

Copy link
Copy Markdown
Contributor

@incertum incertum Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally, sorry should have been more clear about that this is a comment not intended for refactor all in once, but rather to not forget about it 🙃

@gnosek just out of curiosity what are those use cases for threat detection that require random kernel side sampling?

yes, also think in the future there can be even better mechanisms for more deterministic kernel side dropping / filtering to alleviate unwanted back pressure from userspace. Will retain some suggestions for a future discussion to not overload this discussion here.

Copy link
Copy Markdown
Contributor

@incertum incertum Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FedeDP oops totally overlooked your previous comment, thanks for linking to it.

Copy link
Copy Markdown
Contributor

@gnosek gnosek Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@incertum not for threat detection but for performance monitoring (the threat detection events are never dropped anyway). When we're running under high load, we can drop the less important events to make more room in the ring buffers for the critical ones. The current implementation is basically "drop non-critical events for all but the first 1/nth of every second", with n being a power of two between 1 and 128 (inclusive), set from userspace.

Copy link
Copy Markdown
Contributor

@incertum incertum Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gnosek , this makes sense for performance monitoring. Hopefully we find the right approach to support both use cases.

For threat detection use cases more system calls should probably have UF_NEVER_DROP set:

__NR_chmod 
__NR_fchmodat
__NR_fchmod
__NR_mount 
__NR_umount
__NR_umount2
__NR_ptrace
__NR_connect
__NR_accept
__NR_accept4
__NR_symlink
__NR_symlinkat
__NR_setuid
__NR_unshare
__NR_bpf
__NR_seccomp
__NR_openat2
__NR_mprotect
__NR_open_by_handle_at
__NR_setns
__NR_capset
...

Copy link
Copy Markdown
Member

@Andreagit97 Andreagit97 Aug 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah right now there is quite a mess in our flags, we need to clean them a little bit, but I think this will be a future step 🚀 We need to specify the specific meaning of each one and understand when we have to label a syscall with it! @incertum these are all good points in my opinion maybe we can create an issue on that 🤔 WDYT?

evt_type = sc_evt->enter_event_type;
drop_flags = sc_evt->flags;
Expand Down Expand Up @@ -92,6 +96,7 @@ BPF_PROBE("raw_syscalls/", sys_exit, sys_exit_args)
enum ppm_event_type evt_type;
int drop_flags;
long id;
bool enabled;

if (bpf_in_ia32_syscall())
return 0;
Expand All @@ -100,6 +105,12 @@ BPF_PROBE("raw_syscalls/", sys_exit, sys_exit_args)
if (id < 0 || id >= SYSCALL_TABLE_SIZE)
return 0;

enabled = is_syscall_interesting(id);
if (enabled == false)
{
return 0;
}

settings = get_bpf_settings();
if (!settings)
return 0;
Expand All @@ -111,9 +122,6 @@ BPF_PROBE("raw_syscalls/", sys_exit, sys_exit_args)
if (!sc_evt)
return 0;

if (sc_evt->flags & UF_UNINTERESTING)
return 0;

if (sc_evt->flags & UF_USED) {
evt_type = sc_evt->exit_event_type;
drop_flags = sc_evt->flags;
Expand Down Expand Up @@ -180,9 +188,6 @@ static __always_inline int bpf_page_fault(struct page_fault_args *ctx)
if (!settings)
return 0;

if (!settings->page_faults)
return 0;

if (!settings->capture_enabled)
return 0;

Expand Down
4 changes: 2 additions & 2 deletions driver/bpf/types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,8 +220,9 @@ enum scap_map_types {
SCAP_TMP_SCRATCH_MAP = 7,
SCAP_SETTINGS_MAP = 8,
SCAP_LOCAL_STATE_MAP = 9,
SCAP_INTERESTING_SYSCALLS_TABLE = 10,
Copy link
Copy Markdown
Contributor Author

@FedeDP FedeDP Aug 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@gnosek gnosek Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm how didn't we have this table before?

Copy link
Copy Markdown
Contributor Author

@FedeDP FedeDP Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#ifndef BPF_SUPPORTS_RAW_TRACEPOINTS
SCAP_STASH_MAP = 10,
SCAP_STASH_MAP = 11,
Copy link
Copy Markdown
Member

@leogr leogr Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@FedeDP FedeDP Sep 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#endif
};

Expand All @@ -232,7 +233,6 @@ struct scap_bpf_settings {
uint32_t sampling_ratio;
bool capture_enabled;
bool do_dynamic_snaplen;
bool page_faults;
bool dropping_mode;
bool is_dropping;
bool tracers_enabled;
Expand Down
270 changes: 135 additions & 135 deletions driver/event_table.c
View file
Open in desktop

Large diffs are not rendered by default.

Loading