[v1] mptcp: fix extra_subflows leak on failed passive join

[PATCH mptcp-net] mptcp: fix extra_subflows leak on failed passive join

Posted by Chenguang Zhao 4 days, 19 hours ago

From: Chenguang Zhao <zhaochenguang@kylinos.cn>

mptcp_pm_allow_new_subflow() increments extra_subflows
before __mptcp_finish_join() on the passive MP_JOIN path.
On synchronous join failure the subflow is dropped without
calling mptcp_close_ssk(), so the counter is not rolled back.

Call mptcp_pm_close_subflow() when the join completion fails.

Signed-off-by: Chenguang Zhao <zhaochenguang@kylinos.cn>
---
 net/mptcp/protocol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index cb9515f505aa..b32f0cd262a7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3907,6 +3907,7 @@ bool mptcp_finish_join(struct sock *ssk)
 	mptcp_data_unlock(parent);
 
 	if (!ret) {
+		mptcp_pm_close_subflow(msk);
 err_prohibited:
 		subflow->reset_reason = MPTCP_RST_EPROHIBIT;
 		return false;
-- 
2.25.1

Re: [PATCH mptcp-net] mptcp: fix extra_subflows leak on failed passive join

Posted by MPTCP CI 4 days, 17 hours ago

Hi Chenguang,

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): Success! ✅
- 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/28438179349

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


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)

Re: [PATCH mptcp-net] mptcp: fix extra_subflows leak on failed passive join

Posted by Matthieu Baerts 4 days, 18 hours ago

Hi Chenguang,

Thank you for the fix.

> mptcp_pm_allow_new_subflow() increments extra_subflows
> before __mptcp_finish_join() on the passive MP_JOIN path.

I see that this code path is currently not exercised by our test suite:

  https://ci-results.mptcp.dev/lcov/export/mptcp/protocol.c.gcov.html#L4031

How did you spot the issue? Do you have a reproducer? Or was it by
reading the code? Assisted by a tool (in this case, please add the
Assisted-by tag)?

I see we have a test for sync connect failure, but not sync join. Do you
mind adding a new Packetdrill test please? Similar to simult_connect.pkt
I suppose:

  https://github.com/multipath-tcp/packetdrill/blob/mptcp-net-next/gtests/net/mptcp/syscalls/simult_connect.pkt

> On synchronous join failure the subflow is dropped without
> calling mptcp_close_ssk(), so the counter is not rolled back.
>
> Call mptcp_pm_close_subflow() when the join completion fails.

A fix patch should always have a Fixes tag. Here, I guess it should be:

Fixes: 10f6d46c943d ("mptcp: fix race between MP_JOIN and close")

Apart from that, the modification looks good to me:

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

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

Re: [PATCH mptcp-net] mptcp: fix extra_subflows leak on failed passive join

Posted by Chenguang Zhao 1 day, 19 hours ago

Hi Matthieu,

Regarding the sync join failure, I have already submitted a PR.
https://github.com/multipath-tcp/packetdrill/pull/200

Thanks

在 2026/6/30 18:47, Matthieu Baerts 写道:
> Hi Chenguang,
>
> Thank you for the fix.
>
>> mptcp_pm_allow_new_subflow() increments extra_subflows
>> before __mptcp_finish_join() on the passive MP_JOIN path.
> I see that this code path is currently not exercised by our test suite:
>
>   https://ci-results.mptcp.dev/lcov/export/mptcp/protocol.c.gcov.html#L4031
>
> How did you spot the issue? Do you have a reproducer? Or was it by
> reading the code? Assisted by a tool (in this case, please add the
> Assisted-by tag)?
>
> I see we have a test for sync connect failure, but not sync join. Do you
> mind adding a new Packetdrill test please? Similar to simult_connect.pkt
> I suppose:
>
>   https://github.com/multipath-tcp/packetdrill/blob/mptcp-net-next/gtests/net/mptcp/syscalls/simult_connect.pkt
>
>> On synchronous join failure the subflow is dropped without
>> calling mptcp_close_ssk(), so the counter is not rolled back.
>>
>> Call mptcp_pm_close_subflow() when the join completion fails.
> A fix patch should always have a Fixes tag. Here, I guess it should be:
>
> Fixes: 10f6d46c943d ("mptcp: fix race between MP_JOIN and close")
>
> Apart from that, the modification looks good to me:
>
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
>

[PATCH mptcp-net v2] mptcp: fix extra_subflows leak on failed passive join

Posted by Chenguang Zhao 4 days, 1 hour ago

From: Chenguang Zhao <zhaochenguang@kylinos.cn>

mptcp_pm_allow_new_subflow() increments extra_subflows
before __mptcp_finish_join() on the passive MP_JOIN path.
On synchronous join failure the subflow is dropped without
calling mptcp_close_ssk(), so the counter is not rolled back.

Call mptcp_pm_close_subflow() when the join completion fails.

Fixes: 3e5014909b56 ("mptcp: cleanup MPJ subflow list handling")
Signed-off-by: Chenguang Zhao <zhaochenguang@kylinos.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
v2:
 Hi Matthieu,
 Thanks for the review.
 - I spotted this by reading the code. I don't have a reproducer
   yet. I also see we already have a Packetdrill test for
   synchronous connect failure (simult_connect.pkt), but not for
   synchronous join failure. I'll add a similar Packetdrill test
   for that case and send it in a follow-up patch.
 - Add Fixes and Reviewd-by tag.
 - I looked again at the history, and I think the Fixes tag
   should point to 3e5014909b56 ("mptcp: cleanup MPJ subflow list
   handling"), not 10f6d46c943d ("mptcp: fix race between MP_JOIN and close"). 
v1:
 - https://lore.kernel.org/all/20260630095432.1340629-1-chenguang.zhao@linux.dev/

 net/mptcp/protocol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index cb9515f505aa..b32f0cd262a7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3907,6 +3907,7 @@ bool mptcp_finish_join(struct sock *ssk)
 	mptcp_data_unlock(parent);
 
 	if (!ret) {
+		mptcp_pm_close_subflow(msk);
 err_prohibited:
 		subflow->reset_reason = MPTCP_RST_EPROHIBIT;
 		return false;
-- 
2.25.1

Re: [PATCH mptcp-net v2] mptcp: fix extra_subflows leak on failed passive join

Posted by Geliang Tang 3 days, 22 hours ago

Hi Chenguang,

Thanks for this v2. However, if there are no code changes, there is no
need to send a v2; we can just reply to the email thread on v1 to
discuss it. 

On Wed, 2026-07-01 at 11:20 +0800, Chenguang Zhao wrote:
> From: Chenguang Zhao <zhaochenguang@kylinos.cn>
> 
> mptcp_pm_allow_new_subflow() increments extra_subflows
> before __mptcp_finish_join() on the passive MP_JOIN path.
> On synchronous join failure the subflow is dropped without
> calling mptcp_close_ssk(), so the counter is not rolled back.
> 
> Call mptcp_pm_close_subflow() when the join completion fails.
> 
> Fixes: 3e5014909b56 ("mptcp: cleanup MPJ subflow list handling")
> Signed-off-by: Chenguang Zhao <zhaochenguang@kylinos.cn>
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> v2:
>  Hi Matthieu,
>  Thanks for the review.
>  - I spotted this by reading the code. I don't have a reproducer
>    yet. I also see we already have a Packetdrill test for
>    synchronous connect failure (simult_connect.pkt), but not for
>    synchronous join failure. I'll add a similar Packetdrill test
>    for that case and send it in a follow-up patch.
>  - Add Fixes and Reviewd-by tag.
>  - I looked again at the history, and I think the Fixes tag
>    should point to 3e5014909b56 ("mptcp: cleanup MPJ subflow list
>    handling"), not 10f6d46c943d ("mptcp: fix race between MP_JOIN and
> close"). 

The Fixes tag is incorrect and should be 10f6d46c943d ("mptcp: fix race
between MP_JOIN and close"), because that commit introduced the call to
mptcp_pm_allow_new_subflow. There is no need to send a v3, as this can
be updated when applying.

I have changed the status of v2 to "Queued" and v1 to "Superseded".

Thanks,
-Geliang

> v1:
>  -
> https://lore.kernel.org/all/20260630095432.1340629-1-chenguang.zhao@linux.dev/
> 
>  net/mptcp/protocol.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index cb9515f505aa..b32f0cd262a7 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3907,6 +3907,7 @@ bool mptcp_finish_join(struct sock *ssk)
>  	mptcp_data_unlock(parent);
>  
>  	if (!ret) {
> +		mptcp_pm_close_subflow(msk);
>  err_prohibited:
>  		subflow->reset_reason = MPTCP_RST_EPROHIBIT;
>  		return false;

Re: [PATCH mptcp-net v2] mptcp: fix extra_subflows leak on failed passive join

Posted by MPTCP CI 4 days ago

Hi Chenguang,

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): Success! ✅
- 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/28491654012

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


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)