selftests: mptcp: be more conservative with cookie MPJ limits

[PATCH mptcp-net] selftests: mptcp: be more conservative with cookie MPJ limits

Posted by Paolo Abeni 2 years, 2 months ago

Since commit 2843ff6f36db ("mptcp: remote addresses fullmesh"), an
MPTCP client can attempt creating multiple MPJ subflow simultaneusly.

In such scenario the server, when syncookies are enabled, could end-up
accepting incoming MPJ syn even above the configured subflow limit, as
the such limit can be enforced in a reliable way only after the subflow
creation. In case of syncookie, only after the 3rd ack reception.

As a consequence the related self-tests case sporadically fails, as it
verify that the server always accept the expected number of MPJ syn.

Address the issues relaxing the MPJ syn number constrain. Note that the
check on the accepted number of MPJ 3rd ack still remains intact.

Fixes: 2843ff6f36db ("mptcp: remote addresses fullmesh")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
this fixes for me some rare failure I observe in debug build:

072 subflows limited by server w cookies syn[ ok ] - synack[fail] got 2 JOIN[s] synack expected 1
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 66ac990415e6..9bcb3653f78b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -822,6 +822,7 @@ chk_join_nr()
 	local ack_nr=$4
 	local count
 	local dump_stats
+	local with_cookie
 
 	printf "%03u %-36s %s" "$TEST_COUNT" "$msg" "syn"
 	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPJoinSynRx | awk '{print $2}'`
@@ -835,12 +836,20 @@ chk_join_nr()
 	fi
 
 	echo -n " - synack"
+	with_cookie=`ip netns exec $ns2 sysctl -n net.ipv4.tcp_syncookies`
 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtMPJoinSynAckRx | awk '{print $2}'`
 	[ -z "$count" ] && count=0
 	if [ "$count" != "$syn_ack_nr" ]; then
-		echo "[fail] got $count JOIN[s] synack expected $syn_ack_nr"
-		ret=1
-		dump_stats=1
+		# simult connections exeeding the limit with cookie enabled could go up to
+		# synack validation as the conn limit can be enforced reliably only after
+		# the subflow creation
+		if [ "$with_cookie" = 2 -a "$count" -gt "$syn_ack_nr" -a "$count" -le "$syn_nr" ]; then
+			echo -n "[ ok ]"
+		else
+			echo "[fail] got $count JOIN[s] synack expected $syn_ack_nr"
+			ret=1
+			dump_stats=1
+		fi
 	else
 		echo -n "[ ok ]"
 	fi
-- 
2.34.1

Re: [PATCH mptcp-net] selftests: mptcp: be more conservative with cookie MPJ limits

Posted by Mat Martineau 2 years, 2 months ago

On Wed, 9 Feb 2022, Paolo Abeni wrote:

> Since commit 2843ff6f36db ("mptcp: remote addresses fullmesh"), an
> MPTCP client can attempt creating multiple MPJ subflow simultaneusly.
>
> In such scenario the server, when syncookies are enabled, could end-up
> accepting incoming MPJ syn even above the configured subflow limit, as
> the such limit can be enforced in a reliable way only after the subflow
> creation. In case of syncookie, only after the 3rd ack reception.
>
> As a consequence the related self-tests case sporadically fails, as it
> verify that the server always accept the expected number of MPJ syn.
>
> Address the issues relaxing the MPJ syn number constrain. Note that the
> check on the accepted number of MPJ 3rd ack still remains intact.
>
> Fixes: 2843ff6f36db ("mptcp: remote addresses fullmesh")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> this fixes for me some rare failure I observe in debug build:
>
> 072 subflows limited by server w cookies syn[ ok ] - synack[fail] got 2 JOIN[s] synack expected 1

Looks good to me:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

> ---
> tools/testing/selftests/net/mptcp/mptcp_join.sh | 15 ++++++++++++---
> 1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 66ac990415e6..9bcb3653f78b 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -822,6 +822,7 @@ chk_join_nr()
> 	local ack_nr=$4
> 	local count
> 	local dump_stats
> +	local with_cookie
>
> 	printf "%03u %-36s %s" "$TEST_COUNT" "$msg" "syn"
> 	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPJoinSynRx | awk '{print $2}'`
> @@ -835,12 +836,20 @@ chk_join_nr()
> 	fi
>
> 	echo -n " - synack"
> +	with_cookie=`ip netns exec $ns2 sysctl -n net.ipv4.tcp_syncookies`
> 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtMPJoinSynAckRx | awk '{print $2}'`
> 	[ -z "$count" ] && count=0
> 	if [ "$count" != "$syn_ack_nr" ]; then
> -		echo "[fail] got $count JOIN[s] synack expected $syn_ack_nr"
> -		ret=1
> -		dump_stats=1
> +		# simult connections exeeding the limit with cookie enabled could go up to
> +		# synack validation as the conn limit can be enforced reliably only after
> +		# the subflow creation
> +		if [ "$with_cookie" = 2 -a "$count" -gt "$syn_ack_nr" -a "$count" -le "$syn_nr" ]; then
> +			echo -n "[ ok ]"
> +		else
> +			echo "[fail] got $count JOIN[s] synack expected $syn_ack_nr"
> +			ret=1
> +			dump_stats=1
> +		fi
> 	else
> 		echo -n "[ ok ]"
> 	fi
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net] selftests: mptcp: be more conservative with cookie MPJ limits

Posted by Matthieu Baerts 2 years, 2 months ago

Hi Paolo, Mat,

On 09/02/2022 18:15, Paolo Abeni wrote:
> Since commit 2843ff6f36db ("mptcp: remote addresses fullmesh"), an
> MPTCP client can attempt creating multiple MPJ subflow simultaneusly.
> 
> In such scenario the server, when syncookies are enabled, could end-up
> accepting incoming MPJ syn even above the configured subflow limit, as
> the such limit can be enforced in a reliable way only after the subflow
> creation. In case of syncookie, only after the 3rd ack reception.
> 
> As a consequence the related self-tests case sporadically fails, as it
> verify that the server always accept the expected number of MPJ syn.
> 
> Address the issues relaxing the MPJ syn number constrain. Note that the
> check on the accepted number of MPJ 3rd ack still remains intact.

Now in our tree (fix for -net) with Mat's RvB tag, without a typo
detected by codespell and replaced

  [ ... -a ... ]

by

  [ ... ] && [ ... ]

(linked to my series refactoring mptcp_join.sh)

- 7c2d506acce5: selftests: mptcp: be more conservative with cookie MPJ
limits
- Results: af8e6fcc9de1..388ba6d7cd29

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220210T151226
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net