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

On 8/5/26 5:29 PM, [email protected] wrote:
> 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 <[email protected]>
>
> 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)?


I think checking "oom_kill" in memory.events would be more accurate.


That said, I'm not sure patches 3 and 4 are necessary, they only 
exercise the memcg behavior,

so we could drop them when merging (if the series is accepted).


Both ok for me.