Re: [PATCH net 05/14] mptcp: options: handle MPC data + csum reqd + no csum

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
mptcp: options: handle MPC data + csum reqd + no csum

The parser in mptcp_parse_option() no longer sets OPTION_MPTCP_CSUMREQD from
the MP_CAPABLE 'A' flag when the option carries a data-len, so only the
presence of the 2-byte checksum field marks a data-bearing MP_CAPABLE as
checksummed.  This avoids using the uninitialised 'csum' field when a peer
sets the flag but omits the checksum bytes.

Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.1-23

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index b8318e0301389..92f27b9e087a6 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;

[Severity: Medium]
On the passive side, is the third ACK's 'A' bit not the only place where the
peer's checksum requirement is recorded?

subflow_init_req() derives the request state purely from the local sysctl:

net/mptcp/subflow.c:subflow_init_req() {
	...
	subflow_req->csum_reqd = mptcp_is_checksum_enabled(sock_net(sk_listener));
	...
}

There is only that single store of csum_reqd in net/mptcp/subflow.c, so the
'A' bit seen in the remote SYN is not saved anywhere.  The only server-side
promotion into connection state is in mptcp_sk_clone_init():

net/mptcp/protocol.c:mptcp_sk_clone_init() {
	...
	if (mp_opt->suboptions & OPTION_MPTCP_CSUMREQD)
		WRITE_ONCE(msk->csum_enabled, true);
	...
}

With net.mptcp.checksum_enabled=0 (the default in net/mptcp/ctrl.c) and a
peer whose SYN had A=1, does the new opsize test mean msk->csum_enabled now
stays false for a 22-byte MP_CAPABLE + ACK that carries a data-len, sets A=1
and omits the checksum field?  That option length is still accepted by the
size check just above:

		if (opsize != expected_opsize &&
		    (expected_opsize != TCPOLEN_MPTCP_MPC_ACK_DATA ||
		     opsize != TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM))
			break;

Downstream, mptcp_incoming_options() then sets mpext->csum_reqd = 0, and
get_mapping_status() compares:

net/mptcp/subflow.c:get_mapping_status() {
	...
	/* Cfr RFC 8684 Section 3.3.0 */
	if (unlikely(subflow->map_csum_reqd != csum_reqd))
		return MAPPING_INVALID;
	...
}

Both sides being false, the mapping is accepted rather than rejected, so no
RST is sent for the non-conforming peer and the connection runs without
checksums even though the peer asked for them.  Our own writer still emits
the bit on data-carrying MPC + ACKs:

net/mptcp/options.c:mptcp_write_options() {
	...
	if (opts->csum_reqd)
		flag |= MPTCP_CAP_CHECKSUM_REQD;
	...
}

Related to this, the changelog says the csum field "is only used to compare
with the expected one, if previously negotiated in the 3WHS".  Since the
passive side never records the peer's SYN 'A' bit, does that statement hold
for a listener?

[Severity: Low]
This isn't an issue introduced by this patch, but while touching this area:
the trailing pr_debug() in mptcp_parse_option() prints fields that are only
conditionally assigned:

		pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u\n",
			 version, flags, opsize, mp_opt->sndr_key,
			 mp_opt->rcvr_key, mp_opt->data_len, mp_opt->csum);

sndr_key is only set for opsize >= TCPOLEN_MPTCP_MPC_SYNACK, rcvr_key for
opsize >= TCPOLEN_MPTCP_MPC_ACK, data_len for opsize >=
TCPOLEN_MPTCP_MPC_ACK_DATA and csum only for opsize ==
TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM.

mptcp_get_options() only clears the status group:

net/mptcp/options.c:mptcp_get_options() {
	...
	*(u32 *)&mp_opt->status = 0;
	...
}

and struct mptcp_options_received in net/mptcp/protocol.h places sndr_key,
rcvr_key, data_len and csum before struct_group(status, ...), while callers
such as mptcp_incoming_options(), subflow_check_req() and
mptcp_subflow_init_cookie_req() declare the struct on the stack without a
memset.

When the dynamic-debug callsite is enabled, does this print stale caller
stack bytes for MP_CAPABLE SYN, SYN/ACK, plain MPC + ACK, and the
data-without-checksum case this patch targets?  Would initialising csum and
data_len (or extending the zeroed region) make the stated goal of the patch
hold regardless of the debug setting, including for the stable backport?