[PATCH mptcp-net] mptcp: fix possible memory leak on syn_recv race

* [PATCH mptcp-net] mptcp: fix possible memory leak on syn_recv race
@ 2023-03-01 17:56 Paolo Abeni
  2023-03-01 18:15 ` mptcp: fix possible memory leak on syn_recv race: Build Failure MPTCP CI
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Paolo Abeni @ 2023-03-01 17:56 UTC (permalink / raw)
  To: mptcp; +Cc: Christoph Paasch

When subflow_syn_recv_sock() loses the inet hash insert race, the
newly created children will be released by inet_csk_complete_hashdance().

In that scenario, without any further hint, the ulp release callback
will keep the ulp context alive, expecting that the msk socket will
later free it.

Anyway the dying child is not linked to any msk socket, and the context
will be leaked, as reported by Christoph.

Address the issue explicitly releasing the context in the critical
scenario.

Reported-by: Christoph Paasch <cpaasch@apple.com>
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/356
Fixes: cec37a6e41aa ("mptcp: Handle MP_CAPABLE options for outgoing connections")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
This patch is tested only vs the self tests, I have no actual proof
it really closes 356, but it should at least avoid some real leak
---
 net/mptcp/subflow.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index cda6dc4cc6a7..1ca8d30e9276 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -806,7 +806,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	child = listener->icsk_af_ops->syn_recv_sock(sk, skb, req, dst,
 						     req_unhash, own_req);
 
-	if (child && *own_req) {
+	if (likely(child && *own_req)) {
 		struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(child);
 
 		tcp_rsk(req)->drop_req = false;
@@ -898,6 +898,12 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKRX);
 			tcp_rsk(req)->drop_req = true;
 		}
+	} else if (child) {
+		/* inet_csk_complete_hashdance() is going to drop the sock
+		 * soon, but context must be explicitly deleted or will be
+		 * leaked
+		 */
+		mptcp_subflow_drop_ctx(child);
 	}
 
 out:
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread