Skip to contentSkip to conversation

mptcp: a bunch of fixes

PATCH In review Paolo Abeni 3 participants 8 messages May 29, 2020 – May 31, 2020

Subsystems (3)
lore
Share & tools
lore

Paolo Abeni

author maintainer
#0 lore raw

cover mptcp: a bunch of fixes

This patch series pulls together a few bugfixes for MPTCP bug observed while doing stress-test with apache bench - forced to use MPTCP and multiple subflows.

Paolo Abeni (3):
mptcp: fix unblocking connect()
mptcp: fix race between MP_JOIN and close
mptcp: remove msk from the token container at destruction time.

 net/mptcp/protocol.c | 64 +++++++++++++++++++++++++++++++-------------
 1 file changed, 46 insertions(+), 18 deletions(-)
Signature · 1 line
2.21.3
author maintainer
#1 lore raw

PATCH 1/3 mptcp: fix unblocking connect()

Currently unblocking connect() on MPTCP sockets fails frequently. If mptcp_stream_connect() is invoked to complete a previously attempted unblocking connection, it will still try to create the first subflow via __mptcp_socket_create(). If the 3whs is completed and the 'can_ack' flag is already set, the latter will fail with -EINVAL.

This change addresses the issue checking for pending connect and delegating the completion to the first subflow. Additionally do msk addresses and sk_state changes only when needed.

Fixes 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")
---
 net/mptcp/protocol.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)
net/mptcp/protocol.c +18 -2
index c8675d2eb5b9..8959a74f707d 100644
@@ -1712,6 +1712,14 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 	int err;
 
 	lock_sock(sock->sk);
+	if (sock->state != SS_UNCONNECTED && msk->subflow) {
+		/* pending connection or invalid state, let existing subflow
+		 * cope with that
+		 */
+		ssock = msk->subflow;
+		goto do_connect;
+	}
+
 	ssock = __mptcp_socket_create(msk, TCP_SYN_SENT);
 	if (IS_ERR(ssock)) {
 		err = PTR_ERR(ssock);
@@ -1726,9 +1734,17 @@ static int mptcp_stream_connect(struct socket *sock, struct sockaddr *uaddr,
 		mptcp_subflow_ctx(ssock->sk)->request_mptcp = 0;
 #endif
 
+do_connect:
 	err = ssock->ops->connect(ssock, uaddr, addr_len, flags);
-	inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
-	mptcp_copy_inaddrs(sock->sk, ssock->sk);
+	sock->state = ssock->state;
+
+	/* on successful connect, the msk state will be moved to established by
+	 * subflow_finish_connect()
+	 */
+	if (!err || err == EINPROGRESS)
+		mptcp_copy_inaddrs(sock->sk, ssock->sk);
+	else
+		inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
 
 unlock:
 	release_sock(sock->sk);
Signature · 1 line
2.21.3
Reviewed-by on v1 1/3
#2 lore raw

Re: [PATCH net 1/3] mptcp: fix unblocking connect()

Quoted 11 lines · Paolo Abeni

Currently unblocking connect() on MPTCP sockets fails frequently. If mptcp_stream_connect() is invoked to complete a previously attempted unblocking connection, it will still try to create the first subflow via __mptcp_socket_create(). If the 3whs is completed and the 'can_ack' flag is already set, the latter will fail with -EINVAL.

This change addresses the issue checking for pending connect and delegating the completion to the first subflow. Additionally do msk addresses and sk_state changes only when needed.

Quoted 6 lines
Fixes 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")

---
net/mptcp/protocol.c | 20 ++++++++++++++++++--

1 file changed, 18 insertions(+), 2 deletions(-)

--
Mat Martineau
Intel

author maintainer
#3 lore raw

PATCH 2/3 mptcp: fix race between MP_JOIN and close

In reply to Paolo Abeni

If a MP_JOIN subflow completes the 3whs while another CPU is closing the master msk, we can hit the following race:

CPU1 CPU2

close()
mptcp_close
subflow_syn_recv_sock
mptcp_token_get_sock mptcp_finish_join
inet_sk_state_load
mptcp_token_destroy inet_sk_state_store(TCP_CLOSE) __mptcp_flush_join_list()
mptcp_sock_graft list_add_tail
sk_common_release
sock_orphan()
<socket free>

The MP_JOIN socket will be leaked. Additionally we can hit UaF for the msk 'struct socket' referenced via the 'conn' field.

This change try to address the issue introducing some synchronization between the MP_JOIN 3whs and mptcp_close via the join_list spinlock. If we detect the msk is closing the MP_JOIN socket is closed, too.

Fixes f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")
---
 net/mptcp/protocol.c | 42 +++++++++++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 15 deletions(-)
net/mptcp/protocol.c +27 -15
index 8959a74f707d..35bdfb4f3eae 100644
@@ -1266,8 +1266,12 @@ static void mptcp_close(struct sock *sk, long timeout)
 	mptcp_token_destroy(msk->token);
 	inet_sk_state_store(sk, TCP_CLOSE);
 
-	__mptcp_flush_join_list(msk);
-
+	/* be sure to always acquire the join list lock, to sync vs
+	 * mptcp_finish_join().
+	 */
+	spin_lock_bh(&msk->join_list_lock);
+	list_splice_tail_init(&msk->join_list, &msk->conn_list);
+	spin_unlock_bh(&msk->join_list_lock);
 	list_splice_init(&msk->conn_list, &conn_list);
 
 	data_fin_tx_seq = msk->write_seq;
@@ -1623,22 +1627,30 @@ bool mptcp_finish_join(struct sock *sk)
 	if (!msk->pm.server_side)
 		return true;
 
-	/* passive connection, attach to msk socket */
+	if (!mptcp_pm_allow_new_subflow(msk))
+		return false;
+
+	/* active connections are already on conn_list, and we can't acquire
+	 * msk lock here.
+	 * use the join list lock as synchronization point and double-check
+	 * msk status to avoid racing with mptcp_close()
+	 */
+	spin_lock_bh(&msk->join_list_lock);
+	ret = inet_sk_state_load(parent) == TCP_ESTABLISHED;
+	if (ret && !WARN_ON_ONCE(!list_empty(&subflow->node)))
+		list_add_tail(&subflow->node, &msk->join_list);
+	spin_unlock_bh(&msk->join_list_lock);
+	if (!ret)
+		return false;
+
+	/* attach to msk socket only after we are sure he will deal with us
+	 * at close time
+	 */
 	parent_sock = READ_ONCE(parent->sk_socket);
 	if (parent_sock && !sk->sk_socket)
 		mptcp_sock_graft(sk, parent_sock);
-
-	ret = mptcp_pm_allow_new_subflow(msk);
-	if (ret) {
-		subflow->map_seq = msk->ack_seq;
-
-		/* active connections are already on conn_list */
-		spin_lock_bh(&msk->join_list_lock);
-		if (!WARN_ON_ONCE(!list_empty(&subflow->node)))
-			list_add_tail(&subflow->node, &msk->join_list);
-		spin_unlock_bh(&msk->join_list_lock);
-	}
-	return ret;
+	subflow->map_seq = msk->ack_seq;
+	return true;
 }
 
 bool mptcp_sk_is_subflow(const struct sock *sk)
Signature · 1 line
2.21.3
Reviewed-by on v1 2/3
#4 lore raw

Re: [PATCH net 2/3] mptcp: fix race between MP_JOIN and close

Quoted 30 lines · Paolo Abeni

If a MP_JOIN subflow completes the 3whs while another CPU is closing the master msk, we can hit the following race:

CPU1 CPU2

close()
mptcp_close
subflow_syn_recv_sock
mptcp_token_get_sock mptcp_finish_join
inet_sk_state_load
mptcp_token_destroy inet_sk_state_store(TCP_CLOSE) __mptcp_flush_join_list()
mptcp_sock_graft list_add_tail
sk_common_release
sock_orphan()
<socket free>

The MP_JOIN socket will be leaked. Additionally we can hit UaF for the msk 'struct socket' referenced via the 'conn' field.

This change try to address the issue introducing some synchronization between the MP_JOIN 3whs and mptcp_close via the join_list spinlock. If we detect the msk is closing the MP_JOIN socket is closed, too.

Quoted 6 lines
Fixes f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")

---
net/mptcp/protocol.c | 42 +++++++++++++++++++++++++++---------------

1 file changed, 27 insertions(+), 15 deletions(-)

--
Mat Martineau
Intel

author maintainer
#5 lore raw

PATCH 3/3 mptcp: remove msk from the token container at destruction time.

In reply to Paolo Abeni

Currently we remote the msk from the token container only via mptcp_close(). The MPTCP master socket can be destroyed also via other paths (e.g. if not yet accepted, when shutting down the listener socket). When we hit the latter scenario, dangling msk references are left into the token container, leading to memory corruption and/or UaF.

This change addresses the issue by moving the token removal into the msk destructor.

Fixes 79c0949e9a09 ("mptcp: Add key generation and token tree")
---
 net/mptcp/protocol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/mptcp/protocol.c +1 -1
index 35bdfb4f3eae..34dd0e278a82 100644
@@ -1263,7 +1263,6 @@ static void mptcp_close(struct sock *sk, long timeout)
 
 	lock_sock(sk);
 
-	mptcp_token_destroy(msk->token);
 	inet_sk_state_store(sk, TCP_CLOSE);
 
 	/* be sure to always acquire the join list lock, to sync vs
@@ -1461,6 +1460,7 @@ static void mptcp_destroy(struct sock *sk)
 {
 	struct mptcp_sock *msk = mptcp_sk(sk);
 
+	mptcp_token_destroy(msk->token);
 	if (msk->cached_ext)
 		__skb_ext_put(msk->cached_ext);
 
Signature · 1 line
2.21.3
Reviewed-by on v1 3/3
#6 lore raw

Re: [PATCH net 3/3] mptcp: remove msk from the token container at destruction time.

Quoted 7 lines · Paolo Abeni

Currently we remote the msk from the token container only via mptcp_close(). The MPTCP master socket can be destroyed also via other paths (e.g. if not yet accepted, when shutting down the listener socket). When we hit the latter scenario, dangling msk references are left into the token container, leading to memory corruption and/or UaF.

Quoted 8 lines

This change addresses the issue by moving the token removal into the msk destructor.

Fixes 79c0949e9a09 ("mptcp: Add key generation and token tree")

---
net/mptcp/protocol.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

--
Mat Martineau
Intel

maintainer
#7 lore raw
In reply to Paolo Abeni

From: Paolo Abeni <[email protected]>
Date: Fri, 29 May 2020 17:43:28 +0200

Quoted 3 lines

This patch series pulls together a few bugfixes for MPTCP bug observed while doing stress-test with apache bench - forced to use MPTCP and multiple subflows.

Series applied, and patch #1 and #3 queued up for -stable, thanks.