af_unix: Fix data races in unix_release_sock/unix_stream_sendmsg

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 <[email protected]>

---
 net/unix/af_unix.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

net/unix/af_unix.c +1 -1

index 9a6ad5974dff..74795e6d13c6 100644
@@ -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) {

Signature · 1 line

2.43.0

From: Breno Leitao <[email protected]>
Date: Wed, 8 May 2024 04:17:45 -0700

Quoted 32 lines

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

Quoted 8 lines

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

Quoted 24 lines

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 <[email protected]>

---
 net/unix/af_unix.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

net/unix/af_unix.c +1 -1

index 9a6ad5974dff..74795e6d13c6 100644
@@ -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) {

net/unix/af_unix.c int __unix_dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t size, · line 2449

Quoted 5 lines

net/unix/af_unix.c +1 -1

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

net/unix/af_unix.c static long unix_stream_data_wait(struct sock *sk, long timeo, · line 2572

Quoted 12 lines

net/unix/af_unix.c +1 -1

 			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,

Quoted 8 lines

net/unix/af_unix.c +1 -1

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

net/unix/af_unix.c static int unix_stream_read_generic(struct unix_stream_read_state *state, · line 2770

Quoted 1 line

net/unix/af_unix.c +0 -0

 			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 ?