Re: [PATCH bpf-next v2 4/4] selftests/bpf: Add a test for arena fault-in under memory.max

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The test passes on unexpected crashes because it only asserts that the termination signal is not SIGSEGV, rather than strictly expecting SIGKILL from the memcg OOM killer.
--

--- Patch [4]: [PATCH bpf-next v2 4/4] selftests/bpf: Add a test for arena fault-in under memory.max ---
commit d5e6a0b924679b20cf7f391518ec1a560ae725e3
Author: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>

selftests/bpf: Add a test for arena fault-in under memory.max

This commit adds a BPF selftest for arena fault-in under a constrained memory
cgroup. It creates a child process to fault in an arena and verifies it is
killed by the memcg OOM killer rather than incorrectly dying from SIGSEGV.

> diff --git a/tools/testing/selftests/bpf/prog_tests/arena_memcg.c b/tools/testing/selftests/bpf/prog_tests/arena_memcg.c
> new file mode 100644
> index 0000000000000..ca039ebd3d674
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/arena_memcg.c
[ ... ]
> +void serial_test_arena_memcg(void)
> +{
[ ... ]
> +	/*
> +	 * Faulting a valid arena address until memory.max is hit must not look
> +	 * like an invalid access. Without the fix the fault path allocated with
> +	 * the non-blocking allocator, turned its -ENOMEM into VM_FAULT_SIGSEGV,
> +	 * and the child died with SIGSEGV on a valid address; now it is handled
> +	 * by the memcg OOM path and the child is killed by SIGKILL instead.
> +	 */
> +	if (!ASSERT_TRUE(WIFSIGNALED(status), "child killed by signal"))
> +		goto out;
> +	if (!ASSERT_NEQ(WTERMSIG(status), SIGSEGV, "not killed by SIGSEGV"))
> +		dump_memcg(read_cgroup_file);

[Severity: Medium]
Could this mask other failures by accepting any signal other than SIGSEGV?

If the child process crashes due to an unrelated issue like a libbpf internal
assertion (SIGABRT) or an invalid memory access (SIGBUS), this assertion would
evaluate to true and the test would silently pass.

Should this explicitly expect SIGKILL from the memcg OOM killer using
ASSERT_EQ(WTERMSIG(status), SIGKILL)?

> +out:
> +	if (cgroup_fd >= 0)
> +		close(cgroup_fd);
> +	cleanup_cgroup_environment();
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805091720.139924-1-jiayuan.chen@xxxxxxxxx?part=4