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

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5292 bytes --]

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