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

[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 1/8] Squash to "mptcp: implement fastclose xmit path"

[Matthieu Baerts]

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

MP_FAIL can be sent with RST or DSS, and FASTCLOSE can be sent with
RST too. So we should use a similar xmit logic for FASTCLOSE and
MP_FAIL in both mptcp_write_options() and mptcp_established_options*().

Cc: Paolo Abeni <pabeni@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>
---

Notes:
    v8:
     - 'ptr += 2;' was missing for FastClose (Geliang)
     - moving the MP_FAIL option is now in a dedicated patch
       ("mptcp: reduce branching when writing MP_FAIL option")

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

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index c6726e8389ec..aa3ed37bc59a 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -829,8 +829,12 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 
 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
 		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
+		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
+			*size += opt_size;
+			remaining -= opt_size;
+		}
+		/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
+		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
 			*size += opt_size;
 			remaining -= opt_size;
 		}
@@ -1460,19 +1464,23 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 				ptr += 1;
 			}
 		}
-	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
-		/* RST is mutually exclusive with everything else */
-		*ptr++ = mptcp_option(MPTCPOPT_RST,
-				      TCPOLEN_MPTCP_RST,
-				      opts->reset_transient,
-				      opts->reset_reason);
-		return;
 	} else if (unlikely(OPTION_MPTCP_FASTCLOSE & opts->suboptions)) {
-		/* FASTCLOSE is mutually exclusive with everything else */
+		/* FASTCLOSE is mutually exclusive with others except RST */
 		*ptr++ = mptcp_option(MPTCPOPT_MP_FASTCLOSE,
 				      TCPOLEN_MPTCP_FASTCLOSE,
 				      0, 0);
 		put_unaligned_be64(opts->rcvr_key, ptr);
+		ptr += 2;
+
+		if (OPTION_MPTCP_RST & opts->suboptions)
+			goto mp_rst;
+		return;
+	} else if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
+mp_rst:
+		*ptr++ = mptcp_option(MPTCPOPT_RST,
+				      TCPOLEN_MPTCP_RST,
+				      opts->reset_transient,
+				      opts->reset_reason);
 		return;
 	}
 
-- 
2.33.1

[PATCH mptcp-next v8 2/8] mptcp: fix opt size when sending DSS + MP_FAIL

[Matthieu Baerts]

When these two options had to be sent -- which is not common -- the DSS
size was not being taken into account in the remaining size.

Additionaly in this situation, the reported size was only the one of
the MP_FAIL which can cause issue if at the end, we need to write more
in the TCP options than previously said.

Here we use a dedicated variable for MP_FAIL size to keep the
WARN_ON_ONCE() just after.

Fixes: c25aeb4e0953 ("mptcp: MP_FAIL suboption sending")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index aa3ed37bc59a..36ab69906af3 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -845,10 +845,13 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 	if (mptcp_established_options_mp(sk, skb, snd_data_fin, &opt_size, remaining, opts))
 		ret = true;
 	else if (mptcp_established_options_dss(sk, skb, snd_data_fin, &opt_size, remaining, opts)) {
+		unsigned int mp_fail_size;
+
 		ret = true;
-		if (mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
-			*size += opt_size;
-			remaining -= opt_size;
+		if (mptcp_established_options_mp_fail(sk, &mp_fail_size,
+						      remaining - opt_size, opts)) {
+			*size += opt_size + mp_fail_size;
+			remaining -= opt_size - mp_fail_size;
 			return true;
 		}
 	}
-- 
2.33.1

[PATCH mptcp-next v8 3/8] mptcp: fix a DSS option writing error

[Matthieu Baerts]

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

'ptr += 1;' was omitted in the original code.

If the DSS is the last option -- which is what we have most of the
time -- that's not an issue.

Fixes: 1bff1e43a30e ("mptcp: optimize out option generation")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 36ab69906af3..bbf61dedb4b5 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1334,6 +1334,7 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 				put_unaligned_be32(mpext->data_len << 16 |
 						   TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
 			}
+			ptr += 1;
 		}
 	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
 		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
-- 
2.33.1

[PATCH mptcp-next v8 4/8] mptcp: reduce branching when writing MP_FAIL option

[Matthieu Baerts]

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

Here, we do the same in mptcp_write_options().

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

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index bbf61dedb4b5..2f94069808a4 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1264,21 +1264,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
 void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			 struct mptcp_out_options *opts)
 {
-	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
-		const struct sock *ssk = (const struct sock *)tp;
-		struct mptcp_subflow_context *subflow;
-
-		subflow = mptcp_subflow_ctx(ssk);
-		subflow->send_mp_fail = 0;
-
-		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
-				      TCPOLEN_MPTCP_FAIL,
-				      0, 0);
-		put_unaligned_be64(opts->fail_seq, ptr);
-		ptr += 2;
-	}
-
-	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
+	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
 	 * see mptcp_established_options*()
 	 */
 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
@@ -1336,6 +1322,10 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 			}
 			ptr += 1;
 		}
+
+		/* We might need to add MP_FAIL options in rare cases */
+		if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
+			goto mp_fail;
 	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
 		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
 
@@ -1476,6 +1466,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
 		put_unaligned_be64(opts->rcvr_key, ptr);
 		ptr += 2;
 
+		if (OPTION_MPTCP_RST & opts->suboptions)
+			goto mp_rst;
+		return;
+	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
+mp_fail:
+	{
+		/* MP_FAIL is mutually exclusive with others except RST */
+		const struct sock *ssk = (const struct sock *)tp;
+		struct mptcp_subflow_context *subflow;
+
+		subflow = mptcp_subflow_ctx(ssk);
+		subflow->send_mp_fail = 0;
+
+		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
+				      TCPOLEN_MPTCP_FAIL,
+				      0, 0);
+		put_unaligned_be64(opts->fail_seq, ptr);
+		ptr += 2;
+
 		if (OPTION_MPTCP_RST & opts->suboptions)
 			goto mp_rst;
 		return;
-- 
2.33.1

[PATCH mptcp-next v8 5/8] mptcp: clarify when options can be used

[Matthieu Baerts]

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

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

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

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

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

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

[PATCH mptcp-next v8 6/8] mptcp: allow sending both ADD_ADDR and RM_ADDR

[Matthieu Baerts]

This is mainly to be sync with what is done in mptcp_write_options()
where after having written the ADD_ADDR option, we check if the RM_ADDR
option also needs to be added.

But if we have room, it makes sense to write the two options in the same
packet. So best to remove the 'else'.

While at it, also mention that mptcp_established_options_add_addr() can
remove options.

Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 8553b928c5bf..06f64101d27b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -864,11 +864,14 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 
 	*size += opt_size;
 	remaining -= opt_size;
+	/* Note: this can remove previously set options in some conditions */
 	if (mptcp_established_options_add_addr(sk, skb, &opt_size, remaining, opts)) {
 		*size += opt_size;
 		remaining -= opt_size;
 		ret = true;
-	} else if (mptcp_established_options_rm_addr(sk, &opt_size, remaining, opts)) {
+	}
+
+	if (mptcp_established_options_rm_addr(sk, &opt_size, remaining, opts)) {
 		*size += opt_size;
 		remaining -= opt_size;
 		ret = true;
-- 
2.33.1

[PATCH mptcp-next v8 7/8] mptcp: print out reset infos of MP_RST

[Matthieu Baerts]

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

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

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

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 06f64101d27b..182fb885f5e6 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -336,6 +336,8 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		flags = *ptr++;
 		mp_opt->reset_transient = flags & MPTCP_RST_TRANSIENT;
 		mp_opt->reset_reason = *ptr;
+		pr_debug("MP_RST: transient=%u reason=%u",
+			 mp_opt->reset_transient, mp_opt->reset_reason);
 		break;
 
 	case MPTCPOPT_MP_FAIL:
-- 
2.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

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

[Geliang Tang]

Hi Matt,

Thank you so much for this new version. Good catch on patch 2. It works on
my test and all looks good to me...

Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>

... except patch 6.

I thinks we'd better to drop patch 6 ("mptcp: allow sending both ADD_ADDR
and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
efforts to let ADD_ADDR not to be sent with other subopthins. I thinks it's
better not to let ADD_ADDR sent with others by ourselves.

On the other hand, I'm not sure our code can deal with ADD_ADDR and RM_ADDR
together correctly, it's not tested.

WDYT?

Thanks,
-Geliang

Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年1月5日周三 23:57写道：
>
> 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
>
>

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

[Paolo Abeni]

On Thu, 2022-01-06 at 10:58 +0800, Geliang Tang wrote:
> Hi Matt,
> 
> Thank you so much for this new version. Good catch on patch 2. It works on
> my test and all looks good to me...
> 
> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
> 
> ... except patch 6.
> 
> I thinks we'd better to drop patch 6 ("mptcp: allow sending both ADD_ADDR
> and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
> efforts to let ADD_ADDR not to be sent with other subopthins. I thinks it's
> better not to let ADD_ADDR sent with others by ourselves.

+1

/P

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

[Matthieu Baerts]

Hi Geliang,

On 06/01/2022 03:58, Geliang Tang wrote:
> Hi Matt,
> 
> Thank you so much for this new version. Good catch on patch 2. It works on
> my test and all looks good to me...
> 
> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>

Great, thank you for having checked that!

> ... except patch 6.
> 
> I thinks we'd better to drop patch 6 ("mptcp: allow sending both ADD_ADDR
> and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
> efforts to let ADD_ADDR not to be sent with other subopthins. I thinks it's
> better not to let ADD_ADDR sent with others by ourselves.

ADD_ADDR suboption can be too big in some cases but not all the time.
If I remember well, we decided to have a dedicated ACK to simplify the
code: not having the ADD_ADDR removing the DSS options in some cases but
not all, also simplifying packetdrill tests.

Here, the idea is to add the RM_ADDR if asked and if there is room, just
like what we do in mptcp_write_options().

> On the other hand, I'm not sure our code can deal with ADD_ADDR and RM_ADDR
> together correctly, it's not tested.

Good point, we need a test validating that.

I'm not even sure we can have a test because each netlink command will
cause the corresponding option to be sent: depending on the timing, when
the next packet is sent, we might have one or two options to be sent.

If we drop patch 6, we need to replace it with a patch updating the new
comment at the beginning of mptcp_write_options() and not allowing
ADD_ADDR to be sent with RM_ADDR + MP_PRIO as we currently do.

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

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

[Matthieu Baerts]

Hi Geliang, Mat,

On 06/01/2022 03:58, Geliang Tang wrote:
> Hi Matt,
> 
> Thank you so much for this new version. Good catch on patch 2. It works on
> my test and all looks good to me...
> 
> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>

As discussed in another thread on the ML, I just applied the 3 first
patches:

- 62aa763b1676: "squashed" patch 1/8 in "mptcp: implement fastclose xmit
path"
- 5c6f43e1c2c0: "Signed-off-by" + "Co-developed-by"
- Results: 720eaaa70083..02e9b8087979

- 55c56c19c276: mptcp: fix opt size when sending DSS + MP_FAIL
- 025c1fcd8fa3: mptcp: fix a DSS option writing error
(- 73463b55be2a: mptcp: Check reclaim amount before reducing allocation)
- Results: 02e9b8087979..d500488d62db

Builds and tests are now in progress:

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

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

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

[Mat Martineau]

On Thu, 6 Jan 2022, Matthieu Baerts wrote:

> Hi Geliang, Mat,
>
> On 06/01/2022 03:58, Geliang Tang wrote:
>> Hi Matt,
>>
>> Thank you so much for this new version. Good catch on patch 2. It works on
>> my test and all looks good to me...
>>
>> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
>
> As discussed in another thread on the ML, I just applied the 3 first
> patches:
>
> - 62aa763b1676: "squashed" patch 1/8 in "mptcp: implement fastclose xmit
> path"
> - 5c6f43e1c2c0: "Signed-off-by" + "Co-developed-by"
> - Results: 720eaaa70083..02e9b8087979
>
> - 55c56c19c276: mptcp: fix opt size when sending DSS + MP_FAIL
> - 025c1fcd8fa3: mptcp: fix a DSS option writing error
> (- 73463b55be2a: mptcp: Check reclaim amount before reducing allocation)
> - Results: 02e9b8087979..d500488d62db
>

Thanks for applying these to the export branch so they could get tested 
(results look ok), that will help with getting them upstream this week.

> Builds and tests are now in progress:
>
> https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220106T114429
> https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v8 4/8] mptcp: reduce branching when writing MP_FAIL option

[Mat Martineau]

On Wed, 5 Jan 2022, Matthieu Baerts wrote:

> MP_FAIL should be use in very rare cases, either when the TCP RST flag
> is set -- with or without an MP_RST -- or with a DSS, see
> mptcp_established_options().
>
> Here, we do the same in mptcp_write_options().
>
> Co-developed-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> net/mptcp/options.c | 39 ++++++++++++++++++++++++---------------
> 1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index bbf61dedb4b5..2f94069808a4 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1264,21 +1264,7 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 			 struct mptcp_out_options *opts)
> {
> -	if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
> -		const struct sock *ssk = (const struct sock *)tp;
> -		struct mptcp_subflow_context *subflow;
> -
> -		subflow = mptcp_subflow_ctx(ssk);
> -		subflow->send_mp_fail = 0;
> -
> -		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
> -				      TCPOLEN_MPTCP_FAIL,
> -				      0, 0);
> -		put_unaligned_be64(opts->fail_seq, ptr);
> -		ptr += 2;
> -	}
> -
> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually exclusive,
> +	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,

Seems like the comment was ok as-is? As you note in the commit message, 
DSS and FAIL can occur together.

> 	 * see mptcp_established_options*()
> 	 */
> 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> @@ -1336,6 +1322,10 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 			}
> 			ptr += 1;
> 		}
> +
> +		/* We might need to add MP_FAIL options in rare cases */
> +		if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
> +			goto mp_fail;
> 	} else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
> 		u8 len, flag = MPTCP_CAP_HMAC_SHA256;
>
> @@ -1476,6 +1466,25 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 		put_unaligned_be64(opts->rcvr_key, ptr);
> 		ptr += 2;
>
> +		if (OPTION_MPTCP_RST & opts->suboptions)
> +			goto mp_rst;
> +		return;
> +	} else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
> +mp_fail:
> +	{

Formatting quibble: seems better to put the '{' at the end of the line 
before the mp_fail label, like the existing code does just before the 
mp_rst label just below this block of code.

> +		/* MP_FAIL is mutually exclusive with others except RST */
> +		const struct sock *ssk = (const struct sock *)tp;
> +		struct mptcp_subflow_context *subflow;
> +
> +		subflow = mptcp_subflow_ctx(ssk);
> +		subflow->send_mp_fail = 0;
> +
> +		*ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
> +				      TCPOLEN_MPTCP_FAIL,
> +				      0, 0);
> +		put_unaligned_be64(opts->fail_seq, ptr);
> +		ptr += 2;
> +
> 		if (OPTION_MPTCP_RST & opts->suboptions)
> 			goto mp_rst;
> 		return;
> -- 
> 2.33.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v8 5/8] mptcp: clarify when options can be used

[Mat Martineau]

On Wed, 5 Jan 2022, Matthieu Baerts wrote:

> RFC8684 doesn't seem to clearly specify which MPTCP options can be used
> together.
>
> Some options are mutually exclusive -- e.g. MP_CAPABLE and MP_JOIN --,
> some can be used together -- e.g. DSS + MP_PRIO --, some can but we
> prefer not to -- e.g. DSS + ADD_ADDR -- and some have to be used
> together at some points -- e.g. MP_FAIL and DSS.
>
> We need to clarify this as a base before allowing other modifications.
>
> For example, does it make sense to send a RM_ADDR with an MPC or MPJ?
> This remains open for possible future discussions.
>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> net/mptcp/options.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 2f94069808a4..8553b928c5bf 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1264,8 +1264,27 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 			 struct mptcp_out_options *opts)
> {
> -	/* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually exclusive,
> -	 * see mptcp_established_options*()
> +	/* Which options can be used together?
> +	 *
> +	 * X: mutually exclusive
> +	 * O: often used together
> +	 * C: can be used together in some cases
> +	 * P: could be used together but we prefer not to (optimisations)
> +	 *
> +	 *  Opt: | MPC  | MPJ  | DSS  | ADD  |  RM  | PRIO | FAIL |  FC  |
> +	 * ------|------|------|------|------|------|------|------|------|
> +	 *  MPC  |------|------|------|------|------|------|------|------|
> +	 *  MPJ  |  X   |------|------|------|------|------|------|------|
> +	 *  DSS  |  X   |  X   |------|------|------|------|------|------|
> +	 *  ADD  |  X   |  X   |  P   |------|------|------|------|------|
> +	 *  RM   |  C   |  C   |  C   |  C   |------|------|------|------|
> +	 *  PRIO |  X   |  C   |  C   |  C   |  C   |------|------|------|
> +	 *  FAIL |  X   |  X   |  C   |  X   |  X   |  X   |------|------|
> +	 *  FC   |  X   |  X   |  X   |  X   |  X   |  X   |  X   |------|
> +	 *  RST  |  X   |  X   |  X   |  X   |  X   |  X   |  O   |  O   |
> +	 * ------|------|------|------|------|------|------|------|------|
> +	 *
> +	 * The same applies in mptcp_established_options() function.

This is very helpful! (and makes my comment on the previous patch 
irrelevant :) )

> 	 */
> 	if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> 		struct mptcp_ext *mpext = &opts->ext_copy;
> -- 
> 2.33.1
>
>
>

--
Mat Martineau
Intel

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

[Mat Martineau]

On Thu, 6 Jan 2022, Matthieu Baerts wrote:

> Hi Geliang,
>
> On 06/01/2022 03:58, Geliang Tang wrote:
>> Hi Matt,
>>
>> Thank you so much for this new version. Good catch on patch 2. It works on
>> my test and all looks good to me...
>>
>> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
>
> Great, thank you for having checked that!
>
>> ... except patch 6.
>>
>> I thinks we'd better to drop patch 6 ("mptcp: allow sending both ADD_ADDR
>> and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
>> efforts to let ADD_ADDR not to be sent with other subopthins. I thinks it's
>> better not to let ADD_ADDR sent with others by ourselves.
>
> ADD_ADDR suboption can be too big in some cases but not all the time.
> If I remember well, we decided to have a dedicated ACK to simplify the
> code: not having the ADD_ADDR removing the DSS options in some cases but
> not all, also simplifying packetdrill tests.
>
> Here, the idea is to add the RM_ADDR if asked and if there is room, just
> like what we do in mptcp_write_options().
>
>> On the other hand, I'm not sure our code can deal with ADD_ADDR and RM_ADDR
>> together correctly, it's not tested.
>
> Good point, we need a test validating that.
>

Sounds like a job for packetdrill!

> I'm not even sure we can have a test because each netlink command will
> cause the corresponding option to be sent: depending on the timing, when
> the next packet is sent, we might have one or two options to be sent.
>
> If we drop patch 6, we need to replace it with a patch updating the new
> comment at the beginning of mptcp_write_options() and not allowing
> ADD_ADDR to be sent with RM_ADDR + MP_PRIO as we currently do.
>

I think it would be good to drop patch 6 and updating the comment as 
Matthieu says.

I'm still seeing the file content mismatch with the "MP_FAIL, multiple 
subflows" test, which doesn't seem right. Will try to look into what's 
going on there next week.

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v8 4/8] mptcp: reduce branching when writing MP_FAIL option

[Matthieu Baerts]

Hi Mat,

Thank you for the review!

On 08/01/2022 01:54, Mat Martineau wrote:
> On Wed, 5 Jan 2022, Matthieu Baerts wrote:
> 
>> MP_FAIL should be use in very rare cases, either when the TCP RST flag
>> is set -- with or without an MP_RST -- or with a DSS, see
>> mptcp_established_options().
>>
>> Here, we do the same in mptcp_write_options().
>>
>> Co-developed-by: Geliang Tang <geliang.tang@suse.com>
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>> ---
>> net/mptcp/options.c | 39 ++++++++++++++++++++++++---------------
>> 1 file changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
>> index bbf61dedb4b5..2f94069808a4 100644
>> --- a/net/mptcp/options.c
>> +++ b/net/mptcp/options.c
>> @@ -1264,21 +1264,7 @@ static u16 mptcp_make_csum(const struct
>> mptcp_ext *mpext)
>> void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
>>              struct mptcp_out_options *opts)
>> {
>> -    if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions)) {
>> -        const struct sock *ssk = (const struct sock *)tp;
>> -        struct mptcp_subflow_context *subflow;
>> -
>> -        subflow = mptcp_subflow_ctx(ssk);
>> -        subflow->send_mp_fail = 0;
>> -
>> -        *ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>> -                      TCPOLEN_MPTCP_FAIL,
>> -                      0, 0);
>> -        put_unaligned_be64(opts->fail_seq, ptr);
>> -        ptr += 2;
>> -    }
>> -
>> -    /* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and RST are mutually
>> exclusive,
>> +    /* DSS, MPC, MPJ, ADD_ADDR, FASTCLOSE and FAIL are mutually
>> exclusive,
> 
> Seems like the comment was ok as-is? As you note in the commit message,
> DSS and FAIL can occur together.

Yes indeed, in some conditions.
I removed this comment after but it is true we should not modify it here.

I see Geliang fixed that in the v9, thanks.

> 
>>      * see mptcp_established_options*()
>>      */
>>     if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
>> @@ -1336,6 +1322,10 @@ void mptcp_write_options(__be32 *ptr, const
>> struct tcp_sock *tp,
>>             }
>>             ptr += 1;
>>         }
>> +
>> +        /* We might need to add MP_FAIL options in rare cases */
>> +        if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
>> +            goto mp_fail;
>>     } else if (OPTIONS_MPTCP_MPC & opts->suboptions) {
>>         u8 len, flag = MPTCP_CAP_HMAC_SHA256;
>>
>> @@ -1476,6 +1466,25 @@ void mptcp_write_options(__be32 *ptr, const
>> struct tcp_sock *tp,
>>         put_unaligned_be64(opts->rcvr_key, ptr);
>>         ptr += 2;
>>
>> +        if (OPTION_MPTCP_RST & opts->suboptions)
>> +            goto mp_rst;
>> +        return;
>> +    } else if (unlikely(OPTION_MPTCP_FAIL & opts->suboptions))
>> +mp_fail:
>> +    {
> 
> Formatting quibble: seems better to put the '{' at the end of the line
> before the mp_fail label, like the existing code does just before the
> mp_rst label just below this block of code.

I had to do that if I wanted to keep declaring the two "const" variables
just below, only in case of MP_FAIL.

The other solution is to always declare them at the beginning of the
function like Geliang did in the v9.

Cheers,
Matt

> 
>> +        /* MP_FAIL is mutually exclusive with others except RST */
>> +        const struct sock *ssk = (const struct sock *)tp;
>> +        struct mptcp_subflow_context *subflow;
>> +
>> +        subflow = mptcp_subflow_ctx(ssk);
>> +        subflow->send_mp_fail = 0;
>> +
>> +        *ptr++ = mptcp_option(MPTCPOPT_MP_FAIL,
>> +                      TCPOLEN_MPTCP_FAIL,
>> +                      0, 0);
>> +        put_unaligned_be64(opts->fail_seq, ptr);
>> +        ptr += 2;
>> +
>>         if (OPTION_MPTCP_RST & opts->suboptions)
>>             goto mp_rst;
>>         return;
>> -- 
>> 2.33.1
>>
>>
>>
> 
> -- 
> Mat Martineau
> Intel

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

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

[Matthieu Baerts]

On 08/01/2022 02:19, Mat Martineau wrote:
> On Thu, 6 Jan 2022, Matthieu Baerts wrote:
> 
>> Hi Geliang,
>>
>> On 06/01/2022 03:58, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thank you so much for this new version. Good catch on patch 2. It
>>> works on
>>> my test and all looks good to me...
>>>
>>> Acked-and-tested-by: Geliang Tang <geliang.tang@suse.com>
>>
>> Great, thank you for having checked that!
>>
>>> ... except patch 6.
>>>
>>> I thinks we'd better to drop patch 6 ("mptcp: allow sending both
>>> ADD_ADDR
>>> and RM_ADDR"). Since ADD_ADDR suboption is too big, we had made a lot of
>>> efforts to let ADD_ADDR not to be sent with other subopthins. I
>>> thinks it's
>>> better not to let ADD_ADDR sent with others by ourselves.
>>
>> ADD_ADDR suboption can be too big in some cases but not all the time.
>> If I remember well, we decided to have a dedicated ACK to simplify the
>> code: not having the ADD_ADDR removing the DSS options in some cases but
>> not all, also simplifying packetdrill tests.
>>
>> Here, the idea is to add the RM_ADDR if asked and if there is room, just
>> like what we do in mptcp_write_options().
>>
>>> On the other hand, I'm not sure our code can deal with ADD_ADDR and
>>> RM_ADDR
>>> together correctly, it's not tested.
>>
>> Good point, we need a test validating that.
>>
> 
> Sounds like a job for packetdrill!

Yes but still, we need a netlink command to ask the kernel sending an
ADD_ADDR/RM_ADDR/MP_PRIO, one at a time.
I don't think we can have a proper test always making the kernel sending
a packet with two notifications, no?

>> I'm not even sure we can have a test because each netlink command will
>> cause the corresponding option to be sent: depending on the timing, when
>> the next packet is sent, we might have one or two options to be sent.
>>
>> If we drop patch 6, we need to replace it with a patch updating the new
>> comment at the beginning of mptcp_write_options() and not allowing
>> ADD_ADDR to be sent with RM_ADDR + MP_PRIO as we currently do.
>>
> 
> I think it would be good to drop patch 6 and updating the comment as
> Matthieu says.

I think we should also modify the code in "mptcp_write_options()" and in
"mptcp_established_options()" to reflect that.

Maybe it would simplify things to say: we can send an RM_ADDR and
MP_PRIO only with a DSS, one at a time. That we can validate (we already
do) and I don't think we will need to send in the same TCP packet an
ADD_ADDR + RM_ADDR + MP_PRIO.

That would replace a lot of "C" (can) by "P" (prefer not) in the
previous comment and reduce cases to manage.

WDYT?

> 
> I'm still seeing the file content mismatch with the "MP_FAIL, multiple
> subflows" test, which doesn't seem right. Will try to look into what's
> going on there next week.

Same here.

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