[PATCH mptcp-next v9 0/5] Clarify when options can be used

[PATCH mptcp-next v9 0/5] Clarify when options can be used

[Geliang Tang]

v9:
 - drop patch 1-3 in v8, they are accepted.
 - drop patch 6 in v8.
 - add a new patch "move the declarations of ssk and subflow" to avoid
   the compiling errors when moving the mp_fail label.
 - update patch "reduce branching when writing MP_FAIL option" as Mat
   suggested.
 - Change 'ADD+RM' from 'C' to 'P' in patch "clarify when options can
   be used".

Geliang Tang (3):
  mptcp: move the declarations of ssk and subflow
  mptcp: print out reset infos of MP_RST
  selftests: mptcp: add mp_fail testcases

Matthieu Baerts (2):
  mptcp: reduce branching when writing MP_FAIL option
  mptcp: clarify when options can be used

 net/mptcp/options.c                           |  64 +++++++---
 tools/testing/selftests/net/mptcp/config      |   5 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
 3 files changed, 152 insertions(+), 28 deletions(-)

-- 
2.31.1

[PATCH mptcp-next v9 1/5] mptcp: move the declarations of ssk and subflow

[Geliang Tang]

Move the declarations of ssk and subflow in MP_FAIL and MP_PRIO to the
beginning of the function mptcp_write_options().

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

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index bbf61dedb4b5..3230166d6f83 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1264,10 +1264,10 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
 void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			 struct mptcp_out_options *opts)
 {
-	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
-		const struct sock *ssk = (const struct sock *)tp;
-		struct mptcp_subflow_context *subflow;
+	const struct sock *ssk = (const struct sock *)tp;
+	struct mptcp_subflow_context *subflow;
 
+	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
 		subflow = mptcp_subflow_ctx(ssk);
 		subflow->send_mp_fail = 0;
 
@@ -1489,9 +1489,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 	}
 
 	if (OPTION_MPTCP_PRIO & opts->suboptions) {
-		const struct sock *ssk = (const struct sock *)tp;
-		struct mptcp_subflow_context *subflow;
-
 		subflow = mptcp_subflow_ctx(ssk);
 		subflow->send_mp_prio = 0;
 
-- 
2.31.1

[PATCH mptcp-next v9 2/5] mptcp: reduce branching when writing MP_FAIL option

[Geliang Tang]

From: Matthieu Baerts <matthieu.baerts@tessares.net>

MP_FAIL should be use in very rare cases, either when the TCP RST flag
is set -- with or without an MP_RST -- or with a DSS, see
mptcp_established_options().

Here, we do the same in mptcp_write_options().

Co-developed-by: Geliang Tang <geliang.tang@suse.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 3230166d6f83..491362d8dcda 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1267,17 +1267,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 	const struct sock *ssk = (const struct sock *)tp;
 	struct mptcp_subflow_context *subflow;
 
-	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
-		subflow = mptcp_subflow_ctx(ssk);
-		subflow->send_mp_fail = 0;
-
-		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
-				      TCPOLEN_MPTCP_FAIL,
-				      0, 0);
-		put_unaligned_be64(opts->fail_seq, ptr);
-		ptr += 2;
-	}
-
 	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
 	 * see mptcp_established_options*()
 	 */
@@ -1336,6 +1325,10 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			}
 			ptr += 1;
 		}
+
+		/* We might need to add MP_FAIL options in rare cases */
+		if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
+			goto mp_fail;
 	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
 		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
 
@@ -1476,6 +1469,21 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 		put_unaligned_be64(opts->rcvr_key, ptr);
 		ptr += 2;
 
+		if (OPTION_MPTCP_RST & opts->suboptions)
+			goto mp_rst;
+		return;
+	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
+mp_fail:
+		/* MP_FAIL is mutually exclusive with others except RST */
+		subflow = mptcp_subflow_ctx(ssk);
+		subflow->send_mp_fail = 0;
+
+		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
+				      TCPOLEN_MPTCP_FAIL,
+				      0, 0);
+		put_unaligned_be64(opts->fail_seq, ptr);
+		ptr += 2;
+
 		if (OPTION_MPTCP_RST & opts->suboptions)
 			goto mp_rst;
 		return;
-- 
2.31.1

[PATCH mptcp-next v9 3/5] mptcp: clarify when options can be used

[Geliang Tang]

From: Matthieu Baerts <matthieu.baerts@tessares.net>

RFC8684 doesn't seem to clearly specify which MPTCP options can be used
together.

Some options are mutually exclusive -- e.g. MP_CAPABLE and MP_JOIN --,
some can be used together -- e.g. DSS + MP_PRIO --, some can but we
prefer not to -- e.g. DSS + ADD_ADDR -- and some have to be used
together at some points -- e.g. MP_FAIL and DSS.

We need to clarify this as a base before allowing other modifications.

For example, does it make sense to send a RM_ADDR with an MPC or MPJ?
This remains open for possible future discussions.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 v9:
 - Change 'ADD+RM' from 'C' to 'P'.
---
 net/mptcp/options.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 491362d8dcda..bc2beeb6b5a0 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1267,8 +1267,27 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 	const struct sock *ssk = (const struct sock *)tp;
 	struct mptcp_subflow_context *subflow;
 
-	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
-	 * see mptcp_established_options*()
+	/* Which options can be used together?
+	 *
+	 * X: mutually exclusive
+	 * O: often used together
+	 * C: can be used together in some cases
+	 * P: could be used together but we prefer not to (optimisations)
+	 *
+	 *  Opt: | MPC  | MPJ  | DSS  | ADD  |  RM  | PRIO | FAIL |  FC  |
+	 * ------|------|------|------|------|------|------|------|------|
+	 *  MPC  |------|------|------|------|------|------|------|------|
+	 *  MPJ  |  X   |------|------|------|------|------|------|------|
+	 *  DSS  |  X   |  X   |------|------|------|------|------|------|
+	 *  ADD  |  X   |  X   |  P   |------|------|------|------|------|
+	 *  RM   |  C   |  C   |  C   |  P   |------|------|------|------|
+	 *  PRIO |  X   |  C   |  C   |  C   |  C   |------|------|------|
+	 *  FAIL |  X   |  X   |  C   |  X   |  X   |  X   |------|------|
+	 *  FC   |  X   |  X   |  X   |  X   |  X   |  X   |  X   |------|
+	 *  RST  |  X   |  X   |  X   |  X   |  X   |  X   |  O   |  O   |
+	 * ------|------|------|------|------|------|------|------|------|
+	 *
+	 * The same applies in mptcp_established_options() function.
 	 */
 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
 		struct mptcp_ext *mpext = &opts->ext_copy;
-- 
2.31.1

[PATCH mptcp-next v9 4/5] 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>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index bc2beeb6b5a0..0d0d2eb8c8ca 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 v9 5/5] 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>
---
 tools/testing/selftests/net/mptcp/config      |   5 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
 2 files changed, 107 insertions(+), 9 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 e48ce23d2386..535baa3c01e7 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -16,6 +16,7 @@ mptcp_connect=""
 capture=0
 checksum=0
 do_all_tests=1
+nr_fail=0
 
 TEST_COUNT=0
 
@@ -58,6 +59,8 @@ init()
 		fi
 	done
 
+	validate_checksum=$checksum
+
 	#  ns1              ns2
 	# ns1eth1    ns2eth1
 	# ns1eth2    ns2eth2
@@ -161,6 +164,45 @@ 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
+
+	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"
@@ -179,6 +221,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
@@ -233,6 +281,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()
 {
@@ -589,6 +652,8 @@ dump_stats()
 chk_csum_nr()
 {
 	local msg=${1:-""}
+	local csum_ns1=${2:-0}
+	local csum_ns2=${3:-0}
 	local count
 	local dump_stats
 
@@ -600,8 +665,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
@@ -610,8 +675,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
@@ -690,6 +755,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
 
@@ -726,10 +793,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
 }
 
@@ -1985,6 +2052,27 @@ userspace_tests()
 	chk_rm_nr 0 0
 }
 
+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 2
+	get_nr_fail 2
+	chk_join_nr "$nr_fail MP_FAIL, multiple subflows" 2 2 2 $nr_fail
+
+	# 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 2
+	get_nr_fail 1
+	chk_join_nr "$nr_fail MP_FAIL, single subflow" 0 0 0 $nr_fail $nr_fail
+}
+
 all_tests()
 {
 	subflows_tests
@@ -2003,6 +2091,7 @@ all_tests()
 	deny_join_id0_tests
 	fullmesh_tests
 	userspace_tests
+	fail_tests
 }
 
 usage()
@@ -2024,6 +2113,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 "  -h help"
@@ -2059,7 +2149,7 @@ if [ $do_all_tests -eq 1 ]; then
 	exit $ret
 fi
 
-while getopts 'fesltra64bpkdmuchCS' opt; do
+while getopts 'fesltra64bpkdmuchCSF' opt; do
 	case $opt in
 		f)
 			subflows_tests
@@ -2109,6 +2199,9 @@ while getopts 'fesltra64bpkdmuchCS' opt; do
 		u)
 			userspace_tests
 			;;
+		F)
+			fail_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.31.1

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

[Mat Martineau]

On Mon, 10 Jan 2022, 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>
> ---
> tools/testing/selftests/net/mptcp/config      |   5 +
> .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
> 2 files changed, 107 insertions(+), 9 deletions(-)
>

Does anyone else see the "leaked reference" errors when running these fail 
tests with the latest export branch? Looks like the reference tracker 
finds a tc-related error:

[  238.871372] leaked reference.
[  238.872393]  fl_change+0x2db/0x2520
[  238.873148]  tc_new_tfilter+0x6c4/0x11f0
[  238.873959]  rtnetlink_rcv_msg+0x49f/0x640
[  238.874798]  netlink_rcv_skb+0xc4/0x1f0
[  238.875570]  netlink_unicast+0x2d5/0x410
[  238.876364]  netlink_sendmsg+0x3b3/0x6c0
[  238.877155]  sock_sendmsg+0x91/0xa0
[  238.877890]  ____sys_sendmsg+0x3ad/0x3f0
[  238.878684]  ___sys_sendmsg+0xdb/0x140
[  238.879354]  __sys_sendmsg+0xae/0x120
[  238.879947]  do_syscall_64+0x3b/0x90
[  238.880683]  entry_SYSCALL_64_after_hwframe+0x44/0xae

It doesn't look like MPTCP is to blame, but I'm curious if others see the 
same.

Since this test till has the file mismatch error, I don't think it's ready 
for the export branch yet. But I think fixing this will involve two 
things:

1. (Likely) A fix to the code that flushes the received data from the 
subflow rx buffer, so corrupt data is dropped before it gets copied to the 
MPTCP-level buffer. Still trying to narrow this down, and it will likely 
involve a separate patch (maybe to squash to an existing patch in the 
export branch, maybe not).

2. A way to accept the expected bit flips in the "infinite fallback" case, 
where the checksum failure causes fallback and the data is not rejected. 
check_transfer just uses 'cmp' right now to make sure the temp files are 
identical. Another tool (maybe awk, maybe a small c program in our test 
directory) could compare bytes in each file allowing either identical 
bytes or inverted bytes:

// assuming files already mmap'd

for (i = 0; i < length; i++) {
     if ((file1[i] != file2[i]) && (file1[i] != ~file2[i]))
         return 1;
}

return 0;

For the case where inverted bytes are valid, use the alternate comparison 
tool.

- Mat

> 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 e48ce23d2386..535baa3c01e7 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -16,6 +16,7 @@ mptcp_connect=""
> capture=0
> checksum=0
> do_all_tests=1
> +nr_fail=0
>
> TEST_COUNT=0
>
> @@ -58,6 +59,8 @@ init()
> 		fi
> 	done
>
> +	validate_checksum=$checksum
> +
> 	#  ns1              ns2
> 	# ns1eth1    ns2eth1
> 	# ns1eth2    ns2eth2
> @@ -161,6 +164,45 @@ 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
> +
> +	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"
> @@ -179,6 +221,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
> @@ -233,6 +281,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()
> {
> @@ -589,6 +652,8 @@ dump_stats()
> chk_csum_nr()
> {
> 	local msg=${1:-""}
> +	local csum_ns1=${2:-0}
> +	local csum_ns2=${3:-0}
> 	local count
> 	local dump_stats
>
> @@ -600,8 +665,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
> @@ -610,8 +675,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
> @@ -690,6 +755,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
>
> @@ -726,10 +793,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
> }
>
> @@ -1985,6 +2052,27 @@ userspace_tests()
> 	chk_rm_nr 0 0
> }
>
> +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 2
> +	get_nr_fail 2
> +	chk_join_nr "$nr_fail MP_FAIL, multiple subflows" 2 2 2 $nr_fail
> +
> +	# 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 2
> +	get_nr_fail 1
> +	chk_join_nr "$nr_fail MP_FAIL, single subflow" 0 0 0 $nr_fail $nr_fail
> +}
> +
> all_tests()
> {
> 	subflows_tests
> @@ -2003,6 +2091,7 @@ all_tests()
> 	deny_join_id0_tests
> 	fullmesh_tests
> 	userspace_tests
> +	fail_tests
> }
>
> usage()
> @@ -2024,6 +2113,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 "  -h help"
> @@ -2059,7 +2149,7 @@ if [ $do_all_tests -eq 1 ]; then
> 	exit $ret
> fi
>
> -while getopts 'fesltra64bpkdmuchCS' opt; do
> +while getopts 'fesltra64bpkdmuchCSF' opt; do
> 	case $opt in
> 		f)
> 			subflows_tests
> @@ -2109,6 +2199,9 @@ while getopts 'fesltra64bpkdmuchCS' opt; do
> 		u)
> 			userspace_tests
> 			;;
> +		F)
> +			fail_tests
> +			;;
> 		c)
> 			;;
> 		C)
> -- 
> 2.31.1
>
>
>

--
Mat Martineau
Intel

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

[Matthieu Baerts]

Hi Mat,

On 13/01/2022 02:06, Mat Martineau wrote:
> On Mon, 10 Jan 2022, 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>
>> ---
>> tools/testing/selftests/net/mptcp/config      |   5 +
>> .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
>> 2 files changed, 107 insertions(+), 9 deletions(-)
>>
> 
> Does anyone else see the "leaked reference" errors when running these
> fail tests with the latest export branch? Looks like the reference
> tracker finds a tc-related error:
> 
> [  238.871372] leaked reference.
> [  238.872393]  fl_change+0x2db/0x2520
> [  238.873148]  tc_new_tfilter+0x6c4/0x11f0
> [  238.873959]  rtnetlink_rcv_msg+0x49f/0x640
> [  238.874798]  netlink_rcv_skb+0xc4/0x1f0
> [  238.875570]  netlink_unicast+0x2d5/0x410
> [  238.876364]  netlink_sendmsg+0x3b3/0x6c0
> [  238.877155]  sock_sendmsg+0x91/0xa0
> [  238.877890]  ____sys_sendmsg+0x3ad/0x3f0
> [  238.878684]  ___sys_sendmsg+0xdb/0x140
> [  238.879354]  __sys_sendmsg+0xae/0x120
> [  238.879947]  do_syscall_64+0x3b/0x90
> [  238.880683]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> It doesn't look like MPTCP is to blame, but I'm curious if others see
> the same.

The public CI didn't report that:

  https://api.cirrus-ci.com/v1/task/5851145250799616/logs/test.log

We can see a lot of TC warnings that still need to be fixed:

  tc action pedit offset 162 out of bounds

I guess we need to change the test to send data mainly in one direction
to avoid all these pure ACKs causing issues with the TC filter and
probably causing the corresponding test to fail.

For more details about the results from the CI:

===== 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/4725245343956992
  - Summary:
https://api.cirrus-ci.com/v1/artifact/task/4725245343956992/summary/summary.txt

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

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

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

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

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]

On Wed, 12 Jan 2022, Mat Martineau wrote:

> On Mon, 10 Jan 2022, 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>
>> ---
>> tools/testing/selftests/net/mptcp/config      |   5 +
>> .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
>> 2 files changed, 107 insertions(+), 9 deletions(-)
>> 
>
> Does anyone else see the "leaked reference" errors when running these fail 
> tests with the latest export branch? Looks like the reference tracker finds a 
> tc-related error:
>
> [  238.871372] leaked reference.
> [  238.872393]  fl_change+0x2db/0x2520
> [  238.873148]  tc_new_tfilter+0x6c4/0x11f0
> [  238.873959]  rtnetlink_rcv_msg+0x49f/0x640
> [  238.874798]  netlink_rcv_skb+0xc4/0x1f0
> [  238.875570]  netlink_unicast+0x2d5/0x410
> [  238.876364]  netlink_sendmsg+0x3b3/0x6c0
> [  238.877155]  sock_sendmsg+0x91/0xa0
> [  238.877890]  ____sys_sendmsg+0x3ad/0x3f0
> [  238.878684]  ___sys_sendmsg+0xdb/0x140
> [  238.879354]  __sys_sendmsg+0xae/0x120
> [  238.879947]  do_syscall_64+0x3b/0x90
> [  238.880683]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>
> It doesn't look like MPTCP is to blame, but I'm curious if others see the 
> same.

I've attached my kernel config as Davide requested in the meeting today.

I had applied this version of the "Clarify when options can be used" 
series to export/20220111T054942, which is based on net-next/master at 
fe8152b38d3a (still the current HEAD).

--
Mat Martineau
Intel

[-- Attachment #2: Type: application/x-gzip, Size: 23690 bytes --]

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

[Davide Caratti]

hello,

started looking at this, but the wild-guess is that the problem should
be fixed with [1]: I'll do a quick test on my VM now.

-- 
davide

[1] https://lore.kernel.org/netdev/20220110094750.236478-1-eric.dumazet@gmail.com/

On Thu, Jan 13, 2022 at 6:09 PM Mat Martineau
<mathew.j.martineau@linux.intel.com> wrote:
>
> On Wed, 12 Jan 2022, Mat Martineau wrote:
>
> > On Mon, 10 Jan 2022, 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>
> >> ---
> >> tools/testing/selftests/net/mptcp/config      |   5 +
> >> .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
> >> 2 files changed, 107 insertions(+), 9 deletions(-)
> >>
> >
> > Does anyone else see the "leaked reference" errors when running these fail
> > tests with the latest export branch? Looks like the reference tracker finds a
> > tc-related error:
> >
> > [  238.871372] leaked reference.
> > [  238.872393]  fl_change+0x2db/0x2520
> > [  238.873148]  tc_new_tfilter+0x6c4/0x11f0
> > [  238.873959]  rtnetlink_rcv_msg+0x49f/0x640
> > [  238.874798]  netlink_rcv_skb+0xc4/0x1f0
> > [  238.875570]  netlink_unicast+0x2d5/0x410
> > [  238.876364]  netlink_sendmsg+0x3b3/0x6c0
> > [  238.877155]  sock_sendmsg+0x91/0xa0
> > [  238.877890]  ____sys_sendmsg+0x3ad/0x3f0
> > [  238.878684]  ___sys_sendmsg+0xdb/0x140
> > [  238.879354]  __sys_sendmsg+0xae/0x120
> > [  238.879947]  do_syscall_64+0x3b/0x90
> > [  238.880683]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > It doesn't look like MPTCP is to blame, but I'm curious if others see the
> > same.
>
> I've attached my kernel config as Davide requested in the meeting today.
>
> I had applied this version of the "Clarify when options can be used"
> series to export/20220111T054942, which is based on net-next/master at
> fe8152b38d3a (still the current HEAD).
>
> --
> Mat Martineau
> Intel

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

[Matthieu Baerts]

Hi Davide,

On 18/01/2022 14:32, Davide Caratti wrote:
> hello,
> 
> started looking at this, but the wild-guess is that the problem should
> be fixed with [1]: I'll do a quick test on my VM now.
> 
> [1] https://lore.kernel.org/netdev/20220110094750.236478-1-eric.dumazet@gmail.com/

Good point, it might be linked to that.

This commit is not in net-next yet, hence not in our tree ("export"
branch). I can easily add it there if needed.

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

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

[Davide Caratti]

On Tue, Jan 18, 2022 at 3:15 PM Matthieu Baerts
<matthieu.baerts@tessares.net> wrote:
>
> Hi Davide,
>
> On 18/01/2022 14:32, Davide Caratti wrote:
> > hello,
> >
> > started looking at this, but the wild-guess is that the problem should
> > be fixed with [1]: I'll do a quick test on my VM now.
> >
> > [1] https://lore.kernel.org/netdev/20220110094750.236478-1-eric.dumazet@gmail.com/
>
> Good point, it might be linked to that.

hello,

confirmed, the problem happens when kernels are built with
CONFIG_NET_NS_REFCNT_TRACKER=y and the problem is fixed by the
above-mentioned patch from Eric. It can be easily verified also with
tdc kselftest:

# pushd tools/testing/selftests/tc-testing;  ./tdc.py -f
tc-tests/filters/tests.json ; popd

thanks!
-- 
davide

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

[Mat Martineau]

On Tue, 18 Jan 2022, Davide Caratti wrote:

> On Tue, Jan 18, 2022 at 3:15 PM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> Hi Davide,
>>
>> On 18/01/2022 14:32, Davide Caratti wrote:
>>> hello,
>>>
>>> started looking at this, but the wild-guess is that the problem should
>>> be fixed with [1]: I'll do a quick test on my VM now.
>>>
>>> [1] https://lore.kernel.org/netdev/20220110094750.236478-1-eric.dumazet@gmail.com/
>>
>> Good point, it might be linked to that.
>
> hello,
>
> confirmed, the problem happens when kernels are built with
> CONFIG_NET_NS_REFCNT_TRACKER=y and the problem is fixed by the
> above-mentioned patch from Eric. It can be easily verified also with
> tdc kselftest:
>
> # pushd tools/testing/selftests/tc-testing;  ./tdc.py -f
> tc-tests/filters/tests.json ; popd
>

Thanks for tracking this down and confirming, Davide!

--
Mat Martineau
Intel

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

[Matthieu Baerts]

Hi Davide,

On 18/01/2022 17:48, Davide Caratti wrote:
> On Tue, Jan 18, 2022 at 3:15 PM Matthieu Baerts
> <matthieu.baerts@tessares.net> wrote:
>>
>> Hi Davide,
>>
>> On 18/01/2022 14:32, Davide Caratti wrote:
>>> hello,
>>>
>>> started looking at this, but the wild-guess is that the problem should
>>> be fixed with [1]: I'll do a quick test on my VM now.
>>>
>>> [1] https://lore.kernel.org/netdev/20220110094750.236478-1-eric.dumazet@gmail.com/
>>
>> Good point, it might be linked to that.
> 
> hello,
> 
> confirmed, the problem happens when kernels are built with
> CONFIG_NET_NS_REFCNT_TRACKER=y and the problem is fixed by the
> above-mentioned patch from Eric.

Thank you for having checked!

I just added this patch in our tree:

- 943a4ed011fc: net: sched: do not allocate a tracker in tcf_exts_init()
- Results: a99ed2058930..fbaefb5453f9

Builds and tests are now in progress:

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

I guess I should better enable this kconfig option for the debug builds, no?

Cheers,
Matt


-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v9 0/5] Clarify when options can be used

[Mat Martineau]

On Mon, 10 Jan 2022, Geliang Tang wrote:

> v9:
> - drop patch 1-3 in v8, they are accepted.
> - drop patch 6 in v8.
> - add a new patch "move the declarations of ssk and subflow" to avoid
>   the compiling errors when moving the mp_fail label.
> - update patch "reduce branching when writing MP_FAIL option" as Mat
>   suggested.
> - Change 'ADD+RM' from 'C' to 'P' in patch "clarify when options can
>   be used".

Hi Geliang -

I think patches 1-4 are ok for the export branch:

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


There are still some fixes we'll need to squash (or get from upstream) 
before updating the unit tests and upstreaming, see patch 5 comments for 
details.

>
> Geliang Tang (3):
>  mptcp: move the declarations of ssk and subflow
>  mptcp: print out reset infos of MP_RST
>  selftests: mptcp: add mp_fail testcases
>
> Matthieu Baerts (2):
>  mptcp: reduce branching when writing MP_FAIL option
>  mptcp: clarify when options can be used
>
> net/mptcp/options.c                           |  64 +++++++---
> tools/testing/selftests/net/mptcp/config      |   5 +
> .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
> 3 files changed, 152 insertions(+), 28 deletions(-)
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v9 0/5] Clarify when options can be used

[Matthieu Baerts]

Hi Geliang, Mat,

On 13/01/2022 00:33, Mat Martineau wrote:
> On Mon, 10 Jan 2022, Geliang Tang wrote:
> 
>> v9:
>> - drop patch 1-3 in v8, they are accepted.
>> - drop patch 6 in v8.
>> - add a new patch "move the declarations of ssk and subflow" to avoid
>>   the compiling errors when moving the mp_fail label.
>> - update patch "reduce branching when writing MP_FAIL option" as Mat
>>   suggested.
>> - Change 'ADD+RM' from 'C' to 'P' in patch "clarify when options can
>>   be used".
> 
> Hi Geliang -
> 
> I think patches 1-4 are ok for the export branch:
> 
> Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

Thank you for having sent this new version and for the reviews!

Now in our tree (feat. for net-next) with Mat's RvB tag, without my SoB
on patch 4/5 and without patch 5/5:

- ac07cdb70be9: mptcp: move the declarations of ssk and subflow
- 77ef153f0748: mptcp: reduce branching when writing MP_FAIL option
- be77cda2def6: mptcp: clarify when options can be used
- d185331aff82: mptcp: print out reset infos of MP_RST
- Results: 459ae9092f30..26c341404c60

Builds and tests are now in progress:

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

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