Re: [PATCH bpf-next 3/4] selftests/bpf: add thread_wq cgroup test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing memory barriers between the worker kthread and userspace polling can cause test flakiness on weakly-ordered architectures.
- [Medium] The test uses an arbitrary `sleep(2)` to wait for asynchronous map destruction, creating a race condition that can leak the cgroup.
--

--- Patch [3]: [PATCH bpf-next 3/4] selftests/bpf: add thread_wq cgroup test ---
commit 0a433d40343251b2dbfb625551f8882c1faf6d99
Author: Hui Zhu <zhuhui@kylinos.cn>

selftests/bpf: add thread_wq cgroup test

This commit adds test cases for bpf_thread_wq with cgroup attachment.
It tests execution both within a specified cgroup and without cgroup
attachment, verifying that callbacks run in the appropriate context.

> diff --git a/tools/testing/selftests/bpf/prog_tests/thread_wq_cgroup.c b/tools/testing/selftests/bpf/prog_tests/thread_wq_cgroup.c
> new file mode 100644
> index 0000000000000..7537b03f17e2a
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/thread_wq_cgroup.c
> @@ -0,0 +1,87 @@
[ ... ]
> +	for (waited_secs = 0; waited_secs < WAIT_TIMEOUT_SECS; waited_secs++) {
> +		if (skel->bss->twq_done)
> +			break;
> +		sleep(1);
> +	}
> +	if (!ASSERT_TRUE(skel->bss->twq_done, "twq_done in cgroup"))
> +		goto cleanup;
> +	if (!ASSERT_EQ(skel->bss->callback_cgroup_id, cg_id,
> +		       "callback_cgroup_id in cgroup"))

[Severity: Medium]
Could this result in spurious test failures on architectures with weak memory
ordering?

Since the userspace reader loops and polls the twq_done flag without an
acquire memory barrier, might it observe twq_done == 1 but read a stale
callback_cgroup_id of 0, thus failing the check in test_thread_wq_cgroup()?

> +		goto cleanup;
> +
> +	/* Run bpf_thread_wq without cgroup attachment (cgroup_id = 0). */
[ ... ]
> +cleanup:
> +	if (skel) {
> +		thread_wq_cgroup__destroy(skel);
> +		/* Wait thread_wq kthread quit. */
> +		sleep(2);
> +	}
> +	if (cg_fd >= 0)
> +		close(cg_fd);
> +	cleanup_cgroup_environment();
> +}

[Severity: Medium]
Is it possible for the test cgroup to leak if the map destruction takes
longer than 2 seconds?

The cleanup path relies on an arbitrary sleep(2) to wait for asynchronous map
destruction. When thread_wq_cgroup__destroy() closes the map file descriptor,
it triggers deferred map destruction via a system workqueue.

If the system is under heavy load and this takes longer than 2 seconds,
could cleanup_cgroup_environment() attempt to remove the test cgroup while
the kthread is still attached, causing the removal to fail with -EBUSY?

> diff --git a/tools/testing/selftests/bpf/progs/thread_wq_cgroup.c b/tools/testing/selftests/bpf/progs/thread_wq_cgroup.c
> new file mode 100644
> index 0000000000000..c70a37f55397d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/thread_wq_cgroup.c
> @@ -0,0 +1,56 @@
[ ... ]
> +__u64 target_cgroup_id;
> +__u64 callback_cgroup_id;
> +int twq_done;
> +int test_key;
> +
> +static int twq_callback(void *map, int *key, void *value)
> +{
> +	callback_cgroup_id = bpf_get_current_cgroup_id();
> +	twq_done = 1;
> +	return 0;
> +}

[Severity: Medium]
Can this write ordering lead to stale payload reads in userspace on weakly
ordered architectures?

The BPF worker kthread stores callback_cgroup_id and then sets twq_done = 1
without a release memory barrier. Can the CPU or compiler reorder these
operations in twq_callback(), allowing userspace to see twq_done == 1
before the payload is visible?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786086076.git.zhuhui@kylinos.cn?part=3