Re: [PATCH v3 bpf-next 4/5] selftests/bpf: Use calloc to allocate subtest_states

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory leak and NULL pointer dereference in `test__start_subtest_with_desc()` on `realloc()` failure.
- [High] Protocol errors or worker crashes during log/subtest retrieval cause tests to be falsely reported as successful.
--

--- Patch [4]: [PATCH v3 bpf-next 4/5] selftests/bpf: Use calloc to allocate subtest_states ---
commit c27dc00f23d100647c7fe316391aa3eb9b2496b5
Author: Feng Yang <yangfeng@kylinos.cn>

selftests/bpf: Use calloc to allocate subtest_states
    
An early return triggered by read_prog_test_msg leaves uninitialized elements,
which leads to memory corruption during free_test_states cleanup.

> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 301c6e11ceafa..07da45230c4bf 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -1516,7 +1516,7 @@ static int dispatch_thread_send_subtests(int sock_fd, struct test_state *state)
>  	struct subtest_state *subtest_state;
>  	int subtest_num = state->subtest_num;
>  
> -	state->subtest_states = malloc(subtest_num * sizeof(*subtest_state));
> +	state->subtest_states = calloc(subtest_num, sizeof(*subtest_state));

[Severity: High]
This is a pre-existing issue, but is it possible to leak state->subtest_states
and cause a NULL pointer dereference in test__start_subtest_with_desc() on a
realloc() failure?

If realloc() fails, it returns NULL which overwrites the only pointer to
the existing array, leaking the previously allocated memory:

tools/testing/selftests/bpf/test_progs.c:test__start_subtest_with_desc() {
...
	state->subtest_num++;
	state->subtest_states =
		realloc(state->subtest_states,
			state->subtest_num * sub_state_size);
...
}

Because state->subtest_num is incremented before the failure, the cleanup
code in free_test_states() will attempt to iterate over the now-NULL
subtest_states array:

tools/testing/selftests/bpf/test_progs.c:free_test_states() {
...
		for (j = 0; j < test_state->subtest_num; j++)
			free_subtest_state(&test_state->subtest_states[j]);
...
}

Can this compute an invalid address and cause a segmentation fault when trying
to free the test states?

>  	if (!state->subtest_states) {
>  		state->subtest_num = 0;
>  		return -ENOMEM;
> @@ -1525,8 +1525,6 @@ static int dispatch_thread_send_subtests(int sock_fd, struct test_state *state)
>  	for (int i = 0; i < subtest_num; i++) {
>  		subtest_state = &state->subtest_states[i];
>  
> -		memset(subtest_state, 0, sizeof(*subtest_state));
> -
>  		if (read_prog_test_msg(sock_fd, &msg, MSG_SUBTEST_DONE))
>  			return 1;

[Severity: High]
This isn't a bug introduced by this patch, but what happens if a protocol error
or a worker crash occurs during log or subtest retrieval?

In dispatch_thread(), the test is marked as successful before all of its
logs and subtest states are collected:

tools/testing/selftests/bpf/test_progs.c:dispatch_thread() {
...
			state = &test_states[test_to_run];
			state->tested = true;
			state->error_cnt = msg.test_done.error_cnt;
...
			if (dispatch_thread_send_subtests(sock_fd, state))
				goto error;
...
}

If dispatch_thread_send_subtests() returns 1 because read_prog_test_msg()
fails (e.g., due to a worker crash or a closed socket), the dispatcher thread
jumps to the error label and exits.

Because state->tested is already true and error_cnt is 0 (assuming the main
test passed before the crash), calculate_summary_and_print_errors() adds the
test to succ_cnt. Does this falsely report the incomplete or crashed test as
successful and mask test suite failures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723085100.482147-1-yangfeng59949@163.com?part=4