Re: [PATCH net v2 04/15] mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Incomplete fix: in mptcp_nl_remove_id_zero_address()
  (net/mptcp/pm_kernel.c) the new cleanup forces `anno_addr.port = 0`…
- [Medium] Newly introduced wrong-entry teardown in
  mptcp_nl_remove_id_zero_address() (net/mptcp/pm_kernel.c:1152-1157):…

--

mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0

This makes the in-kernel path manager's id 0 removal path symmetric with the
non-zero id path by dropping a pending ADD_ADDR announcement and decrementing
add_addr_signaled before queuing the RM_ADDR.  The lookup key is the msk local
address with the port forced to 0.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/620

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 424f1a7f9248..1a7750813235 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -1137,6 +1137,8 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
>  	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
>  		struct sock *sk = (struct sock *)msk;
>  		struct mptcp_addr_info msk_local;
> +		struct mptcp_addr_info anno_addr;
> +		bool announced;
>  
>  		if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk))
>  			goto next;
> @@ -1146,7 +1148,13 @@ static int mptcp_nl_remove_id_zero_address(struct net *net,
>  			goto next;
>  
>  		lock_sock(sk);
> +		/* Drop a possibly pending ADD_ADDR for this address. */
> +		anno_addr = msk_local;
> +		anno_addr.port = 0;
> +		announced = mptcp_pm_announced_remove(msk, &anno_addr);

[Severity: High]
Does zeroing the port here make the lookup miss the announcements this
patch is meant to remove?

The commit message says:

    Subtle detail: signal endpoints are stored in anno_list with port 0, but
    msk_local carries the connection's local port. In other words, entries
    linked to ID0 paths should have port == 0. A follow-up patch will ensure
    that.

Since that invariant does not exist yet, a SIGNAL-only endpoint keeps its
port.  mptcp_pm_nl_append_new_local_addr() only clears it when
address_use_port() is false:

	if (!address_use_port(entry))
		entry->addr.port = 0;

and address_use_port() returns true for SIGNAL-without-SUBFLOW endpoints.
mptcp_pm_create_subflow_or_signal_addr() then copies that endpoint verbatim
from select_signal_address() and only rewrites the id:

		endp_id = local.addr.id;
		if (endp_id == msk->mpc_endpoint_id)
			local.addr.id = 0;

		if (!mptcp_pm_announced_alloc(msk, &local.addr))
			return;

so the anno_list entry can be {A, port P, id 0}.  The removal side compares
with the port:

net/mptcp/pm.c:mptcp_pm_announced_lookup() {
	list_for_each_entry(entry, &msk->pm.anno_list, list) {
		if (mptcp_addresses_equal(&entry->addr, addr, true))
			return entry;
	}
}

msk_local is A:P and would have matched that entry exactly, but the zeroed
key does not.  In that case announced stays false, the entry stays linked and
add_addr_signaled is never decremented.

__mark_subflow_endp_available(msk, 0) then re-sets the endpoint's id bit, so
after another signal endpoint is added the PM reselects the same endpoint,
rewrites its id to 0, and mptcp_pm_announced_alloc() still finds the stale
entry:

net/mptcp/pm.c:mptcp_pm_announced_alloc() {
	add_entry = mptcp_pm_announced_lookup(msk, addr);
	if (add_entry) {
		if (WARN_ON_ONCE(mptcp_pm_is_kernel(msk)))
			return false;
}

Is the syzbot-reported warning therefore still reachable for port-based
signal endpoints, both here and in the stable backport of this patch alone?
A diff over the rest of the series shows nothing later touches
mptcp_nl_remove_id_zero_address() or normalizes anno_list ports.

Would matching on the entry id 0, or passing msk_local unmodified, be closer
to what the non-zero id path does?

> +		spin_lock_bh(&msk->pm.lock);
> +		if (announced)
> +			msk->pm.add_addr_signaled--;
>  		mptcp_pm_remove_addr(msk, &list);

[Severity: Medium]
Can the address-only key match and free the pending ADD_ADDR of a different
endpoint?

mptcp_pm_announced_remove() passes check_id=false, and in that case the id of
the found entry is never verified before it is unlinked and freed:

net/mptcp/pm.c:mptcp_pm_announced_del_timer() {
	entry = mptcp_pm_announced_lookup(msk, addr);
	if (entry && (!check_id || entry->addr.id == addr->id)) {
		entry->retrans_times = ADD_ADDR_RETRANS_MAX;
		stop_timer = true;
	}
	if (!check_id && entry)
		list_del(&entry->list);
}

Two endpoints with the same IP can coexist when they differ only by port,
because mptcp_pm_nl_append_new_local_addr() compares them with:

		if (mptcp_addresses_equal(&cur->addr, &entry->addr,
					  cur->addr.port || entry->addr.port)) {

So with E1 = {A, port P, SIGNAL} as the msk's mpc/ID0 endpoint and
E2 = {A, port 0, SIGNAL, id N} owning a pending announcement stored as
{A, port 0, id N}, the zeroed key matches E2's entry and destroys it.  The
same happens if msk->mpc_endpoint_id stayed 0 because no endpoint matched
when mptcp_mpc_endpoint_setup() ran.

The RM_ADDR queued right below carries only id 0 (list.ids[0] = 0), so the
peer is never told about id N.  __mark_subflow_endp_available(msk, 0) only
sets the bit for msk->mpc_endpoint_id:

	if (!__test_and_set_bit(id ? : msk->mpc_endpoint_id, msk->pm.id_avail_bitmap) &&

so id N stays cleared in id_avail_bitmap and is never re-announced, and a
later DEL_ADDR of endpoint N takes mptcp_pm_remove_announced() with
announced == false.  Do the local and peer views of announced addresses
diverge permanently in that case?

The peer call site keeps the key and the signalled id paired:

net/mptcp/pm_kernel.c:mptcp_pm_remove_announced() {
	list.ids[list.nr++] = mptcp_endp_get_local_id(msk, addr);

	announced = mptcp_pm_announced_remove(msk, addr);

Would matching on the entry's id 0 instead of a fabricated port preserve
that pairing here too?  The id 0 path touched no anno_list entry before this
patch, and this collateral teardown is not covered in the commit message.

>  		mptcp_pm_rm_subflow(msk, &list);
>  		__mark_subflow_endp_available(msk, 0);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-net-mptcp-misc-fixes-7-3-rc1-v2-0-df1de70348b6%40kernel.org