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

[PATCH mptcp-net v3 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. If it was
still established when closed, it keeps the local ID 0, which is also
the ID used by the re-added endpoint, and fill_remote_addresses_fullmesh()
then marks its remote ID as unavailable. If it was already in TCP_CLOSE,
e.g. after a reset, it is disconnected and its context is reset: the
'closing' flag and the remote ID are cleared, and its remote address
and port are zeroed. It is then picked as a remote address, with the
address 0.0.0.0, the port 0 and the ID 0, which also marks the remote
ID 0 as unavailable. In both cases, when the peer has a single address,
all subflows use the remote ID 0, and no valid remote address is left.

Skip the subflows being closed or in TCP_CLOSE state, both when looking
for the remote IDs already in use and when picking the remote addresses.
__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 | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 1a7750813..24cdb170b 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -199,6 +199,16 @@ fill_remote_addr(struct mptcp_sock *msk, struct mptcp_addr_info *local,
 	return 1;
 }
 
+/* Closed subflows can stay in the conn_list, e.g. the initial one: their
+ * addresses and IDs are not reliable, possibly even reset.
+ */
+static bool mptcp_subflow_is_closed(struct mptcp_subflow_context *subflow)
+{
+	struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+
+	return subflow->closing || inet_sk_state_load(ssk) == TCP_CLOSE;
+}
+
 static unsigned int
 fill_remote_addresses_fullmesh(struct mptcp_sock *msk,
 			       struct mptcp_addr_info *local,
@@ -216,11 +226,18 @@ 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) {
+		if (mptcp_subflow_is_closed(subflow))
+			continue;
+
 		if (READ_ONCE(subflow->local_id) == local->id)
 			__set_bit(subflow->remote_id, unavail_id);
+	}
 
 	mptcp_for_each_subflow(msk, subflow) {
+		if (mptcp_subflow_is_closed(subflow))
+			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 v3 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: 1 · Medium: 0 · Low: 1

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] After this patch, fill_remote_addresses_fullmesh()
  (net/mptcp/pm_kernel.c:229-239) skips the retained initial subflow in…
- [Low] The commit message's account of the reset case is wrong about the
  remote address.

--

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

This patch makes fill_remote_addresses_fullmesh() skip subflows that are
closing or in TCP_CLOSE. They are skipped both when building the unavail_id
bitmap and when picking remote addresses. The goal is to let a re-added
fullmesh endpoint, used by the initial subflow, create a new subflow again.

> If it was already in TCP_CLOSE,
> e.g. after a reset, it is disconnected and its context is reset: the
> 'closing' flag and the remote ID are cleared, and its remote address
> and port are zeroed. It is then picked as a remote address, with the
> address 0.0.0.0, the port 0 and the ID 0, which also marks the remote
> ID 0 as unavailable.

[Severity: Low]
Is the part about the remote address being zeroed accurate?

__mptcp_subflow_disconnect() calls tcp_disconnect() and then
mptcp_subflow_ctx_reset(). tcp_disconnect() only clears the destination
port and the local source address:

net/ipv4/tcp.c:tcp_disconnect() {
    ...
	inet->inet_dport = 0;

	inet_bhash2_reset_saddr(sk);
    ...
}

That path ends in inet_reset_saddr(), which only clears inet_saddr,
inet_rcv_saddr, np->saddr and sk_v6_rcv_saddr. It does not touch
skc_daddr or skc_v6_daddr. mptcp_subflow_ctx_reset() only resets the
subflow context, not the TCP socket addresses.

So in the old code, mptcp_remote_address() would read the original peer
address with port 0 and ID 0, not 0.0.0.0. The conclusion about remote
ID 0 still holds.

Could the description say that only the port is zeroed?

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index 1a7750813235..24cdb170ba62 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c

[ ... ]

> @@ -216,11 +226,18 @@ 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) {
> +		if (mptcp_subflow_is_closed(subflow))
> +			continue;
> +
>  		if (READ_ONCE(subflow->local_id) == local->id)
>  			__set_bit(subflow->remote_id, unavail_id);
> +	}
>  
>  	mptcp_for_each_subflow(msk, subflow) {
> +		if (mptcp_subflow_is_closed(subflow))
> +			continue;
> +

[Severity: High]
Once the closed initial subflow is skipped here, what provides the peer's
initial address (remote ID 0) when no other live subflow points to it?

__mptcp_close_ssk() unlinks every closed subflow except msk->first:

	dispose_it = msk->free_first || ssk != msk->first;
	if (dispose_it)
		list_del(&subflow->node);

After the initial subflow closes, the only other place a remote ID 0
candidate can come from is a live subflow to the peer's initial address.
fill_remote_addr() falls back to mptcp_remote_address() on the msk. This
fullmesh path has no such fallback.

When this loop finds nothing, mptcp_pm_create_subflow_or_signal_addr()
has already marked the endpoint as used:

		__clear_bit(local.addr.id, msk->pm.id_avail_bitmap);
		...
		nr = fill_remote_addresses_vec(msk, &local.addr, fullmesh,
					       addrs, ARRAY_SIZE(addrs));
		if (nr == 0)
			continue;

The endpoint is then not retried for this msk.

The first case looks like the scenario the commit message describes. Take
a client with a single fullmesh endpoint A and a server with one address.
Removing A gracefully closes the first subflow: closing is set and the msk
stays established. Re-adding A maps it to local ID 0 through
mptcp_nl_add_subflow_or_signal_addr(). Both loops then skip the only
conn_list entry, and nr is 0.

Doesn't this still match the case the patch is meant to fix?

  "if the endpoint used by the initial subflow is removed and then
  added back, no new subflow is created from it"

Without the fullmesh flag, fill_remote_addr() still creates A->S in the
same steps. Selftests that keep a second endpoint (such as 10.0.2.2) with
a live subflow to remote ID 0 would not cover this case.

The second case covers endpoints that used to work. After a graceful
close, the first subflow keeps daddr, dport, remote_id 0 and local_id 0.
tcp_shutdown() does not reset them, and the later mptcp_close_ssk()
returns early on close_event_done.

Before this patch, take a new fullmesh endpoint C with a non-zero ID, for
example when an endpoint is removed and a new one is added. The first
loop would not mark ID 0, because local_id 0 != C's ID. The second loop
would then pick that entry, so C->S was created.

With this patch the entry is skipped, nr is 0 and C never connects. Is
this a regression? The connection would have no usable subflow until the
MPTCP close timeout.

Could the msk-level remote address be added as an ID 0 candidate when ID
0 is not set in unavail_id and deny_id0 is false? That should still
respect an RM_ADDR for ID 0 from the peer. If not, could the claim in the
commit message be narrowed?

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

[PATCH mptcp-net v3 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, and also after a reset of the
initial subflow, before the removal of its endpoint. 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 | 62 +++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 18ce7136a..d2d105cef 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -4445,6 +4445,68 @@ 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
+
+	# same, but the initial subflow is reset before removing its endpoint
+	if reset_with_events "delete and re-add, fullmesh, after reset" &&
+	   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 reset" 2
+
+		if ip netns exec "${ns1}" ${iptables} -A INPUT -s "10.0.1.2" \
+		      -p tcp --tcp-option 30 -j REJECT --reject-with tcp-reset; then
+			wait_event ns2 MPTCP_LIB_EVENT_SUB_CLOSED 1
+			ip netns exec "${ns1}" ${iptables} -F INPUT
+
+			pm_nl_del_endpoint $ns2 1 10.0.1.2
+			pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
+			wait_mpj 2
+			chk_mptcp_info subflows 2 subflows 2
+		fi
+
+		mptcp_lib_kill_group_wait $tests_pid
+		kill_events_pids
+
+		chk_join_nr 2 2 2
+	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 v3 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 2 potential
issue(s) to consider.

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

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] The new subtest "delete and re-add, fullmesh, after reset"
  (endpoint_tests() in tools/testing/selftests/net/mptcp/mptcp_join.sh)…
- [Low] In "delete and re-add, fullmesh, after reset" (endpoint_tests(),
  mptcp_join.sh lines 4493-4507), the reset, the endpoint delete/re-add,…

--

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

This commit adds two subtests to endpoint_tests() in mptcp_join.sh. Both
delete and re-add the endpoint used by the initial subflow with the
fullmesh flag set. The second one first resets the initial subflow, then
removes its endpoint.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 18ce7136a2b0..d2d105cef4a9 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -4445,6 +4445,68 @@ endpoint_tests()

[ ... ]

> +	# same, but the initial subflow is reset before removing its endpoint
> +	if reset_with_events "delete and re-add, fullmesh, after reset" &&
> +	   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 reset" 2
> +
> +		if ip netns exec "${ns1}" ${iptables} -A INPUT -s "10.0.1.2" \
> +		      -p tcp --tcp-option 30 -j REJECT --reject-with tcp-reset; then
> +			wait_event ns2 MPTCP_LIB_EVENT_SUB_CLOSED 1
> +			ip netns exec "${ns1}" ${iptables} -F INPUT
> +
> +			pm_nl_del_endpoint $ns2 1 10.0.1.2
> +			pm_nl_add_endpoint $ns2 10.0.1.2 id 1 dev ns2eth1 flags subflow,fullmesh
> +			wait_mpj 2
> +			chk_mptcp_info subflows 2 subflows 2

[Severity: Medium]
Can this subtest pass even if the initial subflow was never reset?

wait_event() gives up after 100 polls and reports nothing to its caller,
so a timeout looks the same as success:

tools/testing/selftests/net/mptcp/mptcp_join.sh:wait_event() {
    ...
	for _ in $(seq 100); do
		count=$(grep -cw "type:${evt}" "${evts}")
		[ "${count}" -ge "${exp}" ] && break
		sleep 0.1
	done
}

The REJECT rule only causes a reset if ns2 sends an option 30 packet
from 10.0.1.2 while the rule is in place. Nothing forces traffic onto
the initial subflow during that window.

If no reset happens, the rule is flushed and the endpoint of a subflow
that is still established gets deleted and re-added. That is the same
scenario the previous "delete and re-add, fullmesh" subtest already
covers.

The later checks pass in that case as well. "chk_mptcp_info subflows 2
subflows 2" holds because closing the initial subflow does not decrement
extra_subflows, and the re-add adds one join. "chk_join_nr 2 2 2" holds
too.

So the TCP_CLOSE path in __mptcp_subflow_disconnect() in
net/mptcp/protocol.c may never be reached:

	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
	    fastclosing) {
		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
		mptcp_subflow_ctx_reset(subflow);

The commit message says this subtest covers that case ("also after a
reset of the initial subflow, before the removal of its endpoint").

"userspace pm no dup close events after error" follows its wait_event()
with "chk_subflows_total 1 1" to confirm the reset happened. Could this
subtest add a similar check after the SUB_CLOSED wait, for example
chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_CLOSED 1 or chk_rst_nr?

> +		fi
> +
> +		mptcp_lib_kill_group_wait $tests_pid
> +		kill_events_pids
> +
> +		chk_join_nr 2 2 2

[Severity: Low]
Should this chk_join_nr be inside the iptables branch above?

The if around the iptables command exists to tolerate a rule that cannot
be installed. In that case endpoint id 1 is never re-added.

Then only one MP_JOIN happens, from the id 2 fullmesh endpoint. The
check here sits outside the branch, so it reports "got 1 JOIN[s] syn rx
expected 2" and the subtest fails instead of being skipped.

"userspace pm no dup close events after error" in userspace_tests() uses
the same iptables guard but keeps every dependent check inside the
branch. Also, reset_with_tcp_filter() calls mark_as_skipped when it
cannot set a REJECT rule.

> +	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://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019185512.458313-1-w453y.me%40gmail.com