| 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
| ||
|
Message-ID: <20240614185352.85977-1-kuniyu@amazon.com>
Date: Fri, 14 Jun 2024 11:53:52 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <pabeni@...hat.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kent.overstreet@...ux.dev>,
<kuba@...nel.org>, <kuni1840@...il.com>, <kuniyu@...zon.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().
From: Paolo Abeni <pabeni@...hat.com>
Date: Fri, 14 Jun 2024 12:49:35 +0200
> 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?
Will use cached sk_state.
>
> > 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)
> ?
Oops, will fix it.
Thanks!
Powered by blists - more mailing lists