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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardcoded `anno_addr.port = 0` fails to cancel the pending `ADD_ADDR` if the ID 0 endpoint was configured with a specific port.

Pre-existing issues:
- [High] Canceling an `ADD_ADDR` fails to clear `MPTCP_ADD_ADDR_SIGNAL`, silently dropping `RM_ADDR` and erroneously transmitting a canceled `ADD_ADDR`.
- [High] Calling `sk_stop_timer_sync()` while holding `rcu_read_lock()` triggers a sleep-in-atomic bug on PREEMPT_RT kernels.
--

--- Patch [4]: [PATCH net 04/14] mptcp: pm: kernel: drop pending ADD_ADDR when removing ID0 ---
commit 8ec4a41a6bf3f96bdf00a1088430d4fcabb37f97
Author: Kalpan Jani <kalpan.jani@mpiricsoftware.com>

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

The in-kernel MPTCP path manager can leave a stale ADD_ADDR announcement
entry alive when removing the id 0 endpoint. This happens because the id 0
removal path does not tear down pending announcements, unlike the non-zero
id path.

Cc: stable@vger.kernel.org
Reported-by: syzbot+55c2a5c871441261ed14@syzkaller.appspotmail.com
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/620
Suggested-by: Tao Cui <cuitao@kylinos.cn>
Signed-off-by: Kalpan Jani <kalpan.jani@mpiricsoftware.com>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 424f1a7f92482..1a77508132354 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -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;

[Severity: High]
Does hardcoding the port to 0 here prevent mptcp_pm_announced_remove() from
finding the pending announcement if the ID 0 endpoint was configured with a
specific port?

If a signal endpoint matches the initial subflow's IP but has a non-zero
port, the PM maps it to ID 0 and stores it in anno_list with the non-zero
port. Because mptcp_pm_announced_remove() enforces strict port matching,
it seems it would fail to find the announcement, leaving a stale ADD_ADDR
timer that could trigger a WARN_ON_ONCE.

> +		announced = mptcp_pm_announced_remove(msk, &anno_addr);

[Severity: High]
This is a pre-existing issue, but does calling mptcp_pm_announced_remove()
trigger a sleep-in-atomic bug on PREEMPT_RT kernels?

In net/mptcp/pm.c:mptcp_pm_announced_del_timer(), which is called by
mptcp_pm_announced_remove(), there is a call to sk_stop_timer_sync()
inside an RCU read-side critical section:

mptcp_pm_announced_del_timer()
    rcu_read_lock();
    ...
    sk_stop_timer_sync(sk, &entry->timer);

On PREEMPT_RT, timer_delete_sync() can sleep, which is not allowed inside
an RCU critical section.

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

[Severity: High]
This is also a pre-existing issue, but does canceling an ADD_ADDR fail to
clear the signal bit, causing the RM_ADDR to be dropped here?

When mptcp_pm_announced_del_timer() deletes the timer and removes the entry,
it does not clear the corresponding BIT(MPTCP_ADD_ADDR_SIGNAL) in
msk->pm.addr_signal. 

Then, when mptcp_pm_remove_addr() is called, it checks if rm_addr is
non-zero (which it is, because the signal bit is still set):

mptcp_pm_remove_addr()
    if (rm_addr) {
        MPTCP_ADD_STATS(...);
        return -EINVAL;
    }

Would this cause the RM_ADDR request to be silently dropped, while
erroneously transmitting the canceled ADD_ADDR?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824-net-mptcp-misc-fixes-7-3-rc1-v1-0-a92309d121ca@kernel.org?part=4