From: Qing Luo <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 '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 <luoqing@kylinos.cn>
---
 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 luoqing@kylinos.cn” to follow the Western name order convention, which I hope is more appropriate.

Best regards,
luoqing

Hi luoqing,

On 23/07/2026 05:23, luoqing wrote:
> From: Qing Luo <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 '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.

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Now in our tree:

New patches for t/upstream-net and t/upstream:
- f7a2f9a0e5f5: mptcp: pm: fix data race in add_addr timer callback
- Results: 7455a95067b7..e5d7ab308b5b (export-net)
- Results: 52ff22e1c540..2a68b4bc189c (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/3aa4f7a736529c07204d57a41362869f9b777463/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/15a03914501c72388780156b04fb56ca6b01c20e/checks

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

Hi luoqing,

Thank you for the v4 and the reply.

Please next time do not send a new version as a reply to a previous one,
that's easier with a new thread and a link to the previous one.

On 23/07/2026 05:23, luoqing wrote:
>> 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.

To help categorise data race issues, these questions should be ideally
answered in the commit message:

- Is it a potential issue (i.e. code analysis but never reproduced), or
do we have a proof it can happen (i.e. KCSAN trace)

- If there is a proof, is it easy to reproduce? Is there a reliable
reproducer?

- Could this issue cause damages?

With the increase number of issues reported by AI, potential issues
without proofs might not need to be fixed, especially if it introduces a
higher complexity. Having a reproducer or at least a calltrace would
really help not to waste time on potentially unreproducible bug spot by
AI. But AI could also help to find a reproducer (ideally using packetdrill).

> 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.

OK, thank you for the explanation. There are ongoing discussions [1] to
use this tag like this:

  Assisted-by: LLM # find issue, code, review, test

In your case, it would be this I suppose:

  Assisted-by: LLM # review

[1] https://lore.kernel.org/ksummit/87wluv7yzc.fsf@trenco.lwn.net/

But that's still under discussions. In the meantime, it would help if
you write this with word in the commit message, e.g.

  I discovered this potential issue while studying the code. AI tools
  helped me to verify the issue can potentially happen.

(no need to send a v5, I will apply the v4)

>> 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
> luoqing@kylinos.cn” to follow the Western name order convention, which I
> hope is more appropriate.

Thank you for the explanation! I understand that conventions are
different. I was asking because usually the "Western name order
convention" is used, and the full name is required, not a nickname. To
avoid confusions, even if I'm not a big fan of force people using a
different convention, I think it is easier with the Western form, if
that's OK for you.

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

Hi Qing,

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): Success! ✅
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Unstable: 1 failed test(s): selftest_mptcp_connect_splice ⚠️ 
- 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/29977660387

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


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)