Re: [PATCH mptcp-next RFC v4 2/2] mptcp: send MP_FAIL and infinite mapping on the same ACK

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] MP_FAIL is permanently dropped due to TCP options space exhaustion when packed alongside DSS and Timestamps.
- [High] Dropped pure ACKs cause a fatal connection reset because immediate fallback disables MPTCP options on retransmissions.
- [High] The send_infinite_map flag is cleared when transmitting a pure ACK, omitting the map from the first data packet if the ACK drops.
--

--- Patch [2]: [PATCH mptcp-next RFC v4 2/2] mptcp: send MP_FAIL and infinite mapping on the same ACK ---
commit fa151aec0663a3e5b6abb8fb25a4fd411a972b0a
Author: Chenguang Zhao <[email protected]>
mptcp: send MP_FAIL and infinite mapping on the same ACK

This commit updates the handling of an MP_FAIL response on a single subflow.
It emits the infinite mapping on the MP_FAIL reply ACK and then immediately
transitions the socket into fallback mode.

> mptcp: send MP_FAIL and infinite mapping on the same ACK
>
> On a single subflow, accepting MP_FAIL must leave MPTCP mode at once
> (RFC8684 §3.7). Emit the infinite mapping on the MP_FAIL reply ACK,
> then call mptcp_try_fallback() immediately after.

[Severity: High]
If we pack both the MP_FAIL and the infinite mapping on the same ACK,
is there enough TCP options space if timestamps are enabled?

With standard TCP Timestamps (12 bytes), the remaining option space is 28
bytes.

The infinite mapping requires at least 24 bytes (base + 64-bit map + ACK),
leaving at most 4 bytes.

Since MP_FAIL requires 12 bytes, will it fail the space check in
mptcp_established_options() and be silently dropped?

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 2f571766db440..e4f8e976ff163 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c

[ ... ]

> @@ -1567,6 +1589,15 @@ void mptcp_write_options(struct tcphdr *th, __be32 *ptr, struct tcp_sock *tp,
>  						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
>  			}
>  			ptr += 1;
> +
> +			if (unlikely(mpext->infinite_map)) {
> +				subflow = mptcp_subflow_ctx(ssk);
> +				if (subflow->send_infinite_map) {
> +					subflow->send_infinite_map = 0;

[Severity: High]
Does clearing this flag on a pure ACK risk losing the infinite mapping if
the network drops the packet?

Since TCP does not retransmit pure ACKs, the infinite mapping would need to
be piggybacked on the next data packet.

If we clear the send_infinite_map flag here, will the next data packet fail
to include the mapping and transmit as plain TCP instead?

> +					MPTCP_INC_STATS(sock_net(ssk),
> +							MPTCP_MIB_INFINITEMAPTX);
> +				}
> +			}
>  		}
>  
>  		/* We might need to add MP_FAIL options in rare cases */

[ ... ]

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d7c5b50b34cc9..351f9c998d7b6 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -895,6 +895,11 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
>  		pr_debug("MP_FAIL response received\n");
>  		WRITE_ONCE(subflow->fail_tout, 0);
>  	}
> +
> +	if (!mptcp_try_fallback(sk, MPTCP_MIB_MPFAILFALLBACK)) {

[Severity: High]
Does entering fallback immediately after sending the pure ACK prevent
retransmission of MPTCP options if the ACK is lost?

If the initial pure ACK drops, the remote peer will retransmit its MP_FAIL.
When the local stack responds with another pure ACK, skb is NULL.

Because the socket is already in fallback mode, it looks like
mptcp_established_options() will fail the fallback check:

    if (unlikely(__mptcp_check_fallback(msk) && !mptcp_check_infinite_map(skb)))
        return 0;

Could this permanently prevent recovery by omitting the MP_FAIL option on
subsequent ACKs?

> +		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_FALLBACKFAILED);
> +		mptcp_subflow_reset(sk);
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2