Re: [PATCH mptcp-net 15/34] mptcp: close a subflow reset for a bad mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mptcp: first stream byte lost on simultaneous connect fallback
  in subflow_finish_connect() [1]
--

commit 68bf5d98c7b71382964b2e0b6553373923662fac
Author: Chris Mason <mason@kernel.org>

mptcp: close a subflow reset for a bad mapping

This commit calls subflow_sched_work_if_closed() after resetting a subflow
due to a bad mapping when TCP fallback is not possible, so the MPTCP worker
closes the subflow and updates the MPTCP socket state.

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index d23da4ac19684..7b8183b6a13e2 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1499,8 +1499,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  		}
>  
>  		if (!mptcp_try_fallback(ssk, MPTCP_MIB_DSSFALLBACK)) {
> -			/* fatal protocol error, close the socket.
> -			 * subflow_error_report() will introduce the appropriate barriers
> +			/* fatal protocol error, reset the subflow and let the
> +			 * worker close it
>  			 */
>  			subflow->reset_transient = 0;
>  			MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSRESET);
> @@ -1515,6 +1515,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  				sk_eat_skb(ssk, skb);
>  			mptcp_send_active_reset_reason(ssk);
>  			WRITE_ONCE(subflow->data_avail, false);
> +			subflow_sched_work_if_closed(msk, ssk);
>  			return false;
>  		}
>  	}

[Severity: High]
This problem wasn't introduced by this patch, but can the fallback dummy
mapping right below this hunk in subflow_check_data_avail() drop the first
byte of incoming data after a TCP simultaneous connect [1]?

During simultaneous connect in TCP_SYN_SENT, mptcp_syn_options() detects
subflow_simultaneous_connect(), calls mptcp_try_fallback(), and clears
subflow->request_mptcp without setting subflow->conn_finished or
subflow->ssn_offset.  Because tcp_rcv_state_process() moves the socket from
TCP_SYN_RECV to TCP_ESTABLISHED without calling sk_rx_dst_set(),
subflow_finish_connect() is only invoked when the first packet in
TCP_ESTABLISHED reaches tcp_rcv_established().  At that point
TCP_SKB_CB(skb)->seq is peer_isn + 1 instead of peer_isn:

net/mptcp/subflow.c:subflow_finish_connect() {
	...
	subflow->rel_write_seq = 1;
	subflow->conn_finished = 1;
	subflow->ssn_offset = TCP_SKB_CB(skb)->seq;
	...
}

net/mptcp/subflow.c:subflow_check_data_avail() {
	...
fallback:
	...
	skb = skb_peek(&ssk->sk_receive_queue);
	subflow->map_valid = 1;
	subflow->map_data_len = skb->len;
	subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
	subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
					      subflow->iasn +
					      TCP_SKB_CB(skb)->seq -
					      subflow->ssn_offset - 1);
}

When the first data segment (with seq == peer_isn + 1) reaches the fallback
path in subflow_check_data_avail(), subflow->iasn + TCP_SKB_CB(skb)->seq -
subflow->ssn_offset - 1 evaluates to -1, setting subflow->map_seq to -1ULL
while msk->ack_seq is 0.  In __mptcp_move_skb(), a 1-byte initial segment
is then dropped as duplicate data, or a multi-byte segment has its offset
advanced by 1 byte, corrupting the stream.

[1] https://sashiko.dev/#/bug/linux-17755322-ec78-468c-aabb-cd93fe7c5a2f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=15