It should be quite uncommon to set both the subflow and the signal
flags: the initiator of the connection is typically the one creating new
subflows, not the other peer, then no need to announce additional local
addresses, and use it to create subflows.

But some people might be confused about the flags, and set both "just to
be sure at least the right one is set". To verify the previous fix, and
avoid future regressions, this specific case is now validated: the
client announces a new address, and initiates a new subflow from the
same address.

While working on this, another bug has been noticed, where the client
reset the new subflow because an ADD_ADDR echo got received as the 3rd
ACK: check that no RST have been sent by the client and server.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 5f5e498bbb75..b8c0b69db527 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1967,6 +1967,21 @@ signal_address_tests()
 		chk_add_nr 1 1
 	fi
 
+	# uncommon: subflow and signal flags on the same endpoint
+	# or because the user wrongly picked both, but still expects the client
+	# to create additional subflows
+	if reset "subflow and signal together"; then
+		pm_nl_set_limits $ns1 0 2
+		pm_nl_set_limits $ns2 0 2
+		pm_nl_add_endpoint $ns2 10.0.3.2 flags signal,subflow
+		run_tests $ns1 $ns2 10.0.1.1
+		chk_join_nr 1 1 1
+		chk_add_nr 1 1 0 invert  # only initiated by ns2
+		chk_add_nr 0 0 0         # none initiated by ns1
+		chk_rst_nr 0 0 invert    # no RST sent by the client
+		chk_rst_nr 0 0           # no RST sent by the server
+	fi
+
 	# accept and use add_addr with additional subflows
 	if reset "multiple subflows and signal"; then
 		pm_nl_set_limits $ns1 0 3

-- 
2.43.0

On Fri, 21 Jun 2024, Matthieu Baerts (NGI0) wrote:

> It should be quite uncommon to set both the subflow and the signal
> flags: the initiator of the connection is typically the one creating new
> subflows, not the other peer, then no need to announce additional local
> addresses, and use it to create subflows.
>
> But some people might be confused about the flags, and set both "just to
> be sure at least the right one is set". To verify the previous fix, and
> avoid future regressions, this specific case is now validated: the
> client announces a new address, and initiates a new subflow from the
> same address.
>
> While working on this, another bug has been noticed, where the client
> reset the new subflow because an ADD_ADDR echo got received as the 3rd
> ACK: check that no RST have been sent by the client and server.
>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 5f5e498bbb75..b8c0b69db527 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -1967,6 +1967,21 @@ signal_address_tests()
> 		chk_add_nr 1 1
> 	fi
>
> +	# uncommon: subflow and signal flags on the same endpoint
> +	# or because the user wrongly picked both, but still expects the client
> +	# to create additional subflows
> +	if reset "subflow and signal together"; then
> +		pm_nl_set_limits $ns1 0 2
> +		pm_nl_set_limits $ns2 0 2
> +		pm_nl_add_endpoint $ns2 10.0.3.2 flags signal,subflow
> +		run_tests $ns1 $ns2 10.0.1.1
> +		chk_join_nr 1 1 1
> +		chk_add_nr 1 1 0 invert  # only initiated by ns2
> +		chk_add_nr 0 0 0         # none initiated by ns1
> +		chk_rst_nr 0 0 invert    # no RST sent by the client
> +		chk_rst_nr 0 0           # no RST sent by the server
> +	fi
> +
> 	# accept and use add_addr with additional subflows
> 	if reset "multiple subflows and signal"; then
> 		pm_nl_set_limits $ns1 0 3

Matthieu -

Related to my feedback on patch 2, it would be good to add a test to 
confirm that announced addr_ids can be reused after they are removed.

- Mat