[6.3,022/246] rxrpc: Fix potential data race in rxrpc_wait_to_be_connected()

Commit Message

Greg KH May 15, 2023, 4:23 p.m. UTC

From: David Howells <[e-mail]>

[ Upstream commit 2b5fdc0f5caa505afe34d608e2eefadadf2ee67a ]

Inside the loop in rxrpc_wait_to_be_connected() it checks call->error to
see if it should exit the loop without first checking the call state.  This
is probably safe as if call->error is set, the call is dead anyway, but we
should probably wait for the call state to have been set to completion
first, lest it cause surprise on the way out.

Fix this by only accessing call->error if the call is complete.  We don't
actually need to access the error inside the loop as we'll do that after.

This caused the following report:

    BUG: KCSAN: data-race in rxrpc_send_data / rxrpc_set_call_completion

    write to 0xffff888159cf3c50 of 4 bytes by task 25673 on cpu 1:
     rxrpc_set_call_completion+0x71/0x1c0 net/rxrpc/call_state.c:22
     rxrpc_send_data_packet+0xba9/0x1650 net/rxrpc/output.c:479
     rxrpc_transmit_one+0x1e/0x130 net/rxrpc/output.c:714
     rxrpc_decant_prepared_tx net/rxrpc/call_event.c:326 [inline]
     rxrpc_transmit_some_data+0x496/0x600 net/rxrpc/call_event.c:350
     rxrpc_input_call_event+0x564/0x1220 net/rxrpc/call_event.c:464
     rxrpc_io_thread+0x307/0x1d80 net/rxrpc/io_thread.c:461
     kthread+0x1ac/0x1e0 kernel/kthread.c:376
     ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308

    read to 0xffff888159cf3c50 of 4 bytes by task 25672 on cpu 0:
     rxrpc_send_data+0x29e/0x1950 net/rxrpc/sendmsg.c:296
     rxrpc_do_sendmsg+0xb7a/0xc20 net/rxrpc/sendmsg.c:726
     rxrpc_sendmsg+0x413/0x520 net/rxrpc/af_rxrpc.c:565
     sock_sendmsg_nosec net/socket.c:724 [inline]
     sock_sendmsg net/socket.c:747 [inline]
     ____sys_sendmsg+0x375/0x4c0 net/socket.c:2501
     ___sys_sendmsg net/socket.c:2555 [inline]
     __sys_sendmmsg+0x263/0x500 net/socket.c:2641
     __do_sys_sendmmsg net/socket.c:2670 [inline]
     __se_sys_sendmmsg net/socket.c:2667 [inline]
     __x64_sys_sendmmsg+0x57/0x60 net/socket.c:2667
     do_syscall_x64 arch/x86/entry/common.c:50 [inline]
     do_syscall_64+0x41/0xc0 arch/x86/entry/common.c:80
     entry_SYSCALL_64_after_hwframe+0x63/0xcd

    value changed: 0x00000000 -> 0xffffffea

Fixes: 9d35d880e0e4 ("rxrpc: Move client call connection to the I/O thread")
Reported-by: [e-mail]
Link: https://lore.kernel.org/r/[e-mail]/
Signed-off-by: David Howells <[e-mail]>
cc: Marc Dionne <[e-mail]>
cc: Dmitry Vyukov <[e-mail]>
cc: "David S. Miller" <[e-mail]>
cc: Eric Dumazet <[e-mail]>
cc: Jakub Kicinski <[e-mail]>
cc: Paolo Abeni <[e-mail]>
cc: [e-mail]
cc: [e-mail]
cc: [e-mail]
Link: https://lore.kernel.org/r/[e-mail]
Signed-off-by: Paolo Abeni <[e-mail]>
Signed-off-by: Sasha Levin <[e-mail]>
---
 net/rxrpc/sendmsg.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Comments

[e-mail] May 17, 2023, 3:38 a.m. UTC | #1

Hello:

This series was applied to bpf/bpf-next.git (master)
by Arnaldo Carvalho de Melo <[e-mail]>:

On Mon, 15 May 2023 18:23:54 +0200 you wrote:
> From: David Howells <[e-mail]>
> 
> [ Upstream commit 2b5fdc0f5caa505afe34d608e2eefadadf2ee67a ]
> 
> Inside the loop in rxrpc_wait_to_be_connected() it checks call->error to
> see if it should exit the loop without first checking the call state.  This
> is probably safe as if call->error is set, the call is dead anyway, but we
> should probably wait for the call state to have been set to completion
> first, lest it cause surprise on the way out.
> 
> [...]

Here is the summary with links:
  - [6.3,022/246] rxrpc: Fix potential data race in rxrpc_wait_to_be_connected()
    (no matching commit)
  - [6.3,051/246] rxrpc: Fix hard call timeout units
    (no matching commit)
  - [6.3,052/246] rxrpc: Make it so that a waiting process can be aborted
    (no matching commit)
  - [6.3,053/246] rxrpc: Fix timeout of a call that hasnt yet been granted a channel
    (no matching commit)
  - [6.3,100/246] perf lock contention: Fix compiler builtin detection
    https://git.kernel.org/bpf/bpf-next/c/17535a33a9c1

You are awesome, thank you!

Patch

diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index da49fcf1c4567..6caa47d352ed6 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -50,15 +50,11 @@  static int rxrpc_wait_to_be_connected(struct rxrpc_call *call, long *timeo)
 	_enter("%d", call->debug_id);
 
 	if (rxrpc_call_state(call) != RXRPC_CALL_CLIENT_AWAIT_CONN)
-		return call->error;
+		goto no_wait;
 
 	add_wait_queue_exclusive(&call->waitq, &myself);
 
 	for (;;) {
-		ret = call->error;
-		if (ret < 0)
-			break;
-
 		switch (call->interruptibility) {
 		case RXRPC_INTERRUPTIBLE:
 		case RXRPC_PREINTERRUPTIBLE:
@@ -69,10 +65,9 @@  static int rxrpc_wait_to_be_connected(struct rxrpc_call *call, long *timeo)
 			set_current_state(TASK_UNINTERRUPTIBLE);
 			break;
 		}
-		if (rxrpc_call_state(call) != RXRPC_CALL_CLIENT_AWAIT_CONN) {
-			ret = call->error;
+
+		if (rxrpc_call_state(call) != RXRPC_CALL_CLIENT_AWAIT_CONN)
 			break;
-		}
 		if ((call->interruptibility == RXRPC_INTERRUPTIBLE ||
 		     call->interruptibility == RXRPC_PREINTERRUPTIBLE) &&
 		    signal_pending(current)) {
@@ -85,6 +80,7 @@  static int rxrpc_wait_to_be_connected(struct rxrpc_call *call, long *timeo)
 	remove_wait_queue(&call->waitq, &myself);
 	__set_current_state(TASK_RUNNING);
 
+no_wait:
 	if (ret == 0 && rxrpc_call_is_complete(call))
 		ret = call->error;