[PATCH mptcp-next v5 0/3] only remove entry from local_addr_list when sending a REMOVE_ADDR

[PATCH mptcp-next v5 0/3] only remove entry from local_addr_list when sending a REMOVE_ADDR

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v5:
 - rebased.
 - add Matt's reviewed tags.
 - add a RM_ADDR from the server to the client as Sashiko suggested.
 - mptcpd tests (make check) are still OK after this modification.

v4:
 - rebased.
 - Link: https://patchwork.kernel.org/project/mptcp/cover/cover.1776466833.git.tanggeliang@kylinos.cn/

v3:
 - update userspace selftests.
 - Link: https://patchwork.kernel.org/project/mptcp/cover/cover.1744787273.git.tanggeliang@kylinos.cn/

v2:
 - squash patch 1 into patch 2 as Mat suggested.

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

Geliang Tang (3):
  mptcp: pm: userspace: drop delete_local_addr helper
  selftests: mptcp: join: update userspace dump_addr outputs
  selftests: mptcp: userspace: send RM_ADDR between server and client

 net/mptcp/pm_userspace.c                      | 35 ++-----------------
 .../testing/selftests/net/mptcp/mptcp_join.sh |  6 ++--
 .../selftests/net/mptcp/userspace_pm.sh       | 16 +++++++++
 3 files changed, 21 insertions(+), 36 deletions(-)

-- 
2.53.0

[PATCH mptcp-next v5 1/3] mptcp: pm: userspace: drop delete_local_addr helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Address entries should not be removed from local_addr_list when a subflow
is deleted by the userspace PM, should only be removed when sending a
REMOVE_ADDR.

So mptcp_userspace_pm_delete_local_addr() helper shouldn't be called in
mptcp_pm_nl_subflow_create_doit() and mptcp_pm_nl_subflow_destroy_doit().

Since this helper is open-coding in mptcp_pm_nl_remove_doit(), it can be
dropped now.

Address entries are removed from local_addr_list when sending a REMOVE_ADDR
by the userspace PM, the local_addr_used counter of PM should also be
decremented accordingly.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 35 ++---------------------------------
 1 file changed, 2 insertions(+), 33 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index ad6ba658e5a5..45463f2f1e60 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -90,30 +90,6 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 	return ret;
 }
 
-/* If the subflow is closed from the other peer (not via a
- * subflow destroy command then), we want to keep the entry
- * not to assign the same ID to another address and to be
- * able to send RM_ADDR after the removal of the subflow.
- */
-static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
-						struct mptcp_pm_addr_entry *addr)
-{
-	struct sock *sk = (struct sock *)msk;
-	struct mptcp_pm_addr_entry *entry;
-
-	entry = mptcp_userspace_pm_lookup_addr(msk, &addr->addr);
-	if (!entry)
-		return -EINVAL;
-
-	/* TODO: a refcount is needed because the entry can
-	 * be used multiple times (e.g. fullmesh mode).
-	 */
-	list_del_rcu(&entry->list);
-	sock_kfree_s(sk, entry, sizeof(*entry));
-	msk->pm.local_addr_used--;
-	return 0;
-}
-
 static struct mptcp_pm_addr_entry *
 mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
 {
@@ -331,6 +307,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	list_del_rcu(&match->list);
+	msk->pm.local_addr_used--;
 	spin_unlock_bh(&msk->pm.lock);
 
 	mptcp_pm_remove_addr_entry(msk, match);
@@ -416,14 +393,9 @@ int mptcp_pm_nl_subflow_create_doit(struct sk_buff *skb, struct genl_info *info)
 	err = __mptcp_subflow_connect(sk, &local, &addr_r);
 	release_sock(sk);
 
-	if (err) {
+	if (err)
 		GENL_SET_ERR_MSG_FMT(info, "connect error: %d", err);
 
-		spin_lock_bh(&msk->pm.lock);
-		mptcp_userspace_pm_delete_local_addr(msk, &entry);
-		spin_unlock_bh(&msk->pm.lock);
-	}
-
  create_err:
 	sock_put(sk);
 	return err;
@@ -540,9 +512,6 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 		goto release_sock;
 	}
 
-	spin_lock_bh(&msk->pm.lock);
-	mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
-	spin_unlock_bh(&msk->pm.lock);
 	mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 	mptcp_close_ssk(sk, ssk, mptcp_subflow_ctx(ssk));
 	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
-- 
2.53.0

[PATCH mptcp-next v5 2/3] selftests: mptcp: join: update userspace dump_addr outputs

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Now address entries only be removed from local_addr_list when sending a
REMOVE_ADDR by the userspace PM, they're no longer removed when a subflow
is deleted.

To make userspace PM selftests in mptcp_join.sh pass, this patch updates
the outputs of dump_addr.

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 550a6b6117a9..e1cdde5ba722 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -4089,9 +4089,9 @@ userspace_tests()
 		userspace_pm_chk_get_addr "${ns1}" "20" "id 20 flags signal 10.0.3.1"
 		userspace_pm_rm_sf $ns1 "::ffff:10.0.2.1" $MPTCP_LIB_EVENT_SUB_ESTABLISHED
 		userspace_pm_chk_dump_addr "${ns1}" \
-			"id 20 flags signal 10.0.3.1" "after rm_sf 10"
+			$'id 10 flags signal 10.0.2.1\nid 20 flags signal 10.0.3.1' "after rm_sf 10"
 		userspace_pm_rm_addr $ns1 20
-		userspace_pm_chk_dump_addr "${ns1}" "" "after rm_addr 20"
+		userspace_pm_chk_dump_addr "${ns1}" "id 10 flags signal 10.0.2.1" "after rm_addr 20"
 		chk_rm_nr 1 1 invert
 		chk_mptcp_info subflows 0 subflows 0
 		chk_subflows_total 1 1
@@ -4119,7 +4119,7 @@ userspace_tests()
 		userspace_pm_chk_get_addr "${ns2}" "20" "id 20 flags subflow 10.0.3.2"
 		userspace_pm_rm_sf $ns2 10.0.3.2 $MPTCP_LIB_EVENT_SUB_ESTABLISHED
 		userspace_pm_chk_dump_addr "${ns2}" \
-			"" \
+			"id 20 flags subflow 10.0.3.2" \
 			"after rm_sf 20"
 		chk_rm_nr 0 1
 		chk_mptcp_info subflows 0 subflows 0
-- 
2.53.0

[PATCH mptcp-next v5 3/3] selftests: mptcp: userspace: send RM_ADDR between server and client

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Now address entries only be removed from local_addr_list when sending a
REMOVE_ADDR by the userspace PM, they're no longer removed when a subflow
is deleted.

To make the original userspace PM selftests pass, this patch sends RM_ADDR
from server to client machine at the end of each test, and always sends
RM_ADDR from client to server machine to cleanup address IDs for the next
new test.

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/net/mptcp/userspace_pm.sh  | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index e9ae1806ab07..3e7e2a633448 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -608,6 +608,10 @@ test_subflows()
 	   "$client4_token"
 	sleep 0.5
 
+	# RM_ADDR from server to client machine
+	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server4_token"
+	sleep 0.5
+
 	# Attempt to add a listener at dead:beef:2::2:<subflow-port>
 	ip netns exec "$ns2" ./pm_nl_ctl listen dead:beef:2::2\
 	   "$client6_port" &
@@ -642,6 +646,8 @@ test_subflows()
 			      "dead:beef:2::1" "dead:beef:2::2" "$client6_port" "23"\
 			      "$client_addr_id" "ns1" "ns2"
 
+	# RM_ADDR from server to client machine
+	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server6_token"
 	# RM_ADDR from client to server machine
 	ip netns exec "$ns2" ./pm_nl_ctl rem id $client_addr_id token\
 	   "$client6_token"
@@ -680,6 +686,8 @@ test_subflows()
 	verify_subflow_events "$server_evts" "$SUB_CLOSED" "$server4_token" "$AF_INET" "10.0.2.1"\
 			      "10.0.2.2" "$new4_port" "23" "$client_addr_id" "ns1" "ns2"
 
+	# RM_ADDR from server to client machine
+	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server4_token"
 	# RM_ADDR from client to server machine
 	ip netns exec "$ns2" ./pm_nl_ctl rem id $client_addr_id token\
 	   "$client4_token"
@@ -721,6 +729,8 @@ test_subflows()
 	# RM_ADDR from server to client machine
 	ip netns exec "$ns1" ./pm_nl_ctl rem id $server_addr_id token\
 	   "$server4_token"
+	# RM_ADDR from client to server machine
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client4_token"
 	sleep 0.5
 
 	# Attempt to add a listener at dead:beef:2::1:<subflow-port>
@@ -760,6 +770,8 @@ test_subflows()
 	# RM_ADDR6 from server to client machine
 	ip netns exec "$ns1" ./pm_nl_ctl rem id $server_addr_id token\
 	   "$server6_token"
+	# RM_ADDR6 from client to server machine
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client6_token"
 	sleep 0.5
 
 	# Attempt to add a listener at 10.0.2.1:<new-port>
@@ -797,6 +809,8 @@ test_subflows()
 	# RM_ADDR from server to client machine
 	ip netns exec "$ns1" ./pm_nl_ctl rem id $server_addr_id token\
 	   "$server4_token"
+	# RM_ADDR from client to server machine
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client4_token"
 }
 
 test_subflows_v4_v6_mix()
@@ -844,6 +858,8 @@ test_subflows_v4_v6_mix()
 	# RM_ADDR from server to client machine
 	ip netns exec "$ns1" ./pm_nl_ctl rem id $server_addr_id token\
 	   "$server6_token"
+	# RM_ADDR from client to server machine
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client6_token"
 	sleep 0.5
 }
 
-- 
2.53.0

Re: [PATCH mptcp-next v5 0/3] only remove entry from local_addr_list when sending a REMOVE_ADDR

[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): packetdrill_dss ⚠️ 
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/28223950581

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/2e04a2de29b7
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1116901


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

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 (NGI0 Core)

Re: [PATCH mptcp-next v5 0/3] only remove entry from local_addr_list when sending a REMOVE_ADDR

[gang.yan]

> 
> From: Geliang Tang <tanggeliang@kylinos.cn>
>  
>  v5:
>  - rebased.
>  - add Matt's reviewed tags.
>  - add a RM_ADDR from the server to the client as Sashiko suggested.
>  - mptcpd tests (make check) are still OK after this modification.
> 
Hi Matt,

Sorry for the off-list email before, I forget to cc the mptcp@lists.linux.dev

First, no offense intended — this is in no way a comment on this patch. 
The patch itself looks good to me. I'm replying here only because the
thought came up while reading it.

While looking at mptcpd recently, I noticed that 'make check' doesn't
seem to exercise the userspace PM logic in a meaningful way. The
userspace PM cases in test-commands issue the per-connection (mptcpd_pm_*)
calls with a fake token that doesn't correspond to any real MPTCP connection,
so they only really verify that the command dispatches and the genl request
is sent without crashing.

This made me wonder whether it would be worthwhile to add functional tests
for mptcpd within the selftest framework — for example, via a dedicated script
(something like selftest_mptcpd.sh) that actually starts mptcpd with specific
plugins (e.g., sspi) in a namespace/veth environment, and then asserts the
expected path-manager behavior end-to-end.

In other words, I'm asking: does the community think mptcpd's userspace PM
functionality deserves similar test coverage within selftest?

Thanks
Gang

> 
> v4:
>  - rebased.
>  - Link: https://patchwork.kernel.org/project/mptcp/cover/cover.1776466833.git.tanggeliang@kylinos.cn/
>  
>  v3:
>  - update userspace selftests.
>  - Link: https://patchwork.kernel.org/project/mptcp/cover/cover.1744787273.git.tanggeliang@kylinos.cn/
>  
>  v2:
>  - squash patch 1 into patch 2 as Mat suggested.
>  
>  Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403
>  
>  Geliang Tang (3):
>  mptcp: pm: userspace: drop delete_local_addr helper
>  selftests: mptcp: join: update userspace dump_addr outputs
>  selftests: mptcp: userspace: send RM_ADDR between server and client
>  
>  net/mptcp/pm_userspace.c | 35 ++-----------------
>  .../testing/selftests/net/mptcp/mptcp_join.sh | 6 ++--
>  .../selftests/net/mptcp/userspace_pm.sh | 16 +++++++++
>  3 files changed, 21 insertions(+), 36 deletions(-)
>  
>  -- 
>  2.53.0
>