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

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

[Geliang Tang]

v7:
 - fix userspace_pm.sh errors reported by CI.
 - only remove addrs in mptcp_nl_cmd_remove().

v6:
 - send a RM ADDR from userspace.

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: pass addr to mptcp_pm_alloc_anno_list
  mptcp: only remove addrs in nl_cmd_remove
  mptcp: don't clear userspace pm addr id
  mptcp: add addr into userspace pm list
  mptcp: increase userspace pm add_addr_signaled
  mptcp: update userspace pm subflows
  selftests: mptcp: check userspace mptcp_info

 net/mptcp/pm.c                                | 21 ++++++++++---
 net/mptcp/pm_netlink.c                        | 27 +++++++++++++----
 net/mptcp/pm_userspace.c                      | 30 +++++++++++++++++--
 net/mptcp/protocol.h                          |  4 ++-
 .../testing/selftests/net/mptcp/mptcp_join.sh | 22 +++++++++++++-
 5 files changed, 90 insertions(+), 14 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v7 1/7] mptcp: pass addr to mptcp_pm_alloc_anno_list

[Geliang Tang]

Pass addr to mptcp_pm_alloc_anno_list() instead of entry.

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e8336b8bd30e..a02822111218 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -342,7 +342,7 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 }
 
 bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
-			      const struct mptcp_pm_addr_entry *entry)
+			      const struct mptcp_addr_info *addr)
 {
 	struct mptcp_pm_add_entry *add_entry = NULL;
 	struct sock *sk = (struct sock *)msk;
@@ -350,7 +350,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 
 	lockdep_assert_held(&msk->pm.lock);
 
-	add_entry = mptcp_lookup_anno_list_by_saddr(msk, &entry->addr);
+	add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
 
 	if (add_entry) {
 		if (mptcp_pm_is_kernel(msk))
@@ -367,7 +367,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 
 	list_add(&add_entry->list, &msk->pm.anno_list);
 
-	add_entry->addr = entry->addr;
+	add_entry->addr = *addr;
 	add_entry->sock = msk;
 	add_entry->retrans_times = 0;
 
@@ -574,7 +574,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 			return;
 
 		if (local) {
-			if (mptcp_pm_alloc_anno_list(msk, local)) {
+			if (mptcp_pm_alloc_anno_list(msk, &local->addr)) {
 				__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
 				msk->pm.add_addr_signaled++;
 				mptcp_pm_announce_addr(msk, &local->addr, false);
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 27a275805c06..4d0e54fab5cf 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -170,7 +170,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
 	lock_sock((struct sock *)msk);
 	spin_lock_bh(&msk->pm.lock);
 
-	if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
+	if (mptcp_pm_alloc_anno_list(msk, &addr_val.addr)) {
 		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
 		mptcp_pm_nl_addr_send_ack(msk);
 	}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 5a35c77723e3..990c21a97975 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -822,7 +822,7 @@ int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
 				 struct mptcp_addr_info *rem,
 				 u8 bkup);
 bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
-			      const struct mptcp_pm_addr_entry *entry);
+			      const struct mptcp_addr_info *addr);
 void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
 bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
 struct mptcp_pm_add_entry *
-- 
2.35.3

[PATCH mptcp-next v7 2/7] mptcp: only remove addrs in nl_cmd_remove

[Geliang Tang]

Only remove addrs in mptcp_nl_cmd_remove(), add a new helper
mptcp_pm_remove_addrs() to do this.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_netlink.c   | 17 +++++++++++++++++
 net/mptcp/pm_userspace.c |  2 +-
 net/mptcp/protocol.h     |  2 ++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index a02822111218..dd15ed96fae8 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1555,6 +1555,23 @@ static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
+void mptcp_pm_remove_addrs(struct mptcp_sock *msk,
+			   struct list_head *rm_list)
+{
+	struct mptcp_rm_list alist = { .nr = 0 };
+	struct mptcp_pm_addr_entry *entry;
+
+	list_for_each_entry(entry, rm_list, list) {
+		if (remove_anno_list_by_saddr(msk, &entry->addr) &&
+		    alist.nr < MPTCP_RM_IDS_MAX) {
+			alist.ids[alist.nr++] = entry->addr.id;
+			spin_lock_bh(&msk->pm.lock);
+			mptcp_pm_remove_addr(msk, &alist);
+			spin_unlock_bh(&msk->pm.lock);
+		}
+	}
+}
+
 void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 					struct list_head *rm_list)
 {
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 4d0e54fab5cf..07714edb9086 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -232,7 +232,7 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 
 	list_move(&match->list, &free_list);
 
-	mptcp_pm_remove_addrs_and_subflows(msk, &free_list);
+	mptcp_pm_remove_addrs(msk, &free_list);
 
 	release_sock((struct sock *)msk);
 
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 990c21a97975..535c1b3ae6ed 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -845,6 +845,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   bool echo);
 int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
 int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
+void mptcp_pm_remove_addrs(struct mptcp_sock *msk,
+			   struct list_head *rm_list);
 void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 					struct list_head *rm_list);
 
-- 
2.35.3

[PATCH mptcp-next v7 3/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 dd15ed96fae8..ca1d141e3a93 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 07714edb9086..312fdce174fa 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 v7 4/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 | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 312fdce174fa..99a3968f38ac 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -301,6 +301,17 @@ 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);
+	mptcp_pm_alloc_anno_list(msk, &addr_l);
+	msk->pm.local_addr_used++;
+	spin_unlock_bh(&msk->pm.lock);
+
 	lock_sock(sk);
 
 	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
@@ -419,6 +430,18 @@ 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)) {
+				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);
-- 
2.35.3

[PATCH mptcp-next v7 5/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 99a3968f38ac..e5b250d39e57 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.addr)) {
+		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 v7 6/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 e5b250d39e57..4da7f0ac7d8d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -311,6 +311,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 	spin_lock_bh(&msk->pm.lock);
 	mptcp_pm_alloc_anno_list(msk, &addr_l);
 	msk->pm.local_addr_used++;
+	msk->pm.subflows++;
 	spin_unlock_bh(&msk->pm.lock);
 
 	lock_sock(sk);
-- 
2.35.3

[PATCH mptcp-next v7 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>
---
 .../testing/selftests/net/mptcp/mptcp_join.sh | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index fafd19ec7e1f..bc47b99f47e5 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -842,8 +842,20 @@ 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
+				addr="::ffff:$addr"
+				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')
+				ip netns exec ${listener_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))
@@ -906,11 +918,15 @@ 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 rem token $tk id $id
 				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))
@@ -3123,7 +3139,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
 }
@@ -3148,6 +3164,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: selftests: mptcp: check userspace mptcp_info: Build Failure

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://patchwork.kernel.org/project/mptcp/patch/3ddbc897e2e5a766a2b4df591e747126288661ff.1681463340.git.geliang.tang@suse.com/
  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/4698370517

Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/aa4e473b1157

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

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

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):
  - Script error! ❓:
  - Task: https://cirrus-ci.com/task/5081880182128640
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5081880182128640/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Script error! ❓:
  - Task: https://cirrus-ci.com/task/6207780088971264
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6207780088971264/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Script error! ❓:
  - Task: https://cirrus-ci.com/task/5926305112260608
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5926305112260608/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Script error! ❓:
  - Task: https://cirrus-ci.com/task/4800405205417984
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4800405205417984/summary/summary.txt

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


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: 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):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6414330460110848
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6414330460110848/summary/summary.txt

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

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

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

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


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 v7 0/7] mptcp: update userspace pm mptcp_info fields

[Matthieu Baerts]

Hi Geliang,

On 14/04/2023 11:11, Geliang Tang wrote:
> v7:
>  - fix userspace_pm.sh errors reported by CI.
>  - only remove addrs in mptcp_nl_cmd_remove().
> 
> v6:
>  - send a RM ADDR from userspace.

Thank you for this new version!

I have a few comments, some from v5 and some new ones.

Sorry to insist about the reason in the commit messages but I think it
is best to have them clear :)

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

Re: [PATCH mptcp-next v7 1/7] mptcp: pass addr to mptcp_pm_alloc_anno_list

[Matthieu Baerts]

Hi Geliang,

Thank you for this patch!

On 14/04/2023 11:11, Geliang Tang wrote:
> Pass addr to mptcp_pm_alloc_anno_list() instead of entry.

Do you mind just adding the reason why you need that please?

We can easily see you are changing the signature of the function but it
is harder to find why. We can understand that when looking at the patch
4/7 but when looking at this patch alone -- especially later, out of
this context -- it will be hard to guess why this patch was needed.

Also, if you don't mind, may you just say that we can reduce the scope,
e.g. in mptcp_pm_alloc_anno_list(), we only access "entry->addr", we can
then restrict to the pointer to "addr" then.

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

Re: [PATCH mptcp-next v7 2/7] mptcp: only remove addrs in nl_cmd_remove

[Matthieu Baerts]

Hi Geliang,

On 14/04/2023 11:11, Geliang Tang wrote:
> Only remove addrs in mptcp_nl_cmd_remove(), add a new helper
> mptcp_pm_remove_addrs() to do this.

Thank you, the patch looks good to me but here as well, I think it is
important to explain the reason why you are doing that, e.g. according
to the specs, the MPTCP_CMD_REMOVE Netlink command should only send
RM_ADDRs, not deleting associated subflows.

Because it was not supposed to delete subflows, I guess you can add:

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


Also, do you not need to modify the selftests because you changed the
behaviour? I guess "userspace pm add & remove address" and "userspace pm
create destroy subflow" are affected by this modification, no? (it seems
yes, when looking at patch 7/7)
Do you mind doing this modification in the same commit please? (or in a
dedicated commit after this one here explaining the link -- but maybe
clearer to do that in the same commit)


(...)

> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 990c21a97975..535c1b3ae6ed 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -845,6 +845,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>  			   bool echo);
>  int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
>  int mptcp_pm_remove_subflow(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
> +void mptcp_pm_remove_addrs(struct mptcp_sock *msk,
> +			   struct list_head *rm_list);

Small detail: could this go to the previous line please?

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

Re: [PATCH mptcp-next v7 3/7] mptcp: don't clear userspace pm addr id

[Matthieu Baerts]

Hi Geliang,

On 14/04/2023 11:11, Geliang Tang wrote:
> Don't clear the addr id in mptcp_userspace_pm_get_local_id(), clear it
> in mptcp_pm_nl_get_local_id() instead.

Even if it makes sense to do it like that, I guess you are preventing an
issue when using mptcp_userspace_pm_get_local_id() in the next patch. Do
you mind to also add a clear reason in the commit message here as well
please?

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

Re: [PATCH mptcp-next v7 4/7] mptcp: add addr into userspace pm list

[Matthieu Baerts]

Hi Geliang,

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

I'm sorry to send the same kind of comment as on the previous patches
and on the version 5 but can you also add the reason why you need to do
that in the commit message please?


Also, it looks like you are fixing two issues (or adding two features) here:

- Being able to send a remove addr for any additional subflow that have
been created (MPTCP_PM_CMD_SUBFLOW_CREATE) but not announced
(MPTCP_PM_CMD_ADD_ADDR): a part of issue #379 (this doesn't fix the
possibility to remove ID 0 from what I see)

- Increment local_addr_used: a part of issue #329

No?

Maybe could you split this in two? e.g. modifying local_addr_used
counter in another commit?

Could you add:

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

and:

  Fixes: 702c2f646d42 ("mptcp: netlink: allow userspace-driven subflow
establishment")
  Link: https://github.com/multipath-tcp/mptcp_net-next/issues/329

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 312fdce174fa..99a3968f38ac 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -301,6 +301,17 @@ 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);
> +	mptcp_pm_alloc_anno_list(msk, &addr_l);
> +	msk->pm.local_addr_used++;

Correct me if I'm wrong but if the local address has already been used
before, we are going to increment the counter while we should not, no?
e.g. if the client re-use the same local address to create multiple
subflows to different IP.

mptcp_userspace_pm_append_new_local_addr() should probably report if
there was a match of if a new entry has been added.

> +	spin_unlock_bh(&msk->pm.lock);
> +
>  	lock_sock(sk);
>  
>  	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);

Also, should we not increment the counter if there was no errors? (or
maybe in case of errors, the counter is decremented elsewhere? I didn't
check but I don't think so)

While at it, in case of errors, should we not also remove addr_l from
the local list?

> @@ -419,6 +430,18 @@ 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)) {
> +				list_del_rcu(&entry->list);
> +				kfree(entry);
> +				msk->pm.local_addr_used--;

Here as well, I don't think you can remove the entry and decrement the
counter if the local address is used by multiple subflows (e.g. fullmesh
mode), no?

The entry might need a refcount.

> +				break;
> +			}
> +		}
> +		spin_unlock_bh(&msk->pm.lock);

Do we need to do the same (remove the entry and decrement the counter)
when the subflow is removed from the other side or because of a network
error?

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

Re: [PATCH mptcp-next v7 5/7] mptcp: increase userspace pm add_addr_signaled

[Matthieu Baerts]

Hi Geliang,

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

Do you mind adding:

  Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
  Link: https://github.com/multipath-tcp/mptcp_net-next/issues/329

It looks like a bug: we forgot to update existing counters in this new mode.

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

Re: [PATCH mptcp-next v7 6/7] mptcp: update userspace pm subflows

[Matthieu Baerts]

Hi Geliang,


On 14/04/2023 11:11, 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.

Do you mind adding:

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

Same here, it looks like a bug: we forgot to update existing counters in
this new mode.

Also, may you also add that this modification is similar to how the
in-kernel PM is updating the counter: when additional subflows are
created/removed?

> 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);

Should you not also decrement the counter only if one of these
conditions is met to keep the same behaviour as what is done with the
in-kernel PM: 'subflows' counter only look at the additional subflows?

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

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

[Matthieu Baerts]

Hi Geliang,

I think you missed my comments I sent on v5. I re-added them here +
additional ones.

On 14/04/2023 11:11, 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>
> ---
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index fafd19ec7e1f..bc47b99f47e5 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -842,8 +842,20 @@ 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

Probably best to do the check after the sleep here just below.

Also, this will print a message before displaying the title of the test,
can you fix that please? → maybe do the checks in endpoint_tests()?

(I'm not a big fan of this sleep: it might take longer on a very
slow/busy environment making the test unstable (or way shorter and we
wait for nothing) → but it was already there before so OK to keep it)

>  				sleep 1
>  				ip netns exec ${listener_ns} ./pm_nl_ctl rem token $tk id $id
> +				addr="::ffff:$addr"
> +				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')
> +				ip netns exec ${listener_ns} ./pm_nl_ctl dsf lip $addr lport $sp \
> +									rip $da rport $dp token $tk

See my comment on patch 2/7, this should be part of patch 2/7 if you
don't mind.

Also, on slow environments, could we not have a situation where the
REMOVE_ADDR signal is sent to the other peer, it directly reacts by
removing the subflow and at the end, this "dsf" command is not doing
anything, causing the test to fail because it expects the listener side
to delete the address.

Should you not first delete the subflow, then send the remove addr? (or
skip the deletion of the subflow command and expect the client to remove
the subflow?)

> +				sleep 1
> +				chk_mptcp_info subflows_0
>  			fi
>  
>  			counter=$((counter + 1))
> @@ -906,11 +918,15 @@ 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

Here as well: it would be better to do that after the sleep here below
and the message will be printed before the title. → maybe do the checks
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 rem token $tk id $id

I think this new line and the switch from "chk_rm_nr 0 1" to "chk_rm_nr
1 1" here below should be part of a dedicated commit and mention it is
linked to patch 4/7 ("mptcp: add addr into userspace pm list"). It might
even make sense to add this new dedicated commit just after this patch
4/7, no?

>  				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))
> @@ -3123,7 +3139,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

(see above: to this can go to a dedicated commit)

>  		kill_events_pids
>  	fi
>  }
> @@ -3148,6 +3164,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

It might make more sense to add "chk_mptcp_info subflows_1" after each
call to pm_nl_check_endpoint here above, no?

> +		pm_nl_del_endpoint $ns2 1 10.0.2.2

You need to add at least have "sleep 0.5" here I think.
Or better, use:

  wait_rm_addr ${ns2} 1

> +		chk_mptcp_info subflows_0


>  		kill_tests_wait
>  	fi
>  

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