[PATCH mptcp-net] 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)
There is a newer version of this series
net/mptcp/protocol.h |  3 ++-
net/mptcp/subflow.c  | 15 ++++++++++++---
2 files changed, 14 insertions(+), 4 deletions(-)
[PATCH mptcp-net] 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.

Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
---

I tested this on the export-net branch along with Paolo's proposed
checksum fix ("mptcp: fix checksum byte order"). Testing between the
patched export-net kernel and 5.17.5-200.fc35.x86_64 (Fedora) showed the
fallback succeeding if checksums are enabled on one side, export-net was
the listener, and 5.17.5 initiated the connection and sent the first
data. Without this patch, if one side has the fixed checksum byte order
then all connections would reset after a checksum failure.

---
 net/mptcp/protocol.h |  3 ++-
 net/mptcp/subflow.c  | 15 ++++++++++++---
 2 files changed, 14 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..3a7b02a92239 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;
 }
 
@@ -1145,6 +1148,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	enum mapping_status status;
+	bool fallback_impossible;
 	struct mptcp_sock *msk;
 	struct sk_buff *skb;
 
@@ -1218,7 +1222,12 @@ static bool subflow_check_data_avail(struct sock *ssk)
 		return true;
 	}
 
-	if (subflow->mp_join || subflow->fully_established) {
+	if (READ_ONCE(msk->csum_enabled))
+		fallback_impossible = subflow->valid_csum_seen;
+	else
+		fallback_impossible = subflow->fully_established;
+
+	if (subflow->mp_join || fallback_impossible) {
 		/* fatal protocol error, close the socket.
 		 * subflow_error_report() will introduce the appropriate barriers
 		 */
-- 
2.36.1


Re: [PATCH mptcp-net] mptcp: Do TCP fallback on early DSS checksum failure
Posted by Paolo Abeni 1 year, 10 months ago
On Mon, 2022-05-09 at 17:03 -0700, 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.
> 
> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> ---
> 
> I tested this on the export-net branch along with Paolo's proposed
> checksum fix ("mptcp: fix checksum byte order"). Testing between the
> patched export-net kernel and 5.17.5-200.fc35.x86_64 (Fedora) showed the
> fallback succeeding if checksums are enabled on one side, export-net was
> the listener, and 5.17.5 initiated the connection and sent the first
> data. Without this patch, if one side has the fixed checksum byte order
> then all connections would reset after a checksum failure.
> 
> ---
>  net/mptcp/protocol.h |  3 ++-
>  net/mptcp/subflow.c  | 15 ++++++++++++---
>  2 files changed, 14 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..3a7b02a92239 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;
>  }
>  
> @@ -1145,6 +1148,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>  	enum mapping_status status;
> +	bool fallback_impossible;
>  	struct mptcp_sock *msk;
>  	struct sk_buff *skb;
>  
> @@ -1218,7 +1222,12 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  		return true;
>  	}
>  
> -	if (subflow->mp_join || subflow->fully_established) {
> +	if (READ_ONCE(msk->csum_enabled))
> +		fallback_impossible = subflow->valid_csum_seen;
> +	else
> +		fallback_impossible = subflow->fully_established;
> +
> +	if (subflow->mp_join || fallback_impossible) {
>  		/* fatal protocol error, close the socket.
>  		 * subflow_error_report() will introduce the appropriate barriers
>  		 */

LGTM, thanks! Perhaps the last chunk can be encapsulated in an helper:
 
subflow_is_fallback_possible(subflow, msk) 

embedding all the relevant test?

Either ways: 

Acked-by: Paolo Abeni <pabeni@redhat.com>


Re: [PATCH mptcp-net] mptcp: Do TCP fallback on early DSS checksum failure
Posted by Geliang Tang 1 year, 10 months ago
Paolo Abeni <pabeni@redhat.com> 于2022年5月11日周三 18:31写道:
>
> On Mon, 2022-05-09 at 17:03 -0700, 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.
> >
> > Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> > ---
> >
> > I tested this on the export-net branch along with Paolo's proposed
> > checksum fix ("mptcp: fix checksum byte order"). Testing between the
> > patched export-net kernel and 5.17.5-200.fc35.x86_64 (Fedora) showed the
> > fallback succeeding if checksums are enabled on one side, export-net was
> > the listener, and 5.17.5 initiated the connection and sent the first
> > data. Without this patch, if one side has the fixed checksum byte order
> > then all connections would reset after a checksum failure.
> >
> > ---
> >  net/mptcp/protocol.h |  3 ++-
> >  net/mptcp/subflow.c  | 15 ++++++++++++---
> >  2 files changed, 14 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..3a7b02a92239 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;
> >  }
> >
> > @@ -1145,6 +1148,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
> >  {
> >       struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> >       enum mapping_status status;
> > +     bool fallback_impossible;
> >       struct mptcp_sock *msk;
> >       struct sk_buff *skb;
> >
> > @@ -1218,7 +1222,12 @@ static bool subflow_check_data_avail(struct sock *ssk)
> >               return true;
> >       }
> >
> > -     if (subflow->mp_join || subflow->fully_established) {
> > +     if (READ_ONCE(msk->csum_enabled))
> > +             fallback_impossible = subflow->valid_csum_seen;
> > +     else
> > +             fallback_impossible = subflow->fully_established;
> > +

Mat, how about using this instead:

fallback_impossible = READ_ONCE(msk->csum_enabled) ? subflow->valid_csum_seen :

                          subflow->fully_established;

Thanks,
-Geliang

> > +     if (subflow->mp_join || fallback_impossible) {
> >               /* fatal protocol error, close the socket.
> >                * subflow_error_report() will introduce the appropriate barriers
> >                */
>
> LGTM, thanks! Perhaps the last chunk can be encapsulated in an helper:
>
> subflow_is_fallback_possible(subflow, msk)
>
> embedding all the relevant test?
>
> Either ways:
>
> Acked-by: Paolo Abeni <pabeni@redhat.com>
>
>

Re: [PATCH mptcp-net] mptcp: Do TCP fallback on early DSS checksum failure
Posted by Mat Martineau 1 year, 10 months ago
On Wed, 11 May 2022, Geliang Tang wrote:

> Paolo Abeni <pabeni@redhat.com> 于2022年5月11日周三 18:31写道:
>>
>> On Mon, 2022-05-09 at 17:03 -0700, 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.
>>>
>>> Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>> ---
>>>
>>> I tested this on the export-net branch along with Paolo's proposed
>>> checksum fix ("mptcp: fix checksum byte order"). Testing between the
>>> patched export-net kernel and 5.17.5-200.fc35.x86_64 (Fedora) showed the
>>> fallback succeeding if checksums are enabled on one side, export-net was
>>> the listener, and 5.17.5 initiated the connection and sent the first
>>> data. Without this patch, if one side has the fixed checksum byte order
>>> then all connections would reset after a checksum failure.
>>>
>>> ---
>>>  net/mptcp/protocol.h |  3 ++-
>>>  net/mptcp/subflow.c  | 15 ++++++++++++---
>>>  2 files changed, 14 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..3a7b02a92239 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;
>>>  }
>>>
>>> @@ -1145,6 +1148,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>>  {
>>>       struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>>>       enum mapping_status status;
>>> +     bool fallback_impossible;
>>>       struct mptcp_sock *msk;
>>>       struct sk_buff *skb;
>>>
>>> @@ -1218,7 +1222,12 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>>               return true;
>>>       }
>>>
>>> -     if (subflow->mp_join || subflow->fully_established) {
>>> +     if (READ_ONCE(msk->csum_enabled))
>>> +             fallback_impossible = subflow->valid_csum_seen;
>>> +     else
>>> +             fallback_impossible = subflow->fully_established;
>>> +
>
> Mat, how about using this instead:
>
> fallback_impossible = READ_ONCE(msk->csum_enabled) ? subflow->valid_csum_seen :
>
>                          subflow->fully_established;
>

I did consider that but the line wrapping with the ?: operator seemed 
awkward.

>>> +     if (subflow->mp_join || fallback_impossible) {
>>>               /* fatal protocol error, close the socket.
>>>                * subflow_error_report() will introduce the appropriate barriers
>>>                */
>>
>> LGTM, thanks! Perhaps the last chunk can be encapsulated in an helper:
>>
>> subflow_is_fallback_possible(subflow, msk)
>>
>> embedding all the relevant test?
>>

Yeah, I'll do that for v2.

>> Either ways:
>>
>> Acked-by: Paolo Abeni <pabeni@redhat.com>

Thanks!


--
Mat Martineau
Intel