Re: [PATCH mptcp-net] mptcp: pm: cap userspace extra_subflows at U8_MAX

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Error paths in mptcp_pm_nl_subflow_create_doit unconditionally delete pre-existing local addresses, corrupting state and underflowing local_addr_used.
--

commit 09af196be93604649e0aad5970acd8810bf5dcca
Author: Quanye Yang <quanyeyang@proton.me>

mptcp: pm: cap userspace extra_subflows at U8_MAX

The userspace PM increments extra_subflows with no upper bound. The
field is a u8, so the 256th extra subflow wraps the counter to 0 and
the next close hits WARN_ON_ONCE().

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/629

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index fab16d953dbfe..6d798dd967029 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -427,16 +427,22 @@ 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);
> -	msk->pm.extra_subflows++;
> -	spin_unlock_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;
> +	} else {
> +		msk->pm.extra_subflows++;
> +		spin_unlock_bh(&msk->pm.lock);
>  
> -	lock_sock(sk);
> -	err = __mptcp_subflow_connect(sk, &local, &addr_r);
> -	release_sock(sk);
> +		lock_sock(sk);
> +		err = __mptcp_subflow_connect(sk, &local, &addr_r);
> +		release_sock(sk);
> +		if (err)
> +			GENL_SET_ERR_MSG_FMT(info, "connect error: %d", err);
> +	}
>  
>  	if (err) {
> -		GENL_SET_ERR_MSG_FMT(info, "connect error: %d", err);
> -
>  		spin_lock_bh(&msk->pm.lock);
>  		mptcp_userspace_pm_delete_local_addr(msk, &entry);
>  		spin_unlock_bh(&msk->pm.lock);

[Severity: High]
This is a pre-existing issue, but does this error path incorrectly delete
pre-existing local addresses?

If a user issues MPTCP_PM_CMD_SUBFLOW_CREATE using a local address that was
already appended, mptcp_userspace_pm_append_new_local_addr() returns success
without allocating a new entry or incrementing local_addr_used.

However, if the new subflow connection fails (or if the newly introduced
extra_subflows == U8_MAX limit is hit), the error handling path blindly invokes
mptcp_userspace_pm_delete_local_addr(msk, &entry) inside 
mptcp_pm_nl_subflow_create_doit().

Will this inadvertently destroy pre-existing PM state for existing subflows
relying on that address, and underflow the 8-bit msk->pm.local_addr_used
counter since it wasn't incremented?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902-mptcp-pm-extra-subflows-v1-1-68540a866e5a@proton.me?part=1