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

[PATCH mptcp-net v2] 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 'completed' 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.

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6afd39aea110..ed39a1241ffd 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,32 @@ 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)
-		timeout <<= entry->retrans_times;
-	else
+	if (retrans_times < ADD_ADDR_RETRANS_MAX)
+		timeout <<= retrans_times;
+	else {
 		timeout = 0;
+		completed = true;
+	}
 
 	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-net v2] 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_fastclose ⚠️ 
- 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/29561487863

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


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

[gang.yan]

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


> 
> 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 'completed' 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.
> 
> Signed-off-by: luoqing <luoqing@kylinos.cn>
> ---
>  net/mptcp/pm.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 6afd39aea110..ed39a1241ffd 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,32 @@ 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)
> - timeout <<= entry->retrans_times;
> - else
> + if (retrans_times < ADD_ADDR_RETRANS_MAX)
> + timeout <<= retrans_times;
> + else {
>  timeout = 0;
> + completed = true;
> + }
Hi, 

'checkpatch' has reported a 'checks' for this:

'''
CHECK: braces {} should be used on all arms of this statement
#60: FILE: net/mptcp/pm.c:420:
'''

But I think a v3 is not necessary for such a minor change. The patch
itself looks good to me！

It's okay to wait for other reviewers' feedback.

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
>

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

[Matthieu Baerts]

Hi,

Thank you for sharing this patch.

I have similar comments than the ones I sent on:

https://lore.kernel.org/178410664361.3802972.16804879942885801851.b4-review@b4

On 17/07/2026 08:40, luoqing wrote:
> 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.

How did you find the bug? Do you have a reproducer or is it by analysing
the code?

Were you assisted by a tool/LLM? If yes, please add the Assisted-by tag.

> Use a local 'completed' 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.

A fix should have a Fixes tag, please add one.

> Signed-off-by: luoqing <luoqing@kylinos.cn>

For legal reasons, you are supposed to put your full name. Having only
one "word" for your full name, without capital letters looks wrong, no?

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 6afd39aea110..ed39a1241ffd 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;

Do you need two new variables? One should be enough.

>  
>  	pr_debug("msk=%p\n", msk);
>  
> @@ -399,27 +401,32 @@ static void mptcp_pm_add_addr_timer(struct timer_list *timer)
>  
>  	spin_lock_bh(&msk->pm.lock);
>  
> +	retrans_times = READ_ONCE(entry->retrans_times);

Why do you require READ_ONCE() here? Can retrans_times be modified
without the pm lock?

> +
>  	/* 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);

Same here, why WRITE_ONCE()?
>  	}
>  
> -	if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
> -		timeout <<= entry->retrans_times;
> -	else
> +	if (retrans_times < ADD_ADDR_RETRANS_MAX)
> +		timeout <<= retrans_times;
> +	else {
>  		timeout = 0;
> +		completed = true;
> +	}

Either you have:

  completed = entry->retrans_times >= ADD_ADDR_RETRANS_MAX;
  if (!completed)
      timeout <<= retrans_times;
  else
      timeout = 0;

(preferred)

or

  if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
      timeout <<= retrans_times;
  else
      timeout = 0;

  retrans_times = entry->retrans_times;

But I don't think you need to have both "completed" and "retrans_times".
With only "completed", you can minimise this patch.

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

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.