[PATCH mptcp-next v2] mptcp: fix checksum byte order

[PATCH mptcp-next v2] mptcp: fix checksum byte order

[Paolo Abeni]

The MPTCP code typecasts the checksum value to u16 and
then convert it to big endian while storing the value into
the MPTCP option.

As a result, the wire encoding for little endian host is
wrong, and that causes interoperabilty interoperability
issues with other implementation or host with different endianess.

Address the issue writing in the packet the unmodified __sum16 value.

MPTCP checksum is disabled by default, interoperating with systems
with bad mptcp-level csum encodying should cause fallback to TCP.

Fixes: c5b39e26d003 ("mptcp: send out checksum for DSS")
Fixes: 390b95a5fb84 ("mptcp: receive checksum for DSS")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
v1 -> v2:
 - move the typecast inside put_len_csum (Mat)
 - updated the commit message (Mat)
 - fix a few sparse issues
---
 net/mptcp/options.c  | 37 ++++++++++++++++++++++++-------------
 net/mptcp/protocol.h |  2 +-
 net/mptcp/subflow.c  |  2 +-
 3 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index ac3b7b8a02f6..6c90c78b52d1 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -107,7 +107,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 			ptr += 2;
 		}
 		if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) {
-			mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr);
+			mp_opt->csum = get_unaligned((__force __sum16 *)ptr);
 			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
 			ptr += 2;
 		}
@@ -221,7 +221,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 
 			if (opsize == expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {
 				mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
-				mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr);
+				mp_opt->csum = get_unaligned((__force __sum16 *)ptr);
 				ptr += 2;
 			}
 
@@ -1282,7 +1282,7 @@ static void mptcp_set_rwin(struct tcp_sock *tp, struct tcphdr *th)
 	}
 }
 
-u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum)
+__sum16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum)
 {
 	struct csum_pseudo_header header;
 	__wsum csum;
@@ -1298,15 +1298,25 @@ u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum)
 	header.csum = 0;
 
 	csum = csum_partial(&header, sizeof(header), sum);
-	return (__force u16)csum_fold(csum);
+	return csum_fold(csum);
 }
 
-static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
+static __sum16 mptcp_make_csum(const struct mptcp_ext *mpext)
 {
 	return __mptcp_make_csum(mpext->data_seq, mpext->subflow_seq, mpext->data_len,
 				 ~csum_unfold(mpext->csum));
 }
 
+static void put_len_csum(u16 len, __sum16 csum, void *data)
+{
+	__sum16 *sumptr = data + 2;
+	__be16 *ptr = data;
+
+	put_unaligned_be16(len, ptr);
+
+	put_unaligned(csum, sumptr);
+}
+
 void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
 			 struct mptcp_out_options *opts)
 {
@@ -1385,9 +1395,9 @@ void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
 				/* data_len == 0 is reserved for the infinite mapping,
 				 * the checksum will also be set to 0.
 				 */
-				put_unaligned_be32(mpext->data_len << 16 |
-						   (mpext->data_len ? mptcp_make_csum(mpext) : 0),
-						   ptr);
+				put_len_csum(mpext->data_len,
+					     mpext->data_len ? mptcp_make_csum(mpext) : 0,
+					     ptr);
 			} else {
 				put_unaligned_be32(mpext->data_len << 16 |
 						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
@@ -1438,11 +1448,12 @@ void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
 			goto mp_capable_done;
 
 		if (opts->csum_reqd) {
-			put_unaligned_be32(opts->data_len << 16 |
-					   __mptcp_make_csum(opts->data_seq,
-							     opts->subflow_seq,
-							     opts->data_len,
-							     ~csum_unfold(opts->csum)), ptr);
+			put_len_csum(opts->data_len,
+				     __mptcp_make_csum(opts->data_seq,
+						       opts->subflow_seq,
+						       opts->data_len,
+						       ~csum_unfold(opts->csum)),
+				     ptr);
 		} else {
 			put_unaligned_be32(opts->data_len << 16 |
 					   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 59a23838782f..f98c27300e60 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -763,7 +763,7 @@ void mptcp_token_destroy(struct mptcp_sock *msk);
 void mptcp_crypto_key_sha(u64 key, u32 *token, u64 *idsn);
 
 void mptcp_crypto_hmac_sha(u64 key1, u64 key2, u8 *msg, int len, void *hmac);
-u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum);
+__sum16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum);
 
 void __init mptcp_pm_init(void);
 void mptcp_pm_data_init(struct mptcp_sock *msk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 9842dab09c8b..b38a02db924b 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -891,7 +891,7 @@ static enum mapping_status validate_data_csum(struct sock *ssk, struct sk_buff *
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
 	u32 offset, seq, delta;
-	u16 csum;
+	__sum16 csum;
 	int len;
 
 	if (!csum_reqd)
-- 
2.35.3

Re: mptcp: fix checksum byte order: Tests Results

[MPTCP CI]

Hi Paolo,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 1 failed test(s): selftest_simult_flows 🔴:
  - Task: https://cirrus-ci.com/task/5103936225935360
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5103936225935360/summary/summary.txt

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

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


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 v2] mptcp: fix checksum byte order

[Mat Martineau]

On Tue, 10 May 2022, Paolo Abeni wrote:

> The MPTCP code typecasts the checksum value to u16 and
> then convert it to big endian while storing the value into
> the MPTCP option.
>
> As a result, the wire encoding for little endian host is
> wrong, and that causes interoperabilty interoperability
> issues with other implementation or host with different endianess.
>
> Address the issue writing in the packet the unmodified __sum16 value.
>
> MPTCP checksum is disabled by default, interoperating with systems
> with bad mptcp-level csum encodying should cause fallback to TCP.
>
> Fixes: c5b39e26d003 ("mptcp: send out checksum for DSS")
> Fixes: 390b95a5fb84 ("mptcp: receive checksum for DSS")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> v1 -> v2:
> - move the typecast inside put_len_csum (Mat)
> - updated the commit message (Mat)
> - fix a few sparse issues

Hi Paolo, thanks for the v2:

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

(for export-net)

Still ok with you to apply this to export-net? There are a couple of minor 
conflicts when rebasing to export-net, and it also creates a merge 
conflict with net-next. But I think it's worth it to be able to say "every 
5.18 and later kernel has the checksum fix".

Have you tried backporting this commit to 5.17-stable or 5.15-stable?


- Mat


> ---
> net/mptcp/options.c  | 37 ++++++++++++++++++++++++-------------
> net/mptcp/protocol.h |  2 +-
> net/mptcp/subflow.c  |  2 +-
> 3 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index ac3b7b8a02f6..6c90c78b52d1 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -107,7 +107,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> 			ptr += 2;
> 		}
> 		if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) {
> -			mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr);
> +			mp_opt->csum = get_unaligned((__force __sum16 *)ptr);
> 			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
> 			ptr += 2;
> 		}
> @@ -221,7 +221,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>
> 			if (opsize == expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM) {
> 				mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
> -				mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr);
> +				mp_opt->csum = get_unaligned((__force __sum16 *)ptr);
> 				ptr += 2;
> 			}
>
> @@ -1282,7 +1282,7 @@ static void mptcp_set_rwin(struct tcp_sock *tp, struct tcphdr *th)
> 	}
> }
>
> -u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum)
> +__sum16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum)
> {
> 	struct csum_pseudo_header header;
> 	__wsum csum;
> @@ -1298,15 +1298,25 @@ u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum)
> 	header.csum = 0;
>
> 	csum = csum_partial(&header, sizeof(header), sum);
> -	return (__force u16)csum_fold(csum);
> +	return csum_fold(csum);
> }
>
> -static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> +static __sum16 mptcp_make_csum(const struct mptcp_ext *mpext)
> {
> 	return __mptcp_make_csum(mpext->data_seq, mpext->subflow_seq, mpext->data_len,
> 				 ~csum_unfold(mpext->csum));
> }
>
> +static void put_len_csum(u16 len, __sum16 csum, void *data)
> +{
> +	__sum16 *sumptr = data + 2;
> +	__be16 *ptr = data;
> +
> +	put_unaligned_be16(len, ptr);
> +
> +	put_unaligned(csum, sumptr);
> +}
> +
> void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
> 			 struct mptcp_out_options *opts)
> {
> @@ -1385,9 +1395,9 @@ void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
> 				/* data_len == 0 is reserved for the infinite mapping,
> 				 * the checksum will also be set to 0.
> 				 */
> -				put_unaligned_be32(mpext->data_len << 16 |
> -						   (mpext->data_len ? mptcp_make_csum(mpext) : 0),
> -						   ptr);
> +				put_len_csum(mpext->data_len,
> +					     mpext->data_len ? mptcp_make_csum(mpext) : 0,
> +					     ptr);
> 			} else {
> 				put_unaligned_be32(mpext->data_len << 16 |
> 						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> @@ -1438,11 +1448,12 @@ void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
> 			goto mp_capable_done;
>
> 		if (opts->csum_reqd) {
> -			put_unaligned_be32(opts->data_len << 16 |
> -					   __mptcp_make_csum(opts->data_seq,
> -							     opts->subflow_seq,
> -							     opts->data_len,
> -							     ~csum_unfold(opts->csum)), ptr);
> +			put_len_csum(opts->data_len,
> +				     __mptcp_make_csum(opts->data_seq,
> +						       opts->subflow_seq,
> +						       opts->data_len,
> +						       ~csum_unfold(opts->csum)),
> +				     ptr);
> 		} else {
> 			put_unaligned_be32(opts->data_len << 16 |
> 					   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 59a23838782f..f98c27300e60 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -763,7 +763,7 @@ void mptcp_token_destroy(struct mptcp_sock *msk);
> void mptcp_crypto_key_sha(u64 key, u32 *token, u64 *idsn);
>
> void mptcp_crypto_hmac_sha(u64 key1, u64 key2, u8 *msg, int len, void *hmac);
> -u16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum);
> +__sum16 __mptcp_make_csum(u64 data_seq, u32 subflow_seq, u16 data_len, __wsum sum);
>
> void __init mptcp_pm_init(void);
> void mptcp_pm_data_init(struct mptcp_sock *msk);
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 9842dab09c8b..b38a02db924b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -891,7 +891,7 @@ static enum mapping_status validate_data_csum(struct sock *ssk, struct sk_buff *
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> 	u32 offset, seq, delta;
> -	u16 csum;
> +	__sum16 csum;
> 	int len;
>
> 	if (!csum_reqd)
> -- 
> 2.35.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2] mptcp: fix checksum byte order

[Paolo Abeni]

On Tue, 2022-05-10 at 16:10 -0700, Mat Martineau wrote:
> On Tue, 10 May 2022, Paolo Abeni wrote:
> 
> > The MPTCP code typecasts the checksum value to u16 and
> > then convert it to big endian while storing the value into
> > the MPTCP option.
> > 
> > As a result, the wire encoding for little endian host is
> > wrong, and that causes interoperabilty interoperability
> > issues with other implementation or host with different endianess.
> > 
> > Address the issue writing in the packet the unmodified __sum16 value.
> > 
> > MPTCP checksum is disabled by default, interoperating with systems
> > with bad mptcp-level csum encodying should cause fallback to TCP.
> > 
> > Fixes: c5b39e26d003 ("mptcp: send out checksum for DSS")
> > Fixes: 390b95a5fb84 ("mptcp: receive checksum for DSS")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > v1 -> v2:
> > - move the typecast inside put_len_csum (Mat)
> > - updated the commit message (Mat)
> > - fix a few sparse issues
> 
> Hi Paolo, thanks for the v2:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> 
> (for export-net)
> 
> Still ok with you to apply this to export-net? There are a couple of minor 
> conflicts when rebasing to export-net, and it also creates a merge 
> conflict with net-next. But I think it's worth it to be able to say "every 
> 5.18 and later kernel has the checksum fix".

Agreed. 

> Have you tried backporting this commit to 5.17-stable or 5.15-stable?

No, that is likely worthy, but I really have my hands full :( I think I
can have a look not earlier than next week.

/P

Re: [PATCH mptcp-next v2] mptcp: fix checksum byte order

[Mat Martineau]

On Wed, 11 May 2022, Paolo Abeni wrote:

> On Tue, 2022-05-10 at 16:10 -0700, Mat Martineau wrote:
>> On Tue, 10 May 2022, Paolo Abeni wrote:
>>
>>> The MPTCP code typecasts the checksum value to u16 and
>>> then convert it to big endian while storing the value into
>>> the MPTCP option.
>>>
>>> As a result, the wire encoding for little endian host is
>>> wrong, and that causes interoperabilty interoperability
>>> issues with other implementation or host with different endianess.
>>>
>>> Address the issue writing in the packet the unmodified __sum16 value.
>>>
>>> MPTCP checksum is disabled by default, interoperating with systems
>>> with bad mptcp-level csum encodying should cause fallback to TCP.
>>>
>>> Fixes: c5b39e26d003 ("mptcp: send out checksum for DSS")
>>> Fixes: 390b95a5fb84 ("mptcp: receive checksum for DSS")
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> v1 -> v2:
>>> - move the typecast inside put_len_csum (Mat)
>>> - updated the commit message (Mat)
>>> - fix a few sparse issues
>>
>> Hi Paolo, thanks for the v2:
>>
>> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
>>
>> (for export-net)
>>
>> Still ok with you to apply this to export-net? There are a couple of minor
>> conflicts when rebasing to export-net, and it also creates a merge
>> conflict with net-next. But I think it's worth it to be able to say "every
>> 5.18 and later kernel has the checksum fix".
>
> Agreed.
>
>> Have you tried backporting this commit to 5.17-stable or 5.15-stable?
>
> No, that is likely worthy, but I really have my hands full :( I think I
> can have a look not earlier than next week.

No problem - I can handle the backporting and sending to the stable trees.

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2] mptcp: fix checksum byte order

[Matthieu Baerts]

Hi Paolo, Mat,

On 10/05/2022 18:25, Paolo Abeni wrote:
> The MPTCP code typecasts the checksum value to u16 and
> then convert it to big endian while storing the value into
> the MPTCP option.
> 
> As a result, the wire encoding for little endian host is
> wrong, and that causes interoperabilty interoperability
> issues with other implementation or host with different endianess.
> 
> Address the issue writing in the packet the unmodified __sum16 value.
> 
> MPTCP checksum is disabled by default, interoperating with systems
> with bad mptcp-level csum encodying should cause fallback to TCP.

Thank you for the patch and the review!

Now in our tree (fixes for -net) with Mat's RvB tag:

- 5291df4774c1: mptcp: fix checksum byte order (with conflicts)
- Results: 128393ceec9b..35e8b300fc90 (export-net)

- 9a016e9bf5e7: conflict in
top-bases/t/DO-NOT-MERGE-git-markup-fixes-net-nex
- Results: 9634a3a2eced..5ebe27580643 (export)


Builds and tests are now in progress:

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

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

Re: [PATCH mptcp-next v2] mptcp: fix checksum byte order

[Mat Martineau]

On Sat, 14 May 2022, Matthieu Baerts wrote:

> Hi Paolo, Mat,
>
> On 10/05/2022 18:25, Paolo Abeni wrote:
>> The MPTCP code typecasts the checksum value to u16 and
>> then convert it to big endian while storing the value into
>> the MPTCP option.
>>
>> As a result, the wire encoding for little endian host is
>> wrong, and that causes interoperabilty interoperability
>> issues with other implementation or host with different endianess.
>>
>> Address the issue writing in the packet the unmodified __sum16 value.
>>
>> MPTCP checksum is disabled by default, interoperating with systems
>> with bad mptcp-level csum encodying should cause fallback to TCP.
>
> Thank you for the patch and the review!
>
> Now in our tree (fixes for -net) with Mat's RvB tag:
>
> - 5291df4774c1: mptcp: fix checksum byte order (with conflicts)
> - Results: 128393ceec9b..35e8b300fc90 (export-net)
>

Matthieu, Paolo -

I'm planning to send this patch and "mptcp: Do TCP fallback on early DSS 
checksum failure" to the -net branch on Tuesday - any concerns with that?

Thanks,

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2] mptcp: fix checksum byte order

[Matthieu Baerts]

Hi Mat,

On 17/05/2022 01:07, Mat Martineau wrote:
> 
> On Sat, 14 May 2022, Matthieu Baerts wrote:
> 
>> Hi Paolo, Mat,
>>
>> On 10/05/2022 18:25, Paolo Abeni wrote:
>>> The MPTCP code typecasts the checksum value to u16 and
>>> then convert it to big endian while storing the value into
>>> the MPTCP option.
>>>
>>> As a result, the wire encoding for little endian host is
>>> wrong, and that causes interoperabilty interoperability
>>> issues with other implementation or host with different endianess.
>>>
>>> Address the issue writing in the packet the unmodified __sum16 value.
>>>
>>> MPTCP checksum is disabled by default, interoperating with systems
>>> with bad mptcp-level csum encodying should cause fallback to TCP.
>>
>> Thank you for the patch and the review!
>>
>> Now in our tree (fixes for -net) with Mat's RvB tag:
>>
>> - 5291df4774c1: mptcp: fix checksum byte order (with conflicts)
>> - Results: 128393ceec9b..35e8b300fc90 (export-net)
>>
> 
> Matthieu, Paolo -
> 
> I'm planning to send this patch and "mptcp: Do TCP fallback on early DSS
> checksum failure" to the -net branch on Tuesday - any concerns with that?

Thank you for having done that!

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