Re: [PATCH net v5 2/2] mptcp: fix MP_CAPABLE token migration when cloning reqsk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Ignoring the return value of mptcp_init_sched() in mptcp_sk_clone_init() causes a module refcount underflow.
--

commit 18a63fbca98d3e69ad5d14656be7de91ee0af91e
Author: Ruide Cao <caoruide123@gmail.com>

mptcp: fix MP_CAPABLE token migration when cloning reqsk

The patch fixes MP_CAPABLE token migration during TCP request migration by
moving token request ownership from the original request to the clone
under the token bucket lock. It also safely destroys the provisional MPTCP
socket if a passive MP_CAPABLE socket cannot claim the token.

Closes: https://sashiko.dev/#/patchset/86e2514b533bf4d55d4aa2fdbf1404022e8c9430.1776149210.git.caoruide123%40gmail.com

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e1f08f71cdb16..30067f2a7ca8f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -3764,6 +3777,14 @@ struct sock *mptcp_sk_clone_init(const struct sock *sk,
>  	mptcp_rcv_space_init(msk, ssk);
>  	msk->rcvq_space.time = mptcp_stamp();
>  
> +	if (!mptcp_token_accept(subflow_req, msk)) {
> +		list_del_init(&subflow->node);
> +		WRITE_ONCE(msk->first, NULL);
> +		sock_put(ssk);
> +		mptcp_sk_clone_destroy(nsk);

[Severity: Critical]
This isn't a bug introduced by this patch, but could this error path expose a
module refcount underflow?

Earlier in mptcp_sk_clone_init(), the code calls mptcp_init_sched() without
checking its return value:

	msk->setsockopt_seq = mptcp_sk(sk)->setsockopt_seq;
	mptcp_init_sched(msk, mptcp_sk(sk)->sched);

Because sk_clone_lock() byte-copies the parent's msk->sched pointer, if the
BPF scheduler module is concurrently unloading, bpf_try_module_get() can fail
and return -EBUSY. This leaves a stale pointer in msk->sched.

When mptcp_sk_clone_destroy() is called here, it triggers
mptcp_release_sched(). Will this cause a use-after-free and refcount
underflow by unconditionally calling sched->release() and
bpf_module_put() on that stale scheduler pointer?

> +		return NULL;
> +	}
> +
>  	if (mp_opt->suboptions & OPTION_MPTCP_MPC_ACK)
>  		__mptcp_subflow_fully_established(msk, subflow, mp_opt);
>  	bh_unlock_sock(nsk);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788202924.git.caoruide123@gmail.com?part=2