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

[Matthieu Baerts <matttbe@kernel.org>]

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.