Re: [PATCH bpf v2 3/3] selftests/bpf: Test bpf_sock_destroy() on TIME_WAIT and listener socks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The file descriptor returned by `accept()` is leaked on the test failure path in `test_tcp_listen_pending()`.
--

commit a0eed787a7f06953fc57cb1d362195f000753ca9
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

selftests/bpf: Test bpf_sock_destroy() on TIME_WAIT and listener socks

This commit adds two subtests to the sock_destroy BPF selftest. The tests
ensure that bpf_sock_destroy() correctly handles sockets in TIME_WAIT and
listener sockets with pending connections in the accept queue.

> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> index 9c11938fe597d..c9f0885709c7e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c

[ ... ]

> @@ -110,6 +111,120 @@ static void test_tcp_server(struct sock_destroy_prog *skel)
>  		close(serv);
>  }
>  
> +static void test_tcp_listen_pending(struct sock_destroy_prog *skel)
> +{
> +	int serv = -1, clien = -1, n, serv_port;
> +	struct pollfd pfd = { .events = POLLIN };
> +	char buf[1];
> +
> +	serv = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);

[ ... ]

> +	/* Run iterator program that destroys server sockets. */
> +	start_iter_sockets(skel->progs.iter_tcp6_server);
> +
> +	n = accept(serv, NULL, NULL);
> +	if (!ASSERT_LT(n, 0, "accept on destroyed listener"))
> +		goto cleanup;

[Severity: Medium]
Does this code leak the file descriptor on the error path?

If accept(serv, NULL, NULL) unexpectedly succeeds, it returns a valid
file descriptor in n (where n >= 0). The ASSERT_LT(n, 0) check then
fails and execution jumps to the cleanup block.

Because n is not closed in cleanup, the accepted socket is leaked.

> +	ASSERT_EQ(errno, EINVAL, "error code on destroyed listener");
> +
> +	/* The unaccepted child was reset along with the listener. */
> +	n = recv(clien, buf, sizeof(buf), 0);
> +	if (!ASSERT_LT(n, 0, "client recv on reset child"))
> +		goto cleanup;
> +	ASSERT_EQ(errno, ECONNRESET, "error code on reset child");
> +
> +cleanup:
> +	if (clien != -1)
> +		close(clien);
> +	if (serv != -1)
> +		close(serv);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260906074135.185212-1-jiayuan.chen@linux.dev?part=3