Re: [PATCH bpf v2] bpf, sockmap: annotate the psock teardown data races

[Jakub Sitnicki <jakub@cloudflare.com>]

On Thu, Oct 22, 2026 at 03:40 PM GMT, Alexei Starovoitov wrote:
> On Wed, Oct 21, 2026 at 10:48 PM Dudu Lu <phx0fer@gmail.com> wrote:
>> Annotate every one of these accesses with READ_ONCE()/WRITE_ONCE(),
>> matching the existing convention for the same fields: sk->sk_data_ready
>
> This makes KCSAN quiet and leaves the race where it is.
>
> sock_map_link() runs under the sock lock. sock_map_delete_elem() doesn't
> take it. sk_callback_lock is the only lock they share and sock_map_link()
> takes it after psock_set_prog() and sock_map_init_proto().
> So two tcp_bpf_update_proto(sk, psock, false) can run in parallel:
>   cpu0: sock_map_link() for a map with msg_parser sees
>         psock->progs.stream_verdict and picks TCP_BPF_TXRX
>   cpu1: sock_map_del_link() stops the verdict and stores TCP_BPF_TX
>   cpu0: stores TCP_BPF_TXRX

Right, so:
- map_update_elem syscall vs close(fd) - OK (lock_sock protected),
- map_update_elem syscall vs map_delete_elem syscall - RACY,
- map_update_elem syscall vs TCP RST (destroy) - RACY,
- map_update_elem from BPF vs close(fd) - RACY.

> The socket is left with tcp_bpf_recvmsg_parser() and no verdict prog.
> That's what commit c0d95d3380ee fixed.
> Take the lock earlier. Then psock_update_sk_prot needs no annotation.
>
> All accesses to saved_data_ready are under sk_callback_lock except the
> three reads in sock_map_del_link(). Move that check under the lock instead
> of annotating it. No need to touch skmsg.c and skmsg.h.
>
> Only the two lockless reads of sk->sk_prot need READ_ONCE().
>
> Something like this (compile tested only):
>
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 38df842..20fb605 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -152,11 +152,11 @@ static void sock_map_del_link(struct sock *sk,
>  			struct bpf_map *map = link->map;
>  			struct sk_psock_progs *progs = sock_map_progs(map);
>
> -			if (psock->saved_data_ready && progs->stream_parser)
> +			if (progs->stream_parser)
>  				strp_stop = true;
> -			if (psock->saved_data_ready && progs->stream_verdict)
> +			if (progs->stream_verdict)
>  				verdict_stop = true;
> -			if (psock->saved_data_ready && progs->skb_verdict)
> +			if (progs->skb_verdict)
>  				verdict_stop = true;
>  			list_del(&link->list);
>  			sk_psock_free_link(link);
> @@ -164,8 +164,11 @@ static void sock_map_del_link(struct sock *sk,
>  		}
>  	}
>  	spin_unlock_bh(&psock->link_lock);
> -	if (strp_stop || verdict_stop) {
> -		write_lock_bh(&sk->sk_callback_lock);
> +	if (!strp_stop && !verdict_stop)
> +		return;
> +
> +	write_lock_bh(&sk->sk_callback_lock);
> +	if (psock->saved_data_ready) {
>  		if (strp_stop)
>  			sk_psock_stop_strp(sk, psock);
>  		if (verdict_stop)
> @@ -173,8 +176,8 @@ static void sock_map_del_link(struct sock *sk,
>
>  		if (psock->psock_update_sk_prot)
>  			psock->psock_update_sk_prot(sk, psock, false);
> -		write_unlock_bh(&sk->sk_callback_lock);
>  	}
> +	write_unlock_bh(&sk->sk_callback_lock);
>  }
>
>  static void sock_map_unref(struct sock *sk, void *link_raw)
> @@ -202,7 +205,7 @@ static struct sk_psock *sock_map_psock_get_checked(struct sock *sk)
>  	rcu_read_lock();
>  	psock = sk_psock(sk);
>  	if (psock) {
> -		if (sk->sk_prot->close != sock_map_close) {
> +		if (READ_ONCE(sk->sk_prot)->close != sock_map_close) {
>  			psock = ERR_PTR(-EBUSY);
>  			goto out;
>  		}
> @@ -284,6 +287,7 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  		}
>  	}
>
> +	write_lock_bh(&sk->sk_callback_lock);
>  	if (msg_parser)
>  		psock_set_prog(&psock->progs.msg_parser, msg_parser);
>  	if (stream_parser)
> @@ -298,11 +302,11 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  	 */
>  	ret = sock_map_init_proto(sk, psock);
>  	if (ret < 0) {
> +		write_unlock_bh(&sk->sk_callback_lock);
>  		sk_psock_put(sk, psock);
>  		goto out;
>  	}
>
> -	write_lock_bh(&sk->sk_callback_lock);
>  	if (stream_parser && stream_verdict && !psock->saved_data_ready) {
>  		if (sk_is_tcp(sk))
>  			ret = sk_psock_init_strp(sk, psock);
> @@ -536,7 +540,7 @@ static bool sock_map_redirect_allowed(const struct sock *sk)
>
>  static bool sock_map_sk_is_suitable(const struct sock *sk)
>  {
> -	return !!sk->sk_prot->psock_update_sk_prot;
> +	return !!READ_ONCE(sk->sk_prot)->psock_update_sk_prot;
>  }
>
>  static bool sock_map_sk_state_allowed(const struct sock *sk)

Code LGTM