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

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

[Geliang Tang]

From: Geliang Tang <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 (2):
  mptcp: pm: userspace: drop delete_local_addr helper
  selftests: mptcp: send REMOVE_ADDR after subflow is deleted

 net/mptcp/pm_userspace.c                      | 37 +++----------------
 .../testing/selftests/net/mptcp/mptcp_join.sh |  4 +-
 .../selftests/net/mptcp/userspace_pm.sh       |  6 +++
 3 files changed, 14 insertions(+), 33 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v2 1/2] 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
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 37 +++++--------------------------------
 1 file changed, 5 insertions(+), 32 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 7fc19b844384..3824b4165421 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);
@@ -408,14 +385,13 @@ 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);
+		goto create_err;
+	}
 
 	spin_lock_bh(&msk->pm.lock);
-	if (err)
-		mptcp_userspace_pm_delete_local_addr(msk, &entry);
-	else
-		msk->pm.subflows++;
+	msk->pm.subflows++;
 	spin_unlock_bh(&msk->pm.lock);
 
  create_err:
@@ -534,9 +510,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.43.0

[PATCH mptcp-next v2 2/2] selftests: mptcp: send REMOVE_ADDR after subflow is deleted

[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 always sends
a REMOVE_ADDR when a subflow is deleted.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh   | 4 +++-
 tools/testing/selftests/net/mptcp/userspace_pm.sh | 6 ++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index b8af65373b3a..c138b4913ad8 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -3624,11 +3624,12 @@ userspace_tests()
 		userspace_pm_chk_get_addr "${ns1}" "10" "id 10 flags signal 10.0.2.1"
 		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_rm_addr $ns1 10
 		userspace_pm_chk_dump_addr "${ns1}" \
 			"id 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"
-		chk_rm_nr 1 1 invert
+		chk_rm_nr 2 1 invert
 		chk_mptcp_info subflows 0 subflows 0
 		chk_subflows_total 1 1
 		kill_events_pids
@@ -3653,6 +3654,7 @@ userspace_tests()
 			"subflow"
 		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_rm_addr $ns2 20
 		userspace_pm_chk_dump_addr "${ns2}" \
 			"" \
 			"after rm_sf 20"
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index 333064b0b5ac..c16465c9c2ca 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -628,6 +628,7 @@ test_subflows()
 	:>"$server_evts"
 	ip netns exec "$ns1" ./pm_nl_ctl dsf lip dead:beef:2::1 lport "$sport" rip\
 	   dead:beef:2::2 rport "$client6_port" token "$server6_token"
+	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server6_token"
 	sleep 0.5
 	verify_subflow_events "$server_evts" "$SUB_CLOSED" "$server6_token" "$AF_INET6"\
 			      "dead:beef:2::1" "dead:beef:2::2" "$client6_port" "23"\
@@ -667,6 +668,7 @@ test_subflows()
 	:>"$server_evts"
 	ip netns exec "$ns1" ./pm_nl_ctl dsf lip 10.0.2.1 lport "$sport" rip 10.0.2.2 rport\
 	   $new4_port token "$server4_token"
+	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server4_token"
 	sleep 0.5
 	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"
@@ -705,6 +707,7 @@ test_subflows()
 	:>"$client_evts"
 	ip netns exec "$ns2" ./pm_nl_ctl dsf lip 10.0.2.2 lport "$sport" rip 10.0.2.1 rport\
 	   $app4_port token "$client4_token"
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client4_token"
 	sleep 0.5
 	verify_subflow_events "$client_evts" "$SUB_CLOSED" "$client4_token" "$AF_INET" "10.0.2.2"\
 			      "10.0.2.1" "$app4_port" "23" "$server_addr_id" "ns2" "ns1"
@@ -744,6 +747,7 @@ test_subflows()
 	:>"$client_evts"
 	ip netns exec "$ns2" ./pm_nl_ctl dsf lip dead:beef:2::2 lport "$sport" rip\
 	   dead:beef:2::1 rport $app6_port token "$client6_token"
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client6_token"
 	sleep 0.5
 	verify_subflow_events $client_evts $SUB_CLOSED $client6_token $AF_INET6 "dead:beef:2::2"\
 			      "dead:beef:2::1" "$app6_port" "23" "$server_addr_id" "ns2" "ns1"
@@ -781,6 +785,7 @@ test_subflows()
 	:>"$client_evts"
 	ip netns exec "$ns2" ./pm_nl_ctl dsf lip 10.0.2.2 lport "$sport" rip 10.0.2.1 rport\
 	   $new4_port token "$client4_token"
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client4_token"
 	sleep 0.5
 	verify_subflow_events "$client_evts" "$SUB_CLOSED" "$client4_token" "$AF_INET" "10.0.2.2"\
 			      "10.0.2.1" "$new4_port" "23" "$server_addr_id" "ns2" "ns1"
@@ -827,6 +832,7 @@ test_subflows_v4_v6_mix()
 	:>"$client_evts"
 	ip netns exec "$ns2" ./pm_nl_ctl dsf lip 10.0.2.2 lport "$sport" rip 10.0.2.1 rport\
 	   $app6_port token "$client6_token"
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client6_token"
 	sleep 0.5
 	verify_subflow_events "$client_evts" "$SUB_CLOSED" "$client6_token" \
 			      "$AF_INET" "10.0.2.2" "10.0.2.1" "$app6_port" "23"\
-- 
2.43.0

Re: [PATCH mptcp-next v2 0/2] 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: Success! ✅
- KVM Validation: debug: 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/14372398128

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


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 v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Mat Martineau]

On Thu, 10 Apr 2025, Geliang Tang wrote:

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

Hi Geliang -

Have you tried this patch with mptcpd? Does it affect any tests there?


- Mat


> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> net/mptcp/pm_userspace.c | 37 +++++--------------------------------
> 1 file changed, 5 insertions(+), 32 deletions(-)
>
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 7fc19b844384..3824b4165421 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);
> @@ -408,14 +385,13 @@ 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);
> +		goto create_err;
> +	}
>
> 	spin_lock_bh(&msk->pm.lock);
> -	if (err)
> -		mptcp_userspace_pm_delete_local_addr(msk, &entry);
> -	else
> -		msk->pm.subflows++;
> +	msk->pm.subflows++;
> 	spin_unlock_bh(&msk->pm.lock);
>
>  create_err:
> @@ -534,9 +510,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.43.0
>
>
>

Re: [PATCH mptcp-next v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Geliang Tang]

Hi Mat, Matt,

On Fri, 2025-04-11 at 17:26 -0700, Mat Martineau wrote:
> On Thu, 10 Apr 2025, Geliang Tang wrote:
> 
> > 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.
> > 
> 
> Hi Geliang -
> 
> Have you tried this patch with mptcpd? Does it affect any tests
> there?

I haven't tested it. To be honest, I don't know how to run the test
items of mptcpd. Can you give me some guidance?

Thanks,
-Geliang

> 
> 
> - Mat
> 
> 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > net/mptcp/pm_userspace.c | 37 +++++--------------------------------
> > 1 file changed, 5 insertions(+), 32 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index 7fc19b844384..3824b4165421 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);
> > @@ -408,14 +385,13 @@ 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);
> > +		goto create_err;
> > +	}
> > 
> > 	spin_lock_bh(&msk->pm.lock);
> > -	if (err)
> > -		mptcp_userspace_pm_delete_local_addr(msk, &entry);
> > -	else
> > -		msk->pm.subflows++;
> > +	msk->pm.subflows++;
> > 	spin_unlock_bh(&msk->pm.lock);
> > 
> >  create_err:
> > @@ -534,9 +510,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.43.0
> > 
> > 
> > 

Re: [PATCH mptcp-next v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Geliang Tang]

Hi Mat, Matt,

On Tue, 2025-04-15 at 11:09 +0800, Geliang Tang wrote:
> Hi Mat, Matt,
> 
> On Fri, 2025-04-11 at 17:26 -0700, Mat Martineau wrote:
> > On Thu, 10 Apr 2025, Geliang Tang wrote:
> > 
> > > 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.
> > > 
> > 
> > Hi Geliang -
> > 
> > Have you tried this patch with mptcpd? Does it affect any tests
> > there?
> 
> I haven't tested it. To be honest, I don't know how to run the test
> items of mptcpd. Can you give me some guidance?

I just tested it this way, and here are the test results:

~/mptcpd/tests$ LD_PRELOAD=../src/.libs/libmptcpwrap.so ./mptcpwrap-
tester 
Test case 0: PASS
Test case 1: PASS
Test case 2: PASS
Test case 3: PASS
Test case 4: PASS
Test case 5: PASS
Test case 6: PASS

~/mptcpd/tests$ LD_PRELOAD=../src/.libs/libmptcpwrap.so ./test-
mptcpwrap 
Test case 0: PASS
Test case 1: PASS
Test case 2: PASS
Test case 3: mptcpwrap: changing socket protocol from 0x0 to 0x106
(IPPROTO_MPTCP) for family 0x2 type 0x1 fd 3
PASS
Test case 4: mptcpwrap: changing socket protocol from 0x0 to 0x106
(IPPROTO_MPTCP) for family 0xa type 0x1 fd 3
PASS
Test case 5: mptcpwrap: changing socket protocol from 0x6 to 0x106
(IPPROTO_MPTCP) for family 0x2 type 0x1 fd 3
PASS
Test case 6: mptcpwrap: changing socket protocol from 0x6 to 0x106
(IPPROTO_MPTCP) for family 0xa type 0x1 fd 3
PASS

~/mptcpd/tests$ LD_PRELOAD=../src/.libs/libmptcpwrap.so
TEST_PLUGIN_DIR=./plugins ./test-start-stop
Running non-interactive sudo check...
    succeeded
MPTCP is not enabled in the kernel.
Try 'sysctl -w net.mptcp.enabled=1'.
Required kernel MPTCP support not available.
TEST CASE: net.mptcp.enabled = 0 ...passed
Unable to load path manager plugins.
TEST CASE: net.mptcp.enabled = 1 ...failed

~/mptcpd/tests$ sudo ./test-bad-log-empty 
mptcpd: Unknown logging option: ""
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-log-long 
mptcpd: Unknown logging option: "foo"
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-log-short 
mptcpd: Unknown logging option: "foo"
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-option 
/home/tgl/mptcpd/src/.libs/mptcpd: unrecognized option '--foo'
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-path-manager 
mptcpd: Empty default path manager plugin command line option.
Try `mptcpd --help' or `mptcpd --usage' for more information.
~/mptcpd/tests$ sudo ./test-bad-plugin-dir 
mptcpd: Empty plugin directory command line option.
Try `mptcpd --help' or `mptcpd --usage' for more information.

These results appear to be unchanged from before. And in fact these
tests do not test userspace pm.

So it seems that this patch will not affect the tests of mptcpd.

Thanks,
-Geliang

> 
> Thanks,
> -Geliang
> 
> > 
> > 
> > - Mat
> > 
> > 
> > > Closes:
> > > https://github.com/multipath-tcp/mptcp_net-next/issues/403
> > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > > ---
> > > net/mptcp/pm_userspace.c | 37 +++++------------------------------
> > > --
> > > 1 file changed, 5 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > > index 7fc19b844384..3824b4165421 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);
> > > @@ -408,14 +385,13 @@ 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);
> > > +		goto create_err;
> > > +	}
> > > 
> > > 	spin_lock_bh(&msk->pm.lock);
> > > -	if (err)
> > > -		mptcp_userspace_pm_delete_local_addr(msk,
> > > &entry);
> > > -	else
> > > -		msk->pm.subflows++;
> > > +	msk->pm.subflows++;
> > > 	spin_unlock_bh(&msk->pm.lock);
> > > 
> > >  create_err:
> > > @@ -534,9 +510,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.43.0
> > > 
> > > 
> > > 
> 
> 

Re: [PATCH mptcp-next v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Matthieu Baerts]

Hi Geliang,

On 15/04/2025 11:18, Geliang Tang wrote:
> On Tue, 2025-04-15 at 11:09 +0800, Geliang Tang wrote:
>> On Fri, 2025-04-11 at 17:26 -0700, Mat Martineau wrote:
>>> On Thu, 10 Apr 2025, Geliang Tang wrote:
>>>
>>>> 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.
>>>>
>>>
>>> Hi Geliang -
>>>
>>> Have you tried this patch with mptcpd? Does it affect any tests
>>> there?
>>
>> I haven't tested it. To be honest, I don't know how to run the test
>> items of mptcpd. Can you give me some guidance?

'make check' should execute all the tests. Do you mind trying this with
and without your modifications?

Please make sure relevant ones are not skipped because of missing
dependences or tools.

> I just tested it this way, and here are the test results:

(...)

> These results appear to be unchanged from before. And in fact these
> tests do not test userspace pm.

I don't remember what is being tested, but I guess some parts of the
userspace PM are tested somewhere, no?

Coveralls [1] seems to indicate it is tested, but I don't see the details.

[1] https://coveralls.io/github/multipath-tcp/mptcpd

> So it seems that this patch will not affect the tests of mptcpd.

Regarding your patch 2/2 of this series: did you have to modify the
selftests to make the tests passing again due to a changed behaviour, or
did you modify them to extend the behaviour.

It looks like the behaviour has changed, but it is unclear to me if it
is to fix something or add a new feature.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Geliang Tang]

Hi Matt,

On Tue, 2025-04-15 at 13:00 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 15/04/2025 11:18, Geliang Tang wrote:
> > On Tue, 2025-04-15 at 11:09 +0800, Geliang Tang wrote:
> > > On Fri, 2025-04-11 at 17:26 -0700, Mat Martineau wrote:
> > > > On Thu, 10 Apr 2025, Geliang Tang wrote:
> > > > 
> > > > > 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.
> > > > > 
> > > > 
> > > > Hi Geliang -
> > > > 
> > > > Have you tried this patch with mptcpd? Does it affect any tests
> > > > there?
> > > 
> > > I haven't tested it. To be honest, I don't know how to run the
> > > test
> > > items of mptcpd. Can you give me some guidance?
> 
> 'make check' should execute all the tests. Do you mind trying this
> with
> and without your modifications?

This is the output with and without my modifications:

make  check-TESTS
make[3]: Entering directory '/home/tgl/mptcpd/tests'
make[4]: Entering directory '/home/tgl/mptcpd/tests'
PASS: test-plugin
PASS: test-network-monitor
PASS: test-path-manager
PASS: test-commands
PASS: test-configuration
PASS: test-id-manager
PASS: test-listener-manager
PASS: test-sockaddr
PASS: test-addr-info
PASS: test-murmur-hash
PASS: test-cxx-build
PASS: test-bad-log-empty
PASS: test-bad-log-long
PASS: test-bad-log-short
PASS: test-bad-option
PASS: test-bad-path-manager
PASS: test-bad-plugin-dir
SKIP: test-start-stop
PASS: test-mptcpwrap
=======================================================================
=====
Testsuite summary for mptcpd 0.13
=======================================================================
=====
# TOTAL: 19
# PASS:  18
# SKIP:  1
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
=======================================================================
=====
make[4]: Leaving directory '/home/tgl/mptcpd/tests'
make[3]: Leaving directory '/home/tgl/mptcpd/tests'
make[2]: Leaving directory '/home/tgl/mptcpd/tests'
make[1]: Leaving directory '/home/tgl/mptcpd/tests'
Making check in scripts
make[1]: Entering directory '/home/tgl/mptcpd/scripts'
make[1]: Nothing to be done for 'check'.
make[1]: Leaving directory '/home/tgl/mptcpd/scripts'
make[1]: Entering directory '/home/tgl/mptcpd'
make[1]: Nothing to be done for 'check-am'.
make[1]: Leaving directory '/home/tgl/mptcpd'

> 
> Please make sure relevant ones are not skipped because of missing
> dependences or tools.
> 
> > I just tested it this way, and here are the test results:
> 
> (...)
> 
> > These results appear to be unchanged from before. And in fact these
> > tests do not test userspace pm.
> 
> I don't remember what is being tested, but I guess some parts of the
> userspace PM are tested somewhere, no?
> 
> Coveralls [1] seems to indicate it is tested, but I don't see the
> details.
> 
> [1] https://coveralls.io/github/multipath-tcp/mptcpd
> 
> > So it seems that this patch will not affect the tests of mptcpd.
> 
> Regarding your patch 2/2 of this series: did you have to modify the
> selftests to make the tests passing again due to a changed behaviour,
> or
> did you modify them to extend the behaviour.

The first one, I tried to make the tests passing again. I updated these
userspace selftests in v3 to make sure does not change the behaviour of
these tests.

Thanks,
-Geliang

> 
> It looks like the behaviour has changed, but it is unclear to me if
> it
> is to fix something or add a new feature.
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v2 1/2] mptcp: pm: userspace: drop delete_local_addr helper

[Mat Martineau]

On Wed, 16 Apr 2025, Geliang Tang wrote:

> Hi Matt,
>
> On Tue, 2025-04-15 at 13:00 +0200, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 15/04/2025 11:18, Geliang Tang wrote:
>>> On Tue, 2025-04-15 at 11:09 +0800, Geliang Tang wrote:
>>>> On Fri, 2025-04-11 at 17:26 -0700, Mat Martineau wrote:
>>>>> On Thu, 10 Apr 2025, Geliang Tang wrote:
>>>>>
>>>>>> 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.
>>>>>>
>>>>>
>>>>> Hi Geliang -
>>>>>
>>>>> Have you tried this patch with mptcpd? Does it affect any tests
>>>>> there?
>>>>
>>>> I haven't tested it. To be honest, I don't know how to run the
>>>> test
>>>> items of mptcpd. Can you give me some guidance?
>>
>> 'make check' should execute all the tests. Do you mind trying this
>> with
>> and without your modifications?
>
> This is the output with and without my modifications:
>
> make  check-TESTS
> make[3]: Entering directory '/home/tgl/mptcpd/tests'
> make[4]: Entering directory '/home/tgl/mptcpd/tests'
> PASS: test-plugin
> PASS: test-network-monitor
> PASS: test-path-manager
> PASS: test-commands
> PASS: test-configuration
> PASS: test-id-manager
> PASS: test-listener-manager
> PASS: test-sockaddr
> PASS: test-addr-info
> PASS: test-murmur-hash
> PASS: test-cxx-build
> PASS: test-bad-log-empty
> PASS: test-bad-log-long
> PASS: test-bad-log-short
> PASS: test-bad-option
> PASS: test-bad-path-manager
> PASS: test-bad-plugin-dir
> SKIP: test-start-stop
> PASS: test-mptcpwrap
> =======================================================================
> =====
> Testsuite summary for mptcpd 0.13
> =======================================================================
> =====
> # TOTAL: 19
> # PASS:  18
> # SKIP:  1
> # XFAIL: 0
> # FAIL:  0
> # XPASS: 0
> # ERROR: 0
> =======================================================================
> =====
> make[4]: Leaving directory '/home/tgl/mptcpd/tests'
> make[3]: Leaving directory '/home/tgl/mptcpd/tests'
> make[2]: Leaving directory '/home/tgl/mptcpd/tests'
> make[1]: Leaving directory '/home/tgl/mptcpd/tests'
> Making check in scripts
> make[1]: Entering directory '/home/tgl/mptcpd/scripts'
> make[1]: Nothing to be done for 'check'.
> make[1]: Leaving directory '/home/tgl/mptcpd/scripts'
> make[1]: Entering directory '/home/tgl/mptcpd'
> make[1]: Nothing to be done for 'check-am'.
> make[1]: Leaving directory '/home/tgl/mptcpd'
>

Hi Geliang -

Thanks for running these tests. There is some coverage of the affected 
functionality by the 'test-commands' program, where the "remove_addr - 
user" subtest does send the netlink command. It only checks for whether 
the command worked or wasn't supported though, so not any verification of 
behavior. The only use of the userspace PM "remove subflow" command in the 
existing path manager plugins is in the SSPI plugin where new subflows are 
closed if there's already an existing subflow on a network interface. It 
doesn't look like mptcpd is very sensitive to specific kernel behavior 
here.

>>
>> Please make sure relevant ones are not skipped because of missing
>> dependences or tools.
>>
>>> I just tested it this way, and here are the test results:
>>
>> (...)
>>
>>> These results appear to be unchanged from before. And in fact these
>>> tests do not test userspace pm.
>>
>> I don't remember what is being tested, but I guess some parts of the
>> userspace PM are tested somewhere, no?
>>
>> Coveralls [1] seems to indicate it is tested, but I don't see the
>> details.
>>
>> [1] https://coveralls.io/github/multipath-tcp/mptcpd
>>
>>> So it seems that this patch will not affect the tests of mptcpd.
>>
>> Regarding your patch 2/2 of this series: did you have to modify the
>> selftests to make the tests passing again due to a changed behaviour,
>> or
>> did you modify them to extend the behaviour.
>
> The first one, I tried to make the tests passing again. I updated these
> userspace selftests in v3 to make sure does not change the behaviour of
> these tests.
>
> Thanks,
> -Geliang
>
>>
>> It looks like the behaviour has changed, but it is unclear to me if it 
>> is to fix something or add a new feature.

I do think the behavior is changed to fix a problem - with the existing 
kernel code, mptcpd with the SSPI plugin would un-advertise an address if 
the peer attempted to create a second subflow on that address. This would 
prevent any additional connection attempts by a well-behaved peer, but a 
well-behaved peer would also disconnect its existing subflow in response 
to the remove_addr, right?

The main risk here is that an existing MPTCP user with custom plugin code 
might be assuming the old buggy behavior, and they would have to adjust 
their plugin to handle the new "mptcpd_pm_remove_subflow doesn't 
un-advertise addresses" behavior.


- Mat