netdev - Re: [PATCH v2 net 01/15] af_unix: Set sk->sk_state under unix_state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 10 Jun 2024 14:55:08 +0200
From: Michal Luczaj <mhal@...x.co>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: cong.wang@...edance.com, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, kuni1840@...il.com, netdev@...r.kernel.org,
 pabeni@...hat.com
Subject: Re: [PATCH v2 net 01/15] af_unix: Set sk->sk_state under
 unix_state_lock() for truly disconencted peer.

On 6/9/24 23:03, Kuniyuki Iwashima wrote:
> (...)
> Sorry, I think I was wrong and we can't use smp_store_release()
> and smp_load_acquire(), and smp_[rw]mb() is needed.
> 
> Given we avoid adding code in the hotpath in the original fix
> 8866730aed510 [0], I prefer adding unix_state_lock() in the SOCKMAP
> path again.
>
> [0]: https://lore.kernel.org/bpf/6545bc9f7e443_3358c208ae@john.notmuch/

You're saying smp_wmb() in connect() is too much for the hot path, do I
understand correctly?

> ---8<---
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index d3dbb92153f2..67794d2c7498 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -549,7 +549,7 @@ static bool sock_map_sk_state_allowed(const struct sock *sk)
>  	if (sk_is_tcp(sk))
>  		return (1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_LISTEN);
>  	if (sk_is_stream_unix(sk))
> -		return (1 << sk->sk_state) & TCPF_ESTABLISHED;
> +		return (1 << READ_ONCE(sk->sk_state)) & TCPF_ESTABLISHED;
>  	return true;
>  }
>  
> diff --git a/net/unix/unix_bpf.c b/net/unix/unix_bpf.c
> index bd84785bf8d6..1db42cfee70d 100644
> --- a/net/unix/unix_bpf.c
> +++ b/net/unix/unix_bpf.c
> @@ -159,8 +159,6 @@ int unix_dgram_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool re
>  
>  int unix_stream_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool restore)
>  {
> -	struct sock *sk_pair;
> -
>  	/* Restore does not decrement the sk_pair reference yet because we must
>  	 * keep the a reference to the socket until after an RCU grace period
>  	 * and any pending sends have completed.
> @@ -180,9 +178,9 @@ int unix_stream_bpf_update_proto(struct sock *sk, struct sk_psock *psock, bool r
>  	 * be a single matching destroy operation.
>  	 */
>  	if (!psock->sk_pair) {
> -		sk_pair = unix_peer(sk);
> -		sock_hold(sk_pair);
> -		psock->sk_pair = sk_pair;
> +		psock->sk_pair = unix_peer_get(sk);
> +		if (WARN_ON_ONCE(!psock->sk_pair))
> +			return -EINVAL;
>  	}
>  
>  	unix_stream_bpf_check_needs_rebuild(psock->sk_proto);
> ---8<---

FWIW, we've passed sock_map_sk_state_allowed(), so critical section can be
empty:

	if (!psock->sk_pair) {
		unix_state_lock(sk);
		unix_state_unlock(sk);
		sk_pair = READ_ONCE(unix_peer(sk));
		...
	}

Thanks,
Michal