[PATCH mptcp-next v14 0/5] update userspace pm mptcp_info fields part 1

[PATCH mptcp-next v14 0/5] update userspace pm mptcp_info fields part 1

[Geliang Tang]

v14:
 - drop mptcp_pm_remove_addrs helper in patch 1
 - add two flags in patch 4, the address entry'll be removed from
   userspace_pm_local_addr_list only when both flags are set, by doing this,
   it's now independent of the order of the remove_subflows command and
   the remove_addrs command.

v13:
 - move the RM_ADDR command after the destruction of the subflow in
   patch 2 and patch 5.
 - drop mptcp_pm_remove_anno_list_by_saddr in mptcp_nl_cmd_sf_destroy in
   patch 4.
 - update userspace_pm.sh too in patch 5.

v12:
 - address Matt's commits in v11.

v11:
 - #1-#5 part 1, address Matt's comments in v10.
 - #6-#9 part 2, update pm mptcp_info
 - #10-#12 part 3, some cleanups.

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

 include/uapi/linux/mptcp.h                    |  2 +
 net/mptcp/pm_netlink.c                        |  8 +-
 net/mptcp/pm_userspace.c                      | 77 ++++++++++++++++++-
 net/mptcp/protocol.h                          |  2 +
 .../testing/selftests/net/mptcp/mptcp_join.sh | 11 ++-
 .../selftests/net/mptcp/userspace_pm.sh       |  3 +
 6 files changed, 94 insertions(+), 9 deletions(-)

-- 
2.35.3

[PATCH mptcp-next v14 1/5] 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.

mptcp_pm_remove_addr() is invoked 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_userspace.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 27a275805c06..622698fa36f7 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -188,6 +188,7 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
 	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
+	struct mptcp_rm_list rm_list = { .nr = 0 };
 	struct mptcp_pm_addr_entry *match = NULL;
 	struct mptcp_pm_addr_entry *entry;
 	struct mptcp_sock *msk;
@@ -230,12 +231,14 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 		goto remove_err;
 	}
 
-	list_move(&match->list, &free_list);
-
-	mptcp_pm_remove_addrs_and_subflows(msk, &free_list);
+	rm_list.ids[rm_list.nr++] = match->addr.id;
+	spin_lock_bh(&msk->pm.lock);
+	mptcp_pm_remove_addr(msk, &rm_list);
+	spin_unlock_bh(&msk->pm.lock);
 
 	release_sock((struct sock *)msk);
 
+	list_move(&match->list, &free_list);
 	list_for_each_entry_safe(match, entry, &free_list, list) {
 		sock_kfree_s((struct sock *)msk, match, sizeof(*match));
 	}
-- 
2.35.3

Re: [PATCH mptcp-next v14 1/5] mptcp: only send RM_ADDR in nl_cmd_remove

[Matthieu Baerts]

Hi Geliang,

On 22/05/2023 15:11, Geliang Tang wrote:
> 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.
> 
> mptcp_pm_remove_addr() is invoked 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_userspace.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 27a275805c06..622698fa36f7 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -188,6 +188,7 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
>  	struct nlattr *id = info->attrs[MPTCP_PM_ATTR_LOC_ID];
> +	struct mptcp_rm_list rm_list = { .nr = 0 };
>  	struct mptcp_pm_addr_entry *match = NULL;
>  	struct mptcp_pm_addr_entry *entry;
>  	struct mptcp_sock *msk;
> @@ -230,12 +231,14 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
>  		goto remove_err;
>  	}
>  
> -	list_move(&match->list, &free_list);
> -
> -	mptcp_pm_remove_addrs_and_subflows(msk, &free_list);
> +	rm_list.ids[rm_list.nr++] = match->addr.id;

Why do you no longer need to call remove_anno_list_by_saddr()?

Should you not call it to remove timers associated to an address that
has been previously announced and not 'echoed' yet?

(if you need to do that, it looks better to keep the helper -- you can
also modify the helper to take a pointer to address instead of a list of
addresses)

> +	spin_lock_bh(&msk->pm.lock);
> +	mptcp_pm_remove_addr(msk, &rm_list);
> +	spin_unlock_bh(&msk->pm.lock);
>  
>  	release_sock((struct sock *)msk);
>  
> +	list_move(&match->list, &free_list);

Is it OK to do this operation after "release_sock(msk)"? This will
modify "msk->pm.userspace_pm_local_addr_list" list and it should be done
under the lock, no?

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

[PATCH mptcp-next v14 2/5] 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. It's independent of the order of the remove_subflows
command and 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 0044d87556dd..a42745e60976 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -863,7 +863,15 @@ do_transfer()
 				     sed -n 's/.*\(token:\)\([[:digit:]]*\).*$/\2/p;q')
 				ip netns exec ${listener_ns} ./pm_nl_ctl ann $addr token $tk id $id
 				sleep 1
+				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 rem token $tk id $id
+				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

[PATCH mptcp-next v14 3/5] 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 | 8 ++++----
 net/mptcp/protocol.h   | 2 ++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e8336b8bd30e..6daa0ed59ec8 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);
@@ -1566,7 +1566,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 1e8effe395d8..d4c13b488955 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -830,6 +830,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 v14 4/5] mptcp: add address 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().

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.

Add two new removing flags for userspace PM: MPTCP_USER_PM_FLAG_RM_SIGNAL
and MPTCP_USER_PM_FLAG_RM_SUBFLOW. They'll be set in mptcp_nl_cmd_remove()
and mptcp_nl_cmd_sf_destroy(). The address entry'll be removed from
userspace_pm_local_addr_list only when both flags are set.

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>
---
 include/uapi/linux/mptcp.h |  2 ++
 net/mptcp/pm_userspace.c   | 72 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
index 32af2d278cb4..75737e4165f7 100644
--- a/include/uapi/linux/mptcp.h
+++ b/include/uapi/linux/mptcp.h
@@ -86,6 +86,8 @@ enum {
 #define MPTCP_PM_ADDR_FLAG_BACKUP			(1 << 2)
 #define MPTCP_PM_ADDR_FLAG_FULLMESH			(1 << 3)
 #define MPTCP_PM_ADDR_FLAG_IMPLICIT			(1 << 4)
+#define MPTCP_USER_PM_FLAG_RM_SIGNAL			(1 << 5)
+#define MPTCP_USER_PM_FLAG_RM_SUBFLOW			(1 << 6)
 
 enum {
 	MPTCP_PM_CMD_UNSPEC,
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 622698fa36f7..5f0bdb5c8033 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -79,6 +79,30 @@ 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)) {
+			if (entry->flags & MPTCP_USER_PM_FLAG_RM_SUBFLOW)
+				break;
+			entry->flags |= MPTCP_USER_PM_FLAG_RM_SUBFLOW;
+			if (entry->flags & MPTCP_USER_PM_FLAG_RM_SIGNAL) {
+				/* TODO: a refcount is needed because the entry can
+				 * be used multiple times (e.g. fullmesh mode).
+				 */
+				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)
@@ -231,6 +255,12 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 		goto remove_err;
 	}
 
+	if (match->flags & MPTCP_USER_PM_FLAG_RM_SIGNAL) {
+		GENL_SET_ERR_MSG(info, "no need to remove this address");
+		release_sock((struct sock *)msk);
+		goto remove_err;
+	}
+
 	rm_list.ids[rm_list.nr++] = match->addr.id;
 	spin_lock_bh(&msk->pm.lock);
 	mptcp_pm_remove_addr(msk, &rm_list);
@@ -238,9 +268,12 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
 
 	release_sock((struct sock *)msk);
 
-	list_move(&match->list, &free_list);
-	list_for_each_entry_safe(match, entry, &free_list, list) {
-		sock_kfree_s((struct sock *)msk, match, sizeof(*match));
+	match->flags |= MPTCP_USER_PM_FLAG_RM_SIGNAL;
+	if (match->flags & MPTCP_USER_PM_FLAG_RM_SUBFLOW) {
+		list_move(&match->list, &free_list);
+		list_for_each_entry_safe(match, entry, &free_list, list) {
+			sock_kfree_s((struct sock *)msk, match, sizeof(*match));
+		}
 	}
 
 	err = 0;
@@ -254,6 +287,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;
@@ -305,12 +339,35 @@ 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;
+	}
+
+	spin_lock_bh(&msk->pm.lock);
+	if (!mptcp_pm_alloc_anno_list(msk, &local)) {
+		GENL_SET_ERR_MSG(info, "cannot alloc address list");
+		err = -EINVAL;
+		goto anno_list_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);
+		mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
+anno_list_err:
+		mptcp_userspace_pm_delete_local_addr(msk, &local);
+		spin_unlock_bh(&msk->pm.lock);
+	}
+
  create_err:
 	sock_put((struct sock *)msk);
 	return err;
@@ -364,6 +421,11 @@ static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk,
 	return NULL;
 }
 
+/* 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.
+ */
 int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
 {
 	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
@@ -423,7 +485,11 @@ 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 };
 
+		spin_lock_bh(&msk->pm.lock);
+		mptcp_userspace_pm_delete_local_addr(msk, &entry);
+		spin_unlock_bh(&msk->pm.lock);
 		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
 		mptcp_close_ssk(sk, ssk, subflow);
 		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
-- 
2.35.3

Re: [PATCH mptcp-next v14 4/5] mptcp: add address into userspace pm list

[Matthieu Baerts]

Hi Geliang,

On 22/05/2023 15:12, 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().
> 
> 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.
> 
> Add two new removing flags for userspace PM: MPTCP_USER_PM_FLAG_RM_SIGNAL
> and MPTCP_USER_PM_FLAG_RM_SUBFLOW. They'll be set in mptcp_nl_cmd_remove()
> and mptcp_nl_cmd_sf_destroy(). The address entry'll be removed from
> userspace_pm_local_addr_list only when both flags are set.

I don't think we should do that, at least not as part of this commit
fixing a bug (not able to send a RM_ADDR for an address that has not
been announced but used in a subflow that has been created) and needed
for -net.

Also, it is unclear to me what use case you are trying to fix here.
Correct me if I'm wrong but typically, a RM_ADDR is sent either because:
- a previously announced address is no longer available → server use-case
- an address used when created new subflows is no longer usable and it
is no longer possible to close the subflow properly (with FIN) → mostly
a client use-case.

In other words, if the client sends a "destroy" command, it sounds
normal to me that the kernel will close the subflow (send FIN) and
remove all associated resources (if the same address was not used by
other subflows, hence the required refcount for the address entry).
After that, there is no need to send a remove address. Or am I missing
something?

So I think we don't need these flags, WDYT?

> 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>
> ---
>  include/uapi/linux/mptcp.h |  2 ++
>  net/mptcp/pm_userspace.c   | 72 ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 71 insertions(+), 3 deletions(-)
> 
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index 32af2d278cb4..75737e4165f7 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -86,6 +86,8 @@ enum {
>  #define MPTCP_PM_ADDR_FLAG_BACKUP			(1 << 2)
>  #define MPTCP_PM_ADDR_FLAG_FULLMESH			(1 << 3)
>  #define MPTCP_PM_ADDR_FLAG_IMPLICIT			(1 << 4)
> +#define MPTCP_USER_PM_FLAG_RM_SIGNAL			(1 << 5)
> +#define MPTCP_USER_PM_FLAG_RM_SUBFLOW			(1 << 6)
>  
>  enum {
>  	MPTCP_PM_CMD_UNSPEC,
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 622698fa36f7..5f0bdb5c8033 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -79,6 +79,30 @@ 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)) {
> +			if (entry->flags & MPTCP_USER_PM_FLAG_RM_SUBFLOW)
> +				break;
> +			entry->flags |= MPTCP_USER_PM_FLAG_RM_SUBFLOW;
> +			if (entry->flags & MPTCP_USER_PM_FLAG_RM_SIGNAL) {
> +				/* TODO: a refcount is needed because the entry can
> +				 * be used multiple times (e.g. fullmesh mode).
> +				 */
> +				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)
> @@ -231,6 +255,12 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
>  		goto remove_err;
>  	}
>  
> +	if (match->flags & MPTCP_USER_PM_FLAG_RM_SIGNAL) {
> +		GENL_SET_ERR_MSG(info, "no need to remove this address");
> +		release_sock((struct sock *)msk);
> +		goto remove_err;
> +	}
> +
>  	rm_list.ids[rm_list.nr++] = match->addr.id;
>  	spin_lock_bh(&msk->pm.lock);
>  	mptcp_pm_remove_addr(msk, &rm_list);
> @@ -238,9 +268,12 @@ int mptcp_nl_cmd_remove(struct sk_buff *skb, struct genl_info *info)
>  
>  	release_sock((struct sock *)msk);
>  
> -	list_move(&match->list, &free_list);
> -	list_for_each_entry_safe(match, entry, &free_list, list) {
> -		sock_kfree_s((struct sock *)msk, match, sizeof(*match));
> +	match->flags |= MPTCP_USER_PM_FLAG_RM_SIGNAL;
> +	if (match->flags & MPTCP_USER_PM_FLAG_RM_SUBFLOW) {
> +		list_move(&match->list, &free_list);
> +		list_for_each_entry_safe(match, entry, &free_list, list) {
> +			sock_kfree_s((struct sock *)msk, match, sizeof(*match));
> +		}
>  	}
>  
>  	err = 0;
> @@ -254,6 +287,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;
> @@ -305,12 +339,35 @@ 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;
> +	}
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	if (!mptcp_pm_alloc_anno_list(msk, &local)) {

It looks strange to add an entry in the announced list: here we only
create new subflows, we don't announce an address (ADD_ADDR).

All you need here in mptcp_nl_cmd_sf_create() is to add the address in
the local addr list, no?

Did you do that to be able to send a RM_ADDR for addresses used to
create new subflows from here?

If yes, I guess you need to call mptcp_pm_remove_anno_list_by_saddr()
from mptcp_nl_cmd_remove() but ignore the result:

+	/* Remove ADD_ADDR associated timers, then send RM_ADDR */
+	mptcp_pm_remove_anno_list_by_saddr(msk, &match->addr);
+
+	rm_list.ids[rm_list.nr++] = match->addr.id;
+	spin_lock_bh(&msk->pm.lock);
+	mptcp_pm_remove_addr(msk, &rm_list);
+	spin_unlock_bh(&msk->pm.lock);

Note that it might be better to keep the helper mptcp_pm_remove_addrs()
in pm_netlink.c. If you do that, you can remove the patch 3/5 exporting
remove_anno_list_by_saddr().

> +		GENL_SET_ERR_MSG(info, "cannot alloc address list");
> +		err = -EINVAL;
> +		goto anno_list_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);
> +		mptcp_pm_remove_anno_list_by_saddr(msk, &addr_l);
> +anno_list_err:
> +		mptcp_userspace_pm_delete_local_addr(msk, &local);
> +		spin_unlock_bh(&msk->pm.lock);
> +	}
> +
>   create_err:
>  	sock_put((struct sock *)msk);
>  	return err;
> @@ -364,6 +421,11 @@ static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk,
>  	return NULL;
>  }
>  
> +/* 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.
> + */

Could you move this comment above mptcp_userspace_pm_delete_local_addr()
please? I think it makes more sense there (that's about removeing local
address).

>  int mptcp_nl_cmd_sf_destroy(struct sk_buff *skb, struct genl_info *info)
>  {
>  	struct nlattr *raddr = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
> @@ -423,7 +485,11 @@ 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 };
>  
> +		spin_lock_bh(&msk->pm.lock);
> +		mptcp_userspace_pm_delete_local_addr(msk, &entry);
> +		spin_unlock_bh(&msk->pm.lock);
>  		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
>  		mptcp_close_ssk(sk, ssk, subflow);
>  		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);

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

[PATCH mptcp-next v14 5/5] 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 in mptcp_join.sh and userspace_pm.sh, by sending
the a remove_addrs command together after 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 ++-
 tools/testing/selftests/net/mptcp/userspace_pm.sh | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index a42745e60976..46c2095d6e3a 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -939,6 +939,7 @@ do_transfer()
 				     sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
 				ip netns exec ${connector_ns} ./pm_nl_ctl dsf lip $addr lport $sp \
 									rip $da rport $dp token $tk
+				ip netns exec ${connector_ns} ./pm_nl_ctl rem token $tk id $id
 			fi
 			counter=$((counter + 1))
 			add_nr_ns2=$((add_nr_ns2 - 1))
@@ -3210,7 +3211,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
 }
diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
index b1eb7bce599d..02465ffa075f 100755
--- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
+++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
@@ -621,6 +621,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" > /dev/null 2>&1
+	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server6_token" > /dev/null 2>&1
 	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"\
@@ -660,6 +661,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" > /dev/null 2>&1
+	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server4_token" > /dev/null 2>&1
 	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"
@@ -737,6 +739,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" > /dev/null 2>&1
+	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client6_token" > /dev/null 2>&1
 	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"
-- 
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/5356778448224256
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5356778448224256/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 5 failed test(s): packetdrill_add_addr packetdrill_dss packetdrill_fastopen selftest_diag selftest_userspace_pm 🔴:
  - Task: https://cirrus-ci.com/task/4653091006447616
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4653091006447616/summary/summary.txt

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

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

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


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

[Matthieu Baerts]

Hi Geliang,

On 22/05/2023 15:12, Geliang Tang wrote:
> To align with what is done by the in-kernel PM, update userspace pm
> subflow selftests in mptcp_join.sh and userspace_pm.sh, by sending
> the a remove_addrs command together after 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 ++-
>  tools/testing/selftests/net/mptcp/userspace_pm.sh | 3 +++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> index a42745e60976..46c2095d6e3a 100755
> --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
> +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
> @@ -939,6 +939,7 @@ do_transfer()
>  				     sed -n 's/.*\(sport:\)\([[:digit:]]*\).*$/\2/p;q')
>  				ip netns exec ${connector_ns} ./pm_nl_ctl dsf lip $addr lport $sp \
>  									rip $da rport $dp token $tk
> +				ip netns exec ${connector_ns} ./pm_nl_ctl rem token $tk id $id

Linked to my comment on patch 4/5, this should be done before the
previous line: "pm_nl_ctl rem" then "pm_nl_ctl dsf".

>  			fi
>  			counter=$((counter + 1))
>  			add_nr_ns2=$((add_nr_ns2 - 1))
> @@ -3210,7 +3211,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
>  }
> diff --git a/tools/testing/selftests/net/mptcp/userspace_pm.sh b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> index b1eb7bce599d..02465ffa075f 100755
> --- a/tools/testing/selftests/net/mptcp/userspace_pm.sh
> +++ b/tools/testing/selftests/net/mptcp/userspace_pm.sh
> @@ -621,6 +621,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" > /dev/null 2>&1
> +	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server6_token" > /dev/null 2>&1

Same here and the 2 below: "pm_nl_ctl rem" then "pm_nl_ctl dsf".

>  	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"\
> @@ -660,6 +661,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" > /dev/null 2>&1
> +	ip netns exec "$ns1" ./pm_nl_ctl rem id 23 token "$server4_token" > /dev/null 2>&1
>  	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"
> @@ -737,6 +739,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" > /dev/null 2>&1
> +	ip netns exec "$ns2" ./pm_nl_ctl rem id 23 token "$client6_token" > /dev/null 2>&1
>  	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"

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

Re: [PATCH mptcp-next v14 0/5] update userspace pm mptcp_info fields part 1

[Matthieu Baerts]

Hi Geliang,

On 22/05/2023 15:11, Geliang Tang wrote:
> v14:
>  - drop mptcp_pm_remove_addrs helper in patch 1
>  - add two flags in patch 4, the address entry'll be removed from
>    userspace_pm_local_addr_list only when both flags are set, by doing this,
>    it's now independent of the order of the remove_subflows command and
>    the remove_addrs command.

Thank you for this v14.

Please see my replies and questions on the individual patches but in short:

- Patch 1/5:
  - Maybe best to keep the helper. You can also modify it to remove just
one address instead of a list of addresses.
  - see patch 4/5: you might want to ignore the returned value of
remove_anno_list_by_saddr()

- Patch 3/5:
  - see patch 4/5: I think you can remove this one

- Patch 4/5:
  - I think it is better to use the version from v12: if the userspace
asks to destroy the subflow, we remove the linked address entry (if not
used by another one). So yes, it means we cannot send a RM_ADDR after
the destruction of the subflow but that sounds OK to me. I don't see why
it is needed to close a subflow (send FIN) and then send a RM_ADDR
  - Or am I missing something?
  - (if really we want to send a RM_ADDR for an ID we don't have, we can
also create a dummy entry just to be able to send the RM_ADDR but
currently, I don't see why the userspace will need to do that).
  - compared to v12, please:
    - do not call mptcp_pm_alloc_anno_list() (+
mptcp_pm_remove_anno_list_by_saddr()) from mptcp_nl_cmd_sf_create()
    - and move the comment above mptcp_userspace_pm_delete_local_addr()

- Patch 5/5:
  - first send a RM_ADDR, then a destroy subflow (or adapt the
verification we do not to expect a remove address)

- Patch 6 (patch 1/7 from your v13 part 2):
  - please add it to this series, it is also for -net

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