[PATCH mptcp-next v5 0/7] mptcp: update userspace pm mptcp_info fields

[PATCH mptcp-next v5 0/7] mptcp: update userspace pm mptcp_info fields

[Geliang Tang]

v5:
 - fix a memleak error reported by CI.
 - add more delay for userspace pm tests.

v4:
 - add more patches
 - add selftests

v3:
 - update local_addr_used and add_addr_signaled

v2:
 - hold pm locks

Geliang Tang (7):
  mptcp: don't clear userspace pm addr id
  mptcp: add addr into userspace pm list
  mptcp: close remote subflow when destroying it
  mptcp: increase userspace pm add_addr_signaled
  mptcp: update userspace pm subflows
  mptcp: make userspace_pm_append_new_local_addr static
  selftests: mptcp: check userspace mptcp_info

 net/mptcp/pm.c                                | 21 +++++++++---
 net/mptcp/pm_netlink.c                        |  2 +-
 net/mptcp/pm_userspace.c                      | 33 +++++++++++++++++--
 net/mptcp/protocol.h                          |  2 --
 .../testing/selftests/net/mptcp/mptcp_join.sh | 12 ++++++-
 5 files changed, 59 insertions(+), 11 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v5 1/7] mptcp: don't clear userspace pm addr id

[Geliang Tang]

Don't clear the addr id in mptcp_userspace_pm_get_local_id(), clear it
in mptcp_pm_nl_get_local_id() instead.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_netlink.c   | 2 +-
 net/mptcp/pm_userspace.c | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 67995cb4f8b8..df15d846e7db 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1055,8 +1055,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 
 int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
+	struct mptcp_addr_info skc_local = { 0 };
 	struct mptcp_pm_addr_entry *entry;
-	struct mptcp_addr_info skc_local;
 	struct mptcp_addr_info msk_local;
 	struct pm_nl_pernet *pernet;
 	int ret = -1;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index a02d3cbf2a1b..fe4c29a17466 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -113,7 +113,6 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
 
 	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
 	new_entry.addr = *skc;
-	new_entry.addr.id = 0;
 	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
 
 	if (new_entry.addr.port == msk_sport)
-- 
2.35.3

[PATCH mptcp-next v5 2/7] mptcp: add addr into userspace pm list

[Geliang Tang]

Add the address into userspace_pm_local_addr_list when the subflow is
created.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index fe4c29a17466..49f41a040485 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -301,6 +301,16 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 		goto create_err;
 	}
 
+	err = mptcp_userspace_pm_get_local_id(msk, &addr_l);
+	if (err < 0) {
+		GENL_SET_ERR_MSG(info, "did not match address and id");
+		goto create_err;
+	}
+
+	spin_lock_bh(&msk->pm.lock);
+	msk->pm.local_addr_used++;
+	spin_unlock_bh(&msk->pm.lock);
+
 	lock_sock(sk);
 
 	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
-- 
2.35.3

[PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it

[Geliang Tang]

Not only close the local subflow but also send RM_ADDR by invoking
mptcp_pm_remove_addr() to close the remote subflow when a subflow is
destroyed by userspace PM.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c                        | 16 ++++++++++++++++
 tools/testing/selftests/net/mptcp/mptcp_join.sh |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 49f41a040485..8b077564e394 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -429,6 +429,22 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
 	if (ssk) {
 		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+		struct mptcp_pm_addr_entry *entry, *tmp;
+
+		spin_lock_bh(&msk->pm.lock);
+		list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
+			if (mptcp_addresses_equal(&entry->addr, &addr_l, false)) {
+				struct mptcp_rm_list list = { .nr = 0 };
+
+				list.ids[list.nr++] = entry->addr.id;
+				mptcp_pm_remove_addr(msk, &list);
+				list_del_rcu(&entry->list);
+				kfree(entry);
+				msk->pm.local_addr_used--;
+				break;
+			}
+		}
+		spin_unlock_bh(&msk->pm.lock);
 
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fafd19ec7e1f..506120401abe 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3123,7 +3123,7 @@ userspace_tests()
 		pm_nl_set_limits $ns1 0 1
 		run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
 		chk_join_nr 1 1 1
-		chk_rm_nr 0 1
+		chk_rm_nr 1 1
 		kill_events_pids
 	fi
 }
-- 
2.35.3

[PATCH mptcp-next v5 4/7] mptcp: increase userspace pm add_addr_signaled

[Geliang Tang]

Increase add_addr_signaled counter in mptcp_nl_cmd_announce() when the
address is announced by userspace PM.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 8b077564e394..09b4b359d960 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -170,6 +170,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
 	spin_lock_bh(&msk->pm.lock);
 
 	if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
+		msk->pm.add_addr_signaled++;
 		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
 		mptcp_pm_nl_addr_send_ack(msk);
 	}
-- 
2.35.3

[PATCH mptcp-next v5 5/7] mptcp: update userspace pm subflows

[Geliang Tang]

Increase pm subflows counter on both server side and client side when
userspace pm creates a new subflow, and decrease the counter when it
closes a subflow.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm.c           | 21 +++++++++++++++++----
 net/mptcp/pm_userspace.c |  1 +
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 4ed4d29d9c11..bb01f15d8e0a 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -87,8 +87,15 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
 	unsigned int subflows_max;
 	int ret = 0;
 
-	if (mptcp_pm_is_userspace(msk))
-		return mptcp_userspace_pm_active(msk);
+	if (mptcp_pm_is_userspace(msk)) {
+		if (mptcp_userspace_pm_active(msk)) {
+			spin_lock_bh(&pm->lock);
+			pm->subflows++;
+			spin_unlock_bh(&pm->lock);
+			return true;
+		}
+		return false;
+	}
 
 	subflows_max = mptcp_pm_get_subflows_max(msk);
 
@@ -181,8 +188,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
 	struct mptcp_pm_data *pm = &msk->pm;
 	bool update_subflows;
 
-	update_subflows = (subflow->request_join || subflow->mp_join) &&
-			  mptcp_pm_is_kernel(msk);
+	if (mptcp_pm_is_userspace(msk)) {
+		spin_lock_bh(&pm->lock);
+		pm->subflows--;
+		spin_unlock_bh(&pm->lock);
+		return;
+	}
+
+	update_subflows = (subflow->request_join || subflow->mp_join);
 	if (!READ_ONCE(pm->work_pending) && !update_subflows)
 		return;
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 09b4b359d960..465928c59917 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -310,6 +310,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 
 	spin_lock_bh(&msk->pm.lock);
 	msk->pm.local_addr_used++;
+	msk->pm.subflows++;
 	spin_unlock_bh(&msk->pm.lock);
 
 	lock_sock(sk);
-- 
2.35.3

[PATCH mptcp-next v5 6/7] mptcp: make userspace_pm_append_new_local_addr static

[Geliang Tang]

mptcp_userspace_pm_append_new_local_addr is only used in pm_userspace.c,
so make it static.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 465928c59917..2bda1a217709 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -25,8 +25,8 @@ void mptcp_free_local_addr_list(struct mptcp_sock *msk)
 	}
 }
 
-int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
-					     struct mptcp_pm_addr_entry *entry)
+static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
+						    struct mptcp_pm_addr_entry *entry)
 {
 	DECLARE_BITMAP(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
 	struct mptcp_pm_addr_entry *match = NULL;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 46962a2581b4..421e587a4d62 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -849,8 +849,6 @@ int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *
 void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 					struct list_head *rm_list);
 
-int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
-					     struct mptcp_pm_addr_entry *entry);
 void mptcp_free_local_addr_list(struct mptcp_sock *msk);
 int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info);
 int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info);
-- 
2.35.3

[PATCH mptcp-next v5 7/7] selftests: mptcp: check userspace mptcp_info

[Geliang Tang]

This patch invokes chk_mptcp_info() to check mptcp_info of userspace PM.

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

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 506120401abe..8ca5accd5c82 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -842,8 +842,11 @@ do_transfer()
 				tk=$(grep "type:1," "$evts_ns1" |
 				     sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
 				ip netns exec ${listener_ns} ./pm_nl_ctl ann $addr token $tk id $id
+				chk_mptcp_info subflows_1
 				sleep 1
 				ip netns exec ${listener_ns} ./pm_nl_ctl rem token $tk id $id
+				sleep 1
+				chk_mptcp_info subflows_0
 			fi
 
 			counter=$((counter + 1))
@@ -906,11 +909,14 @@ do_transfer()
 				dp=$(sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
 				ip netns exec ${connector_ns} ./pm_nl_ctl csf lip $addr lid $id \
 									rip $da rport $dp token $tk
+				chk_mptcp_info subflows_1
 				sleep 1
 				sp=$(grep "type:10" "$evts_ns2" |
 				     sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
 				ip netns exec ${connector_ns} ./pm_nl_ctl dsf lip $addr lport $sp \
 									rip $da rport $dp token $tk
+				sleep 1
+				chk_mptcp_info subflows_0
 			fi
 			counter=$((counter + 1))
 			add_nr_ns2=$((add_nr_ns2 - 1))
@@ -3148,6 +3154,10 @@ endpoint_tests()
 		pm_nl_add_endpoint $ns2 10.0.2.2 flags signal
 		pm_nl_check_endpoint 0 "modif is allowed" \
 			$ns2 10.0.2.2 id 1 flags signal
+
+		chk_mptcp_info subflows_1
+		pm_nl_del_endpoint $ns2 1 10.0.2.2
+		chk_mptcp_info subflows_0
 		kill_tests_wait
 	fi
 
-- 
2.35.3

Re: [PATCH mptcp-next v5 7/7] selftests: mptcp: check userspace mptcp_info

[Geliang Tang]

Geliang Tang <geliang.tang@suse.com> 于2023年3月14日周二 15:31写道：
>
> This patch invokes chk_mptcp_info() to check mptcp_info of userspace PM.
>
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 506120401abe..8ca5accd5c82 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -842,8 +842,11 @@ do_transfer()
>                                 tk=$(grep "type:1," "$evts_ns1" |
>                                      sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
>                                 ip netns exec ${listener_ns} ./pm_nl_ctl ann $addr token $tk id $id
> +                               chk_mptcp_info subflows_1
>                                 sleep 1
>                                 ip netns exec ${listener_ns} ./pm_nl_ctl rem token $tk id $id
> +                               sleep 1
> +                               chk_mptcp_info subflows_0
>                         fi
>
>                         counter=$((counter + 1))
> @@ -906,11 +909,14 @@ do_transfer()
>                                 dp=$(sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
>                                 ip netns exec ${connector_ns} ./pm_nl_ctl csf lip $addr lid $id \
>                                                                         rip $da rport $dp token $tk
> +                               chk_mptcp_info subflows_1
>                                 sleep 1
>                                 sp=$(grep "type:10" "$evts_ns2" |
>                                      sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
>                                 ip netns exec ${connector_ns} ./pm_nl_ctl dsf lip $addr lport $sp \
>                                                                         rip $da rport $dp token $tk
> +                               sleep 1
> +                               chk_mptcp_info subflows_0
>                         fi
>                         counter=$((counter + 1))
>                         add_nr_ns2=$((add_nr_ns2 - 1))
> @@ -3148,6 +3154,10 @@ endpoint_tests()
>                 pm_nl_add_endpoint $ns2 10.0.2.2 flags signal
>                 pm_nl_check_endpoint 0 "modif is allowed" \
>                         $ns2 10.0.2.2 id 1 flags signal
> +
> +               chk_mptcp_info subflows_1
> +               pm_nl_del_endpoint $ns2 1 10.0.2.2
> +               chk_mptcp_info subflows_0

These lines should be squashed to "selftests: mptcp: add mptcp_info tests".

-Geliang

>                 kill_tests_wait
>         fi
>
> --
> 2.35.3
>
>

Re: selftests: mptcp: check userspace mptcp_info: 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):
  - Unstable: 1 failed test(s): selftest_userspace_pm 🔴:
  - Task: https://cirrus-ci.com/task/5885673681453056
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5885673681453056/summary/summary.txt

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

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

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

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


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 v5 2/7] mptcp: add addr into userspace pm list

[Matthieu Baerts]

Hi Geliang,

On 14/03/2023 08:31, Geliang Tang wrote:
> Add the address into userspace_pm_local_addr_list when the subflow is
> created.

Do you mind explaining (in the commit message or here if that's the only
modification needed) why it is needed? It is difficult to find the
reason when only looking at this patch.

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

Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it

[Matthieu Baerts]

Hi Geliang,

On 14/03/2023 08:31, Geliang Tang wrote:
> Not only close the local subflow but also send RM_ADDR by invoking
> mptcp_pm_remove_addr() to close the remote subflow when a subflow is
> destroyed by userspace PM.

Should it not be the responsibility of the userspace PM daemon to send
this RM_ADDR?

Do you maybe have a use case where it is interesting to force sending
this RM_ADDR when asking for a destroy?

Maybe we could do that only if the userspace PM asks for that by setting
a new attribute or flag? (if there is an interest)

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

Re: [PATCH mptcp-next v5 4/7] mptcp: increase userspace pm add_addr_signaled

[Matthieu Baerts]

Hi Geliang,

On 14/03/2023 08:31, Geliang Tang wrote:
> Increase add_addr_signaled counter in mptcp_nl_cmd_announce() when the
> address is announced by userspace PM.

I guess this can be seen as a bug fix: we forgot to increment the
counter for the userspace PM, no?

If yes, we should probably send that to net with a Fixes tag. Maybe:

Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")

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

Re: [PATCH mptcp-next v5 5/7] mptcp: update userspace pm subflows

[Matthieu Baerts]

Hi Geliang,

On 14/03/2023 08:31, Geliang Tang wrote:
> Increase pm subflows counter on both server side and client side when
> userspace pm creates a new subflow, and decrease the counter when it
> closes a subflow.

I guess here as well, we can target -net and add a Fixes tag, no?

Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow
establishment")
(...)

> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 4ed4d29d9c11..bb01f15d8e0a 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -87,8 +87,15 @@ bool mptcp_pm_allow_new_subflow(struct mptcp_sock *msk)
>  	unsigned int subflows_max;
>  	int ret = 0;
>  
> -	if (mptcp_pm_is_userspace(msk))
> -		return mptcp_userspace_pm_active(msk);
> +	if (mptcp_pm_is_userspace(msk)) {
> +		if (mptcp_userspace_pm_active(msk)) {
> +			spin_lock_bh(&pm->lock);
> +			pm->subflows++;
> +			spin_unlock_bh(&pm->lock);
> +			return true;
> +		}
> +		return false;
> +	}
>  
>  	subflows_max = mptcp_pm_get_subflows_max(msk);
>  
> @@ -181,8 +188,14 @@ void mptcp_pm_subflow_check_next(struct mptcp_sock *msk, const struct sock *ssk,
>  	struct mptcp_pm_data *pm = &msk->pm;
>  	bool update_subflows;
>  
> -	update_subflows = (subflow->request_join || subflow->mp_join) &&
> -			  mptcp_pm_is_kernel(msk);
> +	if (mptcp_pm_is_userspace(msk)) {
> +		spin_lock_bh(&pm->lock);
> +		pm->subflows--;
> +		spin_unlock_bh(&pm->lock);
> +		return;
> +	}
> +
> +	update_subflows = (subflow->request_join || subflow->mp_join);
>  	if (!READ_ONCE(pm->work_pending) && !update_subflows)
>  		return;

I'm sorry, I'm not sure to understand why you do this here (and above)
and not just in mptcp_nl_cmd_sf_create() and _destroy() like you did in
the v3?

Cheers,
Matt

>  
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 09b4b359d960..465928c59917 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -310,6 +310,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
>  
>  	spin_lock_bh(&msk->pm.lock);
>  	msk->pm.local_addr_used++;
> +	msk->pm.subflows++;
>  	spin_unlock_bh(&msk->pm.lock);
>  
>  	lock_sock(sk);

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

Re: [PATCH mptcp-next v5 7/7] selftests: mptcp: check userspace mptcp_info

[Matthieu Baerts]

Hi Geliang,

On 14/03/2023 08:31, Geliang Tang wrote:
> This patch invokes chk_mptcp_info() to check mptcp_info of userspace PM.
> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 506120401abe..8ca5accd5c82 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -842,8 +842,11 @@ do_transfer()
>  				tk=$(grep "type:1," "$evts_ns1" |
>  				     sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
>  				ip netns exec ${listener_ns} ./pm_nl_ctl ann $addr token $tk id $id
> +				chk_mptcp_info subflows_1

Will this not print message before displaying the title of the test?

>  				sleep 1
>  				ip netns exec ${listener_ns} ./pm_nl_ctl rem token $tk id $id
> +				sleep 1

I'm not a big fan of using a sleep here, it is a source of random timing
issues I guess.

Could it possible to wait for an event to stop waiting when this happens
but also with a possible higher limit in case the host running the test
is slow/busy.

Maybe you can use wait_rm_addr?

> +				chk_mptcp_info subflows_0
>  			fi
>  
>  			counter=$((counter + 1))
> @@ -906,11 +909,14 @@ do_transfer()
>  				dp=$(sed -n 's/.*\(dport:\)\([[:digit:]]*\).*$/\2/p;q' "$evts_ns2")
>  				ip netns exec ${connector_ns} ./pm_nl_ctl csf lip $addr lid $id \
>  									rip $da rport $dp token $tk
> +				chk_mptcp_info subflows_1

Same here for the print message before the title? (or maybe enough to
check in endpoint_tests()?)

>  				sleep 1
>  				sp=$(grep "type:10" "$evts_ns2" |
>  				     sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
>  				ip netns exec ${connector_ns} ./pm_nl_ctl dsf lip $addr lport $sp \
>  									rip $da rport $dp token $tk
> +				sleep 1

Same here with the sleep. Maybe a "wait_xxx" function?

> +				chk_mptcp_info subflows_0
>  			fi
>  			counter=$((counter + 1))
>  			add_nr_ns2=$((add_nr_ns2 - 1))
> @@ -3148,6 +3154,10 @@ endpoint_tests()
>  		pm_nl_add_endpoint $ns2 10.0.2.2 flags signal
>  		pm_nl_check_endpoint 0 "modif is allowed" \
>  			$ns2 10.0.2.2 id 1 flags signal
> +
> +		chk_mptcp_info subflows_1
> +		pm_nl_del_endpoint $ns2 1 10.0.2.2

Just to be sure: here, the userspace client will send the Netlink
message and wait for the kernel to do the action before stopping, right?
In other words, just after this line, the counter have been updated, no
need to wait?

> +		chk_mptcp_info subflows_0
>  		kill_tests_wait
>  	fi
>  

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

Re: [PATCH mptcp-next v5 0/7] mptcp: update userspace pm mptcp_info fields

[Matthieu Baerts]

Hi Geliang,

On 14/03/2023 08:31, Geliang Tang wrote:
> v5:
>  - fix a memleak error reported by CI.
>  - add more delay for userspace pm tests.

Thank you for this new version!

I have a couple of questions and comments in the different patches if
you don't mind :)

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

Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it

[Geliang Tang]

Hi Matt,

Thanks for your reviews.

We always remove address together with subflow, see
mptcp_nl_remove_subflow_and_signal_addr() or
mptcp_pm_remove_addrs_and_subflows().

So we always get "chk_rm_nr 1 1" in these tests:

        # single subflow, remove
        if reset "remove single subflow"; then
                pm_nl_set_limits $ns1 0 1
                pm_nl_set_limits $ns2 0 1
                pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
                run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
                chk_join_nr 1 1 1
                chk_rm_nr 1 1
        fi

        # single address, remove
        if reset "remove single address"; then
                pm_nl_set_limits $ns1 0 1
                pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
                pm_nl_set_limits $ns2 1 1
                run_tests $ns1 $ns2 10.0.1.1 0 -1 0 slow
                chk_join_nr 1 1 1
                chk_add_nr 1 1
                chk_rm_nr 1 1 invert
        fi

        # userspace pm add & remove address
        if reset_with_events "userspace pm add & remove address"; then
                set_userspace_pm $ns1
                pm_nl_set_limits $ns2 1 1
                run_tests $ns1 $ns2 10.0.1.1 0 userspace_1 0 slow
                chk_join_nr 1 1 1
                chk_add_nr 1 1
                chk_rm_nr 1 1 invert
                kill_events_pids
        fi

But the userspace remove subflow test got "chk_rm_nr 0 1":

        # userspace pm create destroy subflow
        if reset_with_events "userspace pm create destroy subflow"; then
                set_userspace_pm $ns2
                pm_nl_set_limits $ns1 0 1
                run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
                chk_join_nr 1 1 1
                chk_rm_nr 0 1
                kill_events_pids
        fi

"chk_rm_nr 0 1" means getting no rm_addr, but a rm_subflow.

This behavior is incorrect. This patch fix it. It looks through the
userspace_pm_local_addr_list to find the addr id, and send RM_ADDR
with this id.

So patch 2 is needed to add the address into
userspace_pm_local_addr_list, and patch 1 too, no clear the address
id.

Thanks,
-Geliang

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年3月17日周五 02:26写道：
>
> Hi Geliang,
>
> On 14/03/2023 08:31, Geliang Tang wrote:
> > Not only close the local subflow but also send RM_ADDR by invoking
> > mptcp_pm_remove_addr() to close the remote subflow when a subflow is
> > destroyed by userspace PM.
>
> Should it not be the responsibility of the userspace PM daemon to send
> this RM_ADDR?
>
> Do you maybe have a use case where it is interesting to force sending
> this RM_ADDR when asking for a destroy?
>
> Maybe we could do that only if the userspace PM asks for that by setting
> a new attribute or flag? (if there is an interest)
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it

[Matthieu Baerts]

Hi Geliang,

Thank you for your reply.

On 20/03/2023 12:41, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for your reviews.
> 
> We always remove address together with subflow, see
> mptcp_nl_remove_subflow_and_signal_addr() or
> mptcp_pm_remove_addrs_and_subflows().
> 
> So we always get "chk_rm_nr 1 1" in these tests:
> 
>         # single subflow, remove
>         if reset "remove single subflow"; then
>                 pm_nl_set_limits $ns1 0 1
>                 pm_nl_set_limits $ns2 0 1
>                 pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>                 run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
>                 chk_join_nr 1 1 1
>                 chk_rm_nr 1 1
>         fi
> 
>         # single address, remove
>         if reset "remove single address"; then
>                 pm_nl_set_limits $ns1 0 1
>                 pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>                 pm_nl_set_limits $ns2 1 1
>                 run_tests $ns1 $ns2 10.0.1.1 0 -1 0 slow
>                 chk_join_nr 1 1 1
>                 chk_add_nr 1 1
>                 chk_rm_nr 1 1 invert
>         fi
> 
>         # userspace pm add & remove address
>         if reset_with_events "userspace pm add & remove address"; then
>                 set_userspace_pm $ns1
>                 pm_nl_set_limits $ns2 1 1
>                 run_tests $ns1 $ns2 10.0.1.1 0 userspace_1 0 slow
>                 chk_join_nr 1 1 1
>                 chk_add_nr 1 1
>                 chk_rm_nr 1 1 invert
>                 kill_events_pids
>         fi
> 
> But the userspace remove subflow test got "chk_rm_nr 0 1":
> 
>         # userspace pm create destroy subflow
>         if reset_with_events "userspace pm create destroy subflow"; then
>                 set_userspace_pm $ns2
>                 pm_nl_set_limits $ns1 0 1
>                 run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
>                 chk_join_nr 1 1 1
>                 chk_rm_nr 0 1
>                 kill_events_pids
>         fi
> 
> "chk_rm_nr 0 1" means getting no rm_addr, but a rm_subflow.

Indeed but you have this behaviour only because the userspace PM didn't
send this RM ADDR, no? I agree it might be better to do so but it is not
mandatory if I'm not mistaken, especially for the client side. Then it
is up to the userspace PM to decide to send the RM ADDR or not and we
cannot force it I would say.

Or maybe the client side cannot send a RM ADDR? (on the other hand, the
address was not announced with an ADD_ADDR so why do you want to send a
RM_ADDR?)

(if there is a need, we can ease its use with an extra flag but I don't
think we should force this behaviour, it is up to the PM to do that, no?)

> This behavior is incorrect. This patch fix it. It looks through the
> userspace_pm_local_addr_list to find the addr id, and send RM_ADDR
> with this id.
> 
> So patch 2 is needed to add the address into
> userspace_pm_local_addr_list, and patch 1 too, no clear the address
> id.

Would it not work if you modify do_transfer() to use pm_nl_ctl to send
the RM_ADDR then do the destroy (I don't know if doing the opposite is
supported)?

But again, I don't think we need to send an RM_ADD if no ADD_ADDR have
been sent before by the client here.

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

Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it

[Geliang Tang]

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年3月21日周二 00:09写道：
>
> Hi Geliang,
>
> Thank you for your reply.
>
> On 20/03/2023 12:41, Geliang Tang wrote:
> > Hi Matt,
> >
> > Thanks for your reviews.
> >
> > We always remove address together with subflow, see
> > mptcp_nl_remove_subflow_and_signal_addr() or
> > mptcp_pm_remove_addrs_and_subflows().
> >
> > So we always get "chk_rm_nr 1 1" in these tests:
> >
> >         # single subflow, remove
> >         if reset "remove single subflow"; then
> >                 pm_nl_set_limits $ns1 0 1
> >                 pm_nl_set_limits $ns2 0 1
> >                 pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
> >                 run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
> >                 chk_join_nr 1 1 1
> >                 chk_rm_nr 1 1
> >         fi
> >
> >         # single address, remove
> >         if reset "remove single address"; then
> >                 pm_nl_set_limits $ns1 0 1
> >                 pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
> >                 pm_nl_set_limits $ns2 1 1
> >                 run_tests $ns1 $ns2 10.0.1.1 0 -1 0 slow
> >                 chk_join_nr 1 1 1
> >                 chk_add_nr 1 1
> >                 chk_rm_nr 1 1 invert
> >         fi
> >
> >         # userspace pm add & remove address
> >         if reset_with_events "userspace pm add & remove address"; then
> >                 set_userspace_pm $ns1
> >                 pm_nl_set_limits $ns2 1 1
> >                 run_tests $ns1 $ns2 10.0.1.1 0 userspace_1 0 slow
> >                 chk_join_nr 1 1 1
> >                 chk_add_nr 1 1
> >                 chk_rm_nr 1 1 invert
> >                 kill_events_pids
> >         fi
> >
> > But the userspace remove subflow test got "chk_rm_nr 0 1":
> >
> >         # userspace pm create destroy subflow
> >         if reset_with_events "userspace pm create destroy subflow"; then
> >                 set_userspace_pm $ns2
> >                 pm_nl_set_limits $ns1 0 1
> >                 run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
> >                 chk_join_nr 1 1 1
> >                 chk_rm_nr 0 1
> >                 kill_events_pids
> >         fi
> >
> > "chk_rm_nr 0 1" means getting no rm_addr, but a rm_subflow.
>
> Indeed but you have this behaviour only because the userspace PM didn't
> send this RM ADDR, no? I agree it might be better to do so but it is not
> mandatory if I'm not mistaken, especially for the client side. Then it
> is up to the userspace PM to decide to send the RM ADDR or not and we
> cannot force it I would say.
>
> Or maybe the client side cannot send a RM ADDR? (on the other hand, the
> address was not announced with an ADD_ADDR so why do you want to send a
> RM_ADDR?)
>
> (if there is a need, we can ease its use with an extra flag but I don't
> think we should force this behaviour, it is up to the PM to do that, no?)
>
> > This behavior is incorrect. This patch fix it. It looks through the
> > userspace_pm_local_addr_list to find the addr id, and send RM_ADDR
> > with this id.
> >
> > So patch 2 is needed to add the address into
> > userspace_pm_local_addr_list, and patch 1 too, no clear the address
> > id.
>
> Would it not work if you modify do_transfer() to use pm_nl_ctl to send
> the RM_ADDR then do the destroy (I don't know if doing the opposite is
> supported)?
>
> But again, I don't think we need to send an RM_ADD if no ADD_ADDR have
> been sent before by the client here.

No, we must send an RM_ADDR to shutdown the subflow and close ssk on
the other side. I guess we should hear Paolo's opinion here. @Paolo
Abeni

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

Re: [PATCH mptcp-next v5 3/7] mptcp: close remote subflow when destroying it

[Matthieu Baerts]

Hi Geliang,

On 21/03/2023 03:52, Geliang Tang wrote:
> Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年3月21日周二 00:09写道：
>>
>> Hi Geliang,
>>
>> Thank you for your reply.
>>
>> On 20/03/2023 12:41, Geliang Tang wrote:
>>> Hi Matt,
>>>
>>> Thanks for your reviews.
>>>
>>> We always remove address together with subflow, see
>>> mptcp_nl_remove_subflow_and_signal_addr() or
>>> mptcp_pm_remove_addrs_and_subflows().
>>>
>>> So we always get "chk_rm_nr 1 1" in these tests:
>>>
>>>         # single subflow, remove
>>>         if reset "remove single subflow"; then
>>>                 pm_nl_set_limits $ns1 0 1
>>>                 pm_nl_set_limits $ns2 0 1
>>>                 pm_nl_add_endpoint $ns2 10.0.3.2 flags subflow
>>>                 run_tests $ns1 $ns2 10.0.1.1 0 0 -1 slow
>>>                 chk_join_nr 1 1 1
>>>                 chk_rm_nr 1 1
>>>         fi
>>>
>>>         # single address, remove
>>>         if reset "remove single address"; then
>>>                 pm_nl_set_limits $ns1 0 1
>>>                 pm_nl_add_endpoint $ns1 10.0.2.1 flags signal
>>>                 pm_nl_set_limits $ns2 1 1
>>>                 run_tests $ns1 $ns2 10.0.1.1 0 -1 0 slow
>>>                 chk_join_nr 1 1 1
>>>                 chk_add_nr 1 1
>>>                 chk_rm_nr 1 1 invert
>>>         fi
>>>
>>>         # userspace pm add & remove address
>>>         if reset_with_events "userspace pm add & remove address"; then
>>>                 set_userspace_pm $ns1
>>>                 pm_nl_set_limits $ns2 1 1
>>>                 run_tests $ns1 $ns2 10.0.1.1 0 userspace_1 0 slow
>>>                 chk_join_nr 1 1 1
>>>                 chk_add_nr 1 1
>>>                 chk_rm_nr 1 1 invert
>>>                 kill_events_pids
>>>         fi
>>>
>>> But the userspace remove subflow test got "chk_rm_nr 0 1":
>>>
>>>         # userspace pm create destroy subflow
>>>         if reset_with_events "userspace pm create destroy subflow"; then
>>>                 set_userspace_pm $ns2
>>>                 pm_nl_set_limits $ns1 0 1
>>>                 run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
>>>                 chk_join_nr 1 1 1
>>>                 chk_rm_nr 0 1
>>>                 kill_events_pids
>>>         fi
>>>
>>> "chk_rm_nr 0 1" means getting no rm_addr, but a rm_subflow.
>>
>> Indeed but you have this behaviour only because the userspace PM didn't
>> send this RM ADDR, no? I agree it might be better to do so but it is not
>> mandatory if I'm not mistaken, especially for the client side. Then it
>> is up to the userspace PM to decide to send the RM ADDR or not and we
>> cannot force it I would say.
>>
>> Or maybe the client side cannot send a RM ADDR? (on the other hand, the
>> address was not announced with an ADD_ADDR so why do you want to send a
>> RM_ADDR?)
>>
>> (if there is a need, we can ease its use with an extra flag but I don't
>> think we should force this behaviour, it is up to the PM to do that, no?)
>>
>>> This behavior is incorrect. This patch fix it. It looks through the
>>> userspace_pm_local_addr_list to find the addr id, and send RM_ADDR
>>> with this id.
>>>
>>> So patch 2 is needed to add the address into
>>> userspace_pm_local_addr_list, and patch 1 too, no clear the address
>>> id.
>>
>> Would it not work if you modify do_transfer() to use pm_nl_ctl to send
>> the RM_ADDR then do the destroy (I don't know if doing the opposite is
>> supported)?
>>
>> But again, I don't think we need to send an RM_ADD if no ADD_ADDR have
>> been sent before by the client here.
> 
> No, we must send an RM_ADDR to shutdown the subflow and close ssk on
> the other side. I guess we should hear Paolo's opinion here. @Paolo
> Abeni

We discussed about that at the last meeting: we confirm that here, the
userspace daemon is the owner and then it is in charge of all actions
related to the PM including sending RM_ADDR then.

Typically, the RM_ADDR are sent when a previously announced address has
been removed. In the test you modified ("userspace pm create destroy
subflow"), it might even looks strange for the client to send a RM_ADDR
for a subflow it has created on an address it didn't explicitly
announced. The client no longer wants to use it, fine, it simply sends a
TCP FIN, no need to do more, no? (or at least, no need to force the send
of a RM_ADDR)

WDYT?

Maybe we could even think about changing the in-kernel PM not to send
the RM_ADDR if the address was not announced before?
(I think we should not change that, in the case of the in-kernel PM,
these RM_ADDR are sent only when the address has been explicitly removed
from the endpoints so it somehow makes sense)


While talking about that, I was thinking about this: for the moment, the
userspace PM can only send RM_ADDR for addresses it has previously
announced, right?
In this case, if the userspace PM daemon for the client side detects an
issue with its "2nd" address -- e.g. a Netlink event is sent to the
userspace daemon because a subflow got closed with an error or because
an IP address is no longer available -- the userspace PM daemon cannot
send an RM_ADDR to the server to tell it to stop sending data on this
subflow. I can create a new ticket on GitHub for that.

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