mptcp: a bunch of fixes
Subsystems (3)
Paolo Abeni
maintainercover 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
Paolo Abeni
maintainerPATCH 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.
--- net/mptcp/protocol.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
net/mptcp/protocol.c +18 -2
@@ -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
Re: [PATCH net 1/3] mptcp: fix unblocking connect()
Quoted 11 lines · Paolo Abeni
Currently unblocking
connect()on MPTCP sockets fails frequently. Ifmptcp_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_statechanges only when needed.
Quoted 6 lines
Fixes 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")Signed-off-by Paolo Abeni <[email protected]>---
net/mptcp/protocol.c| 20 ++++++++++++++++++--1 file changed, 18 insertions(+), 2 deletions(-)
--
Mat Martineau
Intel
Paolo Abeni
maintainerPATCH 2/3 mptcp: fix race between MP_JOIN and close
In reply to Paolo AbeniIf 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.
--- net/mptcp/protocol.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-)
net/mptcp/protocol.c +27 -15
@@ -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
Re: [PATCH net 2/3] mptcp: fix race between MP_JOIN and close
Quoted 30 lines · Paolo Abeni
If a
MP_JOINsubflow 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_sockmptcp_finish_join
inet_sk_state_load
mptcp_token_destroyinet_sk_state_store(TCP_CLOSE)__mptcp_flush_join_list()
mptcp_sock_graftlist_add_tail
sk_common_release
sock_orphan()
<socket free>The
MP_JOINsocket 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_JOIN3whs andmptcp_closevia thejoin_listspinlock. If we detect the msk is closing theMP_JOINsocket is closed, too.
Quoted 6 lines
Fixes f296234c98a8 ("mptcp: Add handling of incoming MP_JOIN requests")Signed-off-by Paolo Abeni <[email protected]>---
net/mptcp/protocol.c| 42 +++++++++++++++++++++++++++---------------1 file changed, 27 insertions(+), 15 deletions(-)
--
Mat Martineau
Intel
Paolo Abeni
maintainerPATCH 3/3 mptcp: remove msk from the token container at destruction time.
In reply to Paolo AbeniCurrently 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.
--- net/mptcp/protocol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/mptcp/protocol.c +1 -1
@@ -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
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")Signed-off-by Paolo Abeni <[email protected]>---
net/mptcp/protocol.c| 2 +-1 file changed, 1 insertion(+), 1 deletion(-)
--
Mat Martineau
Intel
David Miller
maintainerFrom: 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.