[PATCH] af_unix: fix regression in read after shutdown

[PATCH] af_unix: fix regression in read after shutdown

[Vincent Whitchurch]

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

Re: [PATCH] af_unix: fix regression in read after shutdown

[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);

Re: [PATCH] af_unix: fix regression in read after shutdown

[patchwork-bot+netdevbpf]

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

Re: Re: [PATCH] af_unix: fix regression in read after shutdown

[Jiang Wang .]

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

Re: [PATCH] af_unix: fix regression in read after shutdown

[Cong Wang]

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.

Re: [PATCH] af_unix: fix regression in read after shutdown

[Vincent Whitchurch]

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.