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

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

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