linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] af_unix: fix regression in read after shutdown
@ 2021-11-19 12:05 Vincent Whitchurch
  2021-11-19 21:15 ` Casey Schaufler
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Vincent Whitchurch @ 2021-11-19 12:05 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko
  Cc: kernel, Vincent Whitchurch, Martin KaFai Lau, Song Liu,
	Yonghong Song, John Fastabend, KP Singh, Jiang Wang,
	Casey Schaufler, netdev, linux-kernel, bpf

On kernels before v5.15, calling read() on a unix socket after
shutdown(SHUT_RD) or shutdown(SHUT_RDWR) would return the data
previously written or EOF.  But now, while read() after
shutdown(SHUT_RD) still behaves the same way, read() after
shutdown(SHUT_RDWR) always fails with -EINVAL.

This behaviour change was apparently inadvertently introduced as part of
a bug fix for a different regression caused by the commit adding sockmap
support to af_unix, commit 94531cfcbe79c359 ("af_unix: Add
unix_stream_proto for sockmap").  Those commits, for unclear reasons,
started setting the socket state to TCP_CLOSE on shutdown(SHUT_RDWR),
while this state change had previously only been done in
unix_release_sock().

Restore the original behaviour.  The sockmap tests in
tests/selftests/bpf continue to pass after this patch.

Fixes: d0c6416bd7091647f60 ("unix: Fix an issue in unix_shutdown causing the other end read/write failures")
Link: https://lore.kernel.org/lkml/20211111140000.GA10779@axis.com/
Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 net/unix/af_unix.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 78e08e82c08c..b0bfc78e421c 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2882,9 +2882,6 @@ 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);
-- 
2.33.1


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

* Re: [PATCH] af_unix: fix regression in read after shutdown
  2021-11-19 12:05 [PATCH] af_unix: fix regression in read after shutdown Vincent Whitchurch
@ 2021-11-19 21:15 ` Casey Schaufler
  2021-11-20 15:20 ` patchwork-bot+netdevbpf
  2021-11-23 23:45 ` Cong Wang
  2 siblings, 0 replies; 6+ messages in thread
From: Casey Schaufler @ 2021-11-19 21:15 UTC (permalink / raw)
  To: Vincent Whitchurch, David S. Miller, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
  Cc: kernel, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Jiang Wang, netdev, linux-kernel, bpf,
	Casey Schaufler

On 11/19/2021 4:05 AM, Vincent Whitchurch wrote:
> On kernels before v5.15, calling read() on a unix socket after
> shutdown(SHUT_RD) or shutdown(SHUT_RDWR) would return the data
> previously written or EOF.  But now, while read() after
> shutdown(SHUT_RD) still behaves the same way, read() after
> shutdown(SHUT_RDWR) always fails with -EINVAL.
>
> This behaviour change was apparently inadvertently introduced as part of
> a bug fix for a different regression caused by the commit adding sockmap
> support to af_unix, commit 94531cfcbe79c359 ("af_unix: Add
> unix_stream_proto for sockmap").  Those commits, for unclear reasons,
> started setting the socket state to TCP_CLOSE on shutdown(SHUT_RDWR),
> while this state change had previously only been done in
> unix_release_sock().
>
> Restore the original behaviour.  The sockmap tests in
> tests/selftests/bpf continue to pass after this patch.
>
> Fixes: d0c6416bd7091647f60 ("unix: Fix an issue in unix_shutdown causing the other end read/write failures")
> Link: https://lore.kernel.org/lkml/20211111140000.GA10779@axis.com/
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>

This change passes the test case that lead to the original
problem report.

Tested-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>   net/unix/af_unix.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 78e08e82c08c..b0bfc78e421c 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2882,9 +2882,6 @@ 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);

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

* Re: [PATCH] af_unix: fix regression in read after shutdown
  2021-11-19 12:05 [PATCH] af_unix: fix regression in read after shutdown Vincent Whitchurch
  2021-11-19 21:15 ` Casey Schaufler
@ 2021-11-20 15:20 ` patchwork-bot+netdevbpf
  2021-11-21  1:45   ` Jiang Wang .
  2021-11-23 23:45 ` Cong Wang
  2 siblings, 1 reply; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-11-20 15:20 UTC (permalink / raw)
  To: Vincent Whitchurch
  Cc: davem, kuba, ast, daniel, andrii, kernel, kafai, songliubraving,
	yhs, john.fastabend, kpsingh, jiang.wang, casey, netdev,
	linux-kernel, bpf

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 19 Nov 2021 13:05:21 +0100 you wrote:
> On kernels before v5.15, calling read() on a unix socket after
> shutdown(SHUT_RD) or shutdown(SHUT_RDWR) would return the data
> previously written or EOF.  But now, while read() after
> shutdown(SHUT_RD) still behaves the same way, read() after
> shutdown(SHUT_RDWR) always fails with -EINVAL.
> 
> This behaviour change was apparently inadvertently introduced as part of
> a bug fix for a different regression caused by the commit adding sockmap
> support to af_unix, commit 94531cfcbe79c359 ("af_unix: Add
> unix_stream_proto for sockmap").  Those commits, for unclear reasons,
> started setting the socket state to TCP_CLOSE on shutdown(SHUT_RDWR),
> while this state change had previously only been done in
> unix_release_sock().
> 
> [...]

Here is the summary with links:
  - af_unix: fix regression in read after shutdown
    https://git.kernel.org/netdev/net/c/f9390b249c90

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] 6+ messages in thread

* Re: Re: [PATCH] af_unix: fix regression in read after shutdown
  2021-11-20 15:20 ` patchwork-bot+netdevbpf
@ 2021-11-21  1:45   ` Jiang Wang .
  0 siblings, 0 replies; 6+ messages in thread
From: Jiang Wang . @ 2021-11-21  1:45 UTC (permalink / raw)
  To: patchwork-bot+netdevbpf
  Cc: Vincent Whitchurch, David S. Miller, Jakub Kicinski,
	Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, kernel,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Casey Schaufler, Networking, open list, bpf

Thanks for the fix.

Regards,

Jiang

On Sat, Nov 20, 2021 at 7:20 AM <patchwork-bot+netdevbpf@kernel.org> wrote:
>
> Hello:
>
> This patch was applied to netdev/net.git (master)
> by David S. Miller <davem@davemloft.net>:
>
> On Fri, 19 Nov 2021 13:05:21 +0100 you wrote:
> > On kernels before v5.15, calling read() on a unix socket after
> > shutdown(SHUT_RD) or shutdown(SHUT_RDWR) would return the data
> > previously written or EOF.  But now, while read() after
> > shutdown(SHUT_RD) still behaves the same way, read() after
> > shutdown(SHUT_RDWR) always fails with -EINVAL.
> >
> > This behaviour change was apparently inadvertently introduced as part of
> > a bug fix for a different regression caused by the commit adding sockmap
> > support to af_unix, commit 94531cfcbe79c359 ("af_unix: Add
> > unix_stream_proto for sockmap").  Those commits, for unclear reasons,
> > started setting the socket state to TCP_CLOSE on shutdown(SHUT_RDWR),
> > while this state change had previously only been done in
> > unix_release_sock().
> >
> > [...]
>
> Here is the summary with links:
>   - af_unix: fix regression in read after shutdown
>     https://git.kernel.org/netdev/net/c/f9390b249c90
>
> 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] 6+ messages in thread

* Re: [PATCH] af_unix: fix regression in read after shutdown
  2021-11-19 12:05 [PATCH] af_unix: fix regression in read after shutdown Vincent Whitchurch
  2021-11-19 21:15 ` Casey Schaufler
  2021-11-20 15:20 ` patchwork-bot+netdevbpf
@ 2021-11-23 23:45 ` Cong Wang
  2021-11-25 10:10   ` Vincent Whitchurch
  2 siblings, 1 reply; 6+ messages in thread
From: Cong Wang @ 2021-11-23 23:45 UTC (permalink / raw)
  To: Vincent Whitchurch
  Cc: David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, kernel, Martin KaFai Lau,
	Song Liu, Yonghong Song, John Fastabend, KP Singh, Jiang Wang,
	Casey Schaufler, netdev, linux-kernel, bpf

On Fri, Nov 19, 2021 at 01:05:21PM +0100, Vincent Whitchurch wrote:
> On kernels before v5.15, calling read() on a unix socket after
> shutdown(SHUT_RD) or shutdown(SHUT_RDWR) would return the data
> previously written or EOF.  But now, while read() after
> shutdown(SHUT_RD) still behaves the same way, read() after
> shutdown(SHUT_RDWR) always fails with -EINVAL.

Maybe just lift the socket tate check in unix_stream_read_generic()?

> 
> This behaviour change was apparently inadvertently introduced as part of
> a bug fix for a different regression caused by the commit adding sockmap
> support to af_unix, commit 94531cfcbe79c359 ("af_unix: Add
> unix_stream_proto for sockmap").  Those commits, for unclear reasons,
> started setting the socket state to TCP_CLOSE on shutdown(SHUT_RDWR),

Not sure why it is unclear here, for an connection oriented socket, it
can be closed for just one direction, in this case we want to prevent it
from being redirected in sockmap, hence the point of the commits.

> while this state change had previously only been done in
> unix_release_sock().
> 
> Restore the original behaviour.  The sockmap tests in
> tests/selftests/bpf continue to pass after this patch.

Isn't this because we don't have shutdown() in sockmap tests?

Thanks.

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

* Re: [PATCH] af_unix: fix regression in read after shutdown
  2021-11-23 23:45 ` Cong Wang
@ 2021-11-25 10:10   ` Vincent Whitchurch
  0 siblings, 0 replies; 6+ messages in thread
From: Vincent Whitchurch @ 2021-11-25 10:10 UTC (permalink / raw)
  To: Cong Wang
  Cc: David S. Miller, Jakub Kicinski, Alexei Starovoitov,
	Daniel Borkmann, Andrii Nakryiko, kernel, Martin KaFai Lau,
	Song Liu, Yonghong Song, John Fastabend, KP Singh, Jiang Wang,
	Casey Schaufler, netdev, linux-kernel, bpf

On Wed, Nov 24, 2021 at 12:45:43AM +0100, Cong Wang wrote:
> On Fri, Nov 19, 2021 at 01:05:21PM +0100, Vincent Whitchurch wrote:
> > On kernels before v5.15, calling read() on a unix socket after
> > shutdown(SHUT_RD) or shutdown(SHUT_RDWR) would return the data
> > previously written or EOF.  But now, while read() after
> > shutdown(SHUT_RD) still behaves the same way, read() after
> > shutdown(SHUT_RDWR) always fails with -EINVAL.
> 
> Maybe just lift the socket tate check in unix_stream_read_generic()?

That would have have handled the specific case of read(2) on
SOCK_STREAM, but the sk->sk_state is checked in many other places in
af_unix.c so there would still be userspace-visible behaviour changes in
several other situations, which could cause regressions.  Just to give
one such example, the sendfile(2) call in the following program gets
killed by SIGPIPE on earlier kernels but would now instead start to
return -ENOTCONN:

#include <err.h>
#include <errno.h>
#include <stdio.h>
#include <sys/socket.h>
#include <sys/unistd.h>
#include <sys/sendfile.h>
#include <sys/types.h>
#include <fcntl.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");

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

  ssize_t bytes = sendfile(sock[0], open(argv[0], O_RDONLY), NULL, 16);
  if (bytes < 0)
    err(1, "sendfile");

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

  return 0;
}

> > 
> > This behaviour change was apparently inadvertently introduced as part of
> > a bug fix for a different regression caused by the commit adding sockmap
> > support to af_unix, commit 94531cfcbe79c359 ("af_unix: Add
> > unix_stream_proto for sockmap").  Those commits, for unclear reasons,
> > started setting the socket state to TCP_CLOSE on shutdown(SHUT_RDWR),
> 
> Not sure why it is unclear here, for an connection oriented socket, it
> can be closed for just one direction, in this case we want to prevent it
> from being redirected in sockmap, hence the point of the commits.

I must admit I'm not really familiar with either af_unix.c or sockmap,
but clearly the existing code in af_unix.c does not expect sk_state to
be changed in shutdown.  If we want to prevent UNIX sockets which have
had shutdown(SHUT_RDWR) called on then from being redirect to sockmap,
then maybe some other flag should be used to achieve that?

(Also, I wonder why the code added by the patch handled SHUT_RDWR
 differently from a SHUT_RD followed by a SHUT_WR?)

> > while this state change had previously only been done in
> > unix_release_sock().
> > 
> > Restore the original behaviour.  The sockmap tests in
> > tests/selftests/bpf continue to pass after this patch.
> 
> Isn't this because we don't have shutdown() in sockmap tests?

That may well be the case, I just assumed that the tests added along
with the new feature were comprehensive.

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

end of thread, other threads:[~2021-11-25 10:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 12:05 [PATCH] af_unix: fix regression in read after shutdown Vincent Whitchurch
2021-11-19 21:15 ` Casey Schaufler
2021-11-20 15:20 ` patchwork-bot+netdevbpf
2021-11-21  1:45   ` Jiang Wang .
2021-11-23 23:45 ` Cong Wang
2021-11-25 10:10   ` Vincent Whitchurch

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).