[PATCH mptcp-next v4 0/6] userspace pm remove id 0 subflow & address

[PATCH mptcp-next v4 0/6] userspace pm remove id 0 subflow & address

[Geliang Tang]

v4:
 - add evts_get_info and chk_subflows helpers.
 - split the selftests patch into three.

v3:
 - address Matt's comments in v2.

v2:
 - fix CI errors.

This patchset addresses #379 and #391, add the abilities to remove id 0
subflow and address for userspace PM. And a selftest.

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

Geliang Tang (6):
  selftests: mptcp: add evts_get_info helper
  selftests: mptcp: userspace pm remove id 0 subflow
  mptcp: userspace pm allow creating id 0 subflow
  selftests: mptcp: userspace pm create id 0 subflow
  mptcp: userspace pm remove id 0 address
  selftests: mptcp: userspace pm remove id 0 address

 net/mptcp/pm_userspace.c                      |  48 ++++--
 .../testing/selftests/net/mptcp/mptcp_join.sh | 145 +++++++++++++++---
 2 files changed, 166 insertions(+), 27 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v4 1/6] selftests: mptcp: add evts_get_info helper

[Geliang Tang]

This patch adds a new helper evts_get_info(), using 'sed' command to
parse the value of the given keyword in the output of 'pm_nl_ctl events'
command, to make the userpsace pm selftests more readable.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ee1f89a872b3..f311fac5fa0d 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3255,14 +3255,22 @@ fail_tests()
 	fi
 }
 
+# $1: info name ; $2: ns ; $3: type
+evts_get_info()
+{
+	local t=${3:-1}
+
+	grep "^type:$t," "${2}" |
+	sed -n 's/.*\('${1}':\)\([0-9a-f:.]*\).*$/\2/p;q'
+}
+
 userspace_pm_add_addr()
 {
 	local addr=$1
 	local id=$2
 	local tk
 
-	tk=$(grep "type:1," "$evts_ns1" |
-	     sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
+	tk=$(evts_get_info token "$evts_ns1")
 	ip netns exec $ns1 ./pm_nl_ctl ann $addr token $tk id $id
 	sleep 1
 }
@@ -3273,14 +3281,10 @@ userspace_pm_rm_sf_addr_ns1()
 	local id=$2
 	local tk sp da dp
 
-	tk=$(grep "type:1," "$evts_ns1" |
-	     sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
-	sp=$(grep "type:10" "$evts_ns1" |
-	     sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
-	da=$(grep "type:10" "$evts_ns1" |
-	     sed -n 's/.*\(daddr6:\)\([0-9a-f:.]*\).*$/\2/p;q')
-	dp=$(grep "type:10" "$evts_ns1" |
-	     sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q')
+	tk=$(evts_get_info token "$evts_ns1")
+	sp=$(evts_get_info sport "$evts_ns1" 10)
+	da=$(evts_get_info daddr6 "$evts_ns1" 10)
+	dp=$(evts_get_info dport "$evts_ns1" 10)
 	ip netns exec $ns1 ./pm_nl_ctl rem token $tk id $id
 	ip netns exec $ns1 ./pm_nl_ctl dsf lip "::ffff:$addr" \
 				lport $sp rip $da rport $dp token $tk
@@ -3294,9 +3298,9 @@ userspace_pm_add_sf()
 	local id=$2
 	local tk da dp
 
-	tk=$(sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
-	da=$(sed -n 's/.*\(daddr4:\)\([0-9.]*\).*$/\2/p;q' "$evts_ns2")
-	dp=$(sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
+	tk=$(evts_get_info token "$evts_ns2")
+	da=$(evts_get_info daddr4 "$evts_ns2")
+	dp=$(evts_get_info dport "$evts_ns2")
 	ip netns exec $ns2 ./pm_nl_ctl csf lip $addr lid $id \
 				rip $da rport $dp token $tk
 	sleep 1
@@ -3308,11 +3312,10 @@ userspace_pm_rm_sf_addr_ns2()
 	local id=$2
 	local tk da dp sp
 
-	tk=$(sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
-	da=$(sed -n 's/.*\(daddr4:\)\([0-9.]*\).*$/\2/p;q' "$evts_ns2")
-	dp=$(sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
-	sp=$(grep "type:10" "$evts_ns2" |
-	     sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
+	tk=$(evts_get_info token "$evts_ns2")
+	da=$(evts_get_info daddr4 "$evts_ns2")
+	dp=$(evts_get_info dport "$evts_ns2")
+	sp=$(evts_get_info sport "$evts_ns2" 10)
 	ip netns exec $ns2 ./pm_nl_ctl rem token $tk id $id
 	ip netns exec $ns2 ./pm_nl_ctl dsf lip $addr lport $sp \
 				rip $da rport $dp token $tk
-- 
2.35.3

[PATCH mptcp-next v4 2/6] selftests: mptcp: userspace pm remove id 0 subflow

[Geliang Tang]

This patch adds a selftest for userpsace PM to remove id 0 subflow.

A new helper userspace_pm_rm_id_0_subflow_ns2() is added, in it use

        ./pm_nl_ctl dsf lip 10.0.1.2 lport $sp \
			rip 10.0.1.1 rport $dp token $tk

to remove id 0 subflow.

Add a new helper +chk_subflows(), in it use 'ss' command

        ss -ti | grep -c tcp-ulp-mptcp

to get the "correct" amount of subflows, including the initial
subflow.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index f311fac5fa0d..52d696f49e58 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3323,6 +3323,43 @@ userspace_pm_rm_sf_addr_ns2()
 	wait_rm_sf $ns2 1
 }
 
+userspace_pm_rm_id_0_subflow_ns2()
+{
+	local tk da dp sp
+
+	tk=$(evts_get_info token "$evts_ns2")
+	da=$(evts_get_info daddr4 "$evts_ns2")
+	dp=$(evts_get_info dport "$evts_ns2")
+	sp=$(evts_get_info sport "$evts_ns2")
+	ip netns exec $ns2 ./pm_nl_ctl dsf lip 10.0.1.2 lport $sp \
+			rip $da rport $dp token $tk
+	wait_rm_sf $ns2 1
+}
+
+# $1: subflows in ns1 ; $2: subflows in ns2
+chk_subflows()
+{
+	local cnt1
+	local cnt2
+
+	print_check "subflows $1:$2"
+
+	cnt1=$(ss -N $ns1 -ti | grep -c tcp-ulp-mptcp)
+	cnt2=$(ss -N $ns2 -ti | grep -c tcp-ulp-mptcp)
+
+	if [ "$1" != "$cnt1" ] || [ "$2" != "$cnt2" ]; then
+		fail_test "got subflows $cnt1:$cnt2 expected $1:$2"
+		dump_stats=1
+	else
+		print_ok
+	fi
+
+	if [ "$dump_stats" = 1 ]; then
+		ss -N $ns1 -ti
+		ss -N $ns2 -ti
+	fi
+}
+
 userspace_tests()
 {
 	# userspace pm type prevents add_addr
@@ -3437,6 +3474,27 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm remove id 0 subflow
+	if reset_with_events "userspace pm remove id 0 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 10.0.3.2 20
+		chk_join_nr 1 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows 2 2
+		userspace_pm_rm_id_0_subflow_ns2
+		chk_rm_nr 0 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows 1 1
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

[PATCH mptcp-next v4 3/6] mptcp: userspace pm allow creating id 0 subflow

[Geliang Tang]

This patch drops id 0 limitation in mptcp_nl_cmd_sf_create() to allow
creating additional subflows with the local addr ID 0.

There is no reason not to allow additional subflows from this local
address: we should be able to create new subflows from the initial
endpoint. This limitation was breaking fullmesh support from userspace.

Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/391
Suggested-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index b5a8aa4c1ebd..d042d32beb4d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -307,12 +307,6 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 		goto create_err;
 	}
 
-	if (addr_l.id == 0) {
-		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "missing local addr id");
-		err = -EINVAL;
-		goto create_err;
-	}
-
 	err = mptcp_pm_parse_addr(raddr, info, &addr_r);
 	if (err < 0) {
 		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "error parsing remote addr");
-- 
2.35.3

[PATCH mptcp-next v4 4/6] selftests: mptcp: userspace pm create id 0 subflow

[Geliang Tang]

This patch adds a selftest to create id 0 subflow. Using

        ./pm_nl_ctl csf lip 10.0.3.2 lid 0 \
                        rip $da rport $dp token $tk

command to create id 0 subflow.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 52d696f49e58..322adac0cb1b 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3475,6 +3475,25 @@ userspace_tests()
 		wait $tests_pid
 	fi
 
+	# userspace pm create id 0 subflow
+	if reset_with_events "userspace pm create id 0 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=slow \
+			run_tests $ns1 $ns2 10.0.1.1 &
+		local tests_pid=$!
+		wait_mpj $ns2
+		chk_mptcp_info subflows 0 subflows 0
+		chk_subflows 1 1
+		userspace_pm_add_sf 10.0.3.2 0
+		chk_join_nr 1 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows 2 2
+		kill_events_pids
+		wait $tests_pid
+	fi
+
 	# userspace pm remove id 0 subflow
 	if reset_with_events "userspace pm remove id 0 subflow" &&
 	   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
-- 
2.35.3

[PATCH mptcp-next v4 5/6] mptcp: userspace pm remove id 0 address

[Geliang Tang]

This patch adds the ability to send RM_ADDR for local ID 0. 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 | 42 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index d042d32beb4d..059b672b55f7 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -236,7 +236,43 @@ int mptcp_nl_cmd_remove(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) {
+		struct mptcp_rm_list list = { .nr = 0 };
+		struct mptcp_subflow_context *subflow;
+		bool has_id_0 = false;
+
+		if (!READ_ONCE(msk->first)) {
+			GENL_SET_ERR_MSG(info, "no subflow in conn_list");
+			goto out;
+		}
+
+		spin_lock_bh(&msk->pm.lock);
+		mptcp_for_each_subflow(msk, subflow) {
+			if (subflow->remote_id == 0) {
+				has_id_0 = true;
+				break;
+			}
+		}
+		spin_unlock_bh(&msk->pm.lock);
+
+		if (!has_id_0) {
+			GENL_SET_ERR_MSG(info, "address with id 0 not found");
+			goto out;
+		}
+
+		list.ids[list.nr++] = 0;
+
+		lock_sock((struct sock *)msk);
+		spin_lock_bh(&msk->pm.lock);
+		mptcp_pm_remove_addr(msk, &list);
+		spin_unlock_bh(&msk->pm.lock);
+		release_sock((struct sock *)msk);
+
+		err = 0;
+		goto out;
 	}
 
 	lock_sock((struct sock *)msk);
@@ -251,7 +287,7 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 	if (!match) {
 		GENL_SET_ERR_MSG(info, "address with specified id not found");
 		release_sock((struct sock *)msk);
-		goto remove_err;
+		goto out;
 	}
 
 	list_move(&match->list, &free_list);
@@ -265,7 +301,7 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	err = 0;
- remove_err:
+out:
 	sock_put((struct sock *)msk);
 	return err;
 }
-- 
2.35.3

[PATCH mptcp-next v4 6/6] selftests: mptcp: userspace pm remove id 0 address

[Geliang Tang]

This patch adds a selftest for userpsace PM to remove id 0 address.

A new helper userspace_pm_rm_id_0_address_ns2() is added, in it use

        ./pm_nl_ctl rem token $tk id 0

to remove id 0 address.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 322adac0cb1b..1c5c0bbae38f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3336,6 +3336,14 @@ userspace_pm_rm_id_0_subflow_ns2()
 	wait_rm_sf $ns2 1
 }
 
+userspace_pm_rm_id_0_address_ns2()
+{
+	local tk=$(evts_get_info token "$evts_ns2")
+
+	ip netns exec $ns2 ./pm_nl_ctl rem token $tk id 0
+	wait_rm_addr $ns2 1
+}
+
 # $1: subflows in ns1 ; $2: subflows in ns2
 chk_subflows()
 {
@@ -3514,6 +3522,27 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm remove id 0 address
+	if reset_with_events "userspace pm remove id 0 address" &&
+	   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 10.0.3.2 20
+		chk_join_nr 1 1 1
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows 2 2
+		userspace_pm_rm_id_0_address_ns2
+		chk_rm_nr 1 0
+		chk_mptcp_info subflows 1 subflows 1
+		chk_subflows 1 1
+		kill_events_pids
+		wait $tests_pid
+	fi
 }
 
 endpoint_tests()
-- 
2.35.3

Re: [PATCH mptcp-next v4 5/6] mptcp: userspace pm remove id 0 address

[Matthieu Baerts]

Hi Geliang

On 18/08/2023 09:12, Geliang Tang wrote:
> This patch adds the ability to send RM_ADDR for local ID 0. 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.

Thank you for the new version!

> 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 | 42 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index d042d32beb4d..059b672b55f7 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -236,7 +236,43 @@ int mptcp_nl_cmd_remove(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) {
> +		struct mptcp_rm_list list = { .nr = 0 };
> +		struct mptcp_subflow_context *subflow;
> +		bool has_id_0 = false;
> +
> +		if (!READ_ONCE(msk->first)) {
> +			GENL_SET_ERR_MSG(info, "no subflow in conn_list");
> +			goto out;
> +		}

Sorry, my previous message was probably not clear: if (msk->first !=
NULL), it means the initial subflow is still there and ID 0 is still
being used. If it is no longer there, ID 0 might still be used by
another subflow and it is required to iterate over the subflow list.

But maybe easier here to skip this "optimisation" (remove this block
above) and simply iterate over the subflow list like we would do for the
other IDs.

EDIT: mmh, if there is another subflow created with ID 0, will it be in
msk->pm.userspace_pm_local_addr_list list? (I didn't check) If yes, it
should also be handled the same way we do for the other subflows. In
this case, maybe it is not needed to iterate over the different subflows
but only the local addr list and if not found (!match), just for ID==0
case, we check if (msk->first != NULL) and create a fake entry? Or we
always create an entry in local_addr_list for ID == 0? (I didn't check
what would be best)

> +
> +		spin_lock_bh(&msk->pm.lock);

Is it enough? To iterate over the subflow list, you don't need the msk
lock? (lock_sock(sk))

> +		mptcp_for_each_subflow(msk, subflow) {
> +			if (subflow->remote_id == 0) {
> +				has_id_0 = true;
> +				break;
> +			}
> +		}
> +		spin_unlock_bh(&msk->pm.lock);
> +
> +		if (!has_id_0) {
> +			GENL_SET_ERR_MSG(info, "address with id 0 not found");
> +			goto out;
> +		}
> +
> +		list.ids[list.nr++] = 0;
> +
> +		lock_sock((struct sock *)msk);
> +		spin_lock_bh(&msk->pm.lock);
> +		mptcp_pm_remove_addr(msk, &list);
> +		spin_unlock_bh(&msk->pm.lock);
> +		release_sock((struct sock *)msk);
> +
> +		err = 0;
> +		goto out;
>  	}
>  
>  	lock_sock((struct sock *)msk);

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

Re: [PATCH mptcp-next v4 6/6] selftests: mptcp: userspace pm remove id 0 address

[Matthieu Baerts]

Hi Geliang,

On 18/08/2023 09:12, Geliang Tang wrote:
> This patch adds a selftest for userpsace PM to remove id 0 address.
> 
> A new helper userspace_pm_rm_id_0_address_ns2() is added, in it use
> 
>         ./pm_nl_ctl rem token $tk id 0
> 
> to remove id 0 address.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 322adac0cb1b..1c5c0bbae38f 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3336,6 +3336,14 @@ userspace_pm_rm_id_0_subflow_ns2()
>  	wait_rm_sf $ns2 1
>  }
>  
> +userspace_pm_rm_id_0_address_ns2()
> +{
> +	local tk=$(evts_get_info token "$evts_ns2")
> +
> +	ip netns exec $ns2 ./pm_nl_ctl rem token $tk id 0
> +	wait_rm_addr $ns2 1
> +}
> +
>  # $1: subflows in ns1 ; $2: subflows in ns2
>  chk_subflows()
>  {
> @@ -3514,6 +3522,27 @@ userspace_tests()
>  		kill_events_pids
>  		wait $tests_pid
>  	fi
> +
> +	# userspace pm remove id 0 address
> +	if reset_with_events "userspace pm remove id 0 address" &&
> +	   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 10.0.3.2 20
> +		chk_join_nr 1 1 1
> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_subflows 2 2
> +		userspace_pm_rm_id_0_address_ns2
> +		chk_rm_nr 1 0
> +		chk_mptcp_info subflows 1 subflows 1
> +		chk_subflows 1 1

When we only send a REMOVE_ADDR, why do we have one subflow that is
being removed? Is it because the other peer disconnected the subflow
linked to ID=0?

Do you mind checking this please?
Because the remove address command should not result in a host sending
both a REMOVE_ADDR and a FIN. That's up to the other host to take an
action when the REMOVE_ADDR is received, no?

Cheers,
Matt

> +		kill_events_pids
> +		wait $tests_pid
> +	fi
>  }
>  
>  endpoint_tests()

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

Re: [PATCH mptcp-next v4 5/6] mptcp: userspace pm remove id 0 address

[Geliang Tang]

Hi Matt,

On Fri, Aug 18, 2023 at 11:30:01AM +0200, Matthieu Baerts wrote:
> Hi Geliang
> 
> On 18/08/2023 09:12, Geliang Tang wrote:
> > This patch adds the ability to send RM_ADDR for local ID 0. 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.
> 
> Thank you for the new version!
> 
> > 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 | 42 +++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index d042d32beb4d..059b672b55f7 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -236,7 +236,43 @@ int mptcp_nl_cmd_remove(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) {
> > +		struct mptcp_rm_list list = { .nr = 0 };
> > +		struct mptcp_subflow_context *subflow;
> > +		bool has_id_0 = false;
> > +
> > +		if (!READ_ONCE(msk->first)) {
> > +			GENL_SET_ERR_MSG(info, "no subflow in conn_list");
> > +			goto out;
> > +		}
> 
> Sorry, my previous message was probably not clear: if (msk->first !=
> NULL), it means the initial subflow is still there and ID 0 is still
> being used. If it is no longer there, ID 0 might still be used by
> another subflow and it is required to iterate over the subflow list.
> 
> But maybe easier here to skip this "optimisation" (remove this block
> above) and simply iterate over the subflow list like we would do for the
> other IDs.

I prefer to skip this block in v5. 

> 
> EDIT: mmh, if there is another subflow created with ID 0, will it be in
> msk->pm.userspace_pm_local_addr_list list? (I didn't check) If yes, it
> should also be handled the same way we do for the other subflows. In
> this case, maybe it is not needed to iterate over the different subflows
> but only the local addr list and if not found (!match), just for ID==0
> case, we check if (msk->first != NULL) and create a fake entry? Or we
> always create an entry in local_addr_list for ID == 0? (I didn't check
> what would be best)
> 
> > +
> > +		spin_lock_bh(&msk->pm.lock);
> 
> Is it enough? To iterate over the subflow list, you don't need the msk
> lock? (lock_sock(sk))
> 
> > +		mptcp_for_each_subflow(msk, subflow) {
> > +			if (subflow->remote_id == 0) {
> > +				has_id_0 = true;
> > +				break;
> > +			}
> > +		}
> > +		spin_unlock_bh(&msk->pm.lock);
> > +
> > +		if (!has_id_0) {
> > +			GENL_SET_ERR_MSG(info, "address with id 0 not found");
> > +			goto out;
> > +		}
> > +
> > +		list.ids[list.nr++] = 0;
> > +
> > +		lock_sock((struct sock *)msk);
> > +		spin_lock_bh(&msk->pm.lock);
> > +		mptcp_pm_remove_addr(msk, &list);
> > +		spin_unlock_bh(&msk->pm.lock);
> > +		release_sock((struct sock *)msk);
> > +
> > +		err = 0;
> > +		goto out;
> >  	}
> >  
> >  	lock_sock((struct sock *)msk);
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net

Re: [PATCH mptcp-next v4 6/6] selftests: mptcp: userspace pm remove id 0 address

[Geliang Tang]

On Fri, Aug 18, 2023 at 11:34:00AM +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 18/08/2023 09:12, Geliang Tang wrote:
> > This patch adds a selftest for userpsace PM to remove id 0 address.
> > 
> > A new helper userspace_pm_rm_id_0_address_ns2() is added, in it use
> > 
> >         ./pm_nl_ctl rem token $tk id 0
> > 
> > to remove id 0 address.
> > 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  .../testing/selftests/net/mptcp/mptcp_join.sh | 29 +++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > index 322adac0cb1b..1c5c0bbae38f 100755
> > --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> > @@ -3336,6 +3336,14 @@ userspace_pm_rm_id_0_subflow_ns2()
> >  	wait_rm_sf $ns2 1
> >  }
> >  
> > +userspace_pm_rm_id_0_address_ns2()
> > +{
> > +	local tk=$(evts_get_info token "$evts_ns2")
> > +
> > +	ip netns exec $ns2 ./pm_nl_ctl rem token $tk id 0
> > +	wait_rm_addr $ns2 1
> > +}
> > +
> >  # $1: subflows in ns1 ; $2: subflows in ns2
> >  chk_subflows()
> >  {
> > @@ -3514,6 +3522,27 @@ userspace_tests()
> >  		kill_events_pids
> >  		wait $tests_pid
> >  	fi
> > +
> > +	# userspace pm remove id 0 address
> > +	if reset_with_events "userspace pm remove id 0 address" &&
> > +	   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 10.0.3.2 20
> > +		chk_join_nr 1 1 1
> > +		chk_mptcp_info subflows 1 subflows 1
> > +		chk_subflows 2 2
> > +		userspace_pm_rm_id_0_address_ns2
> > +		chk_rm_nr 1 0
> > +		chk_mptcp_info subflows 1 subflows 1
> > +		chk_subflows 1 1
> 
> When we only send a REMOVE_ADDR, why do we have one subflow that is
> being removed? Is it because the other peer disconnected the subflow
> linked to ID=0?

Yes. And remove id 0 subflow test got this "chk_subflows 1 1" too.

When closing the initial subflow in __mptcp_close_ssk(), dispose_it is
false, then tcp_disconnect is invoked. This will send a MP_RST to close
a subflow on the peer too. So chk_subflows after closing the initial
subflow is '1 1', not '2 1'.

In v5, chk_rst_nr() is added in these tests.

Thanks,
-Geliang

> 
> Do you mind checking this please?
> Because the remove address command should not result in a host sending
> both a REMOVE_ADDR and a FIN. That's up to the other host to take an
> action when the REMOVE_ADDR is received, no?
> 
> Cheers,
> Matt
> 
> > +		kill_events_pids
> > +		wait $tests_pid
> > +	fi
> >  }
> >  
> >  endpoint_tests()
> 
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net