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

[gang.yan@linux.dev]

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
>