The patch 1 fixes a bug that SOCK_DGRAM's sk->sk_state is changed
to TCP_CLOSE even if the socket is connect()ed to another socket.
The rest of this series annotates lockless accesses to the following
fields:
* sk->sk_state
* sk->sk_sndbuf
* net->unx.sysctl_max_dgram_qlen
* sk->sk_receive_queue.qlen
* sk->sk_shutdown
Note that with this series there is skb_queue_empty() left in
unix_dgram_disconnected() that needs to be changed to lockless
version, and unix_peer(other) access there should be protected
by unix_state_lock().
This will require some refactoring, so another series will follow.
Kuniyuki Iwashima (15):
af_unix: Set sk->sk_state under unix_state_lock() for truly
disconencted peer.
af_unix: Annodate data-races around sk->sk_state for writers.
af_unix: Annotate data-race of sk->sk_state in unix_inq_len().
af_unix: Annotate data-races around sk->sk_state in unix_write_space()
and poll().
af_unix: Annotate data-race of sk->sk_state in unix_stream_connect().
af_unix: Annotate data-race of sk->sk_state in unix_accept().
af_unix: Annotate data-races around sk->sk_state in sendmsg() and
recvmsg().
af_unix: Annotate data-race of sk->sk_state in unix_stream_read_skb().
af_unix: Annotate data-races around sk->sk_state in UNIX_DIAG.
af_unix: Annotate data-races around sk->sk_sndbuf.
af_unix: Annotate data-race of net->unx.sysctl_max_dgram_qlen.
af_unix: Use unix_recvq_full_lockless() in unix_stream_connect().
af_unix: Use skb_queue_empty_lockless() in unix_release_sock().
af_unix: Use skb_queue_len_lockless() in sk_diag_show_rqlen().
af_unix: Annotate data-race of sk->sk_shutdown in sk_diag_fill().
net/unix/af_unix.c | 90 +++++++++++++++++++++++-----------------------
net/unix/diag.c | 12 +++----
2 files changed, 50 insertions(+), 52 deletions(-)
--
2.30.2
When a SOCK_DGRAM socket connect()s to another socket, the both sockets'
sk->sk_state are changed to TCP_ESTABLISHED so that we can register them
to BPF SOCKMAP.
When the socket disconnects from the peer by connect(AF_UNSPEC), the state
is set back to TCP_CLOSE.
Then, the peer's state is also set to TCP_CLOSE, but the update is done
locklessly and unconditionally.
Let's say socket A connect()ed to B, B connect()ed to C, and A disconnects
from B.
After the first two connect()s, all three sockets' sk->sk_state are
TCP_ESTABLISHED:
$ ss -xa
Netid State Recv-Q Send-Q Local Address:Port Peer Address:PortProcess
u_dgr ESTAB 0 0 @A 641 * 642
u_dgr ESTAB 0 0 @B 642 * 643
u_dgr ESTAB 0 0 @C 643 * 0
And after the disconnect, B's state is TCP_CLOSE even though it's still
connected to C and C's state is TCP_ESTABLISHED.
$ ss -xa
Netid State Recv-Q Send-Q Local Address:Port Peer Address:PortProcess
u_dgr UNCONN 0 0 @A 641 * 642
u_dgr UNCONN 0 0 @B 642 * 643
u_dgr ESTAB 0 0 @C 643 * 0
In this case, we cannot register B to SOCKMAP.
So, when a socket disconnects from the peer, we should not set TCP_CLOSE to
the peer if the peer is connected to yet another socket, and this must be
done under unix_state_lock().
Note that we use WRITE_ONCE() for sk->sk_state as there are many lockless
readers. These data-races will be fixed in the following patches.
Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
CC: Cong Wang <redacted>
---
net/unix/af_unix.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
sk->sk_state is changed under unix_state_lock(), but it's read locklessly
in many places.
This patch adds WRITE_ONCE() on the writer side.
We will add READ_ONCE() to the lockless readers in the following patches.
Fixes: 83301b5367a9 ("af_unix: Set TCP_ESTABLISHED for datagram sockets too")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
@@ -738,7 +738,8 @@ static int unix_listen(struct socket *sock, int backlog)if(backlog>sk->sk_max_ack_backlog)wake_up_interruptible_all(&u->peer_wait);sk->sk_max_ack_backlog=backlog;-sk->sk_state=TCP_LISTEN;+WRITE_ONCE(sk->sk_state,TCP_LISTEN);+/* set credentials so connect can copy them */init_peercred(sk);err=0;
ioctl(SIOCINQ) calls unix_inq_len() that checks sk->sk_state first
and returns -EINVAL if it's TCP_LISTEN.
Then, for SOCK_STREAM sockets, unix_inq_len() returns the number of
bytes in recvq.
However, unix_inq_len() does not hold unix_state_lock(), and the
concurrent listen() might change the state after checking sk->sk_state.
If the race occurs, 0 is returned for the listener, instead of -EINVAL,
because the length of skb with embryo is 0.
We could hold unix_state_lock() in unix_inq_len(), but it's overkill
given the result is true for pre-listen() TCP_CLOSE state.
So, let's use READ_ONCE() for sk->sk_state in unix_inq_len().
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
unix_poll() and unix_dgram_poll() read sk->sk_state locklessly and
calls unix_writable() which also reads sk->sk_state without holding
unix_state_lock().
Let's use READ_ONCE() in unix_poll() and unix_dgram_poll() and pass
it to unix_writable().
While at it, we remove TCP_SYN_SENT check in unix_dgram_poll() as
that state does not exist for AF_UNIX socket since the code was added.
Fixes: 1586a5877db9 ("af_unix: do not report POLLOUT on listeners")
Fixes: 3c73419c09a5 ("af_unix: fix 'poll for write'/ connected DGRAM sockets")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
@@ -3199,19 +3203,14 @@ static __poll_t unix_dgram_poll(struct file *file, struct socket *sock,mask|=EPOLLIN|EPOLLRDNORM;/* Connection-based need to check for termination and startup */-if(sk->sk_type==SOCK_SEQPACKET){-if(sk->sk_state==TCP_CLOSE)-mask|=EPOLLHUP;-/* connection hasn't started yet? */-if(sk->sk_state==TCP_SYN_SENT)-returnmask;-}+if(sk->sk_type==SOCK_SEQPACKET&&state==TCP_CLOSE)+mask|=EPOLLHUP;/* No write status requested, avoid expensive OUT tests. */if(!(poll_requested_events(wait)&(EPOLLWRBAND|EPOLLWRNORM|EPOLLOUT)))returnmask;-writable=unix_writable(sk);+writable=unix_writable(sk,state);if(writable){unix_state_lock(sk);
As small optimisation, unix_stream_connect() prefetches the client's
sk->sk_state without unix_state_lock() and checks if it's TCP_CLOSE.
Later, sk->sk_state is checked again under unix_state_lock().
Let's use READ_ONCE() for the first check and TCP_CLOSE directly for
the second check.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
@@ -1571,9 +1570,7 @@ static int unix_stream_connect(struct socket *sock, struct sockaddr *uaddr,Well,andwehavetorecheckthestateaftersocketlocked.*/-st=sk->sk_state;--switch(st){+switch(READ_ONCE(sk->sk_state)){caseTCP_CLOSE:/* This is ok... continue with connect */break;
Once sk->sk_state is changed to TCP_LISTEN, it never changes.
unix_accept() takes the advantage and reads sk->sk_state without
holding unix_state_lock().
Let's use READ_ONCE() there.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -1710,7 +1710,7 @@ static int unix_accept(struct socket *sock, struct socket *newsock,gotoout;arg->err=-EINVAL;-if(sk->sk_state!=TCP_LISTEN)+if(READ_ONCE(sk->sk_state)!=TCP_LISTEN)gotoout;/* If socket state is TCP_LISTEN it cannot change (for now...),
The following functions read sk->sk_state locklessly and proceed only if
the state is TCP_ESTABLISHED.
* unix_stream_sendmsg
* unix_stream_read_generic
* unix_seqpacket_sendmsg
* unix_seqpacket_recvmsg
Let's use READ_ONCE() there.
Fixes: a05d2ad1c1f3 ("af_unix: Only allow recv on connected seqpacket sockets.")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
unix_stream_read_skb() is called from sk->sk_data_ready() context
where unix_state_lock() is not held.
Let's use READ_ONCE() there.
Fixes: 77462de14a43 ("af_unix: Add read_sock for stream socket types")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
While dumping AF_UNIX sockets via UNIX_DIAG, sk->sk_state is read
locklessly.
Let's use READ_ONCE() there.
Note that the result could be inconsistent if the socket is dumped
during the state change. This is common for other SOCK_DIAG and
similar interfaces.
Fixes: c9da99e6475f ("unix_diag: Fixup RQLEN extension report")
Fixes: 2aac7a2cb0d9 ("unix_diag: Pending connections IDs NLA")
Fixes: 45a96b9be6ec ("unix_diag: Dumping all sockets core")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/diag.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
sk_setsockopt() changes sk->sk_sndbuf under lock_sock(), but it's
not used in af_unix.c.
Let's use READ_ONCE() to read sk->sk_sndbuf in unix_writable(),
unix_dgram_sendmsg(), and unix_stream_sendmsg().
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -2247,7 +2247,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,&err,0);}else{/* Keep two messages in the pipe so it schedules better */-size=min_t(int,size,(sk->sk_sndbuf>>1)-64);+size=min_t(int,size,(READ_ONCE(sk->sk_sndbuf)>>1)-64);/* allow fallback to order-0 allocations */size=min_t(int,size,SKB_MAX_HEAD(0)+UNIX_SKB_FRAGS_SZ);
net->unx.sysctl_max_dgram_qlen is exposed as a sysctl knob and can be
changed concurrently.
Let's use READ_ONCE() in unix_create1().
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Once sk->sk_state is changed to TCP_LISTEN, it never changes.
unix_accept() takes advantage of this characteristics; it does not
hold the listener's unix_state_lock() and only acquires recvq lock
to pop one skb.
It means unix_state_lock() does not prevent the queue length from
changing in unix_stream_connect().
Thus, we need to use unix_recvq_full_lockless() to avoid data-race.
Now we remove unix_recvq_full() as no one uses it.
Note that we can remove READ_ONCE() for sk->sk_max_ack_backlog in
unix_recvq_full_lockless() because of the following reasons:
(1) For SOCK_DGRAM, it is a written-once field in unix_create1()
(2) For SOCK_STREAM and SOCK_SEQPACKET, it is changed under the
listener's unix_state_lock() in unix_listen(), and we hold
the lock in unix_stream_connect()
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
If the socket type is SOCK_STREAM or SOCK_SEQPACKET, unix_release_sock()
checks the length of the peer socket's recvq under unix_state_lock().
However, unix_stream_read_generic() calls skb_unlink() after releasing
the lock. Also, for SOCK_SEQPACKET, __skb_try_recv_datagram() unlinks
skb without unix_state_lock().
Thues, unix_state_lock() does not protect qlen.
Let's use skb_queue_empty_lockless() in unix_release_sock().
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/af_unix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -631,7 +631,7 @@ static void unix_release_sock(struct sock *sk, int embrion)unix_state_lock(skpair);/* No more writes */WRITE_ONCE(skpair->sk_shutdown,SHUTDOWN_MASK);-if(!skb_queue_empty(&sk->sk_receive_queue)||embrion)+if(!skb_queue_empty_lockless(&sk->sk_receive_queue)||embrion)WRITE_ONCE(skpair->sk_err,ECONNRESET);unix_state_unlock(skpair);skpair->sk_state_change(skpair);
We can dump the socket queue length via UNIX_DIAG by specifying
UDIAG_SHOW_RQLEN.
If sk->sk_state is TCP_LISTEN, we return the recv queue length,
but here we do not hold recvq lock.
Let's use skb_queue_len_lockless() in sk_diag_show_rqlen().
Fixes: c9da99e6475f ("unix_diag: Fixup RQLEN extension report")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/diag.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
While dumping sockets via UNIX_DIAG, we do not hold unix_state_lock().
Let's use READ_ONCE() to read sk->sk_shutdown.
Fixes: e4e541a84863 ("sock-diag: Report shutdown for inet and unix sockets (v2)")
Signed-off-by: Kuniyuki Iwashima <redacted>
---
net/unix/diag.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)