1. Patchew
  2. MPTCP
  3. View series

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

Paolo Abeni posted 1 patch 1 year, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/1a8201582e7fd63db7b5c9c4077af1ab1953c57e.1651763323.git.pabeni@redhat.com
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Eric Dumazet <edumazet@google.com>, Paolo Abeni <pabeni@redhat.com>, "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Geliang Tang <geliangtang@gmail.com>
There is a newer version of this series
net/mptcp/options.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
[PATCH MPTCP-next] mptcp: fix checksum byte order
Posted by Paolo Abeni 1 year, 11 months ago 
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.

The change is not backward compatible, but I can't see any
other option. MPTCP checksum is disabled by default, and there are
no backward issues with csum disabled.

Fixes: c5b39e26d003 ("mptcp: send out checksum for DSS")
Fixes: 390b95a5fb84 ("mptcp: receive checksum for DSS")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/options.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index ac3b7b8a02f6..5b5849d9fe60 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,23 @@ 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, __be16 *ptr)
+{
+	put_unaligned_be16(len, ptr);
+
+	ptr += 1;
+	put_unaligned(csum, ptr);
+}
+
 void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
 			 struct mptcp_out_options *opts)
 {
@@ -1385,9 +1393,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,
+					     (__force __be16 *)ptr);
 			} else {
 				put_unaligned_be32(mpext->data_len << 16 |
 						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
@@ -1438,11 +1446,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)),
+				     (__force __be16 *)ptr);
 		} else {
 			put_unaligned_be32(opts->data_len << 16 |
 					   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
-- 
2.35.1
Re: [PATCH MPTCP-next] mptcp: fix checksum byte order
Posted by Mat Martineau 1 year, 11 months ago 
On Thu, 5 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.
>
> The change is not backward compatible, but I can't see any
> other option. MPTCP checksum is disabled by default, and there are
> no backward issues with csum disabled.
>

See my reply a few minutes ago on the "Re: apropos 
https://github.com/multipath-tcp/mptcp_net-next/issues/265" thread, maybe 
we do have some backward-compatibility-via-fallback options?


> Fixes: c5b39e26d003 ("mptcp: send out checksum for DSS")
> Fixes: 390b95a5fb84 ("mptcp: receive checksum for DSS")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/options.c | 35 ++++++++++++++++++++++-------------
> 1 file changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index ac3b7b8a02f6..5b5849d9fe60 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,23 @@ 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, __be16 *ptr)

If ptr was a __be32, this code could do the casting inside this function 
and the callers would look a little cleaner. Not a big deal though - 
existing code ok if there's a good rationale for the __be16 that I'm 
missing.

Other than that, there is the warning from CI to fix and basing this on 
mptcp-net.

I haven't had a chance to try this in a mixed-endian setup yet but Maxim's 
tests looked promising.

- Mat

> +{
> +	put_unaligned_be16(len, ptr);
> +
> +	ptr += 1;
> +	put_unaligned(csum, ptr);
> +}
> +
> void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
> 			 struct mptcp_out_options *opts)
> {
> @@ -1385,9 +1393,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,
> +					     (__force __be16 *)ptr);
> 			} else {
> 				put_unaligned_be32(mpext->data_len << 16 |
> 						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> @@ -1438,11 +1446,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)),
> +				     (__force __be16 *)ptr);
> 		} else {
> 			put_unaligned_be32(opts->data_len << 16 |
> 					   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> -- 
> 2.35.1
>
>
>

--
Mat Martineau
Intel

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.