cleanup(driver,userspace/libscap): let kmod use syscalls mask instead of events mask, like bpf and modern bpf by FedeDP · Pull Request #728 · falcosecurity/libs

FedeDP · 2022-11-23T09:37:58Z

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area driver-kmod
/area libscap-engine-kmod

Does this PR require a change in the driver versions?

/version driver-API-version-major

But we already bumped major in #709

What this PR does / why we need it:

Both bpf and modern bpf use a syscalls of interest concept; kmod was instead using an event of interest concept.
I ported kmod to use same concept of bpf and modern bpf.
NOTE: using event of interest also carried bugs, because when an user enforce certain ppm_sc through the scap/sinsp APIs, we would also lose all other non-syscalls event types generated by kmod (ie: signals, drops...), because kmod internally zeroed the event mask.

Which issue(s) this PR fixes:

Described above.

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

cleanup(driver)!: port kmod to use syscalls mask instead of events mask.

… of events mask, like bpf and modern bpf. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP · 2022-11-23T09:39:58Z

/cc @gnosek @Andreagit97

Andreagit97 · 2022-11-23T12:21:45Z

/milestone 0.10.0

Andreagit97

Just one minor question

Andreagit97 · 2022-11-23T12:48:46Z

driver/main.c

 	consumer->fullcapture_port_range_end = 0;
 	consumer->statsd_port = PPM_PORT_STATSD;
-	bitmap_fill(consumer->events_mask, PPM_EVENT_MAX); /* Enable all syscall to be passed to userspace */
+	bitmap_fill(consumer->syscalls_mask, SYSCALL_TABLE_SIZE); /* Enable all syscalls to be passed to userspace */


Why do we need to enable all the syscalls here?

Yep this is a default; i don't think it is necessary given that scap_kmod will set all the interesting syscalls at startup time.

I'd leave it there for now :)

gnosek · 2022-11-23T12:55:47Z

This needs an API version bump, and ideally a new ioctl name/number instead of changing the behavior of existing ones

EDIT:

But we already bumped major in #709

I still disagree about this but I know I'm outnumbered so 🤷‍♂️ 🙃

Andreagit97 · 2022-11-23T12:59:33Z

I agree with the rename but do we really need the old feature on events_mask? do we have to disable events that are not syscalls?

gnosek · 2022-11-23T13:03:45Z

do we have to disable events that are not syscalls?
We probably have to do this via tracepoints now.

In any case, we cannot just change the ioctl behavior and use the same number (there's tons of fun to be had if you e.g. use an ancient consumer with a new driver). IMO:

introduce a completely new ioctl for this
make the old ioctl return -ENOTTY or something so that we can't mess up by accidentally mismatching versions

Andreagit97 · 2022-11-23T13:07:12Z

We probably have to do this via tracepoints now.

The tracepoint events shouldn't be masked, they should be always available, we just directly attach and detach the tracepoints 🤔 if the tracepoint is not there it cannot generate any event (probably this is exactly what you said 😆 )

In any case, we cannot just change the ioctl behavior and use the same number (there's tons of fun to be had if you e.g. use an ancient consumer with a new driver). IMO:
introduce a completely new ioctl for this
make the old ioctl return -ENOTTY or something so that we can't mess up by accidentally mismatching versions

Absolutely agree with that!

gnosek · 2022-11-23T13:07:52Z

The tracepoint events shouldn't be masked, they should be always available, we just directly attach and detach the tracepoints 🤔 if the tracepoint is not there it cannot generate any event

yeah, this is what I mean

FedeDP · 2022-11-23T13:09:08Z

This needs an API version bump, and ideally a new ioctl name/number instead of changing the behavior of existing ones

Api version bump, as said in the op, was already done in this release cycle for #709; no more changes are required.
For the ioctl: note that an old client is not expected to work, given the api maj version bump; that is the reason we added those semver checks!

FedeDP · 2022-11-23T13:34:29Z

I still disagree about this but I know I'm outnumbered so man_shrugging upside_down_face

We are going to reach chromium version in a couple of weeks if we bump every time we break 😆

…ated ioctls; added new ioctls for syscalls mask. Moreover, renamed `scap_eventmask_op` values accordingly. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

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

FedeDP · 2022-11-23T14:12:59Z

@gnosek everything should be ok now :) I dropped the old ioctls and added new ones.

userspace/libscap/scap_vtable.h

… do not need to be bound to kmod ioctls. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

dwindsor · 2022-11-23T15:41:15Z

Both bpf and modern bpf use a syscalls of interest concept; kmod was instead using an event of interest concept.
I ported kmod to use same concept of bpf and modern bpf.

Where does this leave the non-syscall kmod tracepoints (sched_process_exit, page_fault_user et al)?

FedeDP · 2022-11-23T15:48:37Z

Where does this leave the non-syscall kmod tracepoints (sched_process_exit, page_fault_user et al)?

Well, the ppm_sc API is for disabling syscalls (while leaving sys_enter and sys_exit tracepoints attached). You can still disable tracepoints too.
Basically, we have events that maps 1:1 with tracepoints for all tracepoints except sys_enter and sys_exit.
These latter tracepoints map 1:N with events (ie: each tracepoints manages all syscall related enter/exit events).

dwindsor · 2022-11-23T20:20:50Z

Makes sense. I guess what I was getting at was this:

// #define PPM_IOCTL_DISABLE_SIGNAL_DELIVER _IO(PPM_IOCTL_MAGIC, 14) Support dropped
// #define PPM_IOCTL_ENABLE_SIGNAL_DELIVER _IO(PPM_IOCTL_MAGIC, 15) Support dropped

☝️this change (96aa1686) seems fairly recent, does it mean that we can no longer disable these tracepoints (there are others that were disabled as well).

FedeDP · 2022-11-23T20:22:14Z

Of course you can :) but there is now a proper Scap API for that and a TPMASK ioctl! (A bit lower in that very same file)

leogr

SGTM, however, we should also bump the DRIVER API version, shouldn't we?
cc @gnosek @FedeDP @Andreagit97

FedeDP · 2022-11-24T08:36:54Z

Answered above to grzeg with the same question! 😄
See the OP of this PR !
We already bumped API version a week ago when #709 was merged!

Andreagit97

/approve

poiana · 2022-11-24T09:07:42Z

LGTM label has been added.

Details

Git tree hash: 17ca6765b5bdac9f93e80b7f231108ce36d4c3a8

FedeDP · 2022-11-24T11:49:33Z

@gnosek mind to approve if you are ok?

hbrueckner

/lgtm

poiana · 2022-11-24T12:27:08Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,gnosek]

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

poiana added kind/cleanup release-note dco-signoff: yes area/driver-kmod area/libscap-engine-kmod labels Nov 23, 2022

poiana requested review from LucaGuerra and hbrueckner November 23, 2022 09:38

poiana added approved size/M labels Nov 23, 2022

cleanup(driver,userspace/libscap): let kmod use syscalls mask instead…

761dec9

… of events mask, like bpf and modern bpf. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

FedeDP force-pushed the cleanup/kmod_use_syscalls branch from 308227e to 761dec9 Compare November 23, 2022 09:39

poiana requested a review from gnosek November 23, 2022 09:39

poiana added this to the 0.10.0 milestone Nov 23, 2022

Andreagit97 modified the milestones: 0.10.0, next-driver Nov 23, 2022

Andreagit97 reviewed Nov 23, 2022

View reviewed changes

FedeDP added 2 commits November 23, 2022 14:58

cleanup(driver,userspace/libscap): dropped support of events mask rel…

2dcb0b3

…ated ioctls; added new ioctls for syscalls mask. Moreover, renamed `scap_eventmask_op` values accordingly. Signed-off-by: Federico Di Pierro <nierro92@gmail.com>

chore(userspace/libscap): multiple renamings.

f4532c4

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

poiana added size/L and removed size/M labels Nov 23, 2022

gnosek reviewed Nov 23, 2022

View reviewed changes

userspace/libscap/scap_vtable.h Outdated Show resolved Hide resolved

chore(userspace/libscap): scap_eventmask_op and scap_tpmask_op values…

0f63fb5

… do not need to be bound to kmod ioctls. Signed-off-by: Federico Di Pierro <nierro92@gmail.com> Co-authored-by: Grzegorz Nosek <grzegorz.nosek@sysdig.com>

leogr reviewed Nov 24, 2022

View reviewed changes

Andreagit97 approved these changes Nov 24, 2022

View reviewed changes

poiana assigned Andreagit97 Nov 24, 2022

poiana added the lgtm label Nov 24, 2022

hbrueckner approved these changes Nov 24, 2022

View reviewed changes

poiana assigned hbrueckner Nov 24, 2022

gnosek approved these changes Nov 24, 2022

View reviewed changes

poiana assigned gnosek Nov 24, 2022

poiana merged commit f6e014b into master Nov 24, 2022

poiana deleted the cleanup/kmod_use_syscalls branch November 24, 2022 12:27

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Andreagit97 commented Nov 23, 2022

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

Andreagit97 Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

FedeDP Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

FedeDP Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Andreagit97 commented Nov 23, 2022

Uh oh!

gnosek commented Nov 23, 2022

Uh oh!

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

Uh oh!

Uh oh!

gnosek commented Nov 23, 2022

Uh oh!

FedeDP commented Nov 23, 2022

Uh oh!

FedeDP commented Nov 23, 2022

Uh oh!

FedeDP commented Nov 23, 2022

Uh oh!

Uh oh!

dwindsor commented Nov 23, 2022

Uh oh!

FedeDP commented Nov 23, 2022

Uh oh!

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

Uh oh!

Uh oh!

FedeDP commented Nov 23, 2022

Uh oh!

leogr left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Andreagit97 left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Nov 24, 2022

Uh oh!

FedeDP commented Nov 24, 2022

Uh oh!

hbrueckner left a comment

Choose a reason for hiding this comment

Uh oh!

poiana commented Nov 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

FedeDP commented Nov 23, 2022 •

edited

Loading

FedeDP commented Nov 23, 2022 •

edited

Loading

gnosek commented Nov 23, 2022 •

edited

Loading

Andreagit97 commented Nov 23, 2022 •

edited

Loading

dwindsor commented Nov 23, 2022 •

edited

Loading

FedeDP commented Nov 24, 2022 •

edited

Loading