Re: [RFC PATCH bpf-next 3/5] selftests/bpf: add tcp_splice basic round-trip test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] If the second `pthread_create()` fails in `run_bidir_write`, the function returns early without joining the first thread, leading to a leaked thread and a Use-After-Free on the stack.
- [Low] Multi-line comments violate the required BPF subsystem comment style.
--

commit 2de2ff2207c8434507b69004f912f22fbc96a325
Author: Cong Wang <xiyou.wangcong@gmail.com>

selftests/bpf: add tcp_splice basic round-trip test

Adds a test for the tcp_splice BPF functionality using a sock_ops program to
pair local TCP sockets via bpf_sock_splice_pair. The test includes basic
round-trip and bidirectional-write subtests.

> diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_splice.c b/tools/testing/selftests/bpf/prog_tests/tcp_splice.c
> new file mode 100644
> index 0000000000000..b80a1129c6aa8
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/tcp_splice.c
[ ... ]
> +	/* Drive the splice fast path: receiver enters recv() and publishes
> +	 * its bvec, sender then writes directly into it.
> +	 */

[Severity: Low]
This isn't a bug, but does this comment style violate the BPF subsystem
guidelines? Multi-line comments should have the opening /* on its own line.

[ ... ]
> +/* Bidirectional-write deadlock-avoidance test.
> + *
> + * Both sides issue send() before either calls recv(), the classic

[Severity: Low]
This isn't a bug, but shouldn't the opening /* be on its own line
for multi-line comments in the BPF subsystem?

> + * pattern that used to deadlock under synchronous rendezvous (and
> + * the actual cause of "kex_exchange_identification: write: Broken
> + * pipe" with SSH on loopback). The bounded-wait fallback in
> + * tcp_bpf_splice_sendmsg() must let both writes complete via the
> + * normal TCP path within ~1 ms, and the banners must arrive intact
> + * on the other side when recv() is called next.
> + */
> +static int run_bidir_write(int cgroup_fd, struct test_tcp_splice *skel)
> +{
[ ... ]
> +	/* Both sides write first, neither reads yet. Both must return
> +	 * within bounded time (no deadlock).
> +	 */
> +	cs.fd = cfd;
> +	ss.fd = sfd;
> +	if (!ASSERT_OK(pthread_create(&client_send_tid, NULL, send_thread, &cs),
> +		       "client send thread"))
> +		goto out;
> +	if (!ASSERT_OK(pthread_create(&server_send_tid, NULL, send_thread, &ss),
> +		       "server send thread"))
> +		goto out;

[Severity: Medium]
If the second pthread_create() fails, will this jump to out and return, leaving
client_send_tid running in the background? 

Since cs is allocated on the stack, could the background thread's execution of
send_thread() write to cs.n and cs.err after the stack frame is destroyed,
causing a use-after-free memory corruption in the test runner?

> +
> +	pthread_join(client_send_tid, NULL);
> +	pthread_join(server_send_tid, NULL);
[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/test_tcp_splice.c b/tools/testing/selftests/bpf/progs/test_tcp_splice.c
> new file mode 100644
> index 0000000000000..09c7f0f9e3110
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_tcp_splice.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Sock_ops BPF program that pairs locally-connected TCP sockets via the
> + * bpf_sock_splice_pair kfunc. Each side of an established loopback

[Severity: Low]
This isn't a bug, but shouldn't the opening /* be on its own line
for multi-line comments in the BPF subsystem?

> + * connection inserts itself into a sockhash keyed by its 4-tuple and
> + * looks up the peer using the swapped tuple. Whichever side finds the
> + * peer attempts to splice; the race loser sees -EEXIST.
> + */
[ ... ]
> +static __always_inline void mk_key(struct bpf_sock_ops *s,
> +				   struct flow_key *k, int swap)
> +{
> +	/* skops->local_port is already in host byte order. skops->remote_port
> +	 * is laid out as the network-order 16-bit port in the upper half of

[Severity: Low]
This isn't a bug, but shouldn't the opening /* be on its own line
for multi-line comments in the BPF subsystem?

> +	 * a u32 (see sock_ops_convert_ctx_access); bpf_ntohl produces the
> +	 * host-order port directly - no further shift.
> +	 */
> +	__u16 lport = (__u16)s->local_port;
> +	__u16 rport = bpf_ntohl(s->remote_port);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612011452.134466-1-xiyou.wangcong@gmail.com?part=3