netdev - Re: [PATCH v2 net-next 03/11] af_unix: Don't retry after unix_state_lock_nested() in unix_stream

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: Fri, 14 Jun 2024 12:49:35 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>, "David S. Miller"
	 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	 <kuba@...nel.org>
Cc: Kent Overstreet <kent.overstreet@...ux.dev>, Kuniyuki Iwashima
	 <kuni1840@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next 03/11] af_unix: Don't retry after
 unix_state_lock_nested() in unix_stream_connect().

On Tue, 2024-06-11 at 15:28 -0700, Kuniyuki Iwashima wrote:
> When a SOCK_(STREAM|SEQPACKET) socket connect()s to another one, we need
> to lock the two sockets to check their states in unix_stream_connect().
> 
> We use unix_state_lock() for the server and unix_state_lock_nested() for
> client with tricky sk->sk_state check to avoid deadlock.
> 
> The possible deadlock scenario are the following:
> 
>   1) Self connect()
>   2) Simultaneous connect()
> 
> The former is simple, attempt to grab the same lock, and the latter is
> AB-BA deadlock.
> 
> After the server's unix_state_lock(), we check the server socket's state,
> and if it's not TCP_LISTEN, connect() fails with -EINVAL.
> 
> Then, we avoid the former deadlock by checking the client's state before
> unix_state_lock_nested().  If its state is not TCP_LISTEN, we can make
> sure that the client and the server are not identical based on the state.
> 
> Also, the latter deadlock can be avoided in the same way.  Due to the
> server sk->sk_state requirement, AB-BA deadlock could happen only with
> TCP_LISTEN sockets.  So, if the client's state is TCP_LISTEN, we can
> give up the second lock to avoid the deadlock.
> 
>   CPU 1                 CPU 2                  CPU 3
>   connect(A -> B)       connect(B -> A)        listen(A)
>   ---                   ---                    ---
>   unix_state_lock(B)
>   B->sk_state == TCP_LISTEN
>   READ_ONCE(A->sk_state) == TCP_CLOSE
>                             ^^^^^^^^^
>                             ok, will lock A    unix_state_lock(A)
>              .--------------'                  WRITE_ONCE(A->sk_state, TCP_LISTEN)
>              |                                 unix_state_unlock(A)
>              |
>              |          unix_state_lock(A)
>              |          A->sk_sk_state == TCP_LISTEN
>              |          READ_ONCE(B->sk_state) == TCP_LISTEN
>              v                                    ^^^^^^^^^^
>   unix_state_lock_nested(A)                       Don't lock B !!
> 
> Currently, while checking the client's state, we also check if it's
> TCP_ESTABLISHED, but this is unlikely and can be checked after we know
> the state is not TCP_CLOSE.
> 
> Moreover, if it happens after the second lock, we now jump to the restart
> label, but it's unlikely that the server is not found during the retry,
> so the jump is mostly to revist the client state check.
> 
> Let's remove the retry logic and check the state against TCP_CLOSE first.
> 
> Note that sk->sk_state does not change once it's changed from TCP_CLOSE,
> so READ_ONCE() is not needed in the second state read in the first check.
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
>  net/unix/af_unix.c | 34 ++++++++--------------------------
>  1 file changed, 8 insertions(+), 26 deletions(-)
> 
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index c09bf2b03582..a6dc8bb360ca 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -1546,7 +1546,6 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
>  		goto out;
>  	}
>  
> -	/* Latch state of peer */
>  	unix_state_lock(other);
>  
>  	/* Apparently VFS overslept socket death. Retry. */
> @@ -1576,37 +1575,20 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,
>  		goto restart;
>  	}
>  
> -	/* Latch our state.
> -
> -	   It is tricky place. We need to grab our state lock and cannot
> -	   drop lock on peer. It is dangerous because deadlock is
> -	   possible. Connect to self case and simultaneous
> -	   attempt to connect are eliminated by checking socket
> -	   state. other is TCP_LISTEN, if sk is TCP_LISTEN we
> -	   check this before attempt to grab lock.
> -
> -	   Well, and we have to recheck the state after socket locked.
> +	/* self connect and simultaneous connect are eliminated
> +	 * by rejecting TCP_LISTEN socket to avoid deadlock.
>  	 */
> -	switch (READ_ONCE(sk->sk_state)) {
> -	case TCP_CLOSE:
> -		/* This is ok... continue with connect */
> -		break;
> -	case TCP_ESTABLISHED:
> -		/* Socket is already connected */
> -		err = -EISCONN;
> -		goto out_unlock;
> -	default:
> -		err = -EINVAL;
> +	if (unlikely(READ_ONCE(sk->sk_state) != TCP_CLOSE)) {
> +		err = sk->sk_state == TCP_ESTABLISHED ? -EISCONN : -EINVAL;

I find the mixed READ_ONCE()/plain read confusing. What about using a
single READ_ONCE() caching the return value?

>  		goto out_unlock;
>  	}
>  
>  	unix_state_lock_nested(sk, U_LOCK_SECOND);
>  
> -	if (sk->sk_state != TCP_CLOSE) {
> -		unix_state_unlock(sk);
> -		unix_state_unlock(other);
> -		sock_put(other);
> -		goto restart;
> +	if (unlikely(sk->sk_state != TCP_CLOSE)) {
> +		err = sk->sk_state == TCP_ESTABLISHED ? -EISCONN : -EINVAL;
> +		unix_state_lock(sk);

Should likely be:
		unix_state_unlock(sk)
?



Thanks!

Paolo