Re: [PATCH mptcp-net v2] mptcp: options: handle MPC data + csum reqd + no csum
Mat Martineau <[email protected]>
| Newsgroups | dev.linux.lists.mptcp |
|---|---|
| Message-ID | <[email protected]> |
On Wed, 5 Aug 2026, Matthieu Baerts (NGI0) wrote:
> Before this modification, a remote peer could send an MP_CAPABLE with
> data, with the checksum flag set, but without adding the actual 2 bytes
> of checksum. As a result, uninitialised bytes could be used for the
> 'csum' field.
>
> That was not a critical issue, because this 'csum' field is only used to
> compare with the expected one, if previously negotiated in the 3WHS.
> Worst case, the checksum is likely wrong, a fallback is done without a
> reject if the negotiation was done earlier. That's OK.
>
> Yet, better to take the expected path with this case: only look at the
> checksum flag for MP_CAPABLEs not carrying a data-len.
>
> Such packet can be seen as a 3rd or 4th ACK. The RFC8684 mentions [1]
> that the 3rd packet should have the checksum flag set. When an MPC + ACK
> contains data, the checksum flag is redundant with the checksum field.
> It is not clear what should be done for the 4th ACK, nor if the flag has
> to be set if the checksum field is set.
>
> Therefore, it seems fine to only look at the presence of the checksum
> field, not to break the interaction with stacks that were not setting
> both.
>
> Fixes: 208e8f66926c ("mptcp: receive checksum for MP_CAPABLE with data")
> Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.1-23 [1]
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260803-net-mptcp-misc-fixes-7-2-rc6-v2-0-b8f496d71664%40kernel.org?part=1
> Signed-off-by: Matthieu Baerts (NGI0) <[email protected]>
> ---
> Changes in v2:
> - back to my pre-version, only set CSUMREQD for non MPC + ACK + DATA
> with a longer explanation about why it is OK. (Sashiko)
> - Link to v1: https://patch.msgid.link/20260805-mptcp-opt-mpc-csumreqd-no-csum-v1-1-cb2ad0b9feac@kernel.org
> ---
> net/mptcp/options.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index e1b38fe5faf8..73cec164d8bd 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -93,7 +93,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> * In other words, the only way for checksums not to be used
> * is if both hosts in their SYNs set A=0."
> */
> - if (flags & MPTCP_CAP_CHECKSUM_REQD)
> + if ((flags & MPTCP_CAP_CHECKSUM_REQD) &&
> + opsize < TCPOLEN_MPTCP_MPC_ACK_DATA)
> mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
>
> mp_opt->deny_join_id0 = !!(flags & MPTCP_CAP_DENY_JOIN_ID0);
>
> ---
> base-commit: 064fb643fcfcdddbad6da71da8f1ab206f018af7
> change-id: 20260805-mptcp-opt-mpc-csumreqd-no-csum-3f145fa19c4d
>
Yes, this looks like a good tradeoff between not breaking interaction with
other stacks while avoiding an invalid checksum.
Reviewed-by: Mat Martineau <[email protected]>