[PATCH mptcp-next v6 0/2] send MP_FAIL with MP_RST and others

[PATCH mptcp-next v6 0/2] send MP_FAIL with MP_RST and others

[Geliang Tang]

v6:
 - patch 1, use the same xmit logic for FASTCLOSE as MP_FAIL.

v5:
 - Since MP_FAIL could be sent with MP_RST and DSS, and MP_RST could be
sent with MP_FAIL and FASTCLOSE, this patch fixed the MP_RST sending in
mptcp_established_options() and mptcp_write_options().

v4:
 - add Matt's patch, fix the missing ";" in it, dropped two empty lines
and added my Tested-by tag.

v3:
 - update patch 1 as Matt suggested.
 - do more cleanups in patch 1.

v2:
 - rename patch 1 as a squash-to patch.
 - print out reset_transient in patch 2 too. Please keep patch 2 as a
standalone patch, not a squash-to patch.

Two small patches about sending MP_FAIL with MP_RST.

Geliang Tang (2):
  Squash to "mptcp: implement fastclose xmit path"
  mptcp: print out reset infos of MP_RST

 net/mptcp/options.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

-- 
2.31.1

[PATCH mptcp-next v6 1/2] Squash to "mptcp: implement fastclose xmit path"

[Geliang Tang]

MP_FAIL could be sent with RST or DSS, and FASTCLOSE could be sent with
RST or DSS too. So we should use the same xmit logic for FASTCLOSE as
MP_FAIL.

Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/options.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 8a1020e4285c..c5c0dd983ad6 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -828,9 +828,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 		return false;
 
 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
-		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
+		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
+		    mptcp_established_options_fastclose(sk, &opt_size, remaining, opts)) {
+			*size += opt_size;
+			remaining -= opt_size;
+		}
+		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
 			*size += opt_size;
 			remaining -= opt_size;
 		}
@@ -842,7 +845,8 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 		ret = true;
 	else if (mptcp_established_options_dss(sk, skb, snd_data_fin, &opt_size, remaining, opts)) {
 		ret = true;
-		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
+		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
+		    mptcp_established_options_fastclose(sk, &opt_size, remaining, opts)) {
 			*size += opt_size;
 			remaining -= opt_size;
 			return true;
@@ -1269,9 +1273,16 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 				      0, 0);
 		put_unaligned_be64(opts->fail_seq, ptr);
 		ptr += 2;
+	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
+		/* FASTCLOSE is mutually exclusive with others except DSS and RST */
+		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
+				      TCPOLEN_MPTCP_FASTCLOSE,
+				      0, 0);
+		put_unaligned_be64(opts->rcvr_key, ptr);
+		ptr += 2;
 	}
 
-	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
+	/* DSS, MPC, MPJ, ADD_ADDR and RST are mutually exclusive,
 	 * see mptcp_established_options*()
 	 */
 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
@@ -1465,13 +1476,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 				      opts->reset_transient,
 				      opts->reset_reason);
 		return;
-	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
-		/* FASTCLOSE is mutually exclusive with everything else */
-		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
-				      TCPOLEN_MPTCP_FASTCLOSE,
-				      0, 0);
-		put_unaligned_be64(opts->rcvr_key, ptr);
-		return;
 	}
 
 	if (OPTION_MPTCP_PRIO & opts->suboptions) {
-- 
2.31.1

[PATCH mptcp-next v6 2/2] mptcp: print out reset infos of MP_RST

[Geliang Tang]

This patch printed out the reset infos, reset_transient and reset_reason,
of MP_RST in mptcp_parse_option() to show that MP_RST is received.

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

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c5c0dd983ad6..7c101524b48c 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -336,6 +336,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		flags = *ptr++;
 		mp_opt->reset_transient = flags & MPTCP_RST_TRANSIENT;
 		mp_opt->reset_reason = *ptr;
+		pr_debug("MP_RST: transient=%u reason=%u",
+			 mp_opt->reset_transient, mp_opt->reset_reason);
 		break;
 
 	case MPTCPOPT_MP_FAIL:
-- 
2.31.1

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

[Geliang Tang]

Added the test cases for MP_FAIL, use 'tc' command to trigger the
checksum failure.

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>
---
 v4:
 - enable checksum in reset_with_fail().
---
 tools/testing/selftests/net/mptcp/config      |   5 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 113 ++++++++++++++++--
 2 files changed, 110 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
index 419e71560fd1..37124a6891b7 100644
--- a/tools/testing/selftests/net/mptcp/config
+++ b/tools/testing/selftests/net/mptcp/config
@@ -19,3 +19,8 @@ CONFIG_NFT_SOCKET=m
 CONFIG_IP_ADVANCED_ROUTER=y
 CONFIG_IP_MULTIPLE_TABLES=y
 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 2684ef9c0d42..7a9dec7fa648 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -160,6 +160,48 @@ 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
+
+	checksum=1
+
+	make_file "$cin" "client" 20480
+	make_file "$sin" "server" 20480
+
+	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"
@@ -178,6 +220,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
@@ -232,6 +280,24 @@ link_failure()
 	done
 }
 
+mp_fail_del_dev()
+{
+	i="$1"
+
+	for n in `seq 1 100`; do
+		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
+			sleep .1
+			tc -n $ns2 qdisc del dev ns2eth$i clsact
+			break
+		fi
+	done
+}
+
 # $1: IP address
 is_v6()
 {
@@ -371,6 +437,9 @@ do_transfer()
 	if [[ "${addr_nr_ns2}" = "fullmesh_"* ]]; then
 		flags="${flags},fullmesh"
 		addr_nr_ns2=${addr_nr_ns2:9}
+	elif [[ "${addr_nr_ns2}" = "fail_"* ]]; then
+		mp_fail_del_dev ${addr_nr_ns2:5}
+		addr_nr_ns2=0
 	fi
 
 	if [ $addr_nr_ns2 -gt 0 ]; then
@@ -542,6 +611,8 @@ run_tests()
 chk_csum_nr()
 {
 	local msg=${1:-""}
+	local csum_ns1=${2:-0}
+	local csum_ns2=${3:-0}
 	local count
 	local dump_stats
 
@@ -553,8 +624,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
@@ -563,8 +634,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
@@ -658,6 +729,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
 
@@ -700,9 +773,9 @@ chk_join_nr()
 		ip netns exec $ns2 nstat -as | grep MPTcp
 	fi
 	if [ $checksum -eq 1 ]; then
-		chk_csum_nr
-		chk_fail_nr 0 0
-		chk_infi_nr 0 0
+		chk_csum_nr "" $fail_nr
+		chk_fail_nr $fail_nr $fail_nr
+		chk_infi_nr $infi_nr $infi_nr
 	fi
 }
 
@@ -1837,6 +1910,25 @@ fullmesh_tests()
 	chk_add_nr 1 1
 }
 
+fail_tests()
+{
+	# multiple subflows
+	reset_with_fail 2
+	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
+	ip netns exec $ns2 ./pm_nl_ctl limits 0 2
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow
+	ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 dev ns2eth2 flags subflow
+	run_tests $ns1 $ns2 10.0.1.1 0 0 "fail_2"
+	chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
+
+	# single subflow
+	reset_with_fail 1
+	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
+	ip netns exec $ns2 ./pm_nl_ctl limits 0 2
+	run_tests $ns1 $ns2 10.0.1.1 0 0 "fail_1"
+	chk_join_nr "MP_FAIL test, single subflow" 0 0 0 1 1
+}
+
 all_tests()
 {
 	subflows_tests
@@ -1853,6 +1945,7 @@ all_tests()
 	checksum_tests
 	deny_join_id0_tests
 	fullmesh_tests
+	fail_tests
 }
 
 usage()
@@ -1872,6 +1965,7 @@ usage()
 	echo "  -S checksum_tests"
 	echo "  -d deny_join_id0_tests"
 	echo "  -m fullmesh_tests"
+	echo "  -F fail_tests"
 	echo "  -c capture pcap files"
 	echo "  -C enable data checksum"
 	echo "  -h help"
@@ -1907,7 +2001,7 @@ if [ $do_all_tests -eq 1 ]; then
 	exit $ret
 fi
 
-while getopts 'fsltra64bpkdmchCS' opt; do
+while getopts 'fsltra64bpkdmchCSF' opt; do
 	case $opt in
 		f)
 			subflows_tests
@@ -1951,6 +2045,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
 		m)
 			fullmesh_tests
 			;;
+		F)
+			fail_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.31.1

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

[Mat Martineau]

On Mon, 6 Dec 2021, Geliang Tang wrote:

> Added the test cases for MP_FAIL, use 'tc' command to trigger the
> checksum failure.
>
> 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>
> ---
> v4:
> - enable checksum in reset_with_fail().

Thanks for the v4. Some comments below.

I also have a small request - this patch was threaded with the MP_FAIL 
patches:

[PATCH mptcp-next v6 0/2] send MP_FAIL with MP_RST and others
  2021-12-06  3:59 UTC  (4+ messages)
` [PATCH mptcp-next v6 1/2] Squash to "mptcp: implement fastclose xmit path"
` [PATCH mptcp-next v6 2/2] mptcp: print out reset infos of MP_RST
` [PATCH mptcp-next v4] selftests: mptcp: add mp_fail testcases

And the mix of v6 and v4 really confused the b4 tool I use to download 
patches from lore.kernel.org. It would help to either use a 3-patch series 
or separately format the patches to avoid combining the email threads!

> ---
> tools/testing/selftests/net/mptcp/config      |   5 +
> .../testing/selftests/net/mptcp/mptcp_join.sh | 113 ++++++++++++++++--
> 2 files changed, 110 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index 419e71560fd1..37124a6891b7 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -19,3 +19,8 @@ CONFIG_NFT_SOCKET=m
> CONFIG_IP_ADVANCED_ROUTER=y
> CONFIG_IP_MULTIPLE_TABLES=y
> 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 2684ef9c0d42..7a9dec7fa648 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -160,6 +160,48 @@ 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
> +
> +	checksum=1

This assignment will enable checksums on all tests that run after this 
too. Suggest adding a different global variable to compare in 
chk_join_nr(), so you could set that here instead. For example:

validate_checksum=1

here, and

validate_checksum=$checksum

in init(). Then replace $checksum with $validate_checksum in chk_join_nr()

-Mat


> +
> +	make_file "$cin" "client" 20480
> +	make_file "$sin" "server" 20480
> +
> +	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"
> @@ -178,6 +220,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
> @@ -232,6 +280,24 @@ link_failure()
> 	done
> }
>
> +mp_fail_del_dev()
> +{
> +	i="$1"
> +
> +	for n in `seq 1 100`; do
> +		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
> +			sleep .1
> +			tc -n $ns2 qdisc del dev ns2eth$i clsact
> +			break
> +		fi
> +	done
> +}
> +
> # $1: IP address
> is_v6()
> {
> @@ -371,6 +437,9 @@ do_transfer()
> 	if [[ "${addr_nr_ns2}" = "fullmesh_"* ]]; then
> 		flags="${flags},fullmesh"
> 		addr_nr_ns2=${addr_nr_ns2:9}
> +	elif [[ "${addr_nr_ns2}" = "fail_"* ]]; then
> +		mp_fail_del_dev ${addr_nr_ns2:5}
> +		addr_nr_ns2=0
> 	fi
>
> 	if [ $addr_nr_ns2 -gt 0 ]; then
> @@ -542,6 +611,8 @@ run_tests()
> chk_csum_nr()
> {
> 	local msg=${1:-""}
> +	local csum_ns1=${2:-0}
> +	local csum_ns2=${3:-0}
> 	local count
> 	local dump_stats
>
> @@ -553,8 +624,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
> @@ -563,8 +634,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
> @@ -658,6 +729,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
>
> @@ -700,9 +773,9 @@ chk_join_nr()
> 		ip netns exec $ns2 nstat -as | grep MPTcp
> 	fi
> 	if [ $checksum -eq 1 ]; then
> -		chk_csum_nr
> -		chk_fail_nr 0 0
> -		chk_infi_nr 0 0
> +		chk_csum_nr "" $fail_nr
> +		chk_fail_nr $fail_nr $fail_nr
> +		chk_infi_nr $infi_nr $infi_nr
> 	fi
> }
>
> @@ -1837,6 +1910,25 @@ fullmesh_tests()
> 	chk_add_nr 1 1
> }
>
> +fail_tests()
> +{
> +	# multiple subflows
> +	reset_with_fail 2
> +	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
> +	ip netns exec $ns2 ./pm_nl_ctl limits 0 2
> +	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow
> +	ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 dev ns2eth2 flags subflow
> +	run_tests $ns1 $ns2 10.0.1.1 0 0 "fail_2"
> +	chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
> +
> +	# single subflow
> +	reset_with_fail 1
> +	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
> +	ip netns exec $ns2 ./pm_nl_ctl limits 0 2
> +	run_tests $ns1 $ns2 10.0.1.1 0 0 "fail_1"
> +	chk_join_nr "MP_FAIL test, single subflow" 0 0 0 1 1
> +}
> +
> all_tests()
> {
> 	subflows_tests
> @@ -1853,6 +1945,7 @@ all_tests()
> 	checksum_tests
> 	deny_join_id0_tests
> 	fullmesh_tests
> +	fail_tests
> }
>
> usage()
> @@ -1872,6 +1965,7 @@ usage()
> 	echo "  -S checksum_tests"
> 	echo "  -d deny_join_id0_tests"
> 	echo "  -m fullmesh_tests"
> +	echo "  -F fail_tests"
> 	echo "  -c capture pcap files"
> 	echo "  -C enable data checksum"
> 	echo "  -h help"
> @@ -1907,7 +2001,7 @@ if [ $do_all_tests -eq 1 ]; then
> 	exit $ret
> fi
>
> -while getopts 'fsltra64bpkdmchCS' opt; do
> +while getopts 'fsltra64bpkdmchCSF' opt; do
> 	case $opt in
> 		f)
> 			subflows_tests
> @@ -1951,6 +2045,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
> 		m)
> 			fullmesh_tests
> 			;;
> +		F)
> +			fail_tests
> +			;;
> 		c)
> 			;;
> 		C)
> -- 
> 2.31.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v6 1/2] Squash to "mptcp: implement fastclose xmit path"

[Mat Martineau]

On Mon, 6 Dec 2021, Geliang Tang wrote:

> MP_FAIL could be sent with RST or DSS, and FASTCLOSE could be sent with
> RST or DSS too. So we should use the same xmit logic for FASTCLOSE as
> MP_FAIL.
>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Matthieu Baerts <matthieu.baerts@tessares.net>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
> net/mptcp/options.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 8a1020e4285c..c5c0dd983ad6 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -828,9 +828,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> 		return false;
>
> 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
> -		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
> -		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
> -		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> +		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
> +		    mptcp_established_options_fastclose(sk, &opt_size, remaining, opts)) {
> +			*size += opt_size;
> +			remaining -= opt_size;
> +		}
> +		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
> 			*size += opt_size;
> 			remaining -= opt_size;
> 		}
> @@ -842,7 +845,8 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
> 		ret = true;
> 	else if (mptcp_established_options_dss(sk, skb, snd_data_fin, &opt_size, remaining, opts)) {
> 		ret = true;
> -		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
> +		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
> +		    mptcp_established_options_fastclose(sk, &opt_size, remaining, opts)) {

Geliang -

Did you see a case where the stack attempted to send both DSS and 
FASTCLOSE? If so, was it just a DSS_ACK or was there a mapping and data 
payload?

From the RFC, it seems like FASTCLOSE is only expected on a TCP ACK or TCP 
RST. When the FASTCLOSE is sent on all subflows (like the only code path 
that sets send_fastclose does), it is only expected on a TCP RST. If 
FASTCLOSE is showing up on something other than a TCP RST packet I think 
that's a different bug that this patch doesn't address.


The patch has changed a lot since Paolo said "LGTM" so hopefully he can 
comment again!


-Mat

> 			*size += opt_size;
> 			remaining -= opt_size;
> 			return true;
> @@ -1269,9 +1273,16 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 				      0, 0);
> 		put_unaligned_be64(opts->fail_seq, ptr);
> 		ptr += 2;
> +	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
> +		/* FASTCLOSE is mutually exclusive with others except DSS and RST */
> +		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
> +				      TCPOLEN_MPTCP_FASTCLOSE,
> +				      0, 0);
> +		put_unaligned_be64(opts->rcvr_key, ptr);
> +		ptr += 2;
> 	}
>
> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
> +	/* DSS, MPC, MPJ, ADD_ADDR and RST are mutually exclusive,
> 	 * see mptcp_established_options*()
> 	 */
> 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> @@ -1465,13 +1476,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 				      opts->reset_transient,
> 				      opts->reset_reason);
> 		return;
> -	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
> -		/* FASTCLOSE is mutually exclusive with everything else */
> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
> -				      TCPOLEN_MPTCP_FASTCLOSE,
> -				      0, 0);
> -		put_unaligned_be64(opts->rcvr_key, ptr);
> -		return;
> 	}
>
> 	if (OPTION_MPTCP_PRIO & opts->suboptions) {
> -- 
> 2.31.1
>
>
>

--
Mat Martineau
Intel

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

[Matthieu Baerts]

Hi Geliang,

On 06/12/2021 04:59, Geliang Tang wrote:
> Added the test cases for MP_FAIL, use 'tc' command to trigger the
> checksum failure.

Thank you for the new version!

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 2684ef9c0d42..7a9dec7fa648 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -160,6 +160,48 @@ 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
> +
> +	checksum=1
> +
> +	make_file "$cin" "client" 20480
> +	make_file "$sin" "server" 20480

I don't think you can call make_file() with cin and sin: it means all
tests running after will have a different size than the expected one,
no? I guess you should generate files in another variable than $[cs]in.

Also, we should really make sure not to have a lot of pr_info() messages
that will cause confusion and issues if they are too many. To avoid that
and as written in the comment above, only the server in ns2 should send
data otherwise the server might send a lot of pure ACK and cause a lot
of pr_info() from tc. Maybe easier to use $cin with the default one byte
and $sin (using another variable, see above) 20480.

(...)

> +fail_tests()
> +{
> +	# multiple subflows
> +	reset_with_fail 2
> +	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
> +	ip netns exec $ns2 ./pm_nl_ctl limits 0 2
> +	ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 dev ns2eth3 flags subflow
> +	ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 dev ns2eth2 flags subflow
> +	run_tests $ns1 $ns2 10.0.1.1 0 0 "fail_2"
> +	chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
> +
> +	# single subflow
> +	reset_with_fail 1
> +	ip netns exec $ns1 ./pm_nl_ctl limits 0 2
> +	ip netns exec $ns2 ./pm_nl_ctl limits 0 2
> +	run_tests $ns1 $ns2 10.0.1.1 0 0 "fail_1"
> +	chk_join_nr "MP_FAIL test, single subflow" 0 0 0 1 1

With a single flow, are you not going to have an error because one side
didn't fully receive the expected file?

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