[PATCH mptcp-net v2] mptcp: Do TCP fallback on early DSS checksum failure

Mat Martineau posted 1 patch 1 year, 10 months ago
Failed in applying to current master (apply log)
net/mptcp/protocol.h |  3 ++-
net/mptcp/subflow.c  | 21 ++++++++++++++++++---
2 files changed, 20 insertions(+), 4 deletions(-)
[PATCH mptcp-net v2] mptcp: Do TCP fallback on early DSS checksum failure
Posted by Mat Martineau 1 year, 10 months ago
RFC 8684 section 3.7 describes several opportunities for a MPTCP
connection to "fall back" to regular TCP early in the connection
process, before it has been confirmed that MPTCP options can be
successfully propagated on all SYN, SYN/ACK, and data packets. If a peer
acknowledges the first received data packet with a regular TCP header
(no MPTCP options), fallback is allowed.

If the recipient of that first data packet finds a MPTCP DSS checksum
error, this provides an opportunity to fail gracefully with a TCP
fallback rather than resetting the connection (as might happen if a
checksum failure were detected later).

This commit modifies the checksum failure code to attempt fallback on
the initial subflow of a MPTCP connection, only if it's a failure in the
first data mapping. In cases where the peer initiates the connection,
requests checksums, is the first to send data, and the peer is sending
incorrect checksums (see
https://github.com/multipath-tcp/mptcp_net-next/issues/275), this allows
the connection to proceed as TCP rather than reset.

v2: add a helper function for checking whether a subflow is
fallback-capable.

Acked-by: Paolo Abeni <pabeni@redhat.com>
Fixes: dd8bcd1768ff ("mptcp: validate the data checksum")
Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---
 net/mptcp/protocol.h |  3 ++-
 net/mptcp/subflow.c  | 21 ++++++++++++++++++---
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index f4ce28bb0fdc..6c7e78ec88e8 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -443,7 +443,8 @@ struct mptcp_subflow_context {
 		can_ack : 1,        /* only after processing the remote a key */
 		disposable : 1,	    /* ctx can be free at ulp release time */
 		stale : 1,	    /* unable to snd/rcv data, do not use for xmit */
-		local_id_valid : 1; /* local_id is correctly initialized */
+		local_id_valid : 1, /* local_id is correctly initialized */
+		valid_csum_seen : 1;        /* at least one csum validated */
 	enum mptcp_data_avail data_avail;
 	u32	remote_nonce;
 	u64	thmac;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8c37087f0d84..dac38b6a8c53 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -955,11 +955,14 @@ static enum mapping_status validate_data_csum(struct sock *ssk, struct sk_buff *
 				 subflow->map_data_csum);
 	if (unlikely(csum)) {
 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DATACSUMERR);
-		subflow->send_mp_fail = 1;
-		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_MPFAILTX);
+		if (subflow->mp_join || subflow->valid_csum_seen) {
+			subflow->send_mp_fail = 1;
+			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_MPFAILTX);
+		}
 		return subflow->mp_join ? MAPPING_INVALID : MAPPING_DUMMY;
 	}
 
+	subflow->valid_csum_seen = 1;
 	return MAPPING_OK;
 }
 
@@ -1141,6 +1144,18 @@ static void subflow_sched_work_if_closed(struct mptcp_sock *msk, struct sock *ss
 	}
 }
 
+static bool subflow_can_fallback(struct mptcp_subflow_context *subflow)
+{
+	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+
+	if (subflow->mp_join)
+		return false;
+	else if (READ_ONCE(msk->csum_enabled))
+		return !subflow->valid_csum_seen;
+	else
+		return !subflow->fully_established;
+}
+
 static bool subflow_check_data_avail(struct sock *ssk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
@@ -1218,7 +1233,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 		return true;
 	}
 
-	if (subflow->mp_join || subflow->fully_established) {
+	if (!subflow_can_fallback(subflow)) {
 		/* fatal protocol error, close the socket.
 		 * subflow_error_report() will introduce the appropriate barriers
 		 */
-- 
2.36.1


Re: [PATCH mptcp-net v2] mptcp: Do TCP fallback on early DSS checksum failure
Posted by Matthieu Baerts 1 year, 10 months ago
Hi Mat, Paolo,

On 12/05/2022 01:51, Mat Martineau wrote:
> RFC 8684 section 3.7 describes several opportunities for a MPTCP
> connection to "fall back" to regular TCP early in the connection
> process, before it has been confirmed that MPTCP options can be
> successfully propagated on all SYN, SYN/ACK, and data packets. If a peer
> acknowledges the first received data packet with a regular TCP header
> (no MPTCP options), fallback is allowed.
> 
> If the recipient of that first data packet finds a MPTCP DSS checksum
> error, this provides an opportunity to fail gracefully with a TCP
> fallback rather than resetting the connection (as might happen if a
> checksum failure were detected later).
> 
> This commit modifies the checksum failure code to attempt fallback on
> the initial subflow of a MPTCP connection, only if it's a failure in the
> first data mapping. In cases where the peer initiates the connection,
> requests checksums, is the first to send data, and the peer is sending
> incorrect checksums (see
> https://github.com/multipath-tcp/mptcp_net-next/issues/275), this allows
> the connection to proceed as TCP rather than reset.
> 
> v2: add a helper function for checking whether a subflow is
> fallback-capable.

Thank you for the patch and review!

I just applied it in our tree (fixes for -net).


I had a conflict with commit f8d4bcacff3b ("mptcp: infinite mapping
receiving"), may you check if the resolution is correct please?


- 88cf15503ba1: mptcp: Do TCP fallback on early DSS checksum failure
- Results: b453c372356a..128393ceec9b (export-net)
- 74a4236c9eae: conflict in
top-bases/t/DO-NOT-MERGE-git-markup-fixes-net-next
- Results: db7207dab3db..9634a3a2eced (export)


Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20220514T164907
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export-net

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