mptcp: fix master unlock race in mptcp_disconnect

When creating an actual mptcp socket out of a request, a call to
mptcp_create_master_sk() creates a master socket which is locked and
needs to be unlocked in all (error) paths.

Commit 79d7578 ("mptcp: Fix error-cases in TCP_SYNCOOKIES path")
tries to address this by blindly unlocking any locked subflows in
mptcp_disconnect, but this can lead to races with other CPUs that may
also have taken the lock on this socket. Our internal test suite
occasionally pops a warning that indicates that this happens, although
very rarely. This modification removes this scary unlock.

Commit 79d7578 ("mptcp: Fix error-cases in TCP_SYNCOOKIES path")
only mentions the path coming from TCP_SYNCOOKIES, but in reality there
are other paths that can trigger the error and subsequent unlock in
mptcp_disconnect() through tcp_conn_request(), tcp_check_req() and
mptcp_check_req_master(). Only the path in tcp_conn_request() needs to
be adapted after having changed inet_csk_reqsk_queue_add().

Fixes: 79d7578 ("mptcp: Fix error-cases in TCP_SYNCOOKIES path")
Signed-off-by: Tim Froidcoeur <[email protected]>
Acked-by: Christoph Paasch <[email protected]>
Signed-off-by: Matthieu Baerts <[email protected]>

net/ipv4/inet_connection_sock.c

@@ -934,6 +934,14 @@ struct sock *inet_csk_reqsk_queue_add(struct sock *sk,
 
 	spin_lock(&queue->rskq_lock);
 	if (unlikely(sk->sk_state != TCP_LISTEN)) {
+		struct tcp_sock *tp = tcp_sk(sk);
+
+		/* in case of mptcp, two locks may been taken, one
+		 * on the meta, the other on master_sk
+		 */
+		if (mptcp(tp) && tp->mpcb && tp->mpcb->master_sk)
+			bh_unlock_sock(tp->mpcb->master_sk);
+
 		inet_child_forget(sk, req, child);
 		child = NULL;
 	} else {

net/ipv4/tcp_input.c

@@ -6860,9 +6860,11 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
 		/* Add the child socket directly into the accept queue */
 		if (!inet_csk_reqsk_queue_add(sk, req, meta_sk)) {
 			reqsk_fastopen_remove(fastopen_sk, req, false);
-			bh_unlock_sock(fastopen_sk);
-			if (meta_sk != fastopen_sk)
-				bh_unlock_sock(meta_sk);
+			/* in the case of mptcp, on failure, the master subflow
+			 * socket (==fastopen_sk) will already have been unlocked
+			 * by the failed call to inet_csk_reqsk_queue_add
+			 */
+			bh_unlock_sock(meta_sk);
 			sock_put(fastopen_sk);
 			goto drop_and_free;
 		}

net/mptcp/mptcp_ctrl.c

@@ -2116,9 +2116,6 @@ void mptcp_disconnect(struct sock *meta_sk)
 	mptcp_for_each_sub_safe(meta_tp->mpcb, mptcp, tmp) {
 		struct sock *subsk = mptcp_to_sock(mptcp);
 
-		if (spin_is_locked(&subsk->sk_lock.slock))
-			bh_unlock_sock(subsk);
-
 		tcp_sk(subsk)->tcp_disconnect = 1;
 
 		meta_sk->sk_prot->disconnect(subsk, O_NONBLOCK);