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

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

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

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

Geliang Tang (3):
  mptcp: pm: userspace: local_addr_used-- after sending REMOVE_ADDR
  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 1/3] mptcp: pm: userspace: local_addr_used-- after sending REMOVE_ADDR

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

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.

Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 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 7fc19b844384..db471eb836b3 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -331,6 +331,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);
-- 
2.43.0

[PATCH mptcp-next 2/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.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 36 ++++--------------------------------
 1 file changed, 4 insertions(+), 32 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index db471eb836b3..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)
 {
@@ -409,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:
@@ -535,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 3/3] 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 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: Unstable: 1 failed test(s): selftest_mptcp_connect 🔴
- 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/14210668405

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


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 1/3] mptcp: pm: userspace: local_addr_used-- after sending REMOVE_ADDR

[Mat Martineau]

On Wed, 2 Apr 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> 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.
>
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
> 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 7fc19b844384..db471eb836b3 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -331,6 +331,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);


Hi Geliang -

This should be squashed with patch 2, since it's not bisectable (wouldn't 
want this patch to get cherry-picked without the other, and have the 
variable decremented twice).



- Mat

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

[Mat Martineau]

On Wed, 2 Apr 2025, Geliang Tang wrote:

> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403

Geliang and Matthieu -

Since this is a bug fix, should it go to mptcp-net rather than -next? Or 
does the associated selftest change make that complicated?

I think there's a nearby bug in mptcp_pm_nl_remove_doit() that I noticed 
while reviewing this series. If we upstream this series to the net tree it 
might make sense to send the fixes together.

In this code:

 	list_del_rcu(&match->list);
 	msk->pm.local_addr_used--;
 	spin_unlock_bh(&msk->pm.lock);

 	mptcp_pm_remove_addr_entry(msk, match);

 	release_sock(sk);

 	sock_kfree_s(sk, match, sizeof(*match));

The struct is removed from a linked list with list_del_rcu(), but doesn't 
wait for a grace period before the sock_kfree_s() in the final line. The 
easy solution is to make the kfree rcu-aware and open-code the handling of 
sk_omem_alloc, like this:

 	atomic_sub(sizeof(*match), &sk->sk_omem_alloc);
 	kfree_rcu(match);

instead of calling sock_kfree_s(). tcp_md5_do_del() takes this approach. 
This does allow possible allocation beyond the optmem limit, but only for 
the duration of the rcu grace period.

I can send a patch tomorrow, unless someone wants to implement it before 
then.


- Mat


>
> Geliang Tang (3):
>  mptcp: pm: userspace: local_addr_used-- after sending REMOVE_ADDR
>  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
>
>
>

Re: [PATCH mptcp-next 1/3] mptcp: pm: userspace: local_addr_used-- after sending REMOVE_ADDR

[Geliang Tang]

Hi Mat,

Thanks for the review.

On Tue, 2025-04-08 at 16:51 -0700, Mat Martineau wrote:
> On Wed, 2 Apr 2025, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > 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.
> > 
> > Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> > 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 7fc19b844384..db471eb836b3 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -331,6 +331,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);
> 
> 
> Hi Geliang -
> 
> This should be squashed with patch 2, since it's not bisectable

Sure. Will squash it in v2.

Thanks,
-Geliang

> (wouldn't 
> want this patch to get cherry-picked without the other, and have the 
> variable decremented twice).
> 
> 
> 
> - Mat

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

[Geliang Tang]

Hi Mat,

On Tue, 2025-04-08 at 17:23 -0700, Mat Martineau wrote:
> On Wed, 2 Apr 2025, Geliang Tang wrote:
> 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/403
> 
> Geliang and Matthieu -
> 
> Since this is a bug fix, should it go to mptcp-net rather than -next?
> Or 
> does the associated selftest change make that complicated?
> 
> I think there's a nearby bug in mptcp_pm_nl_remove_doit() that I
> noticed 
> while reviewing this series. If we upstream this series to the net
> tree it 
> might make sense to send the fixes together.
> 
> In this code:
> 
>  	list_del_rcu(&match->list);
>  	msk->pm.local_addr_used--;
>  	spin_unlock_bh(&msk->pm.lock);
> 
>  	mptcp_pm_remove_addr_entry(msk, match);
> 
>  	release_sock(sk);
> 
>  	sock_kfree_s(sk, match, sizeof(*match));
> 
> The struct is removed from a linked list with list_del_rcu(), but
> doesn't 
> wait for a grace period before the sock_kfree_s() in the final line.
> The 
> easy solution is to make the kfree rcu-aware and open-code the
> handling of 
> sk_omem_alloc, like this:
> 
>  	atomic_sub(sizeof(*match), &sk->sk_omem_alloc);
>  	kfree_rcu(match);

How about adding a new helper sock_kfree_rcu_s() here?

> 
> instead of calling sock_kfree_s(). tcp_md5_do_del() takes this
> approach. 
> This does allow possible allocation beyond the optmem limit, but only
> for 
> the duration of the rcu grace period.
> 
> I can send a patch tomorrow, unless someone wants to implement it
> before 

Great! Looking forward to your patch.

Thanks,
-Geliang

> then.
> 
> 
> - Mat
> 
> 
> > 
> > Geliang Tang (3):
> >  mptcp: pm: userspace: local_addr_used-- after sending REMOVE_ADDR
> >  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
> > 
> > 
> >