Re: [RFC PATCH v2 3/3] selftests/bpf: add tests for the bpf_numab_ops struct_ops

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] selftests/bpf: uninitialized skel pointer dereference on test_hierarchy error path
- [Medium] selftests/bpf: race condition leading to flaky test in test_attach_detach
--

commit 0d0b1887ad5f6f3d52adb583711d13aa30078932
Author: Yafang Shao <laoar.shao@gmail.com>

selftests/bpf: add tests for the bpf_numab_ops struct_ops

This commit adds selftests for the bpf_numab_ops struct_ops. It covers five
scenarios including attach/detach behavior, hierarchy aggregation, multi-policy
combination, inheritance, and surface-dispatching.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_numab_ops.c b/tools/testing/selftests/bpf/prog_tests/bpf_numab_ops.c
> new file mode 100644
> index 0000000000000..3e5dac8e31313
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_numab_ops.c

[ ... ]

> +static void test_attach_detach(void)
> +{

[ ... ]

> +	/*
> +	 * Detach and let a grace period pass so that no reader is still
> +	 * walking the old effective array, then verify the hook stopped
> +	 * running.
> +	 */
> +	before = skel->bss->enable_cnt;
> +	bpf_link__destroy(link);
> +	link = NULL;
> +	sleep(1);
> +	burn_cpu_ms(300);
> +	ASSERT_EQ(skel->bss->enable_cnt, before, "enable_cnt_after_detach");

[Severity: Medium]
Could this snapshot timing cause the test to be flaky?

Because the task is running during bpf_link__destroy() and BPF hooks are
attached via RCU, scheduler ticks can still trigger the hook and increment
the counter after the before snapshot is taken but before the detachment is
complete or during the grace period.

If any ticks hit the task during teardown, the counts will not match and the
test will spuriously fail. Should the baseline counter be snapshotted after
the link is destroyed and the RCU grace period (sleep(1)) has fully elapsed?

[ ... ]

> +static void test_hierarchy(void)
> +{
> +	struct bpf_numab_ops *skel;
> +	struct bpf_link *plink = NULL;
> +	int parent_fd, child_fd = -1;
> +
> +	parent_fd = test__join_cgroup(CGROUP_PATH);
> +	if (!ASSERT_GE(parent_fd, 0, "join_parent_cgroup"))
> +		return;
> +
> +	child_fd = create_and_get_cgroup(CHILD_PATH);
> +	if (!ASSERT_GE(child_fd, 0, "create_child_cgroup"))
> +		goto cleanup;
> +
> +	if (!ASSERT_OK(join_cgroup(CHILD_PATH), "join_child_cgroup"))
> +		goto cleanup;
> +
> +	skel = bpf_numab_ops__open_and_load();

[ ... ]

> +cleanup:
> +	bpf_link__destroy(plink);
> +	bpf_numab_ops__destroy(skel);

[Severity: High]
Does this code dereference an uninitialized pointer if cgroup creation fails?

The skel pointer is declared without initialization. If either
create_and_get_cgroup() or join_cgroup() fails, the code jumps to the cleanup
label before skel is assigned a value.

At the cleanup label, bpf_numab_ops__destroy(skel) is invoked with the
uninitialized skel pointer containing garbage stack data. This bypasses the
internal NULL check in the destructor and can cause a segmentation fault.

Would initializing struct bpf_numab_ops *skel = NULL; prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012120438.52849-1-laoar.shao@gmail.com?part=3