Re: [PATCH mptcp-net v4 1/8] mptcp: pm: userspace: properly handle the ID0 case

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: pm: userspace: ID 0 entry breaks mptcp_pm_userspace_get_local_id()
--

commit 626a00aaeaa0d778f8ba3b1e9d1ca84367bbdac3
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: pm: userspace: properly handle the ID0 case

This commit adds the initial subflow's local address (with ID 0 and port 0)
to the userspace path manager's local address list when a connection is
created so that ID 0 can be handled uniformly with other local addresses.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index f723a134356f3..45faee894ac3d 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -707,6 +707,26 @@ int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
>  	return ret;
>  }
>  
> +/* Add the initial local address (ID0) to the local list: easier that way */
> +void mptcp_pm_userspace_created(struct mptcp_sock *msk, const struct sock *ssk)
> +{
> +	struct mptcp_pm_addr_entry *entry;
> +
> +	entry = sock_kmalloc((struct sock *)msk, sizeof(*entry), GFP_ATOMIC);
> +	/* Fine not to handle the ID0 case in memory pressure */
> +	if (!entry)
> +		return;
> +
> +	memset(entry, 0, sizeof(*entry));
> +	mptcp_local_address((struct sock_common *)ssk, &entry->addr);
> +	entry->addr.port = 0; /* msk port */
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);
> +	/* The initial address ID doesn't increment local_addr_used */
> +	spin_unlock_bh(&msk->pm.lock);
> +}

[Severity: High]
Does adding an entry with entry->addr.id == 0 to
msk->pm.userspace_pm_local_addr_list cause a regression in
mptcp_pm_userspace_get_local_id() when allocating a local ID for a new,
unannounced address?

When mptcp_pm_get_local_id() -> mptcp_pm_userspace_get_local_id() is called
for a subflow whose local IP address differs from the initial subflow's IP
address and is not yet in userspace_pm_local_addr_list, skc->addr.id is 0 and
it calls mptcp_userspace_pm_append_new_local_addr(msk, skc, true):

net/mptcp/pm_userspace.c:mptcp_pm_userspace_get_local_id() {
	...
	if (skc->addr.port == msk_sport)
		skc->addr.port = 0;

	return mptcp_userspace_pm_append_new_local_addr(msk, skc, true);
}

Inside mptcp_userspace_pm_append_new_local_addr(), the loop iterates over
userspace_pm_local_addr_list and encounters the initial ID 0 entry added by
mptcp_pm_userspace_created():

net/mptcp/pm_userspace.c:mptcp_userspace_pm_append_new_local_addr() {
	...
	mptcp_for_each_userspace_pm_addr(msk, e) {
		addr_match = mptcp_addresses_equal(&e->addr, &entry->addr, true);
		if (addr_match && entry->addr.id == 0 && needs_id)
			entry->addr.id = e->addr.id;
		id_match = (e->addr.id == entry->addr.id);
		if (addr_match || id_match)
			break;
		__set_bit(e->addr.id, id_bitmap);
	}

	if (!addr_match && !id_match) {
		...
	} else if (addr_match && id_match) {
		ret = entry->addr.id;
	}

append_err:
	spin_unlock_bh(&msk->pm.lock);
	return ret;
}

Because the new address does not match the initial subflow's address,
addr_match is false and entry->addr.id remains 0. However, e->addr.id is
also 0 on the initial entry, so id_match becomes true and the loop breaks
with addr_match == false and id_match == true, causing the function to
return -EINVAL instead of assigning a new ID from id_bitmap.

Although the v2 changelog notes mention handling needs_id where the addresses
do not match but the IDs (0) do, that check does not appear to be present in
this commit or later in the series.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292@kernel.org?part=1