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

[Jiayuan Chen <jiayuan.chen@linux.dev>]

On 5/19/26 4:25 PM, sashiko-bot@kernel.org wrote:
> 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.


For this point above, I think it's not true since 
bpf_sock_ops_setsockopt will return EOPNOTSUPP when

bpf_sock->op <= BPF_SOCK_OPS_WRITE_HDR_OPT_CB


>
> 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)