Re: [PATCH bpf-next v2 2/2] selftests/bpf: add memcg async reclaim test

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Test fails consistently on systems where `/tmp` is a `tmpfs` because file operations will create unevictable shmem pages, leading to an OOM kill.
- [Medium] Race condition in the cleanup path can send SIGKILL to unrelated processes due to sending signals to reaped PIDs.
--

--- Patch [2]: [PATCH bpf-next v2 2/2] selftests/bpf: add memcg async reclaim test ---
commit bbf998f1580c0a48a6f9d3478a89f37c3c38e8c9
Author: Hui Zhu <[email protected]>

selftests/bpf: add memcg async reclaim test

Add memcg_async_reclaim selftest that verifies BPF-driven async
proactive reclaim can mitigate refault-induced slowdown under memory
pressure.

> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c
> new file mode 100644
> index 0000000000000..6fab88203e7d3
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c

[ ... ]

> +static int setup_high_low_cgroups(u64 *high_cgroup_id, u64 *low_cgroup_id)
> +{

[ ... ]

> +	ret = write_cgroup_file(CG_DIR, "memory.swap.max", "0");
> +	if (!ASSERT_OK(ret, "write_cgroup_file memory.swap.max"))
> +		goto cleanup;

[ ... ]

> +static int
> +run_high_low_workload(double *high_elapsed, double *low_elapsed, int read_times)
> +{
> +	char high_data_file[] = "/tmp/memcg_async_high_data_XXXXXX";
> +	char low_data_file[] = "/tmp/memcg_async_low_data_XXXXXX";

[Severity: Medium]
Will this test consistently fail with an OOM kill on systems where /tmp is
mounted as tmpfs?

Since the test explicitly disables swap via "memory.swap.max" = "0", any
tmpfs pages created by these workloads become completely unevictable. This
will cause the cgroup to hit its memory limit and OOM instead of triggering
reclaim.

Additionally, tmpfs pages are treated as anonymous memory rather than file
pages, so they will not generate the WORKINGSET_REFAULT_FILE events that the
BPF program relies on in get_cgroup_event().

Could these temporary files be created in a directory backed by a standard
filesystem, or could the test skip if /tmp is a tmpfs?

[ ... ]

> +	if (!ASSERT_GT(waitpid(low_pid, &status, 0), 0, "low waitpid"))
> +		goto cleanup;
> +	if (!ASSERT_TRUE(WIFEXITED(status), "low exited"))
> +		goto cleanup;
> +	if (!ASSERT_EQ(WEXITSTATUS(status), 0, "low exit status"))
> +		goto cleanup;
> +
> +	if (!ASSERT_GT(waitpid(high_pid, &status, 0), 0, "high waitpid"))
> +		goto cleanup;
> +	if (!ASSERT_TRUE(WIFEXITED(status), "high exited"))
> +		goto cleanup;
> +	if (!ASSERT_EQ(WEXITSTATUS(status), 0, "high exit status"))
> +		goto cleanup;
> +
> +	if (get_time(high_time_file, high_elapsed))
> +		goto cleanup;
> +	if (get_time(low_time_file, low_elapsed))
> +		goto cleanup;
> +
> +	ret = 0;
> +
> +cleanup:
> +	/* On failure, make sure no child process is left behind */
> +	if (ret) {
> +		if (high_pid > 0) {
> +			kill(high_pid, SIGKILL);
> +			(void)waitpid(high_pid, NULL, 0);
> +		}
> +		if (low_pid > 0) {
> +			kill(low_pid, SIGKILL);
> +			(void)waitpid(low_pid, NULL, 0);
> +		}
> +	}

[Severity: Medium]
Can this cleanup path race with PID reuse and send SIGKILL to unrelated
processes?

If the initial waitpid(low_pid, &status, 0) succeeds but a subsequent check
like ASSERT_EQ(WEXITSTATUS(status), 0, ...) fails, the code will jump to
cleanup. Because low_pid is still greater than 0, kill(low_pid, SIGKILL)
will be called.

Since waitpid() has already reaped the child process, the operating system
might have reused the PID for a new process by the time kill() is executed.
Because selftests often run as root, this would blindly terminate whatever
process happened to receive that PID.

Should the pid variables be set to -1 immediately after a successful
waitpid() to prevent them from being signaled during cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2