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

[PATCH mptcp-net v2 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, and makes the PM think the remote ID 0
is already used, or picks its reset remote address.

Patch 1 fixes that, patch 2 adds tests 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 tests fail
without patch 1, and the whole mptcp_join.sh passes with it.

Changes in v2:
- Patch 1: also skip the subflows in TCP_CLOSE: the initial subflow can
  be disconnected and its context reset, clearing the 'closing' flag and
  zeroing its remote address (Sashiko).
- Patch 2: validate this case too, by resetting the initial subflow
  before removing its endpoint.
- v1: https://lore.kernel.org/mptcp/20261019170751.200446-1-w453y.me@gmail.com/

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                         | 19 +++++-
 .../testing/selftests/net/mptcp/mptcp_join.sh | 64 +++++++++++++++++++
 2 files changed, 82 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH mptcp-net v2 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 d3014bf57..fcb5b1c85 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 v2 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] The fix is incomplete, and it adds a regression when no other
  subflow is still alive.
- [Low] The commit message describes the state of a reset initial subflow
  incorrectly.

--

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

This patch makes fill_remote_addresses_fullmesh() skip subflows that are
closing or in TCP_CLOSE state. The skip applies both when building the
bitmap of remote IDs already in use and when picking remote addresses.
The goal is to let the fullmesh endpoint of the initial subflow create
new subflows again after it is removed and re-added.

> 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 statement that the remote address is zeroed accurate?

On the TCP_CLOSE path, __mptcp_subflow_disconnect() does:

    WARN_ON_ONCE(tcp_disconnect(ssk, 0));
    mptcp_subflow_ctx_reset(subflow);

tcp_disconnect() only clears the destination port and the source
addresses:

    inet->inet_dport = 0;

    inet_bhash2_reset_saddr(sk);

inet_bhash2_reset_saddr() ends up in inet_reset_saddr(). That only
touches inet_saddr and inet_rcv_saddr, plus the IPv6 equivalents.

mptcp_subflow_ctx_reset() only clears the reset struct_group and sets
local_id to -1. Neither path clears skc_daddr or skc_v6_daddr.

So mptcp_remote_address() on that subflow would return the real peer
address with port 0 and ID 0, not 0.0.0.0. Could the commit message be
updated to say so?

> 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
> @@ -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,

[ ... ]

>  	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);

[Severity: High]
What happens when the closed initial subflow (msk->first) is the only
entry left in conn_list?

Both loops skip it, so fill_remote_addresses_fullmesh() returns 0.
fill_remote_addr() takes the address from the msk, but this function has
no such fallback:

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

By that point, mptcp_pm_create_subflow_or_signal_addr() has already
consumed the endpoint ID:

    __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;

As a result, no subflow is created and the endpoint stays marked as used
for this msk.

Two cases seem to reach this.

The first is the case from the commit message, when the initial
subflow's endpoint was the only live one. An example is a single
fullmesh endpoint and a peer with a single address, where the endpoint
is removed and then re-added. Nothing is created, and the msk is closed
once the timeout started in __mptcp_close_ssk() expires:

    if (list_is_singular(&msk->conn_list) && msk->first &&
        inet_sk_state_load(msk->first) == TCP_CLOSE) {
        if (sk->sk_state != TCP_ESTABLISHED ||
            msk->in_accept_queue || sock_flag(sk, SOCK_DEAD)) {
            ...
        } else {
            mptcp_start_tout_timer(sk);

The second case looks like a regression from this patch. The initial
subflow's endpoint is removed, then a different fullmesh endpoint
(ID != 0) is added while msk->first is still closing gracefully.

Removing the endpoint goes through:

pm_nl_del_endpoint()
  mptcp_nl_remove_subflow_and_signal_addr()
    mptcp_pm_rm_addr_or_subflow()
      mptcp_close_ssk()
        __mptcp_close_ssk()

That path sets subflow->closing. msk->first is not disposed, so
__mptcp_subflow_disconnect() only calls tcp_shutdown(ssk, SEND_SHUTDOWN).
The subflow stays in FIN_WAIT1/FIN_WAIT2 and keeps a valid daddr, dport
and remote_id 0.

Before this patch, the first loop did not mark that subflow, since
local_id 0 differs from the new ID. The second loop then picked it as
remote ID 0 with a valid address and port, and the new endpoint created
a subflow.

Now mptcp_subflow_is_closed() skips it. Does the new endpoint still
create anything for this msk?

The non-fullmesh path recovers in both cases, because fill_remote_addr()
uses the msk's remote address. Would it make sense to fall back to the
msk remote address here (ID 0, subject to deny_id0) when it is not
already in use?

The selftest in this series ("selftests: mptcp: join: re-add the initial
subflow endp with fullmesh") always keeps a second endpoint (id 2) with
a live subflow. As far as I can tell, it covers neither case.

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

[PATCH mptcp-net v2 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 | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f4d8afaed..884d635bc 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -4424,6 +4424,70 @@ 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
+
+		init_nftables
+		if ip netns exec "${ns1}" nft add rule ip filter INPUT \
+				ip saddr "10.0.1.2" meta l4proto tcp \
+				reject with tcp reset; then
+			wait_event ns2 MPTCP_LIB_EVENT_SUB_CLOSED 1
+			ip netns exec "${ns1}" nft flush chain ip filter 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 v2 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] selftests: mptcp: undefined init_nftables() breaks endpoint_tests()
--

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

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

This commit adds two selftests to endpoint_tests() in mptcp_join.sh to
verify deleting and re-adding the endpoint of the initial subflow when the
fullmesh flag is set, both normally and after the initial subflow is reset.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 18ce7136a2b0e..0fa69aa2e9b80 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -4445,6 +4445,70 @@ 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
> +
> +		init_nftables
> +		if ip netns exec "${ns1}" nft add rule ip filter INPUT \
> +				ip saddr "10.0.1.2" meta l4proto tcp \
> +				reject with tcp reset; then
> +			wait_event ns2 MPTCP_LIB_EVENT_SUB_CLOSED 1
> +			ip netns exec "${ns1}" nft flush chain ip filter 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

[Severity: Medium]
Where is init_nftables defined? It does not appear to exist in
tools/testing/selftests/net/mptcp/mptcp_join.sh, mptcp_lib.sh, or
net/lib.sh, so calling it here produces "init_nftables: command not found".

Because the ip filter table and INPUT chain are never initialized in
${ns1} (which was freshly created via reset_with_events() ->
init_partial()), the subsequent "nft add rule ip filter INPUT ..." command
also fails with "No such file or directory".

That causes the if block to be skipped without resetting the subflow or
deleting and re-adding endpoint 1, and then chk_join_nr 2 2 2 fails
unconditionally because only 1 MP_JOIN occurred.

Should this use ${iptables} instead, matching how userspace_tests() in
tools/testing/selftests/net/mptcp/mptcp_join.sh injects TCP resets:

		if ip netns exec "${ns1}" ${iptables} -A INPUT -s "10.0.1.2" \
		      -p tcp --tcp-option 30 -j REJECT --reject-with tcp-reset &&

> +
>  	# 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/20261019180306.382039-1-w453y.me@gmail.com?part=2

Re: [PATCH mptcp-net v2 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 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · 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).

- [High] The new "delete and re-add, fullmesh, after reset" test in
  endpoint_tests()…
- [Medium] The "delete and re-add, fullmesh, after reset" test depends on
  the `nft` binary and on IPv4 nf_tables reject support, but declares…
- [Low] Even if the nft rule were installed, the "after reset" test never
  checks that the initial subflow was actually reset before its endpoint…

--

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

This 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. The second one first resets the initial subflow with an nft reject
rule, to validate the previous fix for 045e9d812868.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 18ce7136a2b0..0fa69aa2e9b8 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -4445,6 +4445,70 @@ 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
> +
> +		init_nftables
> +		if ip netns exec "${ns1}" nft add rule ip filter INPUT \
> +				ip saddr "10.0.1.2" meta l4proto tcp \
> +				reject with tcp reset; then

[Severity: High]
Where is init_nftables() defined? A grep over tools/testing/selftests/
finds only this call site. There is no definition in mptcp_join.sh,
mptcp_lib.sh or net/lib.sh. The script doesn't use set -e, so bash
would print "command not found" and keep going.

Without that helper, does anything create the ip filter table and its
INPUT chain in ns1? Each test gets a fresh netns from
reset()->init_partial()->mptcp_lib_ns_init, and this test runs no other
iptables or nft command in ns1.

If so, the nft add rule always fails and the whole body of this if is
skipped: the SUB_CLOSED wait, the endpoint delete and re-add, wait_mpj 2
and chk_mptcp_info.

The unconditional chk_join_nr 2 2 2 further down would then see only the
single MP_JOIN from the id 2 subflow. Wouldn't this subtest then fail on
every kernel, with or without the previous fix?

It also means this part of the commit message isn't actually validated:

  Do the same with the fullmesh flag, and also after a reset of the
  initial subflow, before the removal of its endpoint.

The TCP_CLOSE branch of mptcp_subflow_is_closed() added by the previous
patch would stay untested.

Was a helper hunk dropped from this submission? The existing reset in
userspace_tests() uses iptables and needs no extra helper:

		if ip netns exec "${ns1}" ${iptables} -A INPUT -s "10.0.1.2" \
		      -p tcp --tcp-option 30 -j REJECT --reject-with tcp-reset &&

> +			wait_event ns2 MPTCP_LIB_EVENT_SUB_CLOSED 1
> +			ip netns exec "${ns1}" nft flush chain ip filter INPUT
> +
> +			pm_nl_del_endpoint $ns2 1 10.0.1.2

[Severity: Low]
Once the rule can be installed, does anything confirm that the initial
subflow was actually reset before its endpoint is deleted?

wait_event() polls up to 100 times. If the count is not reached it just
leaves the loop, without calling fail_test:

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 checks that follow, chk_mptcp_info subflows 2 subflows 2 and
chk_join_nr 2 2 2, give the same result whether the initial subflow was
reset or simply closed by pm_nl_del_endpoint. The reason is that
mptcp_pm_subflow_check_next() only updates the counter for join
subflows:

net/mptcp/pm.c:mptcp_pm_subflow_check_next() {
	...
	update_subflows = subflow->request_join || subflow->mp_join;
	...
}

If no reset happens, wouldn't this reduce to the preceding fullmesh
re-add test and still pass?

The userspace reset test asserts the event explicitly:

			chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_CLOSED 2 error 2

Could a similar chk_evt_nr ns2 MPTCP_LIB_EVENT_SUB_CLOSED check be added
here?

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

[Severity: Medium]
Even once init_nftables() exists, should this test be skipped rather
than failed when the nft rule can't be loaded?

The new rule needs three things: the nft binary, an ip family table
(CONFIG_NF_TABLES_IPV4), and reject support (CONFIG_NFT_REJECT and
NFT_REJECT_IPV4). tools/testing/selftests/net/mptcp/config only enables
these nftables options:

CONFIG_NF_TABLES=m
CONFIG_NF_TABLES_INET=y
CONFIG_NFT_COMPAT=m
CONFIG_NFT_SOCKET=m
CONFIG_NFT_TPROXY=m

NF_TABLES_IPV4 has no default. NFT_REJECT defaults to m only when
NETFILTER_ADVANCED=n, and this config sets NETFILTER_ADVANCED=y.

init() also doesn't check for nft:

	mptcp_lib_check_tools ip tc ss "${iptables}" "${ip6tables}"

The if block has no else branch, and chk_join_nr 2 2 2 runs on both
paths. A missing dependency therefore shows up as a hard failure (1 join
instead of the expected 2) rather than a skip.

reset_with_tcp_filter() handles the same situation like this:

		mark_as_skipped "unable to set the filter rules"

Would switching to the iptables idiom, or adding a skip path like this
one, avoid the false failure?

> +	fi
> +

[ ... ]

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

Re: [PATCH mptcp-net v2 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): 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! ✅
- Perf: Critical: No conclusion ❓
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/39579941636

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


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)