Re: [PATCH mptcp-net v4 6/8] mptcp: pm: userspace: don't dump initial ID0

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: pm: userspace: is_init_id0() skips re-added ID 0 without port
--

commit 06caddae470be561eadf1499e52b7564e7ab99b8
Author: Matthieu Baerts (NGI0) <matttbe@kernel.org>

mptcp: pm: userspace: don't dump initial ID0

This patch skips the initial ID 0 address entry in
mptcp_userspace_pm_dump_addr() by checking is_init_id0(entry) so that it is
not dumped when the connection is first created, while intending to allow it
to be dumped if removed and re-added later.

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 0f6400bc3bf8e..282d7f57c32ab 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -637,7 +637,7 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  	lock_sock(sk);
>  	spin_lock_bh(&msk->pm.lock);
>  	mptcp_for_each_userspace_pm_addr(msk, entry) {
> -		if (test_bit(entry->addr.id, bitmap->map))
> +		if (is_init_id0(entry) || test_bit(entry->addr.id, bitmap->map))
>  			continue;

[Severity: Medium]
Does is_init_id0(entry) also skip a re-added ID 0 address when userspace does
not specify an explicit local port?

In net/mptcp/pm_userspace.c, is_init_id0() checks:

static bool is_init_id0(struct mptcp_pm_addr_entry *entry)
{
	return entry->addr.id == 0 && entry->addr.port == 0;
}

When the initial ID 0 entry is removed via mptcp_pm_nl_remove_doit() or
mptcp_pm_nl_subflow_destroy_doit(), and later re-added with a new subflow via
mptcp_pm_nl_subflow_create_doit() (or announced via
mptcp_pm_nl_announce_doit()) without the optional MPTCP_PM_ADDR_ATTR_PORT
attribute on the local address, mptcp_pm_parse_entry() leaves
entry.addr.port == 0:

net/mptcp/pm_userspace.c:mptcp_pm_nl_subflow_create_doit() {
	...
	err = mptcp_pm_parse_entry(laddr, info, true, &entry);
	...
	entry.flags |= MPTCP_PM_ADDR_FLAG_SUBFLOW;
	...
	err = mptcp_userspace_pm_append_new_local_addr(msk, &entry, false);
	...
	local.addr = entry.addr;
	local.flags = entry.flags;
	local.ifindex = entry.ifindex;
	...
	err = __mptcp_subflow_connect(sk, &local, &addr_r);
}

Because mptcp_userspace_pm_append_new_local_addr() stores the entry before
__mptcp_subflow_connect() runs on a stack copy, the entry in
msk->pm.userspace_pm_local_addr_list retains entry->addr.id == 0 and
entry->addr.port == 0.

When mptcp_userspace_pm_dump_addr() subsequently iterates over the list,
wouldn't is_init_id0(entry) return true for this re-added ID 0 entry and skip
it during the dump, contrary to the commit description?

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