Re: [PATCH net-next 5/6] bpf: enable bpf timestamping rx in TCP layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] BPF sock_ops hook is called without holding the socket lock, violating core locking rules and risking data corruption.
- [Medium] Missing BPF RX timestamping for zero-copy receive (tcp_zerocopy_receive).
- [Medium] MSG_PEEK causes event duplication, corrupting BPF statistics.
- [Medium] Data race due to lockless read of sk->sk_bpf_cb_flags.
--

commit bdfc9a5e79a8f2a8060029cde75cfb38b923754a
Author: Jason Xing <kerneljasonxing@gmail.com>

bpf: enable bpf timestamping rx in TCP layer

This commit introduces BPF RX timestamping for TCP by adding a BPF hook
to tcp_recvmsg(). It evaluates timestamping flags and conditionally
calls bpf_skops_rx_timestamping().

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 21ece4c716121..64c69bb3578af 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2949,8 +2949,18 @@ int tcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int flags)
>  	release_sock(sk);
>  
>  	if ((cmsg_flags | msg->msg_get_inq) && ret >= 0) {
> -		if (cmsg_flags & TCP_CMSG_TS)
> -			tcp_recv_timestamp(msg, sk, &tss);
> +		if (cmsg_flags & TCP_CMSG_TS) {
> +			u32 tsflags = READ_ONCE(sk->sk_tsflags);
> +
> +			if (cgroup_bpf_enabled(CGROUP_SOCK_OPS) &&
> +			    SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_RX_TIMESTAMPING))

Since the socket lock was released above, is it safe to read
sk->sk_bpf_cb_flags here without READ_ONCE()? It appears
bpf_sock_ops_cb_flags_set() could modify this concurrently, potentially
causing a data race.

> +				bpf_skops_rx_timestamping(sk, &tss,
> +							  BPF_SOCK_OPS_TSTAMP_RCV_CB);

Does executing the BPF_PROG_TYPE_SOCK_OPS program here without holding the
socket lock risk data races? Since the program receives a full socket
context and can call helpers like bpf_setsockopt(), this might trigger
assertions in sock_owned_by_me(sk) or corrupt socket state.

If MSG_PEEK is passed in the flags, will this cause the BPF program to be
invoked multiple times for the same skb? The callback doesn't seem to
receive arguments indicating a peek, which might artificially inflate packet
counts and skew latency metrics.

> +			if (sock_flag(sk, SOCK_RCVTSTAMP) ||
> +			    tsflags & SOF_TIMESTAMPING_SOFTWARE ||
> +			    tsflags & SOF_TIMESTAMPING_RAW_HARDWARE)
> +				tcp_recv_timestamp(msg, sk, &tss);

Will packets processed via the zero-copy receive path miss these RX
timestamp events? High-performance applications using tcp_zerocopy_receive()
go through tcp_zc_finalize_rx_tstamp(), which directly calls
tcp_recv_timestamp() but seems to omit this new BPF callback.

> +		}
>  		if ((cmsg_flags & TCP_CMSG_INQ) | msg->msg_get_inq) {
>  			msg->msg_inq = tcp_inq_hint(sk);
>  			if (cmsg_flags & TCP_CMSG_INQ)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518082344.96647-1-kerneljasonxing@gmail.com?part=5