Re: [PATCH bpf] bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bpf, sockmap: incomplete data-race fix in sk_psock_start_verdict() and sock_map_unref()
--

commit 3aa9e245b4ee20f4b50e92e1de66b46757cb8a03
Author: Dudu Lu <phx0fer@gmail.com>

bpf, sockmap: use READ_ONCE/WRITE_ONCE for saved_data_ready in the verdict path

This commit annotates accesses to psock->saved_data_ready in
sk_psock_start_verdict() and sk_psock_stop_verdict() to address data races
with sock_map_link() and sock_map_unref().

> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index df385a5a961e8..0bfb8c4e7f36d 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -1260,10 +1260,10 @@ static void sk_psock_verdict_data_ready(struct sock *sk)
>  
>  void sk_psock_start_verdict(struct sock *sk, struct sk_psock *psock)
>  {
> -	if (READ_ONCE(psock->saved_data_ready))
> +	if (READ_ONCE(psock->saved_data_ready))
>  		return;
>  
> -	psock->saved_data_ready = sk->sk_data_ready;
> +	WRITE_ONCE(psock->saved_data_ready, sk->sk_data_ready);
>  	WRITE_ONCE(sk->sk_data_ready, sk_psock_verdict_data_ready);
>  	WRITE_ONCE(sk->sk_write_space, sk_psock_write_space);
>  }
> @@ -1273,9 +1273,9 @@ void sk_psock_stop_verdict(struct sock *sk, struct sk_psock *psock)
>  	psock_set_prog(&psock->progs.stream_verdict, NULL);
>  	psock_set_prog(&psock->progs.skb_verdict, NULL);
>  
> -	if (!psock->saved_data_ready)
> +	if (!READ_ONCE(psock->saved_data_ready))
>  		return;
>  
>  	WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready);
> -	psock->saved_data_ready = NULL;
> +	WRITE_ONCE(psock->saved_data_ready, NULL);
>  }

[Severity: Medium]
When a socket is concurrently added to and removed from sockmaps via
sock_map_update_elem() -> sock_map_link() and sock_map_delete_elem() ->
sock_map_unref() -> sock_map_del_link(), does this change actually resolve
the reported data races?

Every caller of sk_psock_start_verdict(), sk_psock_stop_verdict(),
sk_psock_start_strp(), and sk_psock_stop_strp() (namely sock_map_link() and
sock_map_del_link() in net/core/sock_map.c, and sk_psock_drop() in
net/core/skmsg.c) already holds write_lock_bh(&sk->sk_callback_lock).
Because all writes to psock->saved_data_ready are serialized by
sk->sk_callback_lock, the reads in sk_psock_start_verdict() and
sk_psock_stop_verdict() cannot race with a concurrent writer.

Meanwhile, sk_psock_stop_verdict() in net/core/skmsg.c still reads
psock->saved_data_ready with an unannotated plain load when restoring
sk->sk_data_ready:

	if (!READ_ONCE(psock->saved_data_ready))
		return;

	WRITE_ONCE(sk->sk_data_ready, psock->saved_data_ready);

And the sibling functions sk_psock_start_strp() and sk_psock_stop_strp() in
net/core/skmsg.c, as well as sock_map_link() in net/core/sock_map.c, also
use plain loads and stores for psock->saved_data_ready:

net/core/skmsg.c:sk_psock_start_strp() {
	if (psock->saved_data_ready)
		return;

	psock->saved_data_ready = sk->sk_data_ready;
	...
}

More importantly, the unlocked reads of psock->saved_data_ready that race
with sk_psock_start_verdict() happen in sock_map_unref() ->
sock_map_del_link() in net/core/sock_map.c under
spin_lock_bh(&psock->link_lock), before write_lock_bh(&sk->sk_callback_lock)
is acquired:

net/core/sock_map.c:sock_map_del_link() {
	...
	spin_lock_bh(&psock->link_lock);
	list_for_each_entry_safe(link, tmp, &psock->link, list) {
		if (link->link_raw == link_raw) {
			struct bpf_map *map = link->map;
			struct sk_psock_progs *progs = sock_map_progs(map);

			if (psock->saved_data_ready && progs->stream_parser)
				strp_stop = true;
			if (psock->saved_data_ready && progs->stream_verdict)
				verdict_stop = true;
			if (psock->saved_data_ready && progs->skb_verdict)
				verdict_stop = true;
			...
		}
	}
	spin_unlock_bh(&psock->link_lock);
	if (strp_stop || verdict_stop) {
		write_lock_bh(&sk->sk_callback_lock);
		if (strp_stop)
			sk_psock_stop_strp(sk, psock);
		if (verdict_stop)
			sk_psock_stop_verdict(sk, psock);
		...
		write_unlock_bh(&sk->sk_callback_lock);
	}
}

Because those reads in sock_map_del_link() remain unannotated plain loads
outside sk->sk_callback_lock, won't the data race between
sk_psock_start_verdict() and sock_map_unref() still trigger? In addition,
if sock_map_del_link() checks psock->saved_data_ready before taking
write_lock_bh(&sk->sk_callback_lock), can it observe psock->saved_data_ready
newly set by a concurrent sock_map_link() and then call
sk_psock_stop_verdict(), clearing psock->progs.stream_verdict and
psock->progs.skb_verdict and restoring sk->sk_data_ready for the newly
added link?

Finally, regarding the race between sock_map_link() and sock_map_unref()
mentioned in the commit description, doesn't that race occur in
net/core/sock_map.c where sock_map_link() -> sock_map_init_proto() writes
psock->psock_update_sk_prot without sk->sk_callback_lock while
sock_map_unref() -> sock_map_del_link() reads it under sk->sk_callback_lock:

net/core/sock_map.c:sock_map_init_proto() {
	if (!sk->sk_prot->psock_update_sk_prot)
		return -EINVAL;
	psock->psock_update_sk_prot = sk->sk_prot->psock_update_sk_prot;
	return sk->sk_prot->psock_update_sk_prot(sk, psock, false);
}

which is not touched by this patch?

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