Re: [PATCH bpf-next 4/4] selftests/bpf: add memcg async reclaim test for bpf_wq/bpf_thread_wq

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Medium] Invalid assertion check for `low_cgroup_id`.
- [Medium] Error paths in run_high_low_workload() return incorrect status and leak a child process.
- [Medium] Hardcoded sleep(1) is used to synchronize with asynchronous BPF teardown, leading to flaky tests.
- [Low] Unnecessary file descriptor validation on BPF program obtained from a successfully loaded skeleton.

Pre-existing issues:
- [High] The kfunc `bpf_mem_cgroup_memory_events` fails to properly bounds check negative enum values.
- [High] The kfunc `bpf_mem_cgroup_vm_events` fails to properly bounds check negative enum values.
--

--- Patch [4]: [PATCH bpf-next 4/4] selftests/bpf: add memcg async reclaim test for bpf_wq/bpf_thread_wq ---
commit d37a321d7d40c100863512bb9f49b6ebc1834d5e
Author: Hui Zhu <[email protected]>

selftests/bpf: add memcg async reclaim test for bpf_wq/bpf_thread_wq

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
> --- /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)
> +{
[ ... ]
> +	*high_cgroup_id = get_cgroup_id(CG_HIGH_DIR);
> +	if (!ASSERT_GT(*high_cgroup_id, 0, "get_cgroup_id"))
> +		goto cleanup;
> +
> +	ret = create_and_get_cgroup(CG_LOW_DIR);
> +	if (!ASSERT_GE(ret, 0, "create_and_get_cgroup " CG_LOW_DIR))
> +		goto cleanup;
> +	close(ret);
> +
> +	*low_cgroup_id = get_cgroup_id(CG_LOW_DIR);
> +	if (!ASSERT_GT(*high_cgroup_id, 0, "get_cgroup_id"))
> +		goto cleanup;

[Severity: Medium]
Is it intentional to check *high_cgroup_id here instead of the newly
assigned *low_cgroup_id?

> +
> +	return 0;
> +
> +cleanup:
> +	cleanup_cgroup_environment();
> +	return -1;
> +}
[ ... ]
> +static int
> +run_high_low_workload(double *high_elapsed, double *low_elapsed, int read_times)
> +{
[ ... ]
> +	low_pid = fork();
> +	if (!ASSERT_GE(low_pid, 0, "fork low"))
> +		goto cleanup_low_time;
> +	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")) {
> +		(void)waitpid(low_pid, NULL, 0);
> +		goto cleanup_low_time;
> +	}
> +	if (high_pid == 0)
> +		exit(real_test_child_work(CG_HIGH_DIR, high_data_file,
> +					  high_time_file, read_times));
> +
> +	ret = waitpid(low_pid, &status, 0);
> +	if (!ASSERT_GT(ret, 0, "low waitpid"))
> +		goto cleanup_low_time;

[Severity: Medium]
If this assertion or the subsequent status checks fail, we jump to
cleanup_low_time. Does this leave the concurrently running high_pid as an
orphaned process? 

> +	if (!ASSERT_TRUE(WIFEXITED(status), "low exited"))
> +		goto cleanup_low_time;
[ ... ]
> +static int
> +setup_bpf(u64 high_cgroup_id, u64 low_cgroup_id,
> +	  struct memcg_async_reclaim **skel_ptr, bool use_thread_wq)
> +{
[ ... ]
> +	skel = memcg_async_reclaim__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "memcg_async_reclaim__open_and_load"))
> +		return -1;
> +
> +	if (use_thread_wq)
> +		prog_init_fd = bpf_program__fd(skel->progs.thread_wq_prog_init);
> +	else
> +		prog_init_fd = bpf_program__fd(skel->progs.wq_prog_init);
> +	if (!ASSERT_GE(prog_init_fd, 0, "bpf_program__fd"))
> +		goto error_out;

[Severity: Low]
Is this file descriptor check necessary? The skeleton API guarantees that all
programs have valid file descriptors after a successful __open_and_load().

> +
> +	err = bpf_prog_test_run_opts(prog_init_fd, &run_opts);
[ ... ]
> +void test_memcg_wq_async_reclaim(void)
> +{
[ ... ]
> +out:
> +	if (skel)
> +		memcg_async_reclaim__destroy(skel);
> +	/*
> +	 * Wait for bpf_wq to release the reference to cgroup
> +	 * to ensure the successful deletion of cgroup.
> +	 */
> +	sleep(1);
> +	cleanup_cgroup_environment();
> +}

[Severity: Medium]
Could this hardcoded sleep(1) lead to test flakiness if the asynchronous BPF
workqueue takes longer than one second to release its cgroup references
under load?

> +
> +void test_memcg_thread_wq_async_reclaim(void)
> +{
[ ... ]
> +out:
> +	if (skel)
> +		memcg_async_reclaim__destroy(skel);
> +	/*
> +	 * Wait for bpf_thread_wq to release the reference to cgroup
> +	 * to ensure the successful deletion of cgroup.
> +	 */
> +	sleep(1);
> +	cleanup_cgroup_environment();
> +}

[Severity: Medium]
Similarly, could this hardcoded sleep(1) cause intermittent test failures
if the thread_wq callback is delayed?


[Severity: High]
This isn't a bug introduced by this patch, but does the bounds check in
bpf_mem_cgroup_memory_events() correctly handle negative values? 

mm/bpf_memcontrol.c:bpf_mem_cgroup_memory_events() {
    ...
    if (unlikely(event >= MEMCG_NR_MEMORY_EVENTS))
        return (unsigned long)-1;

    return atomic_long_read(&memcg->memory_events[event]);
}

Since the parameter is signed, a BPF program passing a negative value 
(like -1) could bypass the check and cause an out-of-bounds memory read.


[Severity: High]
This is also a pre-existing issue, but does memcg_vm_event_item_valid()
protect against negative values when called by bpf_mem_cgroup_vm_events()?

mm/bpf_memcontrol.c:bpf_mem_cgroup_vm_events() {
    ...
    if (unlikely(!memcg_vm_event_item_valid(event)))
        return (unsigned long)-1;
    ...
}

mm/memcontrol.c:memcg_vm_event_item_valid() {
    if (idx >= NR_VM_EVENT_ITEMS)
        return false;
    ...
}

Similar to the previous issue, a negative enum value bypasses the upper-bound
check due to signed comparison, allowing out-of-bounds memory access.

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