[PATCH v3 bpf-next 0/5] selftests/bpf: Fix several issues in test_progs.c

[PATCH v3 bpf-next 0/5] selftests/bpf: Fix several issues in test_progs.c

[Feng Yang]

From: Feng Yang <yangfeng@kylinos.cn>

Fix several issues in test_progs.c

v3: Add fix incorrect error checking for pthread_create patch
    Memory allocation null checks for the worker logic are relatively complex;
    remove them for now and submit them separately in a follow-up patch.

v2: Fix several issues raised by sashiko-bot
    https://lore.kernel.org/all/20260722074748.674080-1-yangfeng59949@163.com/

v1: https://lore.kernel.org/all/20260721094404.593127-1-yangfeng59949@163.com/

Feng Yang (5):
  selftests/bpf: Fix double free of subtest_state->name
  selftests/bpf: Fix incorrect error checking for pthread_create
  selftests/bpf: Fix missing allocation null checks in test_progs.c
  selftests/bpf: Use calloc to allocate subtest_states
  selftests/bpf: Fix memory leak on subtest_states reallocation

 tools/testing/selftests/bpf/test_progs.c | 27 ++++++++++++++----------
 1 file changed, 16 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH v3 bpf-next 1/5] selftests/bpf: Fix double free of subtest_state->name

[Feng Yang]

From: Feng Yang <yangfeng@kylinos.cn>

The name has already been freed in the free_subtest_state function
and does not need to be freed again.

Fixes: 0925225956bb ("bpf/selftests: Add granular subtest output for prog_test")
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
 tools/testing/selftests/bpf/test_progs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 7ba82974ee78..1d3caf996971 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1886,7 +1886,6 @@ static int worker_main_send_subtests(int sock, struct test_state *state)
 			worker_main_send_log(sock, subtest_state->log_buf, subtest_state->log_cnt);
 
 		free_subtest_state(subtest_state);
-		free(subtest_state->name);
 	}
 
 out:
-- 
2.43.0

[PATCH v3 bpf-next 2/5] selftests/bpf: Fix incorrect error checking for pthread_create

[Feng Yang]

From: Feng Yang <yangfeng@kylinos.cn>

pthread_create returns 0 on success and a positive error code on failure;
it never returns a negative value. The current conditional branch can never be taken.
Failures during thread creation are silently ignored, which will lead to
invalid memory access when waiting on threads or dereferencing thread handles later.

Fixes: 91b2c0afd00c ("selftests/bpf: Add parallelism to test_progs")
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
 tools/testing/selftests/bpf/test_progs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 1d3caf996971..312743c4337f 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1741,7 +1741,7 @@ static void server_main(void)
 		data[i].worker_id = i;
 		data[i].sock_fd = env.worker_socks[i];
 		rc = pthread_create(&dispatcher_threads[i], NULL, dispatch_thread, &data[i]);
-		if (rc < 0) {
+		if (rc) {
 			perror("Failed to launch dispatcher thread");
 			exit(EXIT_ERR_SETUP_INFRA);
 		}
-- 
2.43.0

[PATCH v3 bpf-next 3/5] selftests/bpf: Fix missing allocation null checks in test_progs.c

[Feng Yang]

From: Feng Yang <yangfeng@kylinos.cn>

Add null checks after memory allocations to prevent potential segmentation faults.

Fixes: 79b453501310 ("tools/bpf: add a test for bpf_get_stack with tracepoint prog")
Fixes: 0925225956bb ("bpf/selftests: Add granular subtest output for prog_test")
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
 tools/testing/selftests/bpf/test_progs.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 312743c4337f..301c6e11ceaf 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -730,11 +730,14 @@ int compare_map_keys(int map1_fd, int map2_fd)
 int compare_stack_ips(int smap_fd, int amap_fd, int stack_trace_len)
 {
 	__u32 key, next_key, *cur_key_p, *next_key_p;
-	char *val_buf1, *val_buf2;
-	int i, err = 0;
+	char *val_buf1 = NULL, *val_buf2 = NULL;
+	int i, err = -ENOMEM;
 
 	val_buf1 = malloc(stack_trace_len);
 	val_buf2 = malloc(stack_trace_len);
+	if (!val_buf1 || !val_buf2)
+		goto out;
+	err = 0;
 	cur_key_p = NULL;
 	next_key_p = &key;
 	while (bpf_map_get_next_key(smap_fd, cur_key_p, next_key_p) == 0) {
@@ -1514,6 +1517,10 @@ static int dispatch_thread_send_subtests(int sock_fd, struct test_state *state)
 	int subtest_num = state->subtest_num;
 
 	state->subtest_states = malloc(subtest_num * sizeof(*subtest_state));
+	if (!state->subtest_states) {
+		state->subtest_num = 0;
+		return -ENOMEM;
+	}
 
 	for (int i = 0; i < subtest_num; i++) {
 		subtest_state = &state->subtest_states[i];
-- 
2.43.0

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

[Feng Yang]

From: Feng Yang <yangfeng@kylinos.cn>

An early return triggered by read_prog_test_msg leaves uninitialized elements,
which leads to memory corruption during free_test_states cleanup.

Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
 tools/testing/selftests/bpf/test_progs.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 301c6e11ceaf..07da45230c4b 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));
 	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;
 
-- 
2.43.0

[PATCH v3 bpf-next 5/5] selftests/bpf: Fix memory leak on subtest_states reallocation

[Feng Yang]

From: Feng Yang <yangfeng@kylinos.cn>

Fix memory leak in subtest_states reallocation,
and revert subtest_num if allocation fails.

Fixes: 0925225956bb ("bpf/selftests: Add granular subtest output for prog_test")
Signed-off-by: Feng Yang <yangfeng@kylinos.cn>
---
 tools/testing/selftests/bpf/test_progs.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 07da45230c4b..aa06bab30966 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -573,18 +573,19 @@ bool test__start_subtest_with_desc(const char *subtest_name, const char *subtest
 	struct subtest_state *subtest_state;
 	const char *subtest_display_name;
 	size_t sub_state_size = sizeof(*subtest_state);
+	void *tmp;
 
 	if (env.subtest_state)
 		test__end_subtest();
 
 	state->subtest_num++;
-	state->subtest_states =
-		realloc(state->subtest_states,
-			state->subtest_num * sub_state_size);
-	if (!state->subtest_states) {
+	tmp = realloc(state->subtest_states, state->subtest_num * sub_state_size);
+	if (!tmp) {
+		state->subtest_num--;
 		fprintf(stderr, "Not enough memory to allocate subtest result\n");
 		return false;
 	}
+	state->subtest_states = tmp;
 
 	subtest_state = &state->subtest_states[state->subtest_num - 1];
 
-- 
2.43.0

Re: [PATCH v3 bpf-next 3/5] selftests/bpf: Fix missing allocation null checks in test_progs.c

[sashiko-bot]

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

Pre-existing issues:
- [High] Early return from `dispatch_thread_send_subtests` leaves `subtest_states` partially uninitialized, leading to a crash during cleanup.
- [High] Missing NULL check for `strdup` in `dispatch_thread_send_subtests` leads to a potential segmentation fault.
- [High] Updating `subtest_num` before allocating `subtest_states` leads to a NULL pointer dereference during cleanup if an error occurs.
- [Medium] Failing `realloc` in `test__start_subtest_with_desc` overwrites the original pointer, leaking the array.
--

--- Patch [3]: [PATCH v3 bpf-next 3/5] selftests/bpf: Fix missing allocation null checks in test_progs.c ---
commit d01ffcd5c8b16e028e42c669b9b1234582456211
Author: Feng Yang <yangfeng@kylinos.cn>

selftests/bpf: Fix missing allocation null checks in test_progs.c

Add null checks after memory allocations to prevent potential segmentation faults.

Fixes: 79b453501310 ("tools/bpf: add a test for bpf_get_stack with tracepoint prog")
Fixes: 0925225956bb ("bpf/selftests: Add granular subtest output for prog_test")

> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 312743c4337fd..301c6e11ceafa 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c

[ ... ]

> @@ -1514,6 +1517,10 @@ static int dispatch_thread_send_subtests(int sock_fd, struct test_state *state)
>  	int subtest_num = state->subtest_num;
>  
>  	state->subtest_states = malloc(subtest_num * sizeof(*subtest_state));

[Severity: High]
This isn't a bug introduced by this patch, but because malloc() does not
zero-initialize memory, could the early return below cause a segmentation fault
during test teardown?

If read_prog_test_msg() returns an error, dispatch_thread_send_subtests()
returns 1:

tools/testing/selftests/bpf/test_progs.c:dispatch_thread_send_subtests() {
    ...
	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;
    ...
}

Any elements after index 'i' are left containing uninitialized memory. Later,
free_test_states() iterates through the entire array based on subtest_num and
attempts to free the pointers in each element:

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]);
    ...
}

tools/testing/selftests/bpf/test_progs.c:free_subtest_state() {
    ...
	free(state->log_buf);
    ...
}

Would it be safer to use calloc() instead of malloc() here to guarantee the
entire array is zero-initialized?

> +	if (!state->subtest_states) {
> +		state->subtest_num = 0;
> +		return -ENOMEM;
> +	}
>  
>  	for (int i = 0; i < subtest_num; i++) {
>  		subtest_state = &state->subtest_states[i];

[Severity: High]
This is a pre-existing issue, but does the strdup() call later in this
function need a NULL check?

tools/testing/selftests/bpf/test_progs.c:dispatch_thread_send_subtests() {
    ...
		if (read_prog_test_msg(sock_fd, &msg, MSG_SUBTEST_DONE))
			return 1;

		subtest_state->name = strdup(msg.subtest_done.name);
    ...
}

If memory allocation fails, subtest_state->name becomes NULL. When
dump_test_log() processes the results, it passes this to jsonw_string_field()
which eventually reaches jsonw_puts(). Since jsonw_puts() unconditionally loops
over the string characters, passing it a NULL pointer leads to a segmentation
fault.

[Severity: High]
This is also a pre-existing issue, but does updating state->subtest_num before
allocating state->subtest_states lead to a NULL pointer dereference?

In dispatch_thread(), subtest_num is set before the array is allocated:

tools/testing/selftests/bpf/test_progs.c:dispatch_thread() {
    ...
			state->subtest_num = msg.test_done.subtest_num;

			/* collect all logs */
			if (msg.test_done.have_log) {
				if (dispatch_thread_read_log(sock_fd,
							     &state->log_buf,
							     &state->log_cnt))
					goto error;
			}
    ...
}

If dispatch_thread_read_log() fails, the thread exits on error. During
cleanup, free_test_states() iterates up to state->subtest_num:

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]);
    ...
}

Because subtest_states is still NULL, free_subtest_state() receives a NULL
pointer (or near-NULL for j > 0) and attempts to free state->log_buf from it,
resulting in a crash.

[Severity: Medium]
This is another pre-existing issue, but does this realloc() pattern leak the
subtest_states array on failure?

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

If realloc() fails, it returns NULL and overwrites the original pointer, losing
the only reference to the previously allocated memory block.

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

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

[sashiko-bot]

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

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

>     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.
>
>     Signed-off-by: Feng Yang <yangfeng@kylinos.cn>

This looks like a fix for a memory corruption bug in
dispatch_thread_send_subtests().  The uninitialized elements that
free_test_states() later frees were introduced when the malloc() plus
per-element memset() allocation was added.  Should this include:

  Fixes: 0925225956bb ("bpf/selftests: Add granular subtest output for prog_test")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/29993688332