Re: [PATCH bpf-next v3 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 1 potential issue(s) to consider:
- [Low] Multi-line comments in the new BPF selftest program do not follow the mandatory BPF subsystem comment style.
--

commit 0974a803b9e5fed2b4e8c0c5360735a19cae9901
Author: Amery Hung <ameryhung@gmail.com>

selftests/bpf: Add test for bpf_tcp_ops header option hooks

This commit adds a test for the bpf_tcp_ops header option hooks. It uses a
struct_ops program to reserve, store, and load an experimental TCP header
option to ensure the parse_hdr, hdr_opt_len, and write_hdr_opt callbacks
work together correctly.

> 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 0000000000000..46618a604d965
> --- /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 does this follow the BPF subsystem comment style?

Multi-line comments under BPF-related paths should have the opening /* on
its own line.

> +	bpf_reserve_hdr_opt(ctx, TEST_OPT_LEN, 0);
> +}
> +
> +SEC("struct_ops")
> +void BPF_PROG(test_write_hdr_opt, struct sock *sk, struct sk_buff *skb,
> +	      struct request_sock *req, struct sk_buff *syn_skb,
> +	      enum tcp_synack_type synack_type, __u32 opt_off)
> +{
> +	__u8 opt[TEST_OPT_LEN] = {
> +		TEST_OPT_KIND, TEST_OPT_LEN, TEST_OPT_D0, TEST_OPT_D1,
> +	};
> +
> +	/* bpf_store_hdr_opt() takes the program ctx (the kernel reads the
> +	 * outgoing skb from it); it appends after any options already written
> +	 * in the reserved window, rejects duplicates, and confines the write to
> +	 * the header option scratch. Stacks across progs in the cgroup hierarchy.
> +	 */

[Severity: Low]
This is a similar style issue. Should the opening /* be placed on its own line
to match the required multi-line comment format?

> +	if (bpf_store_hdr_opt(ctx, opt, sizeof(opt), 0))
> +		return;
> +
> +	write_cnt++;
> +}
> +
> +SEC("struct_ops")
> +void BPF_PROG(test_parse_hdr, struct sock *sk, struct sk_buff *skb)
> +{
> +	__u8 opt[TEST_OPT_LEN] = {
> +		TEST_OPT_KIND, TEST_OPT_LEN, TEST_OPT_D0, TEST_OPT_D1,
> +	};
> +
> +	parse_cnt++;
> +
> +	/* Look up the experimental option written by test_write_hdr_opt() in
> +	 * the incoming skb. For an experimental kind the search matches on the
> +	 * 2-byte magic in opt[2..3]; on a match the found option is copied back
> +	 * into opt[].
> +	 */

[Severity: Low]
Here is another multi-line comment that starts on the same line as the opening
/*. Could this be reformatted to align with the modern BPF comment guidelines?

> +	if (bpf_load_hdr_opt(ctx, opt, sizeof(opt), 0) < 0)
> +		return;
> +
> +	found_d0 = opt[2];
> +	found_d1 = opt[3];
> +	found_cnt++;
> +}

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