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

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

[Geliang Tang]

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 (5):
  mptcp: only remove addrs in nl_cmd_remove
  selftests: mptcp: update userspace pm addr tests
  mptcp: add addr into userspace pm list
  mptcp: add addr into pm anno_list
  selftests: mptcp: update userspace pm subflow tests

 net/mptcp/pm_netlink.c                        | 30 ++++++++++++----
 net/mptcp/pm_userspace.c                      | 36 +++++++++++++++++--
 net/mptcp/protocol.h                          |  5 ++-
 .../testing/selftests/net/mptcp/mptcp_join.sh | 11 +++++-
 4 files changed, 70 insertions(+), 12 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v8 1/5] mptcp: only remove addrs in nl_cmd_remove

[Geliang Tang]

Only send RM_ADDRS to remove addrs in mptcp_nl_cmd_remove(), add a new
helper mptcp_pm_remove_addrs() to do this. Use mptcp_nl_cmd_sf_destroy()
to delete associated subflows.

Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/pm_netlink.c   | 16 ++++++++++++++++
 net/mptcp/pm_userspace.c |  2 +-
 net/mptcp/protocol.h     |  1 +
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e8336b8bd30e..d85649bc27e2 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1555,6 +1555,22 @@ static int mptcp_nl_cmd_del_addr(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
+void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
+{
+	struct mptcp_rm_list alist = { .nr = 0 };
+	struct mptcp_pm_addr_entry *entry;
+
+	list_for_each_entry(entry, rm_list, list) {
+		if (remove_anno_list_by_saddr(msk, &entry->addr) &&
+		    alist.nr < MPTCP_RM_IDS_MAX) {
+			alist.ids[alist.nr++] = entry->addr.id;
+			spin_lock_bh(&msk->pm.lock);
+			mptcp_pm_remove_addr(msk, &alist);
+			spin_unlock_bh(&msk->pm.lock);
+		}
+	}
+}
+
 void mptcp_pm_remove_addrs_and_subflows(struct mptcp_sock *msk,
 					struct list_head *rm_list)
 {
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 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

Re: [PATCH mptcp-next v8 1/5] mptcp: only remove addrs in nl_cmd_remove

[Matthieu Baerts]

Hi Geliang,

On 20/04/2023 05:11, Geliang Tang wrote:
> Only send RM_ADDRS to remove addrs in mptcp_nl_cmd_remove(), add a new
> helper mptcp_pm_remove_addrs() to do this. Use mptcp_nl_cmd_sf_destroy()
> to delete associated subflows.

If you need to send a new version, can you add a bit more explanations
to make things clear that the NL command was not supposed to delete
subflow but only send a RM_ADDR? I think it is important because we will
need to have this patch backported.

Maybe something like this:

  mptcp: only send RM_ADDR in nl_cmd_remove

  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]

WDYT?

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

Oops, I gave you the wrong sha, it should be:

Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")

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

Re: [PATCH mptcp-next v8 1/5] mptcp: only remove addrs in nl_cmd_remove

[Geliang Tang]

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年4月21日周五 22:15写道：
>
> Hi Geliang,
>
> On 20/04/2023 05:11, Geliang Tang wrote:
> > Only send RM_ADDRS to remove addrs in mptcp_nl_cmd_remove(), add a new
> > helper mptcp_pm_remove_addrs() to do this. Use mptcp_nl_cmd_sf_destroy()
> > to delete associated subflows.
>
> If you need to send a new version, can you add a bit more explanations
> to make things clear that the NL command was not supposed to delete
> subflow but only send a RM_ADDR? I think it is important because we will
> need to have this patch backported.
>
> Maybe something like this:
>
>   mptcp: only send RM_ADDR in nl_cmd_remove
>
>   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]
>
> WDYT?

Thanks, updated this in v9.

>
> > Fixes: 9ab4807c84a4 ("mptcp: netlink: Add MPTCP_PM_CMD_ANNOUNCE")
>
> Oops, I gave you the wrong sha, it should be:
>
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

Re: [PATCH mptcp-next v8 1/5] mptcp: only remove addrs in nl_cmd_remove

[Matthieu Baerts]

Hi Geliang,

On 25/04/2023 09:57, Geliang Tang wrote:
> Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年4月21日周五 22:15写道：
>>
>> Hi Geliang,
>>
>> On 20/04/2023 05:11, Geliang Tang wrote:
>>> Only send RM_ADDRS to remove addrs in mptcp_nl_cmd_remove(), add a new
>>> helper mptcp_pm_remove_addrs() to do this. Use mptcp_nl_cmd_sf_destroy()
>>> to delete associated subflows.
>>
>> If you need to send a new version, can you add a bit more explanations
>> to make things clear that the NL command was not supposed to delete
>> subflow but only send a RM_ADDR? I think it is important because we will
>> need to have this patch backported.
>>
>> Maybe something like this:
>>
>>   mptcp: only send RM_ADDR in nl_cmd_remove
>>
>>   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]
>>
>> WDYT?
> 
> Thanks, updated this in v9.

Thank you for the individual replies, it helps to see what has been
modified or not.

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

[PATCH mptcp-next v8 2/5] selftests: mptcp: update userspace pm addr tests

[Geliang Tang]

Update userspace pm addr selftests, by sending a remove_subflows
command together after the remove_addrs command.

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..9a9b6e9b28ab 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 v8 2/5] selftests: mptcp: update userspace pm addr tests

[Matthieu Baerts]

Hi Geliang,

On 20/04/2023 05:11, Geliang Tang wrote:
> Update userspace pm addr selftests, by sending a remove_subflows
> command together after the remove_addrs command.

To help with the management of this patch, either add:

Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")

and explain in the commit message that this is linked to the parent commit.

Or squash this in the previous patch.

I think it will be clearer and easier when managing stable versions if
you squash this patch (2/5) with the parent commit (1/5).


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

Please see my comment from v7, patch 7/7: should we not do the remove
address after having deleted the subflow? See:

> (...) on slow environments, could we not have a situation where the
> REMOVE_ADDR signal is sent to the other peer, it directly reacts by
> removing the subflow and at the end, this "dsf" command is not doing
> anything, causing the test to fail because it expects the listener side
> to delete the address.
> 
> Should you not first delete the subflow, then send the remove addr? (or
> skip the deletion of the subflow command and expect the client to remove
> the subflow?)

So just moving the "pm_nl_ctl rem" command after "pm_nl_ctl dsf", no?

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

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

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

[Geliang Tang]

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年4月21日周五 22:16写道：
>
> Hi Geliang,
>
> On 20/04/2023 05:11, Geliang Tang wrote:
> > Update userspace pm addr selftests, by sending a remove_subflows
> > command together after the remove_addrs command.
>
> To help with the management of this patch, either add:
>
> Fixes: d9a4594edabf ("mptcp: netlink: Add MPTCP_PM_CMD_REMOVE")
>
> and explain in the commit message that this is linked to the parent commit.

Updated in v9.

>
> Or squash this in the previous patch.
>
> I think it will be clearer and easier when managing stable versions if
> you squash this patch (2/5) with the parent commit (1/5).

I prefer not to squash them, since they are in different repositories.

>
>
> > 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..9a9b6e9b28ab 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
>
> Please see my comment from v7, patch 7/7: should we not do the remove
> address after having deleted the subflow? See:
>
> > (...) on slow environments, could we not have a situation where the
> > REMOVE_ADDR signal is sent to the other peer, it directly reacts by
> > removing the subflow and at the end, this "dsf" command is not doing
> > anything, causing the test to fail because it expects the listener side
> > to delete the address.
> >
> > Should you not first delete the subflow, then send the remove addr? (or
> > skip the deletion of the subflow command and expect the client to remove
> > the subflow?)
>
> So just moving the "pm_nl_ctl rem" command after "pm_nl_ctl dsf", no?

Updated in v9.

>
> > +                             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
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

[PATCH mptcp-next v8 3/5] mptcp: add addr into userspace pm list

[Geliang Tang]

Add the address into userspace_pm_local_addr_list when the subflow is
created. And delete it in mptcp_nl_cmd_sf_destroy().

A non-zero address id is needed in this case. So don't clear the addr
id in mptcp_userspace_pm_get_local_id(), clear it in
mptcp_pm_nl_get_local_id() instead.

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d85649bc27e2..bb237abb99bb 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1055,8 +1055,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 
 int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
+	struct mptcp_addr_info skc_local = { 0 };
 	struct mptcp_pm_addr_entry *entry;
-	struct mptcp_addr_info skc_local;
 	struct mptcp_addr_info msk_local;
 	struct pm_nl_pernet *pernet;
 	int ret = -1;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6beadea8c67d..a1f8d2fab08d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -113,7 +113,6 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
 
 	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
 	new_entry.addr = *skc;
-	new_entry.addr.id = 0;
 	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
 
 	if (new_entry.addr.port == msk_sport)
@@ -302,6 +301,12 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
 		goto create_err;
 	}
 
+	err = mptcp_userspace_pm_get_local_id(msk, &addr_l);
+	if (err < 0) {
+		GENL_SET_ERR_MSG(info, "did not match address and id");
+		goto create_err;
+	}
+
 	lock_sock(sk);
 
 	err = __mptcp_subflow_connect(sk, &addr_l, &addr_r);
@@ -420,6 +425,18 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
 	if (ssk) {
 		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+		struct mptcp_pm_addr_entry *entry, *tmp;
+
+		spin_lock_bh(&msk->pm.lock);
+		list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
+			if (mptcp_addresses_equal(&entry->addr, &addr_l, false) &&
+			    msk->pm.subflows == 1) {
+				list_del_rcu(&entry->list);
+				kfree(entry);
+				break;
+			}
+		}
+		spin_unlock_bh(&msk->pm.lock);
 
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
-- 
2.35.3

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

[Matthieu Baerts]

Hi Geliang,

On 20/04/2023 05:11, Geliang Tang wrote:
> Add the address into userspace_pm_local_addr_list when the subflow is
> created. And delete it in mptcp_nl_cmd_sf_destroy().

I'm sorry to insist but can you explain the reason(s) why you need to
add addresses into the list? Is it to be able to send a RM_ADDR for a
previously used subflow?

  By doing that, 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

> A non-zero address id is needed in this case. So don't clear the addr
> id in mptcp_userspace_pm_get_local_id(), clear it in
> mptcp_pm_nl_get_local_id() instead.

> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_netlink.c   |  2 +-
>  net/mptcp/pm_userspace.c | 19 ++++++++++++++++++-
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index d85649bc27e2..bb237abb99bb 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1055,8 +1055,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  
>  int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  {
> +	struct mptcp_addr_info skc_local = { 0 };
>  	struct mptcp_pm_addr_entry *entry;
> -	struct mptcp_addr_info skc_local;
>  	struct mptcp_addr_info msk_local;
>  	struct pm_nl_pernet *pernet;
>  	int ret = -1;
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6beadea8c67d..a1f8d2fab08d 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -113,7 +113,6 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
>  
>  	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
>  	new_entry.addr = *skc;
> -	new_entry.addr.id = 0;
>  	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
>  
>  	if (new_entry.addr.port == msk_sport)
> @@ -302,6 +301,12 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
>  		goto create_err;
>  	}
>  
> +	err = mptcp_userspace_pm_get_local_id(msk, &addr_l);

The name is not very clear: It adds the address into
userspace_pm_local_addr_list, right?
If yes, please add a comment above (or rename the function).

Why can you not call mptcp_userspace_pm_append_new_local_addr() directly?

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

In case of error, I guess you should remove the entry from the list, no?

And when the subflow is deleted from the other side, no?

> @@ -420,6 +425,18 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>  	ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
>  	if (ssk) {
>  		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> +		struct mptcp_pm_addr_entry *entry, *tmp;
> +
> +		spin_lock_bh(&msk->pm.lock);
> +		list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> +			if (mptcp_addresses_equal(&entry->addr, &addr_l, false) &&
> +			    msk->pm.subflows == 1) {

Why did you add "msk->pm.subflows == 1"? It looks like a workaround but
not a proper solution :)

Should you not instead add a refcount in "struct mptcp_pm_addr_entry"?

> +				list_del_rcu(&entry->list);
> +				kfree(entry);
> +				break;
> +			}
> +		}
> +		spin_unlock_bh(&msk->pm.lock);
>  
>  		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
>  		mptcp_close_ssk(sk, ssk, subflow);

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

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

[Geliang Tang]

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年4月21日周五 22:17写道：
>
> Hi Geliang,
>
> On 20/04/2023 05:11, Geliang Tang wrote:
> > Add the address into userspace_pm_local_addr_list when the subflow is
> > created. And delete it in mptcp_nl_cmd_sf_destroy().
>
> I'm sorry to insist but can you explain the reason(s) why you need to
> add addresses into the list? Is it to be able to send a RM_ADDR for a
> previously used subflow?
>
>   By doing that, 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

Updated in v9.

>
> > A non-zero address id is needed in this case. So don't clear the addr
> > id in mptcp_userspace_pm_get_local_id(), clear it in
> > mptcp_pm_nl_get_local_id() instead.
>
> >
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/pm_netlink.c   |  2 +-
> >  net/mptcp/pm_userspace.c | 19 ++++++++++++++++++-
> >  2 files changed, 19 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index d85649bc27e2..bb237abb99bb 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -1055,8 +1055,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
> >
> >  int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
> >  {
> > +     struct mptcp_addr_info skc_local = { 0 };
> >       struct mptcp_pm_addr_entry *entry;
> > -     struct mptcp_addr_info skc_local;
> >       struct mptcp_addr_info msk_local;
> >       struct pm_nl_pernet *pernet;
> >       int ret = -1;
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index 6beadea8c67d..a1f8d2fab08d 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -113,7 +113,6 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> >
> >       memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
> >       new_entry.addr = *skc;
> > -     new_entry.addr.id = 0;
> >       new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
> >
> >       if (new_entry.addr.port == msk_sport)
> > @@ -302,6 +301,12 @@ int mptcp_nl_cmd_sf_create(struct sk_buff *skb, struct genl_info *info)
> >               goto create_err;
> >       }
> >
> > +     err = mptcp_userspace_pm_get_local_id(msk, &addr_l);
>
> The name is not very clear: It adds the address into
> userspace_pm_local_addr_list, right?
> If yes, please add a comment above (or rename the function).
>
> Why can you not call mptcp_userspace_pm_append_new_local_addr() directly?

Yes, mptcp_userspace_pm_append_new_local_addr is much better. Updated in v9.

>
> > +     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);
>
> In case of error, I guess you should remove the entry from the list, no?
>
> And when the subflow is deleted from the other side, no?

Updated in v9.

>
> > @@ -420,6 +425,18 @@ int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
> >       ssk = mptcp_nl_find_ssk(msk, &addr_l, &addr_r);
> >       if (ssk) {
> >               struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> > +             struct mptcp_pm_addr_entry *entry, *tmp;
> > +
> > +             spin_lock_bh(&msk->pm.lock);
> > +             list_for_each_entry_safe(entry, tmp, &msk->pm.userspace_pm_local_addr_list, list) {
> > +                     if (mptcp_addresses_equal(&entry->addr, &addr_l, false) &&
> > +                         msk->pm.subflows == 1) {
>
> Why did you add "msk->pm.subflows == 1"? It looks like a workaround but
> not a proper solution :)
>
> Should you not instead add a refcount in "struct mptcp_pm_addr_entry"?

I still use this workaround in v9. Let's add the recount in future.

>
> > +                             list_del_rcu(&entry->list);
> > +                             kfree(entry);
> > +                             break;
> > +                     }
> > +             }
> > +             spin_unlock_bh(&msk->pm.lock);
> >
> >               mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> >               mptcp_close_ssk(sk, ssk, subflow);
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

[PATCH mptcp-next v8 4/5] mptcp: add addr into pm anno_list

[Geliang Tang]

Pass addr parameter to mptcp_pm_alloc_anno_list() instead of entry.

Export remove_anno_list_by_saddr().

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

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index bb237abb99bb..d03c60f54085 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -342,7 +342,7 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
 }
 
 bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
-			      const struct mptcp_pm_addr_entry *entry)
+			      const struct mptcp_addr_info *addr)
 {
 	struct mptcp_pm_add_entry *add_entry = NULL;
 	struct sock *sk = (struct sock *)msk;
@@ -350,7 +350,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 
 	lockdep_assert_held(&msk->pm.lock);
 
-	add_entry = mptcp_lookup_anno_list_by_saddr(msk, &entry->addr);
+	add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
 
 	if (add_entry) {
 		if (mptcp_pm_is_kernel(msk))
@@ -367,7 +367,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
 
 	list_add(&add_entry->list, &msk->pm.anno_list);
 
-	add_entry->addr = entry->addr;
+	add_entry->addr = *addr;
 	add_entry->sock = msk;
 	add_entry->retrans_times = 0;
 
@@ -574,7 +574,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 			return;
 
 		if (local) {
-			if (mptcp_pm_alloc_anno_list(msk, local)) {
+			if (mptcp_pm_alloc_anno_list(msk, &local->addr)) {
 				__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
 				msk->pm.add_addr_signaled++;
 				mptcp_pm_announce_addr(msk, &local->addr, false);
@@ -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 remove_anno_list_by_saddr(struct mptcp_sock *msk,
+			       const struct mptcp_addr_info *addr)
 {
 	struct mptcp_pm_add_entry *entry;
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index a1f8d2fab08d..8c050c118ba9 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -169,7 +169,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
 	lock_sock((struct sock *)msk);
 	spin_lock_bh(&msk->pm.lock);
 
-	if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
+	if (mptcp_pm_alloc_anno_list(msk, &addr_val.addr)) {
 		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
 		mptcp_pm_nl_addr_send_ack(msk);
 	}
@@ -307,12 +307,25 @@ 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, &addr_l)) {
+		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);
 
 	release_sock(sk);
 
+	if (err) {
+		spin_lock_bh(&msk->pm.lock);
+		remove_anno_list_by_saddr(msk, &addr_l);
+		spin_unlock_bh(&msk->pm.lock);
+	}
+
  create_err:
 	sock_put((struct sock *)msk);
 	return err;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 1a2772902e9d..b3942d15ade7 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -822,7 +822,7 @@ int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
 				 struct mptcp_addr_info *rem,
 				 u8 bkup);
 bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
-			      const struct mptcp_pm_addr_entry *entry);
+			      const struct mptcp_addr_info *addr);
 void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
 bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
 struct mptcp_pm_add_entry *
@@ -837,6 +837,8 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
 int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
 						   unsigned int id,
 						   u8 *flags, int *ifindex);
+bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
+			       const struct mptcp_addr_info *addr);
 int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
 				 struct mptcp_pm_addr_entry *loc,
 				 struct mptcp_pm_addr_entry *rem, u8 bkup);
-- 
2.35.3

Re: [PATCH mptcp-next v8 4/5] mptcp: add addr into pm anno_list

[Matthieu Baerts]

Hi Geliang,

On 20/04/2023 05:11, Geliang Tang wrote:
> Pass addr parameter to mptcp_pm_alloc_anno_list() instead of entry.
> 
> Export remove_anno_list_by_saddr().
> 
> Add address into pm anno_list in mptcp_nl_cmd_sf_create(). Remove
> it when connecting fails.

I'm sorry to insist but here you only described what the new code is
doing but you didn't explain why you did that: is it to fix an issue?

Please add a "Fixes" and a "Link" like in the previous patch.

Also, I think it was fine (and even better) to pass addr parameter to
mptcp_pm_alloc_anno_list() instead of entry in a dedicated commit. What
was missing was the reason, e.g. this is needed for the following commit
to be able to (...)

> 
> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/pm_netlink.c   | 12 ++++++------
>  net/mptcp/pm_userspace.c | 15 ++++++++++++++-
>  net/mptcp/protocol.h     |  4 +++-
>  3 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index bb237abb99bb..d03c60f54085 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -342,7 +342,7 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
>  }
>  
>  bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
> -			      const struct mptcp_pm_addr_entry *entry)
> +			      const struct mptcp_addr_info *addr)
>  {
>  	struct mptcp_pm_add_entry *add_entry = NULL;
>  	struct sock *sk = (struct sock *)msk;
> @@ -350,7 +350,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
>  
>  	lockdep_assert_held(&msk->pm.lock);
>  
> -	add_entry = mptcp_lookup_anno_list_by_saddr(msk, &entry->addr);
> +	add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
>  
>  	if (add_entry) {
>  		if (mptcp_pm_is_kernel(msk))
> @@ -367,7 +367,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
>  
>  	list_add(&add_entry->list, &msk->pm.anno_list);
>  
> -	add_entry->addr = entry->addr;
> +	add_entry->addr = *addr;
>  	add_entry->sock = msk;
>  	add_entry->retrans_times = 0;
>  
> @@ -574,7 +574,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
>  			return;
>  
>  		if (local) {
> -			if (mptcp_pm_alloc_anno_list(msk, local)) {
> +			if (mptcp_pm_alloc_anno_list(msk, &local->addr)) {
>  				__clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
>  				msk->pm.add_addr_signaled++;
>  				mptcp_pm_announce_addr(msk, &local->addr, false);
> @@ -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 remove_anno_list_by_saddr(struct mptcp_sock *msk,
> +			       const struct mptcp_addr_info *addr)
>  {
>  	struct mptcp_pm_add_entry *entry;
>  
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index a1f8d2fab08d..8c050c118ba9 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -169,7 +169,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
>  	lock_sock((struct sock *)msk);
>  	spin_lock_bh(&msk->pm.lock);
>  
> -	if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
> +	if (mptcp_pm_alloc_anno_list(msk, &addr_val.addr)) {
>  		mptcp_pm_announce_addr(msk, &addr_val.addr, false);
>  		mptcp_pm_nl_addr_send_ack(msk);
>  	}
> @@ -307,12 +307,25 @@ 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, &addr_l)) {
> +		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);
>  
>  	release_sock(sk);
>  
> +	if (err) {
> +		spin_lock_bh(&msk->pm.lock);
> +		remove_anno_list_by_saddr(msk, &addr_l);
> +		spin_unlock_bh(&msk->pm.lock);
> +	}
> +
>   create_err:
>  	sock_put((struct sock *)msk);
>  	return err;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 1a2772902e9d..b3942d15ade7 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -822,7 +822,7 @@ int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
>  				 struct mptcp_addr_info *rem,
>  				 u8 bkup);
>  bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
> -			      const struct mptcp_pm_addr_entry *entry);
> +			      const struct mptcp_addr_info *addr);
>  void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
>  bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
>  struct mptcp_pm_add_entry *
> @@ -837,6 +837,8 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
>  int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
>  						   unsigned int id,
>  						   u8 *flags, int *ifindex);
> +bool remove_anno_list_by_saddr(struct mptcp_sock *msk,

Please prefix the newly exported function with mptcp_pm_

Also, can you not declare it in the middle of the userspace ones?
Maybe around mptcp_pm_alloc_anno_list() or between
mptcp_pm_get_flags_and_ifindex_by_id() and
mptcp_pm_remove_addrs_and_subflows()?

Cheers,
Matt

> +			       const struct mptcp_addr_info *addr);
>  int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
>  				 struct mptcp_pm_addr_entry *loc,
>  				 struct mptcp_pm_addr_entry *rem, u8 bkup);

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v8 4/5] mptcp: add addr into pm anno_list

[Geliang Tang]

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年4月21日周五 22:17写道：
>
> Hi Geliang,
>
> On 20/04/2023 05:11, Geliang Tang wrote:
> > Pass addr parameter to mptcp_pm_alloc_anno_list() instead of entry.
> >
> > Export remove_anno_list_by_saddr().
> >
> > Add address into pm anno_list in mptcp_nl_cmd_sf_create(). Remove
> > it when connecting fails.
>
> I'm sorry to insist but here you only described what the new code is
> doing but you didn't explain why you did that: is it to fix an issue?
>
> Please add a "Fixes" and a "Link" like in the previous patch.

Updated in v9.

>
> Also, I think it was fine (and even better) to pass addr parameter to
> mptcp_pm_alloc_anno_list() instead of entry in a dedicated commit. What
> was missing was the reason, e.g. this is needed for the following commit
> to be able to (...)

This mptcp_pm_alloc_anno_list() code is dropped now.

>
> >
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/pm_netlink.c   | 12 ++++++------
> >  net/mptcp/pm_userspace.c | 15 ++++++++++++++-
> >  net/mptcp/protocol.h     |  4 +++-
> >  3 files changed, 23 insertions(+), 8 deletions(-)
> >
> > diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> > index bb237abb99bb..d03c60f54085 100644
> > --- a/net/mptcp/pm_netlink.c
> > +++ b/net/mptcp/pm_netlink.c
> > @@ -342,7 +342,7 @@ mptcp_pm_del_add_timer(struct mptcp_sock *msk,
> >  }
> >
> >  bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
> > -                           const struct mptcp_pm_addr_entry *entry)
> > +                           const struct mptcp_addr_info *addr)
> >  {
> >       struct mptcp_pm_add_entry *add_entry = NULL;
> >       struct sock *sk = (struct sock *)msk;
> > @@ -350,7 +350,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
> >
> >       lockdep_assert_held(&msk->pm.lock);
> >
> > -     add_entry = mptcp_lookup_anno_list_by_saddr(msk, &entry->addr);
> > +     add_entry = mptcp_lookup_anno_list_by_saddr(msk, addr);
> >
> >       if (add_entry) {
> >               if (mptcp_pm_is_kernel(msk))
> > @@ -367,7 +367,7 @@ bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
> >
> >       list_add(&add_entry->list, &msk->pm.anno_list);
> >
> > -     add_entry->addr = entry->addr;
> > +     add_entry->addr = *addr;
> >       add_entry->sock = msk;
> >       add_entry->retrans_times = 0;
> >
> > @@ -574,7 +574,7 @@ static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
> >                       return;
> >
> >               if (local) {
> > -                     if (mptcp_pm_alloc_anno_list(msk, local)) {
> > +                     if (mptcp_pm_alloc_anno_list(msk, &local->addr)) {
> >                               __clear_bit(local->addr.id, msk->pm.id_avail_bitmap);
> >                               msk->pm.add_addr_signaled++;
> >                               mptcp_pm_announce_addr(msk, &local->addr, false);
> > @@ -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 remove_anno_list_by_saddr(struct mptcp_sock *msk,
> > +                            const struct mptcp_addr_info *addr)
> >  {
> >       struct mptcp_pm_add_entry *entry;
> >
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index a1f8d2fab08d..8c050c118ba9 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -169,7 +169,7 @@ int mptcp_nl_cmd_announce(struct sk_buff *skb, struct genl_info *info)
> >       lock_sock((struct sock *)msk);
> >       spin_lock_bh(&msk->pm.lock);
> >
> > -     if (mptcp_pm_alloc_anno_list(msk, &addr_val)) {
> > +     if (mptcp_pm_alloc_anno_list(msk, &addr_val.addr)) {
> >               mptcp_pm_announce_addr(msk, &addr_val.addr, false);
> >               mptcp_pm_nl_addr_send_ack(msk);
> >       }
> > @@ -307,12 +307,25 @@ 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, &addr_l)) {
> > +             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);
> >
> >       release_sock(sk);
> >
> > +     if (err) {
> > +             spin_lock_bh(&msk->pm.lock);
> > +             remove_anno_list_by_saddr(msk, &addr_l);
> > +             spin_unlock_bh(&msk->pm.lock);
> > +     }
> > +
> >   create_err:
> >       sock_put((struct sock *)msk);
> >       return err;
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index 1a2772902e9d..b3942d15ade7 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -822,7 +822,7 @@ int mptcp_pm_nl_mp_prio_send_ack(struct mptcp_sock *msk,
> >                                struct mptcp_addr_info *rem,
> >                                u8 bkup);
> >  bool mptcp_pm_alloc_anno_list(struct mptcp_sock *msk,
> > -                           const struct mptcp_pm_addr_entry *entry);
> > +                           const struct mptcp_addr_info *addr);
> >  void mptcp_pm_free_anno_list(struct mptcp_sock *msk);
> >  bool mptcp_pm_sport_in_anno_list(struct mptcp_sock *msk, const struct sock *sk);
> >  struct mptcp_pm_add_entry *
> > @@ -837,6 +837,8 @@ int mptcp_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> >  int mptcp_userspace_pm_get_flags_and_ifindex_by_id(struct mptcp_sock *msk,
> >                                                  unsigned int id,
> >                                                  u8 *flags, int *ifindex);
> > +bool remove_anno_list_by_saddr(struct mptcp_sock *msk,
>
> Please prefix the newly exported function with mptcp_pm_

Updated in v9.

>
> Also, can you not declare it in the middle of the userspace ones?
> Maybe around mptcp_pm_alloc_anno_list() or between
> mptcp_pm_get_flags_and_ifindex_by_id() and
> mptcp_pm_remove_addrs_and_subflows()?

Updated in v9.

>
> Cheers,
> Matt
>
> > +                            const struct mptcp_addr_info *addr);
> >  int mptcp_userspace_pm_set_flags(struct net *net, struct nlattr *token,
> >                                struct mptcp_pm_addr_entry *loc,
> >                                struct mptcp_pm_addr_entry *rem, u8 bkup);
>
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

[PATCH mptcp-next v8 5/5] selftests: mptcp: update userspace pm subflow tests

[Geliang Tang]

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

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 9a9b6e9b28ab..795c141a11f5 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):
  - Unstable: 1 failed test(s): selftest_userspace_pm 🔴:
  - Task: https://cirrus-ci.com/task/5458994014191616
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5458994014191616/summary/summary.txt

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

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

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 3 failed test(s): packetdrill_add_addr packetdrill_sockopts selftest_userspace_pm 🔴:
  - Task: https://cirrus-ci.com/task/4755306572414976
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4755306572414976/summary/summary.txt

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


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

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

Re: selftests: mptcp: update userspace pm subflow tests: Tests Results

[Matthieu Baerts]

Hi Geliang,

On 20/04/2023 06:13, MPTCP CI wrote:
> 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): selftest_userspace_pm 🔴:
>   - Task: https://cirrus-ci.com/task/5458994014191616
>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/5458994014191616/summary/summary.txt
> 
> - KVM Validation: normal (only selftest_mptcp_join):
>   - Success! ✅:
>   - Task: https://cirrus-ci.com/task/6584893921034240
>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/6584893921034240/summary/summary.txt
> 
> - KVM Validation: debug (only selftest_mptcp_join):
>   - Unstable: 1 failed test(s): selftest_mptcp_join 🔴:
>   - Task: https://cirrus-ci.com/task/5881206479257600
>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/5881206479257600/summary/summary.txt
> 
> - KVM Validation: debug (except selftest_mptcp_join):
>   - Unstable: 3 failed test(s): packetdrill_add_addr packetdrill_sockopts selftest_userspace_pm 🔴:
>   - Task: https://cirrus-ci.com/task/4755306572414976
>   - Summary: https://api.cirrus-ci.com/v1/artifact/task/4755306572414976/summary/summary.txt

Thank you for the new version.

It looks like the CI found some issues with this v8.

I also have some comments, see the individual patches.

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

Re: [PATCH mptcp-next v8 5/5] selftests: mptcp: update userspace pm subflow tests

[Matthieu Baerts]

Hi Geliang,

On 20/04/2023 05:11, Geliang Tang wrote:
> 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().

(what's the reason? To align with what is done by the in-kernel PM?)
> 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 9a9b6e9b28ab..795c141a11f5 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

Also here, probably better to remove the subflow, and then send the
ADD_ADDR not to have the other peer closing the subflow before the next
step here below.

But I guess it will not be possible to send the destroy after the remove
because we will remove the local address from the list, no?

Maybe better not to test that then? I mean: I don't really see why the
client would need to send a remove address if the subflow was still
working well. It would send a remove address if the linked local address
got lost (e.g. link down) but why doing both?

Cheers,
Matt

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

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v8 5/5] selftests: mptcp: update userspace pm subflow tests

[Geliang Tang]

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年4月21日周五 22:17写道：
>
> Hi Geliang,
>
> On 20/04/2023 05:11, Geliang Tang wrote:
> > 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().
>
> (what's the reason? To align with what is done by the in-kernel PM?)
> > 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 9a9b6e9b28ab..795c141a11f5 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
>
> Also here, probably better to remove the subflow, and then send the
> ADD_ADDR not to have the other peer closing the subflow before the next
> step here below.

Updated in v9.

>
> But I guess it will not be possible to send the destroy after the remove
> because we will remove the local address from the list, no?
>
> Maybe better not to test that then? I mean: I don't really see why the
> client would need to send a remove address if the subflow was still
> working well. It would send a remove address if the linked local address
> got lost (e.g. link down) but why doing both?

We need to not only close the related local subflow, but also sending
a remove address to close the subflow on the other side.

Thanks,
-Geliang

>
> Cheers,
> Matt
>
> >                               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
> >  }
>
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>