[PATCH mptcp-next v8 0/8] The infinite mapping support

[PATCH mptcp-next v8 0/8] The infinite mapping support

[Geliang Tang]

v8:
 - Patches 1-6 are unchanged, only updated the selftests scripts.
 - The patch (Squash to "mptcp: infinite mapping receiving" for v7) is
dropped too. Since this series only implemented MP_FAIL in one direction.
The TODO items, "MP_FAIL echo" and "MP_FAIL retrans", will implement later
as new patches.

v7:
 - drop the patch "mptcp: add last_ack_dss_start in the msk" in v6
 - set allow_infinite_fallback to false in __mptcp_subflow_connect and
   mptcp_finish_join.

v6:
 - use allow_infinite_fallback instead of last_retrans_seq.
 - rename mptcp_is_data_contiguous to mptcp_allow_infinite_fallback.
 - rename last_fully_acked_dss_start_seq to last_ack_dss_start.

v5:
 - move last_retrans_seq from msk to mptcp_subflow_context

v4:
 - update patch 1 and patch 2

v3:
 - drop MPTCP_INFINITE_DONE flag
 - drop MAPPING_INFINITE
 - add mptcp_is_data_contiguous helper
 - add the fallback check
 - The u32 target testcase has not been completed yet.

v2:
 - add MPTCP_INFINITE_DONE flag
 - add MAPPING_INFINITE mapping status
 - add start_seq in the msk

v1:
 - add noncontiguous flag
 - add the mibs check
 - tag: export/20210904T080009

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/216

Geliang Tang (8):
  mptcp: don't send RST for single subflow
  mptcp: add the fallback check
  mptcp: track and update contiguous data status
  mptcp: infinite mapping sending
  mptcp: infinite mapping receiving
  mptcp: add mib for infinite map sending
  selftests: mptcp: add infinite map mibs check
  selftests: mptcp: add mp_fail testcases

 include/net/mptcp.h                           |   3 +-
 net/mptcp/mib.c                               |   1 +
 net/mptcp/mib.h                               |   1 +
 net/mptcp/options.c                           |   2 +-
 net/mptcp/pm.c                                |   6 +
 net/mptcp/protocol.c                          |  22 ++++
 net/mptcp/protocol.h                          |  13 ++
 net/mptcp/subflow.c                           |  56 +++++----
 tools/testing/selftests/net/mptcp/config      |   5 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
 10 files changed, 186 insertions(+), 34 deletions(-)

-- 
2.26.2

[PATCH mptcp-next v8 1/8] mptcp: don't send RST for single subflow

[Geliang Tang]

When a bad checksum is detected and a single subflow is in use, don't
send RST + MP_FAIL, send data_ack + MP_FAIL instead.

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

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6172f380dfb7..92b45a7c997e 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1167,14 +1167,14 @@ static bool subflow_check_data_avail(struct sock *ssk)
 	/* RFC 8684 section 3.7. */
 	if (subflow->send_mp_fail) {
 		if (mptcp_has_another_subflow(ssk)) {
+			ssk->sk_err = EBADMSG;
+			tcp_set_state(ssk, TCP_CLOSE);
+			subflow->reset_transient = 0;
+			subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
+			tcp_send_active_reset(ssk, GFP_ATOMIC);
 			while ((skb = skb_peek(&ssk->sk_receive_queue)))
 				sk_eat_skb(ssk, skb);
 		}
-		ssk->sk_err = EBADMSG;
-		tcp_set_state(ssk, TCP_CLOSE);
-		subflow->reset_transient = 0;
-		subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
-		tcp_send_active_reset(ssk, GFP_ATOMIC);
 		WRITE_ONCE(subflow->data_avail, 0);
 		return true;
 	}
-- 
2.26.2

[PATCH mptcp-next v8 2/8] mptcp: add the fallback check

[Geliang Tang]

This patch added the fallback check in subflow_check_data_avail(). Only
do the fallback when the msk isn't fallen back yet.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/subflow.c | 45 ++++++++++++++++++++++++---------------------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 92b45a7c997e..87a9ffebcc42 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1164,35 +1164,38 @@ static bool subflow_check_data_avail(struct sock *ssk)
 	return false;
 
 fallback:
-	/* RFC 8684 section 3.7. */
-	if (subflow->send_mp_fail) {
-		if (mptcp_has_another_subflow(ssk)) {
+	if (!__mptcp_check_fallback(msk)) {
+		/* RFC 8684 section 3.7. */
+		if (subflow->send_mp_fail) {
+			if (mptcp_has_another_subflow(ssk)) {
+				ssk->sk_err = EBADMSG;
+				tcp_set_state(ssk, TCP_CLOSE);
+				subflow->reset_transient = 0;
+				subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
+				tcp_send_active_reset(ssk, GFP_ATOMIC);
+				while ((skb = skb_peek(&ssk->sk_receive_queue)))
+					sk_eat_skb(ssk, skb);
+			}
+			WRITE_ONCE(subflow->data_avail, 0);
+			return true;
+		}
+
+		if (subflow->mp_join || subflow->fully_established) {
+			/* fatal protocol error, close the socket.
+			 * subflow_error_report() will introduce the appropriate barriers
+			 */
 			ssk->sk_err = EBADMSG;
 			tcp_set_state(ssk, TCP_CLOSE);
 			subflow->reset_transient = 0;
-			subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
+			subflow->reset_reason = MPTCP_RST_EMPTCP;
 			tcp_send_active_reset(ssk, GFP_ATOMIC);
-			while ((skb = skb_peek(&ssk->sk_receive_queue)))
-				sk_eat_skb(ssk, skb);
+			WRITE_ONCE(subflow->data_avail, 0);
+			return false;
 		}
-		WRITE_ONCE(subflow->data_avail, 0);
-		return true;
-	}
 
-	if (subflow->mp_join || subflow->fully_established) {
-		/* fatal protocol error, close the socket.
-		 * subflow_error_report() will introduce the appropriate barriers
-		 */
-		ssk->sk_err = EBADMSG;
-		tcp_set_state(ssk, TCP_CLOSE);
-		subflow->reset_transient = 0;
-		subflow->reset_reason = MPTCP_RST_EMPTCP;
-		tcp_send_active_reset(ssk, GFP_ATOMIC);
-		WRITE_ONCE(subflow->data_avail, 0);
-		return false;
+		__mptcp_do_fallback(msk);
 	}
 
-	__mptcp_do_fallback(msk);
 	skb = skb_peek(&ssk->sk_receive_queue);
 	subflow->map_valid = 1;
 	subflow->map_seq = READ_ONCE(msk->ack_seq);
-- 
2.26.2

[PATCH mptcp-next v8 3/8] mptcp: track and update contiguous data status

[Geliang Tang]

This patch added a new member allow_infinite_fallback in mptcp_sock,
which gets initialized to 'true' when the connection begins and is set
to 'false' on any retransmit or successful MP_JOIN. Only do infinite
mapping fallback if there is a single subflow AND there have been no
retransmissions AND there have never been any MP_JOINs.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 3 +++
 net/mptcp/protocol.h | 1 +
 net/mptcp/subflow.c  | 4 +++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7803b0dbb1be..f3163647c501 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2387,6 +2387,7 @@ static void __mptcp_retrans(struct sock *sk)
 		dfrag->already_sent = max(dfrag->already_sent, info.sent);
 		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
 			 info.size_goal);
+		WRITE_ONCE(msk->allow_infinite_fallback, false);
 	}
 
 	release_sock(ssk);
@@ -2465,6 +2466,7 @@ static int __mptcp_init_sock(struct sock *sk)
 	msk->first = NULL;
 	inet_csk(sk)->icsk_sync_mss = mptcp_sync_mss;
 	WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
+	WRITE_ONCE(msk->allow_infinite_fallback, true);
 	msk->recovery = false;
 
 	mptcp_pm_data_init(msk);
@@ -3133,6 +3135,7 @@ bool mptcp_finish_join(struct sock *ssk)
 	if (parent_sock && !ssk->sk_socket)
 		mptcp_sock_graft(ssk, parent_sock);
 	subflow->map_seq = READ_ONCE(msk->ack_seq);
+	WRITE_ONCE(msk->allow_infinite_fallback, false);
 out:
 	mptcp_event(MPTCP_EVENT_SUB_ESTABLISHED, msk, ssk, GFP_ATOMIC);
 	return true;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 67a61ac48b20..4b9fe56bd572 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -249,6 +249,7 @@ struct mptcp_sock {
 	bool		rcv_fastclose;
 	bool		use_64bit_ack; /* Set when we received a 64-bit DSN */
 	bool		csum_enabled;
+	bool		allow_infinite_fallback;
 	spinlock_t	join_list_lock;
 	struct work_struct work;
 	struct sk_buff  *ooo_last_skb;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 87a9ffebcc42..93bc298bd41d 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1167,7 +1167,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
 	if (!__mptcp_check_fallback(msk)) {
 		/* RFC 8684 section 3.7. */
 		if (subflow->send_mp_fail) {
-			if (mptcp_has_another_subflow(ssk)) {
+			if (mptcp_has_another_subflow(ssk) ||
+			    !READ_ONCE(msk->allow_infinite_fallback)) {
 				ssk->sk_err = EBADMSG;
 				tcp_set_state(ssk, TCP_CLOSE);
 				subflow->reset_transient = 0;
@@ -1452,6 +1453,7 @@ int __mptcp_subflow_connect(struct sock *sk, const struct mptcp_addr_info *loc,
 	/* discard the subflow socket */
 	mptcp_sock_graft(ssk, sk->sk_socket);
 	iput(SOCK_INODE(sf));
+	WRITE_ONCE(msk->allow_infinite_fallback, false);
 	return err;
 
 failed_unlink:
-- 
2.26.2

[PATCH mptcp-next v8 4/8] mptcp: infinite mapping sending

[Geliang Tang]

This patch added the infinite mapping sending logic.

Added a new flag send_infinite_map in struct mptcp_subflow_context. Set
it true when a single contiguous subflow is in use and the
allow_infinite_fallback flag is true in mptcp_pm_mp_fail_received().

In mptcp_sendmsg_frag(), if this flag is true, call the new function
mptcp_update_infinite_map() to set the infinite mapping.

Added a new flag infinite_map in struct mptcp_ext, set it true in
mptcp_update_infinite_map(), and check this flag in a new helper
mptcp_check_infinite_map().

In mptcp_update_infinite_map(), set data_len and csum to 0, and clear
the send_infinite_map flag, then do fallback.

In mptcp_established_options(), use the helper mptcp_check_infinite_map()
to let the infinite mapping DSS can be sent out in the fallback mode.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/net/mptcp.h  |  3 ++-
 net/mptcp/options.c  |  2 +-
 net/mptcp/pm.c       |  6 ++++++
 net/mptcp/protocol.c | 18 ++++++++++++++++++
 net/mptcp/protocol.h | 12 ++++++++++++
 5 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index a925349b4b89..04f53352a1c9 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -35,7 +35,8 @@ struct mptcp_ext {
 			frozen:1,
 			reset_transient:1;
 	u8		reset_reason:4,
-			csum_reqd:1;
+			csum_reqd:1,
+			infinite_map:1;
 };
 
 #define MPTCP_RM_IDS_MAX	8
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 7c3420afb1a0..932e19645910 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -816,7 +816,7 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 
 	opts->suboptions = 0;
 
-	if (unlikely(__mptcp_check_fallback(msk)))
+	if (unlikely(__mptcp_check_fallback(msk) && !mptcp_check_infinite_map(skb)))
 		return false;
 
 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 6ab386ff3294..86b38a830b4c 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -251,7 +251,13 @@ void mptcp_pm_mp_prio_received(struct sock *sk, u8 bkup)
 
 void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
 {
+	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
+	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+
 	pr_debug("fail_seq=%llu", fail_seq);
+
+	if (!mptcp_has_another_subflow(sk) && READ_ONCE(msk->allow_infinite_fallback))
+		subflow->send_infinite_map = 1;
 }
 
 /* path manager helpers */
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f3163647c501..70f2e0e212fa 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1226,6 +1226,22 @@ static void mptcp_update_data_checksum(struct sk_buff *skb, int added)
 	mpext->csum = csum_fold(csum_block_add(csum, skb_checksum(skb, offset, added, 0), offset));
 }
 
+static void mptcp_update_infinite_map(struct mptcp_sock *msk,
+				      struct sock *ssk,
+				      struct mptcp_ext *mpext)
+{
+	if (!mpext)
+		return;
+
+	mpext->infinite_map = 1;
+	mpext->data_len = 0;
+	mpext->csum = 0;
+
+	mptcp_subflow_ctx(ssk)->send_infinite_map = 0;
+	pr_fallback(msk);
+	__mptcp_do_fallback(msk);
+}
+
 static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 			      struct mptcp_data_frag *dfrag,
 			      struct mptcp_sendmsg_info *info)
@@ -1358,6 +1374,8 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
 out:
 	if (READ_ONCE(msk->csum_enabled))
 		mptcp_update_data_checksum(skb, copy);
+	if (mptcp_subflow_ctx(ssk)->send_infinite_map)
+		mptcp_update_infinite_map(msk, ssk, mpext);
 	mptcp_subflow_ctx(ssk)->rel_write_seq += copy;
 	return copy;
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 4b9fe56bd572..906c65fca04f 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -419,6 +419,7 @@ struct mptcp_subflow_context {
 		backup : 1,
 		send_mp_prio : 1,
 		send_mp_fail : 1,
+		send_infinite_map : 1,
 		rx_eof : 1,
 		can_ack : 1,        /* only after processing the remote a key */
 		disposable : 1,	    /* ctx can be free at ulp release time */
@@ -852,6 +853,17 @@ static inline void mptcp_do_fallback(struct sock *sk)
 
 #define pr_fallback(a) pr_debug("%s:fallback to TCP (msk=%p)", __func__, a)
 
+static inline bool mptcp_check_infinite_map(struct sk_buff *skb)
+{
+	struct mptcp_ext *mpext;
+
+	mpext = skb ? mptcp_get_ext(skb) : NULL;
+	if (mpext && mpext->infinite_map)
+		return true;
+
+	return false;
+}
+
 static inline bool subflow_simultaneous_connect(struct sock *sk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
-- 
2.26.2

[PATCH mptcp-next v8 5/8] mptcp: infinite mapping receiving

[Geliang Tang]

This patch added the infinite mapping receiving logic. When the infinite
mapping is received, set the map_data_len of the subflow to 0.

In subflow_check_data_avail(), only reset the subflow when the map_data_len
of the subflow is non-zero.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/subflow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 93bc298bd41d..9e54122f18f4 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -968,6 +968,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk,
 	data_len = mpext->data_len;
 	if (data_len == 0) {
 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
+		subflow->map_data_len = 0;
 		return MAPPING_INVALID;
 	}
 
@@ -1181,7 +1182,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
 			return true;
 		}
 
-		if (subflow->mp_join || subflow->fully_established) {
+		if ((subflow->mp_join || subflow->fully_established) && subflow->map_data_len) {
 			/* fatal protocol error, close the socket.
 			 * subflow_error_report() will introduce the appropriate barriers
 			 */
-- 
2.26.2

[PATCH mptcp-next v8 6/8] mptcp: add mib for infinite map sending

[Geliang Tang]

This patch added a new mib named MPTCP_MIB_INFINITEMAPTX, increase it
when a infinite mapping has been sent out.

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

diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c
index 3240b72271a7..c12251cb0d44 100644
--- a/net/mptcp/mib.c
+++ b/net/mptcp/mib.c
@@ -24,6 +24,7 @@ static const struct snmp_mib mptcp_snmp_list[] = {
 	SNMP_MIB_ITEM("MPJoinAckRx", MPTCP_MIB_JOINACKRX),
 	SNMP_MIB_ITEM("MPJoinAckHMacFailure", MPTCP_MIB_JOINACKMAC),
 	SNMP_MIB_ITEM("DSSNotMatching", MPTCP_MIB_DSSNOMATCH),
+	SNMP_MIB_ITEM("InfiniteMapTx", MPTCP_MIB_INFINITEMAPTX),
 	SNMP_MIB_ITEM("InfiniteMapRx", MPTCP_MIB_INFINITEMAPRX),
 	SNMP_MIB_ITEM("DSSNoMatchTCP", MPTCP_MIB_DSSTCPMISMATCH),
 	SNMP_MIB_ITEM("DataCsumErr", MPTCP_MIB_DATACSUMERR),
diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h
index ecd3d8b117e0..7901f1338d15 100644
--- a/net/mptcp/mib.h
+++ b/net/mptcp/mib.h
@@ -17,6 +17,7 @@ enum linux_mptcp_mib_field {
 	MPTCP_MIB_JOINACKRX,		/* Received an ACK + MP_JOIN */
 	MPTCP_MIB_JOINACKMAC,		/* HMAC was wrong on ACK + MP_JOIN */
 	MPTCP_MIB_DSSNOMATCH,		/* Received a new mapping that did not match the previous one */
+	MPTCP_MIB_INFINITEMAPTX,	/* Sent an infinite mapping */
 	MPTCP_MIB_INFINITEMAPRX,	/* Received an infinite mapping */
 	MPTCP_MIB_DSSTCPMISMATCH,	/* DSS-mapping did not map with TCP's sequence numbers */
 	MPTCP_MIB_DATACSUMERR,		/* The data checksum fail */
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 70f2e0e212fa..2283efda1bc9 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1237,6 +1237,7 @@ static void mptcp_update_infinite_map(struct mptcp_sock *msk,
 	mpext->data_len = 0;
 	mpext->csum = 0;
 
+	MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPTX);
 	mptcp_subflow_ctx(ssk)->send_infinite_map = 0;
 	pr_fallback(msk);
 	__mptcp_do_fallback(msk);
-- 
2.26.2

[PATCH mptcp-next v8 7/8] selftests: mptcp: add infinite map mibs check

[Geliang Tang]

This patch added a function chk_infi_nr() to check the mibs for the
infinite mapping.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 7ef639a9d4a6..2684ef9c0d42 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -615,6 +615,43 @@ chk_fail_nr()
 	fi
 }
 
+chk_infi_nr()
+{
+	local mp_infi_nr_tx=$1
+	local mp_infi_nr_rx=$2
+	local count
+	local dump_stats
+
+	printf "%-39s %s" " " "itx"
+	count=`ip netns exec $ns2 nstat -as | grep InfiniteMapTx | awk '{print $2}'`
+	[ -z "$count" ] && count=0
+	if [ "$count" != "$mp_infi_nr_tx" ]; then
+		echo "[fail] got $count infinite map[s] TX expected $mp_infi_nr_tx"
+		ret=1
+		dump_stats=1
+	else
+		echo -n "[ ok ]"
+	fi
+
+	echo -n " - irx   "
+	count=`ip netns exec $ns1 nstat -as | grep InfiniteMapRx | awk '{print $2}'`
+	[ -z "$count" ] && count=0
+	if [ "$count" != "$mp_infi_nr_rx" ]; then
+		echo "[fail] got $count infinite map[s] RX expected $mp_infi_nr_rx"
+		ret=1
+		dump_stats=1
+	else
+		echo "[ ok ]"
+	fi
+
+	if [ "${dump_stats}" = 1 ]; then
+		echo Server ns stats
+		ip netns exec $ns1 nstat -as | grep MPTcp
+		echo Client ns stats
+		ip netns exec $ns2 nstat -as | grep MPTcp
+	fi
+}
+
 chk_join_nr()
 {
 	local msg="$1"
@@ -665,6 +702,7 @@ chk_join_nr()
 	if [ $checksum -eq 1 ]; then
 		chk_csum_nr
 		chk_fail_nr 0 0
+		chk_infi_nr 0 0
 	fi
 }
 
-- 
2.26.2

[PATCH mptcp-next v8 8/8] 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>
Suggested-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 | 75 +++++++++++++++++--
 2 files changed, 72 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
index 0faaccd21447..f522288b2204 100644
--- a/tools/testing/selftests/net/mptcp/config
+++ b/tools/testing/selftests/net/mptcp/config
@@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
 CONFIG_NETFILTER_XT_MATCH_BPF=m
 CONFIG_NF_TABLES_IPV4=y
 CONFIG_NF_TABLES_IPV6=y
+CONFIG_NET_ACT_CSUM=m
+CONFIG_NET_ACT_PEDIT=m
+CONFIG_NET_CLS_ACT=m
+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..d33cb5ce0ff3 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -178,6 +178,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 +238,28 @@ link_failure()
 	done
 }
 
+checksum_failure()
+{
+	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 \
+		action pedit munge offset 148 u32 invert \
+		pipe csum tcp \
+		index 100
+
+	while true; do
+		local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
+				jq '.[1].actions[0].stats.packets')
+		if [ $pkt -gt 0 ]; then
+			tc -n $ns2 qdisc del dev ns2eth$i clsact
+			break
+		fi
+	done
+}
+
 # $1: IP address
 is_v6()
 {
@@ -371,6 +399,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
+		checksum_failure ${addr_nr_ns2:5}
+		addr_nr_ns2=0
 	fi
 
 	if [ $addr_nr_ns2 -gt 0 ]; then
@@ -542,6 +573,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 +586,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 +596,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 +691,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 +735,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 +1872,25 @@ fullmesh_tests()
 	chk_add_nr 1 1
 }
 
+fail_tests()
+{
+	# 1 subflow
+	reset
+	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 0 "fail_1" fast
+	chk_join_nr "MP_FAIL test, 1 subflow" 0 0 0 1 1
+
+	# multiple subflows
+	reset
+	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 0 "fail_2" fast
+	chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
+}
+
 all_tests()
 {
 	subflows_tests
@@ -1853,6 +1907,7 @@ all_tests()
 	checksum_tests
 	deny_join_id0_tests
 	fullmesh_tests
+	fail_tests
 }
 
 usage()
@@ -1872,6 +1927,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 +1963,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 +2007,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
 		m)
 			fullmesh_tests
 			;;
+		F)
+			fail_tests
+			;;
 		c)
 			;;
 		C)
-- 
2.26.2

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

[Matthieu Baerts]

Hi Geliang,

Thank you for the new version.

On 29/10/2021 06:40, 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>
> Suggested-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 | 75 +++++++++++++++++--
>  2 files changed, 72 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index 0faaccd21447..f522288b2204 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
>  CONFIG_NETFILTER_XT_MATCH_BPF=m
>  CONFIG_NF_TABLES_IPV4=y
>  CONFIG_NF_TABLES_IPV6=y
> +CONFIG_NET_ACT_CSUM=m
> +CONFIG_NET_ACT_PEDIT=m
> +CONFIG_NET_CLS_ACT=m
> +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..d33cb5ce0ff3 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -178,6 +178,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 +238,28 @@ link_failure()
>  	done
>  }
>  
> +checksum_failure()
> +{
> +	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 \
> +		action pedit munge offset 148 u32 invert \
> +		pipe csum tcp \
> +		index 100
> +
> +	while true; do
> +		local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
> +				jq '.[1].actions[0].stats.packets')
> +		if [ $pkt -gt 0 ]; then
The CI [1] is complaining about that line:

[09:34:44.063] # RTNETLINK answers: Operation not supported
[09:34:44.107] # ./mptcp_join.sh: line 256: [: null: integer expression
expected
[09:34:44.267] # RTNETLINK answers: Operation not supported
[09:34:44.332] # ./mptcp_join.sh: line 256: [: null: integer expression
expected
(...)

This is displayed in a loop as it is a "while true".

But just before the first one, we had this:

[09:34:43.859] # Error: Cannot find ingress queue for specified device.
[09:34:43.955] # Error: Parent Qdisc doesn't exists.
[09:34:43.959] # We have an error talking to the kernel

I guess the previous "tc" command failed and I also guess you don't have
that on your side, right?

Maybe a missing kconfig?

I guess it is "safer" to abort if one of the tc command fails.

Cheers,
Matt

[1] https://cirrus-ci.com/task/6502115922149376
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

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

[Geliang Tang]

Hi Matt,

On Fri, Oct 29, 2021 at 12:02:47PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for the new version.
> 
> On 29/10/2021 06:40, 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>
> > Suggested-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 | 75 +++++++++++++++++--
> >  2 files changed, 72 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> > index 0faaccd21447..f522288b2204 100644
> > --- a/tools/testing/selftests/net/mptcp/config
> > +++ b/tools/testing/selftests/net/mptcp/config
> > @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
> >  CONFIG_NETFILTER_XT_MATCH_BPF=m
> >  CONFIG_NF_TABLES_IPV4=y
> >  CONFIG_NF_TABLES_IPV6=y
> > +CONFIG_NET_ACT_CSUM=m
> > +CONFIG_NET_ACT_PEDIT=m
> > +CONFIG_NET_CLS_ACT=m
> > +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..d33cb5ce0ff3 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -178,6 +178,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 +238,28 @@ link_failure()
> >  	done
> >  }
> >  
> > +checksum_failure()
> > +{
> > +	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 \
> > +		action pedit munge offset 148 u32 invert \
> > +		pipe csum tcp \
> > +		index 100
> > +
> > +	while true; do
> > +		local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
> > +				jq '.[1].actions[0].stats.packets')
> > +		if [ $pkt -gt 0 ]; then
> The CI [1] is complaining about that line:
> 
> [09:34:44.063] # RTNETLINK answers: Operation not supported
> [09:34:44.107] # ./mptcp_join.sh: line 256: [: null: integer expression
> expected
> [09:34:44.267] # RTNETLINK answers: Operation not supported
> [09:34:44.332] # ./mptcp_join.sh: line 256: [: null: integer expression
> expected
> (...)
> 
> This is displayed in a loop as it is a "while true".
> 
> But just before the first one, we had this:
> 
> [09:34:43.859] # Error: Cannot find ingress queue for specified device.
> [09:34:43.955] # Error: Parent Qdisc doesn't exists.
> [09:34:43.959] # We have an error talking to the kernel
> 
> I guess the previous "tc" command failed and I also guess you don't have
> that on your side, right?

Yes, it works on my side.

> 
> Maybe a missing kconfig?

I think so. How can I get the kconfig of the CI?

> 
> I guess it is "safer" to abort if one of the tc command fails.

I just sent a squash-to patch to fix this.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> 
> [1] https://cirrus-ci.com/task/6502115922149376
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

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

[Paolo Abeni]

On Fri, 2021-10-29 at 21:21 +0800, Geliang Tang wrote:
> Hi Matt,
> 
> On Fri, Oct 29, 2021 at 12:02:47PM +0200, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > Thank you for the new version.
> > 
> > On 29/10/2021 06:40, 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>
> > > Suggested-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 | 75 +++++++++++++++++--
> > >  2 files changed, 72 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> > > index 0faaccd21447..f522288b2204 100644
> > > --- a/tools/testing/selftests/net/mptcp/config
> > > +++ b/tools/testing/selftests/net/mptcp/config
> > > @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
> > >  CONFIG_NETFILTER_XT_MATCH_BPF=m
> > >  CONFIG_NF_TABLES_IPV4=y
> > >  CONFIG_NF_TABLES_IPV6=y
> > > +CONFIG_NET_ACT_CSUM=m
> > > +CONFIG_NET_ACT_PEDIT=m
> > > +CONFIG_NET_CLS_ACT=m
> > > +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..d33cb5ce0ff3 100755
> > > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > > @@ -178,6 +178,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 +238,28 @@ link_failure()
> > >  	done
> > >  }
> > >  
> > > +checksum_failure()
> > > +{
> > > +	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 \
> > > +		action pedit munge offset 148 u32 invert \
> > > +		pipe csum tcp \
> > > +		index 100
> > > +
> > > +	while true; do
> > > +		local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
> > > +				jq '.[1].actions[0].stats.packets')
> > > +		if [ $pkt -gt 0 ]; then
> > The CI [1] is complaining about that line:
> > 
> > [09:34:44.063] # RTNETLINK answers: Operation not supported
> > [09:34:44.107] # ./mptcp_join.sh: line 256: [: null: integer expression
> > expected
> > [09:34:44.267] # RTNETLINK answers: Operation not supported
> > [09:34:44.332] # ./mptcp_join.sh: line 256: [: null: integer expression
> > expected
> > (...)
> > 
> > This is displayed in a loop as it is a "while true".
> > 
> > But just before the first one, we had this:
> > 
> > [09:34:43.859] # Error: Cannot find ingress queue for specified device.
> > [09:34:43.955] # Error: Parent Qdisc doesn't exists.
> > [09:34:43.959] # We have an error talking to the kernel
> > 
> > I guess the previous "tc" command failed and I also guess you don't have
> > that on your side, right?
> 
> Yes, it works on my side.
> 
> > Maybe a missing kconfig?
> 
> I think so. How can I get the kconfig of the CI?

The need kernel config flags are correctly specified into:

tools/testing/selftests/net/mptcp/config

In this case it looks like the CI is missing/not applying:

CONFIG_NET_SCH_INGRESS=y

@Mat, does the CI take properly in account the self-tests config?

I guess:

make kselftest-merge

would fix, but could also be overkill (pulling a lot of config for
other, non mptcp self-tests)

Paolo

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

[Matthieu Baerts]

Hi Paolo, Geliang,

On 29/10/2021 16:43, Paolo Abeni wrote:
> On Fri, 2021-10-29 at 21:21 +0800, Geliang Tang wrote:
>> Hi Matt,
>>
>> On Fri, Oct 29, 2021 at 12:02:47PM +0200, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> Thank you for the new version.
>>>
>>> On 29/10/2021 06:40, 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>
>>>> Suggested-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 | 75 +++++++++++++++++--
>>>>  2 files changed, 72 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
>>>> index 0faaccd21447..f522288b2204 100644
>>>> --- a/tools/testing/selftests/net/mptcp/config
>>>> +++ b/tools/testing/selftests/net/mptcp/config
>>>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
>>>>  CONFIG_NETFILTER_XT_MATCH_BPF=m
>>>>  CONFIG_NF_TABLES_IPV4=y
>>>>  CONFIG_NF_TABLES_IPV6=y
>>>> +CONFIG_NET_ACT_CSUM=m
>>>> +CONFIG_NET_ACT_PEDIT=m
>>>> +CONFIG_NET_CLS_ACT=m
>>>> +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..d33cb5ce0ff3 100755
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> @@ -178,6 +178,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 +238,28 @@ link_failure()
>>>>  	done
>>>>  }
>>>>  
>>>> +checksum_failure()
>>>> +{
>>>> +	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 \
>>>> +		action pedit munge offset 148 u32 invert \
>>>> +		pipe csum tcp \
>>>> +		index 100
>>>> +
>>>> +	while true; do
>>>> +		local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
>>>> +				jq '.[1].actions[0].stats.packets')
>>>> +		if [ $pkt -gt 0 ]; then
>>> The CI [1] is complaining about that line:
>>>
>>> [09:34:44.063] # RTNETLINK answers: Operation not supported
>>> [09:34:44.107] # ./mptcp_join.sh: line 256: [: null: integer expression
>>> expected
>>> [09:34:44.267] # RTNETLINK answers: Operation not supported
>>> [09:34:44.332] # ./mptcp_join.sh: line 256: [: null: integer expression
>>> expected
>>> (...)
>>>
>>> This is displayed in a loop as it is a "while true".
>>>
>>> But just before the first one, we had this:
>>>
>>> [09:34:43.859] # Error: Cannot find ingress queue for specified device.
>>> [09:34:43.955] # Error: Parent Qdisc doesn't exists.
>>> [09:34:43.959] # We have an error talking to the kernel
>>>
>>> I guess the previous "tc" command failed and I also guess you don't have
>>> that on your side, right?
>>
>> Yes, it works on my side.
>>
>>> Maybe a missing kconfig?
>>
>> I think so. How can I get the kconfig of the CI?
> 
> The need kernel config flags are correctly specified into:
> 
> tools/testing/selftests/net/mptcp/config
> 
> In this case it looks like the CI is missing/not applying:
> 
> CONFIG_NET_SCH_INGRESS=y
> 
> @Mat, does the CI take properly in account the self-tests config?
> 
> I guess:
> 
> make kselftest-merge
> 
> would fix, but could also be overkill (pulling a lot of config for
> other, non mptcp self-tests)

I'm using something like:

  ./scripts/kconfig/merge_config.sh -m (...)/.config \
     tools/testing/selftests/net/mptcp/config

But I guess the issue is in the config we give:

.config:5004:warning: symbol value 'm' invalid for NET_CLS_ACT

It should then be set to =y. Sorry, I didn't see that before.
I can also change that when applying the patch if no other modifications
are needed. (and if I don't forget :) )

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

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

[Mat Martineau]

On Fri, 29 Oct 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>
> Suggested-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 | 75 +++++++++++++++++--
> 2 files changed, 72 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
> index 0faaccd21447..f522288b2204 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
> CONFIG_NETFILTER_XT_MATCH_BPF=m
> CONFIG_NF_TABLES_IPV4=y
> CONFIG_NF_TABLES_IPV6=y
> +CONFIG_NET_ACT_CSUM=m
> +CONFIG_NET_ACT_PEDIT=m
> +CONFIG_NET_CLS_ACT=m
> +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..d33cb5ce0ff3 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -178,6 +178,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 +238,28 @@ link_failure()
> 	done
> }
>
> +checksum_failure()
> +{
> +	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 \
> +		action pedit munge offset 148 u32 invert \
> +		pipe csum tcp \
> +		index 100

Could you add a comment explaining what the filter is doing? While it 
looks like the idea is to invert the 32-bit word at byte offset 148, I'm 
not tc-literate enough to know if there are other effects (and I didn't 
watch the email threads closely on this).

> +
> +	while true; do
> +		local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
> +				jq '.[1].actions[0].stats.packets')
> +		if [ $pkt -gt 0 ]; then
> +			tc -n $ns2 qdisc del dev ns2eth$i clsact
> +			break
> +		fi
> +	done
> +}
> +
> # $1: IP address
> is_v6()
> {
> @@ -371,6 +399,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
> +		checksum_failure ${addr_nr_ns2:5}
> +		addr_nr_ns2=0
> 	fi
>
> 	if [ $addr_nr_ns2 -gt 0 ]; then
> @@ -542,6 +573,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 +586,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 +596,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 +691,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 +735,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 +1872,25 @@ fullmesh_tests()
> 	chk_add_nr 1 1
> }
>
> +fail_tests()
> +{

Since MP_FAIL is only relevent with checksums enabled, I think it would be 
good to set $checksum=1 here and restore its previous value before 
returning (in case other tests run after this).

> +	# 1 subflow
> +	reset
> +	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 0 "fail_1" fast
> +	chk_join_nr "MP_FAIL test, 1 subflow" 0 0 0 1 1
> +
> +	# multiple subflows
> +	reset
> +	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 0 "fail_2" fast
> +	chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
> +}
> +
> all_tests()
> {
> 	subflows_tests
> @@ -1853,6 +1907,7 @@ all_tests()
> 	checksum_tests
> 	deny_join_id0_tests
> 	fullmesh_tests
> +	fail_tests
> }
>
> usage()
> @@ -1872,6 +1927,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 +1963,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 +2007,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
> 		m)
> 			fullmesh_tests
> 			;;
> +		F)
> +			fail_tests
> +			;;
> 		c)
> 			;;
> 		C)
> -- 
> 2.26.2

When I run:

$ sudo ./mptcp_join.sh -FC

I get:

"""
Created /tmp/tmp.UTIlkuhfRW (size 1 KB) containing data sent by client
Created /tmp/tmp.pMPhJddNfQ (size 1 KB) containing data sent by server
Created /tmp/tmp.OEx5cmVqbp (size 10075 KB) containing data sent by client
Created /tmp/tmp.RAtBQXOXtR (size 6144 KB) containing data sent by server
[ FAIL ] file received by server does not match (in, out):
-rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
Trailing bytes are:
MPTCP_TEST_FILE_END_MARKER
-rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
Trailing bytes are:
MPTCP_TEST_FILE_END_MARKER
01 MP_FAIL test, 1 subflow              syn[ ok ] - synack[ ok ] - ack[ ok ]
                                         sum[ ok ] - csum  [ ok ]
                                         ftx[ ok ] - frx   [ ok ]
                                         itx[ ok ] - irx   [ ok ]
[ FAIL ] file received by server does not match (in, out):
-rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
Trailing bytes are:
MPTCP_TEST_FILE_END_MARKER
-rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
Trailing bytes are:
MPTCP_TEST_FILE_END_MARKER
02 MP_FAIL test, multiple subflows      syn[ ok ] - synack[ ok ] - ack[ ok ]
                                         sum[ ok ] - csum  [ ok ]
                                         ftx[ ok ] - frx   [ ok ]
                                         itx[ ok ] - irx   [ ok ]
"""

The chk_join_nr output shows all [ ok ], which is good. But the file 
mismatches (I assume due to the inverted data) are failures because the 
checksums are intended to reject the bad data. Am I understanding the file 
mismatches correctly?

If MP_FAIL is working, the files should match.

Thanks,

--
Mat Martineau
Intel

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

[Matthieu Baerts]

Hi Geliang,

On 04/11/2021 01:43, Mat Martineau wrote:
> On Fri, 29 Oct 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>
>> Suggested-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 | 75 +++++++++++++++++--
>> 2 files changed, 72 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/config
>> b/tools/testing/selftests/net/mptcp/config
>> index 0faaccd21447..f522288b2204 100644
>> --- a/tools/testing/selftests/net/mptcp/config
>> +++ b/tools/testing/selftests/net/mptcp/config
>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
>> CONFIG_NETFILTER_XT_MATCH_BPF=m
>> CONFIG_NF_TABLES_IPV4=y
>> CONFIG_NF_TABLES_IPV6=y
>> +CONFIG_NET_ACT_CSUM=m
>> +CONFIG_NET_ACT_PEDIT=m
>> +CONFIG_NET_CLS_ACT=m
>> +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..d33cb5ce0ff3 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -178,6 +178,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 +238,28 @@ link_failure()
>>     done
>> }
>>
>> +checksum_failure()
>> +{
>> +    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 \
>> +        action pedit munge offset 148 u32 invert \
>> +        pipe csum tcp \
>> +        index 100
> 
> Could you add a comment explaining what the filter is doing? While it
> looks like the idea is to invert the 32-bit word at byte offset 148, I'm
> not tc-literate enough to know if there are other effects (and I didn't
> watch the email threads closely on this).

Yes that's correct. It takes only TCP packets on the egress side that
are big enough but print a message in dmesg if not. After the
modification, the TCP checksum is recomputed so the packet is valid.

> 
>> +
>> +    while true; do
>> +        local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
>> +                jq '.[1].actions[0].stats.packets')
>> +        if [ $pkt -gt 0 ]; then
>> +            tc -n $ns2 qdisc del dev ns2eth$i clsact
>> +            break
>> +        fi

On my side, I had to add a "sleep .1" to slow things down and have the
tests a bit more stable.

I also used "flower ip_proto tcp tcp_flags 0x10/0xff" and it seems
working fine.

But still and probably caused by the "sleep .1" that seems needed, I
have many messages in dmesg:

    tc action pedit offset 162 out of bounds

Because everything is shown on the serial, it is a bit annoying.

Do you think it would be possible to limit to TCP ACK and also make sure
data are only sent in one direction or limit incoming traffic to 1 byte
if it is easier? With this, we should then have a very few pure ACKs
packets on the egress side and avoid most of the TC warning messages
printed by the kernel, no?

>> +    done
>> +}
>> +
>> # $1: IP address
>> is_v6()
>> {
>> @@ -371,6 +399,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
>> +        checksum_failure ${addr_nr_ns2:5}
>> +        addr_nr_ns2=0
>>     fi
>>
>>     if [ $addr_nr_ns2 -gt 0 ]; then
>> @@ -542,6 +573,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 +586,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 +596,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 +691,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 +735,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 +1872,25 @@ fullmesh_tests()
>>     chk_add_nr 1 1
>> }
>>
>> +fail_tests()
>> +{
> 
> Since MP_FAIL is only relevent with checksums enabled, I think it would
> be good to set $checksum=1 here and restore its previous value before
> returning (in case other tests run after this).
> 
>> +    # 1 subflow
>> +    reset
>> +    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 0 "fail_1" fast
>> +    chk_join_nr "MP_FAIL test, 1 subflow" 0 0 0 1 1
>> +
>> +    # multiple subflows
>> +    reset
>> +    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 0 "fail_2" fast
>> +    chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
>> +}
>> +
>> all_tests()
>> {
>>     subflows_tests
>> @@ -1853,6 +1907,7 @@ all_tests()
>>     checksum_tests
>>     deny_join_id0_tests
>>     fullmesh_tests
>> +    fail_tests
>> }
>>
>> usage()
>> @@ -1872,6 +1927,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 +1963,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 +2007,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
>>         m)
>>             fullmesh_tests
>>             ;;
>> +        F)
>> +            fail_tests
>> +            ;;
>>         c)
>>             ;;
>>         C)
>> -- 
>> 2.26.2
> 
> When I run:
> 
> $ sudo ./mptcp_join.sh -FC
> 
> I get:
> 
> """
> Created /tmp/tmp.UTIlkuhfRW (size 1 KB) containing data sent by client
> Created /tmp/tmp.pMPhJddNfQ (size 1 KB) containing data sent by server
> Created /tmp/tmp.OEx5cmVqbp (size 10075 KB) containing data sent by client
> Created /tmp/tmp.RAtBQXOXtR (size 6144 KB) containing data sent by server
> [ FAIL ] file received by server does not match (in, out):
> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
> Trailing bytes are:
> MPTCP_TEST_FILE_END_MARKER
> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
> Trailing bytes are:
> MPTCP_TEST_FILE_END_MARKER
> 01 MP_FAIL test, 1 subflow              syn[ ok ] - synack[ ok ] - ack[
> ok ]
>                                         sum[ ok ] - csum  [ ok ]
>                                         ftx[ ok ] - frx   [ ok ]
>                                         itx[ ok ] - irx   [ ok ]
> [ FAIL ] file received by server does not match (in, out):
> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
> Trailing bytes are:
> MPTCP_TEST_FILE_END_MARKER
> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
> Trailing bytes are:
> MPTCP_TEST_FILE_END_MARKER
> 02 MP_FAIL test, multiple subflows      syn[ ok ] - synack[ ok ] - ack[
> ok ]
>                                         sum[ ok ] - csum  [ ok ]
>                                         ftx[ ok ] - frx   [ ok ]
>                                         itx[ ok ] - irx   [ ok ]
> """
> 
> The chk_join_nr output shows all [ ok ], which is good. But the file
> mismatches (I assume due to the inverted data) are failures because the
> checksums are intended to reject the bad data. Am I understanding the
> file mismatches correctly?
> 
> If MP_FAIL is working, the files should match.

I also have the same error on my side but not all the time.

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

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

[Geliang Tang]

Hi Matt, Davide,

On Thu, Nov 04, 2021 at 10:14:35AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 04/11/2021 01:43, Mat Martineau wrote:
> > On Fri, 29 Oct 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>
> >> Suggested-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 | 75 +++++++++++++++++--
> >> 2 files changed, 72 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/net/mptcp/config
> >> b/tools/testing/selftests/net/mptcp/config
> >> index 0faaccd21447..f522288b2204 100644
> >> --- a/tools/testing/selftests/net/mptcp/config
> >> +++ b/tools/testing/selftests/net/mptcp/config
> >> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
> >> CONFIG_NETFILTER_XT_MATCH_BPF=m
> >> CONFIG_NF_TABLES_IPV4=y
> >> CONFIG_NF_TABLES_IPV6=y
> >> +CONFIG_NET_ACT_CSUM=m
> >> +CONFIG_NET_ACT_PEDIT=m
> >> +CONFIG_NET_CLS_ACT=m
> >> +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..d33cb5ce0ff3 100755
> >> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> @@ -178,6 +178,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 +238,28 @@ link_failure()
> >>     done
> >> }
> >>
> >> +checksum_failure()
> >> +{
> >> +    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 \
> >> +        action pedit munge offset 148 u32 invert \
> >> +        pipe csum tcp \
> >> +        index 100
> > 
> > Could you add a comment explaining what the filter is doing? While it
> > looks like the idea is to invert the 32-bit word at byte offset 148, I'm
> > not tc-literate enough to know if there are other effects (and I didn't
> > watch the email threads closely on this).

Matt, could you please add this comment for me? I didn't understand
these tc commands very well, and I can't explain them very clearly in
English.

> 
> Yes that's correct. It takes only TCP packets on the egress side that
> are big enough but print a message in dmesg if not. After the
> modification, the TCP checksum is recomputed so the packet is valid.
> 
> > 
> >> +
> >> +    while true; do
> >> +        local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
> >> +                jq '.[1].actions[0].stats.packets')

Should we show the action 'pedit' here, instead of 'csum'?

The action 'csum' output is like this:

# tc -n $ns2 -j -s action show action csum index 100

[{"total acts":1},{"actions":[{"order":0,"kind":"csum","csum":"tcp","control_action":{"type":"pass"},"index":100,"ref":1,"bind":1,"installed":29,"last_used":2,"stats":{"bytes":11576,"packets":42,"drops":0,"overlimits":0,"requeues":0,"backlog":0,"qlen":0}}]}]

"overlimits" is always 0 in 'csum'.

But the action 'pedit' output is like this:

# tc -n $ns2 -j -s action show action pedit index 100

[{"total acts":1},{"actions":[{"order":0 pedit ,"control_action":{"type":"pipe"}keys 1
 	 index 1 ref 1 bind 1,"installed":29,"last_used":2
	 key #0  at 148: val ffffffff mask ffffffff
 ,"stats":{"bytes":11576,"packets":42,"drops":0,"overlimits":36,"requeues":0,"backlog":0,"qlen":0}}]}]

"overlimits" values are all right here, but this output can't be parsed by
'jq'. It looks like something wrong to show 'pedit'.

Is this a bug in act_pedit, or we use pedit incorrectly? How can I fix this?

> >> +        if [ $pkt -gt 0 ]; then
> >> +            tc -n $ns2 qdisc del dev ns2eth$i clsact
> >> +            break
> >> +        fi
> 
> On my side, I had to add a "sleep .1" to slow things down and have the
> tests a bit more stable.
> 
> I also used "flower ip_proto tcp tcp_flags 0x10/0xff" and it seems
> working fine.

Could you please share your checksum_failure function here, I'll test it
on my side?

> 
> But still and probably caused by the "sleep .1" that seems needed, I
> have many messages in dmesg:
> 
>     tc action pedit offset 162 out of bounds
> 
> Because everything is shown on the serial, it is a bit annoying.
> 
> Do you think it would be possible to limit to TCP ACK and also make sure
> data are only sent in one direction or limit incoming traffic to 1 byte
> if it is easier? With this, we should then have a very few pure ACKs
> packets on the egress side and avoid most of the TC warning messages
> printed by the kernel, no?
> 
> >> +    done
> >> +}
> >> +
> >> # $1: IP address
> >> is_v6()
> >> {
> >> @@ -371,6 +399,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
> >> +        checksum_failure ${addr_nr_ns2:5}
> >> +        addr_nr_ns2=0
> >>     fi
> >>
> >>     if [ $addr_nr_ns2 -gt 0 ]; then
> >> @@ -542,6 +573,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 +586,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 +596,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 +691,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 +735,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 +1872,25 @@ fullmesh_tests()
> >>     chk_add_nr 1 1
> >> }
> >>
> >> +fail_tests()
> >> +{
> > 
> > Since MP_FAIL is only relevent with checksums enabled, I think it would
> > be good to set $checksum=1 here and restore its previous value before
> > returning (in case other tests run after this).
> > 
> >> +    # 1 subflow
> >> +    reset
> >> +    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 0 "fail_1" fast

Here I passed 2 as the 4th argument of run_tests like the link_failure
tests to create the bigger data files. I don't know why it dosen't work
when passing 0 as the argument?

> >> +    chk_join_nr "MP_FAIL test, 1 subflow" 0 0 0 1 1
> >> +
> >> +    # multiple subflows
> >> +    reset
> >> +    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 0 "fail_2" fast
> >> +    chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
> >> +}
> >> +
> >> all_tests()
> >> {
> >>     subflows_tests
> >> @@ -1853,6 +1907,7 @@ all_tests()
> >>     checksum_tests
> >>     deny_join_id0_tests
> >>     fullmesh_tests
> >> +    fail_tests
> >> }
> >>
> >> usage()
> >> @@ -1872,6 +1927,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 +1963,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 +2007,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
> >>         m)
> >>             fullmesh_tests
> >>             ;;
> >> +        F)
> >> +            fail_tests
> >> +            ;;
> >>         c)
> >>             ;;
> >>         C)
> >> -- 
> >> 2.26.2
> > 
> > When I run:
> > 
> > $ sudo ./mptcp_join.sh -FC
> > 
> > I get:
> > 
> > """
> > Created /tmp/tmp.UTIlkuhfRW (size 1 KB) containing data sent by client
> > Created /tmp/tmp.pMPhJddNfQ (size 1 KB) containing data sent by server
> > Created /tmp/tmp.OEx5cmVqbp (size 10075 KB) containing data sent by client
> > Created /tmp/tmp.RAtBQXOXtR (size 6144 KB) containing data sent by server
> > [ FAIL ] file received by server does not match (in, out):
> > -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
> > Trailing bytes are:
> > MPTCP_TEST_FILE_END_MARKER
> > -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
> > Trailing bytes are:
> > MPTCP_TEST_FILE_END_MARKER
> > 01 MP_FAIL test, 1 subflow              syn[ ok ] - synack[ ok ] - ack[
> > ok ]
> >                                         sum[ ok ] - csum  [ ok ]
> >                                         ftx[ ok ] - frx   [ ok ]
> >                                         itx[ ok ] - irx   [ ok ]
> > [ FAIL ] file received by server does not match (in, out):
> > -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
> > Trailing bytes are:
> > MPTCP_TEST_FILE_END_MARKER
> > -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
> > Trailing bytes are:
> > MPTCP_TEST_FILE_END_MARKER
> > 02 MP_FAIL test, multiple subflows      syn[ ok ] - synack[ ok ] - ack[
> > ok ]
> >                                         sum[ ok ] - csum  [ ok ]
> >                                         ftx[ ok ] - frx   [ ok ]
> >                                         itx[ ok ] - irx   [ ok ]
> > """
> > 
> > The chk_join_nr output shows all [ ok ], which is good. But the file
> > mismatches (I assume due to the inverted data) are failures because the
> > checksums are intended to reject the bad data. Am I understanding the
> > file mismatches correctly?
> > 
> > If MP_FAIL is working, the files should match.
> 
> I also have the same error on my side but not all the time.

I got these file mismatches errors all the time. Could you please share
your test script and the logs when you get the right output?

The first test "01 MP_FAIL test, 1 subflow" will get the file mismatches
all the time, since the checksum error data can't be retransmitted in
the current codes.

But the second test "02 MP_FAIL test, multiple subflows" shouldn't get
the file mismatches. RST is sent on the checksum error subflow, so this
subflow is closed. The data will be retransmitted on the other subflow.
It works by using the old test case "[PATCH mptcp-next v7 8/8]
DO-NOT-MERGE: mptcp: mp_fail test". Data will be retransmitted, and no
the file mismatches. I don't know why it dosen't work by this new test
case. In this test case, I saw the error data has been dropped, and the
new data has been retransmitted in the log. I'll try to fix this.

Thanks,
-Geliang

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

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

[Matthieu Baerts]

Hi Geliang,

On 04/11/2021 11:30, Geliang Tang wrote:
> Hi Matt, Davide,
> 
> On Thu, Nov 04, 2021 at 10:14:35AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 04/11/2021 01:43, Mat Martineau wrote:
>>> On Fri, 29 Oct 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>
>>>> Suggested-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 | 75 +++++++++++++++++--
>>>> 2 files changed, 72 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tools/testing/selftests/net/mptcp/config
>>>> b/tools/testing/selftests/net/mptcp/config
>>>> index 0faaccd21447..f522288b2204 100644
>>>> --- a/tools/testing/selftests/net/mptcp/config
>>>> +++ b/tools/testing/selftests/net/mptcp/config
>>>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
>>>> CONFIG_NETFILTER_XT_MATCH_BPF=m
>>>> CONFIG_NF_TABLES_IPV4=y
>>>> CONFIG_NF_TABLES_IPV6=y
>>>> +CONFIG_NET_ACT_CSUM=m
>>>> +CONFIG_NET_ACT_PEDIT=m
>>>> +CONFIG_NET_CLS_ACT=m
>>>> +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..d33cb5ce0ff3 100755
>>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>>>> @@ -178,6 +178,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 +238,28 @@ link_failure()
>>>>     done
>>>> }
>>>>
>>>> +checksum_failure()
>>>> +{
>>>> +    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 \
>>>> +        action pedit munge offset 148 u32 invert \
>>>> +        pipe csum tcp \
>>>> +        index 100
>>>
>>> Could you add a comment explaining what the filter is doing? While it
>>> looks like the idea is to invert the 32-bit word at byte offset 148, I'm
>>> not tc-literate enough to know if there are other effects (and I didn't
>>> watch the email threads closely on this).
> 
> Matt, could you please add this comment for me? I didn't understand
> these tc commands very well, and I can't explain them very clearly in
> English.

Sure. Here is my proposition:

    # 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.

This comment makes sense if you use "flower ip_proto tcp tcp_flags
0x10/0xff" and reduce incoming traffic. Otherwise, we need to remove
"ACK" and/or the last sentence but I don't think we should make this
test printing a lot of pr_info().

> 
>>
>> Yes that's correct. It takes only TCP packets on the egress side that
>> are big enough but print a message in dmesg if not. After the
>> modification, the TCP checksum is recomputed so the packet is valid.
>>
>>>
>>>> +
>>>> +    while true; do
>>>> +        local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
>>>> +                jq '.[1].actions[0].stats.packets')
> 
> Should we show the action 'pedit' here, instead of 'csum'?
> 
> The action 'csum' output is like this:
> 
> # tc -n $ns2 -j -s action show action csum index 100
> 
> [{"total acts":1},{"actions":[{"order":0,"kind":"csum","csum":"tcp","control_action":{"type":"pass"},"index":100,"ref":1,"bind":1,"installed":29,"last_used":2,"stats":{"bytes":11576,"packets":42,"drops":0,"overlimits":0,"requeues":0,"backlog":0,"qlen":0}}]}]
> 
> "overlimits" is always 0 in 'csum'.

Good point. I thought 'pedit' would not let the packet passing to csum
if it was over the limits but I guess it does if we see the same values
for packets/bytes counters in 'csum' and 'pedit' actions.

> But the action 'pedit' output is like this:
> 
> # tc -n $ns2 -j -s action show action pedit index 100
> 
> [{"total acts":1},{"actions":[{"order":0 pedit ,"control_action":{"type":"pipe"}keys 1
>  	 index 1 ref 1 bind 1,"installed":29,"last_used":2
> 	 key #0  at 148: val ffffffff mask ffffffff
>  ,"stats":{"bytes":11576,"packets":42,"drops":0,"overlimits":36,"requeues":0,"backlog":0,"qlen":0}}]}]

(When you will get the right output, may you add this in a comment in
the script as an example to explain what you have and what you try to
parse please?)

> 
> "overlimits" values are all right here

From what I see in the code, if the packet was not carrying enough data
'act_pedit' will increase the 'overlimits' counter and the other ones --
bytes and packets I guess -- as well:


https://elixir.bootlin.com/linux/latest/source/net/sched/act_pedit.c#L368

So good point, we should look at 'pedit' counters and more precisely at:

  "packets" - "overlimits" > 0

here.

> but this output can't be parsed by
> 'jq'. It looks like something wrong to show 'pedit'.
>> Is this a bug in act_pedit, or we use pedit incorrectly? How can I fix
this?

Do you mean you have this error?

  parse error: Invalid numeric literal at line 1, column 47

To me, it looks like a bug on printing data, so on 'tc' side.
Which version of 'tc' are you using? May you try to upgrade to a more
recent one? e.g. IPRoute2 5.15.0? Maybe it has already been fixed.

>>>> +        if [ $pkt -gt 0 ]; then
>>>> +            tc -n $ns2 qdisc del dev ns2eth$i clsact
>>>> +            break
>>>> +        fi
>>
>> On my side, I had to add a "sleep .1" to slow things down and have the
>> tests a bit more stable.
>>
>> I also used "flower ip_proto tcp tcp_flags 0x10/0xff" and it seems
>> working fine.
> 
> Could you please share your checksum_failure function here, I'll test it
> on my side?

I did that to have the test finishing on my side:

--------------- 8< ---------------
diff --git a/tools/testing/selftests/net/mptcp/config
b/tools/testing/selftests/net/mptcp/config
index f522288b2204..2aff41754c8f 100644
--- a/tools/testing/selftests/net/mptcp/config
+++ b/tools/testing/selftests/net/mptcp/config
@@ -17,6 +17,6 @@ CONFIG_NF_TABLES_IPV4=y
 CONFIG_NF_TABLES_IPV6=y
 CONFIG_NET_ACT_CSUM=m
 CONFIG_NET_ACT_PEDIT=m
-CONFIG_NET_CLS_ACT=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 d33cb5ce0ff3..8dcd445b1532 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -245,7 +245,7 @@ checksum_failure()
        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 \
+               flower ip_proto tcp tcp_flags 0x10/0xff \
                action pedit munge offset 148 u32 invert \
                pipe csum tcp \
                index 100
@@ -257,6 +257,7 @@ checksum_failure()
                        tc -n $ns2 qdisc del dev ns2eth$i clsact
                        break
                fi
+               sleep .1
        done
 }

--------------- 8< ---------------

> 
>>
>> But still and probably caused by the "sleep .1" that seems needed, I
>> have many messages in dmesg:
>>
>>     tc action pedit offset 162 out of bounds
>>
>> Because everything is shown on the serial, it is a bit annoying.
>>
>> Do you think it would be possible to limit to TCP ACK and also make sure
>> data are only sent in one direction or limit incoming traffic to 1 byte
>> if it is easier? With this, we should then have a very few pure ACKs
>> packets on the egress side and avoid most of the TC warning messages
>> printed by the kernel, no?
>>
>>>> +    done
>>>> +}
>>>> +
>>>> # $1: IP address
>>>> is_v6()
>>>> {
>>>> @@ -371,6 +399,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
>>>> +        checksum_failure ${addr_nr_ns2:5}
>>>> +        addr_nr_ns2=0
>>>>     fi
>>>>
>>>>     if [ $addr_nr_ns2 -gt 0 ]; then
>>>> @@ -542,6 +573,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 +586,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 +596,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 +691,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 +735,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 +1872,25 @@ fullmesh_tests()
>>>>     chk_add_nr 1 1
>>>> }
>>>>
>>>> +fail_tests()
>>>> +{
>>>
>>> Since MP_FAIL is only relevent with checksums enabled, I think it would
>>> be good to set $checksum=1 here and restore its previous value before
>>> returning (in case other tests run after this).
>>>
>>>> +    # 1 subflow
>>>> +    reset
>>>> +    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 0 "fail_1" fast
> 
> Here I passed 2 as the 4th argument of run_tests like the link_failure
> tests to create the bigger data files. I don't know why it dosen't work
> when passing 0 as the argument?

Should we first insert the TC rule, then start the test, then look at
counters?
It would be "safer" because we would not need need to make sure we have
enough data to transfer. WDYT?

>>>> +    chk_join_nr "MP_FAIL test, 1 subflow" 0 0 0 1 1
>>>> +
>>>> +    # multiple subflows
>>>> +    reset
>>>> +    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 0 "fail_2" fast
>>>> +    chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
>>>> +}
>>>> +
>>>> all_tests()
>>>> {
>>>>     subflows_tests
>>>> @@ -1853,6 +1907,7 @@ all_tests()
>>>>     checksum_tests
>>>>     deny_join_id0_tests
>>>>     fullmesh_tests
>>>> +    fail_tests
>>>> }
>>>>
>>>> usage()
>>>> @@ -1872,6 +1927,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 +1963,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 +2007,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
>>>>         m)
>>>>             fullmesh_tests
>>>>             ;;
>>>> +        F)
>>>> +            fail_tests
>>>> +            ;;
>>>>         c)
>>>>             ;;
>>>>         C)
>>>> -- 
>>>> 2.26.2
>>>
>>> When I run:
>>>
>>> $ sudo ./mptcp_join.sh -FC
>>>
>>> I get:
>>>
>>> """
>>> Created /tmp/tmp.UTIlkuhfRW (size 1 KB) containing data sent by client
>>> Created /tmp/tmp.pMPhJddNfQ (size 1 KB) containing data sent by server
>>> Created /tmp/tmp.OEx5cmVqbp (size 10075 KB) containing data sent by client
>>> Created /tmp/tmp.RAtBQXOXtR (size 6144 KB) containing data sent by server
>>> [ FAIL ] file received by server does not match (in, out):
>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
>>> Trailing bytes are:
>>> MPTCP_TEST_FILE_END_MARKER
>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
>>> Trailing bytes are:
>>> MPTCP_TEST_FILE_END_MARKER
>>> 01 MP_FAIL test, 1 subflow              syn[ ok ] - synack[ ok ] - ack[
>>> ok ]
>>>                                         sum[ ok ] - csum  [ ok ]
>>>                                         ftx[ ok ] - frx   [ ok ]
>>>                                         itx[ ok ] - irx   [ ok ]
>>> [ FAIL ] file received by server does not match (in, out):
>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
>>> Trailing bytes are:
>>> MPTCP_TEST_FILE_END_MARKER
>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
>>> Trailing bytes are:
>>> MPTCP_TEST_FILE_END_MARKER
>>> 02 MP_FAIL test, multiple subflows      syn[ ok ] - synack[ ok ] - ack[
>>> ok ]
>>>                                         sum[ ok ] - csum  [ ok ]
>>>                                         ftx[ ok ] - frx   [ ok ]
>>>                                         itx[ ok ] - irx   [ ok ]
>>> """
>>>
>>> The chk_join_nr output shows all [ ok ], which is good. But the file
>>> mismatches (I assume due to the inverted data) are failures because the
>>> checksums are intended to reject the bad data. Am I understanding the
>>> file mismatches correctly?
>>>
>>> If MP_FAIL is working, the files should match.
>>
>> I also have the same error on my side but not all the time.
> 
> I got these file mismatches errors all the time. Could you please share
> your test script and the logs when you get the right output?

I don't think I saw them for both tests all the time but it is possible
it was always there for the 1st one and not for the second one. I will
look at that next time.

> The first test "01 MP_FAIL test, 1 subflow" will get the file mismatches
> all the time, since the checksum error data can't be retransmitted in
> the current codes.

Should we disable this test then?

> But the second test "02 MP_FAIL test, multiple subflows" shouldn't get
> the file mismatches. RST is sent on the checksum error subflow, so this
> subflow is closed. The data will be retransmitted on the other subflow.
> It works by using the old test case "[PATCH mptcp-next v7 8/8]
> DO-NOT-MERGE: mptcp: mp_fail test". Data will be retransmitted, and no
> the file mismatches. I don't know why it dosen't work by this new test
> case. In this test case, I saw the error data has been dropped, and the
> new data has been retransmitted in the log. I'll try to fix this.

Thanks!

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

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

[Geliang Tang]

Hi Matt,

On Thu, Nov 04, 2021 at 12:48:38PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 04/11/2021 11:30, Geliang Tang wrote:
> > Hi Matt, Davide,
> > 
> > On Thu, Nov 04, 2021 at 10:14:35AM +0100, Matthieu Baerts wrote:
> >> Hi Geliang,
> >>
> >> On 04/11/2021 01:43, Mat Martineau wrote:
> >>> On Fri, 29 Oct 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>
> >>>> Suggested-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 | 75 +++++++++++++++++--
> >>>> 2 files changed, 72 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/tools/testing/selftests/net/mptcp/config
> >>>> b/tools/testing/selftests/net/mptcp/config
> >>>> index 0faaccd21447..f522288b2204 100644
> >>>> --- a/tools/testing/selftests/net/mptcp/config
> >>>> +++ b/tools/testing/selftests/net/mptcp/config
> >>>> @@ -15,3 +15,8 @@ CONFIG_NETFILTER_XTABLES=m
> >>>> CONFIG_NETFILTER_XT_MATCH_BPF=m
> >>>> CONFIG_NF_TABLES_IPV4=y
> >>>> CONFIG_NF_TABLES_IPV6=y
> >>>> +CONFIG_NET_ACT_CSUM=m
> >>>> +CONFIG_NET_ACT_PEDIT=m
> >>>> +CONFIG_NET_CLS_ACT=m
> >>>> +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..d33cb5ce0ff3 100755
> >>>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >>>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >>>> @@ -178,6 +178,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 +238,28 @@ link_failure()
> >>>>     done
> >>>> }
> >>>>
> >>>> +checksum_failure()
> >>>> +{
> >>>> +    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 \
> >>>> +        action pedit munge offset 148 u32 invert \
> >>>> +        pipe csum tcp \
> >>>> +        index 100
> >>>
> >>> Could you add a comment explaining what the filter is doing? While it
> >>> looks like the idea is to invert the 32-bit word at byte offset 148, I'm
> >>> not tc-literate enough to know if there are other effects (and I didn't
> >>> watch the email threads closely on this).
> > 
> > Matt, could you please add this comment for me? I didn't understand
> > these tc commands very well, and I can't explain them very clearly in
> > English.
> 
> Sure. Here is my proposition:
> 
>     # 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.
> 
> This comment makes sense if you use "flower ip_proto tcp tcp_flags
> 0x10/0xff" and reduce incoming traffic. Otherwise, we need to remove
> "ACK" and/or the last sentence but I don't think we should make this
> test printing a lot of pr_info().

Thanks very much, I'll add this comment in the next version and your
Co-developed-by tag.

> 
> > 
> >>
> >> Yes that's correct. It takes only TCP packets on the egress side that
> >> are big enough but print a message in dmesg if not. After the
> >> modification, the TCP checksum is recomputed so the packet is valid.
> >>
> >>>
> >>>> +
> >>>> +    while true; do
> >>>> +        local pkt=$(tc -n $ns2 -j -s action show action csum index 100 |
> >>>> +                jq '.[1].actions[0].stats.packets')
> > 
> > Should we show the action 'pedit' here, instead of 'csum'?
> > 
> > The action 'csum' output is like this:
> > 
> > # tc -n $ns2 -j -s action show action csum index 100
> > 
> > [{"total acts":1},{"actions":[{"order":0,"kind":"csum","csum":"tcp","control_action":{"type":"pass"},"index":100,"ref":1,"bind":1,"installed":29,"last_used":2,"stats":{"bytes":11576,"packets":42,"drops":0,"overlimits":0,"requeues":0,"backlog":0,"qlen":0}}]}]
> > 
> > "overlimits" is always 0 in 'csum'.
> 
> Good point. I thought 'pedit' would not let the packet passing to csum
> if it was over the limits but I guess it does if we see the same values
> for packets/bytes counters in 'csum' and 'pedit' actions.
> 
> > But the action 'pedit' output is like this:
> > 
> > # tc -n $ns2 -j -s action show action pedit index 100
> > 
> > [{"total acts":1},{"actions":[{"order":0 pedit ,"control_action":{"type":"pipe"}keys 1
> >  	 index 1 ref 1 bind 1,"installed":29,"last_used":2
> > 	 key #0  at 148: val ffffffff mask ffffffff
> >  ,"stats":{"bytes":11576,"packets":42,"drops":0,"overlimits":36,"requeues":0,"backlog":0,"qlen":0}}]}]
> 
> (When you will get the right output, may you add this in a comment in
> the script as an example to explain what you have and what you try to
> parse please?)
> 
> > 
> > "overlimits" values are all right here
> 
> From what I see in the code, if the packet was not carrying enough data
> 'act_pedit' will increase the 'overlimits' counter and the other ones --
> bytes and packets I guess -- as well:
> 
> 
> https://elixir.bootlin.com/linux/latest/source/net/sched/act_pedit.c#L368
> 
> So good point, we should look at 'pedit' counters and more precisely at:
> 
>   "packets" - "overlimits" > 0

Exactly!

> 
> here.
> 
> > but this output can't be parsed by
> > 'jq'. It looks like something wrong to show 'pedit'.
> >> Is this a bug in act_pedit, or we use pedit incorrectly? How can I fix
> this?
> 
> Do you mean you have this error?
> 
>   parse error: Invalid numeric literal at line 1, column 47
> 
> To me, it looks like a bug on printing data, so on 'tc' side.
> Which version of 'tc' are you using? May you try to upgrade to a more
> recent one? e.g. IPRoute2 5.15.0? Maybe it has already been fixed.

You're right, it's a tc bug. And fixed already in IPRoute2 5.15.0.

> 
> >>>> +        if [ $pkt -gt 0 ]; then
> >>>> +            tc -n $ns2 qdisc del dev ns2eth$i clsact
> >>>> +            break
> >>>> +        fi
> >>
> >> On my side, I had to add a "sleep .1" to slow things down and have the
> >> tests a bit more stable.
> >>
> >> I also used "flower ip_proto tcp tcp_flags 0x10/0xff" and it seems
> >> working fine.
> > 
> > Could you please share your checksum_failure function here, I'll test it
> > on my side?
> 
> I did that to have the test finishing on my side:
> 
> --------------- 8< ---------------
> diff --git a/tools/testing/selftests/net/mptcp/config
> b/tools/testing/selftests/net/mptcp/config
> index f522288b2204..2aff41754c8f 100644
> --- a/tools/testing/selftests/net/mptcp/config
> +++ b/tools/testing/selftests/net/mptcp/config
> @@ -17,6 +17,6 @@ CONFIG_NF_TABLES_IPV4=y
>  CONFIG_NF_TABLES_IPV6=y
>  CONFIG_NET_ACT_CSUM=m
>  CONFIG_NET_ACT_PEDIT=m
> -CONFIG_NET_CLS_ACT=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 d33cb5ce0ff3..8dcd445b1532 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -245,7 +245,7 @@ checksum_failure()
>         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 \
> +               flower ip_proto tcp tcp_flags 0x10/0xff \
>                 action pedit munge offset 148 u32 invert \
>                 pipe csum tcp \
>                 index 100
> @@ -257,6 +257,7 @@ checksum_failure()
>                         tc -n $ns2 qdisc del dev ns2eth$i clsact
>                         break
>                 fi
> +               sleep .1
>         done
>  }
> 
> --------------- 8< ---------------

Thanks, this works on my side too. I'll add this 'tcp_flags 0x10/0xff'
in the next version.

> 
> > 
> >>
> >> But still and probably caused by the "sleep .1" that seems needed, I
> >> have many messages in dmesg:
> >>
> >>     tc action pedit offset 162 out of bounds
> >>
> >> Because everything is shown on the serial, it is a bit annoying.
> >>
> >> Do you think it would be possible to limit to TCP ACK and also make sure
> >> data are only sent in one direction or limit incoming traffic to 1 byte
> >> if it is easier? With this, we should then have a very few pure ACKs
> >> packets on the egress side and avoid most of the TC warning messages
> >> printed by the kernel, no?
> >>
> >>>> +    done
> >>>> +}
> >>>> +
> >>>> # $1: IP address
> >>>> is_v6()
> >>>> {
> >>>> @@ -371,6 +399,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
> >>>> +        checksum_failure ${addr_nr_ns2:5}
> >>>> +        addr_nr_ns2=0
> >>>>     fi
> >>>>
> >>>>     if [ $addr_nr_ns2 -gt 0 ]; then
> >>>> @@ -542,6 +573,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 +586,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 +596,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 +691,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 +735,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 +1872,25 @@ fullmesh_tests()
> >>>>     chk_add_nr 1 1
> >>>> }
> >>>>
> >>>> +fail_tests()
> >>>> +{
> >>>
> >>> Since MP_FAIL is only relevent with checksums enabled, I think it would
> >>> be good to set $checksum=1 here and restore its previous value before
> >>> returning (in case other tests run after this).
> >>>
> >>>> +    # 1 subflow
> >>>> +    reset
> >>>> +    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 0 "fail_1" fast
> > 
> > Here I passed 2 as the 4th argument of run_tests like the link_failure
> > tests to create the bigger data files. I don't know why it dosen't work
> > when passing 0 as the argument?
> 
> Should we first insert the TC rule, then start the test, then look at
> counters?
> It would be "safer" because we would not need need to make sure we have
> enough data to transfer. WDYT?

Good point, I'll try it.

> 
> >>>> +    chk_join_nr "MP_FAIL test, 1 subflow" 0 0 0 1 1
> >>>> +
> >>>> +    # multiple subflows
> >>>> +    reset
> >>>> +    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 0 "fail_2" fast
> >>>> +    chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
> >>>> +}
> >>>> +
> >>>> all_tests()
> >>>> {
> >>>>     subflows_tests
> >>>> @@ -1853,6 +1907,7 @@ all_tests()
> >>>>     checksum_tests
> >>>>     deny_join_id0_tests
> >>>>     fullmesh_tests
> >>>> +    fail_tests
> >>>> }
> >>>>
> >>>> usage()
> >>>> @@ -1872,6 +1927,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 +1963,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 +2007,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
> >>>>         m)
> >>>>             fullmesh_tests
> >>>>             ;;
> >>>> +        F)
> >>>> +            fail_tests
> >>>> +            ;;
> >>>>         c)
> >>>>             ;;
> >>>>         C)
> >>>> -- 
> >>>> 2.26.2
> >>>
> >>> When I run:
> >>>
> >>> $ sudo ./mptcp_join.sh -FC
> >>>
> >>> I get:
> >>>
> >>> """
> >>> Created /tmp/tmp.UTIlkuhfRW (size 1 KB) containing data sent by client
> >>> Created /tmp/tmp.pMPhJddNfQ (size 1 KB) containing data sent by server
> >>> Created /tmp/tmp.OEx5cmVqbp (size 10075 KB) containing data sent by client
> >>> Created /tmp/tmp.RAtBQXOXtR (size 6144 KB) containing data sent by server
> >>> [ FAIL ] file received by server does not match (in, out):
> >>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
> >>> Trailing bytes are:
> >>> MPTCP_TEST_FILE_END_MARKER
> >>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
> >>> Trailing bytes are:
> >>> MPTCP_TEST_FILE_END_MARKER
> >>> 01 MP_FAIL test, 1 subflow              syn[ ok ] - synack[ ok ] - ack[
> >>> ok ]
> >>>                                         sum[ ok ] - csum  [ ok ]
> >>>                                         ftx[ ok ] - frx   [ ok ]
> >>>                                         itx[ ok ] - irx   [ ok ]
> >>> [ FAIL ] file received by server does not match (in, out):
> >>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
> >>> Trailing bytes are:
> >>> MPTCP_TEST_FILE_END_MARKER
> >>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
> >>> Trailing bytes are:
> >>> MPTCP_TEST_FILE_END_MARKER
> >>> 02 MP_FAIL test, multiple subflows      syn[ ok ] - synack[ ok ] - ack[
> >>> ok ]
> >>>                                         sum[ ok ] - csum  [ ok ]
> >>>                                         ftx[ ok ] - frx   [ ok ]
> >>>                                         itx[ ok ] - irx   [ ok ]
> >>> """
> >>>
> >>> The chk_join_nr output shows all [ ok ], which is good. But the file
> >>> mismatches (I assume due to the inverted data) are failures because the
> >>> checksums are intended to reject the bad data. Am I understanding the
> >>> file mismatches correctly?
> >>>
> >>> If MP_FAIL is working, the files should match.
> >>
> >> I also have the same error on my side but not all the time.
> > 
> > I got these file mismatches errors all the time. Could you please share
> > your test script and the logs when you get the right output?
> 
> I don't think I saw them for both tests all the time but it is possible
> it was always there for the 1st one and not for the second one. I will
> look at that next time.
> 
> > The first test "01 MP_FAIL test, 1 subflow" will get the file mismatches
> > all the time, since the checksum error data can't be retransmitted in
> > the current codes.
> 
> Should we disable this test then?

We should keep this test. It's useful for testing sending and receiving
of the infinite mapping. The infinite mapping is only sent in this single
subflow case.

> 
> > But the second test "02 MP_FAIL test, multiple subflows" shouldn't get
> > the file mismatches. RST is sent on the checksum error subflow, so this
> > subflow is closed. The data will be retransmitted on the other subflow.
> > It works by using the old test case "[PATCH mptcp-next v7 8/8]
> > DO-NOT-MERGE: mptcp: mp_fail test". Data will be retransmitted, and no
> > the file mismatches. I don't know why it dosen't work by this new test
> > case. In this test case, I saw the error data has been dropped, and the
> > new data has been retransmitted in the log. I'll try to fix this.
>

Could we merge patches 1-7 of this patchset into the export branch? Then
I'll just iterate a new version of this test case patch. And if there's
something I need to fix next week, I'll send the squash-to patch. Please
discuss this with Mat in today's weekly meeting.

Thanks,
-Geliang

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

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

[Matthieu Baerts]

Hi Geliang,

On 04/11/2021 14:13, Geliang Tang wrote:
> On Thu, Nov 04, 2021 at 12:48:38PM +0100, Matthieu Baerts wrote:
>> On 04/11/2021 11:30, Geliang Tang wrote:
>>> Is this a bug in act_pedit, or we use pedit incorrectly? How can I fix
>>> this?
>>
>> Do you mean you have this error?
>>
>>   parse error: Invalid numeric literal at line 1, column 47
>>
>> To me, it looks like a bug on printing data, so on 'tc' side.
>> Which version of 'tc' are you using? May you try to upgrade to a more
>> recent one? e.g. IPRoute2 5.15.0? Maybe it has already been fixed.
> 
> You're right, it's a tc bug. And fixed already in IPRoute2 5.15.0.

Which version were you using before?
I guess we need at least the 5.8.0 to have this commit:

https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/commit/tc/m_pedit.c?id=081d6c310d3a6e0412431a9652f641dff3f72aee

If yes, we should probably skip/abort the test if we cannot parse the json.

(...)

>>>>>> +    # 1 subflow
>>>>>> +    reset
>>>>>> +    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 0 "fail_1" fast
>>>
>>> Here I passed 2 as the 4th argument of run_tests like the link_failure
>>> tests to create the bigger data files. I don't know why it dosen't work
>>> when passing 0 as the argument?
>>
>> Should we first insert the TC rule, then start the test, then look at
>> counters?
>> It would be "safer" because we would not need need to make sure we have
>> enough data to transfer. WDYT?
> 
> Good point, I'll try it.

Thanks!

May you also make sure that for this test, we transfer data only in one
direction? I don't know if for the other direction, we could not give
/dev/null or generate a 1 byte file.

>>>>>> +    chk_join_nr "MP_FAIL test, 1 subflow" 0 0 0 1 1
>>>>>> +
>>>>>> +    # multiple subflows
>>>>>> +    reset
>>>>>> +    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 0 "fail_2" fast
>>>>>> +    chk_join_nr "MP_FAIL test, multiple subflows" 2 2 2 1
>>>>>> +}
>>>>>> +
>>>>>> all_tests()
>>>>>> {
>>>>>>     subflows_tests
>>>>>> @@ -1853,6 +1907,7 @@ all_tests()
>>>>>>     checksum_tests
>>>>>>     deny_join_id0_tests
>>>>>>     fullmesh_tests
>>>>>> +    fail_tests
>>>>>> }
>>>>>>
>>>>>> usage()
>>>>>> @@ -1872,6 +1927,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 +1963,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 +2007,9 @@ while getopts 'fsltra64bpkdmchCS' opt; do
>>>>>>         m)
>>>>>>             fullmesh_tests
>>>>>>             ;;
>>>>>> +        F)
>>>>>> +            fail_tests
>>>>>> +            ;;
>>>>>>         c)
>>>>>>             ;;
>>>>>>         C)
>>>>>> -- 
>>>>>> 2.26.2
>>>>>
>>>>> When I run:
>>>>>
>>>>> $ sudo ./mptcp_join.sh -FC
>>>>>
>>>>> I get:
>>>>>
>>>>> """
>>>>> Created /tmp/tmp.UTIlkuhfRW (size 1 KB) containing data sent by client
>>>>> Created /tmp/tmp.pMPhJddNfQ (size 1 KB) containing data sent by server
>>>>> Created /tmp/tmp.OEx5cmVqbp (size 10075 KB) containing data sent by client
>>>>> Created /tmp/tmp.RAtBQXOXtR (size 6144 KB) containing data sent by server
>>>>> [ FAIL ] file received by server does not match (in, out):
>>>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
>>>>> Trailing bytes are:
>>>>> MPTCP_TEST_FILE_END_MARKER
>>>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
>>>>> Trailing bytes are:
>>>>> MPTCP_TEST_FILE_END_MARKER
>>>>> 01 MP_FAIL test, 1 subflow              syn[ ok ] - synack[ ok ] - ack[
>>>>> ok ]
>>>>>                                         sum[ ok ] - csum  [ ok ]
>>>>>                                         ftx[ ok ] - frx   [ ok ]
>>>>>                                         itx[ ok ] - irx   [ ok ]
>>>>> [ FAIL ] file received by server does not match (in, out):
>>>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.HtyoJjpCOe
>>>>> Trailing bytes are:
>>>>> MPTCP_TEST_FILE_END_MARKER
>>>>> -rw-------. 1 root root 20633656 Nov  3 17:32 /tmp/tmp.j8HYN1PUVh
>>>>> Trailing bytes are:
>>>>> MPTCP_TEST_FILE_END_MARKER
>>>>> 02 MP_FAIL test, multiple subflows      syn[ ok ] - synack[ ok ] - ack[
>>>>> ok ]
>>>>>                                         sum[ ok ] - csum  [ ok ]
>>>>>                                         ftx[ ok ] - frx   [ ok ]
>>>>>                                         itx[ ok ] - irx   [ ok ]
>>>>> """
>>>>>
>>>>> The chk_join_nr output shows all [ ok ], which is good. But the file
>>>>> mismatches (I assume due to the inverted data) are failures because the
>>>>> checksums are intended to reject the bad data. Am I understanding the
>>>>> file mismatches correctly?
>>>>>
>>>>> If MP_FAIL is working, the files should match.
>>>>
>>>> I also have the same error on my side but not all the time.
>>>
>>> I got these file mismatches errors all the time. Could you please share
>>> your test script and the logs when you get the right output?
>>
>> I don't think I saw them for both tests all the time but it is possible
>> it was always there for the 1st one and not for the second one. I will
>> look at that next time.
>>
>>> The first test "01 MP_FAIL test, 1 subflow" will get the file mismatches
>>> all the time, since the checksum error data can't be retransmitted in
>>> the current codes.
>>
>> Should we disable this test then?
> 
> We should keep this test. It's useful for testing sending and receiving
> of the infinite mapping. The infinite mapping is only sent in this single
> subflow case.

We should then not check the output files if we expect not to receive
the full file. Or we should compare files up to the size of the
smallest. Because what has been given to the app should not be "corrupted".

>>> But the second test "02 MP_FAIL test, multiple subflows" shouldn't get
>>> the file mismatches. RST is sent on the checksum error subflow, so this
>>> subflow is closed. The data will be retransmitted on the other subflow.
>>> It works by using the old test case "[PATCH mptcp-next v7 8/8]
>>> DO-NOT-MERGE: mptcp: mp_fail test". Data will be retransmitted, and no
>>> the file mismatches. I don't know why it dosen't work by this new test
>>> case. In this test case, I saw the error data has been dropped, and the
>>> new data has been retransmitted in the log. I'll try to fix this.
>>
> 
> Could we merge patches 1-7 of this patchset into the export branch? Then
> I'll just iterate a new version of this test case patch. And if there's
> something I need to fix next week, I'll send the squash-to patch. Please
> discuss this with Mat in today's weekly meeting.

Sounds like a good point to me. I will wait for Mat's ACK about that!
I can raise the point at today's weekly meeting.

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

Re: [PATCH mptcp-next v8 0/8] The infinite mapping support

[Paolo Abeni]

On Fri, 2021-10-29 at 12:40 +0800, Geliang Tang wrote:
> v8:
>  - Patches 1-6 are unchanged, only updated the selftests scripts.
>  - The patch (Squash to "mptcp: infinite mapping receiving" for v7) is
> dropped too. Since this series only implemented MP_FAIL in one direction.
> The TODO items, "MP_FAIL echo" and "MP_FAIL retrans", will implement later
> as new patches.
> 
> v7:
>  - drop the patch "mptcp: add last_ack_dss_start in the msk" in v6
>  - set allow_infinite_fallback to false in __mptcp_subflow_connect and
>    mptcp_finish_join.
> 
> v6:
>  - use allow_infinite_fallback instead of last_retrans_seq.
>  - rename mptcp_is_data_contiguous to mptcp_allow_infinite_fallback.
>  - rename last_fully_acked_dss_start_seq to last_ack_dss_start.
> 
> v5:
>  - move last_retrans_seq from msk to mptcp_subflow_context
> 
> v4:
>  - update patch 1 and patch 2
> 
> v3:
>  - drop MPTCP_INFINITE_DONE flag
>  - drop MAPPING_INFINITE
>  - add mptcp_is_data_contiguous helper
>  - add the fallback check
>  - The u32 target testcase has not been completed yet.
> 
> v2:
>  - add MPTCP_INFINITE_DONE flag
>  - add MAPPING_INFINITE mapping status
>  - add start_seq in the msk
> 
> v1:
>  - add noncontiguous flag
>  - add the mibs check
>  - tag: export/20210904T080009
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/216
> 
> Geliang Tang (8):
>   mptcp: don't send RST for single subflow
>   mptcp: add the fallback check
>   mptcp: track and update contiguous data status
>   mptcp: infinite mapping sending
>   mptcp: infinite mapping receiving
>   mptcp: add mib for infinite map sending
>   selftests: mptcp: add infinite map mibs check
>   selftests: mptcp: add mp_fail testcases
> 
>  include/net/mptcp.h                           |   3 +-
>  net/mptcp/mib.c                               |   1 +
>  net/mptcp/mib.h                               |   1 +
>  net/mptcp/options.c                           |   2 +-
>  net/mptcp/pm.c                                |   6 +
>  net/mptcp/protocol.c                          |  22 ++++
>  net/mptcp/protocol.h                          |  13 ++
>  net/mptcp/subflow.c                           |  56 +++++----
>  tools/testing/selftests/net/mptcp/config      |   5 +
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
>  10 files changed, 186 insertions(+), 34 deletions(-)

I haven't reviewed the patch yet, but I have to say: nice new email
address :)

/P

Re: [PATCH mptcp-next v8 0/8] The infinite mapping support

[Geliang Tang]

On Fri, Oct 29, 2021 at 10:17:13AM +0200, Paolo Abeni wrote:
> On Fri, 2021-10-29 at 12:40 +0800, Geliang Tang wrote:
> > v8:
> >  - Patches 1-6 are unchanged, only updated the selftests scripts.
> >  - The patch (Squash to "mptcp: infinite mapping receiving" for v7) is
> > dropped too. Since this series only implemented MP_FAIL in one direction.
> > The TODO items, "MP_FAIL echo" and "MP_FAIL retrans", will implement later
> > as new patches.
> > 
> > v7:
> >  - drop the patch "mptcp: add last_ack_dss_start in the msk" in v6
> >  - set allow_infinite_fallback to false in __mptcp_subflow_connect and
> >    mptcp_finish_join.
> > 
> > v6:
> >  - use allow_infinite_fallback instead of last_retrans_seq.
> >  - rename mptcp_is_data_contiguous to mptcp_allow_infinite_fallback.
> >  - rename last_fully_acked_dss_start_seq to last_ack_dss_start.
> > 
> > v5:
> >  - move last_retrans_seq from msk to mptcp_subflow_context
> > 
> > v4:
> >  - update patch 1 and patch 2
> > 
> > v3:
> >  - drop MPTCP_INFINITE_DONE flag
> >  - drop MAPPING_INFINITE
> >  - add mptcp_is_data_contiguous helper
> >  - add the fallback check
> >  - The u32 target testcase has not been completed yet.
> > 
> > v2:
> >  - add MPTCP_INFINITE_DONE flag
> >  - add MAPPING_INFINITE mapping status
> >  - add start_seq in the msk
> > 
> > v1:
> >  - add noncontiguous flag
> >  - add the mibs check
> >  - tag: export/20210904T080009
> > 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/216
> > 
> > Geliang Tang (8):
> >   mptcp: don't send RST for single subflow
> >   mptcp: add the fallback check
> >   mptcp: track and update contiguous data status
> >   mptcp: infinite mapping sending
> >   mptcp: infinite mapping receiving
> >   mptcp: add mib for infinite map sending
> >   selftests: mptcp: add infinite map mibs check
> >   selftests: mptcp: add mp_fail testcases
> > 
> >  include/net/mptcp.h                           |   3 +-
> >  net/mptcp/mib.c                               |   1 +
> >  net/mptcp/mib.h                               |   1 +
> >  net/mptcp/options.c                           |   2 +-
> >  net/mptcp/pm.c                                |   6 +
> >  net/mptcp/protocol.c                          |  22 ++++
> >  net/mptcp/protocol.h                          |  13 ++
> >  net/mptcp/subflow.c                           |  56 +++++----
> >  tools/testing/selftests/net/mptcp/config      |   5 +
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 111 ++++++++++++++++--
> >  10 files changed, 186 insertions(+), 34 deletions(-)
> 
> I haven't reviewed the patch yet, but I have to say: nice new email
> address :)

Thanks :)

-Geliang
> 
> /P
> 

Re: [PATCH mptcp-next v8 0/8] The infinite mapping support

[Matthieu Baerts]

Hi Geliang, Mat,

On 29/10/2021 06:40, Geliang Tang wrote:
> v8:
>  - Patches 1-6 are unchanged, only updated the selftests scripts.
>  - The patch (Squash to "mptcp: infinite mapping receiving" for v7) is
> dropped too. Since this series only implemented MP_FAIL in one direction.
> The TODO items, "MP_FAIL echo" and "MP_FAIL retrans", will implement later
> as new patches.

(...)

Thank you for the patches and the reviews!

As discussed at the meeting yesterday, to help you moving forward, I
applied the first seven patches (without Mat's RvB tag for the moment).

What is then left to do if I'm not mistaken:

- drop more data from the receive queue c.f. discussions with Mat and
Christoph on the ML on v7

- Improvements around the tests for patch 8/8:
  - Comment around the TC command
  - Add 'tcp_flags 0x10/0xff'
  - Comment around the parsing (example of what you are parsing)
  - Break if jq returns 'null' or nothing.
  - Sleep in the while loop for "tc show"
  - Look for "pedit" counter ("packets" - "overlimits")
  - Not compare the output file if it is normal they are different
  - Remove/Reduce data sent by the host not having the tc command (send
only in one direction)
  - Insert the TC rules, then start the test, then look at counters
  - Force checksum=1 for "fail_tests()"


And for later:

- "MP_FAIL echo"

- "MP_FAIL retrans"

Did I miss anything?


The first patches are now in our tree (features for net-next):

- 19ca240f0cdf: mptcp: don't send RST for single subflow
- 89de2a435c1e: mptcp: add the fallback check
- df3d448153f1: mptcp: track and update contiguous data status
- 44b73ea648cc: mptcp: infinite mapping sending
- e35a1a951132: mptcp: infinite mapping receiving
- 9feb4bc20bf6: mptcp: add mib for infinite map sending
- ac67c5b1d612: selftests: mptcp: add infinite map mibs check
- Results: c1d29ad013b4..0fad1b3cf9ef


Builds and tests are now in progress:

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


> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/216

Because there are still stuff to implement, this ticket has not been closed.

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

[PATCH mptcp-next v8 0/8] Clarify when options can be used

[Matthieu Baerts]

This is a new version of Geliang's series called "send MP_FAIL with MP_RST and
others" (v6 + v7) but also include this time:

- "mptcp: fix a DSS option writting error" (v1)
- "selftests: mptcp: add mp_fail testcases" (v5)

On top of these two new patches from Geliang already sent on the ML, this new
version:

- includes one new patch 2/8 ("mptcp: fix opt size when sending DSS + MP_FAIL"):
  hopefuly fixing the issue reported by Geliang (MPF+DSS ok, DSS+MPF nok)
- has the Squash-to patch 1/8 split: the other part is in patch 4/8
  ("mptcp: reduce branching when writing MP_FAIL option")
- has one fix in patch 1/8 reported by Geliang

Notes: I still have issues with the mptcp_join selftests and TC but this can be
fixed later.

Geliang Tang (4):
  Squash to "mptcp: implement fastclose xmit path"
  mptcp: fix a DSS option writing error
  mptcp: print out reset infos of MP_RST
  selftests: mptcp: add mp_fail testcases

Matthieu Baerts (4):
  mptcp: fix opt size when sending DSS + MP_FAIL
  mptcp: reduce branching when writing MP_FAIL option
  mptcp: clarify when options can be used
  mptcp: allow sending both ADD_ADDR and RM_ADDR

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

-- 
2.33.1

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

[Matthieu Baerts]

From: Geliang Tang <geliang.tang@suse.com>

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.33.1