Re: [RFC PATCH net-next 2/4] tcp: add BPF_SOCK_OPS_TCP_TS_ID_CB to assign a TSval identifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf: bpf_load_hdr_opt() returns -EOPNOTSUPP for BPF_SOCK_OPS_TCP_TS_ID_CB
- [Low] bpf: UAPI multi-line comments violate subsystem guidelines
--

commit 5e1ec68deb60daa79748ce9bcb640700acc7dd70
Author: Willem Ferguson <wferguson@cloudflare.com>
    
tcp: add BPF_SOCK_OPS_TCP_TS_ID_CB to assign a TSval identifier
    
Let a sock_ops program pick the identifier that the previous patch
encodes in the low bits of TSval, for passively opened connections.

> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 732b35cc08d1c..b440a3fd96d0e 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
[ ... ]
> @@ -7137,8 +7138,12 @@ enum {
>  	 * options first before the BPF program does.
>  	 */
>  	BPF_SOCK_OPS_WRITE_HDR_OPT_CB_FLAG = (1<<6),
> +	/* Call bpf to select a TCP timestamp connection ID for a passive
> +	 * connection before sending the SYN-ACK.
> +	 */
> +	BPF_SOCK_OPS_TCP_TS_ID_CB_FLAG = (1 << 7),

[Severity: Low]
Does this multi-line comment violate the BPF subsystem guidelines?
The guidelines strictly require multi-line comments to have the opening /* on
its own line, with the comment text starting on the next line.

>  /* Mask of all currently supported cb flags */
> -	BPF_SOCK_OPS_ALL_CB_FLAGS       = 0x7F,
> +	BPF_SOCK_OPS_ALL_CB_FLAGS       = 0xFF,
>  };
>  
>  enum {
> @@ -7282,6 +7287,14 @@ enum {
>  					 * sendmsg timestamp with corresponding
>  					 * tskey.
>  					 */
> +	BPF_SOCK_OPS_TCP_TS_ID_CB,	/* Called after a passive SYN created a
> +					 * request socket and before the SYN-ACK is
> +					 * sent. Not called for SYN cookies. Arg2 is
> +					 * the number of low timestamp bits available
> +					 * for an identifier. Set reply to an identifier
> +					 * that fits in Arg2 bits. Replies are ignored
> +					 * for usec timestamps.
> +					 */

[Severity: Low]
Similar to the above, does this multi-line comment violate the BPF subsystem
guidelines by starting text on the same line as the opening /*?

[Severity: Medium]
The commit message claims that the BPF program can use bpf_load_hdr_opt() to
inspect the incoming SYN packet during the BPF_SOCK_OPS_TCP_TS_ID_CB callback.

However, the backing kernel helper bpf_sock_ops_load_hdr_opt() restricts access
by calling is_locked_tcp_sock_ops():

net/core/filter.c:is_locked_tcp_sock_ops() {
	return bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB;
}

Because BPF_SOCK_OPS_TCP_TS_ID_CB is appended to the bpf_sock_ops enum here, it
has a higher integer value than BPF_SOCK_OPS_WRITE_HDR_OPT_CB and fails this
check.

Does this cause bpf_load_hdr_opt() to unconditionally return -EOPNOTSUPP,
silently breaking the documented feature capability?

>  };
>  
>  /* List of TCP states. There is a build check in net/ipv4/tcp.c to detect

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013103906.2660384-1-wferguson@cloudflare.com?part=2