From 434acf97c0cb97afbbd89ced4619e3edbb29b827 Mon Sep 17 00:00:00 2001
From: Hawkins Jiawei <yin31149@gmail.com>
Date: Mon, 22 Aug 2022 18:59:32 +0800
Subject: [PATCH v2] rxrpc: fix bad unlock balance in rxrpc_do_sendmsg

Syzkaller reports bad unlock balance bug as follows:
------------[ cut here ]------------
WARNING: bad unlock balance detected!
syz-executor.0/4094 is trying to release lock (&call->user_mutex) at:
[<ffffffff87c1d8d1>] rxrpc_do_sendmsg+0x851/0x1110 net/rxrpc/sendmsg.c:754
but there are no more locks to release!

other info that might help us debug this:
no locks held by syz-executor.0/4094.

stack backtrace:
[...]
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0x57/0x7d lib/dump_stack.c:106
 print_unlock_imbalance_bug include/trace/events/lock.h:69 [inline]
 __lock_release kernel/locking/lockdep.c:5333 [inline]
 lock_release.cold+0x49/0x4e kernel/locking/lockdep.c:5686
 __mutex_unlock_slowpath+0x99/0x5e0 kernel/locking/mutex.c:907
 rxrpc_do_sendmsg+0x851/0x1110 net/rxrpc/sendmsg.c:754
 sock_sendmsg_nosec net/socket.c:714 [inline]
 sock_sendmsg+0xab/0xe0 net/socket.c:734
 ____sys_sendmsg+0x5c2/0x7a0 net/socket.c:2485
 ___sys_sendmsg+0xdb/0x160 net/socket.c:2539
 __sys_sendmsg+0xc3/0x160 net/socket.c:2568
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd
 [...]
 </TASK>
------------------------------------

When kernel wants to send a message through a RxRPC socket in
rxrpc_do_sendmsg(), kernel may need to hold the call->user_mutex
lock, then send the message data, and finally unlock the
call->user_mutex.

Yet the problem is that during rxrpc_do_sendmsg(), kernel may call
rxrpc_wait_for_tx_window_intr() to wait for space to appear in the
tx queue or a signal to occur. When a signal occurs,
rxrpc_wait_for_tx_window_intr() may return from mutex_lock_interruptible(),
without acquiring the call->user_mutex lock. Then kernel will triggers
bad unlock balance bug when unlocking the call->user_mutex in
rxrpc_do_sendmsg().

This patch solves it by introducing an indication holding_mutex,
to indicate whether caller can still holds the call->user_mutex when
returned to caller. And patch will check this indication before
unlocking call->user_mutex.

Reported-and-tested-by: syzbot+7f0483225d0c94cb3441@syzkaller.appspotmail.com
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
 net/rxrpc/sendmsg.c | 53 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 8 deletions(-)

diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 1d38e279e2ef..9ea162cb521f 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -33,11 +33,16 @@ static bool rxrpc_check_tx_space(struct rxrpc_call *call, rxrpc_seq_t *_tx_win)
 }
 
 /*
+ * @holding_mutex: An indication whether caller can still holds
+ * the call->user_mutex when returned to caller.
+ * This argument can be NULL, which will effect nothing.
+ *
  * Wait for space to appear in the Tx queue or a signal to occur.
  */
 static int rxrpc_wait_for_tx_window_intr(struct rxrpc_sock *rx,
 					 struct rxrpc_call *call,
-					 long *timeo)
+					 long *timeo,
+					 bool *holding_mutex)
 {
 	for (;;) {
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -53,8 +58,11 @@ static int rxrpc_wait_for_tx_window_intr(struct rxrpc_sock *rx,
 		trace_rxrpc_transmit(call, rxrpc_transmit_wait);
 		mutex_unlock(&call->user_mutex);
 		*timeo = schedule_timeout(*timeo);
-		if (mutex_lock_interruptible(&call->user_mutex) < 0)
+		if (mutex_lock_interruptible(&call->user_mutex) < 0) {
+			if (holding_mutex)
+				*holding_mutex = false;
 			return sock_intr_errno(*timeo);
+		}
 	}
 }
 
@@ -121,13 +129,18 @@ static int rxrpc_wait_for_tx_window_nonintr(struct rxrpc_sock *rx,
 }
 
 /*
+ * @holding_mutex: An indication whether caller can still holds
+ * the call->user_mutex when returned to caller.
+ * This argument can be NULL, which will effect nothing.
+ *
  * wait for space to appear in the transmit/ACK window
  * - caller holds the socket locked
  */
 static int rxrpc_wait_for_tx_window(struct rxrpc_sock *rx,
 				    struct rxrpc_call *call,
 				    long *timeo,
-				    bool waitall)
+				    bool waitall,
+				    bool *holding_mutex)
 {
 	DECLARE_WAITQUEUE(myself, current);
 	int ret;
@@ -142,7 +155,7 @@ static int rxrpc_wait_for_tx_window(struct rxrpc_sock *rx,
 		if (waitall)
 			ret = rxrpc_wait_for_tx_window_waitall(rx, call);
 		else
-			ret = rxrpc_wait_for_tx_window_intr(rx, call, timeo);
+			ret = rxrpc_wait_for_tx_window_intr(rx, call, timeo, holding_mutex);
 		break;
 	case RXRPC_PREINTERRUPTIBLE:
 	case RXRPC_UNINTERRUPTIBLE:
@@ -284,13 +297,19 @@ static int rxrpc_queue_packet(struct rxrpc_sock *rx, struct rxrpc_call *call,
 
 /*
  * send data through a socket
+ *
+ * @holding_mutex: An indication whether caller can still holds
+ * the call->user_mutex when returned to caller.
+ * This argument can be NULL, which will effect nothing.
+ *
  * - must be called in process context
  * - The caller holds the call user access mutex, but not the socket lock.
  */
 static int rxrpc_send_data(struct rxrpc_sock *rx,
 			   struct rxrpc_call *call,
 			   struct msghdr *msg, size_t len,
-			   rxrpc_notify_end_tx_t notify_end_tx)
+			   rxrpc_notify_end_tx_t notify_end_tx,
+			   bool *holding_mutex)
 {
 	struct rxrpc_skb_priv *sp;
 	struct sk_buff *skb;
@@ -299,6 +318,13 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 	bool more;
 	int ret, copied;
 
+	/*
+	 * The caller holds the call->user_mutex when calls
+	 * rxrpc_send_data(), so initial it with True
+	 */
+	if (holding_mutex)
+		*holding_mutex = true;
+
 	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
 
 	/* this should be in poll */
@@ -337,7 +363,8 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 					goto maybe_error;
 				ret = rxrpc_wait_for_tx_window(rx, call,
 							       &timeo,
-							       msg->msg_flags & MSG_WAITALL);
+							       msg->msg_flags & MSG_WAITALL,
+							       holding_mutex);
 				if (ret < 0)
 					goto maybe_error;
 			}
@@ -631,6 +658,12 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
 	unsigned long now, j;
 	int ret;
 
+	/*
+	 * An indication whether rxrpc_do_sendmsg can hold the
+	 * call->user_mutex, after calling the rxrpc_do_sendmsg()
+	 */
+	bool holding_mutex;
+
 	struct rxrpc_send_params p = {
 		.call.tx_total_len	= -1,
 		.call.user_call_ID	= 0,
@@ -747,7 +780,11 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
 		/* Reply phase not begun or not complete for service call. */
 		ret = -EPROTO;
 	} else {
-		ret = rxrpc_send_data(rx, call, msg, len, NULL);
+		ret = rxrpc_send_data(rx, call, msg, len, NULL, &holding_mutex);
+
+		/* ignore mutex_unlock if did not hold call->user_mutex */
+		if (!holding_mutex)
+			goto error_put;
 	}
 
 out_put_unlock:
@@ -796,7 +833,7 @@ int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
 	case RXRPC_CALL_SERVER_ACK_REQUEST:
 	case RXRPC_CALL_SERVER_SEND_REPLY:
 		ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg, len,
-				      notify_end_tx);
+				      notify_end_tx, NULL);
 		break;
 	case RXRPC_CALL_COMPLETE:
 		read_lock_bh(&call->state_lock);
-- 
2.25.1

