[bug] matchPIDs is using first pid only #3255

arthur-zhang · 2024-12-24T04:00:52Z

Fixes bug

Description

In the code, the function next_pid_value(index, f, ty) is called multiple times to update the index, but the updated value is not written back to sel->index. This wil cause external callers to be unable to obtain the updated state.

matchPIDs only use first pid

selectors:
- matchPIDs:
  - operator: NotIn
    values:
    - 0
    - 1

__u32 index = sel->index;

index = next_pid_value(index, f, ty);

FUNC_INLINE int next_pid_value(__u32 off, __u32 *f, __u32 ty)
{
	return off + 4;
}

offset use selecttor_filter->index

FUNC_INLINE int
process_filter_pid(struct selector_filter *sf, __u32 *f,
		   struct execve_map_value *enter, struct msg_ns *n,
		   struct msg_capabilities *c)
{
	__u32 sel, off = sf->index;  
	__u64 flags = sf->flags;
	__u64 pid;

	if (flags & PID_SELECTOR_FLAG_NSPID) {
		pid = enter->nspid;
	} else {
		pid = enter->key.pid;
	}

	if (off > 1000)
		sel = 0;
	else {
		__u64 o = (__u64)off;
		o = o / 4;
		asm volatile("%[o] &= 0x3ff;\n"
			     : [o] "+r"(o));
		sel = f[o];
	}
}

release note

Fix selector filter to properly update and assign `sel->index` in `pfilter.h`

olsajiri

nice catch, could you please add test for the 2 pids matching spec? thanks

olsajiri · 2024-12-25T22:27:50Z

bpf/process/pfilter.h

@@ -342,15 +342,19 @@ selector_match(__u32 *f, struct selector_filter *sel,
 four:
 	res4 = process_filter(sel, f, enter, &msg->ns, &msg->caps);
 	index = next_pid_value(index, f, ty);
+	sel->index = index;
 three:


could we do directly sel->index = next_pid_value(index, f, ty); ?

maybe not. If we do that, index is not changed. and sel->index will be 0 and 4(using first two pids), not more pids will be chosen. (I already tested this)

maybe we can remove index variable and use sel->index = next_pid_value(sel->index, f, ty) directly

arthur-zhang · 2024-12-27T04:04:32Z

nice catch, could you please add test for the 2 pids matching spec? thanks

OK，I will spend some time to figure out how to do that.

netlify · 2025-01-06T09:52:55Z

✅ Deploy Preview for tetragon ready!

Name	Link
🔨 Latest commit	`ac4f9c7`
🔍 Latest deploy log	https://app.netlify.com/sites/tetragon/deploys/677ba7bf932f710008795cf9
😎 Deploy Preview	https://deploy-preview-3255--tetragon.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

arthur-zhang · 2025-01-06T11:40:48Z

nice catch, could you please add test for the 2 pids matching spec? thanks

write a simple test for that， please take some time to review it

mtardy · 2025-01-06T17:18:06Z

bpf/tests/pid_match_test.go

Hey thanks for taking the time to write such tests, I just wonder if there was a more straightforward way instead of adding a tests under bpf/tests. It's mostly because those are used for BPF unit tests and that would be maybe better to put the actual testing Go code under pkg/someappropriatepkg so that it gets tested along the other tests. Would you know a good place @olsajiri maybe?

hum, I dont mind it there.. it's testing the bpf selector_match function, which seems ok

olsajiri

looks good, left some comments
please split the fix and test code in separate commits

olsajiri · 2025-01-06T18:02:41Z

bpf/tests/pid_match_test.go

+		return fmt.Errorf("failed to update execve map: %w", err)
+	}
+	_, err = ctx.prog.Run(&ebpf.RunOptions{})
+	return err


why not have test_pid_match to return the err and take get it in here,
and you could drop test_result_map map

olsajiri · 2025-01-06T18:03:11Z

bpf/tests/pid_match_test.c

+	res = selector_match(f, &sel, enter, NULL, &process_filter_pid);
+
+	err = map_update_elem(&test_result_map, &zero, &res, BPF_ANY);
+	if (err) {


no need for the condition, but please the other comment for runProg

olsajiri · 2025-01-06T18:07:11Z

bpf/tests/pid_match_test.c

+	struct execve_map_value *enter;
+
+	f = map_lookup_elem(&test_filter_map, &zero);
+


extra empty line

olsajiri · 2025-01-06T18:07:45Z

bpf/tests/pid_match_test.c

+	f = map_lookup_elem(&test_filter_map, &zero);
+
+	if (!f) {
+		return 0;


no need for {} in here

Signed-off-by: arthur-zhang <[email protected]>

arthur-zhang requested a review from a team as a code owner December 24, 2024 04:00

arthur-zhang requested a review from olsajiri December 24, 2024 04:00

arthur-zhang changed the title ~~fix: update selector_filter index assignment to use sel->index~~ [bug] matchPIDs is using first pid only Dec 24, 2024

arthur-zhang force-pushed the dev3 branch 2 times, most recently from 2360997 to 14ca790 Compare December 24, 2024 07:04

olsajiri reviewed Dec 25, 2024

View reviewed changes

arthur-zhang force-pushed the dev3 branch from 14ca790 to ac4f9c7 Compare January 6, 2025 09:51

arthur-zhang force-pushed the dev3 branch from ac4f9c7 to 0ce1cb4 Compare January 6, 2025 10:07

arthur-zhang requested a review from olsajiri January 6, 2025 10:09

mtardy reviewed Jan 6, 2025

View reviewed changes

olsajiri reviewed Jan 6, 2025

View reviewed changes

bpf/tests/pid_match_test.c

f = map_lookup_elem(&test_filter_map, &zero);

if (!f) {

return 0;

Copy link

Contributor

olsajiri Jan 6, 2025

Choose a reason for hiding this comment

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

no need for {} in here

fix: update selector_filter index assignment to use sel->index

c562818

Signed-off-by: arthur-zhang <[email protected]>

arthur-zhang force-pushed the dev3 branch from 0ce1cb4 to 5428d2d Compare January 7, 2025 01:52

arthur-zhang requested review from olsajiri and mtardy January 7, 2025 01:53

add bpf test for selector_match

c9811fa

Signed-off-by: arthur-zhang <[email protected]>

arthur-zhang force-pushed the dev3 branch from 5428d2d to c9811fa Compare January 7, 2025 02:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bug] matchPIDs is using first pid only #3255

[bug] matchPIDs is using first pid only #3255

arthur-zhang commented Dec 24, 2024 •

edited

Loading

olsajiri left a comment

olsajiri Dec 25, 2024

arthur-zhang Dec 26, 2024 •

edited

Loading

arthur-zhang commented Dec 27, 2024

netlify bot commented Jan 6, 2025

arthur-zhang commented Jan 6, 2025 •

edited

Loading

mtardy Jan 6, 2025

olsajiri Jan 6, 2025

olsajiri left a comment

olsajiri Jan 6, 2025

olsajiri Jan 6, 2025

olsajiri Jan 6, 2025

olsajiri Jan 6, 2025

		struct execve_map_value *enter;

		f = map_lookup_elem(&test_filter_map, &zero);

[bug] matchPIDs is using first pid only #3255

Are you sure you want to change the base?

[bug] matchPIDs is using first pid only #3255

Conversation

arthur-zhang commented Dec 24, 2024 • edited Loading

Description

release note

olsajiri left a comment

Choose a reason for hiding this comment

olsajiri Dec 25, 2024

Choose a reason for hiding this comment

arthur-zhang Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

arthur-zhang commented Dec 27, 2024

netlify bot commented Jan 6, 2025

✅ Deploy Preview for tetragon ready!

arthur-zhang commented Jan 6, 2025 • edited Loading

mtardy Jan 6, 2025

Choose a reason for hiding this comment

olsajiri Jan 6, 2025

Choose a reason for hiding this comment

olsajiri left a comment

Choose a reason for hiding this comment

olsajiri Jan 6, 2025

Choose a reason for hiding this comment

olsajiri Jan 6, 2025

Choose a reason for hiding this comment

olsajiri Jan 6, 2025

Choose a reason for hiding this comment

olsajiri Jan 6, 2025

Choose a reason for hiding this comment

arthur-zhang commented Dec 24, 2024 •

edited

Loading

arthur-zhang Dec 26, 2024 •

edited

Loading

arthur-zhang commented Jan 6, 2025 •

edited

Loading