diff mbox series

[v2] Squash-to: "mptcp: make msk established at remote key reception time"

Message ID 672ee216582b6b67960fd618c3ed8d75497e0cd6.1595242203.git.pabeni@redhat.com
State Accepted, archived
Commit e405018681d03a5fb49a8ace9d6984f914898706
Delegated to: Matthieu Baerts
Headers show
Series [v2] Squash-to: "mptcp: make msk established at remote key reception time" | expand

Commit Message

Paolo Abeni July 20, 2020, 10:50 a.m. UTC 
Commit message should be rewritten enterily, comprising the title:
'''
mptcp: explicitly track the fully established status

Currently accepted msk sockets become established only after
accept() returns the new sk to user-space.

As MP_JOIN request are refused as per RFC spec on non fully
established socket, the above causes mp_join self-tests
instabilities.

This change lets the msk entering the established status
as soon as it receives the 3rd ack and propagates the first
subflow fully established status on the msk socket.

Finally we can change the subflow acceptance condition to
take in account both the sock state and the msk fully
established flag.

'''

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - add missing read_once/write_once on fully_established access
---
 net/mptcp/options.c  |  1 +
 net/mptcp/protocol.c |  3 ++-
 net/mptcp/protocol.h |  6 ++++++
 net/mptcp/subflow.c  | 12 ++++++++----
 4 files changed, 17 insertions(+), 5 deletions(-)

Comments

Matthieu Baerts July 20, 2020, 3:51 p.m. UTC | #1 
Hi Paolo, Mat,

On 20/07/2020 12:50, Paolo Abeni wrote:
> Commit message should be rewritten enterily, comprising the title:
> '''
> mptcp: explicitly track the fully established status
> 
> Currently accepted msk sockets become established only after
> accept() returns the new sk to user-space.
> 
> As MP_JOIN request are refused as per RFC spec on non fully
> established socket, the above causes mp_join self-tests
> instabilities.
> 
> This change lets the msk entering the established status
> as soon as it receives the 3rd ack and propagates the first
> subflow fully established status on the msk socket.
> 
> Finally we can change the subflow acceptance condition to
> take in account both the sock state and the msk fully
> established flag.
> 
> '''
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
>   - add missing read_once/write_once on fully_established access

Thank you for the patch and the review!

- e405018681d0: "squashed" in "mptcp: make msk established at remote key 
reception time"
- 86cc99a3e2bd: tg:msg: rewrote commit message
- b8a0c13ffff2..72a81073a595: result

Tests + export are going to be started soon!

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 018025ae9f36..3bc56eb608d8 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -709,6 +709,7 @@  static bool check_fully_established(struct mptcp_sock *msk, struct sock *sk,
 		 * additional ack.
 		 */
 		subflow->fully_established = 1;
+		WRITE_ONCE(msk->fully_established, true);
 		goto fully_established;
 	}
 
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index e96c78657fde..c47282e3de70 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1522,6 +1522,7 @@  struct sock *mptcp_sk_clone(const struct sock *sk,
 	msk->local_key = subflow_req->local_key;
 	msk->token = subflow_req->token;
 	msk->subflow = NULL;
+	WRITE_ONCE(msk->fully_established, false);
 
 	msk->write_seq = subflow_req->idsn + 1;
 	atomic64_set(&msk->snd_una, msk->write_seq);
@@ -1854,7 +1855,7 @@  bool mptcp_finish_join(struct sock *sk)
 	pr_debug("msk=%p, subflow=%p", msk, subflow);
 
 	/* mptcp socket already closing? */
-	if (inet_sk_state_load(parent) != TCP_ESTABLISHED)
+	if (!mptcp_is_fully_established(parent))
 		return false;
 
 	if (!msk->pm.server_side)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 08198b61b8a6..45f2d3c60355 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -198,6 +198,7 @@  struct mptcp_sock {
 	u32		token;
 	unsigned long	flags;
 	bool		can_ack;
+	bool		fully_established;
 	spinlock_t	join_list_lock;
 	struct work_struct work;
 	struct list_head conn_list;
@@ -375,6 +376,11 @@  void mptcp_get_options(const struct sk_buff *skb,
 		       struct mptcp_options_received *mp_opt);
 
 void mptcp_finish_connect(struct sock *sk);
+static inline bool mptcp_is_fully_established(struct sock *sk)
+{
+	return inet_sk_state_load(sk) == TCP_ESTABLISHED &&
+	       READ_ONCE(mptcp_sk(sk)->fully_established);
+}
 void mptcp_rcv_space_init(struct mptcp_sock *msk, const struct sock *ssk);
 void mptcp_data_ready(struct sock *sk, struct sock *ssk);
 bool mptcp_finish_join(struct sock *sk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index e79508655fa2..9a979746c0c5 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -395,9 +395,7 @@  void mptcp_subflow_fully_established(struct mptcp_subflow_context *subflow,
 	subflow->remote_key = mp_opt->sndr_key;
 	subflow->fully_established = 1;
 	subflow->can_ack = 1;
-
-	inet_sk_state_store(subflow->conn, TCP_ESTABLISHED);
-	mptcp_pm_new_connection(msk, 1);
+	WRITE_ONCE(msk->fully_established, true);
 }
 
 static struct sock *subflow_syn_recv_sock(const struct sock *sk,
@@ -479,10 +477,16 @@  static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 		}
 
 		if (ctx->mp_capable) {
+			/* this can't race with mptcp_close(), as the msk is
+			 * not yet exposted to user-space
+			 */
+			inet_sk_state_store((void *)new_msk, TCP_ESTABLISHED);
+
 			/* new mpc subflow takes ownership of the newly
 			 * created mptcp socket
 			 */
 			new_msk->sk_destruct = mptcp_sock_destruct;
+			mptcp_pm_new_connection(mptcp_sk(new_msk), 1);
 			mptcp_token_accept(subflow_req, mptcp_sk(new_msk));
 			ctx->conn = new_msk;
 			new_msk = NULL;
@@ -977,7 +981,7 @@  int __mptcp_subflow_connect(struct sock *sk, int ifindex,
 	int addrlen;
 	int err;
 
-	if (sk->sk_state != TCP_ESTABLISHED)
+	if (!mptcp_is_fully_established(sk))
 		return -ENOTCONN;
 
 	err = mptcp_subflow_create_socket(sk, &sf);