[PATCH net v3] mptcp: pm: drop pending ADD_ADDR when removing id 0 endpoint

[PATCH net v3] mptcp: pm: drop pending ADD_ADDR when removing id 0 endpoint

[Kalpan Jani]

syzkaller hit the WARN_ON_ONCE() in mptcp_pm_alloc_anno_list() with
the in-kernel path manager.

When a signal endpoint is removed, the pending ADD_ADDR has to be
cancelled: its retransmit timer stopped and the anno_list entry
unlinked and freed. For a non-zero id endpoint this is done via
mptcp_nl_remove_subflow_and_signal_addr() ->
mptcp_pm_remove_anno_addr() -> mptcp_remove_anno_list_by_saddr().

The id 0 removal path, mptcp_nl_remove_id_zero_address(), does not do
this: it only queues a RM_ADDR and marks the id available again, but
leaves any pending anno_list entry and its armed retransmit timer
alive.

So when the id 0 endpoint is removed and re-added while its previously
sent ADD_ADDR is still awaiting the echo, the stale entry survives.
The kernel PM reselects id 0, reaches mptcp_pm_alloc_anno_list() a
second time, finds the stale entry and hits the WARN.

Make the id 0 removal path symmetric with the non-zero one: drop the
pending ADD_ADDR before queuing the RM_ADDR, and decrement
add_addr_signaled if the address had been announced. This closes the
race at its source, so the WARN_ON_ONCE() stays a valid assertion.

Fixes: 740d798e8767 ("mptcp: remove id 0 address")
Reported-by: syzbot+55c2a5c871441261ed14@syzkaller.appspotmail.com
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/620
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
---
v1 -> v2:
 - Do not drop the WARN_ON_ONCE(); it is a valid assertion.
   (Matthieu Baerts)
 - Fix mptcp_nl_remove_id_zero_address() to tear down the pending
   ADD_ADDR entry: call mptcp_remove_anno_list_by_saddr() and
   decrement add_addr_signaled, mirroring the non-zero id path.
   (Matthieu Baerts)
v2 -> v3:
 - Shorten the inline comment to one line; full context is in
   the commit message. (Matthieu Baerts)

 net/mptcp/pm_kernel.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index fc818b63752e..128a2e71c26e 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -1126,6 +1126,7 @@ 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;
+		bool announced;
 
 		if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk))
 			goto next;
@@ -1135,7 +1136,11 @@ 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. */
+		announced = mptcp_remove_anno_list_by_saddr(msk, &msk_local);
 		spin_lock_bh(&msk->pm.lock);
+		if (announced)
+			msk->pm.add_addr_signaled--;
 		mptcp_pm_remove_addr(msk, &list);
 		mptcp_pm_rm_subflow(msk, &list);
 		__mark_subflow_endp_available(msk, 0);
-- 
2.43.0

Re: [PATCH net v3] mptcp: pm: drop pending ADD_ADDR when removing id 0 endpoint

[MPTCP CI]

Hi Kalpan,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_mptcp_connect_checksum ⚠️ 
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/26628789956

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/6e2e266d3b4b
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1102741


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH net v3] mptcp: pm: drop pending ADD_ADDR when removing id 0 endpoint

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

Hi Kalpan,

First of all, thanks for this patch!

While playing with it on top of the current tree, I think I may have
bumped into a small detail, and I just wanted to mention it in case it
is useful — please do correct me if I got any of this wrong.

After the "mptcp: pm: uniform announced addresses helpers" refactor
(7d4dacc8ccca), the helper became mptcp_pm_announced_remove(), and its
lookup uses mptcp_addresses_equal(..., /* use_port = */ true). A pending
ADD_ADDR entry coming from a signal endpoint added without a port
(`ip mptcp endpoint add ADDR signal`) is keyed with port 1, while
msk_local carries the connection's local port. With a listener on port
5000, a temporary printk in the id 0 path shows:

    msk_local.port = 5000
    announced(port0_lookup) = 1

So, at least in this configuration, passing msk_local directly does not
match the entry: the stale entry survives, and the WARN still shows up
on the next reselection. It only goes away when the port is ignored
during the lookup.

If that matches what you see, the smallest change that keeps the v3
shape would be to clear the port before the lookup, something like:

        struct mptcp_addr_info anno_addr = msk_local;

        anno_addr.port = 0;
        announced = mptcp_pm_announced_remove(msk, &anno_addr);

(An address-only variant of the lookup might be a touch cleaner, since a
signal entry never carries the connection port — but the one-liner above
is enough to fix the case I reproduced.)

I have a small repro that triggers it deterministically if it would help
— happy to share.

Thanks again for looking into this, and sorry for the noise if I'm
missing something obvious here.

Cheers,
Tao