Re: [PATCH mptcp-net 26/34] mptcp: don't send a FIN before the pending data on fallback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mptcp: missing TCP FIN in mptcp_try_fallback() after data
  reinjection
- [Medium] mptcp: stalled pending data and FIN after fallback in
  mptcp_try_fallback()
--

commit a99710492ed63fdd9aa5e8bcc3f5436e41a0c574
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: don't send a FIN before the pending data on fallback

This commit updates mptcp_try_fallback() to skip shutting down the subflow
at fallback time when mptcp_send_head(sk) is non-NULL, deferring the TCP
FIN until the pending data has been pushed.

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index e61b6a3a955de..d76eaed4f01eb 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1339,7 +1339,10 @@ static inline bool mptcp_try_fallback(struct sock *ssk, int fb_mib)
>  
>  	if (!__mptcp_try_fallback(msk, fb_mib))
>  		return false;
> -	if (READ_ONCE(msk->snd_data_fin_enable) && !(ssk->sk_shutdown & SEND_SHUTDOWN)) {
> +
> +	/* no FIN before pending data: see mptcp_check_send_data_fin() */
> +	if (READ_ONCE(msk->snd_data_fin_enable) && !mptcp_send_head(sk) &&
> +	    !(ssk->sk_shutdown & SEND_SHUTDOWN)) {
>  		gfp_t saved_allocation = ssk->sk_allocation;
>  
>  		/* we are in a atomic (BH) scope, override ssk default for data

[Severity: Medium]
Can this regression skip sending a TCP FIN altogether if
__mptcp_retransmit_pending_data() has reset msk->first_pending after
mptcp_check_send_data_fin() already ran?

Once all data is pushed to the initial subflow and the socket is shut
down, mptcp_check_send_data_fin() sets msk->snd_nxt = msk->write_seq.
If an unestablished MP_JOIN subflow then closes via __mptcp_close_ssk(),
__mptcp_retransmit_pending_data() resets msk->first_pending to rtx_head
without changing msk->snd_nxt.  If the initial subflow falls back to TCP
before that reinjected data is processed, !mptcp_send_head(sk) is false
and mptcp_try_fallback() skips tcp_shutdown(ssk, SEND_SHUTDOWN) even
though ssk already has all data below msk->snd_nxt in its write queue.

Later, mptcp_sendmsg_frag() skips all bytes below msk->snd_nxt and
clears msk->first_pending back to NULL, but mptcp_check_send_data_fin()
will never run again because msk->snd_nxt == msk->write_seq:

net/mptcp/protocol.c:mptcp_check_send_data_fin() {
	if (!msk->snd_data_fin_enable || msk->snd_nxt + 1 != msk->write_seq ||
	    mptcp_send_head(sk))
		return;

	WRITE_ONCE(msk->snd_nxt, msk->write_seq);
    ...
}

As a result, the subflow never sends a TCP FIN and stays in
TCP_ESTABLISHED while the MPTCP socket remains stuck in TCP_FIN_WAIT2.
Allowing the shutdown when mptcp_data_fin_enabled(msk) is true (i.e.
!mptcp_send_head(sk) || mptcp_data_fin_enabled(msk)) would avoid this.

[Severity: Medium]
Can this regression also leave both the pending data and the TCP FIN
unsent when a non-TFO subflow falls back while both sent unacknowledged
data and unsent queued data exist?

Suppose mptcp_rtx_head(sk) has dfrag->already_sent > 0, unsent data
remains queued at mptcp_send_head(sk), and the application calls
shutdown(SHUT_WR).  When the peer sends a TCP ACK without MPTCP options
that acks the in-flight TCP data on ssk, check_fully_established() (when
!tcp_sk(ssk)->syn_fastopen_child) or subflow_check_data_avail() calls
mptcp_try_fallback() without updating msk->snd_una or calling
__mptcp_check_push(sk, ssk):

net/mptcp/options.c:check_fully_established() {
    ...
		if (!mptcp_try_fallback(ssk, MPTCP_MIB_MPCAPABLEACTFALLBACK))
			goto Inevitable;

		if (tcp_sk(ssk)->syn_fastopen_child) {
			mptcp_data_lock(sk);
			__mptcp_check_push(sk, ssk);
			mptcp_data_unlock(sk);
		}
		return false;
    ...
}

mptcp_try_fallback() clears MPTCP_RTX_ENABLED and skips
tcp_shutdown(ssk, SEND_SHUTDOWN) because mptcp_send_head(sk) is
non-NULL.  When the MPTCP retransmit timer later fires, msk->snd_una is
still below msk->snd_nxt, so __mptcp_retrans() sees
dfrag->already_sent > 0 after fallback, gets -1 from
__mptcp_push_retrans(), and jumps to clear_scheduled without calling
mptcp_check_and_set_pending(sk).  With ssk's TCP write queue empty and
the peer waiting for the remaining data and FIN, nothing ever pushes
mptcp_send_head(sk) or sends the FIN, stalling the connection.

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