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

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

[Geliang Tang]

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

Geliang Tang (2):
  mptcp: add MP_FAIL response support
  mptcp: reset the subflow when MP_FAIL is lost

 net/mptcp/pm.c       | 12 ++++++++++--
 net/mptcp/protocol.c | 32 +++++++++++++++++++++++++++++++-
 net/mptcp/protocol.h |  2 ++
 net/mptcp/subflow.c  |  3 +++
 4 files changed, 46 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH mptcp-next 1/2] 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       | 11 +++++++++--
 net/mptcp/protocol.h |  1 +
 net/mptcp/subflow.c  |  2 ++
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d0d31d5c198a..c9080230b800 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -279,8 +279,15 @@ 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;
+		} else {
+			subflow->mp_fail_response_expect = 0;
+		}
+	}
 }
 
 /* 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

Re: [PATCH mptcp-next 1/2] mptcp: add MP_FAIL response support

[Mat Martineau]

On Thu, 3 Mar 2022, Geliang Tang wrote:

> 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       | 11 +++++++++--
> net/mptcp/protocol.h |  1 +
> net/mptcp/subflow.c  |  2 ++
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d0d31d5c198a..c9080230b800 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -279,8 +279,15 @@ 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;
> +		} else {
> +			subflow->mp_fail_response_expect = 0;

I don't think there's any need to clear this flag. Once the MP_FAIL 
response is received, this side is supposed to fall back and another 
incoming MP_FAIL would be strange to see. But if one somehow propagated 
here, it would be better to "expect" it by having the flag still set so no 
more MP_FAILs are sent in response. I mentioned this before in my RFCv2 
review - if you think this line of code should remain, please explain.

Thanks!

-Mat

> +		}
> +	}
> }
>
> /* 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
>
>
>

--
Mat Martineau
Intel

[PATCH mptcp-next 2/2] 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       |  1 +
 net/mptcp/protocol.c | 32 +++++++++++++++++++++++++++++++-
 net/mptcp/protocol.h |  1 +
 net/mptcp/subflow.c  |  1 +
 4 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index c9080230b800..441f5c79b601 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -285,6 +285,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
 			subflow->send_infinite_map = 1;
 		} else {
+			sk_stop_timer((struct sock *)msk, &msk->sk.icsk_retransmit_timer);
 			subflow->mp_fail_response_expect = 0;
 		}
 	}
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 3cb975227d12..59d2deec1ae8 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -866,6 +866,36 @@ static void mptcp_reset_timer(struct sock *sk)
 	sk_reset_timer(sk, &icsk->icsk_retransmit_timer, jiffies + tout);
 }
 
+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, *ssk;
+	struct mptcp_subflow_context *subflow;
+
+	if (!sk || inet_sk_state_load(sk) == TCP_CLOSE)
+		return;
+
+	bh_lock_sock(sk);
+	subflow = mptcp_subflow_ctx(mptcp_sk(sk)->first);
+	ssk = mptcp_subflow_tcp_sock(subflow);
+	pr_debug("MP_FAIL is lost, reset the subflow");
+	mptcp_subflow_reset(ssk);
+	bh_unlock_sock(sk);
+	sock_put(sk);
+}
+
+void mptcp_setup_mp_fail_timer(struct mptcp_sock *msk)
+{
+	struct sock *sk = (struct sock *)msk;
+
+	/* 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);
+}
+
 bool mptcp_schedule_work(struct sock *sk)
 {
 	if (inet_sk_state_load(sk) != TCP_CLOSE &&
@@ -1604,7 +1634,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
 
 out:
 	/* ensure the rtx timer is running */
-	if (!mptcp_timer_pending(sk))
+	if (!mptcp_timer_pending(sk) && !__mptcp_check_fallback(msk))
 		mptcp_reset_timer(sk);
 	if (copied)
 		__mptcp_check_send_data_fin(sk);
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:

- {"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)

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

[Mat Martineau]

On Thu, 3 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       |  1 +
> net/mptcp/protocol.c | 32 +++++++++++++++++++++++++++++++-
> net/mptcp/protocol.h |  1 +
> net/mptcp/subflow.c  |  1 +
> 4 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index c9080230b800..441f5c79b601 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -285,6 +285,7 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
> 			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
> 			subflow->send_infinite_map = 1;
> 		} else {
> +			sk_stop_timer((struct sock *)msk, &msk->sk.icsk_retransmit_timer);

mptcp_data_lock() needs to be held when manipulating the msk 
retransmit_timer.

> 			subflow->mp_fail_response_expect = 0;
> 		}
> 	}
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3cb975227d12..59d2deec1ae8 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -866,6 +866,36 @@ static void mptcp_reset_timer(struct sock *sk)
> 	sk_reset_timer(sk, &icsk->icsk_retransmit_timer, jiffies + tout);
> }
>
> +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, *ssk;
> +	struct mptcp_subflow_context *subflow;
> +
> +	if (!sk || inet_sk_state_load(sk) == TCP_CLOSE)
> +		return;
> +
> +	bh_lock_sock(sk);
> +	subflow = mptcp_subflow_ctx(mptcp_sk(sk)->first);

Right now, the code only attempts an MP_FAIL fallback if there have never 
been any other subflows addes, so the msk->first assumption seems to work. 
This is fragile if other code changes to support fallback in other 
scenarios (as allowed by the RFC). Would be better to find the subflow 
with mp_fail_response_expect set and shut that one down.

> +	ssk = mptcp_subflow_tcp_sock(subflow);
> +	pr_debug("MP_FAIL is lost, reset the subflow");
> +	mptcp_subflow_reset(ssk);

Need to handle ssk locking too.

-Mat

> +	bh_unlock_sock(sk);
> +	sock_put(sk);
> +}
> +
> +void mptcp_setup_mp_fail_timer(struct mptcp_sock *msk)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +
> +	/* 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);
> +}
> +
> bool mptcp_schedule_work(struct sock *sk)
> {
> 	if (inet_sk_state_load(sk) != TCP_CLOSE &&
> @@ -1604,7 +1634,7 @@ void __mptcp_push_pending(struct sock *sk, unsigned int flags)
>
> out:
> 	/* ensure the rtx timer is running */
> -	if (!mptcp_timer_pending(sk))
> +	if (!mptcp_timer_pending(sk) && !__mptcp_check_fallback(msk))
> 		mptcp_reset_timer(sk);
> 	if (copied)
> 		__mptcp_check_send_data_fin(sk);
> 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: 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)