[PATCH mptcp-next v4 0/5] add mp_fail testcases

[PATCH mptcp-next v4 0/5] add mp_fail testcases

[Geliang Tang]

v4:
 - add the mibs for MP_RST
 - patch 4 "selftests: mptcp: add MP_RST mibs check" uses the variable
$nr_blank, so it depends on the commit "selftests: mptcp: adjust output
alignment for more tests".

v3:
 - check the exit code of iptables.
 - add ip6tables support for reset_with_fail too.
 - add the null check for $packets
 - rename nr_mp_fail to pedit_action and get_nr_mp_fail to
pedit_action_happened

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

Geliang Tang (5):
  Squash to "mptcp: infinite mapping receiving"
  Squash to "selftests: mptcp: add infinite map mibs check"
  mptcp: add mibs for MP_RST
  selftests: mptcp: add MP_RST mibs check
  selftests: mptcp: add mp_fail testcases

 net/mptcp/mib.c                               |   2 +
 net/mptcp/mib.h                               |   2 +
 net/mptcp/options.c                           |   2 +
 net/mptcp/subflow.c                           |   1 +
 tools/testing/selftests/net/mptcp/config      |   8 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 217 +++++++++++++++---
 6 files changed, 204 insertions(+), 28 deletions(-)

-- 
2.34.1

[PATCH mptcp-next v4 1/5] 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 v4 2/5] Squash to "selftests: mptcp: add infinite map mibs check"

[Geliang Tang]

Rename mp_infi_nr_tx, mp_infi_nr_rx and irx.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 90a6adc36490..3577716cd5e6 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -784,27 +784,27 @@ chk_fail_nr()
 
 chk_infi_nr()
 {
-	local mp_infi_nr_tx=$1
-	local mp_infi_nr_rx=$2
+	local infi_tx=$1
+	local infi_rx=$2
 	local count
 	local dump_stats
 
 	printf "%-${nr_blank}s %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
-		echo "[fail] got $count infinite map[s] TX expected $mp_infi_nr_tx"
+	if [ "$count" != "$infi_tx" ]; then
+		echo "[fail] got $count infinite map[s] TX expected $infi_tx"
 		ret=1
 		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
 
-	echo -n " - irx   "
+	echo -n " - infirx"
 	count=`ip netns exec $ns1 nstat -as | grep InfiniteMapRx | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != "$mp_infi_nr_rx" ]; then
-		echo "[fail] got $count infinite map[s] RX expected $mp_infi_nr_rx"
+	if [ "$count" != "$infi_rx" ]; then
+		echo "[fail] got $count infinite map[s] RX expected $infi_rx"
 		ret=1
 		dump_stats=1
 	else
-- 
2.34.1

[PATCH mptcp-next v4 3/5] mptcp: add mibs for MP_RST

[Geliang Tang]

This patch added two mibs for MP_RST, MPTCP_MIB_MPRSTTX for the MP_RST
sending and MPTCP_MIB_MPRSTRX for the MP_RST receiving.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/mib.c     | 2 ++
 net/mptcp/mib.h     | 2 ++
 net/mptcp/options.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index c12251cb0d44..bb7808eecab0 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -47,6 +47,8 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("MPPrioRx", MPTCP_MIB_MPPRIORX),
 	SNMP_MIB_ITEM("MPFailTx", MPTCP_MIB_MPFAILTX),
 	SNMP_MIB_ITEM("MPFailRx", MPTCP_MIB_MPFAILRX),
+	SNMP_MIB_ITEM("MPRstTx", MPTCP_MIB_MPRSTTX),
+	SNMP_MIB_ITEM("MPRstRx", MPTCP_MIB_MPRSTRX),
 	SNMP_MIB_ITEM("RcvPruned", MPTCP_MIB_RCVPRUNED),
 	SNMP_MIB_ITEM("SubflowStale", MPTCP_MIB_SUBFLOWSTALE),
 	SNMP_MIB_ITEM("SubflowRecover", MPTCP_MIB_SUBFLOWRECOVER),
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index 7901f1338d15..37226d8d5339 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -40,6 +40,8 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_MPPRIORX,		/* Received a MP_PRIO */
 	MPTCP_MIB_MPFAILTX,		/* Transmit a MP_FAIL */
 	MPTCP_MIB_MPFAILRX,		/* Received a MP_FAIL */
+	MPTCP_MIB_MPRSTTX,		/* Transmit a MP_RST */
+	MPTCP_MIB_MPRSTRX,		/* Received a MP_RST */
 	MPTCP_MIB_RCVPRUNED,		/* Incoming packet dropped due to memory limit */
 	MPTCP_MIB_SUBFLOWSTALE,		/* Subflows entered 'stale' status */
 	MPTCP_MIB_SUBFLOWRECOVER,	/* Subflows returned to active status after being stale */
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 670364cd455f..556d68e0e50b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -838,6 +838,7 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
 			*size += opt_size;
 			remaining -= opt_size;
+			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPRSTTX);
 		}
 		return true;
 	}
@@ -1159,6 +1160,7 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
 			subflow->reset_seen = 1;
 			subflow->reset_reason = mp_opt.reset_reason;
 			subflow->reset_transient = mp_opt.reset_transient;
+			MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPRSTRX);
 		}
 
 		if (!(mp_opt.suboptions & OPTION_MPTCP_DSS))
-- 
2.34.1

[PATCH mptcp-next v4 4/5] selftests: mptcp: add MP_RST mibs check

[Geliang Tang]

This patch added a function chk_rst_nr() to check the numbers of the
MP_RST mibs for sending and receiving.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 3577716cd5e6..95d61c97ccad 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -782,6 +782,38 @@ chk_fail_nr()
 	[ "${dump_stats}" = 1 ] && dump_stats
 }
 
+chk_rst_nr()
+{
+	local rst_tx=$1
+	local rst_rx=$2
+	local count
+	local dump_stats
+
+	printf "%-${nr_blank}s %s" " " "rtx"
+	count=`ip netns exec $ns1 nstat -as | grep MPTcpExtMPRstTx | awk '{print $2}'`
+	[ -z "$count" ] && count=0
+	if [ "$count" != "$rst_tx" ]; then
+		echo "[fail] got $count MP_RST[s] TX expected $rst_tx"
+		ret=1
+		dump_stats=1
+	else
+		echo -n "[ ok ]"
+	fi
+
+	echo -n " - rstrx "
+	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtMPRstRx | awk '{print $2}'`
+	[ -z "$count" ] && count=0
+	if [ "$count" != "$rst_rx" ]; then
+		echo "[fail] got $count MP_RST[s] RX expected $rst_rx"
+		ret=1
+		dump_stats=1
+	else
+		echo "[ ok ]"
+	fi
+
+	[ "${dump_stats}" = 1 ] && dump_stats
+}
+
 chk_infi_nr()
 {
 	local infi_tx=$1
@@ -859,6 +891,7 @@ chk_join_nr()
 	if [ $checksum -eq 1 ]; then
 		chk_csum_nr
 		chk_fail_nr 0 0
+		chk_rst_nr 0 0
 		chk_infi_nr 0 0
 	fi
 }
-- 
2.34.1

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

[Geliang Tang]

Added the test cases for MP_FAIL, the multiple subflows test for the MP_RST
case and the single subflow one for the infinite mapping case.

These tests used the new test_linkfail value 3 to make 512KB test files for
both the client and server.

Added a new function reset_with_fail(), in it use 'iptables' and 'tc
action pedit' commands to trigger the checksum failures.

Added a new global variable pedit_action to trace whether the tc pedit
action happened during the test. Check and set it in the new function
pedit_action_happened(). Show it in the test description and pass it to
chk_join_nr() to check the numbers of the checksum failures, MP_FAIL
sending and receiving, the infinite mapping sending and receiving.

Added a new global variable validate_checksum to enable checksums for
the MP_FAIL tests without passing the '-C' argument.

Also added the tests needed kernel configures in the config file.

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 | 172 +++++++++++++++---
 2 files changed, 158 insertions(+), 22 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 95d61c97ccad..880430058492 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
+pedit_action=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,63 @@ 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
+	pedit_action=0
+	local i="$1"
+	local ip="${2:-4}"
+	local tables
+
+	tables="iptables"
+	if [ $ip -eq 6 ]; then
+		tables="ip6tables"
+	fi
+
+	ip netns exec $ns2 $tables \
+		-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
+	if [ $? -ne 0 ];then
+		echo "SKIP: Couldn not add the $tables rule"
+		exit $ksft_skip
+	fi
+
+	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 +245,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 +311,19 @@ link_failure()
 	done
 }
 
+pedit_action_happened()
+{
+	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')
+
+	if [ $packets != "null" ] && [ $packets -gt 0 ]; then
+		pedit_action=1
+	fi
+	tc -n $ns2 qdisc del dev ns2eth$i clsact
+}
+
 # $1: IP address
 is_v6()
 {
@@ -446,7 +525,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 +545,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 +717,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 +766,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 +784,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 +814,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 +827,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 +837,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
@@ -752,27 +849,27 @@ chk_csum_nr()
 
 chk_fail_nr()
 {
-	local mp_fail_nr_tx=$1
-	local mp_fail_nr_rx=$2
+	local fail_tx=$1
+	local fail_rx=$2
 	local count
 	local dump_stats
 
 	printf "%-${nr_blank}s %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
-		echo "[fail] got $count MP_FAIL[s] TX expected $mp_fail_nr_tx"
+	if [ "$count" != "$fail_tx" ]; then
+		echo "[fail] got $count MP_FAIL[s] TX expected $fail_tx"
 		ret=1
 		dump_stats=1
 	else
 		echo -n "[ ok ]"
 	fi
 
-	echo -n " - frx   "
+	echo -n " - failrx"
 	count=`ip netns exec $ns2 nstat -as | grep MPTcpExtMPFailRx | awk '{print $2}'`
 	[ -z "$count" ] && count=0
-	if [ "$count" != "$mp_fail_nr_rx" ]; then
-		echo "[fail] got $count MP_FAIL[s] RX expected $mp_fail_nr_rx"
+	if [ "$count" != "$fail_rx" ]; then
+		echo "[fail] got $count MP_FAIL[s] RX expected $fail_rx"
 		ret=1
 		dump_stats=1
 	else
@@ -852,6 +949,9 @@ chk_join_nr()
 	local syn_nr=$2
 	local syn_ack_nr=$3
 	local ack_nr=$4
+	local fail_nr=${5:-0}
+	local rst_nr=${6:-0}
+	local infi_nr=${7:-0}
 	local count
 	local dump_stats
 
@@ -888,11 +988,11 @@ 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_rst_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_rst_nr $rst_nr $rst_nr
+		chk_infi_nr $infi_nr $infi_nr
 	fi
 }
 
@@ -2197,6 +2297,29 @@ 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
+	pedit_action_happened 2
+	chk_join_nr "MP_FAIL MP_RST: $pedit_action pedit action" 2 2 2 \
+		$pedit_action $pedit_action
+
+	# 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
+	pedit_action_happened 1
+	chk_join_nr "MP_FAIL infinite map: $pedit_action pedit action" 0 0 0 \
+		$pedit_action 0 $pedit_action
+}
+
 all_tests()
 {
 	subflows_tests
@@ -2215,6 +2338,7 @@ all_tests()
 	deny_join_id0_tests
 	fullmesh_tests
 	userspace_tests
+	fail_tests
 }
 
 usage()
@@ -2236,6 +2360,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"
@@ -2275,7 +2400,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
@@ -2325,6 +2450,9 @@ while getopts 'fesltra64bpkdmuchCSi' opt; do
 		u)
 			userspace_tests
 			;;
+		F)
+			fail_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.34.1

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

[Matthieu Baerts]

Hi Geliang,

On 09/02/2022 09:57, Geliang Tang wrote:
> Added the test cases for MP_FAIL, the multiple subflows test for the MP_RST
> case and the single subflow one for the infinite mapping case.

Thank you for the new version!

> These tests used the new test_linkfail value 3 to make 512KB test files for
> both the client and server.
> 
> Added a new function reset_with_fail(), in it use 'iptables' and 'tc
> action pedit' commands to trigger the checksum failures.
> 
> Added a new global variable pedit_action to trace whether the tc pedit
> action happened during the test. Check and set it in the new function
> pedit_action_happened(). Show it in the test description and pass it to
> chk_join_nr() to check the numbers of the checksum failures, MP_FAIL

Related to my comment in the v2: I don't think it is a good idea to pass
it to chk_join_nr(): we expect an MP_FAIL, we should not ignore it if it
was not there.

(...)

> +	ip netns exec $ns2 $tables \
> +		-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
> +	if [ $? -ne 0 ];then
> +		echo "SKIP: Couldn not add the $tables rule"
> +		exit $ksft_skip
> +	fi

Related to my comment in the v2: I think we should exit with an error if
the IPTables command fails.

  (...)
  -j MARK --set-mark 42 || exit 1

The error message from IPTables should be enough I think.

In Mat's case, it was confusing because the test continued and even
succeeded!

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

Same for these two tc commands, we can add "|| exit 1".

(...)

> +pedit_action_happened()
> +{
> +	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')
> +
> +	if [ $packets != "null" ] && [ $packets -gt 0 ]; then
> +		pedit_action=1
> +	fi
> +	tc -n $ns2 qdisc del dev ns2eth$i clsact

It is a good idea to keep this just in case there was an issue with TC.
But then, I think we should print the number of packets, that's enough no?

(also we don't need to delete the clsact, it will be removed when the NS
will be removed)

So it would be enough to have:

--------
pedit_action_pkt()
{
    tc -n $ns2 -j -s action show action pedit index 100 | \
        jq '.[1].actions[0].stats.packets'
}

(...)

chk_join_nr "MP_FAIL MP_RST: $(pedit_action_pkt) corrupted pkt" 2 2 2 1 1
--------

We can even remove the dependence to 'jq' for this simple command:

    tc -n $ns2 -j -s action show action pedit index 100 | \
        sed 's/.*"packets":\([0-9]\+\),.*/\1/'

WDYT?

> +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
> +	pedit_action_happened 2
> +	chk_join_nr "MP_FAIL MP_RST: $pedit_action pedit action" 2 2 2 \
> +		$pedit_action $pedit_action

Related to my comment in the v2: we should not use $pedit_action in the
args we need to verify I think. Instead we should probably then have:

  chk_join_nr "(...)" 2 2 2 1 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 3
> +	pedit_action_happened 1
> +	chk_join_nr "MP_FAIL infinite map: $pedit_action pedit action" 0 0 0 \
> +		$pedit_action 0 $pedit_action

Same here:

  chk_join_nr "(...)" 0 0 0 1 0 1

No?

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

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

[Geliang Tang]

Hi Matt,

On Wed, Feb 09, 2022 at 10:23:52AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/02/2022 09:57, Geliang Tang wrote:
> > Added the test cases for MP_FAIL, the multiple subflows test for the MP_RST
> > case and the single subflow one for the infinite mapping case.
> 
> Thank you for the new version!
> 
> > These tests used the new test_linkfail value 3 to make 512KB test files for
> > both the client and server.
> > 
> > Added a new function reset_with_fail(), in it use 'iptables' and 'tc
> > action pedit' commands to trigger the checksum failures.
> > 
> > Added a new global variable pedit_action to trace whether the tc pedit
> > action happened during the test. Check and set it in the new function
> > pedit_action_happened(). Show it in the test description and pass it to
> > chk_join_nr() to check the numbers of the checksum failures, MP_FAIL
> 
> Related to my comment in the v2: I don't think it is a good idea to pass
> it to chk_join_nr(): we expect an MP_FAIL, we should not ignore it if it
> was not there.
> 
> (...)
> 
> > +	ip netns exec $ns2 $tables \
> > +		-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
> > +	if [ $? -ne 0 ];then
> > +		echo "SKIP: Couldn not add the $tables rule"
> > +		exit $ksft_skip
> > +	fi
> 
> Related to my comment in the v2: I think we should exit with an error if
> the IPTables command fails.
> 
>   (...)
>   -j MARK --set-mark 42 || exit 1
> 
> The error message from IPTables should be enough I think.
> 
> In Mat's case, it was confusing because the test continued and even
> succeeded!
> 
> > +
> > +	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
> 
> Same for these two tc commands, we can add "|| exit 1".
> 
> (...)
> 
> > +pedit_action_happened()
> > +{
> > +	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')
> > +
> > +	if [ $packets != "null" ] && [ $packets -gt 0 ]; then
> > +		pedit_action=1
> > +	fi
> > +	tc -n $ns2 qdisc del dev ns2eth$i clsact
> 
> It is a good idea to keep this just in case there was an issue with TC.
> But then, I think we should print the number of packets, that's enough no?
> 
> (also we don't need to delete the clsact, it will be removed when the NS
> will be removed)
> 
> So it would be enough to have:
> 
> --------
> pedit_action_pkt()
> {
>     tc -n $ns2 -j -s action show action pedit index 100 | \
>         jq '.[1].actions[0].stats.packets'
> }
> 
> (...)
> 
> chk_join_nr "MP_FAIL MP_RST: $(pedit_action_pkt) corrupted pkt" 2 2 2 1 1
> --------
> 
> We can even remove the dependence to 'jq' for this simple command:
> 
>     tc -n $ns2 -j -s action show action pedit index 100 | \
>         sed 's/.*"packets":\([0-9]\+\),.*/\1/'
> 
> WDYT?
> 
> > +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
> > +	pedit_action_happened 2
> > +	chk_join_nr "MP_FAIL MP_RST: $pedit_action pedit action" 2 2 2 \
> > +		$pedit_action $pedit_action
> 
> Related to my comment in the v2: we should not use $pedit_action in the
> args we need to verify I think. Instead we should probably then have:
> 
>   chk_join_nr "(...)" 2 2 2 1 1

Thanks for your review and suggestions. I updated them in v5.

We will get this failure in v5 in rare cases, about one percent
probability:

Created /tmp/tmp.e4nE5Q14mj (size 1024 KB) containing data sent by client
Created /tmp/tmp.QwpQYClFnm (size 1024 KB) containing data sent by server
001 MP_FAIL MP_RST: 0 corrupted pkts     syn[ ok ] - synack[ ok ] - ack[ ok ]
                                         sum[fail] got 0 data checksum error[s] expected 1
 - csum  [ ok ]
Server ns stats
TcpPassiveOpens                 3                  0.0
TcpInSegs                       311                0.0
TcpOutSegs                      937                0.0
TcpExtTCPPureAcks               155                0.0
TcpExtTCPOrigDataSent           787                0.0
TcpExtTCPDelivered              787                0.0
MPTcpExtMPCapableSYNRX          1                  0.0
MPTcpExtMPCapableACKRX          1                  0.0
MPTcpExtMPJoinSynRx             2                  0.0
MPTcpExtMPJoinAckRx             2                  0.0
Client ns stats
TcpActiveOpens                  3                  0.0
TcpInSegs                       305                0.0
TcpOutSegs                      943                0.0
TcpExtTW                        3                  0.0
TcpExtTCPPureAcks               147                0.0
TcpExtTCPOrigDataSent           785                0.0
TcpExtTCPDelivered              788                0.0
MPTcpExtMPCapableSYNTX          1                  0.0
MPTcpExtMPCapableSYNACKRX       1                  0.0
MPTcpExtMPJoinSynAckRx          2                  0.0
                                         ftx[fail] got 0 MP_FAIL[s] TX expected 1
 - failrx[fail] got 0 MP_FAIL[s] RX expected 1
Server ns stats
TcpPassiveOpens                 3                  0.0
TcpInSegs                       311                0.0
TcpOutSegs                      937                0.0
TcpExtTCPPureAcks               155                0.0
TcpExtTCPOrigDataSent           787                0.0
TcpExtTCPDelivered              787                0.0
MPTcpExtMPCapableSYNRX          1                  0.0
MPTcpExtMPCapableACKRX          1                  0.0
MPTcpExtMPJoinSynRx             2                  0.0
MPTcpExtMPJoinAckRx             2                  0.0
Client ns stats
TcpActiveOpens                  3                  0.0
TcpInSegs                       305                0.0
TcpOutSegs                      943                0.0
TcpExtTW                        3                  0.0
TcpExtTCPPureAcks               147                0.0
TcpExtTCPOrigDataSent           785                0.0
TcpExtTCPDelivered              788                0.0
MPTcpExtMPCapableSYNTX          1                  0.0
MPTcpExtMPCapableSYNACKRX       1                  0.0
MPTcpExtMPJoinSynAckRx          2                  0.0
                                         rtx[fail] got 0 MP_RST[s] TX expected 1
 - rstrx [fail] got 0 MP_RST[s] RX expected 1
Server ns stats
TcpPassiveOpens                 3                  0.0
TcpInSegs                       311                0.0
TcpOutSegs                      937                0.0
TcpExtTCPPureAcks               155                0.0
TcpExtTCPOrigDataSent           787                0.0
TcpExtTCPDelivered              787                0.0
MPTcpExtMPCapableSYNRX          1                  0.0
MPTcpExtMPCapableACKRX          1                  0.0
MPTcpExtMPJoinSynRx             2                  0.0
MPTcpExtMPJoinAckRx             2                  0.0
Client ns stats
TcpActiveOpens                  3                  0.0
TcpInSegs                       305                0.0
TcpOutSegs                      943                0.0
TcpExtTW                        3                  0.0
TcpExtTCPPureAcks               147                0.0
TcpExtTCPOrigDataSent           785                0.0
TcpExtTCPDelivered              788                0.0
MPTcpExtMPCapableSYNTX          1                  0.0
MPTcpExtMPCapableSYNACKRX       1                  0.0
MPTcpExtMPJoinSynAckRx          2                  0.0
                                         itx[ ok ] - infirx[ ok ]

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
> > +	pedit_action_happened 1
> > +	chk_join_nr "MP_FAIL infinite map: $pedit_action pedit action" 0 0 0 \
> > +		$pedit_action 0 $pedit_action
> 
> Same here:
> 
>   chk_join_nr "(...)" 0 0 0 1 0 1
> 
> No?
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

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

[Matthieu Baerts]

Hi Geliang,

On 09/02/2022 11:55, Geliang Tang wrote:
> On Wed, Feb 09, 2022 at 10:23:52AM +0100, Matthieu Baerts wrote:
>> On 09/02/2022 09:57, 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
>>> +	pedit_action_happened 2
>>> +	chk_join_nr "MP_FAIL MP_RST: $pedit_action pedit action" 2 2 2 \
>>> +		$pedit_action $pedit_action
>>
>> Related to my comment in the v2: we should not use $pedit_action in the
>> args we need to verify I think. Instead we should probably then have:
>>
>>   chk_join_nr "(...)" 2 2 2 1 1
> 
> Thanks for your review and suggestions. I updated them in v5.
> 
> We will get this failure in v5 in rare cases, about one percent
> probability

Do you know why?

We invert a byte so it should always cause an issue, no? (we can always
have 2 same hashes for two different sources but it is a very very rare
case we don't have to consider I think, especially when the content is
so close, no?)

Or maybe we need to improve the IPTables and TC rules?

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

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

[Geliang Tang]

Hi Matt,

On Wed, Feb 09, 2022 at 12:13:00PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 09/02/2022 11:55, Geliang Tang wrote:
> > On Wed, Feb 09, 2022 at 10:23:52AM +0100, Matthieu Baerts wrote:
> >> On 09/02/2022 09:57, 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
> >>> +	pedit_action_happened 2
> >>> +	chk_join_nr "MP_FAIL MP_RST: $pedit_action pedit action" 2 2 2 \
> >>> +		$pedit_action $pedit_action
> >>
> >> Related to my comment in the v2: we should not use $pedit_action in the
> >> args we need to verify I think. Instead we should probably then have:
> >>
> >>   chk_join_nr "(...)" 2 2 2 1 1
> > 
> > Thanks for your review and suggestions. I updated them in v5.
> > 
> > We will get this failure in v5 in rare cases, about one percent
> > probability
> 
> Do you know why?
> 
> We invert a byte so it should always cause an issue, no? (we can always
> have 2 same hashes for two different sources but it is a very very rare
> case we don't have to consider I think, especially when the content is
> so close, no?)
> 
> Or maybe we need to improve the IPTables and TC rules?
> 

I think we should go back to the old version of this patch. We shouldn't
always expect getting 1 MP_FAIL. If no checksum failure occur, we should
expect no MP_FAIL. I just sent a squash-to patch to fix this.

Thanks,
-Geliang

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