| Message ID | 1a8201582e7fd63db7b5c9c4077af1ab1953c57e.1651763323.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [MPTCP-next] mptcp: fix checksum byte order | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | warning | Build error with: make C=1 net/mptcp/options.o |
| matttbe/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 78 lines checked |
| matttbe/KVM_Validation__normal | warning | Unstable: 2 failed test(s): selftest_mptcp_join selftest_simult_flows |
| matttbe/KVM_Validation__debug | warning | Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join |
Hi Paolo, Thank you for your modifications, that's great! But sadly, our CI spotted some issues with it when trying to build it. You can find more details there: https://patchwork.kernel.org/project/mptcp/patch/1a8201582e7fd63db7b5c9c4077af1ab1953c57e.1651763323.git.pabeni@redhat.com/ https://github.com/multipath-tcp/mptcp_net-next/actions/runs/2276563461 Status: failure Initiator: MPTCPimporter Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/e13b663008d0 Feel free to reply to this email if you cannot access logs, if you need some support to fix the error, if this doesn't seem to be caused by your modifications or if the error is a false positive one. Cheers, MPTCP GH Action bot Bot operated by Matthieu Baerts (Tessares)
Hi Paolo, Thank you for your modifications, that's great! Our CI did some validations and here is its report: - KVM Validation: normal: - Unstable: 2 failed test(s): selftest_mptcp_join selftest_simult_flows
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
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);
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(-)