Re: [PATCH bpf-next v2 15/15] selftests/bpf: Add test for bpf_tcp_ops header option hooks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unchecked ASSERT_EQ failures on blocking sockets can cause the test suite to hang indefinitely.
- [Low] Multi-line comments do not follow the mandatory BPF subsystem style.
--

commit a1c417fa7da14f2f4f18705173e644ec97f1f5c8
Author: Amery Hung <ameryhung@gmail.com>
Subject: Add a test exercising the bpf_tcp_ops parse_hdr, hdr_opt_len and write_hdr_opt members together with the header option helpers.

This commit adds a new BPF selftest that verifies the behavior of TCP header
option struct_ops callbacks and their associated helpers. It uses a
cgroup-attached program to reserve, write, and parse custom TCP header
options during socket communication.

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops_hdr.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops_hdr.c
> new file mode 100644
> index 000000000000..73e34d2be9a4
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ops_hdr.c

[ ... ]

> +static void send_recv(void)
> +{
> +	char buf[64] = {};
> +	int server_fd, client_fd, accept_fd;
> +	ssize_t n;
> +
> +	server_fd = start_server(AF_INET6, SOCK_STREAM, "::1", 0, 0);
> +	if (!ASSERT_GE(server_fd, 0, "start_server"))
> +		return;
> +
> +	client_fd = connect_to_fd(server_fd, 0);
> +	if (!ASSERT_OK_FD(client_fd, "connect_to_fd"))
> +		goto close_server;
> +
> +	accept_fd = accept(server_fd, NULL, NULL);
> +	if (!ASSERT_OK_FD(accept_fd, "accept"))
> +		goto close_client;
> +
> +	/* Exchange data both directions so option-bearing data packets
> +	 * are sent and parsed on each side.
> +	 */

[Severity: Low]
This isn't a bug, but does this multi-line comment violate the BPF subsystem
coding style? The BPF guidelines specify that the opening /* should be on
its own line.

> +	n = send(client_fd, buf, sizeof(buf), 0);
> +	ASSERT_EQ(n, sizeof(buf), "client_send");
> +	n = recv(accept_fd, buf, sizeof(buf), 0);

[Severity: Medium]
If the send() call fails, ASSERT_EQ will log a failure but execution will
fall through to recv(). Will this cause the test suite to hang indefinitely
waiting on the blocking recv() call? 

Should the return value of ASSERT_EQ be checked (e.g., using an if statement)
to prevent the test from hanging on local socket errors?

> +	ASSERT_EQ(n, sizeof(buf), "server_recv");
> +
> +	n = send(accept_fd, buf, sizeof(buf), 0);
> +	ASSERT_EQ(n, sizeof(buf), "server_send");
> +	n = recv(client_fd, buf, sizeof(buf), 0);
> +	ASSERT_EQ(n, sizeof(buf), "client_recv");
> +
> +	close(accept_fd);

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/bpf_tcp_ops_hdr.c b/tools/testing/selftests/bpf/progs/bpf_tcp_ops_hdr.c
> new file mode 100644
> index 000000000000..46618a604d96
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_tcp_ops_hdr.c

[ ... ]

> +SEC("struct_ops")
> +void BPF_PROG(test_hdr_opt_len, struct sock *sk, struct sk_buff *skb,
> +	      struct request_sock *req, struct sk_buff *syn_skb,
> +	      enum tcp_synack_type synack_type, unsigned int *remaining)
> +{
> +	hdr_opt_len_cnt++;
> +
> +	/* Reserve TEST_OPT_LEN bytes; the helper decrements *remaining. Stacks
> +	 * with other progs in the cgroup hierarchy.
> +	 */

[Severity: Low]
This isn't a bug, but should the opening /* be placed on its own line here 
and in the other multi-line comments in this file to match the BPF
subsystem comment style?

> +	bpf_reserve_hdr_opt(ctx, TEST_OPT_LEN, 0);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623175006.3136053-1-ameryhung@gmail.com?part=15