Skip to contentSkip to conversation

mptcp: fix races on accept()

PATCH In review Paolo Abeni 3 participants 8 messages Apr 20, 2020 – Apr 21, 2020

Subsystems (3)
Networking Network drivers Networking MPTCP
lore
Share & tools
lore

Paolo Abeni

author maintainer
#0 lore raw

cover mptcp: fix races on accept()

This series includes some fixes for accept() races which may cause inconsistent MPTCP socket status and oops. Please see the individual patches for the technical details.

Florian Westphal (1):
mptcp: handle mptcp listener destruction via rcu

Paolo Abeni (2):
mptcp: avoid flipping mp_capable field in syn_recv_sock()
mptcp: drop req socket remote_key* fields

 net/mptcp/protocol.c | 11 ++++++--
 net/mptcp/protocol.h |  8 +++---
 net/mptcp/subflow.c  | 66 +++++++++++++++++++++++++++-----------------
 3 files changed, 52 insertions(+), 33 deletions(-)
Signature · 1 line
2.21.1
author maintainer
#1 lore raw

PATCH 1/3 mptcp: handle mptcp listener destruction via rcu

From: Florian Westphal <[email protected]>

Following splat can occur during self test:

 BUG: KASAN: use-after-free in subflow_data_ready+0x156/0x160
 Read of size 8 at addr ffff888100c35c28 by task mptcp_connect/4808

  subflow_data_ready+0x156/0x160
  tcp_child_process+0x6a3/0xb30
  tcp_v4_rcv+0x2231/0x3730
  ip_protocol_deliver_rcu+0x5c/0x860
  ip_local_deliver_finish+0x220/0x360
  ip_local_deliver+0x1c8/0x4e0
  ip_rcv_finish+0x1da/0x2f0
  ip_rcv+0xd0/0x3c0
  __netif_receive_skb_one_core+0xf5/0x160
  __netif_receive_skb+0x27/0x1c0
  process_backlog+0x21e/0x780
  net_rx_action+0x35f/0xe90
  do_softirq+0x4c/0x50
  [..]

This occurs when accessing subflow_ctx->conn.

Problem is that tcp_child_process() calls listen sockets' sk_data_ready() notification, but it doesn't hold the listener lock. Another cpu calling close() on the listener will then cause transition of refcount to 0.

Fixes 58b09919626bf ("mptcp: create msk early")
Signed-off-by Florian Westphal <[email protected]>
Signed-off-by Paolo Abeni <[email protected]>
---
 net/mptcp/protocol.c | 3 +++
 1 file changed, 3 insertions(+)
net/mptcp/protocol.c +3 -0
index b2d2193453bc..d275c1e827fe 100644
@@ -1391,6 +1391,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
 		msk->ack_seq = ack_seq;
 	}
 
+	sock_reset_flag(nsk, SOCK_RCU_FREE);
 	/* will be fully established after successful MPC subflow creation */
 	inet_sk_state_store(nsk, TCP_SYN_RECV);
 	bh_unlock_sock(nsk);

@@ -1792,6 +1793,8 @@ static int mptcp_listen(struct socket *sock, int backlog)
 		goto unlock;
 	}
 
+	sock_set_flag(sock->sk, SOCK_RCU_FREE);
+
 	err = ssock->ops->listen(ssock, backlog);
 	inet_sk_state_store(sock->sk, inet_sk_state_load(ssock->sk));
 	if (!err)
Signature · 1 line
2.21.1
author maintainer
#2 lore raw

PATCH 2/3 mptcp: avoid flipping mp_capable field in syn_recv_sock()

In reply to Paolo Abeni

If multiple CPUs races on the same req_sock in syn_recv_sock(), flipping such field can cause inconsistent child socket status.

When racing, the CPU losing the req ownership may still change the mptcp request socket mp_capable flag while the CPU owning the request is cloning the socket, leaving the child socket with 'is_mptcp' set but no 'mp_capable' flag.

Such socket will stay with 'conn' field cleared, heading to oops in later mptcp callback.

Address the issue tracking the fallback status in a local variable.

Fixes 58b09919626b ("mptcp: create msk early")
Co-developed-by Florian Westphal <[email protected]>
Signed-off-by Florian Westphal <[email protected]>
Signed-off-by Paolo Abeni <[email protected]>
---
 net/mptcp/subflow.c | 46 +++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)
net/mptcp/subflow.c +30 -16
index 3a94f859347a..10090ca3d3e0 100644
@@ -376,6 +376,17 @@ static void mptcp_force_close(struct sock *sk)
 	sk_common_release(sk);
 }
 
+static void subflow_ulp_fallback(struct sock *sk,
+				 struct mptcp_subflow_context *old_ctx)
+{
+	struct inet_connection_sock *icsk = inet_csk(sk);
+
+	mptcp_subflow_tcp_fallback(sk, old_ctx);
+	icsk->icsk_ulp_ops = NULL;
+	rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
+	tcp_sk(sk)->is_mptcp = 0;
+}
+
 static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 					  struct sk_buff *skb,
 					  struct request_sock *req,

@@ -388,6 +399,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	struct tcp_options_received opt_rx;
 	bool fallback_is_fatal = false;
 	struct sock *new_msk = NULL;
+	bool fallback = false;
 	struct sock *child;
 
 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);

@@ -412,14 +424,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			subflow_req->remote_key = opt_rx.mptcp.sndr_key;
 			subflow_req->remote_key_valid = 1;
 		} else {
-			subflow_req->mp_capable = 0;
+			fallback = true;
 			goto create_child;
 		}
 
 create_msk:
 		new_msk = mptcp_sk_clone(listener->conn, req);
 		if (!new_msk)
-			subflow_req->mp_capable = 0;
+			fallback = true;
 	} else if (subflow_req->mp_join) {
 		fallback_is_fatal = true;
 		opt_rx.mptcp.mp_join = 0;

@@ -438,12 +450,18 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	if (child && *own_req) {
 		struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(child);
 
-		/* we have null ctx on TCP fallback, which is fatal on
-		 * MPJ handshake
+		/* we need to fallback on ctx allocation failure and on pre-reqs
+		 * checking above. In the latter scenario we additionally need
+		 * to reset the context to non MPTCP status.
 		 */
-		if (!ctx) {
+		if (!ctx || fallback) {
 			if (fallback_is_fatal)
 				goto close_child;
+
+			if (ctx) {
+				subflow_ulp_fallback(child, ctx);
+				kfree_rcu(ctx, rcu);
+			}
 			goto out;
 		}
 

@@ -474,6 +492,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 	/* dispose of the left over mptcp master, if any */
 	if (unlikely(new_msk))
 		mptcp_force_close(new_msk);
+
+	/* check for expected invariant - should never trigger, just help
+	 * catching eariler subtle bugs
+	 */
+	WARN_ON_ONCE(*own_req && child && tcp_sk(child)->is_mptcp &&
+		     (!mptcp_subflow_ctx(child) ||
+		      !mptcp_subflow_ctx(child)->conn));
 	return child;
 
 close_child:

@@ -1094,17 +1119,6 @@ static void subflow_ulp_release(struct sock *sk)
 	kfree_rcu(ctx, rcu);
 }
 
-static void subflow_ulp_fallback(struct sock *sk,
-				 struct mptcp_subflow_context *old_ctx)
-{
-	struct inet_connection_sock *icsk = inet_csk(sk);
-
-	mptcp_subflow_tcp_fallback(sk, old_ctx);
-	icsk->icsk_ulp_ops = NULL;
-	rcu_assign_pointer(icsk->icsk_ulp_data, NULL);
-	tcp_sk(sk)->is_mptcp = 0;
-}
-
 static void subflow_ulp_clone(const struct request_sock *req,
 			      struct sock *newsk,
 			      const gfp_t priority)
Signature · 1 line
2.21.1
author maintainer
#3 lore raw

PATCH 3/3 mptcp: drop req socket remote_key* fields

In reply to Paolo Abeni

We don't need them, as we can use the current ingress opt data instead. Setting them in syn_recv_sock() may causes inconsistent mptcp socket status, as per previous commit.

Fixes cc7972ea1932 ("mptcp: parse and emit MP_CAPABLE option according to v1 spec")
Signed-off-by Paolo Abeni <[email protected]>
---
 net/mptcp/protocol.c |  8 +++++---
 net/mptcp/protocol.h |  8 ++++----
 net/mptcp/subflow.c  | 20 ++++++++++----------
 3 files changed, 19 insertions(+), 17 deletions(-)
net/mptcp/protocol.c +5 -3
index d275c1e827fe..58ad03fc1bbc 100644
@@ -1345,7 +1345,9 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
 }
 #endif
 
-struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
+struct sock *mptcp_sk_clone(const struct sock *sk,
+			    const struct tcp_options_received *opt_rx,
+			    struct request_sock *req)
 {
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
 	struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);

@@ -1383,9 +1385,9 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
 
 	msk->write_seq = subflow_req->idsn + 1;
 	atomic64_set(&msk->snd_una, msk->write_seq);
-	if (subflow_req->remote_key_valid) {
+	if (opt_rx->mptcp.mp_capable) {
 		msk->can_ack = true;
-		msk->remote_key = subflow_req->remote_key;
+		msk->remote_key = opt_rx->mptcp.sndr_key;
 		mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
 		ack_seq++;
 		msk->ack_seq = ack_seq;
net/mptcp/protocol.h +4 -4
index 67448002a2d7..a2b3048037d0 100644
@@ -206,12 +206,10 @@ struct mptcp_subflow_request_sock {
 	struct	tcp_request_sock sk;
 	u16	mp_capable : 1,
 		mp_join : 1,
-		backup : 1,
-		remote_key_valid : 1;
+		backup : 1;
 	u8	local_id;
 	u8	remote_id;
 	u64	local_key;
-	u64	remote_key;
 	u64	idsn;
 	u32	token;
 	u32	ssn_offset;

@@ -332,7 +330,9 @@ void mptcp_proto_init(void);
 int mptcp_proto_v6_init(void);
 #endif
 
-struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req);
+struct sock *mptcp_sk_clone(const struct sock *sk,
+			    const struct tcp_options_received *opt_rx,
+			    struct request_sock *req);
 void mptcp_get_options(const struct sk_buff *skb,
 		       struct tcp_options_received *opt_rx);
net/mptcp/subflow.c +10 -10
index 10090ca3d3e0..87c094702d63 100644
@@ -133,7 +133,6 @@ static void subflow_init_req(struct request_sock *req,
 
 	subflow_req->mp_capable = 0;
 	subflow_req->mp_join = 0;
-	subflow_req->remote_key_valid = 0;
 
 #ifdef CONFIG_TCP_MD5SIG
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of

@@ -404,6 +403,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
 
+	opt_rx.mptcp.mp_capable = 0;
 	if (tcp_rsk(req)->is_mptcp == 0)
 		goto create_child;
 

@@ -418,18 +418,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			goto create_msk;
 		}
 
-		opt_rx.mptcp.mp_capable = 0;
 		mptcp_get_options(skb, &opt_rx);
-		if (opt_rx.mptcp.mp_capable) {
-			subflow_req->remote_key = opt_rx.mptcp.sndr_key;
-			subflow_req->remote_key_valid = 1;
-		} else {
+		if (!opt_rx.mptcp.mp_capable) {
 			fallback = true;
 			goto create_child;
 		}
 
 create_msk:
-		new_msk = mptcp_sk_clone(listener->conn, req);
+		new_msk = mptcp_sk_clone(listener->conn, &opt_rx, req);
 		if (!new_msk)
 			fallback = true;
 	} else if (subflow_req->mp_join) {

@@ -473,6 +469,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			mptcp_pm_new_connection(mptcp_sk(new_msk), 1);
 			ctx->conn = new_msk;
 			new_msk = NULL;
+
+			/* with OoO packets we can reach here without ingress
+			 * mpc option
+			 */
+			ctx->remote_key = opt_rx.mptcp.sndr_key;
+			ctx->fully_established = opt_rx.mptcp.mp_capable;
+			ctx->can_ack = opt_rx.mptcp.mp_capable;
 		} else if (ctx->mp_join) {
 			struct mptcp_sock *owner;
 

@@ -1152,9 +1155,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		 * is fully established only after we receive the remote key
 		 */
 		new_ctx->mp_capable = 1;
-		new_ctx->fully_established = subflow_req->remote_key_valid;
-		new_ctx->can_ack = subflow_req->remote_key_valid;
-		new_ctx->remote_key = subflow_req->remote_key;
 		new_ctx->local_key = subflow_req->local_key;
 		new_ctx->token = subflow_req->token;
 		new_ctx->ssn_offset = subflow_req->ssn_offset;
Signature · 1 line
2.21.1
maintainer
#6 lore raw
In reply to Paolo Abeni

From: Paolo Abeni <[email protected]>
Date: Mon, 20 Apr 2020 16:25:03 +0200

Quoted 3 lines

This series includes some fixes for accept() races which may cause inconsistent MPTCP socket status and oops. Please see the individual patches for the technical details.

Series applied, thanks.

It seems like patch #3 might be relevant for v5.6 -stable, what's the story here?

#4 lore raw

Re: [PATCH net 3/3] mptcp: drop req socket remote_key* fields

In reply to Paolo Abeni
Quoted 110 lines · Paolo Abeni

We don't need them, as we can use the current ingress opt data instead. Setting them in syn_recv_sock() may causes inconsistent mptcp socket status, as per previous commit.

Fixes cc7972ea1932 ("mptcp: parse and emit MP_CAPABLE option according to v1 spec")
Signed-off-by Paolo Abeni <[email protected]>

---

net/mptcp/protocol.c |  8 +++++---
net/mptcp/protocol.h |  8 ++++----
net/mptcp/subflow.c  | 20 ++++++++++----------
3 files changed, 19 insertions(+), 17 deletions(-)
net/mptcp/protocol.c +5 -3
index d275c1e827fe..58ad03fc1bbc 100644
@@ -1345,7 +1345,9 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
 }
 #endif
 
-struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
+struct sock *mptcp_sk_clone(const struct sock *sk,
+			    const struct tcp_options_received *opt_rx,
+			    struct request_sock *req)
 {
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
 	struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);

@@ -1383,9 +1385,9 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
 
 	msk->write_seq = subflow_req->idsn + 1;
 	atomic64_set(&msk->snd_una, msk->write_seq);
-	if (subflow_req->remote_key_valid) {
+	if (opt_rx->mptcp.mp_capable) {
 		msk->can_ack = true;
-		msk->remote_key = subflow_req->remote_key;
+		msk->remote_key = opt_rx->mptcp.sndr_key;
 		mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
 		ack_seq++;
 		msk->ack_seq = ack_seq;
net/mptcp/protocol.h +4 -4
index 67448002a2d7..a2b3048037d0 100644
@@ -206,12 +206,10 @@ struct mptcp_subflow_request_sock {
 	struct	tcp_request_sock sk;
 	u16	mp_capable : 1,
 		mp_join : 1,
-		backup : 1,
-		remote_key_valid : 1;
+		backup : 1;
 	u8	local_id;
 	u8	remote_id;
 	u64	local_key;
-	u64	remote_key;
 	u64	idsn;
 	u32	token;
 	u32	ssn_offset;

@@ -332,7 +330,9 @@ void mptcp_proto_init(void);
 int mptcp_proto_v6_init(void);
 #endif
 
-struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req);
+struct sock *mptcp_sk_clone(const struct sock *sk,
+			    const struct tcp_options_received *opt_rx,
+			    struct request_sock *req);
 void mptcp_get_options(const struct sk_buff *skb,
 		       struct tcp_options_received *opt_rx);
net/mptcp/subflow.c +2 -6
index 10090ca3d3e0..87c094702d63 100644
@@ -133,7 +133,6 @@ static void subflow_init_req(struct request_sock *req,
 
 	subflow_req->mp_capable = 0;
 	subflow_req->mp_join = 0;
-	subflow_req->remote_key_valid = 0;
 
 #ifdef CONFIG_TCP_MD5SIG
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of

@@ -404,6 +403,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
 
+	opt_rx.mptcp.mp_capable = 0;
 	if (tcp_rsk(req)->is_mptcp == 0)
 		goto create_child;
 

@@ -418,18 +418,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			goto create_msk;
 		}
 
-		opt_rx.mptcp.mp_capable = 0;
 		mptcp_get_options(skb, &opt_rx);
-		if (opt_rx.mptcp.mp_capable) {
-			subflow_req->remote_key = opt_rx.mptcp.sndr_key;
-			subflow_req->remote_key_valid = 1;
-		} else {
+		if (!opt_rx.mptcp.mp_capable) {
 			fallback = true;
 			goto create_child;
 		}

create_msk:

net/mptcp/subflow.c +1 -1
-		new_msk = mptcp_sk_clone(listener->conn, req);
+		new_msk = mptcp_sk_clone(listener->conn, &opt_rx, req);

		if (!new_msk)
			fallback = true;
	} else if (subflow_req->mp_join) {

net/mptcp/subflow.c static struct sock *subflow_syn_recv_sock(const struct sock *sk, · line 478

Quoted 10 lines
net/mptcp/subflow.c +7 -0
@@ -473,6 +469,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			mptcp_pm_new_connection(mptcp_sk(new_msk), 1);
 			ctx->conn = new_msk;
 			new_msk = NULL;
+
+			/* with OoO packets we can reach here without ingress
+			 * mpc option
+			 */
+			ctx->remote_key = opt_rx.mptcp.sndr_key;
+			ctx->fully_established = opt_rx.mptcp.mp_capable;
+			ctx->can_ack = opt_rx.mptcp.mp_capable;

If this code is reached without an incoming mpc option, it looks like ctx->remote_key gets junk off the stack. Maybe this instead?

+ if (opt_rx.mptcp.mp_capable) {
+ ctx->remote_key = opt_rx.mptcp.sndr_key;
+ ctx->fully_established = 1;
+ ctx->can_ack = 1;
+ }

Mat

Quoted 17 lines
net/mptcp/subflow.c +0 -3
 		} else if (ctx->mp_join) {
 			struct mptcp_sock *owner;
 

@@ -1152,9 +1155,6 @@ static void subflow_ulp_clone(const struct request_sock *req,
 		 * is fully established only after we receive the remote key
 		 */
 		new_ctx->mp_capable = 1;
-		new_ctx->fully_established = subflow_req->remote_key_valid;
-		new_ctx->can_ack = subflow_req->remote_key_valid;
-		new_ctx->remote_key = subflow_req->remote_key;
 		new_ctx->local_key = subflow_req->local_key;
 		new_ctx->token = subflow_req->token;
 		new_ctx->ssn_offset = subflow_req->ssn_offset;
Signature · 1 line
2.21.1

--
Mat Martineau
Intel

author maintainer
#5 lore raw

Re: [PATCH net 3/3] mptcp: drop req socket remote_key* fields

Quoted 117 lines · Mat Martineau
Quoted 115 lines · Paolo Abeni

We don't need them, as we can use the current ingress opt data instead. Setting them in syn_recv_sock() may causes inconsistent mptcp socket status, as per previous commit.

Fixes cc7972ea1932 ("mptcp: parse and emit MP_CAPABLE option according to v1 spec")
Signed-off-by Paolo Abeni <[email protected]>

---

net/mptcp/protocol.c |  8 +++++---
net/mptcp/protocol.h |  8 ++++----
net/mptcp/subflow.c  | 20 ++++++++++----------
3 files changed, 19 insertions(+), 17 deletions(-)
net/mptcp/protocol.c +5 -3
index d275c1e827fe..58ad03fc1bbc 100644
@@ -1345,7 +1345,9 @@ static struct ipv6_pinfo *mptcp_inet6_sk(const struct sock *sk)
 }
 #endif
 
-struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
+struct sock *mptcp_sk_clone(const struct sock *sk,
+			    const struct tcp_options_received *opt_rx,
+			    struct request_sock *req)
 {
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
 	struct sock *nsk = sk_clone_lock(sk, GFP_ATOMIC);

@@ -1383,9 +1385,9 @@ struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req)
 
 	msk->write_seq = subflow_req->idsn + 1;
 	atomic64_set(&msk->snd_una, msk->write_seq);
-	if (subflow_req->remote_key_valid) {
+	if (opt_rx->mptcp.mp_capable) {
 		msk->can_ack = true;
-		msk->remote_key = subflow_req->remote_key;
+		msk->remote_key = opt_rx->mptcp.sndr_key;
 		mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
 		ack_seq++;
 		msk->ack_seq = ack_seq;
net/mptcp/protocol.h +4 -4
index 67448002a2d7..a2b3048037d0 100644
@@ -206,12 +206,10 @@ struct mptcp_subflow_request_sock {
 	struct	tcp_request_sock sk;
 	u16	mp_capable : 1,
 		mp_join : 1,
-		backup : 1,
-		remote_key_valid : 1;
+		backup : 1;
 	u8	local_id;
 	u8	remote_id;
 	u64	local_key;
-	u64	remote_key;
 	u64	idsn;
 	u32	token;
 	u32	ssn_offset;

@@ -332,7 +330,9 @@ void mptcp_proto_init(void);
 int mptcp_proto_v6_init(void);
 #endif
 
-struct sock *mptcp_sk_clone(const struct sock *sk, struct request_sock *req);
+struct sock *mptcp_sk_clone(const struct sock *sk,
+			    const struct tcp_options_received *opt_rx,
+			    struct request_sock *req);
 void mptcp_get_options(const struct sk_buff *skb,
 		       struct tcp_options_received *opt_rx);
net/mptcp/subflow.c +2 -6
index 10090ca3d3e0..87c094702d63 100644
@@ -133,7 +133,6 @@ static void subflow_init_req(struct request_sock *req,
 
 	subflow_req->mp_capable = 0;
 	subflow_req->mp_join = 0;
-	subflow_req->remote_key_valid = 0;
 
 #ifdef CONFIG_TCP_MD5SIG
 	/* no MPTCP if MD5SIG is enabled on this socket or we may run out of

@@ -404,6 +403,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 	pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
 
+	opt_rx.mptcp.mp_capable = 0;
 	if (tcp_rsk(req)->is_mptcp == 0)
 		goto create_child;
 

@@ -418,18 +418,14 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			goto create_msk;
 		}
 
-		opt_rx.mptcp.mp_capable = 0;
 		mptcp_get_options(skb, &opt_rx);
-		if (opt_rx.mptcp.mp_capable) {
-			subflow_req->remote_key = opt_rx.mptcp.sndr_key;
-			subflow_req->remote_key_valid = 1;
-		} else {
+		if (!opt_rx.mptcp.mp_capable) {
 			fallback = true;
 			goto create_child;
 		}

create_msk:

net/mptcp/subflow.c +1 -1
-		new_msk = mptcp_sk_clone(listener->conn, req);
+		new_msk = mptcp_sk_clone(listener->conn, &opt_rx, req);

		if (!new_msk)
			fallback = true;
	} else if (subflow_req->mp_join) {
net/mptcp/subflow.c +2 -0
@@ -473,6 +469,13 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 			mptcp_pm_new_connection(mptcp_sk(new_msk), 1);
 			ctx->conn = new_msk;
 			new_msk = NULL;
+
+			/* with OoO packets we can reach here without ingress
Quoted 8 lines

net/mptcp/subflow.c static struct sock *subflow_syn_recv_sock(const struct sock *sk, · line 478

Quoted 5 lines
net/mptcp/subflow.c +5 -0
+			 * mpc option
+			 */
+			ctx->remote_key = opt_rx.mptcp.sndr_key;
+			ctx->fully_established = opt_rx.mptcp.mp_capable;
+			ctx->can_ack = opt_rx.mptcp.mp_capable;

If this code is reached without an incoming mpc option, it looks like ctx->remote_key gets junk off the stack. Maybe this instead?

The idea here is to avoid additional conditional. The remote_key will be used only after 'can_ack' becomes true, and the '!can_ack' condition here is unlikely. Overall this should be faster and safe.

Thanks for checking,

Paolo

author maintainer
#7 lore raw
In reply to David Miller
Quoted 11 lines · David Miller

From: Paolo Abeni <[email protected]>
Date: Mon, 20 Apr 2020 16:25:03 +0200

Quoted 3 lines

This series includes some fixes for accept() races which may cause inconsistent MPTCP socket status and oops. Please see the individual patches for the technical details.

Series applied, thanks.

It seems like patch #3 might be relevant for v5.6 -stable, what's the story here?

Yes, it addresses a race condition present since cc7972ea1932 ("mptcp:
parse and emit MP_CAPABLE option according to v1 spec"). I see now that the changelog is probably a bit too vague, I'm sorry.

Cheers,

Paolo