Re: [PATCH mptcp-next v8 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:
- [High] mptcp: race in mptcp_close_retry_subflows() due to unlocked ssk state check
--

commit c03fc05db7095624e90e910918ec9a00af7081a3
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
client-initiated or joined subflows that were closed by TCP after a path
timeout error. It also closes any re-created subflows that are still
connecting when the parent MPTCP socket is closed.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..f941533a1eb48 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -3501,6 +3506,34 @@ static void mptcp_check_listen_stop(struct sock *sk)
>  	release_sock(ssk);
>  }
>  
> +/* Re-created subflows still connecting are useless once the msk is closed,
> + * and their SYN retransmissions could keep them around for a long time.
> + */
> +static void mptcp_close_retry_subflows(struct sock *sk)
> +{
> +	struct mptcp_subflow_context *subflow, *tmp;
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +
> +	mptcp_for_each_subflow_safe(msk, subflow, tmp) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		int state;
> +
> +		if (!subflow->retry || READ_ONCE(subflow->fully_established))
> +			continue;
> +
> +		/* still connecting, or already disconnected by
> +		 * mptcp_subflow_shutdown(), e.g. after shutdown(SHUT_WR)
> +		 */
> +		state = inet_sk_state_load(ssk);
> +		if (state != TCP_SYN_SENT && state != TCP_CLOSE)
> +			continue;
> +
> +		/* it never carried data: nothing to push or re-inject */
> +		mptcp_pm_subflow_check_next(msk, subflow);
> +		__mptcp_close_ssk(sk, ssk, subflow, 0);
> +	}
> +}

[Severity: High]
Can mptcp_close_retry_subflows() in net/mptcp/protocol.c race with softirq
packet processing on ssk because subflow->fully_established and
inet_sk_state_load(ssk) are checked before ssk is locked in
__mptcp_close_ssk()?

When __mptcp_close() calls mptcp_close_retry_subflows(sk), the msk socket
lock is held, sk->sk_state is still TCP_ESTABLISHED, and msk->allow_subflows
is true, while ssk is not locked until __mptcp_close_ssk() runs:

net/mptcp/protocol.c:__mptcp_close_ssk() {
    lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
    subflow->closing = 1;

    if (flags & MPTCP_CF_PUSH)
        mptcp_cleanup_ssk_backlog(sk, ssk);
    ...
}

Between the lockless checks in mptcp_close_retry_subflows() and
lock_sock_nested(ssk) in __mptcp_close_ssk(), TCP softirq on another CPU can
process incoming packets under bh_lock_sock_nested(ssk):

1. If ssk is observed in TCP_SYN_SENT, an incoming MP_JOIN handshake can
transition ssk to TCP_ESTABLISHED via:

  tcp_rcv_synsent_state_process()
    -> subflow_finish_connect()
      -> mptcp_finish_join()

which succeeds without acquiring the msk socket lock because subflow->node
is on msk->conn_list and msk->allow_subflows is true. A subsequent packet
carrying the 4th ACK and DSS data can then set subflow->fully_established =
true in check_fully_established() and queue skbs onto msk->backlog_list via:

  subflow_data_ready()
    -> mptcp_data_ready()
      -> __mptcp_move_skbs_from_subflow(msk, ssk, false)
        -> __mptcp_add_backlog()

which sets skb->destructor = NULL and skb->sk = ssk while leaving
skb->truesize charged to ssk->sk_rmem_alloc.

2. Similarly, if ssk already completed the 3-way handshake
(TCP_ESTABLISHED, subflow->fully_established == false) when
READ_ONCE(subflow->fully_established) is checked, softirq can process the
4th ACK with DSS data (queuing skbs onto msk->backlog_list) and then a
subsequent FIN or RST transitioning ssk to TCP_CLOSE before
inet_sk_state_load(ssk) is read.

When __mptcp_close_ssk(sk, ssk, subflow, 0) then runs without MPTCP_CF_PUSH,
it skips mptcp_cleanup_ssk_backlog(sk, ssk) and destroys ssk while skbs on
msk->backlog_list still hold skb->sk = ssk:

net/mptcp/protocol.h:mptcp_borrow_fwdmem() {
    struct sock *ssk = skb->sk;
    ...
    if (!ssk)
        return;

    atomic_sub(skb->truesize, &ssk->sk_rmem_alloc);
    skb->sk = NULL;
}

Wouldn't this trigger the sk->sk_rmem_alloc warning in inet_sock_destruct()
and a use-after-free atomic_sub() on ssk->sk_rmem_alloc in
mptcp_borrow_fwdmem() when msk->backlog_list is later drained or purged?

不断地

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