LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures
@ 2021-10-04 23:25 Jiang Wang
  2021-10-05  0:04 ` Casey Schaufler
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Jiang Wang @ 2021-10-04 23:25 UTC (permalink / raw)
  To: bpf
  Cc: cong.wang, Casey Schaufler, David S. Miller, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Al Viro, Eric Dumazet, Christian Brauner, Rao Shoaib,
	Kuniyuki Iwashima, Jakub Sitnicki, netdev, linux-kernel

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 <cong.wang@bytedance.com>
Reported-by: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
---
 net/unix/af_unix.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index efac5989edb5..0878ab86597b 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -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);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures
  2021-10-04 23:25 [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures Jiang Wang
@ 2021-10-05  0:04 ` Casey Schaufler
  2021-10-05 22:17   ` Song Liu
  2021-10-06 12:50 ` patchwork-bot+netdevbpf
  2021-11-11 14:00 ` Vincent Whitchurch
  2 siblings, 1 reply; 7+ messages in thread
From: Casey Schaufler @ 2021-10-05  0:04 UTC (permalink / raw)
  To: Jiang Wang, bpf
  Cc: cong.wang, David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Al Viro, Eric Dumazet,
	Christian Brauner, Rao Shoaib, Kuniyuki Iwashima, Jakub Sitnicki,
	netdev, linux-kernel, Casey Schaufler

On 10/4/2021 4:25 PM, Jiang Wang 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.
>
> Fixes: 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
> Suggested-by: Cong Wang <cong.wang@bytedance.com>
> Reported-by: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>

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 <casey@schaufler-ca.com>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  net/unix/af_unix.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index efac5989edb5..0878ab86597b 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -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);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures
  2021-10-05  0:04 ` Casey Schaufler
@ 2021-10-05 22:17   ` Song Liu
  0 siblings, 0 replies; 7+ messages in thread
From: Song Liu @ 2021-10-05 22:17 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Jiang Wang, bpf, Cong Wang, David S. Miller, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
	Yonghong Song, John Fastabend, KP Singh, Al Viro, Eric Dumazet,
	Christian Brauner, Rao Shoaib, Kuniyuki Iwashima, Jakub Sitnicki,
	netdev, linux-kernel



> On Oct 4, 2021, at 5:04 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> On 10/4/2021 4:25 PM, Jiang Wang 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.
>> 
>> Fixes: 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
>> Suggested-by: Cong Wang <cong.wang@bytedance.com>
>> Reported-by: Casey Schaufler <casey@schaufler-ca.com>
>> Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
> 
> 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 <casey@schaufler-ca.com>
> Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>

Acked-by: Song Liu <songliubraving@fb.com>

> 
>> ---
>> net/unix/af_unix.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>> 
>> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
>> index efac5989edb5..0878ab86597b 100644
>> --- a/net/unix/af_unix.c
>> +++ b/net/unix/af_unix.c
>> @@ -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);


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures
  2021-10-04 23:25 [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures Jiang Wang
  2021-10-05  0:04 ` Casey Schaufler
@ 2021-10-06 12:50 ` patchwork-bot+netdevbpf
  2021-11-11 14:00 ` Vincent Whitchurch
  2 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-10-06 12:50 UTC (permalink / raw)
  To: Jiang Wang
  Cc: bpf, cong.wang, casey, davem, kuba, ast, daniel, andrii, kafai,
	songliubraving, yhs, john.fastabend, kpsingh, viro, edumazet,
	christian.brauner, Rao.Shoaib, kuniyu, jakub, netdev,
	linux-kernel

Hello:

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

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures
  2021-10-04 23:25 [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures Jiang Wang
  2021-10-05  0:04 ` Casey Schaufler
  2021-10-06 12:50 ` patchwork-bot+netdevbpf
@ 2021-11-11 14:00 ` Vincent Whitchurch
  2021-11-19 14:14   ` Jakub Kicinski
  2 siblings, 1 reply; 7+ messages in thread
From: Vincent Whitchurch @ 2021-11-11 14:00 UTC (permalink / raw)
  To: Jiang Wang
  Cc: bpf, cong.wang, Casey Schaufler, David S. Miller, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Al Viro, Eric Dumazet, Christian Brauner, Rao Shoaib,
	Kuniyuki Iwashima, Jakub Sitnicki, netdev, linux-kernel

On Mon, Oct 04, 2021 at 11:25:28PM +0000, Jiang Wang 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.
> 
> Fixes: 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
> Suggested-by: Cong Wang <cong.wang@bytedance.com>
> Reported-by: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>

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;
}

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures
  2021-11-11 14:00 ` Vincent Whitchurch
@ 2021-11-19 14:14   ` Jakub Kicinski
  2021-11-19 14:28     ` Jakub Kicinski
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2021-11-19 14:14 UTC (permalink / raw)
  To: Jiang Wang, cong.wang
  Cc: Vincent Whitchurch, bpf, Casey Schaufler, David S. Miller,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Al Viro, Eric Dumazet, Christian Brauner, Rao Shoaib,
	Kuniyuki Iwashima, Jakub Sitnicki, netdev, linux-kernel

On Thu, 11 Nov 2021 15:00:02 +0100 Vincent Whitchurch wrote:
> On Mon, Oct 04, 2021 at 11:25:28PM +0000, Jiang Wang 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.
> > 
> > Fixes: 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
> > Suggested-by: Cong Wang <cong.wang@bytedance.com>
> > Reported-by: Casey Schaufler <casey@schaufler-ca.com>
> > Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>  
> 
> 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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures
  2021-11-19 14:14   ` Jakub Kicinski
@ 2021-11-19 14:28     ` Jakub Kicinski
  0 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2021-11-19 14:28 UTC (permalink / raw)
  To: Jiang Wang, cong.wang
  Cc: Vincent Whitchurch, bpf, Casey Schaufler, David S. Miller,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Al Viro, Eric Dumazet, Christian Brauner, Rao Shoaib,
	Kuniyuki Iwashima, Jakub Sitnicki, netdev, linux-kernel

On Fri, 19 Nov 2021 06:14:19 -0800 Jakub Kicinski wrote:
> On Thu, 11 Nov 2021 15:00:02 +0100 Vincent Whitchurch wrote:
> > On Mon, Oct 04, 2021 at 11:25:28PM +0000, Jiang Wang 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.
> > > 
> > > Fixes: 94531cfcbe79 (af_unix: Add unix_stream_proto for sockmap)
> > > Suggested-by: Cong Wang <cong.wang@bytedance.com>
> > > Reported-by: Casey Schaufler <casey@schaufler-ca.com>
> > > Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>    
> > 
> > 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?

Ah, just saw the patch. What timing.

Thanks Vincent!

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-11-19 14:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-04 23:25 [PATCH bpf v1] unix: fix an issue in unix_shutdown causing the other end read/write failures Jiang Wang
2021-10-05  0:04 ` Casey Schaufler
2021-10-05 22:17   ` Song Liu
2021-10-06 12:50 ` patchwork-bot+netdevbpf
2021-11-11 14:00 ` Vincent Whitchurch
2021-11-19 14:14   ` Jakub Kicinski
2021-11-19 14:28     ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).