[PATCH mptcp-net] mptcp: fix race between close() and accept()

Paolo Abeni posted 1 patch 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/beea1c3bd7e3fc9b8bc309874cec2597766d7fba.1635958931.git.pabeni@redhat.com
Maintainers: Jakub Kicinski <kuba@kernel.org>, Matthieu Baerts <matthieu.baerts@tessares.net>, "David S. Miller" <davem@davemloft.net>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Paolo Abeni <pabeni@redhat.com>, Davide Caratti <dcaratti@redhat.com>
net/mptcp/protocol.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)

[PATCH mptcp-net] mptcp: fix race between close() and accept()

Posted by Paolo Abeni 1 month ago
The current implementation of mptcp_accept() is prone to the
following race vs mptcp_close():

(on CPU 0)
listen(msk)
accept(msk)
  |-> mptcp_stream_accept(msk)
        |-> <listen status check successful>

(preempted or on CPU 1)
close(msk)
  |-> <msk->sk_state = TCP_CLOSE, ssk is orphaned, ssk state is unchanged>

(preempted or on CPU 0, continuing accept())
  |-> mptcp_accept()
        |-> inet_csk_accept()
              |-> <ssk status check is successful even if ssk is orphaned: oops>

The solution is atomically close the listener subflow at mptcp_close()
time.

Reported-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note: this should close:

https://github.com/multipath-tcp/mptcp_net-next/issues/239

but without a repro I can't be sure...
---
 net/mptcp/protocol.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 1ee3530ac2a3..4c96a9eb2142 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2712,18 +2712,25 @@ static void mptcp_close(struct sock *sk, long timeout)
 	lock_sock(sk);
 	sk->sk_shutdown = SHUTDOWN_MASK;
 
-	if ((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE)) {
-		inet_sk_state_store(sk, TCP_CLOSE);
+	if (sk->sk_state == TCP_LISTEN) {
+		struct socket *listener;
+
+		listener = __mptcp_nmpc_socket(mptcp_sk(sk));
+		if (listener)
+			__mptcp_close_ssk(sk, listener->sk, mptcp_subflow_ctx(listener->sk));
 		goto cleanup;
 	}
 
+	if (sk->sk_state == TCP_CLOSE)
+		goto cleanup;
+
 	if (mptcp_close_state(sk))
 		__mptcp_wr_shutdown(sk);
 
 	sk_stream_wait_close(sk, timeout);
 
 cleanup:
-	/* orphan all the subflows */
+	/* orphan all the non listener subflows */
 	inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32;
 	mptcp_for_each_subflow(mptcp_sk(sk), subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
-- 
2.26.3