[PATCH v2 net-next] af_unix: ensure POLLOUT on remote close() for connected dgram socket

[PATCH v2 net-next] af_unix: ensure POLLOUT on remote close() for connected dgram socket

[Jason Baron]

Applications use -ECONNREFUSED as returned from write() in order to
determine that a socket should be closed. However, when using connected
dgram unix sockets in a poll/write loop, a final POLLOUT event can be
missed when the remote end closes. Thus, the poll is stuck forever:

          thread 1 (client)                   thread 2 (server)

connect() to server
write() returns -EAGAIN
unix_dgram_poll()
 -> unix_recvq_full() is true
                                       close()
                                        ->unix_release_sock()
                                         ->wake_up_interruptible_all()
unix_dgram_poll() (due to the
     wake_up_interruptible_all)
 -> unix_recvq_full() still is true
                                         ->free all skbs


Now thread 1 is stuck and will not receive anymore wakeups. In this
case, when thread 1 gets the -EAGAIN, it has not queued any skbs
otherwise the 'free all skbs' step would in fact cause a wakeup and
a POLLOUT return. So the race here is probably fairly rare because
it means there are no skbs that thread 1 queued and that thread 1
schedules before the 'free all skbs' step.

This issue was reported as a hang when /dev/log is closed.

The fix is to signal POLLOUT if the socket is marked as SOCK_DEAD, which
means a subsequent write() will get -ECONNREFUSED.

Reported-by: Ian Lance Taylor <iant@golang.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Rainer Weikusat <rweikusat@mobileactivedefense.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Jason Baron <jbaron@akamai.com>
---
v2: use check for SOCK_DEAD, since skb's can be purged in unix_sock_destructor()
---
 net/unix/af_unix.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 1772a0e..d1edfa3 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -430,7 +430,12 @@ static int unix_dgram_peer_wake_me(struct sock *sk, struct sock *other)
 
 	connected = unix_dgram_peer_wake_connect(sk, other);
 
-	if (unix_recvq_full(other))
+	/* If other is SOCK_DEAD, we want to make sure we signal
+	 * POLLOUT, such that a subsequent write() can get a
+	 * -ECONNREFUSED. Otherwise, if we haven't queued any skbs
+	 * to other and its full, we will hang waiting for POLLOUT.
+	 */
+	if (unix_recvq_full(other) && !sock_flag(other, SOCK_DEAD))
 		return 1;
 
 	if (connected)
-- 
1.9.1

Re: [PATCH v2 net-next] af_unix: ensure POLLOUT on remote close() for connected dgram socket

[David Miller]

From: Jason Baron <jbaron@akamai.com>
Date: Fri,  3 Aug 2018 17:24:53 -0400

> Applications use -ECONNREFUSED as returned from write() in order to
> determine that a socket should be closed. However, when using connected
> dgram unix sockets in a poll/write loop, a final POLLOUT event can be
> missed when the remote end closes. Thus, the poll is stuck forever:
> 
>           thread 1 (client)                   thread 2 (server)
> 
> connect() to server
> write() returns -EAGAIN
> unix_dgram_poll()
>  -> unix_recvq_full() is true
>                                        close()
>                                         ->unix_release_sock()
>                                          ->wake_up_interruptible_all()
> unix_dgram_poll() (due to the
>      wake_up_interruptible_all)
>  -> unix_recvq_full() still is true
>                                          ->free all skbs
> 
> 
> Now thread 1 is stuck and will not receive anymore wakeups. In this
> case, when thread 1 gets the -EAGAIN, it has not queued any skbs
> otherwise the 'free all skbs' step would in fact cause a wakeup and
> a POLLOUT return. So the race here is probably fairly rare because
> it means there are no skbs that thread 1 queued and that thread 1
> schedules before the 'free all skbs' step.
> 
> This issue was reported as a hang when /dev/log is closed.
> 
> The fix is to signal POLLOUT if the socket is marked as SOCK_DEAD, which
> means a subsequent write() will get -ECONNREFUSED.
> 
> Reported-by: Ian Lance Taylor <iant@golang.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Rainer Weikusat <rweikusat@mobileactivedefense.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> ---
> v2: use check for SOCK_DEAD, since skb's can be purged in unix_sock_destructor()

Applied, thanks Jason.