The current patch has at least 2 issues:
- the first subflow context cleanup can race with
  the path manager: we should acquire the full ssk socket.
- such subflow can accept data while the msk is closed, leading
  to fwd memory corruption.

Address the issues moving the ssk cleanup into plain mptcp_close_ssk
and detaching the ssk from the msk before closing the latter: no
input data can land to the msk at that point.

v1 -> v2:
- fix memory leak for unaccepted MPC sockets (previous version lacked
  a sock_put()

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 24 ++++++++++++++----------
 net/mptcp/subflow.c  | 19 +++++--------------
 2 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 045570ebad96..741cf0bbffa6 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2373,20 +2373,23 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	bool need_push, dispose_it;
 
-	/* If the first subflow moved to a close state, e.g. due to
-	 * incoming reset and we reach here before accept we need to be able
-	 * to deliver the msk to user-space.
-	 * Do nothing at the moment and take action at accept and/or listener
-	 * shutdown.
-	 * If instead such subflow has been destroyed, e.g. by inet_child_forget
-	 * do the kill
+	/* If the first subflow moved to a close state before accept, e.g. due
+	 * to an incoming reset, mptcp either:
+	 * - if either the subflow or the msk are dead, destroy the context
+	 *   (the subflow socket is deleted by inet_child_forget) and the msk
+	 * - otherwise do nothing at the moment and take action at accept and/or
+	 *   listener shutdown - user-space must be able to accept() the closed
+	 *   socket.
 	 */
 	if (msk->in_accept_queue && msk->first == ssk) {
-		if (!sock_flag(ssk, SOCK_DEAD))
+		if (!sock_flag(sk, SOCK_DEAD) && !sock_flag(ssk, SOCK_DEAD))
 			return;
 
-		/* ensure later check in mptcp_worker will dispose the msk */
+		/* ensure later check in mptcp_worker() will dispose the msk */
 		sock_set_flag(sk, SOCK_DEAD);
+		lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
+		mptcp_subflow_drop_ctx(ssk);
+		goto out_release;
 	}
 
 	dispose_it = !msk->subflow || ssk != msk->subflow->sk;
@@ -2410,7 +2413,6 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 		msk->subflow->state = SS_UNCONNECTED;
 		mptcp_subflow_ctx_reset(subflow);
 		release_sock(ssk);
-
 		goto out;
 	}
 
@@ -2437,6 +2439,8 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 		/* close acquired an extra ref */
 		__sock_put(ssk);
 	}
+
+out_release:
 	release_sock(ssk);
 
 	sock_put(ssk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c6ae5ddd3bb0..38d43a15502b 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -716,9 +716,11 @@ void mptcp_subflow_drop_ctx(struct sock *ssk)
 		return;
 
 	list_del(&mptcp_subflow_ctx(ssk)->node);
-	subflow_ulp_fallback(ssk, ctx);
-	if (ctx->conn)
-		sock_put(ctx->conn);
+	if (inet_csk(ssk)->icsk_ulp_ops) {
+		subflow_ulp_fallback(ssk, ctx);
+		if (ctx->conn)
+			sock_put(ctx->conn);
+	}
 
 	kfree_rcu(ctx, rcu);
 }
@@ -1852,23 +1854,12 @@ void mptcp_subflow_queue_clean(struct sock *listener_sk, struct sock *listener_s
 		sk = (struct sock *)msk;
 
 		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
-		ssk = msk->first;
 		next = msk->dl_next;
 		msk->dl_next = NULL;
 
 		__mptcp_unaccepted_force_close(sk);
 		release_sock(sk);
 
-		/* the first subflow is not touched by the above, as the msk
-		 * is still in the accept queue, see __mptcp_close_ssk,
-		 * we need to release only the ctx related resources, the
-		 * tcp socket will be destroyed by inet_csk_listen_stop()
-		 */
-		lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
-		mptcp_subflow_drop_ctx(ssk);
-		release_sock(ssk);
-		sock_put(ssk);
-
 		/* lockdep will report a false positive ABBA deadlock
 		 * between cancel_work_sync and the listener socket.
 		 * The involved locks belong to different sockets WRT
-- 
2.39.2

Hi Paolo,

On 12/04/2023 15:45, Paolo Abeni wrote:
> The current patch has at least 2 issues:
> - the first subflow context cleanup can race with
>   the path manager: we should acquire the full ssk socket.
> - such subflow can accept data while the msk is closed, leading
>   to fwd memory corruption.
> 
> Address the issues moving the ssk cleanup into plain mptcp_close_ssk
> and detaching the ssk from the msk before closing the latter: no
> input data can land to the msk at that point.

Thank you for the new version!

> v1 -> v2:
> - fix memory leak for unaccepted MPC sockets (previous version lacked
>   a sock_put()

Good catch, I see the modification.

New patches for t/upstream-net and t/upstream:
- 0e8001f374ec: Revert "Squash-to: "mptcp: fix accept vs worker race""
- 30fc4dff89a7: Revert "mptcp: reduce scope of ssk"
- e6b49563f4b4: "squashed" in "mptcp: fix accept vs worker race"
- 8a56bac92a70: Revert "Revert "mptcp: reduce scope of ssk""
- Results: 497462bd7901..a234873f266c (export-net)
- Results: 5edc47a5f768..263b6c28fab6 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230412T140112
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230412T140112

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net