[PATCH mptcp-net v5 0/2] mptcp: fix another close hang-up

[PATCH mptcp-net v5 0/2] mptcp: fix another close hang-up

[Paolo Abeni]

If the TCP subflows close silently due to timeouts, the msk socket
can hang-up indefinitely.

Implement an additional close timeout triggered with the last subflow
close event and move the msk to TCP_CLOSE when it fires.

Should address issues/430 and issues/431.

v4 -> v5:
 - let the tout always expire for dead socks (Mat)
 - fix comment type (Mat)
 - targeting net, added fixes tag

v3 -> v4:
 - fix off-by-one in timer update/check (sigh, trival math is too
   hard:/)

v2 -> v3:
 - fix int overlflow in patch 2/2

v1 -> v2:
 - fix timeout timer scheduling in patch 2/2 (timeout never fired on v1)

Paolo Abeni (2):
  mptcp: rename timer related helper to less confusing names
  mptcp: fix dangling connection hang-up

 net/mptcp/protocol.c | 124 +++++++++++++++++++++----------------------
 net/mptcp/protocol.h |  26 ++++++++-
 net/mptcp/subflow.c  |   3 +-
 3 files changed, 88 insertions(+), 65 deletions(-)

-- 
2.41.0

[PATCH mptcp-net v5 1/2] mptcp: rename timer related helper to less confusing names

[Paolo Abeni]

The msk socket uses to different timeout to track close related
events and retransmissions. The existing helpers do not indicate
clearly which timer they actually touch, making the related code
quite confusing.

Change the existing helpers name to avoid such confusion. No
functional change intended.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/protocol.c | 42 +++++++++++++++++++++---------------------
 net/mptcp/protocol.h |  2 +-
 net/mptcp/subflow.c  |  2 +-
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index e9b74295d475..b9497712eb8f 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -405,7 +405,7 @@ static bool __mptcp_move_skb(struct mptcp_sock *msk, struct sock *ssk,
 	return false;
 }
 
-static void mptcp_stop_timer(struct sock *sk)
+static void mptcp_stop_rtx_timer(struct sock *sk)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
 
@@ -911,12 +911,12 @@ static void __mptcp_flush_join_list(struct sock *sk, struct list_head *join_list
 	}
 }
 
-static bool mptcp_timer_pending(struct sock *sk)
+static bool mptcp_rtx_timer_pending(struct sock *sk)
 {
 	return timer_pending(&inet_csk(sk)->icsk_retransmit_timer);
 }
 
-static void mptcp_reset_timer(struct sock *sk)
+static void mptcp_reset_rtx_timer(struct sock *sk)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	unsigned long tout;
@@ -1050,10 +1050,10 @@ static void __mptcp_clean_una(struct sock *sk)
 out:
 	if (snd_una == READ_ONCE(msk->snd_nxt) &&
 	    snd_una == READ_ONCE(msk->write_seq)) {
-		if (mptcp_timer_pending(sk) && !mptcp_data_fin_enabled(msk))
-			mptcp_stop_timer(sk);
+		if (mptcp_rtx_timer_pending(sk) && !mptcp_data_fin_enabled(msk))
+			mptcp_stop_rtx_timer(sk);
 	} else {
-		mptcp_reset_timer(sk);
+		mptcp_reset_rtx_timer(sk);
 	}
 }
 
@@ -1626,8 +1626,8 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 		mptcp_push_release(ssk, &info);
 
 	/* ensure the rtx timer is running */
-	if (!mptcp_timer_pending(sk))
-		mptcp_reset_timer(sk);
+	if (!mptcp_rtx_timer_pending(sk))
+		mptcp_reset_rtx_timer(sk);
 	if (do_check_data_fin)
 		mptcp_check_send_data_fin(sk);
 }
@@ -1690,8 +1690,8 @@ static void __mptcp_subflow_push_pending(struct sock *sk, struct sock *ssk, bool
 	if (copied) {
 		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
 			 info.size_goal);
-		if (!mptcp_timer_pending(sk))
-			mptcp_reset_timer(sk);
+		if (!mptcp_rtx_timer_pending(sk))
+			mptcp_reset_rtx_timer(sk);
 
 		if (msk->snd_data_fin_enable &&
 		    msk->snd_nxt + 1 == msk->write_seq)
@@ -2260,7 +2260,7 @@ static void mptcp_retransmit_timer(struct timer_list *t)
 	sock_put(sk);
 }
 
-static void mptcp_timeout_timer(struct timer_list *t)
+static void mptcp_tout_timer(struct timer_list *t)
 {
 	struct sock *sk = from_timer(sk, t, sk_timer);
 
@@ -2629,14 +2629,14 @@ static void __mptcp_retrans(struct sock *sk)
 reset_timer:
 	mptcp_check_and_set_pending(sk);
 
-	if (!mptcp_timer_pending(sk))
-		mptcp_reset_timer(sk);
+	if (!mptcp_rtx_timer_pending(sk))
+		mptcp_reset_rtx_timer(sk);
 }
 
 /* schedule the timeout timer for the relevant event: either close timeout
  * or mp_fail timeout. The close timeout takes precedence on the mp_fail one
  */
-void mptcp_reset_timeout(struct mptcp_sock *msk, unsigned long fail_tout)
+void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout)
 {
 	struct sock *sk = (struct sock *)msk;
 	unsigned long timeout, close_timeout;
@@ -2669,7 +2669,7 @@ static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
 	WRITE_ONCE(mptcp_subflow_ctx(ssk)->fail_tout, 0);
 	unlock_sock_fast(ssk, slow);
 
-	mptcp_reset_timeout(msk, 0);
+	mptcp_reset_tout_timer(msk, 0);
 }
 
 static void mptcp_do_fastclose(struct sock *sk)
@@ -2758,7 +2758,7 @@ static void __mptcp_init_sock(struct sock *sk)
 
 	/* re-use the csk retrans timer for MPTCP-level retrans */
 	timer_setup(&msk->sk.icsk_retransmit_timer, mptcp_retransmit_timer, 0);
-	timer_setup(&sk->sk_timer, mptcp_timeout_timer, 0);
+	timer_setup(&sk->sk_timer, mptcp_tout_timer, 0);
 }
 
 static void mptcp_ca_reset(struct sock *sk)
@@ -2849,8 +2849,8 @@ void mptcp_subflow_shutdown(struct sock *sk, struct sock *ssk, int how)
 		} else {
 			pr_debug("Sending DATA_FIN on subflow %p", ssk);
 			tcp_send_ack(ssk);
-			if (!mptcp_timer_pending(sk))
-				mptcp_reset_timer(sk);
+			if (!mptcp_rtx_timer_pending(sk))
+				mptcp_reset_rtx_timer(sk);
 		}
 		break;
 	}
@@ -2933,7 +2933,7 @@ static void __mptcp_destroy_sock(struct sock *sk)
 
 	might_sleep();
 
-	mptcp_stop_timer(sk);
+	mptcp_stop_rtx_timer(sk);
 	sk_stop_timer(sk, &sk->sk_timer);
 	msk->pm.status = 0;
 	mptcp_release_sched(msk);
@@ -3053,7 +3053,7 @@ bool __mptcp_close(struct sock *sk, long timeout)
 		__mptcp_destroy_sock(sk);
 		do_cancel_work = true;
 	} else {
-		mptcp_reset_timeout(msk, 0);
+		mptcp_reset_tout_timer(msk, 0);
 	}
 
 	return do_cancel_work;
@@ -3116,7 +3116,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	mptcp_check_listen_stop(sk);
 	inet_sk_state_store(sk, TCP_CLOSE);
 
-	mptcp_stop_timer(sk);
+	mptcp_stop_rtx_timer(sk);
 	sk_stop_timer(sk, &sk->sk_timer);
 
 	if (msk->token)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 12d70ce24843..4fcce9ad7d04 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -725,7 +725,7 @@ void mptcp_get_options(const struct sk_buff *skb,
 
 void mptcp_finish_connect(struct sock *sk);
 void __mptcp_set_connected(struct sock *sk);
-void mptcp_reset_timeout(struct mptcp_sock *msk, unsigned long fail_tout);
+void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout);
 static inline bool mptcp_is_fully_established(struct sock *sk)
 {
 	return inet_sk_state_load(sk) == TCP_ESTABLISHED &&
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 2f40c23fdb0d..433f290984c8 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1226,7 +1226,7 @@ static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk)
 	WRITE_ONCE(subflow->fail_tout, fail_tout);
 	tcp_send_ack(ssk);
 
-	mptcp_reset_timeout(msk, subflow->fail_tout);
+	mptcp_reset_tout_timer(msk, subflow->fail_tout);
 }
 
 static bool subflow_check_data_avail(struct sock *ssk)
-- 
2.41.0

[PATCH mptcp-net v5 2/2] mptcp: fix dangling connection hang-up

[Paolo Abeni]

According to RFC 8684 section 3.3:

  A connection is not closed unless [...] or an implementation-specific
  connection-level send timeout.

Currently the MPTCP protocol does not implement such timeout, and
connection timing-out at the TCP-level never move to close state.

Introduces a catch-up condition at subflow close time to move the
MPTCP socket to close, too.

That additionally allow removing similar existing inside the worker.

Finally, allow some additional timeout for plain ESTABLISHED mptcp
sockets, as the protocol allows creating new subflows even at that
point and making the connection functional again.

The issues is actually present since the beginning, but it basically
impossible to solve without a long chain of functional pre-requisites
topped by the blamed commit below.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/430
Fixes: bbd49d114d57 ("mptcp: consolidate transition to TCP_CLOSE in mptcp_do_fastclose()")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v4 -> v5:
 - let the tout always expire for dead socks (Mat)
 - fix comment type (Mat)
 - targeting net, added fixes tag

v3 -> v4:
 - fix off-by-one in timer update/check (sigh, trival math is too
   hard:/)

v2 -> v3:
 - fix integer overflow in mptcp_close_tout_expired()
v1 -> v2:
 - really set the close timeout as need.
---
 net/mptcp/protocol.c | 86 ++++++++++++++++++++++----------------------
 net/mptcp/protocol.h | 24 +++++++++++++
 net/mptcp/subflow.c  |  1 +
 3 files changed, 67 insertions(+), 44 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index b9497712eb8f..3facdc006adc 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -892,6 +892,7 @@ static bool __mptcp_finish_join(struct mptcp_sock *msk, struct sock *ssk)
 	mptcp_subflow_ctx(ssk)->subflow_id = msk->subflow_id++;
 	mptcp_sockopt_sync_locked(msk, ssk);
 	mptcp_subflow_joined(msk, ssk);
+	mptcp_stop_tout_timer(sk);
 	return true;
 }
 
@@ -2369,18 +2370,14 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	bool dispose_it, need_push = false;
 
 	/* 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.
+	 * to an incoming reset or listener shutdown, the subflow socket is
+	 * already deleted by inet_child_forget() and the mptcp socket can't
+	 * survive too.
 	 */
-	if (msk->in_accept_queue && msk->first == ssk) {
-		if (!sock_flag(sk, SOCK_DEAD) && !sock_flag(ssk, SOCK_DEAD))
-			return;
-
+	if (msk->in_accept_queue && msk->first == ssk &&
+	    (sock_flag(sk, SOCK_DEAD) || sock_flag(ssk, SOCK_DEAD))) {
 		/* ensure later check in mptcp_worker() will dispose the msk */
+		mptcp_set_close_tout(sk, tcp_jiffies32 - (TCP_TIMEWAIT_LEN + 1));
 		sock_set_flag(sk, SOCK_DEAD);
 		lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
 		mptcp_subflow_drop_ctx(ssk);
@@ -2443,6 +2440,22 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 out:
 	if (need_push)
 		__mptcp_push_pending(sk, 0);
+
+	/* Catch every 'all subflows closed' scenario, including peers silently
+	 * closing them, e.g. due to timeout.
+	 * For established sockets, allow an additional timeout before closing,
+	 * as the protocol can still create more subflows.
+	 */
+	if (list_is_singular(&msk->conn_list) && msk->first &&
+	    inet_sk_state_load(msk->first) == TCP_CLOSE) {
+		if (sk->sk_state != TCP_ESTABLISHED ||
+		    msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
+			inet_sk_state_store(sk, TCP_CLOSE);
+			mptcp_close_wake_up(sk);
+		} else {
+			mptcp_start_tout_timer(sk);
+		}
+	}
 }
 
 void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
@@ -2486,23 +2499,14 @@ static void __mptcp_close_subflow(struct sock *sk)
 
 }
 
-static bool mptcp_should_close(const struct sock *sk)
+static bool mptcp_close_tout_expired(const struct sock *sk)
 {
-	s32 delta = tcp_jiffies32 - inet_csk(sk)->icsk_mtup.probe_timestamp;
-	struct mptcp_subflow_context *subflow;
-
-	if (delta >= TCP_TIMEWAIT_LEN || mptcp_sk(sk)->in_accept_queue)
-		return true;
+	if (!inet_csk(sk)->icsk_mtup.probe_timestamp ||
+	    sk->sk_state == TCP_CLOSE)
+		return false;
 
-	/* if all subflows are in closed status don't bother with additional
-	 * timeout
-	 */
-	mptcp_for_each_subflow(mptcp_sk(sk), subflow) {
-		if (inet_sk_state_load(mptcp_subflow_tcp_sock(subflow)) !=
-		    TCP_CLOSE)
-			return false;
-	}
-	return true;
+	return time_after32(tcp_jiffies32,
+		  inet_csk(sk)->icsk_mtup.probe_timestamp + TCP_TIMEWAIT_LEN);
 }
 
 static void mptcp_check_fastclose(struct mptcp_sock *msk)
@@ -2641,15 +2645,16 @@ void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout)
 	struct sock *sk = (struct sock *)msk;
 	unsigned long timeout, close_timeout;
 
-	if (!fail_tout && !sock_flag(sk, SOCK_DEAD))
+	if (!fail_tout && !inet_csk(sk)->icsk_mtup.probe_timestamp)
 		return;
 
-	close_timeout = inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32 + jiffies + TCP_TIMEWAIT_LEN;
+	close_timeout = inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32 + jiffies +
+			TCP_TIMEWAIT_LEN;
 
 	/* the close timeout takes precedence on the fail one, and here at least one of
 	 * them is active
 	 */
-	timeout = sock_flag(sk, SOCK_DEAD) ? close_timeout : fail_tout;
+	timeout = inet_csk(sk)->icsk_mtup.probe_timestamp ? close_timeout : fail_tout;
 
 	sk_reset_timer(sk, &sk->sk_timer, timeout);
 }
@@ -2668,8 +2673,6 @@ static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
 	mptcp_subflow_reset(ssk);
 	WRITE_ONCE(mptcp_subflow_ctx(ssk)->fail_tout, 0);
 	unlock_sock_fast(ssk, slow);
-
-	mptcp_reset_tout_timer(msk, 0);
 }
 
 static void mptcp_do_fastclose(struct sock *sk)
@@ -2706,18 +2709,14 @@ static void mptcp_worker(struct work_struct *work)
 	if (test_and_clear_bit(MPTCP_WORK_CLOSE_SUBFLOW, &msk->flags))
 		__mptcp_close_subflow(sk);
 
-	/* There is no point in keeping around an orphaned sk timedout or
-	 * closed, but we need the msk around to reply to incoming DATA_FIN,
-	 * even if it is orphaned and in FIN_WAIT2 state
-	 */
-	if (sock_flag(sk, SOCK_DEAD)) {
-		if (mptcp_should_close(sk))
-			mptcp_do_fastclose(sk);
+	if (mptcp_close_tout_expired(sk)) {
+		mptcp_do_fastclose(sk);
+		mptcp_close_wake_up(sk);
+	}
 
-		if (sk->sk_state == TCP_CLOSE) {
-			__mptcp_destroy_sock(sk);
-			goto unlock;
-		}
+	if (sock_flag(sk, SOCK_DEAD) && sk->sk_state == TCP_CLOSE) {
+		__mptcp_destroy_sock(sk);
+		goto unlock;
 	}
 
 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
@@ -3016,7 +3015,6 @@ bool __mptcp_close(struct sock *sk, long timeout)
 
 cleanup:
 	/* orphan all the subflows */
-	inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32;
 	mptcp_for_each_subflow(msk, subflow) {
 		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
 		bool slow = lock_sock_fast_nested(ssk);
@@ -3053,7 +3051,7 @@ bool __mptcp_close(struct sock *sk, long timeout)
 		__mptcp_destroy_sock(sk);
 		do_cancel_work = true;
 	} else {
-		mptcp_reset_tout_timer(msk, 0);
+		mptcp_start_tout_timer(sk);
 	}
 
 	return do_cancel_work;
@@ -3117,7 +3115,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
 	inet_sk_state_store(sk, TCP_CLOSE);
 
 	mptcp_stop_rtx_timer(sk);
-	sk_stop_timer(sk, &sk->sk_timer);
+	mptcp_stop_tout_timer(sk);
 
 	if (msk->token)
 		mptcp_event(MPTCP_EVENT_CLOSED, msk, NULL, GFP_KERNEL);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4fcce9ad7d04..74889f072d2d 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -726,6 +726,30 @@ void mptcp_get_options(const struct sk_buff *skb,
 void mptcp_finish_connect(struct sock *sk);
 void __mptcp_set_connected(struct sock *sk);
 void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout);
+
+static inline void mptcp_stop_tout_timer(struct sock *sk)
+{
+	if (!inet_csk(sk)->icsk_mtup.probe_timestamp)
+		return;
+
+	sk_stop_timer(sk, &sk->sk_timer);
+	inet_csk(sk)->icsk_mtup.probe_timestamp = 0;
+}
+
+static inline void mptcp_set_close_tout(struct sock *sk, unsigned long tout)
+{
+	/* avoid 0 timestamp, at that means no close timeout */
+	inet_csk(sk)->icsk_mtup.probe_timestamp = tout;
+	if (!inet_csk(sk)->icsk_mtup.probe_timestamp)
+		inet_csk(sk)->icsk_mtup.probe_timestamp = 1;
+}
+
+static inline void mptcp_start_tout_timer(struct sock *sk)
+{
+	mptcp_set_close_tout(sk, tcp_jiffies32);
+	mptcp_reset_tout_timer(mptcp_sk(sk), 0);
+}
+
 static inline bool mptcp_is_fully_established(struct sock *sk)
 {
 	return inet_sk_state_load(sk) == TCP_ESTABLISHED &&
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 433f290984c8..918c1a235790 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1552,6 +1552,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	mptcp_sock_graft(ssk, sk->sk_socket);
 	iput(SOCK_INODE(sf));
 	WRITE_ONCE(msk->allow_infinite_fallback, false);
+	mptcp_stop_tout_timer(sk);
 	return 0;
 
 failed_unlink:
-- 
2.41.0

Re: mptcp: fix dangling connection hang-up: Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6431617787363328
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6431617787363328/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5587192857231360
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5587192857231360/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5024242903810048
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5024242903810048/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 3 failed test(s): packetdrill_dss packetdrill_fastclose packetdrill_mp_join 🔴:
  - Task: https://cirrus-ci.com/task/6150142810652672
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6150142810652672/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/957900765691


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-net v5 2/2] mptcp: fix dangling connection hang-up

[Matthieu Baerts]

Hi Paolo,

On 06/09/2023 10:30, Paolo Abeni wrote:
> According to RFC 8684 section 3.3:
> 
>   A connection is not closed unless [...] or an implementation-specific
>   connection-level send timeout.
> 
> Currently the MPTCP protocol does not implement such timeout, and
> connection timing-out at the TCP-level never move to close state.
> 
> Introduces a catch-up condition at subflow close time to move the
> MPTCP socket to close, too.
> 
> That additionally allow removing similar existing inside the worker.
> 
> Finally, allow some additional timeout for plain ESTABLISHED mptcp
> sockets, as the protocol allows creating new subflows even at that
> point and making the connection functional again.
> 
> The issues is actually present since the beginning, but it basically
> impossible to solve without a long chain of functional pre-requisites
> topped by the blamed commit below.

Would it not be better to put another "Fixes" tag and mention that
bbd49d114d57 is required for the backports?

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/430
> Fixes: bbd49d114d57 ("mptcp: consolidate transition to TCP_CLOSE in mptcp_do_fastclose()")

My fear is that this patch will not be automatically selected to be
backported below 6.5.

Instead, I suggest to use:

Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")

The issue was certainly there before but hard to backport without that
one anyway.

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

Re: [PATCH mptcp-net v5 2/2] mptcp: fix dangling connection hang-up

[Matthieu Baerts]

Hi Paolo,

On 06/09/2023 10:30, Paolo Abeni wrote:
> According to RFC 8684 section 3.3:
> 
>   A connection is not closed unless [...] or an implementation-specific
>   connection-level send timeout.
> 
> Currently the MPTCP protocol does not implement such timeout, and
> connection timing-out at the TCP-level never move to close state.
> 
> Introduces a catch-up condition at subflow close time to move the
> MPTCP socket to close, too.
> 
> That additionally allow removing similar existing inside the worker.
> 
> Finally, allow some additional timeout for plain ESTABLISHED mptcp
> sockets, as the protocol allows creating new subflows even at that
> point and making the connection functional again.
> 
> The issues is actually present since the beginning, but it basically
> impossible to solve without a long chain of functional pre-requisites
> topped by the blamed commit below.

Thank you for the update!

(...)

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 4fcce9ad7d04..74889f072d2d 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -726,6 +726,30 @@ void mptcp_get_options(const struct sk_buff *skb,
>  void mptcp_finish_connect(struct sock *sk);
>  void __mptcp_set_connected(struct sock *sk);
>  void mptcp_reset_tout_timer(struct mptcp_sock *msk, unsigned long fail_tout);
> +
> +static inline void mptcp_stop_tout_timer(struct sock *sk)
> +{
> +	if (!inet_csk(sk)->icsk_mtup.probe_timestamp)
> +		return;
> +
> +	sk_stop_timer(sk, &sk->sk_timer);
> +	inet_csk(sk)->icsk_mtup.probe_timestamp = 0;
> +}
> +
> +static inline void mptcp_set_close_tout(struct sock *sk, unsigned long tout)
> +{
> +	/* avoid 0 timestamp, at that means no close timeout */
> +	inet_csk(sk)->icsk_mtup.probe_timestamp = tout;
> +	if (!inet_csk(sk)->icsk_mtup.probe_timestamp)
> +		inet_csk(sk)->icsk_mtup.probe_timestamp = 1;

Small detail: maybe more readable with:

  /* avoid 0 timestamp, as that means no close timeout */
  inet_csk(sk)->icsk_mtup.probe_timestamp = tout ? : 1;


I can do the modification when applying the patch if that's OK.

> +}
> +
> +static inline void mptcp_start_tout_timer(struct sock *sk)
> +{
> +	mptcp_set_close_tout(sk, tcp_jiffies32);
> +	mptcp_reset_tout_timer(mptcp_sk(sk), 0);
> +}
> +
>  static inline bool mptcp_is_fully_established(struct sock *sk)
>  {
>  	return inet_sk_state_load(sk) == TCP_ESTABLISHED &&
Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-net v5 0/2] mptcp: fix another close hang-up

[Matthieu Baerts]

Hi Paolo, Mat,

On 06/09/2023 10:30, Paolo Abeni wrote:
> If the TCP subflows close silently due to timeouts, the msk socket
> can hang-up indefinitely.
> 
> Implement an additional close timeout triggered with the last subflow
> close event and move the msk to TCP_CLOSE when it fires.
> 
> Should address issues/430 and issues/431.
> 
> v4 -> v5:
>  - let the tout always expire for dead socks (Mat)
>  - fix comment type (Mat)
>  - targeting net, added fixes tag

Thank you for your patience (sorry for the delay), the new version and
the previous reviews! It looks good to me!

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

(@Mat: I can add your RvB tag as well, especially because you did the
previous reviews.)

I just applied these two patches in our tree (fixes for -net) with the 2
suggestions I mentioned in a reply to patch 2/2 (still possible to undo
them if needed).

New patches for t/upstream-net and t/upstream:
- 292454bf7376: mptcp: rename timer related helper to less confusing names
- 246aefd6e9b3: mptcp: fix dangling connection hang-up
- Results: 07a89d3454a9..c35c49bddbfc (export-net)
- Results: 07a689bb4cbf..c23e74879d43 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230909T160908
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230909T160908

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

Re: [PATCH mptcp-net v5 2/2] mptcp: fix dangling connection hang-up

[Paolo Abeni]

On Sat, 2023-09-09 at 17:50 +0200, Matthieu Baerts wrote:
> Hi Paolo,
> 
> On 06/09/2023 10:30, Paolo Abeni wrote:
> > According to RFC 8684 section 3.3:
> > 
> >   A connection is not closed unless [...] or an implementation-specific
> >   connection-level send timeout.
> > 
> > Currently the MPTCP protocol does not implement such timeout, and
> > connection timing-out at the TCP-level never move to close state.
> > 
> > Introduces a catch-up condition at subflow close time to move the
> > MPTCP socket to close, too.
> > 
> > That additionally allow removing similar existing inside the worker.
> > 
> > Finally, allow some additional timeout for plain ESTABLISHED mptcp
> > sockets, as the protocol allows creating new subflows even at that
> > point and making the connection functional again.
> > 
> > The issues is actually present since the beginning, but it basically
> > impossible to solve without a long chain of functional pre-requisites
> > topped by the blamed commit below.
> 
> Would it not be better to put another "Fixes" tag and mention that
> bbd49d114d57 is required for the backports?
> 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/430
> > Fixes: bbd49d114d57 ("mptcp: consolidate transition to TCP_CLOSE in mptcp_do_fastclose()")
> 
> My fear is that this patch will not be automatically selected to be
> backported below 6.5.
> 
> Instead, I suggest to use:
> 
> Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
> 
> The issue was certainly there before but hard to backport without that
> one anyway.

Can we keep both tags? I would like to track the dep on bbd49d114d57
explicitly.

Cheers,

Paolo

Re: [PATCH mptcp-net v5 2/2] mptcp: fix dangling connection hang-up

[Matthieu Baerts]

Hi Paolo,

On 11/09/2023 08:13, Paolo Abeni wrote:
> On Sat, 2023-09-09 at 17:50 +0200, Matthieu Baerts wrote:
>> On 06/09/2023 10:30, Paolo Abeni wrote:
>>> According to RFC 8684 section 3.3:
>>>
>>>   A connection is not closed unless [...] or an implementation-specific
>>>   connection-level send timeout.
>>>
>>> Currently the MPTCP protocol does not implement such timeout, and
>>> connection timing-out at the TCP-level never move to close state.
>>>
>>> Introduces a catch-up condition at subflow close time to move the
>>> MPTCP socket to close, too.
>>>
>>> That additionally allow removing similar existing inside the worker.
>>>
>>> Finally, allow some additional timeout for plain ESTABLISHED mptcp
>>> sockets, as the protocol allows creating new subflows even at that
>>> point and making the connection functional again.
>>>
>>> The issues is actually present since the beginning, but it basically
>>> impossible to solve without a long chain of functional pre-requisites
>>> topped by the blamed commit below.
>>
>> Would it not be better to put another "Fixes" tag and mention that
>> bbd49d114d57 is required for the backports?
>>
>>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/430
>>> Fixes: bbd49d114d57 ("mptcp: consolidate transition to TCP_CLOSE in mptcp_do_fastclose()")
>>
>> My fear is that this patch will not be automatically selected to be
>> backported below 6.5.
>>
>> Instead, I suggest to use:
>>
>> Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
>>
>> The issue was certainly there before but hard to backport without that
>> one anyway.
> 
> Can we keep both tags? I would like to track the dep on bbd49d114d57
> explicitly.

I don't know: if we put multiple "Fixes" tags, how is it interpreted?
Does it mean: we backport this patch to kernel versions having all (or
at least?) the commits mentioned in the Fixes tags?

As long as it is not confusing for the stable team :)

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

Re: [PATCH mptcp-net v5 2/2] mptcp: fix dangling connection hang-up

[Paolo Abeni]

On Mon, 2023-09-11 at 10:42 +0200, Matthieu Baerts wrote:
> On 11/09/2023 08:13, Paolo Abeni wrote:
> > Can we keep both tags? I would like to track the dep on bbd49d114d57
> > explicitly.
> 
> I don't know: if we put multiple "Fixes" tags, how is it interpreted?
> Does it mean: we backport this patch to kernel versions having all (or
> at least?) the commits mentioned in the Fixes tags?
> 
> As long as it is not confusing for the stable team :)

I also don't know how multiple tags are handled by the stable team. But
I really want 

bbd49d114d57 ("mptcp: consolidate transition to TCP_CLOSE in
mptcp_do_fastclose()")

being explicitly there in the commit message, otherwise I fear backport
will cause more bugs then the one they are supposed to fix.

Cheers,

Paolo

Re: [PATCH mptcp-net v5 2/2] mptcp: fix dangling connection hang-up

[Matthieu Baerts]

On 11/09/2023 12:12, Paolo Abeni wrote:
> On Mon, 2023-09-11 at 10:42 +0200, Matthieu Baerts wrote:
>> On 11/09/2023 08:13, Paolo Abeni wrote:
>>> Can we keep both tags? I would like to track the dep on bbd49d114d57
>>> explicitly.
>>
>> I don't know: if we put multiple "Fixes" tags, how is it interpreted?
>> Does it mean: we backport this patch to kernel versions having all (or
>> at least?) the commits mentioned in the Fixes tags?
>>
>> As long as it is not confusing for the stable team :)
> 
> I also don't know how multiple tags are handled by the stable team. But
> I really want 
> 
> bbd49d114d57 ("mptcp: consolidate transition to TCP_CLOSE in
> mptcp_do_fastclose()")
> 
> being explicitly there in the commit message, otherwise I fear backport
> will cause more bugs then the one they are supposed to fix.

When I applied the patch, I modified the end of the last paragraph:

> The issues is actually present since the beginning, but it basically
> impossible to solve without a long chain of functional pre-requisites
> topped by commit bbd49d114d57 ("mptcp: consolidate transition to
> TCP_CLOSE in mptcp_do_fastclose()").
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/430
> Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")

See: https://github.com/multipath-tcp/mptcp_net-next/commit/8bf3aa786cf6

The commit bbd49d114d57 is mentioned in the commit message but not
marked as "Fixes". I can change that if you prefer and do the follow-up
with the stable team to see where they are going to backport it.

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

Re: [PATCH mptcp-net v5 0/2] mptcp: fix another close hang-up

[Mat Martineau]

On Sat, 9 Sep 2023, Matthieu Baerts wrote:

> Hi Paolo, Mat,
>
> On 06/09/2023 10:30, Paolo Abeni wrote:
>> If the TCP subflows close silently due to timeouts, the msk socket
>> can hang-up indefinitely.
>>
>> Implement an additional close timeout triggered with the last subflow
>> close event and move the msk to TCP_CLOSE when it fires.
>>
>> Should address issues/430 and issues/431.
>>
>> v4 -> v5:
>>  - let the tout always expire for dead socks (Mat)
>>  - fix comment type (Mat)
>>  - targeting net, added fixes tag
>
> Thank you for your patience (sorry for the delay), the new version and
> the previous reviews! It looks good to me!
>
> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>
> (@Mat: I can add your RvB tag as well, especially because you did the
> previous reviews.)

Sure, series looks good to me as well:

Reviewed-by: Mat Martineau <martineau@kernel.org>

>
> I just applied these two patches in our tree (fixes for -net) with the 2
> suggestions I mentioned in a reply to patch 2/2 (still possible to undo
> them if needed).
>
> New patches for t/upstream-net and t/upstream:
> - 292454bf7376: mptcp: rename timer related helper to less confusing names
> - 246aefd6e9b3: mptcp: fix dangling connection hang-up
> - Results: 07a89d3454a9..c35c49bddbfc (export-net)
> - Results: 07a689bb4cbf..c23e74879d43 (export)
>
> Tests are now in progress:
>
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230909T160908
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230909T160908
>
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>
>

Re: [PATCH mptcp-net v5 2/2] mptcp: fix dangling connection hang-up

[Paolo Abeni]

On Mon, 2023-09-11 at 13:34 +0200, Matthieu Baerts wrote:
> On 11/09/2023 12:12, Paolo Abeni wrote:
> > On Mon, 2023-09-11 at 10:42 +0200, Matthieu Baerts wrote:
> > > On 11/09/2023 08:13, Paolo Abeni wrote:
> > > > Can we keep both tags? I would like to track the dep on bbd49d114d57
> > > > explicitly.
> > > 
> > > I don't know: if we put multiple "Fixes" tags, how is it interpreted?
> > > Does it mean: we backport this patch to kernel versions having all (or
> > > at least?) the commits mentioned in the Fixes tags?
> > > 
> > > As long as it is not confusing for the stable team :)
> > 
> > I also don't know how multiple tags are handled by the stable team. But
> > I really want 
> > 
> > bbd49d114d57 ("mptcp: consolidate transition to TCP_CLOSE in
> > mptcp_do_fastclose()")
> > 
> > being explicitly there in the commit message, otherwise I fear backport
> > will cause more bugs then the one they are supposed to fix.
> 
> When I applied the patch, I modified the end of the last paragraph:
> 
> > The issues is actually present since the beginning, but it basically
> > impossible to solve without a long chain of functional pre-requisites
> > topped by commit bbd49d114d57 ("mptcp: consolidate transition to
> > TCP_CLOSE in mptcp_do_fastclose()").
> > 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/430
> > Fixes: e16163b6e2b7 ("mptcp: refactor shutdown and close")
> 
> See: https://github.com/multipath-tcp/mptcp_net-next/commit/8bf3aa786cf6
> 
> The commit bbd49d114d57 is mentioned in the commit message but not
> marked as "Fixes". I can change that if you prefer and do the follow-up
> with the stable team to see where they are going to backport it.

The current form LGTM, thanks!

Paolo

Re: [PATCH mptcp-net v5 0/2] mptcp: fix another close hang-up

[Matthieu Baerts]

Hi Mat,

(I just noticed I forgot to press "Sent" yesterday)

On 11/09/2023 22:16, Mat Martineau wrote:
> On Sat, 9 Sep 2023, Matthieu Baerts wrote:
> 
>> Hi Paolo, Mat,
>>
>> On 06/09/2023 10:30, Paolo Abeni wrote:
>>> If the TCP subflows close silently due to timeouts, the msk socket
>>> can hang-up indefinitely.
>>>
>>> Implement an additional close timeout triggered with the last subflow
>>> close event and move the msk to TCP_CLOSE when it fires.
>>>
>>> Should address issues/430 and issues/431.
>>>
>>> v4 -> v5:
>>>  - let the tout always expire for dead socks (Mat)
>>>  - fix comment type (Mat)
>>>  - targeting net, added fixes tag
>>
>> Thank you for your patience (sorry for the delay), the new version and
>> the previous reviews! It looks good to me!
>>
>> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>
>> (@Mat: I can add your RvB tag as well, especially because you did the
>> previous reviews.)
> 
> Sure, series looks good to me as well:
> 
> Reviewed-by: Mat Martineau <martineau@kernel.org>

Thank you, done:

- 7af61f39531d: tg:msg: add Mat's RvB tag
- ca5fe234927d: tg:msg: add Mat's RvB tag

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