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

From: Qing Luo <[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.

Use a local 'retransmit' flag set inside pm.lock to capture whether
retransmission is still possible. This ensures that
mptcp_pm_subflow_established() is only called when the retransmission
naturally exhausts.

Fixes: 348d5c1dec60 ("mptcp: move to next addr when timeout")
Signed-off-by: Qing Luo <[email protected]>
---
 net/mptcp/pm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6afd39aea110..c71dcf887683 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -380,6 +380,7 @@ 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 retransmit;
 
 	pr_debug("msk=%p\n", msk);
 
@@ -412,14 +413,15 @@ static void mptcp_pm_add_addr_timer(struct timer_list *timer)
 		entry->retrans_times++;
 	}
 
-	if (entry->retrans_times < ADD_ADDR_RETRANS_MAX)
+	retransmit = entry->retrans_times < ADD_ADDR_RETRANS_MAX;
+	if (retransmit)
 		timeout <<= entry->retrans_times;
 	else
 		timeout = 0;
 
 	spin_unlock_bh(&msk->pm.lock);
 
-	if (entry->retrans_times == ADD_ADDR_RETRANS_MAX)
+	if (!retransmit)
 		mptcp_pm_subflow_established(msk);
 
 out:
-- 
2.25.1
Hi,

Thank you for your review and suggestions. I'm very sorry for not replying to your question in time and causing you inconvenience. It's because some issues are difficult to explain, so I merely followed your suggestion to analyze and submit the code.

> Don't forget to reply to each question from previous reviews 😉
>
> 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.
I discovered this potential issue while studying the new MPTCP path manager code. Through code analysis, I noticed that entry->retrans_times could be accessed both inside and outside the pm.lock, which appeared to create a race condition with mptcp_pm_announced_del_timer().

Unfortunately, I don’t have a reproducer for this issue at the moment, as it’s difficult to construct a test scenario that reliably triggers this specific race condition. I submitted the patch based on code analysis to prevent potential issues.

I used AI tools to help verify and analyze the code logic during my investigation. However, I’m not entirely certain about the extent to which I should acknowledge this assistance, so I didn’t add an Assisted-by tag in the current version. If you think it’s necessary, I’d be happy to add it.

> 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?
I apologize for the confusion. “Luoqing” is actually my full name in Chinese convention - “Luo” (罗) is my family name and “Qing” (庆) is my given name. In Chinese, we don’t typically capitalize individual characters or add spaces between them. However, I understand the concern about the formatting, and I’ve updated the Signed-off-by to “Qing Luo [email protected]” to follow the Western name order convention, which I hope is more appropriate.

Best regards,
luoqing