[PATCH mptcp-next v2 0/3] MP_FAIL echo and retrans

[PATCH mptcp-next v2 0/3] MP_FAIL echo and retrans

[Geliang Tang]

v2:
 - don't clear mp_fail_response_expect flag.
 - add a helper mp_fail_response_expect_subflow to get the subflow,
   instead of using msk->first.
 - add locks as Mat suggested.
 - add a selftest.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/261

Geliang Tang (3):
  mptcp: add MP_FAIL response support
  selftests: mptcp: check MP_FAIL response mibs
  mptcp: reset the subflow when MP_FAIL is lost

 net/mptcp/pm.c                                | 14 ++++-
 net/mptcp/protocol.c                          | 63 ++++++++++++++++++-
 net/mptcp/protocol.h                          |  2 +
 net/mptcp/subflow.c                           |  3 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 19 +++++-
 5 files changed, 94 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH mptcp-next v2 1/3] mptcp: add MP_FAIL response support

[Geliang Tang]

This patch added a new struct member mp_fail_response_expect in struct
mptcp_subflow_context to support MP_FAIL response. In the single subflow
with checksum error and contiguous data special case, a MP_FAIL sent in
response to another MP_FAIL.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c       | 9 +++++++--
 net/mptcp/protocol.h | 1 +
 net/mptcp/subflow.c  | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d0d31d5c198a..7aae8c2e845b 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -279,8 +279,13 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 
 	pr_debug("fail_seq=%llu", fail_seq);
 
-	if (!mptcp_has_another_subflow(sk) && READ_ONCE(msk->allow_infinite_fallback))
-		subflow->send_infinite_map = 1;
+	if (!mptcp_has_another_subflow(sk) && READ_ONCE(msk->allow_infinite_fallback)) {
+		if (!subflow->mp_fail_response_expect) {
+			subflow->send_mp_fail = 1;
+			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
+			subflow->send_infinite_map = 1;
+		}
+	}
 }
 
 /* path manager helpers */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index c8bada4537e2..39aa22595add 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -448,6 +448,7 @@ struct mptcp_subflow_context {
 		backup : 1,
 		send_mp_prio : 1,
 		send_mp_fail : 1,
+		mp_fail_response_expect : 1,
 		send_fastclose : 1,
 		send_infinite_map : 1,
 		rx_eof : 1,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 30ffb00661bb..63a256a94b65 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1217,6 +1217,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
 				tcp_send_active_reset(ssk, GFP_ATOMIC);
 				while ((skb = skb_peek(&ssk->sk_receive_queue)))
 					sk_eat_skb(ssk, skb);
+			} else {
+				subflow->mp_fail_response_expect = 1;
 			}
 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
 			return true;
-- 
2.34.1

[PATCH mptcp-next v2 2/3] selftests: mptcp: check MP_FAIL response mibs

[Geliang Tang]

This patch added a new argument invert for chk_fail_nr to allow it can
check the MP_FAIL Tx and Rx mibs from the opposite direction. Used it
to check the MP_FAIL response mibs.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 959e46122a84..02da0f7bd1b0 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1055,11 +1055,21 @@ chk_fail_nr()
 {
 	local fail_tx=$1
 	local fail_rx=$2
+	local ns_invert=${3:-""}
 	local count
 	local dump_stats
+	local ns_tx=$ns1
+	local ns_rx=$ns2
+	local extra_msg=""
+
+	if [[ $ns_invert = "invert" ]]; then
+		ns_tx=$ns2
+		ns_rx=$ns1
+		extra_msg="   invert"
+	fi
 
 	printf "%-${nr_blank}s %s" " " "ftx"
-	count=$(ip netns exec $ns1 nstat -as | grep MPTcpExtMPFailTx | awk '{print $2}')
+	count=$(ip netns exec $ns_tx nstat -as | grep MPTcpExtMPFailTx | awk '{print $2}')
 	[ -z "$count" ] && count=0
 	if [ "$count" != "$fail_tx" ]; then
 		echo "[fail] got $count MP_FAIL[s] TX expected $fail_tx"
@@ -1070,17 +1080,19 @@ chk_fail_nr()
 	fi
 
 	echo -n " - failrx"
-	count=$(ip netns exec $ns2 nstat -as | grep MPTcpExtMPFailRx | awk '{print $2}')
+	count=$(ip netns exec $ns_rx nstat -as | grep MPTcpExtMPFailRx | awk '{print $2}')
 	[ -z "$count" ] && count=0
 	if [ "$count" != "$fail_rx" ]; then
 		echo "[fail] got $count MP_FAIL[s] RX expected $fail_rx"
 		fail_test
 		dump_stats=1
 	else
-		echo "[ ok ]"
+		echo -n "[ ok ]"
 	fi
 
 	[ "${dump_stats}" = 1 ] && dump_stats
+
+	echo "$extra_msg"
 }
 
 chk_fclose_nr()
@@ -2672,6 +2684,7 @@ fail_tests()
 	if reset_with_fail "Infinite map" 1; then
 		run_tests $ns1 $ns2 10.0.1.1 128
 		chk_join_nr 0 0 0 +1 +0 1 0 1 "$(pedit_action_pkts)"
+		chk_fail_nr 1 1 invert
 	fi
 }
 
-- 
2.34.1

[PATCH mptcp-next v2 3/3] mptcp: reset the subflow when MP_FAIL is lost

[Geliang Tang]

This patch reuses icsk_retransmit_timer to trigger a check if we have
not received a response from the peer after sending MP_FAIL. If the
peer doesn't respond properly, reset the subflow.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c       |  5 ++++
 net/mptcp/protocol.c | 63 ++++++++++++++++++++++++++++++++++++++++++--
 net/mptcp/protocol.h |  1 +
 net/mptcp/subflow.c  |  1 +
 4 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 7aae8c2e845b..2630018786c6 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -276,6 +276,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+	struct sock *s = (struct sock *)msk;
 
 	pr_debug("fail_seq=%llu", fail_seq);
 
@@ -284,6 +285,10 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 			subflow->send_mp_fail = 1;
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
 			subflow->send_infinite_map = 1;
+		} else {
+			mptcp_data_lock(s);
+			sk_stop_timer(s, &msk->sk.icsk_retransmit_timer);
+			mptcp_data_unlock(s);
 		}
 	}
 }
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3cb975227d12..8702f7123143 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -866,6 +866,59 @@ static void mptcp_reset_timer(struct sock *sk)
 	sk_reset_timer(sk, &icsk->icsk_retransmit_timer, jiffies + tout);
 }
 
+static struct mptcp_subflow_context *
+mp_fail_response_expect_subflow(struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow, *ret = NULL;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		if (subflow->mp_fail_response_expect) {
+			ret = subflow;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static void mptcp_mp_fail_timer(struct timer_list *t)
+{
+	struct inet_connection_sock *icsk = from_timer(icsk, t,
+						       icsk_retransmit_timer);
+	struct sock *sk = &icsk->icsk_inet.sk;
+	struct mptcp_sock *msk = mptcp_sk(sk);
+	struct mptcp_subflow_context *subflow;
+
+	if (!sk || inet_sk_state_load(sk) == TCP_CLOSE)
+		return;
+
+	spin_lock_bh(&msk->pm.lock);
+	subflow = mp_fail_response_expect_subflow(msk);
+	spin_unlock_bh(&msk->pm.lock);
+	if (subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+		pr_debug("MP_FAIL is lost, reset the subflow");
+		bh_lock_sock(ssk);
+		mptcp_subflow_reset(ssk);
+		bh_unlock_sock(ssk);
+	}
+	sock_put(sk);
+}
+
+void mptcp_setup_mp_fail_timer(struct mptcp_sock *msk)
+{
+	struct sock *sk = (struct sock *)msk;
+
+	mptcp_data_lock(sk);
+	/* re-use the csk retrans timer for MP_FAIL retrans */
+	sk_stop_timer(sk, &msk->sk.icsk_retransmit_timer);
+	timer_setup(&msk->sk.icsk_retransmit_timer, mptcp_mp_fail_timer, 0);
+	__mptcp_set_timeout(sk, TCP_RTO_MAX);
+	mptcp_reset_timer(sk);
+	mptcp_data_unlock(sk);
+}
+
 bool mptcp_schedule_work(struct sock *sk)
 {
 	if (inet_sk_state_load(sk) != TCP_CLOSE &&
@@ -1428,6 +1481,7 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 	struct subflow_send_info send_info[SSK_MODE_MAX];
 	struct mptcp_subflow_context *subflow;
 	struct sock *sk = (struct sock *)msk;
+	u8 mp_fail_response_expect = 0;
 	u32 pace, burst, wmem;
 	int i, nr_active = 0;
 	struct sock *ssk;
@@ -1442,11 +1496,15 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 		return sk_stream_memory_free(msk->first) ? msk->first : NULL;
 	}
 
+	if (mp_fail_response_expect_subflow(msk))
+		mp_fail_response_expect = 1;
+
 	/* re-use last subflow, if the burst allow that */
 	if (msk->last_snd && msk->snd_burst > 0 &&
 	    sk_stream_memory_free(msk->last_snd) &&
 	    mptcp_subflow_active(mptcp_subflow_ctx(msk->last_snd))) {
-		mptcp_set_timeout(sk);
+		if (!mp_fail_response_expect)
+			mptcp_set_timeout(sk);
 		return msk->last_snd;
 	}
 
@@ -1479,7 +1537,8 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
 			send_info[subflow->backup].linger_time = linger_time;
 		}
 	}
-	__mptcp_set_timeout(sk, tout);
+	if (!mp_fail_response_expect)
+		__mptcp_set_timeout(sk, tout);
 
 	/* pick the best backup if no other subflow is active */
 	if (!nr_active)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 39aa22595add..313c1a6c616f 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -669,6 +669,7 @@ 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);
 bool mptcp_schedule_work(struct sock *sk);
+void mptcp_setup_mp_fail_timer(struct mptcp_sock *msk);
 int mptcp_setsockopt(struct sock *sk, int level, int optname,
 		     sockptr_t optval, unsigned int optlen);
 int mptcp_getsockopt(struct sock *sk, int level, int optname,
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 63a256a94b65..960b161b1cb2 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1219,6 +1219,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 					sk_eat_skb(ssk, skb);
 			} else {
 				subflow->mp_fail_response_expect = 1;
+				mptcp_setup_mp_fail_timer(msk);
 			}
 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
 			return true;
-- 
2.34.1

Re: mptcp: reset the subflow when MP_FAIL is lost: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/5236831624101888
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5236831624101888/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6362731530944512
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6362731530944512/summary/summary.txt

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

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-next v2 3/3] mptcp: reset the subflow when MP_FAIL is lost

[Mat Martineau]

On Mon, 7 Mar 2022, Geliang Tang wrote:

> This patch reuses icsk_retransmit_timer to trigger a check if we have
> not received a response from the peer after sending MP_FAIL. If the
> peer doesn't respond properly, reset the subflow.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/pm.c       |  5 ++++
> net/mptcp/protocol.c | 63 ++++++++++++++++++++++++++++++++++++++++++--
> net/mptcp/protocol.h |  1 +
> net/mptcp/subflow.c  |  1 +
> 4 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 7aae8c2e845b..2630018786c6 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -276,6 +276,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> +	struct sock *s = (struct sock *)msk;
>
> 	pr_debug("fail_seq=%llu", fail_seq);
>
> @@ -284,6 +285,10 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> 			subflow->send_mp_fail = 1;
> 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
> 			subflow->send_infinite_map = 1;
> +		} else {
> +			mptcp_data_lock(s);
> +			sk_stop_timer(s, &msk->sk.icsk_retransmit_timer);
> +			mptcp_data_unlock(s);
> 		}
> 	}
> }
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3cb975227d12..8702f7123143 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -866,6 +866,59 @@ static void mptcp_reset_timer(struct sock *sk)
> 	sk_reset_timer(sk, &icsk->icsk_retransmit_timer, jiffies + tout);
> }
>
> +static struct mptcp_subflow_context *
> +mp_fail_response_expect_subflow(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow, *ret = NULL;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		if (subflow->mp_fail_response_expect) {
> +			ret = subflow;
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void mptcp_mp_fail_timer(struct timer_list *t)
> +{
> +	struct inet_connection_sock *icsk = from_timer(icsk, t,
> +						       icsk_retransmit_timer);
> +	struct sock *sk = &icsk->icsk_inet.sk;
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct mptcp_subflow_context *subflow;
> +
> +	if (!sk || inet_sk_state_load(sk) == TCP_CLOSE)
> +		return;

Is it possible for sk to be NULL here? mptcp_timeout_timer() and 
mptcp_retransmit_timer() don't check for NULL.

Also, early return leaks a refcount on the sk (a sock_put() is always 
needed).

> +
> +	spin_lock_bh(&msk->pm.lock);
> +	subflow = mp_fail_response_expect_subflow(msk);
> +	spin_unlock_bh(&msk->pm.lock);

msk->conn_list is protected by the msk lock, so the pm.lock isn't going to 
protect it here.

I think if you used msk->sk_timer and a msk->flags bit, the existing 
mptcp_timeout_timer() handler could be used and some new code in 
mptcp_worker() could handle resetting the subflow. This would avoid a lot 
of locking here in the timer handler.

The timing for a missing MP_FAIL is not strict, so it doesn't seem 
important to handle it all in a low-level timer callback.


> +	if (subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		pr_debug("MP_FAIL is lost, reset the subflow");
> +		bh_lock_sock(ssk);
> +		mptcp_subflow_reset(ssk);
> +		bh_unlock_sock(ssk);
> +	}
> +	sock_put(sk);
> +}
> +
> +void mptcp_setup_mp_fail_timer(struct mptcp_sock *msk)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +
> +	mptcp_data_lock(sk);
> +	/* re-use the csk retrans timer for MP_FAIL retrans */
> +	sk_stop_timer(sk, &msk->sk.icsk_retransmit_timer);
> +	timer_setup(&msk->sk.icsk_retransmit_timer, mptcp_mp_fail_timer, 0);
> +	__mptcp_set_timeout(sk, TCP_RTO_MAX);
> +	mptcp_reset_timer(sk);
> +	mptcp_data_unlock(sk);
> +}
> +
> bool mptcp_schedule_work(struct sock *sk)
> {
> 	if (inet_sk_state_load(sk) != TCP_CLOSE &&
> @@ -1428,6 +1481,7 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> 	struct subflow_send_info send_info[SSK_MODE_MAX];
> 	struct mptcp_subflow_context *subflow;
> 	struct sock *sk = (struct sock *)msk;
> +	u8 mp_fail_response_expect = 0;
> 	u32 pace, burst, wmem;
> 	int i, nr_active = 0;
> 	struct sock *ssk;
> @@ -1442,11 +1496,15 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> 		return sk_stream_memory_free(msk->first) ? msk->first : NULL;
> 	}
>
> +	if (mp_fail_response_expect_subflow(msk))
> +		mp_fail_response_expect = 1;

Using sk_timer as I mention above will also make it unnecessary to add 
this check and all the mp_fail_response_expect logic to this function.

- Mat

> +
> 	/* re-use last subflow, if the burst allow that */
> 	if (msk->last_snd && msk->snd_burst > 0 &&
> 	    sk_stream_memory_free(msk->last_snd) &&
> 	    mptcp_subflow_active(mptcp_subflow_ctx(msk->last_snd))) {
> -		mptcp_set_timeout(sk);
> +		if (!mp_fail_response_expect)
> +			mptcp_set_timeout(sk);
> 		return msk->last_snd;
> 	}
>
> @@ -1479,7 +1537,8 @@ static struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
> 			send_info[subflow->backup].linger_time = linger_time;
> 		}
> 	}
> -	__mptcp_set_timeout(sk, tout);
> +	if (!mp_fail_response_expect)
> +		__mptcp_set_timeout(sk, tout);
>
> 	/* pick the best backup if no other subflow is active */
> 	if (!nr_active)
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 39aa22595add..313c1a6c616f 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -669,6 +669,7 @@ 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);
> bool mptcp_schedule_work(struct sock *sk);
> +void mptcp_setup_mp_fail_timer(struct mptcp_sock *msk);
> int mptcp_setsockopt(struct sock *sk, int level, int optname,
> 		     sockptr_t optval, unsigned int optlen);
> int mptcp_getsockopt(struct sock *sk, int level, int optname,
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 63a256a94b65..960b161b1cb2 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1219,6 +1219,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
> 					sk_eat_skb(ssk, skb);
> 			} else {
> 				subflow->mp_fail_response_expect = 1;
> +				mptcp_setup_mp_fail_timer(msk);
> 			}
> 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
> 			return true;
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2 3/3] mptcp: reset the subflow when MP_FAIL is lost

[Mat Martineau]

On Tue, 8 Mar 2022, Mat Martineau wrote:

> On Mon, 7 Mar 2022, Geliang Tang wrote:
>
>> This patch reuses icsk_retransmit_timer to trigger a check if we have
>> not received a response from the peer after sending MP_FAIL. If the
>> peer doesn't respond properly, reset the subflow.
>> 
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>> ---
>> net/mptcp/pm.c       |  5 ++++
>> net/mptcp/protocol.c | 63 ++++++++++++++++++++++++++++++++++++++++++--
>> net/mptcp/protocol.h |  1 +
>> net/mptcp/subflow.c  |  1 +
>> 4 files changed, 68 insertions(+), 2 deletions(-)
>> 
>> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
>> index 7aae8c2e845b..2630018786c6 100644
>> --- a/net/mptcp/pm.c
>> +++ b/net/mptcp/pm.c
>> @@ -276,6 +276,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 
>> fail_seq)
>> {
>> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>> 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
>> +	struct sock *s = (struct sock *)msk;
>>
>> 	pr_debug("fail_seq=%llu", fail_seq);
>> 
>> @@ -284,6 +285,10 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 
>> fail_seq)
>> 			subflow->send_mp_fail = 1;
>> 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
>> 			subflow->send_infinite_map = 1;
>> +		} else {
>> +			mptcp_data_lock(s);
>> +			sk_stop_timer(s, &msk->sk.icsk_retransmit_timer);
>> +			mptcp_data_unlock(s);
>> 		}
>> 	}
>> }
>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>> index 3cb975227d12..8702f7123143 100644
>> --- a/net/mptcp/protocol.c
>> +++ b/net/mptcp/protocol.c
>> @@ -866,6 +866,59 @@ static void mptcp_reset_timer(struct sock *sk)
>> 	sk_reset_timer(sk, &icsk->icsk_retransmit_timer, jiffies + tout);
>> }
>> 
>> +static struct mptcp_subflow_context *
>> +mp_fail_response_expect_subflow(struct mptcp_sock *msk)
>> +{
>> +	struct mptcp_subflow_context *subflow, *ret = NULL;
>> +
>> +	mptcp_for_each_subflow(msk, subflow) {
>> +		if (subflow->mp_fail_response_expect) {

One more thing: if this value is read without the subflow lock held, it 
would be better to use READ_ONCE/WRITE_ONCE. That would also require it to 
be a non-bitfield struct member.

- Mat

>> +			ret = subflow;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void mptcp_mp_fail_timer(struct timer_list *t)
>> +{
>> +	struct inet_connection_sock *icsk = from_timer(icsk, t,
>> + 
>> icsk_retransmit_timer);
>> +	struct sock *sk = &icsk->icsk_inet.sk;
>> +	struct mptcp_sock *msk = mptcp_sk(sk);
>> +	struct mptcp_subflow_context *subflow;
>> +
>> +	if (!sk || inet_sk_state_load(sk) == TCP_CLOSE)
>> +		return;
>
> Is it possible for sk to be NULL here? mptcp_timeout_timer() and 
> mptcp_retransmit_timer() don't check for NULL.
>
> Also, early return leaks a refcount on the sk (a sock_put() is always 
> needed).
>
>> +
>> +	spin_lock_bh(&msk->pm.lock);
>> +	subflow = mp_fail_response_expect_subflow(msk);
>> +	spin_unlock_bh(&msk->pm.lock);
>
> msk->conn_list is protected by the msk lock, so the pm.lock isn't going to 
> protect it here.
>
> I think if you used msk->sk_timer and a msk->flags bit, the existing 
> mptcp_timeout_timer() handler could be used and some new code in 
> mptcp_worker() could handle resetting the subflow. This would avoid a lot of 
> locking here in the timer handler.
>
> The timing for a missing MP_FAIL is not strict, so it doesn't seem important 
> to handle it all in a low-level timer callback.
>
>
>> +	if (subflow) {
>> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
>> +
>> +		pr_debug("MP_FAIL is lost, reset the subflow");
>> +		bh_lock_sock(ssk);
>> +		mptcp_subflow_reset(ssk);
>> +		bh_unlock_sock(ssk);
>> +	}
>> +	sock_put(sk);
>> +}
>> +
>> +void mptcp_setup_mp_fail_timer(struct mptcp_sock *msk)
>> +{
>> +	struct sock *sk = (struct sock *)msk;
>> +
>> +	mptcp_data_lock(sk);
>> +	/* re-use the csk retrans timer for MP_FAIL retrans */
>> +	sk_stop_timer(sk, &msk->sk.icsk_retransmit_timer);
>> +	timer_setup(&msk->sk.icsk_retransmit_timer, mptcp_mp_fail_timer, 0);
>> +	__mptcp_set_timeout(sk, TCP_RTO_MAX);
>> +	mptcp_reset_timer(sk);
>> +	mptcp_data_unlock(sk);
>> +}
>> +
>> bool mptcp_schedule_work(struct sock *sk)
>> {
>> 	if (inet_sk_state_load(sk) != TCP_CLOSE &&
>> @@ -1428,6 +1481,7 @@ static struct sock *mptcp_subflow_get_send(struct 
>> mptcp_sock *msk)
>> 	struct subflow_send_info send_info[SSK_MODE_MAX];
>> 	struct mptcp_subflow_context *subflow;
>> 	struct sock *sk = (struct sock *)msk;
>> +	u8 mp_fail_response_expect = 0;
>> 	u32 pace, burst, wmem;
>> 	int i, nr_active = 0;
>> 	struct sock *ssk;
>> @@ -1442,11 +1496,15 @@ static struct sock *mptcp_subflow_get_send(struct 
>> mptcp_sock *msk)
>> 		return sk_stream_memory_free(msk->first) ? msk->first : NULL;
>> 	}
>> 
>> +	if (mp_fail_response_expect_subflow(msk))
>> +		mp_fail_response_expect = 1;
>
> Using sk_timer as I mention above will also make it unnecessary to add this 
> check and all the mp_fail_response_expect logic to this function.
>
> - Mat
>
>> +
>> 	/* re-use last subflow, if the burst allow that */
>> 	if (msk->last_snd && msk->snd_burst > 0 &&
>> 	    sk_stream_memory_free(msk->last_snd) &&
>> 	    mptcp_subflow_active(mptcp_subflow_ctx(msk->last_snd))) {
>> -		mptcp_set_timeout(sk);
>> +		if (!mp_fail_response_expect)
>> +			mptcp_set_timeout(sk);
>> 		return msk->last_snd;
>> 	}
>> 
>> @@ -1479,7 +1537,8 @@ static struct sock *mptcp_subflow_get_send(struct 
>> mptcp_sock *msk)
>> 			send_info[subflow->backup].linger_time = linger_time;
>> 		}
>> 	}
>> -	__mptcp_set_timeout(sk, tout);
>> +	if (!mp_fail_response_expect)
>> +		__mptcp_set_timeout(sk, tout);
>>
>> 	/* pick the best backup if no other subflow is active */
>> 	if (!nr_active)
>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
>> index 39aa22595add..313c1a6c616f 100644
>> --- a/net/mptcp/protocol.h
>> +++ b/net/mptcp/protocol.h
>> @@ -669,6 +669,7 @@ 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);
>> bool mptcp_schedule_work(struct sock *sk);
>> +void mptcp_setup_mp_fail_timer(struct mptcp_sock *msk);
>> int mptcp_setsockopt(struct sock *sk, int level, int optname,
>> 		     sockptr_t optval, unsigned int optlen);
>> int mptcp_getsockopt(struct sock *sk, int level, int optname,
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 63a256a94b65..960b161b1cb2 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1219,6 +1219,7 @@ static bool subflow_check_data_avail(struct sock 
>> *ssk)
>> 					sk_eat_skb(ssk, skb);
>> 			} else {
>> 				subflow->mp_fail_response_expect = 1;
>> +				mptcp_setup_mp_fail_timer(msk);
>> 			}
>> 			WRITE_ONCE(subflow->data_avail, 
>> MPTCP_SUBFLOW_NODATA);
>> 			return true;
>> -- 
>> 2.34.1
>> 
>> 
>> 
>
> --
> Mat Martineau
> Intel
>
>

--
Mat Martineau
Intel

Re: mptcp: reset the subflow when MP_FAIL is lost: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5749632264306688
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5749632264306688/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 1 failed test(s): selftest_diag 🔴:
  - Task: https://cirrus-ci.com/task/4623732357464064
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4623732357464064/summary/summary.txt

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

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: mptcp: reset the subflow when MP_FAIL is lost: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- {"code":404,"message":
  - "HTTP 404 Not Found"}:
  - Task: https://cirrus-ci.com/task/6506211624353792
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6506211624353792/summary/summary.txt

- {"code":404,"message":
  - "HTTP 404 Not Found"}:
  - Task: https://cirrus-ci.com/task/5189979209990144
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5189979209990144/summary/summary.txt

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

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)