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.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 23 ++++++++++++++---------
net/mptcp/subflow.c | 19 +++++--------------
2 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 045570ebad96..4d1c099f4b29 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2373,20 +2373,25 @@ 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);
+ release_sock(ssk);
+ msk->first = NULL;
+ return;
}
dispose_it = !msk->subflow || ssk != msk->subflow->sk;
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