[PATCH mptcp-next v14 0/7] userspace pm remove id 0 subflow & address

[PATCH mptcp-next v14 0/7] userspace pm remove id 0 subflow & address

[Geliang Tang]

v14:
 - reuse MPTCP_CF_FASTCLOSE flag.
 - use tcp_shutdown instead of mptcp_subflow_shutdown.

v13:
 - invoke mptcp_subflow_shutdown() in patch 1 as Paolo suggested.

v12:
 - address Matt's comments in v11.

v11:
 - avoid sending RSTs.
 - rename 'id 0 subflow' to 'inital subflow'.

Geliang Tang (5):
  mptcp: add __mptcp_subflow_disconnect helper
  selftests: mptcp: userspace pm remove initial subflow
  mptcp: userspace pm send RM_ADDR for ID 0
  mptcp: userspace pm rename remove_err to out
  selftests: mptcp: userspace pm send RM_ADDR for ID 0

Matthieu Baerts (2):
  selftests: mptcp: join: correctly check for no RST
  selftests: mptcp: join: no RST when rm subflow/addr

 net/mptcp/pm_userspace.c                      | 45 +++++++++++-
 net/mptcp/protocol.c                          | 28 ++++++--
 .../testing/selftests/net/mptcp/mptcp_join.sh | 68 ++++++++++++++++++-
 3 files changed, 130 insertions(+), 11 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v14 1/7] selftests: mptcp: join: correctly check for no RST

[Geliang Tang]

From: Matthieu Baerts <matttbe@kernel.org>

The commit mentioned below was more tolerant with the number of RST seen
during a test because in some uncontrollable situations, multiple RST
can be generated.

But it was not taking into account the case where no RST are expected:
this validation was then no longer reporting issues for the 0 RST case
because it is not possible to have less than 0 RST in the counter. This
patch fixes the issue by adding a specific condition.

Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose test-cases")
Signed-off-by: Matthieu Baerts <matttbe@kernel.org>
Reviewed-by: Mat Martineau <martineau@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ae38b428e42e..01480663c102 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -1432,7 +1432,9 @@ chk_rst_nr()
 	count=$(get_counter ${ns_tx} "MPTcpExtMPRstTx")
 	if [ -z "$count" ]; then
 		print_skip
-	elif [ $count -lt $rst_tx ]; then
+	# accept more rst than expected except if we don't expect any
+	elif { [ $rst_tx -ne 0 ] && [ $count -lt $rst_tx ]; } ||
+	     { [ $rst_tx -eq 0 ] && [ $count -ne 0 ]; }; then
 		fail_test "got $count MP_RST[s] TX expected $rst_tx"
 	else
 		print_ok
@@ -1442,7 +1444,9 @@ chk_rst_nr()
 	count=$(get_counter ${ns_rx} "MPTcpExtMPRstRx")
 	if [ -z "$count" ]; then
 		print_skip
-	elif [ "$count" -lt "$rst_rx" ]; then
+	# accept more rst than expected except if we don't expect any
+	elif { [ $rst_rx -ne 0 ] && [ $count -lt $rst_rx ]; } ||
+	     { [ $rst_rx -eq 0 ] && [ $count -ne 0 ]; }; then
 		fail_test "got $count MP_RST[s] RX expected $rst_rx"
 	else
 		print_ok
-- 
2.35.3

[PATCH mptcp-next v14 2/7] selftests: mptcp: join: no RST when rm subflow/addr

[Geliang Tang]

From: Matthieu Baerts <matttbe@kernel.org>

Recently, we noticed that some RST were wrongly generated when removing
the initial subflow.

This patch makes sure RST are not sent when removing any subflows or any
addresses.

Signed-off-by: Matthieu Baerts <matttbe@kernel.org>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 01480663c102..ab6908b7b143 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -2343,6 +2343,7 @@ remove_tests()
 		chk_join_nr 1 1 1
 		chk_rm_tx_nr 1
 		chk_rm_nr 1 1
+		chk_rst_nr 0 0
 	fi
 
 	# multiple subflows, remove
@@ -2355,6 +2356,7 @@ remove_tests()
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 2 2 2
 		chk_rm_nr 2 2
+		chk_rst_nr 0 0
 	fi
 
 	# single address, remove
@@ -2367,6 +2369,7 @@ remove_tests()
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
 		chk_rm_nr 1 1 invert
+		chk_rst_nr 0 0
 	fi
 
 	# subflow and signal, remove
@@ -2380,6 +2383,7 @@ remove_tests()
 		chk_join_nr 2 2 2
 		chk_add_nr 1 1
 		chk_rm_nr 1 1
+		chk_rst_nr 0 0
 	fi
 
 	# subflows and signal, remove
@@ -2394,6 +2398,7 @@ remove_tests()
 		chk_join_nr 3 3 3
 		chk_add_nr 1 1
 		chk_rm_nr 2 2
+		chk_rst_nr 0 0
 	fi
 
 	# addresses remove
@@ -2408,6 +2413,7 @@ remove_tests()
 		chk_join_nr 3 3 3
 		chk_add_nr 3 3
 		chk_rm_nr 3 3 invert
+		chk_rst_nr 0 0
 	fi
 
 	# invalid addresses remove
@@ -2422,6 +2428,7 @@ remove_tests()
 		chk_join_nr 1 1 1
 		chk_add_nr 3 3
 		chk_rm_nr 3 1 invert
+		chk_rst_nr 0 0
 	fi
 
 	# subflows and signal, flush
@@ -2436,6 +2443,7 @@ remove_tests()
 		chk_join_nr 3 3 3
 		chk_add_nr 1 1
 		chk_rm_nr 1 3 invert simult
+		chk_rst_nr 0 0
 	fi
 
 	# subflows flush
@@ -2455,6 +2463,7 @@ remove_tests()
 		else
 			chk_rm_nr 3 3
 		fi
+		chk_rst_nr 0 0
 	fi
 
 	# addresses flush
@@ -2469,6 +2478,7 @@ remove_tests()
 		chk_join_nr 3 3 3
 		chk_add_nr 3 3
 		chk_rm_nr 3 3 invert simult
+		chk_rst_nr 0 0
 	fi
 
 	# invalid addresses flush
@@ -2483,6 +2493,7 @@ remove_tests()
 		chk_join_nr 1 1 1
 		chk_add_nr 3 3
 		chk_rm_nr 3 1 invert
+		chk_rst_nr 0 0
 	fi
 
 	# remove id 0 subflow
@@ -2494,6 +2505,7 @@ remove_tests()
 			run_tests $ns1 $ns2 10.0.1.1
 		chk_join_nr 1 1 1
 		chk_rm_nr 1 1
+		chk_rst_nr 0 0 ## TODO: currently failing, Geliang is working on a fix
 	fi
 
 	# remove id 0 address
@@ -2506,6 +2518,7 @@ remove_tests()
 		chk_join_nr 1 1 1
 		chk_add_nr 1 1
 		chk_rm_nr 1 1 invert
+		chk_rst_nr 0 0 invert ## TODO: currently failing, Geliang is working on a fix
 	fi
 }
 
-- 
2.35.3

[PATCH mptcp-next v14 3/7] mptcp: add __mptcp_subflow_disconnect helper

[Geliang Tang]

When closing the msk->first socket in __mptcp_close_ssk(), if there's
another subflow available, it's better to avoid resetting it, just shut
down it.

This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
we invoke tcp_shutdown() instead of tcp_disconnect().

Co-developed-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 30e0c29ae0a4..1a54d55f8bb2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
 #define MPTCP_CF_PUSH		BIT(1)
 #define MPTCP_CF_FASTCLOSE	BIT(2)
 
+/* be sure to send a reset only if the caller asked for it, also
+ * clean completely the subflow status when the subflow reaches
+ * TCP_CLOSE state
+ */
+static void __mptcp_subflow_disconnect(struct sock *ssk,
+				       struct mptcp_subflow_context *subflow,
+				       unsigned int flags)
+{
+	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
+	    (flags & MPTCP_CF_FASTCLOSE)) {
+		/* The MPTCP code never wait on the subflow sockets, TCP-level
+		 * disconnect should never fail
+		 */
+		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
+		mptcp_subflow_ctx_reset(subflow);
+	} else {
+		tcp_shutdown(ssk, SEND_SHUTDOWN);
+	}
+}
+
 /* subflow sockets can be either outgoing (connect) or incoming
  * (accept).
  *
@@ -2403,7 +2423,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
 
 	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
-		/* be sure to force the tcp_disconnect() path,
+		/* be sure to force the tcp_close path
 		 * to generate the egress reset
 		 */
 		ssk->sk_lingertime = 0;
@@ -2413,11 +2433,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
 
 	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
 	if (!dispose_it) {
-		/* The MPTCP code never wait on the subflow sockets, TCP-level
-		 * disconnect should never fail
-		 */
-		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
-		mptcp_subflow_ctx_reset(subflow);
+		__mptcp_subflow_disconnect(ssk, subflow, flags);
 		release_sock(ssk);
 
 		goto out;
-- 
2.35.3

[PATCH mptcp-next v14 4/7] selftests: mptcp: userspace pm remove initial subflow

[Geliang Tang]

This patch adds a selftest for userpsace PM to remove the initial
subflow. Use userspace_pm_add_sf() to add a subflow, and pass initial
ip address to userspace_pm_rm_sf() to remove the initial subflow.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ab6908b7b143..e1304383057c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3507,6 +3507,28 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm remove initial subflow
+	if reset_with_events "userspace pm remove initial subflow" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+		set_userspace_pm $ns2
+		pm_nl_set_limits $ns1 0 1
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns2
+		userspace_pm_add_sf $ns2 10.0.3.2 20
+		chk_join_nr 1 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		userspace_pm_rm_sf $ns2 10.0.1.2
+		chk_rm_nr 0 1
+		chk_rst_nr 0 0 invert
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

[PATCH mptcp-next v14 5/7] mptcp: userspace pm send RM_ADDR for ID 0

[Geliang Tang]

This patch adds the ability to send RM_ADDR for local ID 0. Check
whether id 0 address is removed, if not, put id 0 into a removing
list, pass it to mptcp_pm_remove_addr() to remove id 0 address.

There is no reason not to allow the userspace to remove the initial
address (ID 0). This special case was not taken into account not
letting the userspace to delete all addresses as announced.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6b8083650bc1..ea50e694125d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -211,6 +211,40 @@ int mptcp_pm_nl_announce_doit(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
+static int mptcp_userspace_remove_id_zero_address(struct mptcp_sock *msk,
+						  struct genl_info *info)
+{
+	struct mptcp_rm_list list = { .nr = 0 };
+	struct mptcp_subflow_context *subflow;
+	struct sock *sk = (struct sock *)msk;
+	bool has_id_0 = false;
+	int err = -EINVAL;
+
+	lock_sock(sk);
+	mptcp_for_each_subflow(msk, subflow) {
+		if (subflow->local_id == 0) {
+			has_id_0 = true;
+			break;
+		}
+	}
+	if (!has_id_0) {
+		GENL_SET_ERR_MSG(info, "address with id 0 not found");
+		goto remove_err;
+	}
+
+	list.ids[list.nr++] = 0;
+
+	spin_lock_bh(&msk->pm.lock);
+	mptcp_pm_remove_addr(msk, &list);
+	spin_unlock_bh(&msk->pm.lock);
+
+	err = 0;
+
+remove_err:
+	release_sock(sk);
+	return err;
+}
+
 int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
@@ -245,6 +279,11 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 		goto remove_err;
 	}
 
+	if (id_val == 0) {
+		err = mptcp_userspace_remove_id_zero_address(msk, info);
+		goto remove_err;
+	}
+
 	lock_sock(sk);
 
 	list_for_each_entry(entry, &msk->pm.userspace_pm_local_addr_list, list) {
-- 
2.35.3

[PATCH mptcp-next v14 6/7] mptcp: userspace pm rename remove_err to out

[Geliang Tang]

The value of 'err' will be not only '-EINVAL', but alse '0' most of the
time. So it's better to rename the lable 'remove_err' to 'out'.

Suggested-by: Matthieu Baerts <matttbe@kernel.org>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index ea50e694125d..cdff3e631d2d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -276,12 +276,12 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 
 	if (!mptcp_pm_is_userspace(msk)) {
 		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
-		goto remove_err;
+		goto out;
 	}
 
 	if (id_val == 0) {
 		err = mptcp_userspace_remove_id_zero_address(msk, info);
-		goto remove_err;
+		goto out;
 	}
 
 	lock_sock(sk);
@@ -296,7 +296,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 	if (!match) {
 		GENL_SET_ERR_MSG(info, "address with specified id not found");
 		release_sock(sk);
-		goto remove_err;
+		goto out;
 	}
 
 	list_move(&match->list, &free_list);
@@ -310,7 +310,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	err = 0;
- remove_err:
+out:
 	sock_put(sk);
 	return err;
 }
-- 
2.35.3

[PATCH mptcp-next v14 7/7] selftests: mptcp: userspace pm send RM_ADDR for ID 0

[Geliang Tang]

This patch adds a selftest for userpsace PM to remove id 0 address.
Use userspace_pm_add_addr() helper to add a id 10 address, then use
userspace_pm_rm_addr() helper to remove id 0 address.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index e1304383057c..2233b24687cf 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3529,6 +3529,31 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm send RM_ADDR for ID 0
+	if reset_with_events "userspace pm send RM_ADDR for ID 0" &&
+	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+		set_userspace_pm $ns1
+		pm_nl_set_limits $ns2 1 1
+		speed=10 \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns1
+		userspace_pm_add_addr $ns1 10.0.2.1 10
+		chk_join_nr 1 1 1
+		chk_add_nr 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
+		userspace_pm_rm_addr $ns1 0
+		sleep 0.5
+		chk_rm_nr 1 0 invert
+		chk_rst_nr 0 0 invert
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows_total 2 2
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: selftests: mptcp: userspace pm send RM_ADDR for ID 0: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- {"code":404,"message":
  - "Can't find artifacts containing file conclusion.txt"}:
  - Task: https://cirrus-ci.com/task/6688622972239872
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6688622972239872/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6432463807840256
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6432463807840256/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5306563900997632
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5306563900997632/summary/summary.txt

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

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/a5d21f5759cf


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: selftests: mptcp: userspace pm send RM_ADDR for ID 0: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5994835887259648
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5994835887259648/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6432463807840256
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6432463807840256/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5306563900997632
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5306563900997632/summary/summary.txt

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

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/a5d21f5759cf


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

Re: [PATCH mptcp-next v14 4/7] selftests: mptcp: userspace pm remove initial subflow

[Matthieu Baerts]

Hi Geliang,

On 11/10/2023 15:34, Geliang Tang wrote:
> This patch adds a selftest for userpsace PM to remove the initial
> subflow. Use userspace_pm_add_sf() to add a subflow, and pass initial
> ip address to userspace_pm_rm_sf() to remove the initial subflow.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ab6908b7b143..e1304383057c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3507,6 +3507,28 @@ userspace_tests()
>  		kill_events_pids
>  		wait $tests_pid
>  	fi
> +
> +	# userspace pm remove initial subflow
> +	if reset_with_events "userspace pm remove initial subflow" &&
> +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> +		set_userspace_pm $ns2
> +		pm_nl_set_limits $ns1 0 1
> +		speed=10 \
> +			run_tests $ns1 $ns2 10.0.1.1 &
> +		local tests_pid=$!
> +		wait_mpj $ns2
> +		userspace_pm_add_sf $ns2 10.0.3.2 20
> +		chk_join_nr 1 1 1
> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_subflows_total 2 2
> +		userspace_pm_rm_sf $ns2 10.0.1.2

If this command removes the initial subflow ...

> +		chk_rm_nr 0 1

(it is a bit confusing, we could think there are RM_ADDR being sent
here. If you need to change something else, can you add a comment here
that we don't look at the counter linked to the RM_ADDR but to the one
linked to the subflows that have been removed please?)

> +		chk_rst_nr 0 0 invert
> +		chk_mptcp_info subflows 1 subflows 1

... that's normal to still have 1 "extra subflow" here but ...

> +		chk_subflows_total 2 2

... this should be "1 1", no? One subflow has been removed in between,
so we should not have the same counter as before, no?

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

Re: [PATCH mptcp-next v14 7/7] selftests: mptcp: userspace pm send RM_ADDR for ID 0

[Matthieu Baerts]

Hi Geliang,

On 11/10/2023 15:35, Geliang Tang wrote:
> This patch adds a selftest for userpsace PM to remove id 0 address.
> Use userspace_pm_add_addr() helper to add a id 10 address, then use
> userspace_pm_rm_addr() helper to remove id 0 address.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index e1304383057c..2233b24687cf 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3529,6 +3529,31 @@ userspace_tests()
>  		kill_events_pids
>  		wait $tests_pid
>  	fi
> +
> +	# userspace pm send RM_ADDR for ID 0
> +	if reset_with_events "userspace pm send RM_ADDR for ID 0" &&
> +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> +		set_userspace_pm $ns1
> +		pm_nl_set_limits $ns2 1 1
> +		speed=10 \
> +			run_tests $ns1 $ns2 10.0.1.1 &
> +		local tests_pid=$!
> +		wait_mpj $ns1
> +		userspace_pm_add_addr $ns1 10.0.2.1 10
> +		chk_join_nr 1 1 1
> +		chk_add_nr 1 1
> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_subflows_total 2 2
> +		chk_mptcp_info add_addr_signal 1 add_addr_accepted 1
> +		userspace_pm_rm_addr $ns1 0

If this commands forces the server to send a RM_ADDR to the client and
the client, using the Netlink PM, reacts as expected by closing the
initial subflow ...

> +		sleep 0.5

(Maybe move this sleep to userspace_pm_rm_addr()? (even if the best is
to wait for an event...))

> +		chk_rm_nr 1 0 invert
> +		chk_rst_nr 0 0 invert
> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_subflows_total 2 2

... should we not have one subflow here? (and one removed subflow with
'chk_rm_nr 1 1 invert')

Or maybe the client doesn't react by removing all subflows linked to the
address ID that is given in the RM_ADDR? In this case, we have another
issue :) (in the Netlink PM). We can create a new ticket if that's
easier not to block this series.

Or do we have an issue with the counter displaying 2 while it should be
1? Or an issue with patch 3/7 not closing the subflow as expected?

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

Re: [PATCH mptcp-next v14 3/7] mptcp: add __mptcp_subflow_disconnect helper

[Matthieu Baerts]

Hi Geliang, Paolo,

On 11/10/2023 15:34, Geliang Tang wrote:
> When closing the msk->first socket in __mptcp_close_ssk(), if there's
> another subflow available, it's better to avoid resetting it, just shut
> down it.
> 
> This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
> flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
> we invoke tcp_shutdown() instead of tcp_disconnect().
> 
> Co-developed-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 30e0c29ae0a4..1a54d55f8bb2 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
>  #define MPTCP_CF_PUSH		BIT(1)
>  #define MPTCP_CF_FASTCLOSE	BIT(2)
>  
> +/* be sure to send a reset only if the caller asked for it, also
> + * clean completely the subflow status when the subflow reaches
> + * TCP_CLOSE state
> + */
> +static void __mptcp_subflow_disconnect(struct sock *ssk,
> +				       struct mptcp_subflow_context *subflow,
> +				       unsigned int flags)
> +{
> +	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
> +	    (flags & MPTCP_CF_FASTCLOSE)) {
> +		/* The MPTCP code never wait on the subflow sockets, TCP-level
> +		 * disconnect should never fail
> +		 */
> +		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> +		mptcp_subflow_ctx_reset(subflow);
> +	} else {
> +		tcp_shutdown(ssk, SEND_SHUTDOWN);

I didn't check in detail but should we not also call
__mptcp_subflow_error_report()?

And maybe other helpers? What is done between the 'goto out' and the
'out' label in __mptcp_close_ssk() here below? → what we do when other
subflows are being closed.

Cheers,
Matt

> +	}
> +}
> +
>  /* subflow sockets can be either outgoing (connect) or incoming
>   * (accept).
>   *
> @@ -2403,7 +2423,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
>  
>  	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
> -		/* be sure to force the tcp_disconnect() path,
> +		/* be sure to force the tcp_close path
>  		 * to generate the egress reset
>  		 */
>  		ssk->sk_lingertime = 0;
> @@ -2413,11 +2433,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
>  
>  	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
>  	if (!dispose_it) {
> -		/* The MPTCP code never wait on the subflow sockets, TCP-level
> -		 * disconnect should never fail
> -		 */
> -		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> -		mptcp_subflow_ctx_reset(subflow);
> +		__mptcp_subflow_disconnect(ssk, subflow, flags);
>  		release_sock(ssk);
>  
>  		goto out;

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

Re: [PATCH mptcp-next v14 3/7] mptcp: add __mptcp_subflow_disconnect helper

[Geliang Tang]

Hi Matt,

On Wed, Oct 11, 2023 at 06:53:17PM +0200, Matthieu Baerts wrote:
> Hi Geliang, Paolo,
> 
> On 11/10/2023 15:34, Geliang Tang wrote:
> > When closing the msk->first socket in __mptcp_close_ssk(), if there's
> > another subflow available, it's better to avoid resetting it, just shut
> > down it.
> > 
> > This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
> > flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
> > we invoke tcp_shutdown() instead of tcp_disconnect().
> > 
> > Co-developed-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
> >  1 file changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 30e0c29ae0a4..1a54d55f8bb2 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
> >  #define MPTCP_CF_PUSH		BIT(1)
> >  #define MPTCP_CF_FASTCLOSE	BIT(2)
> >  
> > +/* be sure to send a reset only if the caller asked for it, also
> > + * clean completely the subflow status when the subflow reaches
> > + * TCP_CLOSE state
> > + */
> > +static void __mptcp_subflow_disconnect(struct sock *ssk,
> > +				       struct mptcp_subflow_context *subflow,
> > +				       unsigned int flags)
> > +{
> > +	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
> > +	    (flags & MPTCP_CF_FASTCLOSE)) {
> > +		/* The MPTCP code never wait on the subflow sockets, TCP-level
> > +		 * disconnect should never fail
> > +		 */
> > +		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> > +		mptcp_subflow_ctx_reset(subflow);
> > +	} else {
> > +		tcp_shutdown(ssk, SEND_SHUTDOWN);
> 
> I didn't check in detail but should we not also call
> __mptcp_subflow_error_report()?

When the socket shuts down, it's state is TCPF_FIN_WAIT2.
__mptcp_subflow_error_report will return and do nothing in this case:

      if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
              return false;

So no need to call __mptcp_subflow_error_report.

> 
> And maybe other helpers? What is done between the 'goto out' and the
> 'out' label in __mptcp_close_ssk() here below? → what we do when other
> subflows are being closed.

No need to go the following path too:

        sock_put(ssk);

        if (ssk == msk->first)
                WRITE_ONCE(msk->first, NULL);

So we can keep this patch as it.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> 
> > +	}
> > +}
> > +
> >  /* subflow sockets can be either outgoing (connect) or incoming
> >   * (accept).
> >   *
> > @@ -2403,7 +2423,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >  	lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
> >  
> >  	if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) {
> > -		/* be sure to force the tcp_disconnect() path,
> > +		/* be sure to force the tcp_close path
> >  		 * to generate the egress reset
> >  		 */
> >  		ssk->sk_lingertime = 0;
> > @@ -2413,11 +2433,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> >  
> >  	need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk);
> >  	if (!dispose_it) {
> > -		/* The MPTCP code never wait on the subflow sockets, TCP-level
> > -		 * disconnect should never fail
> > -		 */
> > -		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
> > -		mptcp_subflow_ctx_reset(subflow);
> > +		__mptcp_subflow_disconnect(ssk, subflow, flags);
> >  		release_sock(ssk);
> >  
> >  		goto out;
> 
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v14 4/7] selftests: mptcp: userspace pm remove initial subflow

[Geliang Tang]

On Wed, Oct 11, 2023 at 05:57:42PM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 11/10/2023 15:34, Geliang Tang wrote:
> > This patch adds a selftest for userpsace PM to remove the initial
> > subflow. Use userspace_pm_add_sf() to add a subflow, and pass initial
> > ip address to userspace_pm_rm_sf() to remove the initial subflow.
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 22 +++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index ab6908b7b143..e1304383057c 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -3507,6 +3507,28 @@ userspace_tests()
> >  		kill_events_pids
> >  		wait $tests_pid
> >  	fi
> > +
> > +	# userspace pm remove initial subflow
> > +	if reset_with_events "userspace pm remove initial subflow" &&
> > +	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> > +		set_userspace_pm $ns2
> > +		pm_nl_set_limits $ns1 0 1
> > +		speed=10 \
> > +			run_tests $ns1 $ns2 10.0.1.1 &
> > +		local tests_pid=$!
> > +		wait_mpj $ns2
> > +		userspace_pm_add_sf $ns2 10.0.3.2 20
> > +		chk_join_nr 1 1 1
> > +		chk_mptcp_info subflows 1 subflows 1
> > +		chk_subflows_total 2 2
> > +		userspace_pm_rm_sf $ns2 10.0.1.2
> 
> If this command removes the initial subflow ...
> 
> > +		chk_rm_nr 0 1
> 
> (it is a bit confusing, we could think there are RM_ADDR being sent
> here. If you need to change something else, can you add a comment here
> that we don't look at the counter linked to the RM_ADDR but to the one
> linked to the subflows that have been removed please?)
> 
> > +		chk_rst_nr 0 0 invert
> > +		chk_mptcp_info subflows 1 subflows 1
> 
> ... that's normal to still have 1 "extra subflow" here but ...
> 
> > +		chk_subflows_total 2 2
> 
> ... this should be "1 1", no? One subflow has been removed in between,
> so we should not have the same counter as before, no?

It will be "2 1" if we test TCPF_FIN_WAIT2 as well as TCPF_CLOSE in
__mptcp_has_initial_subflow too. This is no subflow removed in ns1, one
subflow has been removed in ns2. Patch 7 will get "2 1" too.

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

Re: [PATCH mptcp-next v14 3/7] mptcp: add __mptcp_subflow_disconnect helper

[Matthieu Baerts]

Hi Geliang,

On 12/10/2023 11:03, Geliang Tang wrote:
> Hi Matt,
> 
> On Wed, Oct 11, 2023 at 06:53:17PM +0200, Matthieu Baerts wrote:
>> Hi Geliang, Paolo,
>>
>> On 11/10/2023 15:34, Geliang Tang wrote:
>>> When closing the msk->first socket in __mptcp_close_ssk(), if there's
>>> another subflow available, it's better to avoid resetting it, just shut
>>> down it.
>>>
>>> This patch adds a new helper __mptcp_subflow_disconnect(), and reuse
>>> flag MPTCP_CF_FASTCLOSE in this case. When MPTCP_CF_FASTCLOSE isn't set,
>>> we invoke tcp_shutdown() instead of tcp_disconnect().
>>>
>>> Co-developed-by: Paolo Abeni <pabeni@redhat.com>
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>>> ---
>>>  net/mptcp/protocol.c | 28 ++++++++++++++++++++++------
>>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
>>> index 30e0c29ae0a4..1a54d55f8bb2 100644
>>> --- a/net/mptcp/protocol.c
>>> +++ b/net/mptcp/protocol.c
>>> @@ -2366,6 +2366,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk)
>>>  #define MPTCP_CF_PUSH		BIT(1)
>>>  #define MPTCP_CF_FASTCLOSE	BIT(2)
>>>  
>>> +/* be sure to send a reset only if the caller asked for it, also
>>> + * clean completely the subflow status when the subflow reaches
>>> + * TCP_CLOSE state
>>> + */
>>> +static void __mptcp_subflow_disconnect(struct sock *ssk,
>>> +				       struct mptcp_subflow_context *subflow,
>>> +				       unsigned int flags)
>>> +{
>>> +	if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) ||
>>> +	    (flags & MPTCP_CF_FASTCLOSE)) {
>>> +		/* The MPTCP code never wait on the subflow sockets, TCP-level
>>> +		 * disconnect should never fail
>>> +		 */
>>> +		WARN_ON_ONCE(tcp_disconnect(ssk, 0));
>>> +		mptcp_subflow_ctx_reset(subflow);
>>> +	} else {
>>> +		tcp_shutdown(ssk, SEND_SHUTDOWN);
>>
>> I didn't check in detail but should we not also call
>> __mptcp_subflow_error_report()?
> 
> When the socket shuts down, it's state is TCPF_FIN_WAIT2.
> __mptcp_subflow_error_report will return and do nothing in this case:
> 
>       if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
>               return false;
> 
> So no need to call __mptcp_subflow_error_report.
> 
>>
>> And maybe other helpers? What is done between the 'goto out' and the
>> 'out' label in __mptcp_close_ssk() here below? → what we do when other
>> subflows are being closed.
> 
> No need to go the following path too:
> 
>         sock_put(ssk);
> 
>         if (ssk == msk->first)
>                 WRITE_ONCE(msk->first, NULL);
> 
> So we can keep this patch as it.

Thank you for having check the two cases!

All good then!

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