set sampling ratio of dropping for single syscall individually #1309

wangyongfeng5 · 2023-08-29T07:07:41Z

What type of PR is this?

/kind feature

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 libscap

/area libpman

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

What this PR does / why we need it:
Set sampling ratio of dropping for single syscall individually，users can set different discard rates for system calls of different importance.

poiana · 2023-08-29T07:07:44Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

poiana · 2023-08-29T07:07:51Z

Welcome @wangyongfeng5! It looks like this is your first PR to falcosecurity/libs 🎉

poiana · 2023-08-29T07:08:00Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wangyongfeng5
Once this PR has been reviewed and has the lgtm label, please assign molter73 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

github-actions · 2023-08-29T07:08:11Z

Please double check driver/SCHEMA_VERSION file. See versioning.

/hold

FedeDP · 2023-08-29T07:31:48Z

Hi! Thanks for your contribution!
This is a rather interesting idea; i think you need to bump driver/API_VERSION major (and SCAP_MINIMUM_API_VERSION).
Also, it would be great to have tests around the new feature; we already cover the sampling ratio feature: https://github.com/falcosecurity/libs/tree/master/test/drivers/test_suites/actions_suite; can you expand/fixup them?

Signed-off-by: Manny Wang <[email protected]>

Andreagit97 · 2023-08-29T09:04:34Z

Seems like a nice improvement, tagging our sampling ratio expert @gnosek! Moreover, we are in the process of releasing a new libs version, not sure we can do this for the next tag but we will try to do our best!

Signed-off-by: Manny Wang <[email protected]>

gnosek

Sorry to say, I'm not too excited about this PR as it currently stands. The issues I have with it are:

we really need DROP_[EX] events to contain the sampling ratio (getting rid of the sampling ratio obviously complicates this :))
replacing "the" sampling ratio with an array feels wrong. With your patch, the sampling ratio now becomes an attribute of the individual syscalls, so maybe it should become an extension of the ppm_sc_of_interest concept (accept/drop becomes accept/accept-1/nth-of-the-time/drop) rather than an extension of the global sampling ratio concept?

My suggestion would be to leave the global sampling ratio alone and add a separate per-syscall sampling step afterwards, so that the higher sampling ratio (global or per-syscall) wins, although you'd realistically only use one or the other.

Also, what's the end use case for this? What would you use the extra flexibility for?

gnosek · 2023-08-29T10:53:56Z

driver/main.c


-		vpr_info("new sampling ratio: %d\n", new_sampling_ratio);
+		vpr_info("new default sampling ratio: %d\n", new_sampling_ratio);


This isn't really setting just the default, it's overwriting any previous per-syscall sampling ratios configured

gnosek · 2023-08-29T10:57:48Z

driver/main.c

@@ -1186,6 +1199,43 @@ static long ppm_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		ret = 0;
 		goto cleanup_ioctl;
 	}
+	case PPM_IOCTL_SET_DROPPING_RATIO:


Bikeshedding names is always fun, but I'd rather keep the SAMPLING in the name here (e.g. PPM_IOCTL_SET_SYSCALL_SAMPLING_RATIO?). Otherwise I'd keep wondering if sampling_ratio and dropping_ratio are the same thing and why do we need two ioctls to manage them.

Also, please consider (not forcing this in any way, just please consider :)) passing a (two-u32) struct by pointer instead of unpacking the arguments from a single (by-value) u64.

gnosek · 2023-08-29T11:04:31Z

driver/bpf/fillers.h

@@ -4939,7 +4939,7 @@ FILLER(sched_drop, false)
 	/*
 	 * ratio
 	 */
-	return bpf_push_u32_to_ring(data, data->settings->sampling_ratio);
+	return bpf_push_u32_to_ring(data, 0);


If I'm correct, this is going to cause a lot of pain for us :( we actually do rely on getting the sampling ratio in drop events) and would require a major redesign once there's no single sampling ratio.

It feels like a way forward would be to keep the notion of a single sampling ratio and disable it only when a consumer uses the per-syscall ioctl (and we'd never do that then).

something like:

if(settings->sampling_ratio != PER_SYSCALL_SAMPLING_RATIO) { sampling_ratio = settings->sampling_ratio; } else { sampling_ratio = settings->sampling_ratios[syscall_id]; }

and in the ioctl that sets per syscall sampling ratios, just set settings->sampling_ratio = PER_SYSCALL_SAMPLING_RATIO too

If there are both global and per-system call ratios, which one should be reported here? Global? Or the one being sampled? In the komd driver, it seems difficult to obtain the system call being sampled based on its mechanism that may delay the insertion of drop events.

gnosek · 2023-08-29T11:06:21Z

driver/main.c

+		vpr_info("PPM_IOCTL_SET_DROPPING_RATIO, syscall(%u), ratio(%u), consumer %p\n", syscall_to_set, new_sampling_ratio, consumer_id);
+
+		if (syscall_to_set >= SYSCALL_TABLE_SIZE) {
+			pr_err("invalid syscall %u\n", syscall_to_set);


This will appear in the kernel log without any extra context, so maybe add a few words here (like that we're in this particular ioctl for example). I'm sure there's precedent for cryptic log messages but let's try to make things better one line at a time :)

gnosek · 2023-08-29T11:07:37Z

driver/modern_bpf/programs/tail_called/events/custom_logic/drop.bpf.c

@@ -22,7 +22,7 @@ int BPF_PROG(t1_drop_e)

 	/*=============================== COLLECT PARAMETERS ===========================*/

-	ringbuf__store_u32(&ringbuf, maps__get_sampling_ratio());
+	ringbuf__store_u32(&ringbuf, 0);


Same as for the other engines, we can't work without a reliable sampling ratio report (as seen by the driver) :<

gnosek · 2023-08-29T11:08:43Z

driver/ppm_fillers.c

@@ -4525,7 +4525,7 @@ int f_sched_drop(struct event_filler_arguments *args)
 	/*
 	 * ratio
 	 */
-	res = val_to_ring(args, args->consumer->sampling_ratio, 0, false, 0);
+	res = val_to_ring(args, 0, 0, false, 0);


Same. We need it :(

gnosek · 2023-08-29T11:09:55Z

userspace/libpman/src/maps.c

+	if(g_syscall_table[syscall_id].flags & (UF_NEVER_DROP | UF_ALWAYS_DROP)
+	  || g_syscall_table[syscall_id].flags == UF_NONE 
+	  || !(g_syscall_table[syscall_id].flags & UF_USED))
+	{
+		return 1;
+	}


We don't check the event flags in pman_set_default_sampling_ratio, and I guess it's not critical here either (setting the sampling ratio for an unused event feels harmless?)

wangyongfeng5 · 2023-08-30T03:37:24Z

Sorry to say, I'm not too excited about this PR as it currently stands. The issues I have with it are:

we really need DROP_[EX] events to contain the sampling ratio (getting rid of the sampling ratio obviously complicates this :))

replacing "the" sampling ratio with an array feels wrong. With your patch, the sampling ratio now becomes an attribute of the individual syscalls, so maybe it should become an extension of the ppm_sc_of_interest concept (accept/drop becomes accept/accept-1/nth-of-the-time/drop) rather than an extension of the global sampling ratio concept?

My suggestion would be to leave the global sampling ratio alone and add a separate per-syscall sampling step afterwards, so that the higher sampling ratio (global or per-syscall) wins, although you'd realistically only use one or the other.

Also, what's the end use case for this? What would you use the extra flexibility for?

My end use case: The user process makes some unreasonable system calls, such as calling accpet on a non-blocking socket, which generates a large number of useless events, so we have to enable sampling, but at the same time we don't want to lose other useful system calls, such as 'sendto ', as they are important to the upper-level rules, here are two ways:

Provide a method to protect certain system calls from being discarded during sampling
Provide a method to set an individual sampling rate, and set the protected system call to 100%, while providing more options other than 100%
This PR uses the second method. In fact, I have tried both methods.

So, I would like to know:
1.Do you think the above usage scenarios need to be met?
2.If yes, which of the above two methods do you think is more suitable?

Andreagit97 · 2023-08-30T12:26:03Z

Uhm the scenario is clear now, thank you!

Let's say, if you want to enable only some syscalls and disable others that are noisy, we have a solution already in place, "the interesting syscall" logic. As you may imagine you pass to the drivers a simple table saying which syscalls you want.

if I understood well here you are searching for something slightly different. You don't want to disable sycalls you just want to sampling them because maybe some accept events (as in your example) could be useful so you don't want to drop them all. If this is the case maybe we could follow the approach 1, i.e. "allow the driver users to specify which syscall they don't want to be sampled". Probably approach 1 is more convenient because we already have almost all in place (see UF_NEVER_DROP and UF_ALWAYS_DROP flags). We just need to add a custom logic to allow users to put these flags on the various syscalls.

WDYT? @gnosek @FedeDP @wangyongfeng5

FedeDP · 2023-08-30T13:04:11Z

if I understood well here you are searching for something slightly different. You don't want to disable sycalls you just want to sampling them because maybe some accept events (as in your example) could be useful so you don't want to drop them all.

As far as i understood, @wangyongfeng5 just wants to drop all of them; in that scenario, syscalls of interest feature is enough.
In the scenario @Andreagit97 imagined, i agree with him, approach 1 is the most likely to be implemented :)

wangyongfeng5 · 2023-08-30T13:33:39Z

Uhm the scenario is clear now, thank you!

Let's say, if you want to enable only some syscalls and disable others that are noisy, we have a solution already in place, "the interesting syscall" logic. As you may imagine you pass to the drivers a simple table saying which syscalls you want.

if I understood well here you are searching for something slightly different. You don't want to disable sycalls you just want to sampling them because maybe some accept events (as in your example) could be useful so you don't want to drop them all. If this is the case maybe we could follow the approach 1, i.e. "allow the driver users to specify which syscall they don't want to be sampled". Probably approach 1 is more convenient because we already have almost all in place (see UF_NEVER_DROP and UF_ALWAYS_DROP flags). We just need to add a custom logic to allow users to put these flags on the various syscalls.

WDYT? @gnosek @FedeDP @wangyongfeng5

Yes, if accept does not return EAGAIN, the event is still very important, so we don't want to discard all accept events.

In addition, we are also trying to find a way to filter useless events to replace the indiscriminate discarding of sampling. For example, in kernel mode, that is, before the event data reaches the ring buffer, according to syscall id, return value, pid and some other conditions to filter events.

FedeDP · 2023-08-30T13:38:35Z

Yes, if accept does not return EAGAIN, the event is still very important, so we don't want to discard all accept events.

Oh i see, sorry! Thank you for the explanation.

Andreagit97 · 2023-08-30T13:45:37Z

In addition, we are also trying to find a way to filter useless events to replace the indiscriminate discarding of sampling. For example, in kernel mode, that is, before the event data reaches the ring buffer, according to syscall id, return value, pid and some other conditions to filter events.

That would be very nice, we thought different times at filtering events kernel side using comm or pid. The only blocker we always faced was the possible overhead on non-filtered events. For example, if for every event we have to check among a list of 50 comm we pay a high price for each event for maybe filtering just 1/1000 of them. But yes maybe finding the right combination could be a great improvement. What I don't like about the sampling is that you lose possible important events and you don't have any way to control it :/

wangyongfeng5 · 2023-08-30T14:17:09Z

In addition, we are also trying to find a way to filter useless events to replace the indiscriminate discarding of sampling. For example, in kernel mode, that is, before the event data reaches the ring buffer, according to syscall id, return value, pid and some other conditions to filter events.

That would be very nice, we thought different times at filtering events kernel side using comm or pid. The only blocker we always faced was the possible overhead on non-filtered events. For example, if for every event we have to check among a list of 50 comm we pay a high price for each event for maybe filtering just 1/1000 of them. But yes maybe finding the right combination could be a great improvement. What I don't like about the sampling is that you lose possible important events and you don't have any way to control it :/

I think it's worth it:

In threat detection scenarios, filtering out events generated by trusted processes should be very useful, because in most production environments, events generated by business processes account for the vast majority, and they are usually harmless.
Regarding the filtering of junk events (such as the accept that returns EAGAIN), if we can support flexible conditions such as combining pid, syscall, direction, errno, etc., we should be able to filter out most of them.

gnosek · 2023-08-30T15:15:49Z

It feels to me that sampling is the wrong approach here: you do not care about e.g. accept() calls returning -EAGAIN at all, so it would be best to filter them out completely, not (semi) randomly sample them and hope we catch the non-EAGAIN ones.

IMHO what you want is UF_NEVER_DROP on the accept* syscalls (it's weird that we don't have this already...) combined with improvements to drop_nostate_event for kmod (and its counterparts in other engines, I don't know them off the top of my head) so that we can drop accept() syscalls returning -EAGAIN (or maybe just drop everything that returns -EAGAIN and be done with it?)

@Andreagit97 @FedeDP do you think it's realistic to expose UF_ALWAYS_DROP/UF_NEVER_DROP via ppm_sc_of_interest? Again, going from accept/drop to accept/accept-when-not-sampling/drop.

There are several, unfortunately conflicting, viewpoints on syscall/event filtering:

troubleshooting (e.g. sysdig) wants to see everything
security (e.g. falco) wants to reliably see a fairly small subset of events
performance metrics can be fine with subsampling some (but not all) syscalls and extrapolating the metrics from a (known) proportion of sampled data

In threat detection scenarios, filtering out events generated by trusted processes should be very useful, because in most production environments, events generated by business processes account for the vast majority, and they are usually harmless.

... and one 0day later the process connects to somewhere in Lower Elbonia ;)

Regarding the filtering of junk events (such as the accept that returns EAGAIN), if we can support flexible conditions such as combining pid, syscall, direction, errno, etc., we should be able to filter out most of them.

I'm mildly pessimistic about flexible filtering in the kernel, precisely due to the overhead for non-filtered events.

We can benchmark to measure the overhead but it will always depend on the ratio of filtered to non-filtered events (is the non-filtered overhead greater than the overhead of generating events that will get immediately dropped by userspace?)

Long term, I'd love to see the probe generated on demand based on whatever filters we have, but I might not live long enough to see it happen ;)

wangyongfeng5 · 2023-08-31T06:53:18Z

Uhm the scenario is clear now, thank you!

Let's say, if you want to enable only some syscalls and disable others that are noisy, we have a solution already in place, "the interesting syscall" logic. As you may imagine you pass to the drivers a simple table saying which syscalls you want.

if I understood well here you are searching for something slightly different. You don't want to disable sycalls you just want to sampling them because maybe some accept events (as in your example) could be useful so you don't want to drop them all. If this is the case maybe we could follow the approach 1, i.e. "allow the driver users to specify which syscall they don't want to be sampled". Probably approach 1 is more convenient because we already have almost all in place (see UF_NEVER_DROP and UF_ALWAYS_DROP flags). We just need to add a custom logic to allow users to put these flags on the various syscalls.

WDYT? @gnosek @FedeDP @wangyongfeng5

I created another PR to implement the approach 1

Andreagit97 · 2023-08-31T09:30:32Z

thank you we will take a look ASAP right now we are a little bit busy for the release

poiana · 2023-11-29T09:46:49Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

FedeDP · 2023-11-29T09:47:48Z

/remove-lifecycle stale

Andreagit97 · 2023-11-29T14:23:20Z

yeah, it is not clear to us how to collocate this change between the existing ones, we know this is a valuable feature but we don't want to explode the complexity with yet another logic in the drivers... I am moving ti to /TBD until we have clearer ideas, sorry for that

poiana · 2024-02-27T15:49:21Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

FedeDP · 2024-02-27T15:51:27Z

/remove-lifecycle stale

poiana · 2024-05-27T15:53:13Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

Andreagit97 · 2024-05-28T07:31:24Z

/remove-lifecycle stale

poiana · 2024-08-26T10:10:19Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

Andreagit97 · 2024-08-27T08:44:07Z

/remove-lifecycle stale

poiana · 2024-11-25T10:12:24Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh with /remove-lifecycle stale.

Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Provide feedback via https://github.com/falcosecurity/community.

/lifecycle stale

poiana added kind/feature New feature or request dco-signoff: no area/driver-kmod do-not-merge/release-note-label-needed area/driver-bpf area/driver-modern-bpf area/libscap-engine-bpf area/libscap-engine-kmod area/libscap-engine-modern-bpf area/libscap labels Aug 29, 2023

poiana added area/libpman size/XL labels Aug 29, 2023

poiana requested review from hbrueckner and Molter73 August 29, 2023 07:08

poiana added the do-not-merge/hold label Aug 29, 2023

set sampling ratio of dropping for single syscall individually

4235cfd

Signed-off-by: Manny Wang <[email protected]>

wangyongfeng5 force-pushed the individual_dropping_ratio branch from e790789 to 4235cfd Compare August 29, 2023 08:04

poiana added dco-signoff: yes and removed dco-signoff: no labels Aug 29, 2023

Add individual sampling ratio of syscall

5d1e8de

Signed-off-by: Manny Wang <[email protected]>

wangyongfeng5 force-pushed the individual_dropping_ratio branch from 9bee2a4 to 5d1e8de Compare August 29, 2023 10:49

Add individual sampling ratio of syscall

9e5f247

Signed-off-by: Manny Wang <[email protected]>

gnosek reviewed Aug 29, 2023

View reviewed changes

Andreagit97 added this to the libs-backlog milestone Aug 30, 2023

poiana added the lifecycle/stale label Nov 29, 2023

poiana removed the lifecycle/stale label Nov 29, 2023

poiana added the lifecycle/stale label Feb 27, 2024

poiana removed the lifecycle/stale label Feb 27, 2024

poiana added the lifecycle/stale label May 27, 2024

poiana removed the lifecycle/stale label May 28, 2024

poiana added the lifecycle/stale label Aug 26, 2024

poiana removed the lifecycle/stale label Aug 27, 2024

poiana added the lifecycle/stale label Nov 25, 2024


		vpr_info("new sampling ratio: %d\n", new_sampling_ratio);
		vpr_info("new default sampling ratio: %d\n", new_sampling_ratio);

set sampling ratio of dropping for single syscall individually #1309

Are you sure you want to change the base?

set sampling ratio of dropping for single syscall individually #1309

Conversation

wangyongfeng5 commented Aug 29, 2023

poiana commented Aug 29, 2023

poiana commented Aug 29, 2023

poiana commented Aug 29, 2023

github-actions bot commented Aug 29, 2023 • edited Loading

FedeDP commented Aug 29, 2023 • edited Loading

Andreagit97 commented Aug 29, 2023

gnosek left a comment

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

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

Choose a reason for hiding this comment

wangyongfeng5 commented Aug 30, 2023 • edited Loading

Andreagit97 commented Aug 30, 2023

FedeDP commented Aug 30, 2023

wangyongfeng5 commented Aug 30, 2023 • edited Loading

FedeDP commented Aug 30, 2023

Andreagit97 commented Aug 30, 2023

wangyongfeng5 commented Aug 30, 2023

gnosek commented Aug 30, 2023

wangyongfeng5 commented Aug 31, 2023

Andreagit97 commented Aug 31, 2023

poiana commented Nov 29, 2023

FedeDP commented Nov 29, 2023

Andreagit97 commented Nov 29, 2023

poiana commented Feb 27, 2024

FedeDP commented Feb 27, 2024

poiana commented May 27, 2024

Andreagit97 commented May 28, 2024

poiana commented Aug 26, 2024

Andreagit97 commented Aug 27, 2024

poiana commented Nov 25, 2024

github-actions bot commented Aug 29, 2023 •

edited

Loading

FedeDP commented Aug 29, 2023 •

edited

Loading

wangyongfeng5 commented Aug 30, 2023 •

edited

Loading

wangyongfeng5 commented Aug 30, 2023 •

edited

Loading