1. Patchew
  2. MPTCP
  3. View series

[PATCH mptcp-next] Squash to "mptcp: avoid resetting when another subflow available"

Geliang Tang posted 1 patch 6 months, 2 weeks ago
Failed in applying to current master (apply log)
net/mptcp/protocol.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
[PATCH mptcp-next] Squash to "mptcp: avoid resetting when another subflow available"
Posted by Geliang Tang 6 months, 2 weeks ago 
Invoke mptcp_subflow_shutdown() as Paolo suggested.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index a94e1f519788..977bae817e62 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2396,11 +2396,17 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 		goto out_release;
 	}
 
-	dispose_it = msk->free_first || ssk != msk->first ||
-		     !list_is_singular(&msk->conn_list);
+	dispose_it = msk->free_first || ssk != msk->first;
 	if (dispose_it)
 		list_del(&subflow->node);
 
+	if (!dispose_it &&
+	    !list_empty(&msk->conn_list) &&
+	    !list_is_singular(&msk->conn_list)) {
+		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
+		goto out;
+	}
+
 	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
 
 	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
@@ -2447,7 +2453,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 
 	sock_put(ssk);
 
-	if (ssk == msk->first && list_is_singular(&msk->conn_list))
+	if (ssk == msk->first)
 		WRITE_ONCE(msk->first, NULL);
 
 out:
-- 
2.35.3
Re: [PATCH mptcp-next] Squash to "mptcp: avoid resetting when another subflow available"
Posted by Paolo Abeni 6 months, 2 weeks ago 
On Wed, 2023-10-11 at 14:39 +0800, Geliang Tang wrote:
> Invoke mptcp_subflow_shutdown() as Paolo suggested.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/protocol.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index a94e1f519788..977bae817e62 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2396,11 +2396,17 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  		goto out_release;
>  	}
>  
> -	dispose_it = msk->free_first || ssk != msk->first ||
> -		     !list_is_singular(&msk->conn_list);
> +	dispose_it = msk->free_first || ssk != msk->first;
>  	if (dispose_it)
>  		list_del(&subflow->node);
>  
> +	if (!dispose_it &&
> +	    !list_empty(&msk->conn_list) &&
> +	    !list_is_singular(&msk->conn_list)) {
> +		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);


I think there was a misunderstanding. I intended suggesting using
tcp_shutdown(). Note that mptcp_subflow_shutdown() for established msk
is intended to carry the MPTCP-level data fin, while here we want to
only to close the subflow.

I think it's better to let the mptcp_close_ssk() caller specify this
behavior via the 'flags' value. It looks like the already existing
flags are enough: we want this behavior if and only if '!(flags &
MPTCP_CF_FASTCLOSE)'.

All in all, something like the following (untested, based on the
current export branch) should do:
---
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 30e0c29ae0a4..2dabc118d78d 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
 #define MPTCP_CF_PUSH		BIT(1)
 #define MPTCP_CF_FASTCLOSE	BIT(2)
 
+/* be sure to send a reset only if the caller asked for it, also
+ * clean completely the subflow status when the subflow reaches
+ * TCP_CLOSE state
+ */
+static void __mptcp_subflow_disconnect(struct sock *ssk, 
+				       struct mptcp_subflow_context *subflow,
+				       unsigned int flags)
+{
+	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
+	    (flags & MPTCP_CF_FASTCLOSE)) {
+		/* The MPTCP code never wait on the subflow sockets, TCP-level
+		 * disconnect should never fail
+		 */
+		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
+		mptcp_subflow_ctx_reset(subflow);
+	} else {
+		tcp_shutdown(ssk, SEND_SHUTDOWN);
+	}
+}
+
 /* subflow sockets can be either outgoing (connect) or incoming
  * (accept).
  *
@@ -2403,8 +2423,8 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
 
 	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
-		/* be sure to force the tcp_disconnect() path,
-		 * to generate the egress reset
+		/* be sure to force the tcp_close path to generate
+		 * the egress reset
 		 */
 		ssk->sk_lingertime = 0;
 		sock_set_flag(ssk, SOCK_LINGER);
@@ -2413,13 +2433,8 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 
 	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
 	if (!dispose_it) {
-		/* The MPTCP code never wait on the subflow sockets, TCP-level
-		 * disconnect should never fail
-		 */
-		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
-		mptcp_subflow_ctx_reset(subflow);
+		__mptcp_subflow_disconnect(ssk, subflow, flags);
 		release_sock(ssk);
-
 		goto out;
 	}
Re: [PATCH mptcp-next] Squash to "mptcp: avoid resetting when another subflow available"
Posted by Matthieu Baerts 6 months, 2 weeks ago 
Hi Geliang,

11 Oct 2023 08:38:05 Geliang Tang <geliang.tang@suse.com>:

> Invoke mptcp_subflow_shutdown() as Paolo suggested.

Thank you for the new version.

I didn't review it yet but do you mind sending the whole series to have the CI validating it? Here it looks important to do that. Feel free to add in your series my two patches I sent yesterday.

Squash-to patches for series that have not been applied yet are fine to fix comments and small details but it should be limited to that because most of the time, the CI cannot exercise them.

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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.