Re: [PATCH mptcp-next v14 4/6] mptcp: pm: in-kernel: retry subflows closed after an error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: pm: __mptcp_close_ssk() schedules new subflows on MPTCP_CF_RETRY
--

commit 8b4a965d9506046df65ec4c54a676dc6bc6eda03
Author: Anil Kaushik <anilkaushikwireless@gmail.com>

mptcp: pm: in-kernel: retry subflows closed after an error

This commit teaches the in-kernel path manager to re-create established
subflows that were closed after a path timeout while the MPTCP connection
remains established and at least one other subflow can still send data. It
also closes any pending re-created subflows if no other subflow can send data
anymore or if the MPTCP socket is shut down or closed.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..cb1350ba1a41a 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -2695,6 +2700,26 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	 * subflow close.
>  	 */
>  	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
> +
> +	/* a re-created subflow, see mptcp_close_retry_subflows(): the ssk lock
> +	 * defers the 4th ACK, so it cannot get fully established meanwhile
> +	 */
> +	if (flags & MPTCP_CF_RETRY) {
> +		if (READ_ONCE(subflow->fully_established)) {
> +			release_sock(ssk);
> +			return;
> +		}
> +
> +		/* like mptcp_close_ssk(): it can have joined already */
> +		if (sk->sk_state == TCP_ESTABLISHED &&
> +		    !subflow->close_event_done) {
> +			subflow->close_event_done = true;
> +			mptcp_event(MPTCP_EVENT_SUB_CLOSED, msk, ssk,
> +				    GFP_KERNEL);
> +		}
> +		mptcp_pm_subflow_check_next(msk, subflow);
> +	}

[Severity: Medium]
When closing stale re-created subflows via mptcp_close_retry_subflows() ->
__mptcp_close_ssk() with MPTCP_CF_RETRY because no other subflow can send
data anymore, can calling mptcp_pm_subflow_check_next() schedule new subflow
creation and cancel the close_timeout timer?

In net/mptcp/pm.c, mptcp_pm_subflow_check_next() decrements extra_subflows
via __mptcp_pm_close_subflow() and then checks whether more work is pending:

	if (mptcp_is_fully_established(sk) &&
	    mptcp_pm_nl_check_work_pending(msk))
		mptcp_pm_schedule_work(msk, MPTCP_PM_SUBFLOW_ESTABLISHED);

Consider a connection where limit_extra_subflows is 1 and two local subflow
endpoints (endpoint 1 and endpoint 2) are configured:

1. Subflow 1 (using endpoint 1) is established while endpoint 2 remains
   unused in id_avail_bitmap.
2. Subflow 1 times out while the initial subflow (msk->first) is still
   active, so __mptcp_close_subflow() re-creates Subflow 1 with
   subflow->retry = true and subflow->request_join = 1.
3. Before the re-created Subflow 1 becomes fully_established, msk->first
   also times out and enters TCP_CLOSE. Now no subflow can send data while
   sk->sk_state is still TCP_ESTABLISHED.
4. mptcp_worker() closes msk->first in __mptcp_close_subflow() and then sees
   mptcp_retry_subflows_stale(msk) is true, calling
   mptcp_close_retry_subflows(sk, true) -> __mptcp_close_ssk() with
   MPTCP_CF_RETRY.
5. __mptcp_close_ssk() calls mptcp_pm_subflow_check_next(), which decrements
   msk->pm.extra_subflows to 0 and schedules MPTCP_PM_SUBFLOW_ESTABLISHED
   because sk->sk_state is TCP_ESTABLISHED and endpoint 2 is available in
   id_avail_bitmap. __mptcp_close_ssk() then finishes closing the retry
   subflow and starts the close_timeout timer via mptcp_start_tout_timer(sk).
6. The scheduled PM work runs mptcp_pm_nl_subflow_established() ->
   mptcp_pm_create_subflow_or_signal_addr() -> __mptcp_subflow_connect() for
   endpoint 2. __mptcp_subflow_connect() creates a new subflow (with
   subflow->retry == false) while no subflow can send data and calls
   mptcp_stop_tout_timer(sk), cancelling the close_timeout timer. Because
   this new subflow has subflow->retry == false, mptcp_retry_subflows_stale()
   will not close it if it hangs in TCP_SYN_SENT or waits for the 4th ACK.

Should the MPTCP_CF_RETRY path in __mptcp_close_ssk() only release the
subflow slot (or avoid scheduling MPTCP_PM_SUBFLOW_ESTABLISHED when no
subflow can send data) instead of calling mptcp_pm_subflow_check_next()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101072051.3853492-1-anilkaushikwireless@gmail.com?part=4