[PATCH mptcp-next v3 0/3] userspace pm remove id 0 subflow & address

[PATCH mptcp-next v3 0/3] userspace pm remove id 0 subflow & address

[Geliang Tang]

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 (3):
  mptcp: allow creating id 0 subflow
  mptcp: remove id 0 address
  selftests: mptcp: remove id 0 subflow & address

 net/mptcp/pm.c                                |  2 +-
 net/mptcp/pm_userspace.c                      | 36 +++++++++++----
 .../testing/selftests/net/mptcp/mptcp_join.sh | 46 +++++++++++++++++++
 3 files changed, 74 insertions(+), 10 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v3 1/3] mptcp: 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 v3 2/3] mptcp: 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 remve 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/subflows as announced.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c           |  2 +-
 net/mptcp/pm_userspace.c | 30 +++++++++++++++++++++++++++---
 2 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index d8da5374d9e1..6c610fb494ab 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -192,7 +192,7 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
 
 	update_subflows = subflow->request_join || subflow->mp_join;
 	if (mptcp_pm_is_userspace(msk)) {
-		if (update_subflows) {
+		if (update_subflows || subflow->request_mptcp || subflow->mp_capable) {
 			spin_lock_bh(&pm->lock);
 			pm->subflows--;
 			spin_unlock_bh(&pm->lock);
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index d042d32beb4d..3e5ac4015c8e 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -236,7 +236,31 @@ 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;
+		int id_0 = 0;
+
+		mptcp_for_each_subflow(msk, subflow) {
+			if (subflow->remote_id == 0)
+				id_0 = 1;
+		}
+		if (!id_0)
+			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 +275,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 +289,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

Re: [PATCH mptcp-next v3 2/3] mptcp: remove id 0 address

[Matthieu Baerts]

Hi Geliang,

On 17/08/2023 16:10, 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 remve id

(typo: remve -> remove)

> 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/subflows as announced.

only 'addresses' in this commit, not 'subflows', right?

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/379
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow establishment")

I guess the second "Fixes" is no longer needed.

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm.c           |  2 +-
>  net/mptcp/pm_userspace.c | 30 +++++++++++++++++++++++++++---
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index d8da5374d9e1..6c610fb494ab 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -192,7 +192,7 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
>  
>  	update_subflows = subflow->request_join || subflow->mp_join;
>  	if (mptcp_pm_is_userspace(msk)) {
> -		if (update_subflows) {
> +		if (update_subflows || subflow->request_mptcp || subflow->mp_capable) {

I don't think we can do that: it is confusing but pm->subflows counts
the number of "additional" subflows. Probably we should rename it...
Anyway, it means that if we remove the initial subflow, we should not
decrement it not to break the "user API".

It also means that in the selftests, if we remove the initial subflow,
this "subflows" counter should not change (but still good to validate
that). We can also look with 'ss' (ss -ti | grep -c tcp-ulp-mptcp) or
getsockopt(MPTCP_FULL_INFO) (or others except MPTCP_INFO) and then
mptcp_subflow_data->num_subflows to get the "correct" amount of
subflows. Or only check if the remove address has been sent and received.

(not directly related to this but I wonder if we should not expose to
userspace if the initial subflow has been removed → I just created the
ticket #428 on GitHub)

>  			spin_lock_bh(&pm->lock);
>  			pm->subflows--;
>  			spin_unlock_bh(&pm->lock);
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index d042d32beb4d..3e5ac4015c8e 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -236,7 +236,31 @@ 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;
> +		int id_0 = 0;

(could be a bool: has_id_0)

> +
> +		mptcp_for_each_subflow(msk, subflow) {

This should be done under the msk lock, no?

(we could also look at READ_ONCE(msk->first) before iterating over all
subflows but not sure it is worth it and probably best to do it in a
second part, for net-next and not -net)

> +			if (subflow->remote_id == 0)
> +				id_0 = 1;

You can "break" here.

> +		}
> +		if (!id_0)

It would be good to add an error message, e.g.

  GENL_SET_ERR_MSG(info, "address with id 0 not found")

To let the userspace understanding why it was not OK.

> +			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

[PATCH mptcp-next v3 3/3] selftests: mptcp: remove id 0 subflow & address

[Geliang Tang]

This patch adds selftests for userpsace PM to remove id 0 subflow and
id 0 address.

A new helper userspace_pm_rm_id_0_subflow_or_address_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, and 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 | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index ee1f89a872b3..2d7ac76697bc 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3320,6 +3320,26 @@ userspace_pm_rm_sf_addr_ns2()
 	wait_rm_sf $ns2 1
 }
 
+# $1: command (rem/dsf)
+userspace_pm_rm_id_0_subflow_or_address_ns2()
+{
+	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=$(sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
+
+	if [ "$1" == "subflow" ]; then
+		ip netns exec $ns2 ./pm_nl_ctl dsf lip 10.0.1.2 lport $sp \
+				rip $da rport $dp token $tk
+	elif [ "$1" == "address" ]; then
+		ip netns exec $ns2 ./pm_nl_ctl rem token $tk id 0
+	fi
+
+	sleep 0.5
+}
+
 userspace_tests()
 {
 	# userspace pm type prevents add_addr
@@ -3434,6 +3454,32 @@ userspace_tests()
 		kill_events_pids
 		wait $tests_pid
 	fi
+
+	# userspace pm remove id 0 subflow & address
+	for type in "subflow" "address"; do
+		if reset_with_events "userspace pm remove id 0 $type" &&
+		   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
+			set_userspace_pm $ns2
+			pm_nl_set_limits $ns1 0 2
+			speed=10 \
+				run_tests $ns1 $ns2 10.0.1.1 &
+			local tests_pid=$!
+			wait_mpj $ns2
+			userspace_pm_add_sf 10.0.2.2 0
+			userspace_pm_add_sf 10.0.3.2 20
+			chk_join_nr 2 2 2
+			chk_mptcp_info subflows 2 subflows 2
+			userspace_pm_rm_id_0_subflow_or_address_ns2 "$type"
+			if [ "$type" == "subflow" ]; then
+				chk_rm_nr 0 1
+			elif [ "$type" == "address" ]; then
+				chk_rm_nr 1 0
+			fi
+			chk_mptcp_info subflows 2 subflows 1
+			kill_events_pids
+			wait $tests_pid
+		fi
+	done
 }
 
 endpoint_tests()
-- 
2.35.3

Re: selftests: mptcp: remove id 0 subflow & address: 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/5698108105949184
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5698108105949184/summary/summary.txt

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

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

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

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


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 v3 3/3] selftests: mptcp: remove id 0 subflow & address

[Matthieu Baerts]

Hi Geliang,

On 17/08/2023 16:10, Geliang Tang wrote:
> This patch adds selftests for userpsace PM to remove id 0 subflow and
> id 0 address.
> 
> A new helper userspace_pm_rm_id_0_subflow_or_address_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, and 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 | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index ee1f89a872b3..2d7ac76697bc 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -3320,6 +3320,26 @@ userspace_pm_rm_sf_addr_ns2()
>  	wait_rm_sf $ns2 1
>  }
>  
> +# $1: command (rem/dsf)

(now it is more a "type" and not a command and it can be 'subflow' or
'address')

> +userspace_pm_rm_id_0_subflow_or_address_ns2()
> +{
> +	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=$(sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")

Most of these parameters are only needed to remove a subflow, not to
remove an address. Would it not be better to have two different helpers?
Also below, just after having called this helper, there is a check on
the "type": so fine to call different helpers from there, no?

(see below, maybe better without the for-loop?)


Note that if you wanted to have a helper because the "sed" command are
not very readable, you can always have helpers to get the token, etc. e.g.

  tk=$(evts_get_token "${evts_ns2}")

you can even have something more generic:

  # $1: info name ; $2: ns
  evts_get_info()
  {
          sed -n "s/.*\(${1}:\)\([0-9.]\+\).*$/\2/p;q" "${2}"
  }

  tk=$(evts_get_info token "${evts_ns2}")

But this can also be done after in commits for net-next, not like here
for -net.

> +
> +	if [ "$1" == "subflow" ]; then
> +		ip netns exec $ns2 ./pm_nl_ctl dsf lip 10.0.1.2 lport $sp \
> +				rip $da rport $dp token $tk
> +	elif [ "$1" == "address" ]; then
> +		ip netns exec $ns2 ./pm_nl_ctl rem token $tk id 0
> +	fi
> +
> +	sleep 0.5
> +}
> +
>  userspace_tests()
>  {
>  	# userspace pm type prevents add_addr
> @@ -3434,6 +3454,32 @@ userspace_tests()
>  		kill_events_pids
>  		wait $tests_pid
>  	fi
> +
> +	# userspace pm remove id 0 subflow & address
> +	for type in "subflow" "address"; do
> +		if reset_with_events "userspace pm remove id 0 $type" &&

I think it would be clearer, for the "subflow" case, to say that we
remove the "initial subflow", not all subflows with local id 0.

(see below, maybe better without the for-loop?)

> +		   continue_if mptcp_lib_has_file '/proc/sys/net/mptcp/pm_type'; then
> +			set_userspace_pm $ns2
> +			pm_nl_set_limits $ns1 0 2
> +			speed=10 \
> +				run_tests $ns1 $ns2 10.0.1.1 &
> +			local tests_pid=$!
> +			wait_mpj $ns2
> +			userspace_pm_add_sf 10.0.2.2 0

Oh I guess here you check that we can create a new subflow using ID 0 to
validate patch 1/3, right? Maybe better to have a dedicated test  for
that? Or modify an existing one ("userspace pm create destroy subflow")
to add a new subflow from ID 0? → if you add this test, maybe better to
place it just after the first patch of this series.

> +			userspace_pm_add_sf 10.0.3.2 20
> +			chk_join_nr 2 2 2
> +			chk_mptcp_info subflows 2 subflows 2
> +			userspace_pm_rm_id_0_subflow_or_address_ns2 "$type"
> +			if [ "$type" == "subflow" ]; then

(if you have 'if' here, I wonder if it is still interesting to have a
for-loop here: we already have have many tests that are similar but one
or two lines are different ; also, it might help to find the test
without the variable)

WDYT? Maybe the for loop is still better?

If you split, maybe better to split the commits? From what I understood,
removing the initial subflow was working before this series but it was
not tested. Only sending the RM_ADDR for ID 0 was not working, right?
Then we could have one test validating the creation of a new subflow
with ID 0 + destroy the initial subflow. And a second one to send a
remove address with ID 0 (or edit an existing one like "userspace pm add
& remove address")

> +				chk_rm_nr 0 1
> +			elif [ "$type" == "address" ]; then
> +				chk_rm_nr 1 0
> +			fi
> +			chk_mptcp_info subflows 2 subflows 1
> +			kill_events_pids
> +			wait $tests_pid
> +		fi
> +	done
>  }
>  
>  endpoint_tests()

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

Re: [PATCH mptcp-next v3 0/3] userspace pm remove id 0 subflow & address

[Matthieu Baerts]

Hi Geliang,

On 17/08/2023 16:10, Geliang Tang wrote:
> v3:
>  - address Matt's comments in v2.

Thank you for this new version!

I think you forgot to reply to some questions I had in the v2, mainly:
was it OK before this series to remove the initial subflow?

I have other comments, please see the different patches.

Regarding the selftests, it might be good to have 3 different tests:

1) remove the initial subflow: was working before this series but not tested
2) create an additional subflow using the same source IP address as the
initial subflow (ID 0): to validate patch 1/3 from this series
3) send a RM_ADDR for the ID 0: to validate patch 2/3 from this series

For (1) and (2), the test "userspace pm create destroy subflow" could be
modified. Or a new test to cover both at the same time, e.g. to add a
new subflow with local ID 0 and then remove the initial subflow. WDYT?

Note that these 3 tests could also be validated from userspace_pm.sh
instead, no? It maybe makes more sense and would be faster? Up to you.

(also, this series is more for -net (mptcp-net) than net-next)

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