| Message ID | 20211004232530.2377085-1-jiang.wang@bytedance.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d0c6416bd7091647f6041599f396bfa19ae30368 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,v1] unix: fix an issue in unix_shutdown causing the other end read/write failures | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 19 of 19 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 4 this patch: 4 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 23 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 4 this patch: 4 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| bpf/vmtest-bpf | success | VM_Test |
| bpf/vmtest-bpf-PR | success | PR summary |
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);
> 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);
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
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; }
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?
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!
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);
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(-)