* [PATCH net-next] af_unix: Fix data races in unix_release_sock/unix_stream_sendmsg
@ 2024-05-08 11:17 Breno Leitao
2024-05-08 17:33 ` Kuniyuki Iwashima
0 siblings, 1 reply; 3+ messages in thread
From: Breno Leitao @ 2024-05-08 11:17 UTC (permalink / raw)
To: kuniyu, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: netdev, horms, paulmck, David Howells, Alexander Mikhalitsyn,
John Fastabend, Daan De Meyer, open list
A data-race condition has been identified in af_unix. In one data path,
the write function unix_release_sock() atomically writes to
sk->sk_shutdown using WRITE_ONCE. However, on the reader side,
unix_stream_sendmsg() does not read it atomically. Consequently, this
issue is causing the following KCSAN splat to occur:
BUG: KCSAN: data-race in unix_release_sock / unix_stream_sendmsg
write (marked) to 0xffff88867256ddbb of 1 bytes by task 7270 on cpu 28:
unix_release_sock (net/unix/af_unix.c:640)
unix_release (net/unix/af_unix.c:1050)
sock_close (net/socket.c:659 net/socket.c:1421)
__fput (fs/file_table.c:422)
__fput_sync (fs/file_table.c:508)
__se_sys_close (fs/open.c:1559 fs/open.c:1541)
__x64_sys_close (fs/open.c:1541)
x64_sys_call (arch/x86/entry/syscall_64.c:33)
do_syscall_64 (arch/x86/entry/common.c:?)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
read to 0xffff88867256ddbb of 1 bytes by task 989 on cpu 14:
unix_stream_sendmsg (net/unix/af_unix.c:2273)
__sock_sendmsg (net/socket.c:730 net/socket.c:745)
____sys_sendmsg (net/socket.c:2584)
__sys_sendmmsg (net/socket.c:2638 net/socket.c:2724)
__x64_sys_sendmmsg (net/socket.c:2753 net/socket.c:2750 net/socket.c:2750)
x64_sys_call (arch/x86/entry/syscall_64.c:33)
do_syscall_64 (arch/x86/entry/common.c:?)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
value changed: 0x01 -> 0x03
The line numbers are related to commit dd5a440a31fa ("Linux 6.9-rc7").
Commit e1d09c2c2f57 ("af_unix: Fix data races around sk->sk_shutdown.")
addressed a comparable issue in the past regarding sk->sk_shutdown.
However, it overlooked resolving this particular data path.
To prevent potential race conditions in the future, all read accesses to
sk->sk_shutdown in af_unix need be marked with READ_ONCE(). Although
there are additional reads in other->sk_shutdown without atomic reads,
I'm excluding them as I'm uncertain about their potential parallel
execution.
Signed-off-by: Breno Leitao <leitao@debian.org>
---
net/unix/af_unix.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 9a6ad5974dff..74795e6d13c6 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -2270,7 +2270,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
goto out_err;
}
- if (sk->sk_shutdown & SEND_SHUTDOWN)
+ if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN)
goto pipe_err;
while (sent < len) {
@@ -2446,7 +2446,7 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
unix_state_lock(sk);
/* Signal EOF on disconnected non-blocking SEQPACKET socket. */
if (sk->sk_type == SOCK_SEQPACKET && err == -EAGAIN &&
- (sk->sk_shutdown & RCV_SHUTDOWN))
+ (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN))
err = 0;
unix_state_unlock(sk);
goto out;
@@ -2566,7 +2566,7 @@ static long unix_stream_data_wait(struct sock *sk, long timeo,
if (tail != last ||
(tail && tail->len != last_len) ||
sk->sk_err ||
- (sk->sk_shutdown & RCV_SHUTDOWN) ||
+ (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN) ||
signal_pending(current) ||
!timeo)
break;
@@ -2764,7 +2764,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
err = sock_error(sk);
if (err)
goto unlock;
- if (sk->sk_shutdown & RCV_SHUTDOWN)
+ if (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN)
goto unlock;
unix_state_unlock(sk);
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net-next] af_unix: Fix data races in unix_release_sock/unix_stream_sendmsg
2024-05-08 11:17 [PATCH net-next] af_unix: Fix data races in unix_release_sock/unix_stream_sendmsg Breno Leitao
@ 2024-05-08 17:33 ` Kuniyuki Iwashima
2024-05-08 19:26 ` Breno Leitao
0 siblings, 1 reply; 3+ messages in thread
From: Kuniyuki Iwashima @ 2024-05-08 17:33 UTC (permalink / raw)
To: leitao
Cc: alexander, daan.j.demeyer, davem, dhowells, edumazet, horms,
john.fastabend, kuba, kuniyu, linux-kernel, netdev, pabeni,
paulmck
From: Breno Leitao <leitao@debian.org>
Date: Wed, 8 May 2024 04:17:45 -0700
> A data-race condition has been identified in af_unix. In one data path,
> the write function unix_release_sock() atomically writes to
> sk->sk_shutdown using WRITE_ONCE. However, on the reader side,
> unix_stream_sendmsg() does not read it atomically. Consequently, this
> issue is causing the following KCSAN splat to occur:
>
> BUG: KCSAN: data-race in unix_release_sock / unix_stream_sendmsg
>
> write (marked) to 0xffff88867256ddbb of 1 bytes by task 7270 on cpu 28:
> unix_release_sock (net/unix/af_unix.c:640)
> unix_release (net/unix/af_unix.c:1050)
> sock_close (net/socket.c:659 net/socket.c:1421)
> __fput (fs/file_table.c:422)
> __fput_sync (fs/file_table.c:508)
> __se_sys_close (fs/open.c:1559 fs/open.c:1541)
> __x64_sys_close (fs/open.c:1541)
> x64_sys_call (arch/x86/entry/syscall_64.c:33)
> do_syscall_64 (arch/x86/entry/common.c:?)
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
>
> read to 0xffff88867256ddbb of 1 bytes by task 989 on cpu 14:
> unix_stream_sendmsg (net/unix/af_unix.c:2273)
> __sock_sendmsg (net/socket.c:730 net/socket.c:745)
> ____sys_sendmsg (net/socket.c:2584)
> __sys_sendmmsg (net/socket.c:2638 net/socket.c:2724)
> __x64_sys_sendmmsg (net/socket.c:2753 net/socket.c:2750 net/socket.c:2750)
> x64_sys_call (arch/x86/entry/syscall_64.c:33)
> do_syscall_64 (arch/x86/entry/common.c:?)
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
>
> value changed: 0x01 -> 0x03
>
> The line numbers are related to commit dd5a440a31fa ("Linux 6.9-rc7").
>
> Commit e1d09c2c2f57 ("af_unix: Fix data races around sk->sk_shutdown.")
> addressed a comparable issue in the past regarding sk->sk_shutdown.
> However, it overlooked resolving this particular data path.
>
> To prevent potential race conditions in the future, all read accesses to
> sk->sk_shutdown in af_unix need be marked with READ_ONCE().
Let's not add READ_ONCE() if not needed. Othwewise, someone reading
the code would assess wrongly that the value could be updated locklessly
elsewhere.
You can find all writers of sk->sk_shutdown do that update under
unix_state_lock().
> Although
> there are additional reads in other->sk_shutdown without atomic reads,
> I'm excluding them as I'm uncertain about their potential parallel
> execution.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
> net/unix/af_unix.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 9a6ad5974dff..74795e6d13c6 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -2270,7 +2270,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> goto out_err;
> }
>
> - if (sk->sk_shutdown & SEND_SHUTDOWN)
> + if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN)
> goto pipe_err;
>
> while (sent < len) {
> @@ -2446,7 +2446,7 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
> unix_state_lock(sk);
> /* Signal EOF on disconnected non-blocking SEQPACKET socket. */
> if (sk->sk_type == SOCK_SEQPACKET && err == -EAGAIN &&
> - (sk->sk_shutdown & RCV_SHUTDOWN))
> + (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN))
Here we locked unix_state_lock() just before accessing sk_shutdown,
so no need for READ_ONCE().
> err = 0;
> unix_state_unlock(sk);
> goto out;
> @@ -2566,7 +2566,7 @@ static long unix_stream_data_wait(struct sock *sk, long timeo,
> if (tail != last ||
> (tail && tail->len != last_len) ||
> sk->sk_err ||
> - (sk->sk_shutdown & RCV_SHUTDOWN) ||
> + (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN) ||
> signal_pending(current) ||
> !timeo)
> break;
Same here,
> @@ -2764,7 +2764,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
> err = sock_error(sk);
> if (err)
> goto unlock;
> - if (sk->sk_shutdown & RCV_SHUTDOWN)
> + if (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN)
> goto unlock;
>
> unix_state_unlock(sk);
and here.
Could you update the changelog and repost v2 for unix_stream_sendmsg()
targetting net tree with this Fixes tag ?
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Thanks!
> --
> 2.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net-next] af_unix: Fix data races in unix_release_sock/unix_stream_sendmsg
2024-05-08 17:33 ` Kuniyuki Iwashima
@ 2024-05-08 19:26 ` Breno Leitao
0 siblings, 0 replies; 3+ messages in thread
From: Breno Leitao @ 2024-05-08 19:26 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: alexander, daan.j.demeyer, davem, dhowells, edumazet, horms,
john.fastabend, kuba, linux-kernel, netdev, pabeni, paulmck
On Wed, May 08, 2024 at 10:33:24AM -0700, Kuniyuki Iwashima wrote:
> From: Breno Leitao <leitao@debian.org>
> Date: Wed, 8 May 2024 04:17:45 -0700
> > A data-race condition has been identified in af_unix. In one data path,
> > the write function unix_release_sock() atomically writes to
> > sk->sk_shutdown using WRITE_ONCE. However, on the reader side,
> > unix_stream_sendmsg() does not read it atomically. Consequently, this
> > issue is causing the following KCSAN splat to occur:
> >
> > BUG: KCSAN: data-race in unix_release_sock / unix_stream_sendmsg
> >
> > write (marked) to 0xffff88867256ddbb of 1 bytes by task 7270 on cpu 28:
> > unix_release_sock (net/unix/af_unix.c:640)
> > unix_release (net/unix/af_unix.c:1050)
> > sock_close (net/socket.c:659 net/socket.c:1421)
> > __fput (fs/file_table.c:422)
> > __fput_sync (fs/file_table.c:508)
> > __se_sys_close (fs/open.c:1559 fs/open.c:1541)
> > __x64_sys_close (fs/open.c:1541)
> > x64_sys_call (arch/x86/entry/syscall_64.c:33)
> > do_syscall_64 (arch/x86/entry/common.c:?)
> > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
> >
> > read to 0xffff88867256ddbb of 1 bytes by task 989 on cpu 14:
> > unix_stream_sendmsg (net/unix/af_unix.c:2273)
> > __sock_sendmsg (net/socket.c:730 net/socket.c:745)
> > ____sys_sendmsg (net/socket.c:2584)
> > __sys_sendmmsg (net/socket.c:2638 net/socket.c:2724)
> > __x64_sys_sendmmsg (net/socket.c:2753 net/socket.c:2750 net/socket.c:2750)
> > x64_sys_call (arch/x86/entry/syscall_64.c:33)
> > do_syscall_64 (arch/x86/entry/common.c:?)
> > entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:130)
> >
> > value changed: 0x01 -> 0x03
> >
> > The line numbers are related to commit dd5a440a31fa ("Linux 6.9-rc7").
> >
> > Commit e1d09c2c2f57 ("af_unix: Fix data races around sk->sk_shutdown.")
> > addressed a comparable issue in the past regarding sk->sk_shutdown.
> > However, it overlooked resolving this particular data path.
> >
> > To prevent potential race conditions in the future, all read accesses to
> > sk->sk_shutdown in af_unix need be marked with READ_ONCE().
>
> Let's not add READ_ONCE() if not needed. Othwewise, someone reading
> the code would assess wrongly that the value could be updated locklessly
> elsewhere.
>
> You can find all writers of sk->sk_shutdown do that update under
> unix_state_lock().
>
>
> > Although
> > there are additional reads in other->sk_shutdown without atomic reads,
> > I'm excluding them as I'm uncertain about their potential parallel
> > execution.
> >
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> > net/unix/af_unix.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > index 9a6ad5974dff..74795e6d13c6 100644
> > --- a/net/unix/af_unix.c
> > +++ b/net/unix/af_unix.c
> > @@ -2270,7 +2270,7 @@ static int unix_stream_sendmsg(struct socket *sock, struct msghdr *msg,
> > goto out_err;
> > }
> >
> > - if (sk->sk_shutdown & SEND_SHUTDOWN)
> > + if (READ_ONCE(sk->sk_shutdown) & SEND_SHUTDOWN)
> > goto pipe_err;
> >
> > while (sent < len) {
> > @@ -2446,7 +2446,7 @@ int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size,
> > unix_state_lock(sk);
> > /* Signal EOF on disconnected non-blocking SEQPACKET socket. */
> > if (sk->sk_type == SOCK_SEQPACKET && err == -EAGAIN &&
> > - (sk->sk_shutdown & RCV_SHUTDOWN))
> > + (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN))
>
> Here we locked unix_state_lock() just before accessing sk_shutdown,
> so no need for READ_ONCE().
>
>
> > err = 0;
> > unix_state_unlock(sk);
> > goto out;
> > @@ -2566,7 +2566,7 @@ static long unix_stream_data_wait(struct sock *sk, long timeo,
> > if (tail != last ||
> > (tail && tail->len != last_len) ||
> > sk->sk_err ||
> > - (sk->sk_shutdown & RCV_SHUTDOWN) ||
> > + (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN) ||
> > signal_pending(current) ||
> > !timeo)
> > break;
>
> Same here,
>
>
> > @@ -2764,7 +2764,7 @@ static int unix_stream_read_generic(struct unix_stream_read_state *state,
> > err = sock_error(sk);
> > if (err)
> > goto unlock;
> > - if (sk->sk_shutdown & RCV_SHUTDOWN)
> > + if (READ_ONCE(sk->sk_shutdown) & RCV_SHUTDOWN)
> > goto unlock;
> >
> > unix_state_unlock(sk);
>
> and here.
>
> Could you update the changelog and repost v2 for unix_stream_sendmsg()
> targetting net tree with this Fixes tag ?
Sure. I will keep the READ_ONCE only in unix_stream_sendmsg() then.
Thanks for the review!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-05-08 19:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-08 11:17 [PATCH net-next] af_unix: Fix data races in unix_release_sock/unix_stream_sendmsg Breno Leitao
2024-05-08 17:33 ` Kuniyuki Iwashima
2024-05-08 19:26 ` Breno Leitao
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).