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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Process leak due to overwriting child PID variable with the result of waitpid().
- [Low] BPF comment style violation for multiline comments.
--

--- Patch [2]: [PATCH bpf-next v3 2/2] selftests/bpf: add memcg async reclaim test ---
commit 4779648533dd0e49d1c2f5c9bdbbad1260ba333c
Author: Hui Zhu <zhuhui@kylinos.cn>

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..e990d1fdc79fd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_async_reclaim.c

[ ... ]

> +	low_pid = fork();
> +	if (!ASSERT_GE(low_pid, 0, "fork low"))
> +		goto cleanup;
> +	if (low_pid == 0)
> +		exit(real_test_child_work(CG_LOW_DIR, low_data_file,
> +					  low_time_file, read_times));
> +
> +	high_pid = fork();
> +	if (!ASSERT_GE(high_pid, 0, "fork high"))
> +		goto cleanup;
> +	if (high_pid == 0)
> +		exit(real_test_child_work(CG_HIGH_DIR, high_data_file,
> +					  high_time_file, read_times));
> +
> +	low_pid = waitpid(low_pid, &status, 0);
> +	if (!ASSERT_GT(low_pid, 0, "low waitpid"))
> +		goto cleanup;

[Severity: Medium]
If waitpid() fails and returns -1, does this overwrite low_pid with the error
value? 

Later in the cleanup path, the code relies on the PID being positive to kill
the left-behind child processes:

	if (ret) {
		if (high_pid > 0) {
			...
		}
		if (low_pid > 0) {
			kill(low_pid, SIGKILL);
			(void)waitpid(low_pid, NULL, 0);
		}
	}

Since low_pid was overwritten with -1, the cleanup logic will fail to kill the
child process, potentially leaving it running and consuming resources. Would it
be safer to store the waitpid() return value in a separate status or pid 
variable?

> +	/*
> +	 * The child has been reaped and its PID can already be reused,
> +	 * so mark it to keep cleanup from signaling an unrelated process.
> +	 */
> +	low_pid = -1;
> +	if (!ASSERT_TRUE(WIFEXITED(status), "low exited"))
> +		goto cleanup;
> +	if (WEXITSTATUS(status) != CHILD_EXIT_OK) {
> +		PRINT_FAIL("low child failed at: %s (exit status %d)",
> +			   child_exit_str(WEXITSTATUS(status)),
> +			   WEXITSTATUS(status));
> +		goto cleanup;
> +	}
> +
> +	high_pid = waitpid(high_pid, &status, 0);
> +	if (!ASSERT_GT(high_pid, 0, "high waitpid"))
> +		goto cleanup;

[Severity: Medium]
Similar to the issue above, if waitpid() returns an error, high_pid is
overwritten. This prevents the cleanup code from terminating the high_pid child
process if the wait call is interrupted.

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/memcg_async_reclaim.c b/tools/testing/selftests/bpf/progs/memcg_async_reclaim.c
> new file mode 100644
> index 0000000000000..225f0bc667113
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/memcg_async_reclaim.c

[ ... ]

> +static u64 wq_high_cgroup_id;
> +static u64 wq_low_cgroup_id;
> +
> +/* Statistics exposed to userspace through .bss, so the test can verify
> + * that reclaim actually happened instead of relying on timing alone.
> + */

[Severity: Low]
This isn't a bug, but the BPF subsystem prefers multi-line comments to have
the opening /* on its own line:

/*
 * Statistics exposed to userspace through .bss, so the test can verify
 * that reclaim actually happened instead of relying on timing alone.
 */

> +u64 reclaim_calls;
> +u64 reclaimed_pages;
> +

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