diff mbox series

[net-next] af_unix: Fix data races in unix_release_sock/unix_stream_sendmsg

Message ID 20240508111749.2386649-1-leitao@debian.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] af_unix: Fix data races in unix_release_sock/unix_stream_sendmsg | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 932 this patch: 932
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 938 this patch: 938
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 943 this patch: 943
netdev/checkpatch warning WARNING: Unknown commit id 'dd5a440a31fa', maybe rebased or not pulled?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-05-08--18-00 (tests: 1003)

Commit Message

Breno Leitao May 8, 2024, 11:17 a.m. UTC 
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(-)

Comments

Kuniyuki Iwashima May 8, 2024, 5:33 p.m. UTC | #1 
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
Breno Leitao May 8, 2024, 7:26 p.m. UTC | #2 
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!
diff mbox series

Patch

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