[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

[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