[v1] add mp_fail testcases

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

Posted by Geliang Tang 4 years ago

Added the test cases for MP_FAIL, 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      |   5 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 148 +++++++++++++++---
 2 files changed, 135 insertions(+), 18 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
index d36b7da5082a..26955abe49f0 100644
--- a/tools/testing/selftests/net/mptcp/config
+++ b/tools/testing/selftests/net/mptcp/config
@@ -19,3 +19,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_FLOWER=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 76c45c845d3b..44176419ff1d 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_fail=0
 do_all_tests=1
 
 TEST_COUNT=0
@@ -61,6 +63,7 @@ init()
 	done
 
 	check_invert=0
+	validate_checksum=$checksum
 
 	#  ns1              ns2
 	# ns1eth1    ns2eth1
@@ -166,6 +169,46 @@ 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 32-bit word at byte offset 148 for all 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.
+# But because the MPTCP checksum, covering the TCP options and data,
+# has not been updated, we will detect the modification and emit an
+# MP_FAIL: what we want to validate here.
+#
+# Please note that this rule will produce this pr_info() message for
+# each TCP ACK packets not carrying enough data:
+#
+#     tc action pedit offset 162 out of bounds
+#
+# But this should be limited to a very few numbers of packets as we
+# restrict this rule to outgoing TCP traffic with only the ACK flag
+# + except the 3rd ACK, only packets carrying data should be seen in
+# this direction.
+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_fail=0
+	i="$1"
+
+	tc -n $ns2 qdisc add dev ns2eth$i clsact
+	tc -n $ns2 filter add dev ns2eth$i egress \
+		protocol ip prio 1000 \
+		flower ip_proto tcp tcp_flags 0x10/0xff \
+		action pedit munge offset 148 u32 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"
@@ -184,6 +227,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
@@ -244,6 +293,21 @@ link_failure()
 	done
 }
 
+get_nr_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')
+	local overlimits=$(echo $action | jq '.[1].actions[0].stats.overlimits')
+
+	let pkt=$packets-$overlimits
+	if [ $pkt -gt 0 ]; then
+		nr_fail=1
+	fi
+	tc -n $ns2 qdisc del dev ns2eth$i clsact
+}
+
 # $1: IP address
 is_v6()
 {
@@ -458,7 +522,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} \
@@ -478,13 +542,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=$!
 
@@ -652,7 +722,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"
@@ -701,7 +771,13 @@ 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
+		cinfail=$(mktemp)
+		make_file "$cinfail" "client" 10240
+	elif [ "$test_linkfail" -eq 4 ]; then
+		cinfail=$(mktemp)
+		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
@@ -714,7 +790,13 @@ run_tests()
 		make_file "$cinfail" "client" $size
 	fi
 
-	if [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
+	if [ "$test_linkfail" -eq 3 ]; then
+		sinfail=$(mktemp)
+		make_file "$sinfail" "server" 10240
+	elif [ "$test_linkfail" -eq 4 ]; then
+		sinfail=$(mktemp)
+		make_file "$sinfail" "server" 512
+	elif [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
 		size=$((RANDOM%16))
 		size=$((size+1))
 		size=$((size*2048))
@@ -739,6 +821,8 @@ dump_stats()
 chk_csum_nr()
 {
 	local msg=${1:-""}
+	local csum_ns1=${2:-0}
+	local csum_ns2=${3:-0}
 	local count
 	local dump_stats
 
@@ -750,8 +834,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" -lt $csum_ns1 ]; then
+		echo "[fail] got $count data checksum error[s] expected $csum_ns1"
 		ret=1
 		dump_stats=1
 	else
@@ -760,8 +844,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" -lt $csum_ns2 ]; then
+		echo "[fail] got $count data checksum error[s] expected $csum_ns2"
 		ret=1
 		dump_stats=1
 	else
@@ -780,7 +864,7 @@ chk_fail_nr()
 	printf "%-39s %s" " " "ftx"
 	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPFailTx | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != "$mp_fail_nr_tx" ]; then
+	if [ "$count" -lt "$mp_fail_nr_tx" ]; then
 		echo "[fail] got $count MP_FAIL[s] TX expected $mp_fail_nr_tx"
 		ret=1
 		dump_stats=1
@@ -791,7 +875,7 @@ chk_fail_nr()
 	echo -n " - frx   "
 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtMPFailRx | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != "$mp_fail_nr_rx" ]; then
+	if [ "$count" -lt "$mp_fail_nr_rx" ]; then
 		echo "[fail] got $count MP_FAIL[s] RX expected $mp_fail_nr_rx"
 		ret=1
 		dump_stats=1
@@ -812,7 +896,7 @@ chk_infi_nr()
 	printf "%-39s %s" " " "itx"
 	count=`ip netns exec $ns2 nstat -as | grep InfiniteMapTx | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != "$mp_infi_nr_tx" ]; then
+	if [ "$count" -lt "$mp_infi_nr_tx" ]; then
 		echo "[fail] got $count infinite map[s] TX expected $mp_infi_nr_tx"
 		ret=1
 		dump_stats=1
@@ -823,7 +907,7 @@ chk_infi_nr()
 	echo -n " - irx   "
 	count=`ip netns exec $ns1 nstat -as | grep InfiniteMapRx | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != "$mp_infi_nr_rx" ]; then
+	if [ "$count" -lt "$mp_infi_nr_rx" ]; then
 		echo "[fail] got $count infinite map[s] RX expected $mp_infi_nr_rx"
 		ret=1
 		dump_stats=1
@@ -840,6 +924,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
 
@@ -876,10 +962,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
 }
 
@@ -2183,6 +2269,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_fail 2
+	chk_join_nr "$nr_fail MP_FAIL, multiple subflows" 2 2 2 $nr_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 4
+	get_nr_fail 1
+	chk_join_nr "$nr_fail MP_FAIL, single subflow" 0 0 0 $nr_fail 0
+}
+
 all_tests()
 {
 	subflows_tests
@@ -2201,6 +2308,7 @@ all_tests()
 	deny_join_id0_tests
 	fullmesh_tests
 	userspace_tests
+	fail_tests
 }
 
 usage()
@@ -2222,6 +2330,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"
@@ -2261,7 +2370,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
@@ -2311,6 +2420,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do
 		u)
 			userspace_tests
 			;;
+		F)
+			fail_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.31.1

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

Posted by Mat Martineau 4 years ago

On Tue, 25 Jan 2022, Geliang Tang wrote:

> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
> checksum failures.
>

This patch is working well with the inverted byte detection, thanks!

-Mat

> 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      |   5 +
> .../testing/selftests/net/mptcp/mptcp_join.sh | 148 +++++++++++++++---
> 2 files changed, 135 insertions(+), 18 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index d36b7da5082a..26955abe49f0 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -19,3 +19,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_FLOWER=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 76c45c845d3b..44176419ff1d 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_fail=0
> do_all_tests=1
>
> TEST_COUNT=0
> @@ -61,6 +63,7 @@ init()
> 	done
>
> 	check_invert=0
> +	validate_checksum=$checksum
>
> 	#  ns1              ns2
> 	# ns1eth1    ns2eth1
> @@ -166,6 +169,46 @@ 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 32-bit word at byte offset 148 for all 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.
> +# But because the MPTCP checksum, covering the TCP options and data,
> +# has not been updated, we will detect the modification and emit an
> +# MP_FAIL: what we want to validate here.
> +#
> +# Please note that this rule will produce this pr_info() message for
> +# each TCP ACK packets not carrying enough data:
> +#
> +#     tc action pedit offset 162 out of bounds
> +#
> +# But this should be limited to a very few numbers of packets as we
> +# restrict this rule to outgoing TCP traffic with only the ACK flag
> +# + except the 3rd ACK, only packets carrying data should be seen in
> +# this direction.
> +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_fail=0
> +	i="$1"
> +
> +	tc -n $ns2 qdisc add dev ns2eth$i clsact
> +	tc -n $ns2 filter add dev ns2eth$i egress \
> +		protocol ip prio 1000 \
> +		flower ip_proto tcp tcp_flags 0x10/0xff \
> +		action pedit munge offset 148 u32 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"
> @@ -184,6 +227,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
> @@ -244,6 +293,21 @@ link_failure()
> 	done
> }
>
> +get_nr_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')
> +	local overlimits=$(echo $action | jq '.[1].actions[0].stats.overlimits')
> +
> +	let pkt=$packets-$overlimits
> +	if [ $pkt -gt 0 ]; then
> +		nr_fail=1
> +	fi
> +	tc -n $ns2 qdisc del dev ns2eth$i clsact
> +}
> +
> # $1: IP address
> is_v6()
> {
> @@ -458,7 +522,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} \
> @@ -478,13 +542,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=$!
>
> @@ -652,7 +722,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"
> @@ -701,7 +771,13 @@ 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
> +		cinfail=$(mktemp)
> +		make_file "$cinfail" "client" 10240
> +	elif [ "$test_linkfail" -eq 4 ]; then
> +		cinfail=$(mktemp)
> +		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
> @@ -714,7 +790,13 @@ run_tests()
> 		make_file "$cinfail" "client" $size
> 	fi
>
> -	if [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
> +	if [ "$test_linkfail" -eq 3 ]; then
> +		sinfail=$(mktemp)
> +		make_file "$sinfail" "server" 10240
> +	elif [ "$test_linkfail" -eq 4 ]; then
> +		sinfail=$(mktemp)
> +		make_file "$sinfail" "server" 512
> +	elif [ "$test_linkfail" -eq 2 -a -z "$sinfail" ]; then
> 		size=$((RANDOM%16))
> 		size=$((size+1))
> 		size=$((size*2048))
> @@ -739,6 +821,8 @@ dump_stats()
> chk_csum_nr()
> {
> 	local msg=${1:-""}
> +	local csum_ns1=${2:-0}
> +	local csum_ns2=${3:-0}
> 	local count
> 	local dump_stats
>
> @@ -750,8 +834,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" -lt $csum_ns1 ]; then
> +		echo "[fail] got $count data checksum error[s] expected $csum_ns1"
> 		ret=1
> 		dump_stats=1
> 	else
> @@ -760,8 +844,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" -lt $csum_ns2 ]; then
> +		echo "[fail] got $count data checksum error[s] expected $csum_ns2"
> 		ret=1
> 		dump_stats=1
> 	else
> @@ -780,7 +864,7 @@ chk_fail_nr()
> 	printf "%-39s %s" " " "ftx"
> 	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPFailTx | awk '{print $2}'`
> 	[ -z "$count" ] && count=0
> -	if [ "$count" != "$mp_fail_nr_tx" ]; then
> +	if [ "$count" -lt "$mp_fail_nr_tx" ]; then
> 		echo "[fail] got $count MP_FAIL[s] TX expected $mp_fail_nr_tx"
> 		ret=1
> 		dump_stats=1
> @@ -791,7 +875,7 @@ chk_fail_nr()
> 	echo -n " - frx   "
> 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtMPFailRx | awk '{print $2}'`
> 	[ -z "$count" ] && count=0
> -	if [ "$count" != "$mp_fail_nr_rx" ]; then
> +	if [ "$count" -lt "$mp_fail_nr_rx" ]; then
> 		echo "[fail] got $count MP_FAIL[s] RX expected $mp_fail_nr_rx"
> 		ret=1
> 		dump_stats=1
> @@ -812,7 +896,7 @@ chk_infi_nr()
> 	printf "%-39s %s" " " "itx"
> 	count=`ip netns exec $ns2 nstat -as | grep InfiniteMapTx | awk '{print $2}'`
> 	[ -z "$count" ] && count=0
> -	if [ "$count" != "$mp_infi_nr_tx" ]; then
> +	if [ "$count" -lt "$mp_infi_nr_tx" ]; then
> 		echo "[fail] got $count infinite map[s] TX expected $mp_infi_nr_tx"
> 		ret=1
> 		dump_stats=1
> @@ -823,7 +907,7 @@ chk_infi_nr()
> 	echo -n " - irx   "
> 	count=`ip netns exec $ns1 nstat -as | grep InfiniteMapRx | awk '{print $2}'`
> 	[ -z "$count" ] && count=0
> -	if [ "$count" != "$mp_infi_nr_rx" ]; then
> +	if [ "$count" -lt "$mp_infi_nr_rx" ]; then
> 		echo "[fail] got $count infinite map[s] RX expected $mp_infi_nr_rx"
> 		ret=1
> 		dump_stats=1
> @@ -840,6 +924,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
>
> @@ -876,10 +962,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
> }
>
> @@ -2183,6 +2269,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_fail 2
> +	chk_join_nr "$nr_fail MP_FAIL, multiple subflows" 2 2 2 $nr_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 4
> +	get_nr_fail 1
> +	chk_join_nr "$nr_fail MP_FAIL, single subflow" 0 0 0 $nr_fail 0
> +}
> +
> all_tests()
> {
> 	subflows_tests
> @@ -2201,6 +2308,7 @@ all_tests()
> 	deny_join_id0_tests
> 	fullmesh_tests
> 	userspace_tests
> +	fail_tests
> }
>
> usage()
> @@ -2222,6 +2330,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"
> @@ -2261,7 +2370,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
> @@ -2311,6 +2420,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do
> 		u)
> 			userspace_tests
> 			;;
> +		F)
> +			fail_tests
> +			;;
> 		c)
> 			;;
> 		C)
> -- 
> 2.31.1
>
>
>

--
Mat Martineau
Intel

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

Posted by Matthieu Baerts 4 years ago

Hi Mat, Geliang,

On 27/01/2022 01:07, Mat Martineau wrote:
> On Tue, 25 Jan 2022, Geliang Tang wrote:
> 
>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
>> checksum failures.
>>
> 
> This patch is working well with the inverted byte detection, thanks!

The public CI seems quite happy with the new version, good work :)

Here is its report:

====== 8< ======
Our CI did some validations and here is its report:

- KVM Validation: normal:
  - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/4993794666921984
  - Summary:
https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/4682233209421824
  - Summary:
https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt

Initiator: Patchew Applier
Commits:
https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
====== 8< ======

But if you look at the logs [1], there are probably 3 last things to
improve:

[1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log

This line is repeated 90+ times:

  tc action pedit offset 162 out of bounds

(we talked about this one at the last meeting with ideas on how to
reduce/get rid of them)

But also this one now, repeated 40+ times with different offsets:

  file received by server has inverted byte at 106565

And one last thing, we can see that now we have more than 99 tests, the
alignement is no longer OK :)


  # 101 1 MP_FAIL, single subflow            syn[ ok ] - synack[ ok ]...
  #                                         sum[ ok ] - csum  [ ok ]
  #                                         ftx[ ok ] - frx   [ ok ]
  #                                         itx[ ok ] - irx   [ ok ]

A very small detail, up to you to fix it or not ;)

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

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

Posted by Geliang Tang 4 years ago

Hi Matt,

Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
>
> Hi Mat, Geliang,
>
> On 27/01/2022 01:07, Mat Martineau wrote:
> > On Tue, 25 Jan 2022, Geliang Tang wrote:
> >
> >> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
> >> checksum failures.
> >>
> >
> > This patch is working well with the inverted byte detection, thanks!
>
> The public CI seems quite happy with the new version, good work :)
>
> Here is its report:
>
> ====== 8< ======
> Our CI did some validations and here is its report:
>
> - KVM Validation: normal:
>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
>   - Task: https://cirrus-ci.com/task/4993794666921984
>   - Summary:
> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
>
> - KVM Validation: debug:
>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
>   - Task: https://cirrus-ci.com/task/4682233209421824
>   - Summary:
> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
>
> Initiator: Patchew Applier
> Commits:
> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
> ====== 8< ======
>
> But if you look at the logs [1], there are probably 3 last things to
> improve:
>
> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
>
> This line is repeated 90+ times:
>
>   tc action pedit offset 162 out of bounds
>
> (we talked about this one at the last meeting with ideas on how to
> reduce/get rid of them)
>

Dose 'tc + iptables' work for our case?

Use iptables to select the packets with enough data. Then use tc to
edit the selected packets only. (I don't know how to do it yet.)

Here's a link about this:

https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
.
'''
Using tc + iptables

Iptables has a method called fwmark that can be used to mark packets
across interfaces.

First, this makes packets marked with 6, to be processed by the 1:30 class

# tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30

This sets that mark 6, using iptables

# iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6

You can then use iptables normally to match packets and then mark them
with fwmark.
'''

> But also this one now, repeated 40+ times with different offsets:
>
>   file received by server has inverted byte at 106565
>

These inverted byte logs will not be there if we add the code to drop
the bad data in future. (I haven't figured out how to implement this
yet.) Right now, just let them be there.

> And one last thing, we can see that now we have more than 99 tests, the
> alignement is no longer OK :)
>
>
>   # 101 1 MP_FAIL, single subflow            syn[ ok ] - synack[ ok ]...
>   #                                         sum[ ok ] - csum  [ ok ]
>   #                                         ftx[ ok ] - frx   [ ok ]
>   #                                         itx[ ok ] - irx   [ ok ]
>
> A very small detail, up to you to fix it or not ;)
>

I'll update this in the next version.

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

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

Posted by Matthieu Baerts 4 years ago

Hi Geliang,

On 07/02/2022 05:13, Geliang Tang wrote:
> Hi Matt,
> 
> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
>>
>> Hi Mat, Geliang,
>>
>> On 27/01/2022 01:07, Mat Martineau wrote:
>>> On Tue, 25 Jan 2022, Geliang Tang wrote:
>>>
>>>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
>>>> checksum failures.
>>>>
>>>
>>> This patch is working well with the inverted byte detection, thanks!
>>
>> The public CI seems quite happy with the new version, good work :)
>>
>> Here is its report:
>>
>> ====== 8< ======
>> Our CI did some validations and here is its report:
>>
>> - KVM Validation: normal:
>>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
>>   - Task: https://cirrus-ci.com/task/4993794666921984
>>   - Summary:
>> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
>>
>> - KVM Validation: debug:
>>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
>>   - Task: https://cirrus-ci.com/task/4682233209421824
>>   - Summary:
>> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
>>
>> Initiator: Patchew Applier
>> Commits:
>> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
>> ====== 8< ======
>>
>> But if you look at the logs [1], there are probably 3 last things to
>> improve:
>>
>> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
>>
>> This line is repeated 90+ times:
>>
>>   tc action pedit offset 162 out of bounds
>>
>> (we talked about this one at the last meeting with ideas on how to
>> reduce/get rid of them)
>>
> 
> Dose 'tc + iptables' work for our case?
> 
> Use iptables to select the packets with enough data. Then use tc to
> edit the selected packets only. (I don't know how to do it yet.)
> 
> Here's a link about this:
> 
> https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
> .
> '''
> Using tc + iptables
> 
> Iptables has a method called fwmark that can be used to mark packets
> across interfaces.
> 
> First, this makes packets marked with 6, to be processed by the 1:30 class
> 
> # tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30
> 
> This sets that mark 6, using iptables
> 
> # iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6
> 
> You can then use iptables normally to match packets and then mark them
> with fwmark.
> '''

Good idea, probably simpler!

I guess we can mark some packets in POSTROUTING and modify the egress TC
rule.

Do you need a hand for that?

>> But also this one now, repeated 40+ times with different offsets:
>>
>>   file received by server has inverted byte at 106565
>>
> 
> These inverted byte logs will not be there if we add the code to drop
> the bad data in future. (I haven't figured out how to implement this
> yet.) Right now, just let them be there.

I see. I guess later, when the code to drop the bad data in the future
will be ready, this "warning" will become an "error" we will no longer
ignore.

>> And one last thing, we can see that now we have more than 99 tests, the
>> alignement is no longer OK :)
>>
>>
>>   # 101 1 MP_FAIL, single subflow            syn[ ok ] - synack[ ok ]...
>>   #                                         sum[ ok ] - csum  [ ok ]
>>   #                                         ftx[ ok ] - frx   [ ok ]
>>   #                                         itx[ ok ] - irx   [ ok ]
>>
>> A very small detail, up to you to fix it or not ;)
>>
> 
> I'll update this in the next version.

Thanks!

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

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

Posted by Geliang Tang 4 years ago

On Mon, Feb 07, 2022 at 11:32:05AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/02/2022 05:13, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
> >>
> >> Hi Mat, Geliang,
> >>
> >> On 27/01/2022 01:07, Mat Martineau wrote:
> >>> On Tue, 25 Jan 2022, Geliang Tang wrote:
> >>>
> >>>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
> >>>> checksum failures.
> >>>>
> >>>
> >>> This patch is working well with the inverted byte detection, thanks!
> >>
> >> The public CI seems quite happy with the new version, good work :)
> >>
> >> Here is its report:
> >>
> >> ====== 8< ======
> >> Our CI did some validations and here is its report:
> >>
> >> - KVM Validation: normal:
> >>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
> >>   - Task: https://cirrus-ci.com/task/4993794666921984
> >>   - Summary:
> >> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
> >>
> >> - KVM Validation: debug:
> >>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
> >>   - Task: https://cirrus-ci.com/task/4682233209421824
> >>   - Summary:
> >> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
> >>
> >> Initiator: Patchew Applier
> >> Commits:
> >> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
> >> ====== 8< ======
> >>
> >> But if you look at the logs [1], there are probably 3 last things to
> >> improve:
> >>
> >> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
> >>
> >> This line is repeated 90+ times:
> >>
> >>   tc action pedit offset 162 out of bounds
> >>
> >> (we talked about this one at the last meeting with ideas on how to
> >> reduce/get rid of them)
> >>
> > 
> > Dose 'tc + iptables' work for our case?
> > 
> > Use iptables to select the packets with enough data. Then use tc to
> > edit the selected packets only. (I don't know how to do it yet.)
> > 
> > Here's a link about this:
> > 
> > https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
> > .
> > '''
> > Using tc + iptables
> > 
> > Iptables has a method called fwmark that can be used to mark packets
> > across interfaces.
> > 
> > First, this makes packets marked with 6, to be processed by the 1:30 class
> > 
> > # tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30
> > 
> > This sets that mark 6, using iptables
> > 
> > # iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6
> > 
> > You can then use iptables normally to match packets and then mark them
> > with fwmark.
> > '''
> 
> Good idea, probably simpler!
> 
> I guess we can mark some packets in POSTROUTING and modify the egress TC
> rule.
> 
> Do you need a hand for that?
> 

Thanks, Matt, I really need your help :)

> >> But also this one now, repeated 40+ times with different offsets:
> >>
> >>   file received by server has inverted byte at 106565
> >>
> > 
> > These inverted byte logs will not be there if we add the code to drop
> > the bad data in future. (I haven't figured out how to implement this
> > yet.) Right now, just let them be there.
> 
> I see. I guess later, when the code to drop the bad data in the future
> will be ready, this "warning" will become an "error" we will no longer
> ignore.
> 
> >> And one last thing, we can see that now we have more than 99 tests, the
> >> alignement is no longer OK :)
> >>
> >>
> >>   # 101 1 MP_FAIL, single subflow            syn[ ok ] - synack[ ok ]...
> >>   #                                         sum[ ok ] - csum  [ ok ]
> >>   #                                         ftx[ ok ] - frx   [ ok ]
> >>   #                                         itx[ ok ] - irx   [ ok ]
> >>
> >> A very small detail, up to you to fix it or not ;)
> >>
> > 
> > I'll update this in the next version.
> 
> Thanks!
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

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

Posted by Matthieu Baerts 4 years ago

Hi Geliang,

On 07/02/2022 15:08, Geliang Tang wrote:
> On Mon, Feb 07, 2022 at 11:32:05AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 07/02/2022 05:13, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
>>>>
>>>> Hi Mat, Geliang,
>>>>
>>>> On 27/01/2022 01:07, Mat Martineau wrote:
>>>>> On Tue, 25 Jan 2022, Geliang Tang wrote:
>>>>>
>>>>>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
>>>>>> checksum failures.
>>>>>>
>>>>>
>>>>> This patch is working well with the inverted byte detection, thanks!
>>>>
>>>> The public CI seems quite happy with the new version, good work :)
>>>>
>>>> Here is its report:
>>>>
>>>> ====== 8< ======
>>>> Our CI did some validations and here is its report:
>>>>
>>>> - KVM Validation: normal:
>>>>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
>>>>   - Task: https://cirrus-ci.com/task/4993794666921984
>>>>   - Summary:
>>>> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
>>>>
>>>> - KVM Validation: debug:
>>>>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
>>>>   - Task: https://cirrus-ci.com/task/4682233209421824
>>>>   - Summary:
>>>> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
>>>>
>>>> Initiator: Patchew Applier
>>>> Commits:
>>>> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
>>>> ====== 8< ======
>>>>
>>>> But if you look at the logs [1], there are probably 3 last things to
>>>> improve:
>>>>
>>>> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
>>>>
>>>> This line is repeated 90+ times:
>>>>
>>>>   tc action pedit offset 162 out of bounds
>>>>
>>>> (we talked about this one at the last meeting with ideas on how to
>>>> reduce/get rid of them)
>>>>
>>>
>>> Dose 'tc + iptables' work for our case?
>>>
>>> Use iptables to select the packets with enough data. Then use tc to
>>> edit the selected packets only. (I don't know how to do it yet.)
>>>
>>> Here's a link about this:
>>>
>>> https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
>>> .
>>> '''
>>> Using tc + iptables
>>>
>>> Iptables has a method called fwmark that can be used to mark packets
>>> across interfaces.
>>>
>>> First, this makes packets marked with 6, to be processed by the 1:30 class
>>>
>>> # tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30
>>>
>>> This sets that mark 6, using iptables
>>>
>>> # iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6
>>>
>>> You can then use iptables normally to match packets and then mark them
>>> with fwmark.
>>> '''
>>
>> Good idea, probably simpler!
>>
>> I guess we can mark some packets in POSTROUTING and modify the egress TC
>> rule.
>>
>> Do you need a hand for that?
>>
> 
> Thanks, Matt, I really need your help :)

Sure.
May you try the attached patch if you don't mind?

I now have this output:

001 0 MP_FAIL, multiple subflows         syn[ ok ] - synack[ ok ] - ack[
ok ]
                                         sum[ ok ] - csum  [ ok ]
                                         ftx[ ok ] - frx   [ ok ]
                                         itx[ ok ] - irx   [ ok ]
Created /tmp/tmp.SNhvH7ucj9 (size 512 KB) containing data sent by client
Created /tmp/tmp.RFEqEXi0bl (size 512 KB) containing data sent by server
file received by server has inverted byte at 69
file received by server has inverted byte at 70
file received by server has inverted byte at 71
file received by server has inverted byte at 72
002 1 MP_FAIL, single subflow            syn[ ok ] - synack[ ok ] - ack[
ok ]
                                         sum[ ok ] - csum  [ ok ]
                                         ftx[ ok ] - frx   [ ok ]
                                         itx[ ok ] - irx   [ ok ]

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

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

Posted by Geliang Tang 4 years ago

Hi Matt, Mat,

On Tue, Feb 08, 2022 at 01:26:10PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/02/2022 15:08, Geliang Tang wrote:
> > On Mon, Feb 07, 2022 at 11:32:05AM +0100, Matthieu Baerts wrote:
> >> Hi Geliang,
> >>
> >> On 07/02/2022 05:13, Geliang Tang wrote:
> >>> Hi Matt,
> >>>
> >>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
> >>>>
> >>>> Hi Mat, Geliang,
> >>>>
> >>>> On 27/01/2022 01:07, Mat Martineau wrote:
> >>>>> On Tue, 25 Jan 2022, Geliang Tang wrote:
> >>>>>
> >>>>>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
> >>>>>> checksum failures.
> >>>>>>
> >>>>>
> >>>>> This patch is working well with the inverted byte detection, thanks!
> >>>>
> >>>> The public CI seems quite happy with the new version, good work :)
> >>>>
> >>>> Here is its report:
> >>>>
> >>>> ====== 8< ======
> >>>> Our CI did some validations and here is its report:
> >>>>
> >>>> - KVM Validation: normal:
> >>>>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
> >>>>   - Task: https://cirrus-ci.com/task/4993794666921984
> >>>>   - Summary:
> >>>> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
> >>>>
> >>>> - KVM Validation: debug:
> >>>>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
> >>>>   - Task: https://cirrus-ci.com/task/4682233209421824
> >>>>   - Summary:
> >>>> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
> >>>>
> >>>> Initiator: Patchew Applier
> >>>> Commits:
> >>>> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
> >>>> ====== 8< ======
> >>>>
> >>>> But if you look at the logs [1], there are probably 3 last things to
> >>>> improve:
> >>>>
> >>>> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
> >>>>
> >>>> This line is repeated 90+ times:
> >>>>
> >>>>   tc action pedit offset 162 out of bounds
> >>>>
> >>>> (we talked about this one at the last meeting with ideas on how to
> >>>> reduce/get rid of them)
> >>>>
> >>>
> >>> Dose 'tc + iptables' work for our case?
> >>>
> >>> Use iptables to select the packets with enough data. Then use tc to
> >>> edit the selected packets only. (I don't know how to do it yet.)
> >>>
> >>> Here's a link about this:
> >>>
> >>> https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
> >>> .
> >>> '''
> >>> Using tc + iptables
> >>>
> >>> Iptables has a method called fwmark that can be used to mark packets
> >>> across interfaces.
> >>>
> >>> First, this makes packets marked with 6, to be processed by the 1:30 class
> >>>
> >>> # tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30
> >>>
> >>> This sets that mark 6, using iptables
> >>>
> >>> # iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6
> >>>
> >>> You can then use iptables normally to match packets and then mark them
> >>> with fwmark.
> >>> '''
> >>
> >> Good idea, probably simpler!
> >>
> >> I guess we can mark some packets in POSTROUTING and modify the egress TC
> >> rule.
> >>
> >> Do you need a hand for that?
> >>
> > 
> > Thanks, Matt, I really need your help :)
> 
> Sure.
> May you try the attached patch if you don't mind?

Great! Thanks Matt. I just tested your patch, the single subflow test
("002 1 MP_FAIL, single subflow") works well. And very stable. The two
issues that I mentioned in the cover letter of this series didn't happen
when running this new selftest. So the first two squash-to patches in
this series can be dropped.

Mat, it looks like no need to analyse my attached pcaps any more. Thanks
for your help.

> 
> I now have this output:
> 
> 001 0 MP_FAIL, multiple subflows         syn[ ok ] - synack[ ok ] - ack[
> ok ]
>                                          sum[ ok ] - csum  [ ok ]
>                                          ftx[ ok ] - frx   [ ok ]
>                                          itx[ ok ] - irx   [ ok ]

But the multiple subflows test ("001 0 MP_FAIL, multiple subflows") doesn't
work. I'll try to fix it tomorrow. I think it's easy to fix.

Thanks,
-Geliang

> Created /tmp/tmp.SNhvH7ucj9 (size 512 KB) containing data sent by client
> Created /tmp/tmp.RFEqEXi0bl (size 512 KB) containing data sent by server
> file received by server has inverted byte at 69
> file received by server has inverted byte at 70
> file received by server has inverted byte at 71
> file received by server has inverted byte at 72
> 002 1 MP_FAIL, single subflow            syn[ ok ] - synack[ ok ] - ack[
> ok ]
>                                          sum[ ok ] - csum  [ ok ]
>                                          ftx[ ok ] - frx   [ ok ]
>                                          itx[ ok ] - irx   [ ok ]
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index 26955abe49f0..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
> @@ -22,5 +25,5 @@ CONFIG_IPV6_MULTIPLE_TABLES=y
>  CONFIG_NET_ACT_CSUM=m
>  CONFIG_NET_ACT_PEDIT=m
>  CONFIG_NET_CLS_ACT=y
> -CONFIG_NET_CLS_FLOWER=m
> +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 a51b0915b7b1..52a0f3df3b2e 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -180,15 +180,12 @@ reset_with_allow_join_id0()
>  # has not been updated, we will detect the modification and emit an
>  # MP_FAIL: what we want to validate here.
>  #
> -# Please note that this rule will produce this pr_info() message for
> -# each TCP ACK packets not carrying enough data:
> +# 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
>  #
> -# But this should be limited to a very few numbers of packets as we
> -# restrict this rule to outgoing TCP traffic with only the ACK flag
> -# + except the 3rd ACK, only packets carrying data should be seen in
> -# this direction.
> +# Netfilter is used to mark packets with enough data.
>  reset_with_fail()
>  {
>  	reset
> @@ -199,12 +196,20 @@ reset_with_fail()
>  	check_invert=1
>  	validate_checksum=1
>  	nr_fail=0
> -	i="$1"
> +	local i="$1"
> +
> +	ip netns exec $ns2 iptables \
> +		-t mangle \
> +		-A OUTPUT \
> +		-p tcp \
> +		-m length --length 150:9999 \
> +		-m statistic --mode nth --packet 0 --every 9999 \
> +		-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 \
> -		flower ip_proto tcp tcp_flags 0x10/0xff \
> +		handle 42 fw \
>  		action pedit munge offset 148 u32 invert \
>  		pipe csum tcp \
>  		index 100
> @@ -296,14 +301,12 @@ link_failure()
>  
>  get_nr_fail()
>  {
> -	i="$1"
> +	local 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')
> -	local overlimits=$(echo $action | jq '.[1].actions[0].stats.overlimits')
>  
> -	let pkt=$packets-$overlimits
> -	if [ $pkt -gt 0 ]; then
> +	if [ $packets -gt 0 ]; then
>  		nr_fail=1
>  	fi
>  	tc -n $ns2 qdisc del dev ns2eth$i clsact

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

Posted by Matthieu Baerts 4 years ago

Hi Geliang,

On 08/02/2022 16:41, Geliang Tang wrote:
> Hi Matt, Mat,
> 
> On Tue, Feb 08, 2022 at 01:26:10PM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 07/02/2022 15:08, Geliang Tang wrote:
>>> On Mon, Feb 07, 2022 at 11:32:05AM +0100, Matthieu Baerts wrote:
>>>> Hi Geliang,
>>>>
>>>> On 07/02/2022 05:13, Geliang Tang wrote:
>>>>> Hi Matt,
>>>>>
>>>>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
>>>>>>
>>>>>> Hi Mat, Geliang,
>>>>>>
>>>>>> On 27/01/2022 01:07, Mat Martineau wrote:
>>>>>>> On Tue, 25 Jan 2022, Geliang Tang wrote:
>>>>>>>
>>>>>>>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
>>>>>>>> checksum failures.
>>>>>>>>
>>>>>>>
>>>>>>> This patch is working well with the inverted byte detection, thanks!
>>>>>>
>>>>>> The public CI seems quite happy with the new version, good work :)
>>>>>>
>>>>>> Here is its report:
>>>>>>
>>>>>> ====== 8< ======
>>>>>> Our CI did some validations and here is its report:
>>>>>>
>>>>>> - KVM Validation: normal:
>>>>>>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
>>>>>>   - Task: https://cirrus-ci.com/task/4993794666921984
>>>>>>   - Summary:
>>>>>> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
>>>>>>
>>>>>> - KVM Validation: debug:
>>>>>>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
>>>>>>   - Task: https://cirrus-ci.com/task/4682233209421824
>>>>>>   - Summary:
>>>>>> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
>>>>>>
>>>>>> Initiator: Patchew Applier
>>>>>> Commits:
>>>>>> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
>>>>>> ====== 8< ======
>>>>>>
>>>>>> But if you look at the logs [1], there are probably 3 last things to
>>>>>> improve:
>>>>>>
>>>>>> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
>>>>>>
>>>>>> This line is repeated 90+ times:
>>>>>>
>>>>>>   tc action pedit offset 162 out of bounds
>>>>>>
>>>>>> (we talked about this one at the last meeting with ideas on how to
>>>>>> reduce/get rid of them)
>>>>>>
>>>>>
>>>>> Dose 'tc + iptables' work for our case?
>>>>>
>>>>> Use iptables to select the packets with enough data. Then use tc to
>>>>> edit the selected packets only. (I don't know how to do it yet.)
>>>>>
>>>>> Here's a link about this:
>>>>>
>>>>> https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
>>>>> .
>>>>> '''
>>>>> Using tc + iptables
>>>>>
>>>>> Iptables has a method called fwmark that can be used to mark packets
>>>>> across interfaces.
>>>>>
>>>>> First, this makes packets marked with 6, to be processed by the 1:30 class
>>>>>
>>>>> # tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30
>>>>>
>>>>> This sets that mark 6, using iptables
>>>>>
>>>>> # iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6
>>>>>
>>>>> You can then use iptables normally to match packets and then mark them
>>>>> with fwmark.
>>>>> '''
>>>>
>>>> Good idea, probably simpler!
>>>>
>>>> I guess we can mark some packets in POSTROUTING and modify the egress TC
>>>> rule.
>>>>
>>>> Do you need a hand for that?
>>>>
>>>
>>> Thanks, Matt, I really need your help :)
>>
>> Sure.
>> May you try the attached patch if you don't mind?
> 
> Great! Thanks Matt. I just tested your patch, the single subflow test
> ("002 1 MP_FAIL, single subflow") works well. And very stable. The two
> issues that I mentioned in the cover letter of this series didn't happen
> when running this new selftest. So the first two squash-to patches in
> this series can be dropped.

That's nice! Thank you for having tested, I'm glad it helped!

> Mat, it looks like no need to analyse my attached pcaps any more. Thanks
> for your help.
> 
>>
>> I now have this output:
>>
>> 001 0 MP_FAIL, multiple subflows         syn[ ok ] - synack[ ok ] - ack[
>> ok ]
>>                                          sum[ ok ] - csum  [ ok ]
>>                                          ftx[ ok ] - frx   [ ok ]
>>                                          itx[ ok ] - irx   [ ok ]
> 
> But the multiple subflows test ("001 0 MP_FAIL, multiple subflows") doesn't
> work. I'll try to fix it tomorrow. I think it's easy to fix.

Yes, it looked strange to me. I guess the test should fail if 'nr_fail'
is set to 0, no?
Or probably we should no longer use 'get_nr_fail' as we expect to have
only one MP_FAIL, no?

In the IPTables rule I added, I asked to mark the first packet that is
over 150 bytes. We can change "--packet" if we want to mark the Xth one.
And "--every" if we have more packets but worst case, that's not an
issue to corrupt another one I think: on this path, either we have no
more subflow (multiple subflows case) or no more MPTCP connection (1
subflow case).

> 
> Thanks,
> -Geliang
> 
>> Created /tmp/tmp.SNhvH7ucj9 (size 512 KB) containing data sent by client
>> Created /tmp/tmp.RFEqEXi0bl (size 512 KB) containing data sent by server
>> file received by server has inverted byte at 69
>> file received by server has inverted byte at 70
>> file received by server has inverted byte at 71
>> file received by server has inverted byte at 72

We can also modify the TC command to invert only a u8 instead of a u32
to reduce the log entries here.

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

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

Posted by Matthieu Baerts 3 years, 12 months ago

Hi Geliang,

On 08/02/2022 17:24, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 08/02/2022 16:41, Geliang Tang wrote:
>> Hi Matt, Mat,
>>
>> On Tue, Feb 08, 2022 at 01:26:10PM +0100, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> On 07/02/2022 15:08, Geliang Tang wrote:
>>>> On Mon, Feb 07, 2022 at 11:32:05AM +0100, Matthieu Baerts wrote:
>>>>> Hi Geliang,
>>>>>
>>>>> On 07/02/2022 05:13, Geliang Tang wrote:
>>>>>> Hi Matt,
>>>>>>
>>>>>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
>>>>>>>
>>>>>>> Hi Mat, Geliang,
>>>>>>>
>>>>>>> On 27/01/2022 01:07, Mat Martineau wrote:
>>>>>>>> On Tue, 25 Jan 2022, Geliang Tang wrote:
>>>>>>>>
>>>>>>>>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
>>>>>>>>> checksum failures.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This patch is working well with the inverted byte detection, thanks!
>>>>>>>
>>>>>>> The public CI seems quite happy with the new version, good work :)
>>>>>>>
>>>>>>> Here is its report:
>>>>>>>
>>>>>>> ====== 8< ======
>>>>>>> Our CI did some validations and here is its report:
>>>>>>>
>>>>>>> - KVM Validation: normal:
>>>>>>>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
>>>>>>>   - Task: https://cirrus-ci.com/task/4993794666921984
>>>>>>>   - Summary:
>>>>>>> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
>>>>>>>
>>>>>>> - KVM Validation: debug:
>>>>>>>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
>>>>>>>   - Task: https://cirrus-ci.com/task/4682233209421824
>>>>>>>   - Summary:
>>>>>>> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
>>>>>>>
>>>>>>> Initiator: Patchew Applier
>>>>>>> Commits:
>>>>>>> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
>>>>>>> ====== 8< ======
>>>>>>>
>>>>>>> But if you look at the logs [1], there are probably 3 last things to
>>>>>>> improve:
>>>>>>>
>>>>>>> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
>>>>>>>
>>>>>>> This line is repeated 90+ times:
>>>>>>>
>>>>>>>   tc action pedit offset 162 out of bounds
>>>>>>>
>>>>>>> (we talked about this one at the last meeting with ideas on how to
>>>>>>> reduce/get rid of them)
>>>>>>>
>>>>>>
>>>>>> Dose 'tc + iptables' work for our case?
>>>>>>
>>>>>> Use iptables to select the packets with enough data. Then use tc to
>>>>>> edit the selected packets only. (I don't know how to do it yet.)
>>>>>>
>>>>>> Here's a link about this:
>>>>>>
>>>>>> https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
>>>>>> .
>>>>>> '''
>>>>>> Using tc + iptables
>>>>>>
>>>>>> Iptables has a method called fwmark that can be used to mark packets
>>>>>> across interfaces.
>>>>>>
>>>>>> First, this makes packets marked with 6, to be processed by the 1:30 class
>>>>>>
>>>>>> # tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30
>>>>>>
>>>>>> This sets that mark 6, using iptables
>>>>>>
>>>>>> # iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6
>>>>>>
>>>>>> You can then use iptables normally to match packets and then mark them
>>>>>> with fwmark.
>>>>>> '''
>>>>>
>>>>> Good idea, probably simpler!
>>>>>
>>>>> I guess we can mark some packets in POSTROUTING and modify the egress TC
>>>>> rule.
>>>>>
>>>>> Do you need a hand for that?
>>>>>
>>>>
>>>> Thanks, Matt, I really need your help :)
>>>
>>> Sure.
>>> May you try the attached patch if you don't mind?
>>
>> Great! Thanks Matt. I just tested your patch, the single subflow test
>> ("002 1 MP_FAIL, single subflow") works well. And very stable. The two
>> issues that I mentioned in the cover letter of this series didn't happen
>> when running this new selftest. So the first two squash-to patches in
>> this series can be dropped.
> 
> That's nice! Thank you for having tested, I'm glad it helped!
> 
>> Mat, it looks like no need to analyse my attached pcaps any more. Thanks
>> for your help.
>>
>>>
>>> I now have this output:
>>>
>>> 001 0 MP_FAIL, multiple subflows         syn[ ok ] - synack[ ok ] - ack[
>>> ok ]
>>>                                          sum[ ok ] - csum  [ ok ]
>>>                                          ftx[ ok ] - frx   [ ok ]
>>>                                          itx[ ok ] - irx   [ ok ]
>>
>> But the multiple subflows test ("001 0 MP_FAIL, multiple subflows") doesn't
>> work. I'll try to fix it tomorrow. I think it's easy to fix.

Yes, we need to restrict the 'iptables' command to the same interface as
the one used with the TC command. Otherwise we will mark a flow for
another interface and TC will never see a marked packet for this interface.

> Yes, it looked strange to me. I guess the test should fail if 'nr_fail'
> is set to 0, no?
> Or probably we should no longer use 'get_nr_fail' as we expect to have
> only one MP_FAIL, no?
> 
> In the IPTables rule I added, I asked to mark the first packet that is
> over 150 bytes. We can change "--packet" if we want to mark the Xth one.
> And "--every" if we have more packets but worst case, that's not an
> issue to corrupt another one I think: on this path, either we have no
> more subflow (multiple subflows case) or no more MPTCP connection (1
> subflow case).
> 
>>
>> Thanks,
>> -Geliang
>>
>>> Created /tmp/tmp.SNhvH7ucj9 (size 512 KB) containing data sent by client
>>> Created /tmp/tmp.RFEqEXi0bl (size 512 KB) containing data sent by server
>>> file received by server has inverted byte at 69
>>> file received by server has inverted byte at 70
>>> file received by server has inverted byte at 71
>>> file received by server has inverted byte at 72
> 
> We can also modify the TC command to invert only a u8 instead of a u32
> to reduce the log entries here.

While at it, I also applied the suggestions I made here and above,
please see the attached patch. You can see a diff with the previous
version I sent here: https://paste.opendev.org/show/812608/

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

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

Posted by Geliang Tang 3 years, 12 months ago

On Tue, Feb 08, 2022 at 08:40:13PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 08/02/2022 17:24, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > On 08/02/2022 16:41, Geliang Tang wrote:
> >> Hi Matt, Mat,
> >>
> >> On Tue, Feb 08, 2022 at 01:26:10PM +0100, Matthieu Baerts wrote:
> >>> Hi Geliang,
> >>>
> >>> On 07/02/2022 15:08, Geliang Tang wrote:
> >>>> On Mon, Feb 07, 2022 at 11:32:05AM +0100, Matthieu Baerts wrote:
> >>>>> Hi Geliang,
> >>>>>
> >>>>> On 07/02/2022 05:13, Geliang Tang wrote:
> >>>>>> Hi Matt,
> >>>>>>
> >>>>>> Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月29日周六 01:04写道：
> >>>>>>>
> >>>>>>> Hi Mat, Geliang,
> >>>>>>>
> >>>>>>> On 27/01/2022 01:07, Mat Martineau wrote:
> >>>>>>>> On Tue, 25 Jan 2022, Geliang Tang wrote:
> >>>>>>>>
> >>>>>>>>> Added the test cases for MP_FAIL, use 'tc' commands to trigger the
> >>>>>>>>> checksum failures.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> This patch is working well with the inverted byte detection, thanks!
> >>>>>>>
> >>>>>>> The public CI seems quite happy with the new version, good work :)
> >>>>>>>
> >>>>>>> Here is its report:
> >>>>>>>
> >>>>>>> ====== 8< ======
> >>>>>>> Our CI did some validations and here is its report:
> >>>>>>>
> >>>>>>> - KVM Validation: normal:
> >>>>>>>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
> >>>>>>>   - Task: https://cirrus-ci.com/task/4993794666921984
> >>>>>>>   - Summary:
> >>>>>>> https://api.cirrus-ci.com/v1/artifact/task/4993794666921984/summary/summary.txt
> >>>>>>>
> >>>>>>> - KVM Validation: debug:
> >>>>>>>   - Unstable: 2 failed test(s): selftest_diag selftest_mptcp_join 🔴:
> >>>>>>>   - Task: https://cirrus-ci.com/task/4682233209421824
> >>>>>>>   - Summary:
> >>>>>>> https://api.cirrus-ci.com/v1/artifact/task/4682233209421824/summary/summary.txt
> >>>>>>>
> >>>>>>> Initiator: Patchew Applier
> >>>>>>> Commits:
> >>>>>>> https://github.com/multipath-tcp/mptcp_net-next/commits/c52e51136372
> >>>>>>> ====== 8< ======
> >>>>>>>
> >>>>>>> But if you look at the logs [1], there are probably 3 last things to
> >>>>>>> improve:
> >>>>>>>
> >>>>>>> [1] https://api.cirrus-ci.com/v1/task/4993794666921984/logs/test.log
> >>>>>>>
> >>>>>>> This line is repeated 90+ times:
> >>>>>>>
> >>>>>>>   tc action pedit offset 162 out of bounds
> >>>>>>>
> >>>>>>> (we talked about this one at the last meeting with ideas on how to
> >>>>>>> reduce/get rid of them)
> >>>>>>>
> >>>>>>
> >>>>>> Dose 'tc + iptables' work for our case?
> >>>>>>
> >>>>>> Use iptables to select the packets with enough data. Then use tc to
> >>>>>> edit the selected packets only. (I don't know how to do it yet.)
> >>>>>>
> >>>>>> Here's a link about this:
> >>>>>>
> >>>>>> https://wiki.archlinux.org/title/advanced_traffic_control#Using_tc_+_iptables
> >>>>>> .
> >>>>>> '''
> >>>>>> Using tc + iptables
> >>>>>>
> >>>>>> Iptables has a method called fwmark that can be used to mark packets
> >>>>>> across interfaces.
> >>>>>>
> >>>>>> First, this makes packets marked with 6, to be processed by the 1:30 class
> >>>>>>
> >>>>>> # tc filter add dev eth0 protocol ip parent 1: prio 1 handle 6 fw flowid 1:30
> >>>>>>
> >>>>>> This sets that mark 6, using iptables
> >>>>>>
> >>>>>> # iptables -A PREROUTING -t mangle -i eth0 -j MARK --set-mark 6
> >>>>>>
> >>>>>> You can then use iptables normally to match packets and then mark them
> >>>>>> with fwmark.
> >>>>>> '''
> >>>>>
> >>>>> Good idea, probably simpler!
> >>>>>
> >>>>> I guess we can mark some packets in POSTROUTING and modify the egress TC
> >>>>> rule.
> >>>>>
> >>>>> Do you need a hand for that?
> >>>>>
> >>>>
> >>>> Thanks, Matt, I really need your help :)
> >>>
> >>> Sure.
> >>> May you try the attached patch if you don't mind?
> >>
> >> Great! Thanks Matt. I just tested your patch, the single subflow test
> >> ("002 1 MP_FAIL, single subflow") works well. And very stable. The two
> >> issues that I mentioned in the cover letter of this series didn't happen
> >> when running this new selftest. So the first two squash-to patches in
> >> this series can be dropped.
> > 
> > That's nice! Thank you for having tested, I'm glad it helped!
> > 
> >> Mat, it looks like no need to analyse my attached pcaps any more. Thanks
> >> for your help.
> >>
> >>>
> >>> I now have this output:
> >>>
> >>> 001 0 MP_FAIL, multiple subflows         syn[ ok ] - synack[ ok ] - ack[
> >>> ok ]
> >>>                                          sum[ ok ] - csum  [ ok ]
> >>>                                          ftx[ ok ] - frx   [ ok ]
> >>>                                          itx[ ok ] - irx   [ ok ]
> >>
> >> But the multiple subflows test ("001 0 MP_FAIL, multiple subflows") doesn't
> >> work. I'll try to fix it tomorrow. I think it's easy to fix.
> 
> Yes, we need to restrict the 'iptables' command to the same interface as
> the one used with the TC command. Otherwise we will mark a flow for
> another interface and TC will never see a marked packet for this interface.
> 
> > Yes, it looked strange to me. I guess the test should fail if 'nr_fail'
> > is set to 0, no?
> > Or probably we should no longer use 'get_nr_fail' as we expect to have
> > only one MP_FAIL, no?
> > 
> > In the IPTables rule I added, I asked to mark the first packet that is
> > over 150 bytes. We can change "--packet" if we want to mark the Xth one.
> > And "--every" if we have more packets but worst case, that's not an
> > issue to corrupt another one I think: on this path, either we have no
> > more subflow (multiple subflows case) or no more MPTCP connection (1
> > subflow case).
> > 
> >>
> >> Thanks,
> >> -Geliang
> >>
> >>> Created /tmp/tmp.SNhvH7ucj9 (size 512 KB) containing data sent by client
> >>> Created /tmp/tmp.RFEqEXi0bl (size 512 KB) containing data sent by server
> >>> file received by server has inverted byte at 69
> >>> file received by server has inverted byte at 70
> >>> file received by server has inverted byte at 71
> >>> file received by server has inverted byte at 72
> > 
> > We can also modify the TC command to invert only a u8 instead of a u32
> > to reduce the log entries here.
> 
> While at it, I also applied the suggestions I made here and above,
> please see the attached patch. You can see a diff with the previous
> version I sent here: https://paste.opendev.org/show/812608/

Thanks Matt, I merged your patch in v2, and sent it to ML this morning.
And iterated a v3 according to Mat's suggestion.

The only difference is I kept nr_fail (now renamed to pedit_action) in
v3. It's useful for the cases that the pedit action did't happen.

Thanks,
-Geliang

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

> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index 26955abe49f0..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
> @@ -22,5 +25,5 @@ CONFIG_IPV6_MULTIPLE_TABLES=y
>  CONFIG_NET_ACT_CSUM=m
>  CONFIG_NET_ACT_PEDIT=m
>  CONFIG_NET_CLS_ACT=y
> -CONFIG_NET_CLS_FLOWER=m
> +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 a51b0915b7b1..ae50d64186c2 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -18,7 +18,6 @@ checksum=0
>  ip_mptcp=0
>  check_invert=0
>  validate_checksum=0
> -nr_fail=0
>  do_all_tests=1
>  
>  TEST_COUNT=0
> @@ -172,23 +171,20 @@ reset_with_allow_join_id0()
>  
>  # Modify TCP payload without corrupting the TCP packet
>  #
> -# This rule inverts a 32-bit word at byte offset 148 for all 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.
> -# But because the MPTCP checksum, covering the TCP options and data,
> -# has not been updated, we will detect the modification and emit an
> -# MP_FAIL: what we want to validate here.
> +# 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.
>  #
> -# Please note that this rule will produce this pr_info() message for
> -# each TCP ACK packets not carrying enough data:
> +# 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
>  #
> -# But this should be limited to a very few numbers of packets as we
> -# restrict this rule to outgoing TCP traffic with only the ACK flag
> -# + except the 3rd ACK, only packets carrying data should be seen in
> -# this direction.
> +# Netfilter is used to mark packets with enough data.
>  reset_with_fail()
>  {
>  	reset
> @@ -198,14 +194,22 @@ reset_with_fail()
>  
>  	check_invert=1
>  	validate_checksum=1
> -	nr_fail=0
> -	i="$1"
> +	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 \
> -		flower ip_proto tcp tcp_flags 0x10/0xff \
> -		action pedit munge offset 148 u32 invert \
> +		handle 42 fw \
> +		action pedit munge offset 148 u8 invert \
>  		pipe csum tcp \
>  		index 100
>  }
> @@ -228,12 +232,6 @@ 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
> @@ -294,21 +292,6 @@ link_failure()
>  	done
>  }
>  
> -get_nr_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')
> -	local overlimits=$(echo $action | jq '.[1].actions[0].stats.overlimits')
> -
> -	let pkt=$packets-$overlimits
> -	if [ $pkt -gt 0 ]; then
> -		nr_fail=1
> -	fi
> -	tc -n $ns2 qdisc del dev ns2eth$i clsact
> -}
> -
>  # $1: IP address
>  is_v6()
>  {
> @@ -2259,16 +2242,14 @@ fail_tests()
>  	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_fail 2
> -	chk_join_nr "$nr_fail MP_FAIL, multiple subflows" 2 2 2 $nr_fail
> +	chk_join_nr "MP_FAIL, multiple subflows" 2 2 2 1
>  
>  	# 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 4
> -	get_nr_fail 1
> -	chk_join_nr "$nr_fail MP_FAIL, single subflow" 0 0 0 $nr_fail 0
> +	chk_join_nr "MP_FAIL, single subflow" 0 0 0 1 0
>  }
>  
>  all_tests()

[PATCH mptcp-next 1/3] Squash to "mptcp: infinite mapping sending"
[PATCH mptcp-next 2/3] Squash to "mptcp: infinite mapping receiving"
[PATCH mptcp-next 3/3] selftests: mptcp: add mp_fail testcases