unix: fix an issue in unix_shutdown causing the other end read/write failures

PATCH Applied Jiang Wang 6 participants 7 messages Oct 4, 2021 – Nov 19, 2021

Subsystems (4)

Networking Network drivers BPF Networking unix sockets

lore

Share & tools

lore

Jiang Wang

author

2021-10-04 23:25 UTC #0 lore raw

PATCH 1/1 unix: fix an issue in unix_shutdown causing the other end read/write failures

Commit 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap") sets unix domain socket peer state to TCP_CLOSE
in unix_shutdown. This could happen when the local end is shutdown but the other end is not. Then the other end will get read or write failures which is not expected.

Fix the issue by setting the local state to shutdown.

Fixes 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)

Suggested-by Cong Wang <[email protected]>

Reported-by Casey Schaufler <[email protected]>

Signed-off-by Jiang Wang <[email protected]>

---
 net/unix/af_unix.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

net/unix/af_unix.c +5 -4

index efac5989edb5..0878ab86597b 100644
@@ -2882,6 +2882,9 @@ static int unix_shutdown(struct socket *sock, int mode)
 
 	unix_state_lock(sk);
 	sk->sk_shutdown |= mode;
+	if ((sk->sk_type == SOCK_STREAM || sk->sk_type == SOCK_SEQPACKET) &&
+	    mode == SHUTDOWN_MASK)
+		sk->sk_state = TCP_CLOSE;
 	other = unix_peer(sk);
 	if (other)
 		sock_hold(other);

@@ -2904,12 +2907,10 @@ static int unix_shutdown(struct socket *sock, int mode)
 		other->sk_shutdown |= peer_mode;
 		unix_state_unlock(other);
 		other->sk_state_change(other);
-		if (peer_mode == SHUTDOWN_MASK) {
+		if (peer_mode == SHUTDOWN_MASK)
 			sk_wake_async(other, SOCK_WAKE_WAITD, POLL_HUP);
-			other->sk_state = TCP_CLOSE;
-		} else if (peer_mode & RCV_SHUTDOWN) {
+		else if (peer_mode & RCV_SHUTDOWN)
 			sk_wake_async(other, SOCK_WAKE_WAITD, POLL_IN);
-		}
 	}
 	if (other)
 		sock_put(other);

Signature · 1 line

2.20.1

Casey Schaufler

Tested-by Reviewed-by on v1 1/1

2021-10-05 00:04 UTC #1 lore raw

Quoted 6 lines · Jiang Wang

Commit 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap") sets unix domain socket peer state to TCP_CLOSE
in unix_shutdown. This could happen when the local end is shutdown but the other end is not. Then the other end will get read or write failures which is not expected.

Quoted 6 lines

Fix the issue by setting the local state to shutdown.
Fixes 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
Suggested-by Cong Wang <[email protected]>
Reported-by Casey Schaufler <[email protected]>
Signed-off-by Jiang Wang <[email protected]>

This patch looks like it has fixed the problem. My test cases are now getting expected results consistently. Please add any or all of:

Tested-by Casey Schaufler <[email protected]>

Reviewed-by Casey Schaufler <[email protected]>

Quoted 33 lines

---
 net/unix/af_unix.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

net/unix/af_unix.c +5 -4

index efac5989edb5..0878ab86597b 100644
@@ -2882,6 +2882,9 @@ static int unix_shutdown(struct socket *sock, int mode)
 
 	unix_state_lock(sk);
 	sk->sk_shutdown |= mode;
+	if ((sk->sk_type == SOCK_STREAM || sk->sk_type == SOCK_SEQPACKET) &&
+	    mode == SHUTDOWN_MASK)
+		sk->sk_state = TCP_CLOSE;
 	other = unix_peer(sk);
 	if (other)
 		sock_hold(other);

@@ -2904,12 +2907,10 @@ static int unix_shutdown(struct socket *sock, int mode)
 		other->sk_shutdown |= peer_mode;
 		unix_state_unlock(other);
 		other->sk_state_change(other);
-		if (peer_mode == SHUTDOWN_MASK) {
+		if (peer_mode == SHUTDOWN_MASK)
 			sk_wake_async(other, SOCK_WAKE_WAITD, POLL_HUP);
-			other->sk_state = TCP_CLOSE;
-		} else if (peer_mode & RCV_SHUTDOWN) {
+		else if (peer_mode & RCV_SHUTDOWN)
 			sk_wake_async(other, SOCK_WAKE_WAITD, POLL_IN);
-		}
 	}
 	if (other)
 		sock_put(other);

Song Liu

Acked-by

2021-10-05 22:18 UTC #2 lore raw

Quoted 18 lines · Casey Schaufler

Quoted 6 lines · Jiang Wang
Commit 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap") sets unix domain socket peer state to TCP_CLOSE
in unix_shutdown. This could happen when the local end is shutdown but the other end is not. Then the other end will get read or write failures which is not expected.
Quoted 6 lines
Fix the issue by setting the local state to shutdown.
Fixes 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
Suggested-by Cong Wang <[email protected]>
Reported-by Casey Schaufler <[email protected]>
Signed-off-by Jiang Wang <[email protected]>

Quoted 6 lines

This patch looks like it has fixed the problem. My test cases are now getting expected results consistently. Please add any or all of:
Tested-by Casey Schaufler <[email protected]>
Reviewed-by Casey Schaufler <[email protected]>

Acked-by Song Liu <[email protected]>

Quoted 34 lines

---
net/unix/af_unix.c | 9 +++++----

1 file changed, 5 insertions(+), 4 deletions(-)

net/unix/af_unix.c +5 -4

index efac5989edb5..0878ab86597b 100644
@@ -2882,6 +2882,9 @@ static int unix_shutdown(struct socket *sock, int mode)
 
 	unix_state_lock(sk);
 	sk->sk_shutdown |= mode;
+	if ((sk->sk_type == SOCK_STREAM || sk->sk_type == SOCK_SEQPACKET) &&
+	    mode == SHUTDOWN_MASK)
+		sk->sk_state = TCP_CLOSE;
 	other = unix_peer(sk);
 	if (other)
 		sock_hold(other);

@@ -2904,12 +2907,10 @@ static int unix_shutdown(struct socket *sock, int mode)
 		other->sk_shutdown |= peer_mode;
 		unix_state_unlock(other);
 		other->sk_state_change(other);
-		if (peer_mode == SHUTDOWN_MASK) {
+		if (peer_mode == SHUTDOWN_MASK)
 			sk_wake_async(other, SOCK_WAKE_WAITD, POLL_HUP);
-			other->sk_state = TCP_CLOSE;
-		} else if (peer_mode & RCV_SHUTDOWN) {
+		else if (peer_mode & RCV_SHUTDOWN)
 			sk_wake_async(other, SOCK_WAKE_WAITD, POLL_IN);
-		}
 	}
 	if (other)
 		sock_put(other);

[email protected]

CI Applied

2021-10-06 12:50 UTC #3 lore raw

In reply to Jiang Wang

This patch was applied to bpf/bpf.git (refs/heads/master):

Hello:

This patch was applied to bpf/bpf.git (refs/heads/master):

Quoted 9 lines · On Mon, 4 Oct 2021 23:25:28 +0000 you wrote:

Commit 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap") sets unix domain socket peer state to TCP_CLOSE
in unix_shutdown. This could happen when the local end is shutdown but the other end is not. Then the other end will get read or write failures which is not expected.
Fix the issue by setting the local state to shutdown.
[...]

Here is the summary with links:
- [bpf,v1] unix: fix an issue in unix_shutdown causing the other end read/write failures

    https://git.kernel.org/bpf/bpf/c/d0c6416bd709

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Vincent Whitchurch

2021-11-11 14:00 UTC #4 lore raw

In reply to Jiang Wang

Quoted 6 lines · Jiang Wang

Commit 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap") sets unix domain socket peer state to TCP_CLOSE
in unix_shutdown. This could happen when the local end is shutdown but the other end is not. Then the other end will get read or write failures which is not expected.

Quoted 6 lines

Fix the issue by setting the local state to shutdown.
Fixes 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
Suggested-by Cong Wang <[email protected]>
Reported-by Casey Schaufler <[email protected]>
Signed-off-by Jiang Wang <[email protected]>

This patch changed the behaviour of read(2) after a shutdown(2) on the local end of a UDS. Before this patch, reading from a UDS after a local shutdown(SHUT_RDWR) would return the data written or EOF if there is no data, but now it always returns -EINVAL.

For example, the following test program succeeds with "read 16 bytes" on v5.14 but fails with "read: Invalid argument" on v5.15 and mainline:

#include <err.h>
#include <errno.h>
#include <stdio.h>
#include <sys/socket.h>
#include <sys/unistd.h>

int main(int argc, char *argv[]) {
int sock[2];
int ret;

  ret = socketpair(AF_UNIX, SOCK_STREAM, 0, sock);
  if (ret < 0)
    err(1, "socketpair");

  char buf[16] = {};
  ret = write(sock[1], buf, sizeof(buf));
  if (ret < 0)
    err(1, "write");

  ret = shutdown(sock[0], SHUT_RDWR);
  if (ret < 0)
    err(1, "shutdown");

  ssize_t bytes = read(sock[0], buf, sizeof(buf));
  if (bytes < 0)
    err(1, "read");

  printf("read %zd bytes\n", bytes);

return 0;
}

Jakub Kicinski

maintainer

2021-11-19 14:14 UTC #5 lore raw

Quoted 14 lines · On Thu, 11 Nov 2021 15:00:02 +0100 Vincent Whitchurch wrote:

Quoted 6 lines · Jiang Wang
Commit 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap") sets unix domain socket peer state to TCP_CLOSE
in unix_shutdown. This could happen when the local end is shutdown but the other end is not. Then the other end will get read or write failures which is not expected.
Quoted 6 lines
Fix the issue by setting the local state to shutdown.
Fixes 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
Suggested-by Cong Wang <[email protected]>
Reported-by Casey Schaufler <[email protected]>
Signed-off-by Jiang Wang <[email protected]>

Quoted 7 lines

This patch changed the behaviour of read(2) after a shutdown(2) on the local end of a UDS. Before this patch, reading from a UDS after a local shutdown(SHUT_RDWR) would return the data written or EOF if there is no data, but now it always returns -EINVAL.
For example, the following test program succeeds with "read 16 bytes" on v5.14 but fails with "read: Invalid argument" on v5.15 and mainline:

Cong, Jiang, was this regression addressed?

Jakub Kicinski

maintainer

2021-11-19 14:28 UTC #6 lore raw

Quoted 20 lines · On Fri, 19 Nov 2021 06:14:19 -0800 Jakub Kicinski wrote:

Quoted 19 lines · On Thu, 11 Nov 2021 15:00:02 +0100 Vincent Whitchurch wrote:
Quoted 6 lines · Jiang Wang
Commit 94531cfcbe79 ("af_unix: Add unix_stream_proto for sockmap") sets unix domain socket peer state to TCP_CLOSE
in unix_shutdown. This could happen when the local end is shutdown but the other end is not. Then the other end will get read or write failures which is not expected.
Quoted 6 lines
Fix the issue by setting the local state to shutdown.
Fixes 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
Suggested-by Cong Wang <[email protected]>
Reported-by Casey Schaufler <[email protected]>
Signed-off-by Jiang Wang <[email protected]>
This patch changed the behaviour of read(2) after a shutdown(2) on the local end of a UDS. Before this patch, reading from a UDS after a local shutdown(SHUT_RDWR) would return the data written or EOF if there is no data, but now it always returns -EINVAL.

Quoted 4 lines

Quoted 2 lines
For example, the following test program succeeds with "read 16 bytes" on v5.14 but fails with "read: Invalid argument" on v5.15 and mainline:
Cong, Jiang, was this regression addressed?

Ah, just saw the patch. What timing.

Thanks Vincent!