Re: [PATCH mptcp-net v4 4/8] mptcp: pm: userspace: allow announcing ID0 addr

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says an ANNOUNCE with ID 0 and a non-
  matching address 'will be returned' an error by…
- [Medium] On the active side, mptcp_connect() calls
  mptcp_token_new_connect() (protocol.c), which adds the msk to the…
- [Low] At this commit, MPTCP_PM_CMD_ANNOUNCE with ID 0 for the initial
  address succeeds.

--

mptcp: pm: userspace: allow announcing ID0 addr

This commit removes the explicit rejection of address ID 0 in
mptcp_pm_nl_announce_doit(), so a userspace PM can re-announce the
initial address with ID 0. To catch an ID 0 used with a different
address, it relies on mptcp_userspace_pm_append_new_local_addr()
returning an error.

Link: https://datatracker.ietf.org/doc/html/rfc8684#section-3.4.1-13 [1]

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 74969b86fb812..66dbf67fd6d80 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -239,12 +239,6 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
>  	if (err < 0)
>  		goto announce_err;
>  
> -	if (addr_val.addr.id == 0) {
> -		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid addr id");
> -		err = -EINVAL;
> -		goto announce_err;
> -	}
> -

[Severity: Medium]

The commit message says:

  If the ID is set to 0, but the address doesn't match with the existing
  one, an error will be returned by
  mptcp_userspace_pm_append_new_local_addr().

Is that still true when the ID0 entry is missing from
userspace_pm_local_addr_list but the initial subflow is still alive?

Here needs_id is false and the list has no ID0 entry. So
mptcp_userspace_pm_append_new_local_addr() sets neither addr_match nor
id_match, and it takes this branch:

		} else {
			id = entry->addr.id;
		}
		...
		e->addr.id = id;
		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);

That adds a different address B with ID 0. mptcp_pm_nl_announce_doit()
then calls mptcp_pm_announced_alloc() and mptcp_pm_announce_addr(). The
result is ADD_ADDR id 0 for B while the initial subflow still uses ID 0
on address A, and no RM_ADDR is sent first.

The entry can go missing in at least two ways. The first is when
mptcp_pm_userspace_created() fails to allocate it. sock_kmalloc() also
fails when optmem_max is exceeded:

	entry = sock_kmalloc((struct sock *)msk, sizeof(*entry), GFP_ATOMIC);
	/* Fine not to handle the ID0 case in memory pressure */
	if (!entry)
		return;

The second is a failed MPTCP_PM_CMD_SUBFLOW_CREATE from the initial
address A with port 0 and id 0. That request matches the ID0 entry. If
__mptcp_subflow_connect() then fails (for example -ENETUNREACH or a bind
error), the error path in mptcp_pm_nl_subflow_create_doit() runs:

	if (err) {
		...
		mptcp_userspace_pm_delete_local_addr(msk, &entry);
		...
	}

That lookup is by address, so it frees the existing ID0 entry. A
SUBFLOW_DESTROY of a secondary subflow from A can delete it the same way,
through the port-0 wildcard match in mptcp_userspace_pm_lookup_addr().

Before this patch, the addr_val.addr.id == 0 check rejected these
requests no matter what the list held. The later patches in the series
don't seem to change these paths.

[Severity: Medium]

Can this also leave two ID 0 entries in the list on the active side?

mptcp_connect()->mptcp_token_new_connect() adds the msk to the token hash
at connect() time. From that point mptcp_userspace_pm_get_sock() can find
it, and the token can be read through MPTCP_INFO or mptcp_diag. The ID0
entry is only added later, when the SYN-ACK is processed:

mptcp_finish_connect()
  mptcp_pm_new_connection()
    mptcp_pm_userspace_created()

During that window userspace_pm_local_addr_list is empty. An ANNOUNCE
with ID 0 for any address B gets through
mptcp_userspace_pm_append_new_local_addr() and stays in the list.

When the connection is established, mptcp_pm_userspace_created() appends
the real initial address A with ID 0. It does not check for an existing
ID 0 entry:

	spin_lock_bh(&msk->pm.lock);
	list_add_tail_rcu(&entry->list, &msk->pm.userspace_pm_local_addr_list);

The list then holds two different addresses with ID 0.
mptcp_userspace_pm_lookup_addr_by_id(0) would return B.

Before this patch, ANNOUNCE with id 0 was always rejected. The same
request through SUBFLOW_CREATE cleans up after itself: there,
__mptcp_subflow_connect() returns -ENOTCONN and the error path deletes
the entry. This state can still be reached at the end of the series.

[Severity: Low]

With this patch alone, an ANNOUNCE with ID 0 for the initial address goes
through mptcp_pm_announced_alloc(). That allocates an anno entry, arms
the ADD_ADDR retransmission timer and increments add_addr_signaled. A
REMOVE with id 0 still takes the shortcut in mptcp_pm_nl_remove_doit():

	if (id_val == 0) {
		err = mptcp_userspace_pm_remove_id_zero_address(msk);
		goto out;
	}

mptcp_userspace_pm_remove_id_zero_address() only calls
mptcp_pm_remove_addr(). It never calls mptcp_pm_announced_remove() or
changes add_addr_signaled, and it returns -EINVAL if no subflow has
local_id 0.

Could mptcp_pm_add_addr_timer() then resend ADD_ADDR id 0 after RM_ADDR
id 0 has been sent, with add_addr_signaled drifting as well?

The later patch "mptcp: pm: userspace: no ID0 exception for RM_ADDR"
seems to fix this. It removes the shortcut, so id 0 goes through
mptcp_userspace_pm_remove_addr_entry(), which calls
mptcp_pm_announced_remove() and adjusts add_addr_signaled. Only the state
between the two patches is affected, or a backport that takes this patch
without the next one.

Would it make sense to put the RM_ADDR patch before this one in the
series?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029-mptcp-pm-userspace-id0-case-v4-0-555066fa4292%40kernel.org