Re: [PATCH mptcp-next v2 3/3] mptcp: pm: userspace: cap extra_subflows on Netlink subflow creation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the error path unconditionally deletes and frees a local address entry, even if the address was pre-existing and shared among multiple subflows.
--

commit 147f86e2aec9bdeead6dcc987672ad229b2286ec
Author: Quanye Yang <quanyeyang@proton.me>

mptcp: pm: userspace: cap extra_subflows on Netlink subflow creation

mptcp_pm_nl_subflow_create_doit() also increments the u8 extra_subflows
counter without any limit: a client with CAP_NET_ADMIN can create more
than 255 extra subflows via Netlink and wrap the counter back to 0,
with the same effects as the MP_JOIN path now bounded separately:
the underflow guards warn on the next subflow close, and
mptcpi_subflows_total reported to userspace is corrupted.

Link: https://lore.kernel.org/all/20260902-mptcp-pm-extra-subflows-v1-1-68540a866e5a@proton.me/

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index f723a134356f3..bede75a2e2586 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -427,6 +427,12 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
>  	local.ifindex = entry.ifindex;
>  
>  	spin_lock_bh(&msk->pm.lock);
> +	if (msk->pm.extra_subflows == U8_MAX) {
> +		spin_unlock_bh(&msk->pm.lock);
> +		GENL_SET_ERR_MSG(info, "too many extra subflows");
> +		err = -ENOSPC;
> +		goto delete_addr;
> +	}
>  	msk->pm.extra_subflows++;
>  	spin_unlock_bh(&msk->pm.lock);
>  
> @@ -434,9 +440,11 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
>  	err = __mptcp_subflow_connect(sk, &local, &addr_r);
>  	release_sock(sk);
>  
> -	if (err) {
> +	if (err)
>  		GENL_SET_ERR_MSG_FMT(info, "connect error: %d", err);
>  
> +delete_addr:
> +	if (err) {
>  		spin_lock_bh(&msk->pm.lock);
>  		mptcp_userspace_pm_delete_local_addr(msk, &entry);

[Severity: High]
This is a pre-existing issue, but does this error path incorrectly delete a
shared local address entry if the address was pre-existing?

When mptcp_userspace_pm_append_new_local_addr() is called earlier in
mptcp_pm_nl_subflow_create_doit(), it might match an already existing local
address instead of appending a new one.

If the newly added extra_subflows limit is hit (or if
__mptcp_subflow_connect() fails), the code jumps to this delete_addr error
path.

Because mptcp_userspace_pm_append_new_local_addr() did not allocate a new
entry but simply reused the existing one, unconditionally calling
mptcp_userspace_pm_delete_local_addr() prematurely frees the shared address
and decrements the local_addr_used counter, potentially corrupting the state
for other subflows relying on it.

>  		spin_unlock_bh(&msk->pm.lock);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903131416.1112393-1-cui.tao@linux.dev?part=3