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

July 15, 2026 at 2:05 PM, "luoqing" <[email protected] mailto:[email protected]?to=%22luoqing%22%20%3Cl1138897701%40163.com%3E > wrote:

Hi luoqin,

Thanks for your patch.

> 
> From: luoqing <[email protected]>
> 
> 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 <[email protected]>
> ---
>  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
>