ACK/Cmnt: [SRU][J][PATCH 1/1] mptcp: move subflow cleanup in mptcp_destroy_common()

[Andrei Gherzan]

On 26/09/10 06:35AM, Cengiz Can via kernel-team wrote:
> From: Paolo Abeni <pabeni at redhat.com>
> 
> If the mptcp socket creation fails due to a CGROUP_INET_SOCK_CREATE
> eBPF program, the MPTCP protocol ends-up leaking all the subflows:
> the related cleanup happens in __mptcp_destroy_sock() that is not
> invoked in such code path.
> 
> Address the issue moving the subflow sockets cleanup in the
> mptcp_destroy_common() helper, which is invoked in every msk cleanup
> path.
> 
> Additionally get rid of the intermediate list_splice_init step, which
> is an unneeded relic from the past.
> 
> The issue is present since before the reported root cause commit, but
> any attempt to backport the fix before that hash will require a complete
> rewrite.
> 
> Fixes: e16163b6e2 ("mptcp: refactor shutdown and close")
> Reported-by: Nguyen Dinh Phi <phind.uet at gmail.com>
> Reviewed-by: Mat Martineau <mathew.j.martineau at linux.intel.com>
> Co-developed-by: Nguyen Dinh Phi <phind.uet at gmail.com>
> Signed-off-by: Nguyen Dinh Phi <phind.uet at gmail.com>
> Signed-off-by: Paolo Abeni <pabeni at redhat.com>
> Signed-off-by: Mat Martineau <mathew.j.martineau at linux.intel.com>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (backported from commit c0bf3c6aa444a5ef44acc57ef6cfa53fd4fc1c9b)
> [bot_kybele: This tree's __mptcp_close_ssk is 3-arg (no
>  flags/MPTCP_CF_FASTCLOSE) so mptcp_destroy_common keeps the added flags param
>  but calls the 3-arg form; __mptcp_destroy_sock keeps its join_list->conn_list
>  splice (subflows now closed by mptcp_destroy_common via sk_prot->destroy) and
>  the sk_stop_timer idiom instead of mptcp_stop_timer; mptcp_disconnect here uses
>  tcp_disconnect() per subflow and never calls mptcp_destroy_common, so its
>  upstream hunk was not applicable and HEAD was kept.]
> CVE-2022-50071
> Assisted-by: kybele:claude-opus-4.8
> Signed-off-by: Cengiz Can <cengiz.can at canonical.com>
> ---
>  net/mptcp/protocol.c | 27 +++++++++++++++------------
>  net/mptcp/protocol.h |  2 +-
>  net/mptcp/subflow.c  |  3 ++-
>  3 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 26d32af7a43f..25ff5be71f57 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2806,31 +2806,25 @@ static void __mptcp_wr_shutdown(struct sock *sk)
>  
>  static void __mptcp_destroy_sock(struct sock *sk)
>  {
> -	struct mptcp_subflow_context *subflow, *tmp;
>  	struct mptcp_sock *msk = mptcp_sk(sk);
> -	LIST_HEAD(conn_list);
>  
>  	pr_debug("msk=%p\n", msk);
>  
>  	might_sleep();
>  
> -	/* be sure to always acquire the join list lock, to sync vs
> -	 * mptcp_finish_join().
> +	/* join list will be eventually flushed (with rst) at sock lock
> +	 * release time; be sure to always acquire the join list lock, to
> +	 * sync vs mptcp_finish_join(). The actual subflow cleanup is now
> +	 * performed by mptcp_destroy_common() via sk->sk_prot->destroy().
>  	 */
>  	spin_lock_bh(&msk->join_list_lock);
>  	list_splice_tail_init(&msk->join_list, &msk->conn_list);
>  	spin_unlock_bh(&msk->join_list_lock);
> -	list_splice_init(&msk->conn_list, &conn_list);
>  
>  	sk_stop_timer(sk, &msk->sk.icsk_retransmit_timer);
>  	sk_stop_timer(sk, &sk->sk_timer);
>  	msk->pm.status = 0;
>  
> -	list_for_each_entry_safe(subflow, tmp, &conn_list, node) {
> -		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> -		__mptcp_close_ssk(sk, ssk, subflow);
> -	}
> -
>  	sk->sk_prot->destroy(sk);
>  
>  	WARN_ON_ONCE(msk->wmem_reserved);
> @@ -3122,12 +3116,17 @@ static struct sock *mptcp_accept(struct sock *sk, int flags, int *err,
>  	return newsk;
>  }
>  
> -void mptcp_destroy_common(struct mptcp_sock *msk)
> +void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags)

I can't see where `flags` is used in the function. On the other hand, cleaning
this up would be an "unforced" change.

Acked-by: Andrei Gherzan <andrei.gherzan at canonical.com>

>  {
> +	struct mptcp_subflow_context *subflow, *tmp;
>  	struct sock *sk = (struct sock *)msk;
>  
>  	__mptcp_clear_xmit(sk);
>  
> +	/* join list will be eventually flushed (with rst) at sock lock release time */
> +	list_for_each_entry_safe(subflow, tmp, &msk->conn_list, node)
> +		__mptcp_close_ssk(sk, mptcp_subflow_tcp_sock(subflow), subflow);
> +
>  	/* move to sk_receive_queue, sk_stream_kill_queues will purge it */
>  	skb_queue_splice_tail_init(&msk->receive_queue, &sk->sk_receive_queue);
>  
> @@ -3140,7 +3139,11 @@ static void mptcp_destroy(struct sock *sk)
>  {
>  	struct mptcp_sock *msk = mptcp_sk(sk);
>  
> -	mptcp_destroy_common(msk);
> +	/* clears msk->subflow, allowing the following to close
> +	 * even the initial subflow
> +	 */
> +	mptcp_dispose_initial_subflow(msk);
> +	mptcp_destroy_common(msk, 0);
>  	sk_sockets_allocated_dec(sk);
>  }
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 5492c67ecc9a..30d8760f9060 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -710,7 +710,7 @@ static inline void mptcp_write_space(struct sock *sk)
>  	}
>  }
>  
> -void mptcp_destroy_common(struct mptcp_sock *msk);
> +void mptcp_destroy_common(struct mptcp_sock *msk, unsigned int flags);
>  
>  #define MPTCP_TOKEN_MAX_RETRIES	4
>  
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 6dc26d9d283a..0a2336603550 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -616,7 +616,8 @@ static void mptcp_sock_destruct(struct sock *sk)
>  		sock_orphan(sk);
>  	}
>  
> -	mptcp_destroy_common(mptcp_sk(sk));
> +	/* We don't need to clear msk->subflow, as it's still NULL at this point */
> +	mptcp_destroy_common(mptcp_sk(sk), 0);
>  	inet_sock_destruct(sk);
>  }

-- 
Andrei Gherzan
gpg: rsa4096/D4D94F67AD0E9640
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20260915/05540769/attachment-0001.sig>