Re: [PATCH mptcp-net 20/34] mptcp: stop resending data to a peer that fell back

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] mptcp: drop skb in mptcp_incoming_options() if
  ack_update_msk() resets subflow
--

commit 7d0f1ec53421d182e690548304d3c287dc956345
Author: Chris Mason <mason@kernel.org>

mptcp: stop resending data to a peer that fell back

This commit resets the subflow when mptcp_sendmsg_frag() is about to queue
more old data after previous old data was acknowledged only by bare ACKs,
clears old_data_pending on any non-infinite MPTCP option, and skips zero
window probes while the subflow retransmit queue is non-empty.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 7329deb43c622..388e77da8f16c 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -1527,11 +1528,29 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>  			return 0;
>  		}
>  
> +		/* a new probe would move old_data_end: one at a time */
> +		if (!tcp_rtx_queue_empty(ssk)) {
> +			tcp_remove_empty_skb(ssk);
> +			return 0;
> +		}
> +
>  		zero_window_probe = true;
>  		data_seq = snd_una - 1;
>  		copy = 1;
>  	}
>  
> +	/* old data acked without MPTCP opts: the peer may have fallen back */
> +	if (unlikely(before64(data_seq, msk->snd_nxt) &&
> +		     subflow->old_data_pending &&
> +		     !before(tcp_sk(ssk)->snd_una, subflow->old_data_end))) {
> +		tcp_remove_empty_skb(ssk);
> +		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_FALLBACKFAILED);
> +		subflow->reset_transient = 0;
> +		subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
> +		mptcp_subflow_reset(ssk);
> +		return -EAGAIN;
> +	}

[Severity: Medium]
Can mptcp_incoming_options() still return true to tcp_data_queue() after
mptcp_sendmsg_frag() resets the subflow from ack_update_msk()?

mptcp_incoming_options() only checks for sk->sk_state != TCP_CLOSE right
after check_fully_established(), assuming nothing later in the function
can reset sk.  However, ack_update_msk() calls __mptcp_check_push() ->
__mptcp_subflow_push_pending() -> __subflow_push_pending() ->
mptcp_sendmsg_frag() when !sock_owned_by_user(sk):

net/mptcp/options.c:mptcp_incoming_options() {
	...
	if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
		return sk->sk_state != TCP_CLOSE;
	...
	if (mp_opt.use_ack)
		ack_update_msk(msk, sk, skb, &mp_opt);
	rwin_update(msk, sk, skb);
	...
	return true;
}

If the subflow has old_data_pending set and snd_una has reached
old_data_end (for example via a prior bare ACK or an incoming DSS carrying
both DATA_ACK and an infinite mapping) while msk has unsent reinjected
data, mptcp_sendmsg_frag() calls mptcp_subflow_reset(ssk), which purges
sk_receive_queue, sends a RST, and moves ssk to TCP_CLOSE via tcp_done().
Because mptcp_incoming_options() still returns true, tcp_data_queue()
continues processing the incoming skb on the TCP_CLOSE subflow socket,
queueing payload onto sk_receive_queue, re-arming ACK timers, or hitting
tcp_fin() on a closed socket.

Checking sk->sk_state == TCP_CLOSE after ack_update_msk() and returning
false would let tcp_data_queue() drop the packet.

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