Re: [PATCH net 08/14] mptcp: pm: reset retrans_time when ADD_ADDR entry is reused

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
mptcp: pm: reset retrans_time when ADD_ADDR entry is reused

This moves the retrans_times reset from the fresh-allocation path into the
shared reset_timer block of mptcp_pm_announced_alloc(), so a reused
ADD_ADDR entry gets its retransmission budget back when the timer is
re-armed.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260803-net-mptcp-misc-fixes-7-2-rc6-v2-0-b8f496d71664%40kernel.org?part=4

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 8b68868255c50..b0b71adefb8fe 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -462,10 +462,10 @@ bool mptcp_pm_announced_alloc(struct mptcp_sock *msk,
>  
>  	add_entry->addr = *addr;
>  	add_entry->sock = msk;
> -	add_entry->retrans_times = 0;
>  
>  	timer_setup(&add_entry->timer, mptcp_pm_add_addr_timer, 0);
>  reset_timer:
> +	add_entry->retrans_times = 0;
>  	add_entry->timer_done = false;
>  	timeout = mptcp_adjust_add_addr_timeout(msk);
>  	if (timeout)

[Severity: High]
This isn't a bug introduced by this patch, since the reset_timer block
already contained the timer_done = false plus sk_reset_timer() pair, but
since the patch adds another store into exactly this block, is the block
sufficiently serialized against the timer callback?

The reuse path runs from mptcp_pm_nl_announce_doit(), which holds
lock_sock(sk) and msk->pm.lock:

net/mptcp/pm_userspace.c:mptcp_pm_nl_announce_doit() {
	lock_sock(sk);
	spin_lock_bh(&msk->pm.lock);

	if (mptcp_pm_announced_alloc(msk, &addr_val.addr)) {
	...
}

mptcp_pm_add_addr_timer() writes timer_done under bh_lock_sock() only, and
its TCP_CLOSE early-out comes before the sock_owned_by_user() deferral:

net/mptcp/pm.c:mptcp_pm_add_addr_timer() {
	bh_lock_sock(sk);
	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
		goto out;

	if (sock_owned_by_user(sk)) {
		/* Try again later. */
		timeout = HZ / 20;
		goto out;
	}
	...
out:
	if (timeout)
		sk_reset_timer(sk, timer, jiffies + timeout);
	else
		/* if sock_put calls sk_free: avoid waiting for this timer */
		entry->timer_done = true;
	bh_unlock_sock(sk);
	sock_put(sk);
}

In TCP_CLOSE the callback reaches out: with timeout == 0 and stores
timer_done = true even when a process-context task owns the socket, since
lock_sock() only sets the owned flag and bh_lock_sock() still succeeds.
The reuse path does not take slock and the callback does not take pm.lock
around that store, so can the two stores interleave like this?

CPU0 (MPTCP_PM_CMD_ANNOUNCE re-announce, msk in TCP_CLOSE)
mptcp_pm_announced_alloc()
    add_entry->timer_done = false;
    sk_reset_timer()   /* timer becomes pending again */

CPU1 (previously armed timer expiring)
mptcp_pm_add_addr_timer()
    bh_lock_sock(sk);
    inet_sk_state_load(sk) == TCP_CLOSE -> goto out
    entry->timer_done = true;   /* lands after CPU0 re-armed */

That would leave the timer pending with timer_done == true, and teardown
via mptcp_pm_destroy()->mptcp_pm_free_announced_list() skips the sync stop
in that case:

net/mptcp/pm.c:mptcp_pm_free_announced_list() {
	list_for_each_entry_safe(entry, tmp, &free_list, list) {
		if (!entry->timer_done)
			sk_stop_timer_sync(sk, &entry->timer);
		kfree_rcu(entry, rcu);
	}
}

Can the still-queued timer then fire on the freed entry after the RCU
grace period, reading entry->sock and calling sock_put() on it? The
remaining expiry can be as long as the add_addr_timeout value, which is
much longer than a grace period.

For reachability, mptcp_pm_announced_alloc() only rejects
MPTCP_PM_DESTROYING, and mptcp_pm_nl_announce_doit() does no socket-state
check, so an msk sitting in TCP_CLOSE with a live token and an open fd
still reaches the reuse path.

Would stopping the timer (or taking bh_lock_sock()) before re-arming in
the reset_timer block, or moving the TCP_CLOSE check after the
sock_owned_by_user() check in mptcp_pm_add_addr_timer(), close this?

Also noting that no later patch in this series touches net/mptcp/pm.c, so
this state persists at the end of the series.