[PATCH mptcp-next v10 0/6] update userspace pm mptcp_info fields, pt 1

[PATCH mptcp-next v10 0/6] update userspace pm mptcp_info fields, pt 1

[Geliang Tang]

v10:
 - fix userspace_pm.sh errors reported by CI.
 - fix the bug in mptcp_pm_remove_addrs in patch 1.
 - drop msk->pm.subflow == 1 in mptcp_userspace_pm_delete_local_addr in
   patch 3.
 - exchange the order of "pm_nl_ctl rem" and "pm_nl_ctl dsf" in patch 2
   and 6.
 - update the commit logs.

v9:
 - address Matt's commets in v8.

v8:
 - address Matt's comments.
 - split into two series, pt 2 will send later.

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 (6):
  mptcp: only send RM_ADDR in nl_cmd_remove
  selftests: mptcp: update userspace pm addr tests
  mptcp: add addr into userspace pm list
  mptcp: export remove_anno_list_by_saddr
  mptcp: add addr into pm anno_list
  selftests: mptcp: update userspace pm subflow tests

 net/mptcp/pm_netlink.c                        | 26 +++++++++--
 net/mptcp/pm_userspace.c                      | 45 ++++++++++++++++++-
 net/mptcp/protocol.h                          |  3 ++
 .../testing/selftests/net/mptcp/mptcp_join.sh | 11 ++++-
 4 files changed, 79 insertions(+), 6 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v10 1/6] mptcp: only send RM_ADDR in nl_cmd_remove

[Geliang Tang]

The specifications from [1] about the "REMOVE" command say:

    Announce that an address has been lost to the peer

It was then only supposed to send a RM_ADDR and not trying to delete
associated subflows.

A new helper mptcp_pm_remove_addrs() is then introduced to do just
that, compared to mptcp_pm_remove_addrs_and_subflows() also removing
subflows.

To delete a subflow, the userspace daemon can use the "SUB_DESTROY"
command, see mptcp_nl_cmd_sf_destroy().

Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Link: https://github.com/multipath-tcp/mptcp/blob/mptcp_v0.96/include/uapi/linux/mptcp.h [1]
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_netlink.c   | 18 ++++++++++++++++++
 net/mptcp/pm_userspace.c |  2 +-
 net/mptcp/protocol.h     |  1 +
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e8336b8bd30e..784145e6a314 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1555,6 +1555,24 @@ 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;
+	}
+
+	if (alist.nr) {
+		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 27a275805c06..6beadea8c67d 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 c39e172c95db..1a2772902e9d 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -845,6 +845,7 @@ 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 v10 2/6] selftests: mptcp: update userspace pm addr tests

[Geliang Tang]

This patch is linked to the previous commit ("mptcp: only send RM_ADDR in
nl_cmd_remove").

To align with what is done by the in-kernel PM, update userspace pm addr
selftests, by sending a remove_subflows command together after the
remove_addrs command.

Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Fixes: 97040cf9806e ("selftests: mptcp: userspace pm address tests")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 26310c17b4c6..67d5d724266a 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -850,6 +850,14 @@ do_transfer()
 				ip netns exec ${listener_ns} ./pm_nl_ctl ann $addr token $tk id $id
 				sleep 1
 				ip netns exec ${listener_ns} ./pm_nl_ctl rem token $tk id $id
+				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 "::ffff:$addr" \
+							lport $sp rip $da rport $dp token $tk
 			fi
 
 			counter=$((counter + 1))
-- 
2.35.3

Re: [PATCH mptcp-next v10 2/6] selftests: mptcp: update userspace pm addr tests

[Matthieu Baerts]

Hi Geliang,

On 26/04/2023 10:56, Geliang Tang wrote:
> This patch is linked to the previous commit ("mptcp: only send RM_ADDR in
> nl_cmd_remove").
> 
> To align with what is done by the in-kernel PM, update userspace pm addr
> selftests, by sending a remove_subflows command together after the
> remove_addrs command.
> 
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> Fixes: 97040cf9806e ("selftests: mptcp: userspace pm address tests")
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 26310c17b4c6..67d5d724266a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -850,6 +850,14 @@ do_transfer()
>  				ip netns exec ${listener_ns} ./pm_nl_ctl ann $addr token $tk id $id
>  				sleep 1
>  				ip netns exec ${listener_ns} ./pm_nl_ctl rem token $tk id $id

Out of curiosity, why did you have to move this command before the
destruction of the subflow (just below)? Is it because since v10, we
cannot remove the subflow if the subflow has been destroyed?

For the moment, I guess it is OK because if I'm not mistaken, we don't
check that the subflow destruction has been done, right?

If at some points we do verify the following command is doing what we
expect to do, my fear is that the next command might not work if we are
unlucky: when the RM_ADDR from the previous step will be received, the
other peer will destroy the associated subflows, no? If yes and if it is
done quicker than launching the following command, the subflow could
already be destroyed and the following command might fail, no?

Maybe that's not an issue because we don't check for errors in pm_nl_ctl
(which is maybe not a good thing but that's another problem). So fine to
keep this like that, right?

> +				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 "::ffff:$addr" \
> +							lport $sp rip $da rport $dp token $tk
>  			fi
>  
>  			counter=$((counter + 1))

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

Re: [PATCH mptcp-next v10 2/6] selftests: mptcp: update userspace pm addr tests

[Matthieu Baerts]

Hi Geliang,

On 03/05/2023 19:33, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 26/04/2023 10:56, Geliang Tang wrote:
>> This patch is linked to the previous commit ("mptcp: only send RM_ADDR in
>> nl_cmd_remove").
>>
>> To align with what is done by the in-kernel PM, update userspace pm addr
>> selftests, by sending a remove_subflows command together after the
>> remove_addrs command.
>>
>> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
>> Fixes: 97040cf9806e ("selftests: mptcp: userspace pm address tests")
>> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
>> ---
>>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> index 26310c17b4c6..67d5d724266a 100755
>> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
>> @@ -850,6 +850,14 @@ do_transfer()
>>  				ip netns exec ${listener_ns} ./pm_nl_ctl ann $addr token $tk id $id
>>  				sleep 1
>>  				ip netns exec ${listener_ns} ./pm_nl_ctl rem token $tk id $id
> 
> Out of curiosity, why did you have to move this command before the
> destruction of the subflow (just below)? Is it because since v10, we
> cannot remove the subflow if the subflow has been destroyed?
> 
> For the moment, I guess it is OK because if I'm not mistaken, we don't
> check that the subflow destruction has been done, right?
> 
> If at some points we do verify the following command is doing what we
> expect to do, my fear is that the next command might not work if we are
> unlucky: when the RM_ADDR from the previous step will be received, the
> other peer will destroy the associated subflows, no? If yes and if it is
> done quicker than launching the following command, the subflow could
> already be destroyed and the following command might fail, no?
> 
> Maybe that's not an issue because we don't check for errors in pm_nl_ctl
> (which is maybe not a good thing but that's another problem). So fine to
> keep this like that, right?

Does what I saying above correct: the next command could fail but that's
fine because we ignore errors?

>> +				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 "::ffff:$addr" \
>> +							lport $sp rip $da rport $dp token $tk
>>  			fi
>>  
>>  			counter=$((counter + 1))
> 
> Cheers,
> Matt

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

Re: [PATCH mptcp-next v10 2/6] selftests: mptcp: update userspace pm addr tests

[Geliang Tang]

Hi Matt,

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年5月4日周四 21:07写道：
>
> Hi Geliang,
>
> On 03/05/2023 19:33, Matthieu Baerts wrote:
> > Hi Geliang,
> >
> > On 26/04/2023 10:56, Geliang Tang wrote:
> >> This patch is linked to the previous commit ("mptcp: only send RM_ADDR in
> >> nl_cmd_remove").
> >>
> >> To align with what is done by the in-kernel PM, update userspace pm addr
> >> selftests, by sending a remove_subflows command together after the
> >> remove_addrs command.
> >>
> >> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> >> Fixes: 97040cf9806e ("selftests: mptcp: userspace pm address tests")
> >> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> >> ---
> >>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> index 26310c17b4c6..67d5d724266a 100755
> >> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> >> @@ -850,6 +850,14 @@ do_transfer()
> >>                              ip netns exec ${listener_ns} ./pm_nl_ctl ann $addr token $tk id $id
> >>                              sleep 1
> >>                              ip netns exec ${listener_ns} ./pm_nl_ctl rem token $tk id $id
> >
> > Out of curiosity, why did you have to move this command before the
> > destruction of the subflow (just below)? Is it because since v10, we
> > cannot remove the subflow if the subflow has been destroyed?
> >
> > For the moment, I guess it is OK because if I'm not mistaken, we don't
> > check that the subflow destruction has been done, right?
> >
> > If at some points we do verify the following command is doing what we
> > expect to do, my fear is that the next command might not work if we are
> > unlucky: when the RM_ADDR from the previous step will be received, the
> > other peer will destroy the associated subflows, no? If yes and if it is
> > done quicker than launching the following command, the subflow could
> > already be destroyed and the following command might fail, no?
> >
> > Maybe that's not an issue because we don't check for errors in pm_nl_ctl
> > (which is maybe not a good thing but that's another problem). So fine to
> > keep this like that, right?
>
> Does what I saying above correct: the next command could fail but that's
> fine because we ignore errors?

Now I moved the RM_ADDR command after the destruction of the subflow in v13.

Thank,
-Geliang

>
> >> +                            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 "::ffff:$addr" \
> >> +                                                    lport $sp rip $da rport $dp token $tk
> >>                      fi
> >>
> >>                      counter=$((counter + 1))
> >
> > Cheers,
> > Matt
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

[PATCH mptcp-next v10 3/6] mptcp: add addr into userspace pm list

[Geliang Tang]

Add the address into userspace_pm_local_addr_list when the subflow is
created. Make sure it can be found in mptcp_nl_cmd_remove(). And delete
it in the new helper mptcp_userspace_pm_delete_local_addr().

Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6beadea8c67d..fc96405b7616 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -79,6 +79,22 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 	return ret;
 }
 
+static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
+						struct mptcp_pm_addr_entry *addr)
+{
+	struct mptcp_pm_addr_entry *entry, *tmp;
+
+	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
+		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {
+			list_del_rcu(&entry->list);
+			kfree(entry);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
 int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
 						   unsigned int id,
 						   u8 *flags, int *ifindex)
@@ -251,6 +267,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
 	struct nlattr *laddr = info->attrs[MPTCP_PM_ATTR_ADDR];
+	struct mptcp_pm_addr_entry local = { 0 };
 	struct mptcp_addr_info addr_r;
 	struct mptcp_addr_info addr_l;
 	struct mptcp_sock *msk;
@@ -302,12 +319,25 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 		goto create_err;
 	}
 
+	local.addr = addr_l;
+	err = mptcp_userspace_pm_append_new_local_addr(msk, &local);
+	if (err < 0) {
+		GENL_SET_ERR_MSG(info, "did not match address and id");
+		goto create_err;
+	}
+
 	lock_sock(sk);
 
 	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
 
 	release_sock(sk);
 
+	if (err) {
+		spin_lock_bh(&msk->pm.lock);
+		mptcp_userspace_pm_delete_local_addr(msk, &local);
+		spin_unlock_bh(&msk->pm.lock);
+	}
+
  create_err:
 	sock_put((struct sock *)msk);
 	return err;
@@ -420,10 +450,14 @@ 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 = { .addr = addr_l };
 
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
+		spin_lock_bh(&msk->pm.lock);
+		mptcp_userspace_pm_delete_local_addr(msk, &entry);
+		spin_unlock_bh(&msk->pm.lock);
 		err = 0;
 	} else {
 		err = -ESRCH;
-- 
2.35.3

Re: [PATCH mptcp-next v10 3/6] mptcp: add addr into userspace pm list

[Matthieu Baerts]

Hi Geliang,

On 26/04/2023 10:56, Geliang Tang wrote:
> Add the address into userspace_pm_local_addr_list when the subflow is
> created. Make sure it can be found in mptcp_nl_cmd_remove(). And delete
> it in the new helper mptcp_userspace_pm_delete_local_addr().
> 
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/379
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_userspace.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6beadea8c67d..fc96405b7616 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -79,6 +79,22 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
>  	return ret;
>  }
>  

Regarding my question from the v9 about removing the local addr if the
subflow is closed from the other peer (not via a subflow destroy command
then), do we want to keep the entry not to assign the same ID to another
address and/or to be able to send RM_ADDR after the removal of the
subflow? If yes, it would be good to add a comment here in the code
about that I think, no?

> +static int mptcp_userspace_pm_delete_local_addr(struct mptcp_sock *msk,
> +						struct mptcp_pm_addr_entry *addr)
> +{
> +	struct mptcp_pm_addr_entry *entry, *tmp;
> +
> +	list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> +		if (mptcp_addresses_equal(&entry->addr, &addr->addr, false)) {

I think we should add a TODO comment here to say that a refcount is
needed because the entry can be used multiple times (e.g. fullmesh mode)
and/or create a new ticket. Except if you are already working on that?

> +			list_del_rcu(&entry->list);
> +			kfree(entry);
> +			return 0;
> +		}
> +	}

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

[PATCH mptcp-next v10 4/6] mptcp: export remove_anno_list_by_saddr

[Geliang Tang]

Rename remove_anno_list_by_saddr() with "mptcp_pm_" prefix and export it
in protocol.h.

This function will be re-used in the userspace PM code in the following
commit.

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 784145e6a314..0b34b57fc8bc 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1399,8 +1399,8 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk, unsigned int id
 	return 0;
 }
 
-static bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
-				      const struct mptcp_addr_info *addr)
+bool mptcp_pm_remove_anno_list_by_saddr(struct mptcp_sock *msk,
+					const struct mptcp_addr_info *addr)
 {
 	struct mptcp_pm_add_entry *entry;
 
@@ -1423,7 +1423,7 @@ static bool mptcp_pm_remove_anno_addr(struct mptcp_sock *msk,
 
 	list.ids[list.nr++] = addr->id;
 
-	ret = remove_anno_list_by_saddr(msk, addr);
+	ret = mptcp_pm_remove_anno_list_by_saddr(msk, addr);
 	if (ret || force) {
 		spin_lock_bh(&msk->pm.lock);
 		mptcp_pm_remove_addr(msk, &list);
@@ -1561,7 +1561,7 @@ void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
 	struct mptcp_pm_addr_entry *entry;
 
 	list_for_each_entry(entry, rm_list, list) {
-		if (remove_anno_list_by_saddr(msk, &entry->addr) &&
+		if (mptcp_pm_remove_anno_list_by_saddr(msk, &entry->addr) &&
 		    alist.nr < MPTCP_RM_IDS_MAX)
 			alist.ids[alist.nr++] = entry->addr.id;
 	}
@@ -1584,7 +1584,7 @@ void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 		    slist.nr < MPTCP_RM_IDS_MAX)
 			slist.ids[slist.nr++] = entry->addr.id;
 
-		if (remove_anno_list_by_saddr(msk, &entry->addr) &&
+		if (mptcp_pm_remove_anno_list_by_saddr(msk, &entry->addr) &&
 		    alist.nr < MPTCP_RM_IDS_MAX)
 			alist.ids[alist.nr++] = entry->addr.id;
 	}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 1a2772902e9d..bfa7d93a1c1a 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -831,6 +831,8 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 struct mptcp_pm_add_entry *
 mptcp_lookup_anno_list_by_saddr(const struct mptcp_sock *msk,
 				const struct mptcp_addr_info *addr);
+bool mptcp_pm_remove_anno_list_by_saddr(struct mptcp_sock *msk,
+					const struct mptcp_addr_info *addr);
 int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
 					 unsigned int id,
 					 u8 *flags, int *ifindex);
-- 
2.35.3

[PATCH mptcp-next v10 5/6] mptcp: add addr into pm anno_list

[Geliang Tang]

Add address into pm anno_list in mptcp_nl_cmd_sf_create(). Remove
it when connecting fails.

By doing this, the "REMOVE" command also works with subflows that have
been created via the "SUB_CREATE" command instead of restricting to
the addresses that have been announced via the "ANNOUNCE" command.

Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_userspace.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index fc96405b7616..ab7c692e8c5e 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -326,6 +326,14 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 		goto create_err;
 	}
 
+	spin_lock_bh(&msk->pm.lock);
+	if (!mptcp_pm_alloc_anno_list(msk, &local)) {
+		mptcp_userspace_pm_delete_local_addr(msk, &local);
+		spin_unlock_bh(&msk->pm.lock);
+		goto create_err;
+	}
+	spin_unlock_bh(&msk->pm.lock);
+
 	lock_sock(sk);
 
 	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
@@ -334,6 +342,7 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 
 	if (err) {
 		spin_lock_bh(&msk->pm.lock);
+		mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
 		mptcp_userspace_pm_delete_local_addr(msk, &local);
 		spin_unlock_bh(&msk->pm.lock);
 	}
-- 
2.35.3

Re: [PATCH mptcp-next v10 5/6] mptcp: add addr into pm anno_list

[Matthieu Baerts]

Hi Geliang,

On 26/04/2023 10:56, Geliang Tang wrote:
> Add address into pm anno_list in mptcp_nl_cmd_sf_create(). Remove
> it when connecting fails.
> 
> By doing this, the "REMOVE" command also works with subflows that have
> been created via the "SUB_CREATE" command instead of restricting to
> the addresses that have been announced via the "ANNOUNCE" command.

Out of curiosity, why not moving patch 4/6 before 3/6 and squash 3/6 and
this one (5/6) together?

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

[PATCH mptcp-next v10 6/6] selftests: mptcp: update userspace pm subflow tests

[Geliang Tang]

To align with what is done by the in-kernel PM, update userspace pm
subflow selftests, by sending the a remove_addrs command together
before the remove_subflows command. This will get a RM_ADDR in
chk_rm_nr().

Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
Fixes: 5e986ec46874 ("selftests: mptcp: userspace pm subflow tests")
Link: https://github.com/multipath-tcp/mptcp_net-next/issues/379
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/net/mptcp/mptcp_join.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 67d5d724266a..19fbe1c34a0c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -923,6 +923,7 @@ do_transfer()
 				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
 			fi
@@ -3137,7 +3138,7 @@ userspace_tests()
 		pm_nl_set_limits $ns1 0 1
 		run_tests $ns1 $ns2 10.0.1.1 0 0 userspace_1 slow
 		chk_join_nr 1 1 1
-		chk_rm_nr 0 1
+		chk_rm_nr 1 1
 		kill_events_pids
 	fi
 }
-- 
2.35.3

Re: selftests: mptcp: update userspace pm subflow tests: 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/6494550181543936
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6494550181543936/summary/summary.txt

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

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

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

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


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 v10 6/6] selftests: mptcp: update userspace pm subflow tests

[Matthieu Baerts]

Hi Geliang,

On 26/04/2023 10:56, Geliang Tang wrote:
> To align with what is done by the in-kernel PM, update userspace pm
> subflow selftests, by sending the a remove_addrs command together
> before the remove_subflows command. This will get a RM_ADDR in
> chk_rm_nr().
> 
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
> Fixes: 5e986ec46874 ("selftests: mptcp: userspace pm subflow tests")
> Link: https://github.com/multipath-tcp/mptcp_net-next/issues/379
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  tools/testing/selftests/net/mptcp/mptcp_join.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index 67d5d724266a..19fbe1c34a0c 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -923,6 +923,7 @@ do_transfer()
>  				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

Same questions as on patch 2/6: why did you have to move this command
before the destruction of the subflow (just below)? Is it OK even if the
destroy instruction fails?

>  				ip netns exec ${connector_ns} ./pm_nl_ctl dsf lip $addr lport $sp \
>  									rip $da rport $dp token $tk
>  			fi

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

Re: [PATCH mptcp-next v10 0/6] update userspace pm mptcp_info fields, pt 1

[Matthieu Baerts]

Hi Geliang,

On 26/04/2023 10:56, Geliang Tang wrote:
> v10:
>  - fix userspace_pm.sh errors reported by CI.
>  - fix the bug in mptcp_pm_remove_addrs in patch 1.
>  - drop msk->pm.subflow == 1 in mptcp_userspace_pm_delete_local_addr in
>    patch 3.
>  - exchange the order of "pm_nl_ctl rem" and "pm_nl_ctl dsf" in patch 2
>    and 6.
>  - update the commit logs.
Thank you for this v10 and sorry for the delay!

The code looks good to me, I just have some questions for you in the
individual patches: do you mind replying to these questions please?

Maybe all we need is to add a TODO comment/ticket and re-order/squash
patches? Or maybe not, I will see your replies!

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