[PATCH] mptcp: pm: fix data race in add_addr timer callback

[PATCH] mptcp: pm: fix data race in add_addr timer callback

[luoqing]

From: luoqing <luoqing@kylinos.cn>

The timer callback reads entry->retrans_times outside pm.lock to decide
whether to call mptcp_pm_subflow_established(). Since
mptcp_pm_announced_del_timer() can concurrently set retrans_times =
ADD_ADDR_RETRANS_MAX under pm.lock, a race condition exists.

Use a local 'schedule_work' flag set inside pm.lock only when the timer
callback itself increments retrans_times to ADD_ADDR_RETRANS_MAX. This
ensures that mptcp_pm_subflow_established() is only called when the
retransmission naturally exhausts.

Additionally, use READ_ONCE/WRITE_ONCE for retrans_times and timer_done
to prevent compiler optimizations.

Signed-off-by: luoqing <luoqing@kylinos.cn>
---
 net/mptcp/pm.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6afd39aea110..4c039e7843d3 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -380,6 +380,8 @@ static void mptcp_pm_add_addr_timer(struct timer_list *timer)
 	struct mptcp_sock *msk = entry->sock;
 	struct sock *sk = (struct sock *)msk;
 	unsigned int timeout = 0;
+	bool completed = false;
+	u8 retrans_times;
 
 	pr_debug("msk=%p\n", msk);
 
@@ -399,27 +401,33 @@ static void mptcp_pm_add_addr_timer(struct timer_list *timer)
 
 	spin_lock_bh(&msk->pm.lock);
 
+	retrans_times = READ_ONCE(entry->retrans_times);
+
 	/* The cancel path (mptcp_pm_announced_del_timer()) can race with this
 	 * callback. Once cancel updates retrans_times to MAX, suppress further
 	 * retransmissions here. If this callback acquires pm.lock first, one
 	 * final transmit attempt is still possible.
 	 */
-	if (entry->retrans_times < ADD_ADDR_RETRANS_MAX &&
+	if (retrans_times < ADD_ADDR_RETRANS_MAX &&
 	    !mptcp_pm_should_add_signal_addr(msk)) {
 		pr_debug("retransmit ADD_ADDR id=%d\n", entry->addr.id);
 		mptcp_pm_announce_addr(msk, &entry->addr, false);
 		mptcp_pm_add_addr_send_ack(msk);
-		entry->retrans_times++;
+		retrans_times++;
+		WRITE_ONCE(entry->retrans_times, retrans_times);
 	}
 
-	if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
+	if (retrans_times >= ADD_ADDR_RETRANS_MAX)
+		completed = true;
+
+	if (retrans_times < ADD_ADDR_RETRANS_MAX)
 		timeout <<= entry->retrans_times;
 	else
 		timeout = 0;
 
 	spin_unlock_bh(&msk->pm.lock);
 
-	if (entry->retrans_times == ADD_ADDR_RETRANS_MAX)
+	if (completed)
 		mptcp_pm_subflow_established(msk);
 
 out:
-- 
2.25.1

Re: [PATCH] mptcp: pm: fix data race in add_addr timer callback

[MPTCP CI]

Hi luoqing,

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): Unstable: 1 failed test(s): packetdrill_sockopts ⚠️ 
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- 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/29393939925

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


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] mptcp: pm: fix data race in add_addr timer callback

[gang.yan]

July 15, 2026 at 2:05 PM, "luoqing" <l1138897701@163.com mailto:l1138897701@163.com?to=%22luoqing%22%20%3Cl1138897701%40163.com%3E > wrote:

Hi luoqin,

Thanks for your patch.

> 
> From: luoqing <luoqing@kylinos.cn>
> 
> The timer callback reads entry->retrans_times outside pm.lock to decide
> whether to call mptcp_pm_subflow_established(). Since
> mptcp_pm_announced_del_timer() can concurrently set retrans_times =
> ADD_ADDR_RETRANS_MAX under pm.lock, a race condition exists.
> 

@Matt,@geliang. AFAIK, this could happen when ADD_ADDR is echoed by the peer,
and at the same time the timer is woke up. 'mptcp_pm_add_addr_echoed' will call
'mptcp_pm_announced_del_timer' and set 'retrans_times' as 'ADD_ADDR_RETRANS_MAX',
but the timer callback then reads entry->retrans_times outside of pm.lock.

> Use a local 'schedule_work' flag set inside pm.lock only when the timer
> callback itself increments retrans_times to ADD_ADDR_RETRANS_MAX. This
> ensures that mptcp_pm_subflow_established() is only called when the
> retransmission naturally exhausts.
> 
> Additionally, use READ_ONCE/WRITE_ONCE for retrans_times and timer_done
> to prevent compiler optimizations.

@luo
'timer_done' is not fixed in your code, and 'schedule_work' is not seen
either, do you mean 'completed'?  Please make sure the commit message
matches the code.
> 
> Signed-off-by: luoqing <luoqing@kylinos.cn>
> ---
>  net/mptcp/pm.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 6afd39aea110..4c039e7843d3 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -380,6 +380,8 @@ static void mptcp_pm_add_addr_timer(struct timer_list *timer)
>  struct mptcp_sock *msk = entry->sock;
>  struct sock *sk = (struct sock *)msk;
>  unsigned int timeout = 0;
> + bool completed = false;
> + u8 retrans_times;
>  
>  pr_debug("msk=%p\n", msk);
>  
> @@ -399,27 +401,33 @@ static void mptcp_pm_add_addr_timer(struct timer_list *timer)
>  
>  spin_lock_bh(&msk->pm.lock);
>  
> + retrans_times = READ_ONCE(entry->retrans_times);
> +
>  /* The cancel path (mptcp_pm_announced_del_timer()) can race with this
>  * callback. Once cancel updates retrans_times to MAX, suppress further
>  * retransmissions here. If this callback acquires pm.lock first, one
>  * final transmit attempt is still possible.
>  */

Maybe just drop the comments here, I don't think it is worth explaining
separately.

> - if (entry->retrans_times < ADD_ADDR_RETRANS_MAX &&
> + if (retrans_times < ADD_ADDR_RETRANS_MAX &&
>  !mptcp_pm_should_add_signal_addr(msk)) {
>  pr_debug("retransmit ADD_ADDR id=%d\n", entry->addr.id);
>  mptcp_pm_announce_addr(msk, &entry->addr, false);
>  mptcp_pm_add_addr_send_ack(msk);
> - entry->retrans_times++;
> + retrans_times++;
> + WRITE_ONCE(entry->retrans_times, retrans_times);
>  }
>  
> - if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
> + if (retrans_times >= ADD_ADDR_RETRANS_MAX)
> + completed = true;
> +
> + if (retrans_times < ADD_ADDR_RETRANS_MAX)
>  timeout <<= entry->retrans_times;
>  else
>  timeout = 0;

These two if branch could be combined into one, like:

'''
if (retrans_times < ADD_ADDR_RETRANS_MAX)
        timeout <<= entry->retrans_times;
else {
        timeout = 0;
        completed = true;
}
'''
Note: this snippet is untested — please double-check the coding style
and make sure it compiles before submitting.

Thanks
Gang
>  
>  spin_unlock_bh(&msk->pm.lock);
>  
> - if (entry->retrans_times == ADD_ADDR_RETRANS_MAX)
> + if (completed)
>  mptcp_pm_subflow_established(msk);
>  
>  out:
> -- 
> 2.25.1
>