[PATCH mptcp-net 0/2] mptcp: pm: in-kernel: fullmesh: re-add of the initial subflow endp

[PATCH mptcp-net 0/2] mptcp: pm: in-kernel: fullmesh: re-add of the initial subflow endp

[Abdul Wasey]

With fullmesh endpoints, removing and re-adding the endpoint used by the
initial subflow never creates a new subflow from it: the closed initial
subflow stays in the conn_list with the local ID 0, and marks the remote
ID as used.

Patch 1 fixes that, patch 2 adds a test for it.

Found with a router flapping one of its links: the link carrying the
initial subflow never rejoined the existing connections. Tested on top
of the export branch (they also apply on export-net): the new test fails
without patch 1, and the whole mptcp_join.sh passes with it.

Abdul Wasey (2):
  mptcp: pm: in-kernel: fullmesh: skip closed subflows
  selftests: mptcp: join: re-add the initial subflow endp with fullmesh

 net/mptcp/pm_kernel.c                         | 10 ++++++-
 .../testing/selftests/net/mptcp/mptcp_join.sh | 30 +++++++++++++++++++
 2 files changed, 39 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH mptcp-net 1/2] mptcp: pm: in-kernel: fullmesh: skip closed subflows

[Abdul Wasey]

With fullmesh endpoints, if the endpoint used by the initial subflow is
removed and then added back, no new subflow is created from it: the
connection never uses this local address again.

The initial subflow is kept in the conn_list once closed, and it keeps
the local ID 0, which is also the ID used by the re-added endpoint. In
fill_remote_addresses_fullmesh(), this closed subflow then marks its
remote ID as unavailable. When the peer has a single address, all
subflows use the remote ID 0, no remote address is left, and the
endpoint is skipped without trying to create a subflow.

Skip the subflows being closed when looking for the remote IDs already
in use, and when picking the remote addresses: the addresses of a
closed subflow are not reliable, e.g. its destination port can be reset
by tcp_disconnect(). __mptcp_subflow_active() cannot be used here:
subflows still being established have to keep their remote ID marked as
in use, to avoid creating duplicated subflows.

Without the fullmesh flag, the remote address is taken from the MPTCP
socket, so this issue is specific to fullmesh endpoints.

Fixes: 045e9d812868 ("mptcp: fix duplicate subflow creation")
Cc: stable@vger.kernel.org
Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
---
 net/mptcp/pm_kernel.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index d3014bf57..ca794853d 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -216,11 +216,19 @@ fill_remote_addresses_fullmesh(struct mptcp_sock *msk,
 	 * already created by incoming ADD_ADDR
 	 */
 	bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
-	mptcp_for_each_subflow(msk, subflow)
+	mptcp_for_each_subflow(msk, subflow) {
+		/* The initial subflow stays in the list once closed */
+		if (subflow->closing)
+			continue;
+
 		if (READ_ONCE(subflow->local_id) == local->id)
 			__set_bit(subflow->remote_id, unavail_id);
+	}
 
 	mptcp_for_each_subflow(msk, subflow) {
+		if (subflow->closing)
+			continue;
+
 		ssk = mptcp_subflow_tcp_sock(subflow);
 		mptcp_remote_address((struct sock_common *)ssk, &addrs[i]);
 		addrs[i].id = READ_ONCE(subflow->remote_id);
-- 
2.43.0

Re: [PATCH mptcp-net 1/2] mptcp: pm: in-kernel: fullmesh: skip closed subflows

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: fill_remote_addresses_fullmesh() misses disconnected initial subflow
--

commit 4834733b36c6118707c74ccafb4f5dae74a92bbf
Author: Abdul Wasey <w453y.me@gmail.com>

mptcp: pm: in-kernel: fullmesh: skip closed subflows

This commit updates fill_remote_addresses_fullmesh() to skip subflows that
have subflow->closing set when marking remote IDs as unavailable and when
selecting remote addresses for fullmesh endpoints. This aims to avoid
relying on closed subflows kept in conn_list, such as the initial subflow.

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 1a77508132354..b7d7520a3290a 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -216,11 +216,19 @@ fill_remote_addresses_fullmesh(struct mptcp_sock *msk,
>  	 * already created by incoming ADD_ADDR
>  	 */
>  	bitmap_zero(unavail_id, MPTCP_PM_MAX_ADDR_ID + 1);
> -	mptcp_for_each_subflow(msk, subflow)
> +	mptcp_for_each_subflow(msk, subflow) {
> +		/* The initial subflow stays in the list once closed */
> +		if (subflow->closing)
> +			continue;
> +
>  		if (READ_ONCE(subflow->local_id) == local->id)
>  			__set_bit(subflow->remote_id, unavail_id);
> +	}
>  
>  	mptcp_for_each_subflow(msk, subflow) {
> +		if (subflow->closing)
> +			continue;
> +
>  		ssk = mptcp_subflow_tcp_sock(subflow);
>  		mptcp_remote_address((struct sock_common *)ssk, &addrs[i]);
>  		addrs[i].id = READ_ONCE(subflow->remote_id);

[Severity: Medium]
Is checking subflow->closing sufficient to skip the initial subflow after it
has been disconnected via tcp_disconnect()?

When the initial subflow is closed via __mptcp_close_ssk(), subflow->closing
is set to 1, the subflow remains in conn_list, and
__mptcp_subflow_disconnect() is called. Whenever ssk->sk_state is TCP_CLOSE
(or fastclosing is set), __mptcp_subflow_disconnect() disconnects the socket
and resets the subflow context:

net/mptcp/protocol.c:__mptcp_subflow_disconnect() {
	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
	    fastclosing) {
		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
		mptcp_subflow_ctx_reset(subflow);
	}
}

Because subflow->closing and subflow->remote_id are inside the reset
struct_group, mptcp_subflow_ctx_reset() zeroes both of them back to 0 and
sets local_id to -1:

net/mptcp/protocol.h:mptcp_subflow_ctx_reset() {
	memset(&subflow->reset, 0, sizeof(subflow->reset));
	subflow->request_mptcp = 1;
	WRITE_ONCE(subflow->local_id, -1);
}

(This is also why mptcp_close_ssk() checks READ_ONCE(subflow->local_id) < 0
in addition to subflow->close_event_done to detect an already disconnected
initial subflow.)

Additionally, if a subflow transitions to TCP_CLOSE (for example via peer RST
or timeout), subflow->closing is not set until __mptcp_close_subflow() runs,
which in mptcp_worker() executes after mptcp_pm_worker().

When fill_remote_addresses_fullmesh() runs after the initial subflow has been
disconnected and reset, won't subflow->closing be 0, causing the second loop
to read the zeroed remote address and port (skc_dport == 0) from msk->first,
mark remote_id 0 as unavailable in unavail_id (skipping subsequent active
subflows with remote_id 0), increment msk->pm.extra_subflows, and pass port 0
to __mptcp_subflow_connect()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019170751.200446-1-w453y.me@gmail.com?part=1

Re: [PATCH mptcp-net 1/2] mptcp: pm: in-kernel: fullmesh: skip closed subflows

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In fill_remote_addresses_fullmesh() (net/mptcp/pm_kernel.c,
  lines 228-234), the new `if (subflow->closing) continue;` check in the…
- [High] fill_remote_addresses_fullmesh() (net/mptcp/pm_kernel.c) now
  skips every subflow with `closing` set, in both loops.

--

mptcp: pm: in-kernel: fullmesh: skip closed subflows

When a fullmesh endpoint used by the initial subflow is removed and then
added back, the closed initial subflow stays in conn_list and marks remote
ID 0 as unavailable, so no new subflow is created. This patch makes
fill_remote_addresses_fullmesh() skip subflows with closing set, both when
collecting unavailable remote IDs and when picking remote addresses.

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 1a7750813235..b7d7520a3290 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c
> @@ -216,11 +216,19 @@ fill_remote_addresses_fullmesh(struct mptcp_sock *msk,

[ ... ]

>  	mptcp_for_each_subflow(msk, subflow) {
> +		if (subflow->closing)
> +			continue;
> +
>  		ssk = mptcp_subflow_tcp_sock(subflow);
>  		mptcp_remote_address((struct sock_common *)ssk, &addrs[i]);
>  		addrs[i].id = READ_ONCE(subflow->remote_id);

[Severity: High]
Does the closing check catch the tcp_disconnect() case that the commit
message gives as the reason for this check?

msk->first can reach TCP_CLOSE while the msk stays established, for
example after a peer RST or a timeout. In that case __mptcp_close_ssk()
sets closing = 1. The first subflow is not disposed, so
__mptcp_subflow_disconnect() then runs:

net/mptcp/protocol.c:__mptcp_subflow_disconnect() {
    ...
		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
		mptcp_subflow_ctx_reset(subflow);
    ...
}

tcp_disconnect() sets inet_dport to 0 and keeps daddr.
mptcp_subflow_ctx_reset() memsets the reset struct_group, which includes
closing and remote_id, and sets local_id to -1:

net/mptcp/protocol.h:mptcp_subflow_ctx_reset() {
	memset(&subflow->reset, 0, sizeof(subflow->reset));
	subflow->request_mptcp = 1;
	WRITE_ONCE(subflow->local_id, -1);
    ...
}

The subflow stays at the head of conn_list, because msk->first is added
with list_add() and joins use list_add_tail(). It now has closing == 0,
remote_id == 0 and dport == 0.

The first loop does not match it, since local_id is -1. The second loop
does not skip it either, so addrs[0] becomes {peer daddr, port 0, id 0}
and bit 0 is set in unavail_id. It is visited first, so it would also
hide the valid remote ID 0 address of any live subflow later in the list.

When a fullmesh endpoint is added later, would the PM call
__mptcp_subflow_connect() towards <peer>:0? The endpoint would be marked
used and extra_subflows incremented, so that local address would still
never get a working subflow.

The commit message also says:

  "The initial subflow is kept in the conn_list once closed, and it keeps
   the local ID 0"

That seems to hold only on the tcp_shutdown() path. mptcp_close_ssk()
already treats local_id < 0 as a disconnected first subflow:

	/* The first subflow can already be closed or disconnected */
	if (subflow->close_event_done || READ_ONCE(subflow->local_id) < 0)
		return;

Should this also skip subflows with READ_ONCE(subflow->local_id) < 0, or
with the ssk in TCP_CLOSE? Another option is to take the remote ID 0
address from the msk, as fill_remote_addr() does.

[Severity: High]
With every closing subflow skipped in both loops, can this function end
up with no candidate at all? Unlike fill_remote_addr(), there is no
fallback to the msk's remote address.

On graceful endpoint removal, mptcp_pm_rm_addr_or_subflow() calls
mptcp_subflow_shutdown() and then mptcp_close_ssk(). For an established
msk->first, __mptcp_close_ssk() sets closing = 1 and only calls
tcp_shutdown(ssk, SEND_SHUTDOWN). local_id, remote_id, daddr and dport
all stay valid, and the subflow stays in conn_list.

The msk is kept alive on purpose in this state. A comment in
__mptcp_close_ssk() says "the protocol can still create more subflows".

Take a single-subflow connection whose initial endpoint is deleted. The
closed initial subflow is then the only conn_list entry with remote ID 0.
Both new checks skip it, and the function returns 0.
mptcp_pm_create_subflow_or_signal_addr() has already cleared the
endpoint id from id_avail_bitmap at this point:

		nr = fill_remote_addresses_vec(msk, &local.addr, fullmesh,
					       addrs, ARRAY_SIZE(addrs));
		if (nr == 0)
			continue;

As a result, no subflow is attempted from that endpoint.

Does re-adding the initial fullmesh endpoint (local ID 0) still create no
subflow in this case? The selftest in the follow-up patch, "selftests:
mptcp: join: re-add the initial subflow endp with fullmesh", seems to
cover only the case with a second subflow (endpoint id 2) to remote ID 0.

There also seems to be a new regression for a different fullmesh
endpoint X (X != 0) added after the initial endpoint was removed.

Before this patch, the first loop marked nothing (local_id 0 != X). The
second loop then used the closed subflow's still-valid peer address, so
the subflow X -> peer was created.

With this patch, nothing is created and the endpoint is consumed. The
connection can then only wait for the close timeout.

Non-fullmesh endpoints still work, because fill_remote_addr() uses the
msk address:

	mptcp_remote_address((struct sock_common *)sk, &remote);

Would using the msk's remote address for remote ID 0 in the fullmesh
path, subject to deny_join_id0, cover both cases?

Since the patch is tagged for stable, these regressions would also go to
stable kernels.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019170751.200446-1-w453y.me%40gmail.com

Re: [PATCH mptcp-net 1/2] mptcp: pm: in-kernel: fullmesh: skip closed subflows

[Abdul Wasey]

Thanks. The first issue was fixed in v2, which also skips subflows in
TCP_CLOSE. The second one is right: v4 left no remote ID 0 address when
the initial subflow was the only one, and broke adding another endpoint
in that case. v5 falls back to the msk's remote address, like
fill_remote_addr(), with tests for both cases:

https://lore.kernel.org/mptcp/20261021070324.50533-1-w453y.me@gmail.com/

pw-bot: cr

Abdul Wasey

[PATCH mptcp-net 2/2] selftests: mptcp: join: re-add the initial subflow endp with fullmesh

[Abdul Wasey]

The "delete and re-add" test validates the removal and the re-addition
of the endpoint used by the initial subflow, but without the fullmesh
flag.

Do the same with the fullmesh flag: without the previous patch, no new
subflow is created after the re-addition.

The 'Fixes' tag here below is the same as the one from the previous
commit: this patch here is not fixing anything wrong in the selftests,
but it validates the previous fix for an issue introduced by this commit
ID.

Fixes: 045e9d812868 ("mptcp: fix duplicate subflow creation")
Cc: stable@vger.kernel.org
Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f4d8afaed..4fcae963a 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -4424,6 +4424,36 @@ endpoint_tests()
 		chk_rm_nr 4 4
 	fi
 
+	# remove and re-add the endpoint of the initial subflow, with fullmesh
+	if reset_with_events "delete and re-add, fullmesh" &&
+	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
+		pm_nl_set_limits $ns1 0 2
+		pm_nl_set_limits $ns2 0 2
+		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
+		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow,fullmesh
+		{ timeout_test=120 test_linkfail=128 speed=5 \
+			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
+		local tests_pid=$!
+
+		wait_event ns2 MPTCP_LIB_EVENT_ESTABLISHED 1
+		wait_mpj 1
+		chk_subflow_nr "before delete id 0" 2
+
+		pm_nl_del_endpoint $ns2 1 10.0.1.2
+		sleep 0.5
+		chk_subflow_nr "after delete id 0" 1
+
+		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
+		wait_mpj 2
+		chk_subflow_nr "after re-add id 0" 2
+
+		mptcp_lib_kill_group_wait $tests_pid
+		kill_events_pids
+
+		chk_join_nr 2 2 2
+		chk_rm_nr 1 1
+	fi
+
 	# remove and re-add
 	if reset_with_events "delete re-add signal" &&
 	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
-- 
2.43.0

Re: [PATCH mptcp-net 2/2] selftests: mptcp: join: re-add the initial subflow endp with fullmesh

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: pm: fill_remote_addresses_fullmesh() fails to skip reset initial subflow
--

commit 788b7ee55348ab3d2eca027c9a9093fa76b13dc4
Author: Abdul Wasey <w453y.me@gmail.com>

selftests: mptcp: join: re-add the initial subflow endp with fullmesh

This commit adds a selftest in mptcp_join.sh to validate deleting and
re-adding the endpoint used by the initial subflow when the fullmesh flag
is set.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 18ce7136a2b0e..4a7c5f31fc6e2 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -4445,6 +4445,36 @@ endpoint_tests()
>  		chk_rm_nr 4 4
>  	fi
>  
> +	# remove and re-add the endpoint of the initial subflow, with fullmesh
> +	if reset_with_events "delete and re-add, fullmesh" &&
> +	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 0 2
> +		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow,fullmesh
> +		{ timeout_test=120 test_linkfail=128 speed=5 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		wait_event ns2 MPTCP_LIB_EVENT_ESTABLISHED 1
> +		wait_mpj 1
> +		chk_subflow_nr "before delete id 0" 2
> +
> +		pm_nl_del_endpoint $ns2 1 10.0.1.2
> +		sleep 0.5
> +		chk_subflow_nr "after delete id 0" 1
> +
> +		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +		wait_mpj 2
> +		chk_subflow_nr "after re-add id 0" 2

[Severity: Medium]
Does fill_remote_addresses_fullmesh() still fail to skip the initial subflow
when msk->first has been disconnected via tcp_disconnect() and reset via
mptcp_subflow_ctx_reset()?

The preceding commit ("mptcp: pm: in-kernel: fullmesh: skip closed subflows")
added subflow->closing checks in fill_remote_addresses_fullmesh() so that
closed subflows are skipped:

net/mptcp/pm_kernel.c:fill_remote_addresses_fullmesh() {
	mptcp_for_each_subflow(msk, subflow) {
		/* The initial subflow stays in the list once closed */
		if (subflow->closing)
			continue;

		if (READ_ONCE(subflow->local_id) == local->id)
			__set_bit(subflow->remote_id, unavail_id);
	}

	mptcp_for_each_subflow(msk, subflow) {
		if (subflow->closing)
			continue;

		ssk = mptcp_subflow_tcp_sock(subflow);
		mptcp_remote_address((struct sock_common *)ssk, &addrs[i]);
...
}

In this test, deleting the endpoint while msk->first is in TCP_ESTABLISHED
causes __mptcp_close_ssk() -> __mptcp_subflow_disconnect() to take the
tcp_shutdown() branch, which leaves subflow->closing set to 1.

However, when the initial subflow (msk->first) reaches TCP_CLOSE prior to
__mptcp_close_ssk() (for example, due to a peer RST, peer FIN closure,
timeout, or TCP_SYN_SENT shutdown in mptcp_subflow_shutdown()) and
!dispose_it holds (!msk->free_first && ssk == msk->first),
__mptcp_close_ssk() sets subflow->closing = 1 and then calls
__mptcp_subflow_disconnect():

net/mptcp/protocol.c:__mptcp_subflow_disconnect() {
	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
	    fastclosing) {
		/* The MPTCP code never wait on the subflow sockets, TCP-level
		 * disconnect should never fail
		 */
		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
		mptcp_subflow_ctx_reset(subflow);
	} else {
		tcp_shutdown(ssk, SEND_SHUTDOWN);
	}
}

Because ssk->sk_state is TCP_CLOSE, tcp_disconnect(ssk, 0) zeroes inet_dport
and inet_daddr, and mptcp_subflow_ctx_reset() is called immediately after:

net/mptcp/protocol.h:mptcp_subflow_ctx_reset() {
	memset(&subflow->reset, 0, sizeof(subflow->reset));
	subflow->request_mptcp = 1;
	WRITE_ONCE(subflow->local_id, -1);
}

Since subflow->closing is inside struct_group(reset, ...),
mptcp_subflow_ctx_reset() zeroes subflow->closing back to 0 and zeroes
subflow->remote_id to 0 while setting subflow->local_id to -1.

When fill_remote_addresses_fullmesh() runs afterward (such as when re-adding
an endpoint with the fullmesh flag in
mptcp_pm_create_subflow_or_signal_addr()), checking if (subflow->closing)
evaluates to false for the disconnected msk->first still in msk->conn_list.
The second loop in fill_remote_addresses_fullmesh() then copies the wiped
remote address (port 0, address 0.0.0.0, id 0) from msk->first, sets bit 0
in unavail_id (causing any subsequent active subflow with remote_id 0 in
msk->conn_list to be skipped), and passes the port-0 address to
__mptcp_subflow_connect(), where kernel_connect() fails with -EINVAL.

> +
> +		mptcp_lib_kill_group_wait $tests_pid
> +		kill_events_pids
> +
> +		chk_join_nr 2 2 2
> +		chk_rm_nr 1 1
> +	fi
> +
>  	# remove and re-add
>  	if reset_with_events "delete re-add signal" &&
>  	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019170751.200446-1-w453y.me@gmail.com?part=2

Re: [PATCH mptcp-net 2/2] selftests: mptcp: join: re-add the initial subflow endp with fullmesh

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] mptcp: pm: fill_remote_addresses_fullmesh() fails to skip
  reset initial subflow

--

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -4424,6 +4424,36 @@ endpoint_tests()
>  		chk_rm_nr 4 4
>  	fi
>  
> +	# remove and re-add the endpoint of the initial subflow, with fullmesh
> +	if reset_with_events "delete and re-add, fullmesh" &&
> +	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 0 2
> +		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +		pm_nl_add_endpoint $ns2 10.0.2.2 id 2 dev ns2eth2 flags subflow,fullmesh
> +		{ timeout_test=120 test_linkfail=128 speed=5 \
> +			run_tests $ns1 $ns2 10.0.1.1 & } 2>/dev/null
> +		local tests_pid=$!
> +
> +		wait_event ns2 MPTCP_LIB_EVENT_ESTABLISHED 1
> +		wait_mpj 1
> +		chk_subflow_nr "before delete id 0" 2
> +
> +		pm_nl_del_endpoint $ns2 1 10.0.1.2
> +		sleep 0.5
> +		chk_subflow_nr "after delete id 0" 1
> +
> +		pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +		wait_mpj 2
> +		chk_subflow_nr "after re-add id 0" 2
> +
> +		mptcp_lib_kill_group_wait $tests_pid
> +		kill_events_pids
> +
> +		chk_join_nr 2 2 2
> +		chk_rm_nr 1 1
> +	fi
> +
>  	# remove and re-add
>  	if reset_with_events "delete re-add signal" &&
>  	   continue_if mptcp_lib_kallsyms_has "subflow_rebuild_header$"; then

[Severity: Medium]
This only exercises the case where msk->first is still established
when the endpoint is removed. In that case __mptcp_subflow_disconnect()
takes the tcp_shutdown() branch and the subflow context is left intact.
Doesn't the fix from the previous patch still have a hole when the
initial subflow reaches TCP_CLOSE before mptcp_close_ssk() runs? That
can happen after a peer RST or FIN, a timeout, or a shutdown from
TCP_SYN_SENT.

In that case __mptcp_close_ssk() sets subflow->closing and keeps
msk->first in conn_list because dispose_it is false. Then
__mptcp_subflow_disconnect() calls tcp_disconnect() followed by
mptcp_subflow_ctx_reset(). The memset of subflow->reset clears closing
again, zeroes remote_id and sets local_id to -1.

When the fullmesh endpoint is then re-added,
fill_remote_addresses_fullmesh() no longer skips that subflow. The
second loop copies 0.0.0.0 port 0 with id 0 from the disconnected
socket and marks remote id 0 as unavailable, so any live subflow
towards remote id 0 is skipped. __mptcp_subflow_connect() then fails
with -EINVAL on the port 0 address. No subflow towards remote id 0
gets created, which is the same symptom this test is meant to catch.

Would checking the ssk state or the remote port in
fill_remote_addresses_fullmesh() be a more robust guard than
subflow->closing here? Could the selftest also grow a variant where
the initial subflow is reset by the peer before the endpoint is
re-added, so that path is covered too?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019170751.200446-1-w453y.me%40gmail.com

Re: [PATCH mptcp-net 0/2] mptcp: pm: in-kernel: fullmesh: re-add of the initial subflow endp

[MPTCP CI]

Hi Abdul,

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_sendfile ⚠️ 
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Critical: No conclusion ❓
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/39579830342

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


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)