All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH mptcp-next v3 0/3] MP_FAIL echo and retrans
@ 2022-03-09 12:33 Geliang Tang
  2022-03-09 12:33 ` [PATCH mptcp-next v3 1/3] mptcp: add MP_FAIL response support Geliang Tang
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Geliang Tang @ 2022-03-09 12:33 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

v3:
 - use msk->sk_timer and a msk->flags bit.
 - use READ_ONCE/WRITE_ONCE for subflow->mp_fail_response_expect.
 - update selftest.

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
  mptcp: reset subflow when MP_FAIL doesn't respond
  selftests: mptcp: check MP_FAIL response mibs

 net/mptcp/pm.c                                | 11 ++++--
 net/mptcp/protocol.c                          | 21 +++++++++++
 net/mptcp/protocol.h                          |  2 ++
 net/mptcp/subflow.c                           |  4 +++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 36 ++++++++++++++++---
 5 files changed, 67 insertions(+), 7 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH mptcp-next v3 1/3] mptcp: add MP_FAIL response support
  2022-03-09 12:33 [PATCH mptcp-next v3 0/3] MP_FAIL echo and retrans Geliang Tang
@ 2022-03-09 12:33 ` Geliang Tang
  2022-03-09 12:33 ` [PATCH mptcp-next v3 2/3] mptcp: reset subflow when MP_FAIL doesn't respond Geliang Tang
  2022-03-09 12:33 ` [PATCH mptcp-next v3 3/3] selftests: mptcp: check MP_FAIL response mibs Geliang Tang
  2 siblings, 0 replies; 6+ messages in thread
From: Geliang Tang @ 2022-03-09 12:33 UTC (permalink / raw)
  To: mptcp; +Cc: 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..c4622b07b7f5 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 (!READ_ONCE(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..83f0205f0d95 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -456,6 +456,7 @@ struct mptcp_subflow_context {
 		stale : 1,	    /* unable to snd/rcv data, do not use for xmit */
 		local_id_valid : 1; /* local_id is correctly initialized */
 	enum mptcp_data_avail data_avail;
+	bool	mp_fail_response_expect;
 	u32	remote_nonce;
 	u64	thmac;
 	u32	local_nonce;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 30ffb00661bb..ca2352ad20d4 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 {
+				WRITE_ONCE(subflow->mp_fail_response_expect, true);
 			}
 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
 			return true;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH mptcp-next v3 2/3] mptcp: reset subflow when MP_FAIL doesn't respond
  2022-03-09 12:33 [PATCH mptcp-next v3 0/3] MP_FAIL echo and retrans Geliang Tang
  2022-03-09 12:33 ` [PATCH mptcp-next v3 1/3] mptcp: add MP_FAIL response support Geliang Tang
@ 2022-03-09 12:33 ` Geliang Tang
  2022-03-10  0:01   ` Mat Martineau
  2022-03-09 12:33 ` [PATCH mptcp-next v3 3/3] selftests: mptcp: check MP_FAIL response mibs Geliang Tang
  2 siblings, 1 reply; 6+ messages in thread
From: Geliang Tang @ 2022-03-09 12:33 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

This patch added a new msk->flags bit MPTCP_FAIL_NO_RESPONSE, then reused
sk_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       |  2 ++
 net/mptcp/protocol.c | 21 +++++++++++++++++++++
 net/mptcp/protocol.h |  1 +
 net/mptcp/subflow.c  |  2 ++
 4 files changed, 26 insertions(+)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index c4622b07b7f5..c41aca97ba36 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -284,6 +284,8 @@ 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 {
+			clear_bit(MPTCP_FAIL_NO_RESPONSE, &msk->flags);
 		}
 	}
 }
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3cb975227d12..6ff7e6eb44ee 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2494,6 +2494,24 @@ static void __mptcp_retrans(struct sock *sk)
 		mptcp_reset_timer(sk);
 }
 
+static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		if (READ_ONCE(subflow->mp_fail_response_expect)) {
+			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+			bool slow;
+
+			pr_debug("MP_FAIL doesn't respond, reset the subflow");
+			slow = lock_sock_fast(ssk);
+			mptcp_subflow_reset(ssk);
+			unlock_sock_fast(ssk, slow);
+			break;
+		}
+	}
+}
+
 static void mptcp_worker(struct work_struct *work)
 {
 	struct mptcp_sock *msk = container_of(work, struct mptcp_sock, work);
@@ -2534,6 +2552,9 @@ static void mptcp_worker(struct work_struct *work)
 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
 		__mptcp_retrans(sk);
 
+	if (test_and_clear_bit(MPTCP_FAIL_NO_RESPONSE, &msk->flags))
+		mptcp_mp_fail_no_response(msk);
+
 unlock:
 	release_sock(sk);
 	sock_put(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 83f0205f0d95..bf58d3c886f5 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -116,6 +116,7 @@
 #define MPTCP_WORK_EOF		3
 #define MPTCP_FALLBACK_DONE	4
 #define MPTCP_WORK_CLOSE_SUBFLOW 5
+#define MPTCP_FAIL_NO_RESPONSE	6
 
 /* MPTCP socket release cb flags */
 #define MPTCP_PUSH_PENDING	1
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index ca2352ad20d4..5a7aa6e37ca6 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1009,6 +1009,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 		pr_debug("infinite mapping received");
 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
 		subflow->map_data_len = 0;
+		clear_bit(MPTCP_FAIL_NO_RESPONSE, &msk->flags);
 		return MAPPING_INVALID;
 	}
 
@@ -1219,6 +1220,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 					sk_eat_skb(ssk, skb);
 			} else {
 				WRITE_ONCE(subflow->mp_fail_response_expect, true);
+				set_bit(MPTCP_FAIL_NO_RESPONSE, &msk->flags);
 			}
 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
 			return true;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH mptcp-next v3 3/3] selftests: mptcp: check MP_FAIL response mibs
  2022-03-09 12:33 [PATCH mptcp-next v3 0/3] MP_FAIL echo and retrans Geliang Tang
  2022-03-09 12:33 ` [PATCH mptcp-next v3 1/3] mptcp: add MP_FAIL response support Geliang Tang
  2022-03-09 12:33 ` [PATCH mptcp-next v3 2/3] mptcp: reset subflow when MP_FAIL doesn't respond Geliang Tang
@ 2022-03-09 12:33 ` Geliang Tang
  2022-03-09 14:30   ` selftests: mptcp: check MP_FAIL response mibs: Tests Results MPTCP CI
  2 siblings, 1 reply; 6+ messages in thread
From: Geliang Tang @ 2022-03-09 12:33 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

This patch extended chk_fail_nr to check the MP_FAIL response mibs.

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.

When the infinite map was received before the MP_FAIL response, the
response will be lost. A '-' can be added into fail_tx or fail_rx to
represent that MP_FAIL response Tx or Rx can be lost when doing the
checks.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 959e46122a84..294396225f31 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1055,13 +1055,35 @@ 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=""
+	local allow_tx_lost=0
+	local allow_rx_lost=0
+
+	if [[ $ns_invert = "invert" ]]; then
+		ns_tx=$ns2
+		ns_rx=$ns1
+		extra_msg="   invert"
+	fi
+
+	if [[ "${fail_tx}" = "-"* ]]; then
+		allow_tx_lost=1
+		fail_tx=${fail_tx:1}
+	fi
+	if [[ "${fail_rx}" = "-"* ]]; then
+		allow_rx_lost=1
+		fail_rx=${fail_rx:1}
+	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
+	if { [ "$count" != "$fail_tx" ] && [ $allow_tx_lost -eq 0 ]; } ||
+	   { [ "$count" -gt "$fail_tx" ] && [ $allow_tx_lost -eq 1 ]; }; then
 		echo "[fail] got $count MP_FAIL[s] TX expected $fail_tx"
 		fail_test
 		dump_stats=1
@@ -1070,17 +1092,20 @@ 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
+	if { [ "$count" != "$fail_rx" ] && [ $allow_rx_lost -eq 0 ]; } ||
+	   { [ "$count" -gt "$fail_rx" ] && [ $allow_rx_lost -eq 1 ]; }; 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 +2697,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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: selftests: mptcp: check MP_FAIL response mibs: Tests Results
  2022-03-09 12:33 ` [PATCH mptcp-next v3 3/3] selftests: mptcp: check MP_FAIL response mibs Geliang Tang
@ 2022-03-09 14:30   ` MPTCP CI
  0 siblings, 0 replies; 6+ messages in thread
From: MPTCP CI @ 2022-03-09 14:30 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp

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/5689563053031424
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5689563053031424/summary/summary.txt

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

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

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)

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH mptcp-next v3 2/3] mptcp: reset subflow when MP_FAIL doesn't respond
  2022-03-09 12:33 ` [PATCH mptcp-next v3 2/3] mptcp: reset subflow when MP_FAIL doesn't respond Geliang Tang
@ 2022-03-10  0:01   ` Mat Martineau
  0 siblings, 0 replies; 6+ messages in thread
From: Mat Martineau @ 2022-03-10  0:01 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp

On Wed, 9 Mar 2022, Geliang Tang wrote:

> This patch added a new msk->flags bit MPTCP_FAIL_NO_RESPONSE, then reused
> sk_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.
>

I don't see the sk_timer use?

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/pm.c       |  2 ++
> net/mptcp/protocol.c | 21 +++++++++++++++++++++
> net/mptcp/protocol.h |  1 +
> net/mptcp/subflow.c  |  2 ++
> 4 files changed, 26 insertions(+)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index c4622b07b7f5..c41aca97ba36 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -284,6 +284,8 @@ 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 {
> +			clear_bit(MPTCP_FAIL_NO_RESPONSE, &msk->flags);

Here I think you'd clear the sk_timer (if socket not closed).

> 		}
> 	}
> }
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3cb975227d12..6ff7e6eb44ee 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2494,6 +2494,24 @@ static void __mptcp_retrans(struct sock *sk)
> 		mptcp_reset_timer(sk);
> }
>
> +static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		if (READ_ONCE(subflow->mp_fail_response_expect)) {
> +			struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +			bool slow;
> +
> +			pr_debug("MP_FAIL doesn't respond, reset the subflow");
> +			slow = lock_sock_fast(ssk);
> +			mptcp_subflow_reset(ssk);
> +			unlock_sock_fast(ssk, slow);
> +			break;
> +		}
> +	}
> +}
> +
> static void mptcp_worker(struct work_struct *work)
> {
> 	struct mptcp_sock *msk = container_of(work, struct mptcp_sock, work);
> @@ -2534,6 +2552,9 @@ static void mptcp_worker(struct work_struct *work)
> 	if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
> 		__mptcp_retrans(sk);
>
> +	if (test_and_clear_bit(MPTCP_FAIL_NO_RESPONSE, &msk->flags))
> +		mptcp_mp_fail_no_response(msk);
> +

Without the other suggested changes, this would reset the subflow if the 
worker runs for any reason (and at any time) after MP_FAIL is sent.

> unlock:
> 	release_sock(sk);
> 	sock_put(sk);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 83f0205f0d95..bf58d3c886f5 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -116,6 +116,7 @@
> #define MPTCP_WORK_EOF		3
> #define MPTCP_FALLBACK_DONE	4
> #define MPTCP_WORK_CLOSE_SUBFLOW 5
> +#define MPTCP_FAIL_NO_RESPONSE	6
>
> /* MPTCP socket release cb flags */
> #define MPTCP_PUSH_PENDING	1
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index ca2352ad20d4..5a7aa6e37ca6 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1009,6 +1009,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
> 		pr_debug("infinite mapping received");
> 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
> 		subflow->map_data_len = 0;
> +		clear_bit(MPTCP_FAIL_NO_RESPONSE, &msk->flags);

Another spot to clear the timer rather than this flag.

> 		return MAPPING_INVALID;
> 	}
>
> @@ -1219,6 +1220,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
> 					sk_eat_skb(ssk, skb);
> 			} else {
> 				WRITE_ONCE(subflow->mp_fail_response_expect, true);
> +				set_bit(MPTCP_FAIL_NO_RESPONSE, &msk->flags);

Here, start the timer. In the timer handler, set MPTCP_FAIL_NO_RESPONSE 
flag (with required locking) if it was an MP_FAIL timeout.

> 			}
> 			WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
> 			return true;
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-10  0:01 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-09 12:33 [PATCH mptcp-next v3 0/3] MP_FAIL echo and retrans Geliang Tang
2022-03-09 12:33 ` [PATCH mptcp-next v3 1/3] mptcp: add MP_FAIL response support Geliang Tang
2022-03-09 12:33 ` [PATCH mptcp-next v3 2/3] mptcp: reset subflow when MP_FAIL doesn't respond Geliang Tang
2022-03-10  0:01   ` Mat Martineau
2022-03-09 12:33 ` [PATCH mptcp-next v3 3/3] selftests: mptcp: check MP_FAIL response mibs Geliang Tang
2022-03-09 14:30   ` selftests: mptcp: check MP_FAIL response mibs: Tests Results MPTCP CI

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.