[PATCH mptcp-next v2 0/2] add mp_fail testcases

[PATCH mptcp-next v2 0/2] add mp_fail testcases

[Geliang Tang]

This is v12 of the mp_fail testcases with Matt's changes. It works well
and very stable.

Geliang Tang (2):
  Squash to "mptcp: infinite mapping receiving"
  selftests: mptcp: add mp_fail testcases

 net/mptcp/subflow.c                           |   1 +
 tools/testing/selftests/net/mptcp/config      |   8 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 142 ++++++++++++++++--
 3 files changed, 137 insertions(+), 14 deletions(-)

-- 
2.34.1

[PATCH mptcp-next v2 1/2] Squash to "mptcp: infinite mapping receiving"

[Geliang Tang]

Print out the infinite map received info.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/subflow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 485f00dcaf84..ae4d855f3c2f 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -962,6 +962,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 
 	data_len = mpext->data_len;
 	if (data_len == 0) {
+		pr_debug("infinite mapping received");
 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
 		subflow->map_data_len = 0;
 		return MAPPING_INVALID;
-- 
2.34.1

[PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases

[Geliang Tang]

Added the test cases for MP_FAIL, the multiple subflows test for MP_RST
and the single subflow test for the infinite mapping, use 'tc' commands
to trigger the checksum failures.

Suggested-by: Davide Caratti <dcaratti@redhat.com>
Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/config      |   8 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 142 ++++++++++++++++--
 2 files changed, 136 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
index d36b7da5082a..38021a0dd527 100644
--- a/tools/testing/selftests/net/mptcp/config
+++ b/tools/testing/selftests/net/mptcp/config
@@ -12,6 +12,9 @@ CONFIG_NF_TABLES=m
 CONFIG_NFT_COMPAT=m
 CONFIG_NETFILTER_XTABLES=m
 CONFIG_NETFILTER_XT_MATCH_BPF=m
+CONFIG_NETFILTER_XT_MATCH_LENGTH=m
+CONFIG_NETFILTER_XT_MATCH_STATISTIC=m
+CONFIG_NETFILTER_XT_TARGET_MARK=m
 CONFIG_NF_TABLES_INET=y
 CONFIG_NFT_TPROXY=m
 CONFIG_NFT_SOCKET=m
@@ -19,3 +22,8 @@ CONFIG_IP_ADVANCED_ROUTER=y
 CONFIG_IP_MULTIPLE_TABLES=y
 CONFIG_IP_NF_TARGET_REJECT=m
 CONFIG_IPV6_MULTIPLE_TABLES=y
+CONFIG_NET_ACT_CSUM=m
+CONFIG_NET_ACT_PEDIT=m
+CONFIG_NET_CLS_ACT=y
+CONFIG_NET_CLS_FW=m
+CONFIG_NET_SCH_INGRESS=m
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 90a6adc36490..686c54276ea9 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -17,6 +17,8 @@ capture=0
 checksum=0
 ip_mptcp=0
 check_invert=0
+validate_checksum=0
+nr_mp_fail=0
 do_all_tests=1
 
 TEST_COUNT=0
@@ -62,6 +64,7 @@ init()
 	done
 
 	check_invert=0
+	validate_checksum=$checksum
 
 	#  ns1              ns2
 	# ns1eth1    ns2eth1
@@ -167,6 +170,52 @@ reset_with_allow_join_id0()
 	ip netns exec $ns2 sysctl -q net.mptcp.allow_join_initial_addr_port=$ns2_enable
 }
 
+# Modify TCP payload without corrupting the TCP packet
+#
+# This rule inverts a 8-bit word at byte offset 148 for the 2nd TCP ACK packets
+# carrying enough data.
+# Once it is done, the TCP Checksum field is updated so the packet is still
+# considered as valid at the TCP level.
+# Because the MPTCP checksum, covering the TCP options and data, has not been
+# updated, the modification will be detected and an MP_FAIL will be emitted:
+# what we want to validate here without corrupting "random" MPTCP options.
+#
+# To avoid having tc producing this pr_info() message for each TCP ACK packets
+# not carrying enough data:
+#
+#     tc action pedit offset 162 out of bounds
+#
+# Netfilter is used to mark packets with enough data.
+reset_with_fail()
+{
+	reset
+
+	ip netns exec $ns1 sysctl -q net.mptcp.checksum_enabled=1
+	ip netns exec $ns2 sysctl -q net.mptcp.checksum_enabled=1
+
+	check_invert=1
+	validate_checksum=1
+	nr_mp_fail=0
+	local i="$1"
+
+	ip netns exec $ns2 iptables \
+		-t mangle \
+		-A OUTPUT \
+		-o ns2eth$i \
+		-p tcp \
+		-m length --length 150:9999 \
+		-m statistic --mode nth --packet 1 --every 99999 \
+		-j MARK --set-mark 42
+
+	tc -n $ns2 qdisc add dev ns2eth$i clsact
+	tc -n $ns2 filter add dev ns2eth$i egress \
+		protocol ip prio 1000 \
+		handle 42 fw \
+		action pedit munge offset 148 u8 invert \
+		pipe csum tcp \
+		index 100
+}
+
 ip -Version > /dev/null 2>&1
 if [ $? -ne 0 ];then
 	echo "SKIP: Could not run test without ip tool"
@@ -185,6 +234,12 @@ if [ $? -ne 0 ];then
 	exit $ksft_skip
 fi
 
+jq -V > /dev/null 2>&1
+if [ $? -ne 0 ];then
+	echo "SKIP: Could not run all tests without jq tool"
+	exit $ksft_skip
+fi
+
 print_file_err()
 {
 	ls -l "$1" 1>&2
@@ -245,6 +300,19 @@ link_failure()
 	done
 }
 
+get_nr_mp_fail()
+{
+	i="$1"
+
+	local action=$(tc -n $ns2 -j -s action show action pedit index 100)
+	local packets=$(echo $action | jq '.[1].actions[0].stats.packets')
+
+	if [ $packets -gt 0 ]; then
+		nr_mp_fail=1
+	fi
+	tc -n $ns2 qdisc del dev ns2eth$i clsact
+}
+
 # $1: IP address
 is_v6()
 {
@@ -446,7 +514,7 @@ do_transfer()
 		local_addr="0.0.0.0"
 	fi
 
-	if [ "$test_link_fail" -eq 2 ];then
+	if [ "$test_link_fail" -gt 1 ];then
 		timeout ${timeout_test} \
 			ip netns exec ${listener_ns} \
 				$mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} \
@@ -466,13 +534,19 @@ do_transfer()
 			ip netns exec ${connector_ns} \
 				$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
 					$connect_addr < "$cin" > "$cout" &
-	else
+	elif [ "$test_link_fail" -eq 1 ] || [ "$test_link_fail" -eq 2 ];then
 		( cat "$cinfail" ; sleep 2; link_failure $listener_ns ; cat "$cinfail" ) | \
 			tee "$cinsent" | \
 			timeout ${timeout_test} \
 				ip netns exec ${connector_ns} \
 					$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
 						$connect_addr > "$cout" &
+	else
+		cat "$cinfail" | tee "$cinsent" | \
+		timeout ${timeout_test} \
+			ip netns exec ${connector_ns} \
+				$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
+					$connect_addr > "$cout" &
 	fi
 	cpid=$!
 
@@ -632,7 +706,7 @@ do_transfer()
 		return 1
 	fi
 
-	if [ "$test_link_fail" -eq 2 ];then
+	if [ "$test_link_fail" -gt 1 ];then
 		check_transfer $sinfail $cout "file received by client"
 	else
 		check_transfer $sin $cout "file received by client"
@@ -681,7 +755,12 @@ run_tests()
 
 	# create the input file for the failure test when
 	# the first failure test run
-	if [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then
+	if [ "$test_linkfail" -eq 3 ]; then
+		if [ -z "$cinfail" ]; then
+			cinfail=$(mktemp)
+		fi
+		make_file "$cinfail" "client" 512
+	elif [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then
 		# the client file must be considerably larger
 		# of the maximum expected cwin value, or the
 		# link utilization will be not predicable
@@ -694,7 +773,12 @@ run_tests()
 		make_file "$cinfail" "client" $size
 	fi
 
-	if [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
+	if [ "$test_linkfail" -eq 3 ]; then
+		if [ -z "$sinfail" ]; then
+			sinfail=$(mktemp)
+		fi
+		make_file "$sinfail" "server" 512
+	elif [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
 		size=$((RANDOM%16))
 		size=$((size+1))
 		size=$((size*2048))
@@ -719,6 +803,8 @@ dump_stats()
 chk_csum_nr()
 {
 	local msg=${1:-""}
+	local csum_ns1=${2:-0}
+	local csum_ns2=${3:-0}
 	local count
 	local dump_stats
 
@@ -730,8 +816,8 @@ chk_csum_nr()
 	printf " %-36s %s" "$msg" "sum"
 	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtDataCsumErr | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != 0 ]; then
-		echo "[fail] got $count data checksum error[s] expected 0"
+	if [ "$count" != $csum_ns1 ]; then
+		echo "[fail] got $count data checksum error[s] expected $csum_ns1"
 		ret=1
 		dump_stats=1
 	else
@@ -740,8 +826,8 @@ chk_csum_nr()
 	echo -n " - csum  "
 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtDataCsumErr | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != 0 ]; then
-		echo "[fail] got $count data checksum error[s] expected 0"
+	if [ "$count" != $csum_ns2 ]; then
+		echo "[fail] got $count data checksum error[s] expected $csum_ns2"
 		ret=1
 		dump_stats=1
 	else
@@ -820,6 +906,8 @@ chk_join_nr()
 	local syn_nr=$2
 	local syn_ack_nr=$3
 	local ack_nr=$4
+	local fail_nr=${5:-0}
+	local infi_nr=${6:-0}
 	local count
 	local dump_stats
 
@@ -856,10 +944,10 @@ chk_join_nr()
 		echo "[ ok ]"
 	fi
 	[ "${dump_stats}" = 1 ] && dump_stats
-	if [ $checksum -eq 1 ]; then
-		chk_csum_nr
-		chk_fail_nr 0 0
-		chk_infi_nr 0 0
+	if [ $validate_checksum -eq 1 ]; then
+		chk_csum_nr "" $fail_nr
+		chk_fail_nr $fail_nr $fail_nr
+		chk_infi_nr $infi_nr $infi_nr
 	fi
 }
 
@@ -2164,6 +2252,27 @@ userspace_tests()
 	chk_rm_nr 0 0
 }
 
+fail_tests()
+{
+	# multiple subflows
+	reset_with_fail 2
+	pm_nl_set_limits $ns1 0 2
+	pm_nl_set_limits $ns2 0 2
+	pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
+	pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
+	run_tests $ns1 $ns2 10.0.1.1 3
+	get_nr_mp_fail 2
+	chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 2 $nr_mp_fail
+
+	# single subflow
+	reset_with_fail 1
+	pm_nl_set_limits $ns1 0 2
+	pm_nl_set_limits $ns2 0 2
+	run_tests $ns1 $ns2 10.0.1.1 3
+	get_nr_mp_fail 1
+	chk_join_nr "$nr_mp_fail MP_FAIL, single subflow, infinite" 0 0 0 $nr_mp_fail $nr_mp_fail
+}
+
 all_tests()
 {
 	subflows_tests
@@ -2182,6 +2291,7 @@ all_tests()
 	deny_join_id0_tests
 	fullmesh_tests
 	userspace_tests
+	fail_tests
 }
 
 usage()
@@ -2203,6 +2313,7 @@ usage()
 	echo "  -d deny_join_id0_tests"
 	echo "  -m fullmesh_tests"
 	echo "  -u userspace_tests"
+	echo "  -F fail_tests"
 	echo "  -c capture pcap files"
 	echo "  -C enable data checksum"
 	echo "  -i use ip mptcp"
@@ -2242,7 +2353,7 @@ if [ $do_all_tests -eq 1 ]; then
 	exit $ret
 fi
 
-while getopts 'fesltra64bpkdmuchCSi' opt; do
+while getopts 'fesltra64bpkdmuchCSFi' opt; do
 	case $opt in
 		f)
 			subflows_tests
@@ -2292,6 +2403,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do
 		u)
 			userspace_tests
 			;;
+		F)
+			fail_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.34.1

Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases

[Mat Martineau]

On Wed, 9 Feb 2022, Geliang Tang wrote:

> Added the test cases for MP_FAIL, the multiple subflows test for MP_RST
> and the single subflow test for the infinite mapping, use 'tc' commands
> to trigger the checksum failures.
>
> Suggested-by: Davide Caratti <dcaratti@redhat.com>
> Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>

Once I built a correctly configured kernel, this worked great! Thanks 
Geliang and Matthieu.

Before I fixed my configuration, I ran in to a couple of issues, see 
below.

> ---
> tools/testing/selftests/net/mptcp/config      |   8 +
> .../testing/selftests/net/mptcp/mptcp_join.sh | 142 ++++++++++++++++--
> 2 files changed, 136 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index d36b7da5082a..38021a0dd527 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -12,6 +12,9 @@ CONFIG_NF_TABLES=m
> CONFIG_NFT_COMPAT=m
> CONFIG_NETFILTER_XTABLES=m
> CONFIG_NETFILTER_XT_MATCH_BPF=m
> +CONFIG_NETFILTER_XT_MATCH_LENGTH=m
> +CONFIG_NETFILTER_XT_MATCH_STATISTIC=m
> +CONFIG_NETFILTER_XT_TARGET_MARK=m
> CONFIG_NF_TABLES_INET=y
> CONFIG_NFT_TPROXY=m
> CONFIG_NFT_SOCKET=m
> @@ -19,3 +22,8 @@ CONFIG_IP_ADVANCED_ROUTER=y
> CONFIG_IP_MULTIPLE_TABLES=y
> CONFIG_IP_NF_TARGET_REJECT=m
> CONFIG_IPV6_MULTIPLE_TABLES=y
> +CONFIG_NET_ACT_CSUM=m
> +CONFIG_NET_ACT_PEDIT=m
> +CONFIG_NET_CLS_ACT=y
> +CONFIG_NET_CLS_FW=m
> +CONFIG_NET_SCH_INGRESS=m
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 90a6adc36490..686c54276ea9 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -17,6 +17,8 @@ capture=0
> checksum=0
> ip_mptcp=0
> check_invert=0
> +validate_checksum=0
> +nr_mp_fail=0
> do_all_tests=1
>
> TEST_COUNT=0
> @@ -62,6 +64,7 @@ init()
> 	done
>
> 	check_invert=0
> +	validate_checksum=$checksum
>
> 	#  ns1              ns2
> 	# ns1eth1    ns2eth1
> @@ -167,6 +170,52 @@ reset_with_allow_join_id0()
> 	ip netns exec $ns2 sysctl -q net.mptcp.allow_join_initial_addr_port=$ns2_enable
> }
>
> +# Modify TCP payload without corrupting the TCP packet
> +#
> +# This rule inverts a 8-bit word at byte offset 148 for the 2nd TCP ACK packets
> +# carrying enough data.
> +# Once it is done, the TCP Checksum field is updated so the packet is still
> +# considered as valid at the TCP level.
> +# Because the MPTCP checksum, covering the TCP options and data, has not been
> +# updated, the modification will be detected and an MP_FAIL will be emitted:
> +# what we want to validate here without corrupting "random" MPTCP options.
> +#
> +# To avoid having tc producing this pr_info() message for each TCP ACK packets
> +# not carrying enough data:
> +#
> +#     tc action pedit offset 162 out of bounds
> +#
> +# Netfilter is used to mark packets with enough data.
> +reset_with_fail()
> +{
> +	reset
> +
> +	ip netns exec $ns1 sysctl -q net.mptcp.checksum_enabled=1
> +	ip netns exec $ns2 sysctl -q net.mptcp.checksum_enabled=1
> +
> +	check_invert=1
> +	validate_checksum=1
> +	nr_mp_fail=0
> +	local i="$1"
> +
> +	ip netns exec $ns2 iptables \
> +		-t mangle \
> +		-A OUTPUT \
> +		-o ns2eth$i \
> +		-p tcp \
> +		-m length --length 150:9999 \
> +		-m statistic --mode nth --packet 1 --every 99999 \
> +		-j MARK --set-mark 42
> +

I think it would help to check the exit code of iptables here, my first 
attempt was with a misconfigured kernel and it would help to fail in a 
more informative way:

$ sudo ./mptcp_join.sh -F
Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client
Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server
iptables v1.8.7 (legacy): Couldn't load match `length':No such file or directory
Try `iptables -h' or 'iptables --help' for more information.
Error: TC classifier not found.
We have an error talking to the kernel


> +	tc -n $ns2 qdisc add dev ns2eth$i clsact
> +	tc -n $ns2 filter add dev ns2eth$i egress \
> +		protocol ip prio 1000 \
> +		handle 42 fw \
> +		action pedit munge offset 148 u8 invert \
> +		pipe csum tcp \
> +		index 100
> +}
> +
> ip -Version > /dev/null 2>&1
> if [ $? -ne 0 ];then
> 	echo "SKIP: Could not run test without ip tool"
> @@ -185,6 +234,12 @@ if [ $? -ne 0 ];then
> 	exit $ksft_skip
> fi
>
> +jq -V > /dev/null 2>&1
> +if [ $? -ne 0 ];then
> +	echo "SKIP: Could not run all tests without jq tool"
> +	exit $ksft_skip
> +fi
> +
> print_file_err()
> {
> 	ls -l "$1" 1>&2
> @@ -245,6 +300,19 @@ link_failure()
> 	done
> }
>
> +get_nr_mp_fail()
> +{
> +	i="$1"
> +
> +	local action=$(tc -n $ns2 -j -s action show action pedit index 100)
> +	local packets=$(echo $action | jq '.[1].actions[0].stats.packets')
> +
> +	if [ $packets -gt 0 ]; then

This triggered a bash parsing error when I did not have the added 
netfilter kernel config options:

$ sudo ./mptcp_join.sh -F
Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client
Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server
iptables v1.8.7 (legacy): Couldn't load match `length':No such file or directory
Try `iptables -h' or 'iptables --help' for more information.
Error: TC classifier not found.
We have an error talking to the kernel
Created /tmp/tmp.CvX8qnG1gC (size 512 KB) containing data sent by client
Created /tmp/tmp.wenxUAgsj8 (size 512 KB) containing data sent by server
./mptcp_join.sh: line 310: [: null: integer expression expected
001 0 MP_FAIL, multiple subflows, MP_RST syn[ ok ] - synack[ ok ] - ack[ ok ]
                                          sum[ ok ] - csum  [ ok ]
                                          ftx[ ok ] - frx   [ ok ]
                                          itx[ ok ] - irx   [ ok ]



> +		nr_mp_fail=1
> +	fi
> +	tc -n $ns2 qdisc del dev ns2eth$i clsact
> +}
> +
> # $1: IP address
> is_v6()
> {
> @@ -446,7 +514,7 @@ do_transfer()
> 		local_addr="0.0.0.0"
> 	fi
>
> -	if [ "$test_link_fail" -eq 2 ];then
> +	if [ "$test_link_fail" -gt 1 ];then
> 		timeout ${timeout_test} \
> 			ip netns exec ${listener_ns} \
> 				$mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} \
> @@ -466,13 +534,19 @@ do_transfer()
> 			ip netns exec ${connector_ns} \
> 				$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
> 					$connect_addr < "$cin" > "$cout" &
> -	else
> +	elif [ "$test_link_fail" -eq 1 ] || [ "$test_link_fail" -eq 2 ];then
> 		( cat "$cinfail" ; sleep 2; link_failure $listener_ns ; cat "$cinfail" ) | \
> 			tee "$cinsent" | \
> 			timeout ${timeout_test} \
> 				ip netns exec ${connector_ns} \
> 					$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
> 						$connect_addr > "$cout" &
> +	else
> +		cat "$cinfail" | tee "$cinsent" | \
> +		timeout ${timeout_test} \
> +			ip netns exec ${connector_ns} \
> +				$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
> +					$connect_addr > "$cout" &
> 	fi
> 	cpid=$!
>
> @@ -632,7 +706,7 @@ do_transfer()
> 		return 1
> 	fi
>
> -	if [ "$test_link_fail" -eq 2 ];then
> +	if [ "$test_link_fail" -gt 1 ];then
> 		check_transfer $sinfail $cout "file received by client"
> 	else
> 		check_transfer $sin $cout "file received by client"
> @@ -681,7 +755,12 @@ run_tests()
>
> 	# create the input file for the failure test when
> 	# the first failure test run
> -	if [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then
> +	if [ "$test_linkfail" -eq 3 ]; then
> +		if [ -z "$cinfail" ]; then
> +			cinfail=$(mktemp)
> +		fi
> +		make_file "$cinfail" "client" 512
> +	elif [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then
> 		# the client file must be considerably larger
> 		# of the maximum expected cwin value, or the
> 		# link utilization will be not predicable
> @@ -694,7 +773,12 @@ run_tests()
> 		make_file "$cinfail" "client" $size
> 	fi
>
> -	if [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
> +	if [ "$test_linkfail" -eq 3 ]; then
> +		if [ -z "$sinfail" ]; then
> +			sinfail=$(mktemp)
> +		fi
> +		make_file "$sinfail" "server" 512
> +	elif [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
> 		size=$((RANDOM%16))
> 		size=$((size+1))
> 		size=$((size*2048))
> @@ -719,6 +803,8 @@ dump_stats()
> chk_csum_nr()
> {
> 	local msg=${1:-""}
> +	local csum_ns1=${2:-0}
> +	local csum_ns2=${3:-0}
> 	local count
> 	local dump_stats
>
> @@ -730,8 +816,8 @@ chk_csum_nr()
> 	printf " %-36s %s" "$msg" "sum"
> 	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtDataCsumErr | awk '{print $2}'`
> 	[ -z "$count" ] && count=0
> -	if [ "$count" != 0 ]; then
> -		echo "[fail] got $count data checksum error[s] expected 0"
> +	if [ "$count" != $csum_ns1 ]; then
> +		echo "[fail] got $count data checksum error[s] expected $csum_ns1"
> 		ret=1
> 		dump_stats=1
> 	else
> @@ -740,8 +826,8 @@ chk_csum_nr()
> 	echo -n " - csum  "
> 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtDataCsumErr | awk '{print $2}'`
> 	[ -z "$count" ] && count=0
> -	if [ "$count" != 0 ]; then
> -		echo "[fail] got $count data checksum error[s] expected 0"
> +	if [ "$count" != $csum_ns2 ]; then
> +		echo "[fail] got $count data checksum error[s] expected $csum_ns2"
> 		ret=1
> 		dump_stats=1
> 	else
> @@ -820,6 +906,8 @@ chk_join_nr()
> 	local syn_nr=$2
> 	local syn_ack_nr=$3
> 	local ack_nr=$4
> +	local fail_nr=${5:-0}
> +	local infi_nr=${6:-0}
> 	local count
> 	local dump_stats
>
> @@ -856,10 +944,10 @@ chk_join_nr()
> 		echo "[ ok ]"
> 	fi
> 	[ "${dump_stats}" = 1 ] && dump_stats
> -	if [ $checksum -eq 1 ]; then
> -		chk_csum_nr
> -		chk_fail_nr 0 0
> -		chk_infi_nr 0 0
> +	if [ $validate_checksum -eq 1 ]; then
> +		chk_csum_nr "" $fail_nr
> +		chk_fail_nr $fail_nr $fail_nr
> +		chk_infi_nr $infi_nr $infi_nr
> 	fi
> }
>
> @@ -2164,6 +2252,27 @@ userspace_tests()
> 	chk_rm_nr 0 0
> }
>
> +fail_tests()
> +{
> +	# multiple subflows
> +	reset_with_fail 2
> +	pm_nl_set_limits $ns1 0 2
> +	pm_nl_set_limits $ns2 0 2
> +	pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
> +	pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
> +	run_tests $ns1 $ns2 10.0.1.1 3
> +	get_nr_mp_fail 2
> +	chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 2 $nr_mp_fail

Is the first $nr_mp_fail supposed to be in the test description? It's not 
clear what it means in the test output.

> +
> +	# single subflow
> +	reset_with_fail 1
> +	pm_nl_set_limits $ns1 0 2
> +	pm_nl_set_limits $ns2 0 2
> +	run_tests $ns1 $ns2 10.0.1.1 3
> +	get_nr_mp_fail 1
> +	chk_join_nr "$nr_mp_fail MP_FAIL, single subflow, infinite" 0 0 0 $nr_mp_fail $nr_mp_fail

                      ^^^^^^^^^^^ same question

> +}
> +
> all_tests()
> {
> 	subflows_tests
> @@ -2182,6 +2291,7 @@ all_tests()
> 	deny_join_id0_tests
> 	fullmesh_tests
> 	userspace_tests
> +	fail_tests
> }
>
> usage()
> @@ -2203,6 +2313,7 @@ usage()
> 	echo "  -d deny_join_id0_tests"
> 	echo "  -m fullmesh_tests"
> 	echo "  -u userspace_tests"
> +	echo "  -F fail_tests"
> 	echo "  -c capture pcap files"
> 	echo "  -C enable data checksum"
> 	echo "  -i use ip mptcp"
> @@ -2242,7 +2353,7 @@ if [ $do_all_tests -eq 1 ]; then
> 	exit $ret
> fi
>
> -while getopts 'fesltra64bpkdmuchCSi' opt; do
> +while getopts 'fesltra64bpkdmuchCSFi' opt; do
> 	case $opt in
> 		f)
> 			subflows_tests
> @@ -2292,6 +2403,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do
> 		u)
> 			userspace_tests
> 			;;
> +		F)
> +			fail_tests
> +			;;
> 		c)
> 			;;
> 		C)
> -- 
> 2.34.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases

[Geliang Tang]

Hi Mat,

Thanks for your review.

On Tue, Feb 08, 2022 at 06:10:20PM -0800, Mat Martineau wrote:
> On Wed, 9 Feb 2022, Geliang Tang wrote:
> 
> > Added the test cases for MP_FAIL, the multiple subflows test for MP_RST
> > and the single subflow test for the infinite mapping, use 'tc' commands
> > to trigger the checksum failures.
> > 
> > Suggested-by: Davide Caratti <dcaratti@redhat.com>
> > Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> > Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> 
> Once I built a correctly configured kernel, this worked great! Thanks
> Geliang and Matthieu.
> 
> Before I fixed my configuration, I ran in to a couple of issues, see below.
> 
> > ---
> > tools/testing/selftests/net/mptcp/config      |   8 +
> > .../testing/selftests/net/mptcp/mptcp_join.sh | 142 ++++++++++++++++--
> > 2 files changed, 136 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> > index d36b7da5082a..38021a0dd527 100644
> > --- a/tools/testing/selftests/net/mptcp/config
> > +++ b/tools/testing/selftests/net/mptcp/config
> > @@ -12,6 +12,9 @@ CONFIG_NF_TABLES=m
> > CONFIG_NFT_COMPAT=m
> > CONFIG_NETFILTER_XTABLES=m
> > CONFIG_NETFILTER_XT_MATCH_BPF=m
> > +CONFIG_NETFILTER_XT_MATCH_LENGTH=m
> > +CONFIG_NETFILTER_XT_MATCH_STATISTIC=m
> > +CONFIG_NETFILTER_XT_TARGET_MARK=m
> > CONFIG_NF_TABLES_INET=y
> > CONFIG_NFT_TPROXY=m
> > CONFIG_NFT_SOCKET=m
> > @@ -19,3 +22,8 @@ CONFIG_IP_ADVANCED_ROUTER=y
> > CONFIG_IP_MULTIPLE_TABLES=y
> > CONFIG_IP_NF_TARGET_REJECT=m
> > CONFIG_IPV6_MULTIPLE_TABLES=y
> > +CONFIG_NET_ACT_CSUM=m
> > +CONFIG_NET_ACT_PEDIT=m
> > +CONFIG_NET_CLS_ACT=y
> > +CONFIG_NET_CLS_FW=m
> > +CONFIG_NET_SCH_INGRESS=m
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 90a6adc36490..686c54276ea9 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -17,6 +17,8 @@ capture=0
> > checksum=0
> > ip_mptcp=0
> > check_invert=0
> > +validate_checksum=0
> > +nr_mp_fail=0
> > do_all_tests=1
> > 
> > TEST_COUNT=0
> > @@ -62,6 +64,7 @@ init()
> > 	done
> > 
> > 	check_invert=0
> > +	validate_checksum=$checksum
> > 
> > 	#  ns1              ns2
> > 	# ns1eth1    ns2eth1
> > @@ -167,6 +170,52 @@ reset_with_allow_join_id0()
> > 	ip netns exec $ns2 sysctl -q net.mptcp.allow_join_initial_addr_port=$ns2_enable
> > }
> > 
> > +# Modify TCP payload without corrupting the TCP packet
> > +#
> > +# This rule inverts a 8-bit word at byte offset 148 for the 2nd TCP ACK packets
> > +# carrying enough data.
> > +# Once it is done, the TCP Checksum field is updated so the packet is still
> > +# considered as valid at the TCP level.
> > +# Because the MPTCP checksum, covering the TCP options and data, has not been
> > +# updated, the modification will be detected and an MP_FAIL will be emitted:
> > +# what we want to validate here without corrupting "random" MPTCP options.
> > +#
> > +# To avoid having tc producing this pr_info() message for each TCP ACK packets
> > +# not carrying enough data:
> > +#
> > +#     tc action pedit offset 162 out of bounds
> > +#
> > +# Netfilter is used to mark packets with enough data.
> > +reset_with_fail()
> > +{
> > +	reset
> > +
> > +	ip netns exec $ns1 sysctl -q net.mptcp.checksum_enabled=1
> > +	ip netns exec $ns2 sysctl -q net.mptcp.checksum_enabled=1
> > +
> > +	check_invert=1
> > +	validate_checksum=1
> > +	nr_mp_fail=0
> > +	local i="$1"
> > +
> > +	ip netns exec $ns2 iptables \
> > +		-t mangle \
> > +		-A OUTPUT \
> > +		-o ns2eth$i \
> > +		-p tcp \
> > +		-m length --length 150:9999 \
> > +		-m statistic --mode nth --packet 1 --every 99999 \
> > +		-j MARK --set-mark 42
> > +
> 
> I think it would help to check the exit code of iptables here, my first
> attempt was with a misconfigured kernel and it would help to fail in a more
> informative way:
> 
> $ sudo ./mptcp_join.sh -F
> Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client
> Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server
> iptables v1.8.7 (legacy): Couldn't load match `length':No such file or directory
> Try `iptables -h' or 'iptables --help' for more information.
> Error: TC classifier not found.
> We have an error talking to the kernel
> 

Done in v3.

> 
> > +	tc -n $ns2 qdisc add dev ns2eth$i clsact
> > +	tc -n $ns2 filter add dev ns2eth$i egress \
> > +		protocol ip prio 1000 \
> > +		handle 42 fw \
> > +		action pedit munge offset 148 u8 invert \
> > +		pipe csum tcp \
> > +		index 100
> > +}
> > +
> > ip -Version > /dev/null 2>&1
> > if [ $? -ne 0 ];then
> > 	echo "SKIP: Could not run test without ip tool"
> > @@ -185,6 +234,12 @@ if [ $? -ne 0 ];then
> > 	exit $ksft_skip
> > fi
> > 
> > +jq -V > /dev/null 2>&1
> > +if [ $? -ne 0 ];then
> > +	echo "SKIP: Could not run all tests without jq tool"
> > +	exit $ksft_skip
> > +fi
> > +
> > print_file_err()
> > {
> > 	ls -l "$1" 1>&2
> > @@ -245,6 +300,19 @@ link_failure()
> > 	done
> > }
> > 
> > +get_nr_mp_fail()
> > +{
> > +	i="$1"
> > +
> > +	local action=$(tc -n $ns2 -j -s action show action pedit index 100)
> > +	local packets=$(echo $action | jq '.[1].actions[0].stats.packets')
> > +
> > +	if [ $packets -gt 0 ]; then
> 
> This triggered a bash parsing error when I did not have the added netfilter
> kernel config options:
> 
> $ sudo ./mptcp_join.sh -F
> Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client
> Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server
> iptables v1.8.7 (legacy): Couldn't load match `length':No such file or directory
> Try `iptables -h' or 'iptables --help' for more information.
> Error: TC classifier not found.
> We have an error talking to the kernel
> Created /tmp/tmp.CvX8qnG1gC (size 512 KB) containing data sent by client
> Created /tmp/tmp.wenxUAgsj8 (size 512 KB) containing data sent by server
> ./mptcp_join.sh: line 310: [: null: integer expression expected
> 001 0 MP_FAIL, multiple subflows, MP_RST syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                          sum[ ok ] - csum  [ ok ]
>                                          ftx[ ok ] - frx   [ ok ]
>                                          itx[ ok ] - irx   [ ok ]
> 

Done in v3.

> 
> 
> > +		nr_mp_fail=1
> > +	fi
> > +	tc -n $ns2 qdisc del dev ns2eth$i clsact
> > +}
> > +
> > # $1: IP address
> > is_v6()
> > {
> > @@ -446,7 +514,7 @@ do_transfer()
> > 		local_addr="0.0.0.0"
> > 	fi
> > 
> > -	if [ "$test_link_fail" -eq 2 ];then
> > +	if [ "$test_link_fail" -gt 1 ];then
> > 		timeout ${timeout_test} \
> > 			ip netns exec ${listener_ns} \
> > 				$mptcp_connect -t ${timeout_poll} -l -p $port -s ${srv_proto} \
> > @@ -466,13 +534,19 @@ do_transfer()
> > 			ip netns exec ${connector_ns} \
> > 				$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
> > 					$connect_addr < "$cin" > "$cout" &
> > -	else
> > +	elif [ "$test_link_fail" -eq 1 ] || [ "$test_link_fail" -eq 2 ];then
> > 		( cat "$cinfail" ; sleep 2; link_failure $listener_ns ; cat "$cinfail" ) | \
> > 			tee "$cinsent" | \
> > 			timeout ${timeout_test} \
> > 				ip netns exec ${connector_ns} \
> > 					$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
> > 						$connect_addr > "$cout" &
> > +	else
> > +		cat "$cinfail" | tee "$cinsent" | \
> > +		timeout ${timeout_test} \
> > +			ip netns exec ${connector_ns} \
> > +				$mptcp_connect -t ${timeout_poll} -p $port -s ${cl_proto} \
> > +					$connect_addr > "$cout" &
> > 	fi
> > 	cpid=$!
> > 
> > @@ -632,7 +706,7 @@ do_transfer()
> > 		return 1
> > 	fi
> > 
> > -	if [ "$test_link_fail" -eq 2 ];then
> > +	if [ "$test_link_fail" -gt 1 ];then
> > 		check_transfer $sinfail $cout "file received by client"
> > 	else
> > 		check_transfer $sin $cout "file received by client"
> > @@ -681,7 +755,12 @@ run_tests()
> > 
> > 	# create the input file for the failure test when
> > 	# the first failure test run
> > -	if [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then
> > +	if [ "$test_linkfail" -eq 3 ]; then
> > +		if [ -z "$cinfail" ]; then
> > +			cinfail=$(mktemp)
> > +		fi
> > +		make_file "$cinfail" "client" 512
> > +	elif [ "$test_linkfail" -ne 0 -a -z "$cinfail" ]; then
> > 		# the client file must be considerably larger
> > 		# of the maximum expected cwin value, or the
> > 		# link utilization will be not predicable
> > @@ -694,7 +773,12 @@ run_tests()
> > 		make_file "$cinfail" "client" $size
> > 	fi
> > 
> > -	if [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
> > +	if [ "$test_linkfail" -eq 3 ]; then
> > +		if [ -z "$sinfail" ]; then
> > +			sinfail=$(mktemp)
> > +		fi
> > +		make_file "$sinfail" "server" 512
> > +	elif [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
> > 		size=$((RANDOM%16))
> > 		size=$((size+1))
> > 		size=$((size*2048))
> > @@ -719,6 +803,8 @@ dump_stats()
> > chk_csum_nr()
> > {
> > 	local msg=${1:-""}
> > +	local csum_ns1=${2:-0}
> > +	local csum_ns2=${3:-0}
> > 	local count
> > 	local dump_stats
> > 
> > @@ -730,8 +816,8 @@ chk_csum_nr()
> > 	printf " %-36s %s" "$msg" "sum"
> > 	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtDataCsumErr | awk '{print $2}'`
> > 	[ -z "$count" ] && count=0
> > -	if [ "$count" != 0 ]; then
> > -		echo "[fail] got $count data checksum error[s] expected 0"
> > +	if [ "$count" != $csum_ns1 ]; then
> > +		echo "[fail] got $count data checksum error[s] expected $csum_ns1"
> > 		ret=1
> > 		dump_stats=1
> > 	else
> > @@ -740,8 +826,8 @@ chk_csum_nr()
> > 	echo -n " - csum  "
> > 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtDataCsumErr | awk '{print $2}'`
> > 	[ -z "$count" ] && count=0
> > -	if [ "$count" != 0 ]; then
> > -		echo "[fail] got $count data checksum error[s] expected 0"
> > +	if [ "$count" != $csum_ns2 ]; then
> > +		echo "[fail] got $count data checksum error[s] expected $csum_ns2"
> > 		ret=1
> > 		dump_stats=1
> > 	else
> > @@ -820,6 +906,8 @@ chk_join_nr()
> > 	local syn_nr=$2
> > 	local syn_ack_nr=$3
> > 	local ack_nr=$4
> > +	local fail_nr=${5:-0}
> > +	local infi_nr=${6:-0}
> > 	local count
> > 	local dump_stats
> > 
> > @@ -856,10 +944,10 @@ chk_join_nr()
> > 		echo "[ ok ]"
> > 	fi
> > 	[ "${dump_stats}" = 1 ] && dump_stats
> > -	if [ $checksum -eq 1 ]; then
> > -		chk_csum_nr
> > -		chk_fail_nr 0 0
> > -		chk_infi_nr 0 0
> > +	if [ $validate_checksum -eq 1 ]; then
> > +		chk_csum_nr "" $fail_nr
> > +		chk_fail_nr $fail_nr $fail_nr
> > +		chk_infi_nr $infi_nr $infi_nr
> > 	fi
> > }
> > 
> > @@ -2164,6 +2252,27 @@ userspace_tests()
> > 	chk_rm_nr 0 0
> > }
> > 
> > +fail_tests()
> > +{
> > +	# multiple subflows
> > +	reset_with_fail 2
> > +	pm_nl_set_limits $ns1 0 2
> > +	pm_nl_set_limits $ns2 0 2
> > +	pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
> > +	pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
> > +	run_tests $ns1 $ns2 10.0.1.1 3
> > +	get_nr_mp_fail 2
> > +	chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 2 $nr_mp_fail
> 
> Is the first $nr_mp_fail supposed to be in the test description? It's not
> clear what it means in the test output.

I renamed nr_mp_fail to pedit_action and get_nr_mp_fail to
pedit_action_happened in v3. And updated the test descriptions like
this:
001 MP_FAIL MP_RST: 1 pedit action       syn[ ok ] - synack[ ok ] - ack[ ok ]
002 MP_FAIL infinite map: 1 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ]

I put the variable pedit_action in the test description to distinguish
between the following two cases:

1
Created /tmp/tmp.vizRgvd6NM (size 512 KB) containing data sent by client
Created /tmp/tmp.UZqbnNdWAK (size 512 KB) containing data sent by server
001 MP_FAIL MP_RST: 0 pedit action       syn[ ok ] - synack[ ok ] - ack[ ok ]
                                         sum[ ok ] - csum  [ ok ]
                                         ftx[ ok ] - frx   [ ok ]
                                         itx[ ok ] - irx   [ ok ]

All checks passed, and no inverted byte received. In this case, no
checksum failure happened.

2
Created /tmp/tmp.vizRgvd6NM (size 512 KB) containing data sent by client
Created /tmp/tmp.UZqbnNdWAK (size 512 KB) containing data sent by server
001 MP_FAIL MP_RST: 1 pedit action       syn[ ok ] - synack[ ok ] - ack[ ok ]
                                         sum[ ok ] - csum  [ ok ]
                                         ftx[ ok ] - frx   [ ok ]
                                         itx[ ok ] - irx   [ ok ]

All checks passed, and no inverted byte received. In this case, the bad
data is dropped correctly.

If no pedit_action showed in the test description, the outputs of these
two cases will be the same.

Thanks,
-Geliang

> 
> > +
> > +	# single subflow
> > +	reset_with_fail 1
> > +	pm_nl_set_limits $ns1 0 2
> > +	pm_nl_set_limits $ns2 0 2
> > +	run_tests $ns1 $ns2 10.0.1.1 3
> > +	get_nr_mp_fail 1
> > +	chk_join_nr "$nr_mp_fail MP_FAIL, single subflow, infinite" 0 0 0 $nr_mp_fail $nr_mp_fail
> 
>                      ^^^^^^^^^^^ same question
> 
> > +}
> > +
> > all_tests()
> > {
> > 	subflows_tests
> > @@ -2182,6 +2291,7 @@ all_tests()
> > 	deny_join_id0_tests
> > 	fullmesh_tests
> > 	userspace_tests
> > +	fail_tests
> > }
> > 
> > usage()
> > @@ -2203,6 +2313,7 @@ usage()
> > 	echo "  -d deny_join_id0_tests"
> > 	echo "  -m fullmesh_tests"
> > 	echo "  -u userspace_tests"
> > +	echo "  -F fail_tests"
> > 	echo "  -c capture pcap files"
> > 	echo "  -C enable data checksum"
> > 	echo "  -i use ip mptcp"
> > @@ -2242,7 +2353,7 @@ if [ $do_all_tests -eq 1 ]; then
> > 	exit $ret
> > fi
> > 
> > -while getopts 'fesltra64bpkdmuchCSi' opt; do
> > +while getopts 'fesltra64bpkdmuchCSFi' opt; do
> > 	case $opt in
> > 		f)
> > 			subflows_tests
> > @@ -2292,6 +2403,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do
> > 		u)
> > 			userspace_tests
> > 			;;
> > +		F)
> > +			fail_tests
> > +			;;
> > 		c)
> > 			;;
> > 		C)
> > -- 
> > 2.34.1
> > 
> > 
> > 
> 
> --
> Mat Martineau
> Intel
> 

Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases

[Matthieu Baerts]

Hi Mat,

On 09/02/2022 03:10, Mat Martineau wrote:
> On Wed, 9 Feb 2022, Geliang Tang wrote:

(...)

>> +    ip netns exec $ns2 iptables \
>> +        -t mangle \
>> +        -A OUTPUT \
>> +        -o ns2eth$i \
>> +        -p tcp \
>> +        -m length --length 150:9999 \
>> +        -m statistic --mode nth --packet 1 --every 99999 \
>> +        -j MARK --set-mark 42
>> +
> 
> I think it would help to check the exit code of iptables here, my first
> attempt was with a misconfigured kernel and it would help to fail in a
> more informative way:
> 
> $ sudo ./mptcp_join.sh -F
> Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client
> Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server
> iptables v1.8.7 (legacy): Couldn't load match `length':No such file or
> directory
> Try `iptables -h' or 'iptables --help' for more information.
> Error: TC classifier not found.
> We have an error talking to the kernel

I understand it is not good to ignore the error but just to be sure,
here you suggest to exit with an error, right?

In Geliang's v3, the whole test is skip but I don't think that's a good
idea.

I mean: we manage a kernel config file so we expect the right kernel
config is applied. If the kconfig is not the correct one, that's an error.
For the tools, that's different and that's why we check if iptables tool
is available. (a plugin might be missing but no need to check everything
I think).

In other words, I think we should add a " || exit 1" after the IPTables
and TC commands in this function.

@Geliang: WDYT?

(...)

>> +get_nr_mp_fail()
>> +{
>> +    i="$1"
>> +
>> +    local action=$(tc -n $ns2 -j -s action show action pedit index 100)
>> +    local packets=$(echo $action | jq '.[1].actions[0].stats.packets')
>> +
>> +    if [ $packets -gt 0 ]; then
> 
> This triggered a bash parsing error when I did not have the added
> netfilter kernel config options:
> 
> $ sudo ./mptcp_join.sh -F
> Created /tmp/tmp.P4AWbNYGQ5 (size 1 KB) containing data sent by client
> Created /tmp/tmp.ny1Q9MWo85 (size 1 KB) containing data sent by server
> iptables v1.8.7 (legacy): Couldn't load match `length':No such file or
> directory
> Try `iptables -h' or 'iptables --help' for more information.
> Error: TC classifier not found.
> We have an error talking to the kernel
> Created /tmp/tmp.CvX8qnG1gC (size 512 KB) containing data sent by client
> Created /tmp/tmp.wenxUAgsj8 (size 512 KB) containing data sent by server
> ./mptcp_join.sh: line 310: [: null: integer expression expected

I think we don't have to deal with this error if we already check if the
previous TC commands were OK, no?

(...)

>> +fail_tests()
>> +{
>> +    # multiple subflows
>> +    reset_with_fail 2
>> +    pm_nl_set_limits $ns1 0 2
>> +    pm_nl_set_limits $ns2 0 2
>> +    pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
>> +    pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
>> +    run_tests $ns1 $ns2 10.0.1.1 3
>> +    get_nr_mp_fail 2
>> +    chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2
>> 2 $nr_mp_fail
> 
> Is the first $nr_mp_fail supposed to be in the test description? It's
> not clear what it means in the test output.

In the v3, this is written after, it can help if this test fails.

But I don't think we should use it in the MP FAIL we expect. Otherwise
we will have the same situation Mat had: IPTables and TC commands fail
but the test showed no error :)

So we should have:

  chk_join_nr "MP_FAIL (...) $nr_mp_fail" 2 2 2 1

No?

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

Re: [PATCH mptcp-next v2 2/2] selftests: mptcp: add mp_fail testcases

[Matthieu Baerts]

Hi Geliang,

On 09/02/2022 06:47, Geliang Tang wrote:
> On Tue, Feb 08, 2022 at 06:10:20PM -0800, Mat Martineau wrote:
>> On Wed, 9 Feb 2022, Geliang Tang wrote:
>>> +fail_tests()
>>> +{
>>> +	# multiple subflows
>>> +	reset_with_fail 2
>>> +	pm_nl_set_limits $ns1 0 2
>>> +	pm_nl_set_limits $ns2 0 2
>>> +	pm_nl_add_endpoint $ns2 10.0.2.2 dev ns2eth2 flags subflow
>>> +	pm_nl_add_endpoint $ns2 10.0.3.2 dev ns2eth3 flags subflow
>>> +	run_tests $ns1 $ns2 10.0.1.1 3
>>> +	get_nr_mp_fail 2
>>> +	chk_join_nr "$nr_mp_fail MP_FAIL, multiple subflows, MP_RST" 2 2 2 $nr_mp_fail
>>
>> Is the first $nr_mp_fail supposed to be in the test description? It's not
>> clear what it means in the test output.
> 
> I renamed nr_mp_fail to pedit_action and get_nr_mp_fail to
> pedit_action_happened in v3. And updated the test descriptions like
> this:
> 001 MP_FAIL MP_RST: 1 pedit action       syn[ ok ] - synack[ ok ] - ack[ ok ]
> 002 MP_FAIL infinite map: 1 pedit action syn[ ok ] - synack[ ok ] - ack[ ok ]
> 
> I put the variable pedit_action in the test description to distinguish
> between the following two cases:
> 
> 1
> Created /tmp/tmp.vizRgvd6NM (size 512 KB) containing data sent by client
> Created /tmp/tmp.UZqbnNdWAK (size 512 KB) containing data sent by server
> 001 MP_FAIL MP_RST: 0 pedit action       syn[ ok ] - synack[ ok ] - ack[ ok ]
>                                          sum[ ok ] - csum  [ ok ]
>                                          ftx[ ok ] - frx   [ ok ]
>                                          itx[ ok ] - irx   [ ok ]
> 
> All checks passed, and no inverted byte received. In this case, no
> checksum failure happened.

I don't think we can say the test was a success in this case, no?
We expect one MP_FAIL. If we didn't have any, that's an issue.

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