tracing: ratelimit fixes

ratelimiting arguments does not work with multiple actions. The issue is that if there is a rate-limit in one action, subsequent actions will also not be executed. To solve this we first constrain rate-limiting only to post actions, since that is the main use-case. If we need it for other actions, the code can be extended accordingly. There are two reasons why an event will not be posted: - users selecting NoPost - rate-limiting We push the check for both of these cases in the do_action. Signed-off-by: Kornilios Kourtis <[email protected]>
cilium · Aug 21, 2023 · 92a7652 · 92a7652
1 parent 98e9b6f
commit 92a7652
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 26 deletions.
diff --git a/bpf/process/types/basic.h b/bpf/process/types/basic.h
@@ -1798,27 +1798,30 @@ update_pid_tid_from_sock(struct msg_generic_kprobe *e, __u64 sockaddr)
 
 static inline __attribute__((always_inline)) __u32
 do_action(__u32 i, struct msg_generic_kprobe *e,
-	  struct selector_action *actions, struct bpf_map_def *override_tasks)
+	  struct selector_action *actions, struct bpf_map_def *override_tasks, bool *post)
 {
 	int signal __maybe_unused = FGS_SIGKILL;
 	int action = actions->act[i];
-	__u64 ratelimit_interval __maybe_unused = actions->act[++i];
 	__s32 error, *error_p;
 	int fdi, namei;
 	int newfdi, oldfdi;
 	int socki;
 	int err = 0;
 	__u64 id;
 
+	switch (action) {
+	case ACTION_NOPOST:
+		*post = false;
+		break;
+	case ACTION_POST: {
+		__u64 ratelimit_interval __maybe_unused = actions->act[++i];
 #ifdef __LARGE_BPF_PROG
-	// Only support rate limiting on later kernels
-	if (rate_limit(ratelimit_interval, e)) {
-		/* This action is rate-limited. */
-		return 0;
+		if (rate_limit(ratelimit_interval, e))
+			*post = false;
+#endif /* __LARGE_BPF_PROG */
+		break;
 	}
-#endif
 
-	switch (action) {
 	case ACTION_UNFOLLOWFD:
 	case ACTION_FOLLOWFD:
 		fdi = actions->act[++i];
@@ -1885,24 +1888,19 @@ static inline __attribute__((always_inline)) bool
 do_actions(struct msg_generic_kprobe *e, struct selector_action *actions,
 	   struct bpf_map_def *override_tasks)
 {
-	bool nopost = false;
+	bool post = true;
 	__u32 l, i = 0;
 
 #ifndef __LARGE_BPF_PROG
 #pragma unroll
 #endif
 	for (l = 0; l < MAX_ACTIONS; l++) {
 		if (!has_action(actions, i))
-			return !nopost;
-
-		i = do_action(i, e, actions, override_tasks);
-		if (!i)
-			return false;
-
-		nopost |= actions->act[i] == ACTION_NOPOST;
+			break;
+		i = do_action(i, e, actions, override_tasks, &post);
 	}
 
-	return !nopost;
+	return post;
 }
 
 static inline __attribute__((always_inline)) long

diff --git a/pkg/selectors/kernel.go b/pkg/selectors/kernel.go
@@ -752,13 +752,15 @@ func ParseMatchAction(k *KernelSelectorState, action *v1alpha1.ActionSelector, a
 
 	rateLimit := uint32(0)
 	if action.RateLimit != "" {
+		if act != ActionTypePost {
+			return fmt.Errorf("rate limiting can only applied to post action (was applied to '%s')", action.Action)
+		}
 		var err error
 		rateLimit, err = parseRateLimit(action.RateLimit)
 		if err != nil {
 			return err
 		}
 	}
-	WriteSelectorUint32(k, rateLimit)
 
 	switch act {
 	case ActionTypeFollowFd, ActionTypeCopyFd:
@@ -785,7 +787,9 @@ func ParseMatchAction(k *KernelSelectorState, action *v1alpha1.ActionSelector, a
 		WriteSelectorUint32(k, action.ArgSig)
 	case ActionTypeTrackSock, ActionTypeUntrackSock:
 		WriteSelectorUint32(k, action.ArgSock)
-	case ActionTypePost, ActionTypeNoPost:
+	case ActionTypePost:
+		WriteSelectorUint32(k, rateLimit)
+	case ActionTypeNoPost:
 		// no arguments
 	case ActionTypeSigKill:
 		// no arguments

diff --git a/pkg/selectors/kernel_test.go b/pkg/selectors/kernel_test.go
@@ -571,11 +571,11 @@ func TestInitKernelSelectors(t *testing.T) {
 	}
 
 	expected_selsize_small := []byte{
-		0xea, 0x00, 0x00, 0x00, // size = pids + args + actions + namespaces + capabilities  + 4
+		0xe6, 0x00, 0x00, 0x00, // size = pids + args + actions + namespaces + capabilities  + 4
 	}
 
 	expected_selsize_large := []byte{
-		0x1e, 0x01, 0x00, 0x00, // size = pids + args + actions + namespaces + namespacesChanges + capabilities + capabilityChanges + 4
+		0x1a, 0x01, 0x00, 0x00, // size = pids + args + actions + namespaces + namespacesChanges + capabilities + capabilityChanges + 4
 	}
 
 	expected_filters := []byte{
@@ -684,11 +684,10 @@ func TestInitKernelSelectors(t *testing.T) {
 		0x02, 0x00, 0x00, 0x00, // value 2
 
 		// actions header
-		28, 0x00, 0x00, 0x00, // size = (2 * sizeof(uint32) * number of actions) + args + 4
+		24, 0x00, 0x00, 0x00, // size = (2 * sizeof(uint32) * number of actions) + args
 		0x00, 0x00, 0x00, 0x00, // post to userspace
 		0x00, 0x00, 0x00, 0x00, // DontRepeatFor = 0
 		0x01, 0x00, 0x00, 0x00, // fdinstall
-		0x00, 0x00, 0x00, 0x00, // DontRepeatFor = 0
 		0x00, 0x00, 0x00, 0x00, // arg index of fd
 		0x01, 0x00, 0x00, 0x00, // arg index of string filename
 	}
@@ -711,11 +710,10 @@ func TestInitKernelSelectors(t *testing.T) {
 		102, 111, 111, 98, 97, 114, // value ascii "foobar"
 
 		// actions header
-		28, 0x00, 0x00, 0x00, // size = (2 * sizeof(uint32) * number of actions) + args + 4
+		24, 0x00, 0x00, 0x00, // size = (2 * sizeof(uint32) * number of actions) + args + 4
 		0x00, 0x00, 0x00, 0x00, // post to userspace
 		0x00, 0x00, 0x00, 0x00, // DontRepeatFor = 0
 		0x01, 0x00, 0x00, 0x00, // fdinstall
-		0x00, 0x00, 0x00, 0x00, // DontRepeatFor = 0
 		0x00, 0x00, 0x00, 0x00, // arg index of fd
 		0x01, 0x00, 0x00, 0x00, // arg index of string filename
 	}
@@ -790,6 +788,6 @@ func TestInitKernelSelectors(t *testing.T) {
 
 	b, _ := InitKernelSelectors(selectors, args, &actionArgTable)
 	if bytes.Equal(expected[0:len(expected)], b[0:len(expected)]) == false {
-		t.Errorf("InitKernelSelectors: expected %v bytes %v\n", expected, b[0:len(expected)])
+		t.Errorf("InitKernelSelectors:\nexpected %v\nbytes    %v\n", expected, b[0:len(expected)])
 	}
 }