Re: [PATCH bpf 2/2] selftests/bpf: Test bpf_sock_destroy() on a TIME_WAIT sock

[Jiayuan Chen <jiayuan.chen@linux.dev>]

在 9/3/26 9:07 PM, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The test creates a simultaneous close race condition that will cause it to randomly fail by observing the socket in TCP_CLOSING state instead of TCP_TIME_WAIT.
> - [Low] The multi-line comment uses the incorrect style for the BPF subsystem.
> --
>
> commit c91152edb079a3ce44e0f45e8d080efb432c5870
> Author: Jiayuan Chen <jiayuan.chen@linux.dev>
> Date:   Thu Sep 3 12:52:49 2026 +0000
>
> selftests/bpf: Test bpf_sock_destroy() on a TIME_WAIT sock
>
> This commit adds a tcp_timewait subtest to verify that bpf_sock_destroy()
> correctly handles TIME_WAIT sockets. A tcp iterator finds the timewait
> socket by its inherited cookie and destroys it.
>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>> index 9c11938fe597d..6ccc7cda410bf 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/sock_destroy.c
> [ ... ]
>> @@ -110,6 +110,65 @@ static void test_tcp_server(struct sock_destroy_prog *skel)
> [ ... ]
>> +	/* Active close from the client, then close the server side. Once
>> +	 * recv() sees EOF the server FIN has been processed and the client
>> +	 * sock is in TIME_WAIT. Block without timeout so a loaded CI box
>> +	 * can't race us.
>> +	 */
> [Severity: Low]
> Does this multi-line comment follow the BPF subsystem coding style? The
> guidelines prefer placing the opening /* on its own line before starting
> the comment text.
>
>> +	if (!ASSERT_OK(setsockopt(clien, SOL_SOCKET, SO_RCVTIMEO, &tv,
>> +				  sizeof(tv)), "clear rcvtimeo"))
>> +		goto cleanup;
>> +	if (!ASSERT_OK(shutdown(clien, SHUT_WR), "client shutdown"))
>> +		goto cleanup;
>> +
>> +	close(accept_serv);
>> +	accept_serv = -1;
>> +
>> +	/* block until return EOF */
>> +	n = recv(clien, buf, sizeof(buf), 0);
> [Severity: Medium]
> Could this sequence create a simultaneous close race condition that leads to
> random test failures?
>
> If shutdown(clien, SHUT_WR) sends a FIN, and the test immediately calls
> close(accept_serv) on the server without waiting for the client's FIN to be
> processed, the server might send its FIN before receiving the client's FIN.
>
> If these FINs cross (simultaneous close), the client will receive the
> server's FIN, enter the TCP_CLOSING state, and wake up the blocking recv()
> call. The subsequent BPF iterator would then run while the socket is still
> in TCP_CLOSING rather than TCP_TIME_WAIT.
>
> Because the socket is in TCP_CLOSING, bpf_skc_to_tcp_timewait_sock() will
> return NULL, causing the iterator to miss the socket and the assertion
> ASSERT_EQ(skel->bss->tw_found, 1) to fail.


Right, I think I can just recv(accept_serv) to make sure client's FIN

has been processed by server before closing it, so the two FINs won't
cross