[PATCH mptcp-next v3 0/8] BPF path manager, part 2

[PATCH mptcp-next v3 0/8] BPF path manager, part 2

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v3:
 - Thanks Matt for the review on dump_addr interfaces. It seems that
it needs more time to be modified. In this version, I removed the
patches for dump_addr interfaces from this set and added the patches
for set_flags interfaces.
 - set_flags interfaces patches address Matt's comments on the set
"mptcp: use GENL_REQ_ATTR_CHECK in userspace pm" v2.

Based-on: <cover.1736299989.git.tanggeliang@kylinos.cn>

v2:
 - split patch 7 of v1 into two.
 - patches 1-6 have no code changes, only the commit logs have been
updated.
 - more commit log to explain why mptcp_pm_addr_id_bitmap_t needs to
be defined.
 - use mptcp_pm_addr_id_bitmap_t in userspace_pm_append_new_local_addr
too.

In order to implement BPF userspace path manager, it is necessary to
unify the interfaces of the path manager. This set updates get_addr()
and dump_addr() interfaces.

Geliang Tang (8):
  mptcp: make three pm wrappers static
  mptcp: drop skb parameter of get_addr
  mptcp: add id parameter for get_addr
  mptcp: reuse sending nlmsg code in get_addr
  mptcp: userspace pm set_flags id support
  mptcp: drop skb parameter of set_flags
  mptcp: change rem type of set_flags
  mptcp: add local & remote parameters for set_flags

 net/mptcp/pm.c           | 114 ++++++++++++++++++++++++++++++++++++---
 net/mptcp/pm_netlink.c   | 108 ++++++++-----------------------------
 net/mptcp/pm_userspace.c |  99 ++++++----------------------------
 net/mptcp/protocol.h     |  16 +++---
 4 files changed, 154 insertions(+), 183 deletions(-)

-- 
2.45.2

[PATCH mptcp-next v3 1/8] mptcp: make three pm wrappers static

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Three netlink functions:

	mptcp_pm_nl_get_addr_doit()
	mptcp_pm_nl_get_addr_dumpit()
	mptcp_pm_nl_set_flags_doit()

are generic, implemented for each PM, in-kernel PM and userspace PM. It's
clearer to move them from pm_netlink.c to pm.c.

And the linked three path manager wrappers

	mptcp_pm_get_addr()
	mptcp_pm_dump_addr()
	mptcp_pm_set_flags()

can be changed as static functions, no need to export them in protocol.h.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c         | 23 ++++++++++++++++++++---
 net/mptcp/pm_netlink.c | 16 ----------------
 net/mptcp/protocol.h   |  3 ---
 3 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 16c336c51940..a29be5ff73a6 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -10,6 +10,7 @@
 #include "protocol.h"
 
 #include "mib.h"
+#include "mptcp_pm_gen.h"
 
 /* path manager command handlers */
 
@@ -433,14 +434,19 @@ bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
 	return mptcp_pm_nl_is_backup(msk, &skc_local);
 }
 
-int mptcp_pm_get_addr(struct sk_buff *skb, struct genl_info *info)
+static int mptcp_pm_get_addr(struct sk_buff *skb, struct genl_info *info)
 {
 	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
 		return mptcp_userspace_pm_get_addr(skb, info);
 	return mptcp_pm_nl_get_addr(skb, info);
 }
 
-int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
+int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	return mptcp_pm_get_addr(skb, info);
+}
+
+static int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
 {
 	const struct genl_info *info = genl_info_dump(cb);
 
@@ -449,13 +455,24 @@ int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
 	return mptcp_pm_nl_dump_addr(msg, cb);
 }
 
-int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
+int mptcp_pm_nl_get_addr_dumpit(struct sk_buff *msg,
+				struct netlink_callback *cb)
+{
+	return mptcp_pm_dump_addr(msg, cb);
+}
+
+static int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 {
 	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
 		return mptcp_userspace_pm_set_flags(skb, info);
 	return mptcp_pm_nl_set_flags(skb, info);
 }
 
+int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	return mptcp_pm_set_flags(skb, info);
+}
+
 void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
 {
 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 60418b8a6119..13be574bbc35 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1827,11 +1827,6 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
-int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
-{
-	return mptcp_pm_get_addr(skb, info);
-}
-
 int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			  struct netlink_callback *cb)
 {
@@ -1875,12 +1870,6 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 	return msg->len;
 }
 
-int mptcp_pm_nl_get_addr_dumpit(struct sk_buff *msg,
-				struct netlink_callback *cb)
-{
-	return mptcp_pm_dump_addr(msg, cb);
-}
-
 static int parse_limit(struct genl_info *info, int id, unsigned int *limit)
 {
 	struct nlattr *attr = info->attrs[id];
@@ -2056,11 +2045,6 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 	return 0;
 }
 
-int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info *info)
-{
-	return mptcp_pm_set_flags(skb, info);
-}
-
 static void mptcp_nl_mcast_send(struct net *net, struct sk_buff *nlskb, gfp_t gfp)
 {
 	genlmsg_multicast_netns(&mptcp_genl_family, net,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 891ffcfd1088..99ab37417b25 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1028,7 +1028,6 @@ bool mptcp_lookup_subflow_by_saddr(const struct list_head *list,
 				   const struct mptcp_addr_info *saddr);
 bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
 				     const struct mptcp_addr_info *addr);
-int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
 int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info);
 int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
@@ -1121,12 +1120,10 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_in
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc);
 bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
 bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
-int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb);
 int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			  struct netlink_callback *cb);
 int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 				 struct netlink_callback *cb);
-int mptcp_pm_get_addr(struct sk_buff *skb, struct genl_info *info);
 int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info);
 int mptcp_userspace_pm_get_addr(struct sk_buff *skb,
 				struct genl_info *info);
-- 
2.45.2

[PATCH mptcp-next v3 2/8] mptcp: drop skb parameter of get_addr

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The first parameters 'skb' of get_addr() interfaces are now useless
since mptcp_userspace_pm_get_sock() helper is used. This patch drops
these useless parameters of them.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 8 ++++----
 net/mptcp/pm_netlink.c   | 2 +-
 net/mptcp/pm_userspace.c | 3 +--
 net/mptcp/protocol.h     | 5 ++---
 4 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index a29be5ff73a6..526e5bca1fa1 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -434,16 +434,16 @@ bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
 	return mptcp_pm_nl_is_backup(msk, &skc_local);
 }
 
-static int mptcp_pm_get_addr(struct sk_buff *skb, struct genl_info *info)
+static int mptcp_pm_get_addr(struct genl_info *info)
 {
 	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
-		return mptcp_userspace_pm_get_addr(skb, info);
-	return mptcp_pm_nl_get_addr(skb, info);
+		return mptcp_userspace_pm_get_addr(info);
+	return mptcp_pm_nl_get_addr(info);
 }
 
 int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return mptcp_pm_get_addr(skb, info);
+	return mptcp_pm_get_addr(info);
 }
 
 static int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 13be574bbc35..9b5622ffaad2 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1773,7 +1773,7 @@ int mptcp_nl_fill_addr(struct sk_buff *skb,
 	return -EMSGSIZE;
 }
 
-int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info)
+int mptcp_pm_nl_get_addr(struct genl_info *info)
 {
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
 	struct mptcp_pm_addr_entry addr, *entry;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 16337d08186b..be16ab0ea287 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -684,8 +684,7 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 	return ret;
 }
 
-int mptcp_userspace_pm_get_addr(struct sk_buff *skb,
-				struct genl_info *info)
+int mptcp_userspace_pm_get_addr(struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry addr, *entry;
 	struct mptcp_sock *msk;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 99ab37417b25..36e5fbf84b2b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1124,9 +1124,8 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			  struct netlink_callback *cb);
 int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 				 struct netlink_callback *cb);
-int mptcp_pm_nl_get_addr(struct sk_buff *skb, struct genl_info *info);
-int mptcp_userspace_pm_get_addr(struct sk_buff *skb,
-				struct genl_info *info);
+int mptcp_pm_nl_get_addr(struct genl_info *info);
+int mptcp_userspace_pm_get_addr(struct genl_info *info);
 
 static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflow)
 {
-- 
2.45.2

[PATCH mptcp-next v3 3/8] mptcp: add id parameter for get_addr

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The address id is parsed both in mptcp_pm_nl_get_addr() and
mptcp_userspace_pm_get_addr(), this makes the code somewhat repetitive.
So this patch adds a new parameter 'id' for all get_addr() interfaces.
The address id is only parsed in mptcp_pm_nl_get_addr_doit(), then pass
it to both mptcp_pm_nl_get_addr() and mptcp_userspace_pm_get_addr().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 20 ++++++++++++++++----
 net/mptcp/pm_netlink.c   | 14 +++-----------
 net/mptcp/pm_userspace.c | 14 +++-----------
 net/mptcp/protocol.h     |  4 ++--
 4 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 526e5bca1fa1..caf5bfc3cd1d 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -434,16 +434,28 @@ bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
 	return mptcp_pm_nl_is_backup(msk, &skc_local);
 }
 
-static int mptcp_pm_get_addr(struct genl_info *info)
+static int mptcp_pm_get_addr(u8 id, struct genl_info *info)
 {
 	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
-		return mptcp_userspace_pm_get_addr(info);
-	return mptcp_pm_nl_get_addr(info);
+		return mptcp_userspace_pm_get_addr(id, info);
+	return mptcp_pm_nl_get_addr(id, info);
 }
 
 int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return mptcp_pm_get_addr(info);
+	struct mptcp_pm_addr_entry addr;
+	struct nlattr *attr;
+	int ret;
+
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
+		return -EINVAL;
+
+	attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
+	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
+	if (ret < 0)
+		return ret;
+
+	return mptcp_pm_get_addr(addr.addr.id, info);
 }
 
 static int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 9b5622ffaad2..89a720ce432a 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1773,23 +1773,15 @@ int mptcp_nl_fill_addr(struct sk_buff *skb,
 	return -EMSGSIZE;
 }
 
-int mptcp_pm_nl_get_addr(struct genl_info *info)
+int mptcp_pm_nl_get_addr(u8 id, struct genl_info *info)
 {
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
-	struct mptcp_pm_addr_entry addr, *entry;
+	struct mptcp_pm_addr_entry *entry;
 	struct sk_buff *msg;
 	struct nlattr *attr;
 	void *reply;
 	int ret;
 
-	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
-		return -EINVAL;
-
-	attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
-	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
-	if (ret < 0)
-		return ret;
-
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg)
 		return -ENOMEM;
@@ -1803,7 +1795,7 @@ int mptcp_pm_nl_get_addr(struct genl_info *info)
 	}
 
 	rcu_read_lock();
-	entry = __lookup_addr_by_id(pernet, addr.addr.id);
+	entry = __lookup_addr_by_id(pernet, id);
 	if (!entry) {
 		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
 		ret = -EINVAL;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index be16ab0ea287..ca30139d649d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -684,9 +684,9 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 	return ret;
 }
 
-int mptcp_userspace_pm_get_addr(struct genl_info *info)
+int mptcp_userspace_pm_get_addr(u8 id, struct genl_info *info)
 {
-	struct mptcp_pm_addr_entry addr, *entry;
+	struct mptcp_pm_addr_entry *entry;
 	struct mptcp_sock *msk;
 	struct sk_buff *msg;
 	struct nlattr *attr;
@@ -694,20 +694,12 @@ int mptcp_userspace_pm_get_addr(struct genl_info *info)
 	struct sock *sk;
 	void *reply;
 
-	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
-		return ret;
-
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
 		return ret;
 
 	sk = (struct sock *)msk;
 
-	attr = info->attrs[MPTCP_PM_ENDPOINT_ADDR];
-	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
-	if (ret < 0)
-		goto out;
-
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg) {
 		ret = -ENOMEM;
@@ -724,7 +716,7 @@ int mptcp_userspace_pm_get_addr(struct genl_info *info)
 
 	lock_sock(sk);
 	spin_lock_bh(&msk->pm.lock);
-	entry = mptcp_userspace_pm_lookup_addr_by_id(msk, addr.addr.id);
+	entry = mptcp_userspace_pm_lookup_addr_by_id(msk, id);
 	if (!entry) {
 		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
 		ret = -EINVAL;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 36e5fbf84b2b..14d6b6be0483 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1124,8 +1124,8 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			  struct netlink_callback *cb);
 int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 				 struct netlink_callback *cb);
-int mptcp_pm_nl_get_addr(struct genl_info *info);
-int mptcp_userspace_pm_get_addr(struct genl_info *info);
+int mptcp_pm_nl_get_addr(u8 id, struct genl_info *info);
+int mptcp_userspace_pm_get_addr(u8 id, struct genl_info *info);
 
 static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflow)
 {
-- 
2.45.2

[PATCH mptcp-next v3 4/8] mptcp: reuse sending nlmsg code in get_addr

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The netlink messages are sent both in mptcp_pm_nl_get_addr() and
mptcp_userspace_pm_get_addr(), this makes the code somewhat repetitive.
This is because the netlink PM and userspace PM use different locks to
protect the address entry that needs to be sent via the netlink message.
The former uses rcu read lock, and the latter uses msk->pm.lock.

The current get_addr() flow looks like this:

	lock();
	entry = get_entry();
	send_nlmsg(entry);
	unlock();

After holding the lock, get the entry from the list, send the entry, and
finally release the lock.

This patch changes the process by getting the entry while holding the lock,
then making a copy of the entry so that the lock can be released. Finally,
the copy of the entry is sent without locking:

	lock();
	entry = get_entry();
	*copy = *entry;
	unlock();

	send_nlmsg(copy);

This way we can reuse the send_nlmsg() code in get_addr() interfaces
between the netlink PM and userspace PM. They only need to implement their
own get_addr() interfaces to hold the different locks, get the entry from
the different lists, then release the locks.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 39 +++++++++++++++++++++++++++++++++----
 net/mptcp/pm_netlink.c   | 40 ++++++--------------------------------
 net/mptcp/pm_userspace.c | 42 +++++-----------------------------------
 net/mptcp/protocol.h     |  6 ++++--
 4 files changed, 50 insertions(+), 77 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index caf5bfc3cd1d..ba22d17c1451 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -434,17 +434,20 @@ bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
 	return mptcp_pm_nl_is_backup(msk, &skc_local);
 }
 
-static int mptcp_pm_get_addr(u8 id, struct genl_info *info)
+static int mptcp_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
+			     struct genl_info *info)
 {
 	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
-		return mptcp_userspace_pm_get_addr(id, info);
-	return mptcp_pm_nl_get_addr(id, info);
+		return mptcp_userspace_pm_get_addr(id, addr, info);
+	return mptcp_pm_nl_get_addr(id, addr, info);
 }
 
 int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry addr;
 	struct nlattr *attr;
+	struct sk_buff *msg;
+	void *reply;
 	int ret;
 
 	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ENDPOINT_ADDR))
@@ -455,7 +458,35 @@ int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		return ret;
 
-	return mptcp_pm_get_addr(addr.addr.id, info);
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	reply = genlmsg_put_reply(msg, info, &mptcp_genl_family, 0,
+				  info->genlhdr->cmd);
+	if (!reply) {
+		GENL_SET_ERR_MSG(info, "not enough space in Netlink message");
+		ret = -EMSGSIZE;
+		goto fail;
+	}
+
+	ret = mptcp_pm_get_addr(addr.addr.id, &addr, info);
+	if (ret) {
+		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
+		goto fail;
+	}
+
+	ret = mptcp_nl_fill_addr(msg, &addr);
+	if (ret)
+		goto fail;
+
+	genlmsg_end(msg, reply);
+	ret = genlmsg_reply(msg, info);
+	return ret;
+
+fail:
+	nlmsg_free(msg);
+	return ret;
 }
 
 static int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 89a720ce432a..ad7816cafcd4 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1773,49 +1773,21 @@ int mptcp_nl_fill_addr(struct sk_buff *skb,
 	return -EMSGSIZE;
 }
 
-int mptcp_pm_nl_get_addr(u8 id, struct genl_info *info)
+int mptcp_pm_nl_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
+			 struct genl_info *info)
 {
 	struct pm_nl_pernet *pernet = genl_info_pm_nl(info);
 	struct mptcp_pm_addr_entry *entry;
-	struct sk_buff *msg;
-	struct nlattr *attr;
-	void *reply;
-	int ret;
-
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-	if (!msg)
-		return -ENOMEM;
-
-	reply = genlmsg_put_reply(msg, info, &mptcp_genl_family, 0,
-				  info->genlhdr->cmd);
-	if (!reply) {
-		GENL_SET_ERR_MSG(info, "not enough space in Netlink message");
-		ret = -EMSGSIZE;
-		goto fail;
-	}
+	int ret = -EINVAL;
 
 	rcu_read_lock();
 	entry = __lookup_addr_by_id(pernet, id);
-	if (!entry) {
-		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
-		ret = -EINVAL;
-		goto unlock_fail;
+	if (entry) {
+		*addr = *entry;
+		ret = 0;
 	}
-
-	ret = mptcp_nl_fill_addr(msg, entry);
-	if (ret)
-		goto unlock_fail;
-
-	genlmsg_end(msg, reply);
-	ret = genlmsg_reply(msg, info);
-	rcu_read_unlock();
-	return ret;
-
-unlock_fail:
 	rcu_read_unlock();
 
-fail:
-	nlmsg_free(msg);
 	return ret;
 }
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index ca30139d649d..3d69e37f27c6 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -684,15 +684,13 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 	return ret;
 }
 
-int mptcp_userspace_pm_get_addr(u8 id, struct genl_info *info)
+int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
+				struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry *entry;
 	struct mptcp_sock *msk;
-	struct sk_buff *msg;
-	struct nlattr *attr;
 	int ret = -EINVAL;
 	struct sock *sk;
-	void *reply;
 
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
@@ -700,46 +698,16 @@ int mptcp_userspace_pm_get_addr(u8 id, struct genl_info *info)
 
 	sk = (struct sock *)msk;
 
-	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-	if (!msg) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
-	reply = genlmsg_put_reply(msg, info, &mptcp_genl_family, 0,
-				  info->genlhdr->cmd);
-	if (!reply) {
-		GENL_SET_ERR_MSG(info, "not enough space in Netlink message");
-		ret = -EMSGSIZE;
-		goto fail;
-	}
-
 	lock_sock(sk);
 	spin_lock_bh(&msk->pm.lock);
 	entry = mptcp_userspace_pm_lookup_addr_by_id(msk, id);
-	if (!entry) {
-		NL_SET_ERR_MSG_ATTR(info->extack, attr, "address not found");
-		ret = -EINVAL;
-		goto unlock_fail;
+	if (entry) {
+		*addr = *entry;
+		ret = 0;
 	}
-
-	ret = mptcp_nl_fill_addr(msg, entry);
-	if (ret)
-		goto unlock_fail;
-
-	genlmsg_end(msg, reply);
-	ret = genlmsg_reply(msg, info);
 	spin_unlock_bh(&msk->pm.lock);
 	release_sock(sk);
-	sock_put(sk);
-	return ret;
 
-unlock_fail:
-	spin_unlock_bh(&msk->pm.lock);
-	release_sock(sk);
-fail:
-	nlmsg_free(msg);
-out:
 	sock_put(sk);
 	return ret;
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 14d6b6be0483..a43481c40577 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1124,8 +1124,10 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			  struct netlink_callback *cb);
 int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 				 struct netlink_callback *cb);
-int mptcp_pm_nl_get_addr(u8 id, struct genl_info *info);
-int mptcp_userspace_pm_get_addr(u8 id, struct genl_info *info);
+int mptcp_pm_nl_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
+			 struct genl_info *info);
+int mptcp_userspace_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
+				struct genl_info *info);
 
 static inline u8 subflow_get_local_id(const struct mptcp_subflow_context *subflow)
 {
-- 
2.45.2

[PATCH mptcp-next v3 5/8] mptcp: userspace pm set_flags id support

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Similar to in-kernel PM, this patch adds address ID support to set_flags()
interface of userspace PM, allowing it to work with either an address or
an address ID.

When an address ID is used, mptcp_userspace_pm_lookup_addr_by_id() helper
is used to look up the address entry in the local address list instead of
using mptcp_userspace_pm_lookup_addr().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 3d69e37f27c6..3fad1201bc51 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -571,6 +571,7 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 	struct mptcp_pm_addr_entry *entry;
 	struct nlattr *attr, *attr_rem;
 	struct mptcp_sock *msk;
+	u8 lookup_by_id = 0;
 	int ret = -EINVAL;
 	struct sock *sk;
 	u8 bkup = 0;
@@ -590,12 +591,8 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 	if (ret < 0)
 		goto set_flags_err;
 
-	if (loc.addr.family == AF_UNSPEC) {
-		NL_SET_ERR_MSG_ATTR(info->extack, attr,
-				    "invalid local address family");
-		ret = -EINVAL;
-		goto set_flags_err;
-	}
+	if (loc.addr.family == AF_UNSPEC)
+		lookup_by_id = 1;
 
 	attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
 	ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
@@ -613,7 +610,8 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 		bkup = 1;
 
 	spin_lock_bh(&msk->pm.lock);
-	entry = mptcp_userspace_pm_lookup_addr(msk, &loc.addr);
+	entry = lookup_by_id ? mptcp_userspace_pm_lookup_addr_by_id(msk, loc.addr.id) :
+			       mptcp_userspace_pm_lookup_addr(msk, &loc.addr);
 	if (entry) {
 		if (bkup)
 			entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
@@ -623,7 +621,8 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
 	spin_unlock_bh(&msk->pm.lock);
 
 	lock_sock(sk);
-	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr, &rem.addr, bkup);
+	ret = mptcp_pm_nl_mp_prio_send_ack(msk, entry ? &entry->addr : &loc.addr,
+					   &rem.addr, bkup);
 	release_sock(sk);
 
 	/* mptcp_pm_nl_mp_prio_send_ack() only fails in one case */
-- 
2.45.2

[PATCH mptcp-next v3 6/8] mptcp: drop skb parameter of set_flags

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The first parameter 'skb' in mptcp_pm_nl_set_flags() is only used to
obtained the network namespace, which can also be obtained through the
second parameters 'info' by using genl_info_net() helper. This patch
drops these useless parameters 'skb' in all three set_flags() interfaces.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 8 ++++----
 net/mptcp/pm_netlink.c   | 4 ++--
 net/mptcp/pm_userspace.c | 2 +-
 net/mptcp/protocol.h     | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index ba22d17c1451..c213f06bc702 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -504,16 +504,16 @@ int mptcp_pm_nl_get_addr_dumpit(struct sk_buff *msg,
 	return mptcp_pm_dump_addr(msg, cb);
 }
 
-static int mptcp_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
+static int mptcp_pm_set_flags(struct genl_info *info)
 {
 	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
-		return mptcp_userspace_pm_set_flags(skb, info);
-	return mptcp_pm_nl_set_flags(skb, info);
+		return mptcp_userspace_pm_set_flags(info);
+	return mptcp_pm_nl_set_flags(info);
 }
 
 int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return mptcp_pm_set_flags(skb, info);
+	return mptcp_pm_set_flags(info);
 }
 
 void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index ad7816cafcd4..b18c42419d42 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1951,12 +1951,12 @@ static int mptcp_nl_set_flags(struct net *net,
 	return ret;
 }
 
-int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
+int mptcp_pm_nl_set_flags(struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, };
 	u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
 			   MPTCP_PM_ADDR_FLAG_FULLMESH;
-	struct net *net = sock_net(skb->sk);
+	struct net *net = genl_info_net(info);
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
 	struct nlattr *attr;
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 3fad1201bc51..ce3154a87570 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -564,7 +564,7 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 	return err;
 }
 
-int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
+int mptcp_userspace_pm_set_flags(struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
 	struct mptcp_pm_addr_entry rem = { .addr = { .family = AF_UNSPEC }, };
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index a43481c40577..aa014f514af3 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1028,8 +1028,8 @@ bool mptcp_lookup_subflow_by_saddr(const struct list_head *list,
 				   const struct mptcp_addr_info *saddr);
 bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
 				     const struct mptcp_addr_info *addr);
-int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info);
-int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info);
+int mptcp_pm_nl_set_flags(struct genl_info *info);
+int mptcp_userspace_pm_set_flags(struct genl_info *info);
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   const struct mptcp_addr_info *addr,
 			   bool echo);
-- 
2.45.2

[PATCH mptcp-next v3 7/8] mptcp: change rem type of set_flags

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Generally, in the path manager interfaces, the local address is defined as
an mptcp_pm_addr_entry type address, while the remote address is defined as
an mptcp_addr_info type one:

        (struct mptcp_pm_addr_entry *local, struct mptcp_addr_info *remote)

But the set_flags() interface uses two mptcp_pm_addr_entry type parameters.
This patch changes the second one to mptcp_addr_info type and use helper
mptcp_pm_parse_addr() to parse it instead of using mptcp_pm_parse_entry().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index ce3154a87570..a95f52142a33 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -567,7 +567,7 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 int mptcp_userspace_pm_set_flags(struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
-	struct mptcp_pm_addr_entry rem = { .addr = { .family = AF_UNSPEC }, };
+	struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
 	struct mptcp_pm_addr_entry *entry;
 	struct nlattr *attr, *attr_rem;
 	struct mptcp_sock *msk;
@@ -595,11 +595,11 @@ int mptcp_userspace_pm_set_flags(struct genl_info *info)
 		lookup_by_id = 1;
 
 	attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
-	ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
+	ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
 	if (ret < 0)
 		goto set_flags_err;
 
-	if (rem.addr.family == AF_UNSPEC) {
+	if (rem.family == AF_UNSPEC) {
 		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
 				    "invalid remote address family");
 		ret = -EINVAL;
@@ -622,7 +622,7 @@ int mptcp_userspace_pm_set_flags(struct genl_info *info)
 
 	lock_sock(sk);
 	ret = mptcp_pm_nl_mp_prio_send_ack(msk, entry ? &entry->addr : &loc.addr,
-					   &rem.addr, bkup);
+					   &rem, bkup);
 	release_sock(sk);
 
 	/* mptcp_pm_nl_mp_prio_send_ack() only fails in one case */
-- 
2.45.2

[PATCH mptcp-next v3 8/8] mptcp: add local & remote parameters for set_flags

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch updates the interfaces set_flags to reduce repetitive code,
adds two more parameters 'local' and 'remote' for them. These addresses
are parsed in public helper mptcp_pm_nl_set_flags_doit(), then pass them
to mptcp_pm_nl_set_flags() and mptcp_userspace_pm_set_flags().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm.c           | 48 ++++++++++++++++++++++++++++++++++++----
 net/mptcp/pm_netlink.c   | 38 ++++++++++---------------------
 net/mptcp/pm_userspace.c | 41 ++++++++--------------------------
 net/mptcp/protocol.h     |  8 +++++--
 4 files changed, 71 insertions(+), 64 deletions(-)

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index c213f06bc702..25659f3f1221 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -504,16 +504,56 @@ int mptcp_pm_nl_get_addr_dumpit(struct sk_buff *msg,
 	return mptcp_pm_dump_addr(msg, cb);
 }
 
-static int mptcp_pm_set_flags(struct genl_info *info)
+static int mptcp_pm_set_flags(struct mptcp_pm_addr_entry *local,
+			      struct mptcp_addr_info *remote,
+			      struct genl_info *info)
 {
 	if (info->attrs[MPTCP_PM_ATTR_TOKEN])
-		return mptcp_userspace_pm_set_flags(info);
-	return mptcp_pm_nl_set_flags(info);
+		return mptcp_userspace_pm_set_flags(local, remote, info);
+	return mptcp_pm_nl_set_flags(local, remote, info);
 }
 
 int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info *info)
 {
-	return mptcp_pm_set_flags(info);
+	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
+	struct nlattr *token = info->attrs[MPTCP_PM_ATTR_TOKEN];
+	struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
+	struct nlattr *attr_loc, *attr_rem;
+	int ret;
+
+	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
+		return -EINVAL;
+
+	attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
+	ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
+	if (ret < 0)
+		return ret;
+
+	if (loc.addr.family == AF_UNSPEC) {
+		if (!token && !loc.addr.id) {
+			NL_SET_ERR_MSG_ATTR(info->extack, attr_loc,
+					    "missing address ID");
+			return -EOPNOTSUPP;
+		}
+	}
+
+	if (token) {
+		if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
+			return -EINVAL;
+
+		attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
+		ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
+		if (ret < 0)
+			return ret;
+
+		if (rem.family == AF_UNSPEC) {
+			NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
+					    "invalid remote address family");
+			return -EINVAL;
+		}
+	}
+
+	return mptcp_pm_set_flags(&loc, &rem, info);
 }
 
 void mptcp_pm_subflow_chk_stale(const struct mptcp_sock *msk, struct sock *ssk)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index b18c42419d42..01f50bd25b44 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1951,61 +1951,47 @@ static int mptcp_nl_set_flags(struct net *net,
 	return ret;
 }
 
-int mptcp_pm_nl_set_flags(struct genl_info *info)
+int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
+			  struct mptcp_addr_info *remote,
+			  struct genl_info *info)
 {
-	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, };
 	u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
 			   MPTCP_PM_ADDR_FLAG_FULLMESH;
 	struct net *net = genl_info_net(info);
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
-	struct nlattr *attr;
 	u8 lookup_by_id = 0;
 	u8 bkup = 0;
-	int ret;
 
 	pernet = pm_nl_get_pernet(net);
 
-	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
-		return -EINVAL;
-
-	attr = info->attrs[MPTCP_PM_ATTR_ADDR];
-	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
-	if (ret < 0)
-		return ret;
-
-	if (addr.addr.family == AF_UNSPEC) {
+	if (local->addr.family == AF_UNSPEC)
 		lookup_by_id = 1;
-		if (!addr.addr.id) {
-			GENL_SET_ERR_MSG(info, "missing address ID");
-			return -EOPNOTSUPP;
-		}
-	}
 
-	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
+	if (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
 		bkup = 1;
 
 	spin_lock_bh(&pernet->lock);
-	entry = lookup_by_id ? __lookup_addr_by_id(pernet, addr.addr.id) :
-			       __lookup_addr(pernet, &addr.addr);
+	entry = lookup_by_id ? __lookup_addr_by_id(pernet, local->addr.id) :
+			       __lookup_addr(pernet, &local->addr);
 	if (!entry) {
 		spin_unlock_bh(&pernet->lock);
 		GENL_SET_ERR_MSG(info, "address not found");
 		return -EINVAL;
 	}
-	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
+	if ((local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
 	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
 		spin_unlock_bh(&pernet->lock);
 		GENL_SET_ERR_MSG(info, "invalid addr flags");
 		return -EINVAL;
 	}
 
-	changed = (addr.flags ^ entry->flags) & mask;
-	entry->flags = (entry->flags & ~mask) | (addr.flags & mask);
-	addr = *entry;
+	changed = (local->flags ^ entry->flags) & mask;
+	entry->flags = (entry->flags & ~mask) | (local->flags & mask);
+	*local = *entry;
 	spin_unlock_bh(&pernet->lock);
 
-	mptcp_nl_set_flags(net, &addr.addr, bkup, changed);
+	mptcp_nl_set_flags(net, &local->addr, bkup, changed);
 	return 0;
 }
 
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index a95f52142a33..7b5c400f9066 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -564,54 +564,32 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 	return err;
 }
 
-int mptcp_userspace_pm_set_flags(struct genl_info *info)
+int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
+				 struct mptcp_addr_info *remote,
+				 struct genl_info *info)
 {
-	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
-	struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
 	struct mptcp_pm_addr_entry *entry;
-	struct nlattr *attr, *attr_rem;
 	struct mptcp_sock *msk;
 	u8 lookup_by_id = 0;
 	int ret = -EINVAL;
 	struct sock *sk;
 	u8 bkup = 0;
 
-	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR) ||
-	    GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
-		return ret;
-
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
 		return ret;
 
 	sk = (struct sock *)msk;
 
-	attr = info->attrs[MPTCP_PM_ATTR_ADDR];
-	ret = mptcp_pm_parse_entry(attr, info, false, &loc);
-	if (ret < 0)
-		goto set_flags_err;
-
-	if (loc.addr.family == AF_UNSPEC)
+	if (local->addr.family == AF_UNSPEC)
 		lookup_by_id = 1;
 
-	attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
-	ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
-	if (ret < 0)
-		goto set_flags_err;
-
-	if (rem.family == AF_UNSPEC) {
-		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
-				    "invalid remote address family");
-		ret = -EINVAL;
-		goto set_flags_err;
-	}
-
-	if (loc.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
+	if (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
 		bkup = 1;
 
 	spin_lock_bh(&msk->pm.lock);
-	entry = lookup_by_id ? mptcp_userspace_pm_lookup_addr_by_id(msk, loc.addr.id) :
-			       mptcp_userspace_pm_lookup_addr(msk, &loc.addr);
+	entry = lookup_by_id ? mptcp_userspace_pm_lookup_addr_by_id(msk, local->addr.id) :
+			       mptcp_userspace_pm_lookup_addr(msk, &local->addr);
 	if (entry) {
 		if (bkup)
 			entry->flags |= MPTCP_PM_ADDR_FLAG_BACKUP;
@@ -621,15 +599,14 @@ int mptcp_userspace_pm_set_flags(struct genl_info *info)
 	spin_unlock_bh(&msk->pm.lock);
 
 	lock_sock(sk);
-	ret = mptcp_pm_nl_mp_prio_send_ack(msk, entry ? &entry->addr : &loc.addr,
-					   &rem, bkup);
+	ret = mptcp_pm_nl_mp_prio_send_ack(msk, entry ? &entry->addr : &local->addr,
+					   remote, bkup);
 	release_sock(sk);
 
 	/* mptcp_pm_nl_mp_prio_send_ack() only fails in one case */
 	if (ret < 0)
 		GENL_SET_ERR_MSG(info, "subflow not found");
 
-set_flags_err:
 	sock_put(sk);
 	return ret;
 }
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index aa014f514af3..d2626b5ac381 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1028,8 +1028,12 @@ bool mptcp_lookup_subflow_by_saddr(const struct list_head *list,
 				   const struct mptcp_addr_info *saddr);
 bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
 				     const struct mptcp_addr_info *addr);
-int mptcp_pm_nl_set_flags(struct genl_info *info);
-int mptcp_userspace_pm_set_flags(struct genl_info *info);
+int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
+			  struct mptcp_addr_info *remote,
+			  struct genl_info *info);
+int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
+				 struct mptcp_addr_info *remote,
+				 struct genl_info *info);
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   const struct mptcp_addr_info *addr,
 			   bool echo);
-- 
2.45.2

Re: [PATCH mptcp-next v3 0/8] BPF path manager, part 2

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12664174127

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


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

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

For more details:

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


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

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

Re: [PATCH mptcp-next v3 5/8] mptcp: userspace pm set_flags id support

[Matthieu Baerts]

Hi Geliang,

On 08/01/2025 05:21, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Similar to in-kernel PM, this patch adds address ID support to set_flags()
> interface of userspace PM, allowing it to work with either an address or
> an address ID.
> 
> When an address ID is used, mptcp_userspace_pm_lookup_addr_by_id() helper
> is used to look up the address entry in the local address list instead of
> using mptcp_userspace_pm_lookup_addr().

Mmh, I'm still not sure about that. As I was saying in [1], if I'm not
mistaken, with the userspace PM, it is possible not to find any entries
here, e.g.: if a subflow using this address has not been added or the
address has not been announced. (I guess the initial address is not
there then).

Do you think this patch is worth it? Setting by ID for the in-kernel PM
makes sense: unique ID for the netns, easier to type the ID than the
full address. While for the userspace PM, it will be managed by a daemon
that will have to track addresses anyway.

Or in other words, do you have a use-case for this? To me, it looks like
"yes, you can only set the ID, but it might not always work". Then maybe
better to always set the full address, no?

[1]
https://lore.kernel.org/mptcp/d01d0e8a-5606-4152-aabe-32e4402adeeb@kernel.org/

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

Re: [PATCH mptcp-next v3 5/8] mptcp: userspace pm set_flags id support

[Matthieu Baerts]

On 08/01/2025 19:47, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 08/01/2025 05:21, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> Similar to in-kernel PM, this patch adds address ID support to set_flags()
>> interface of userspace PM, allowing it to work with either an address or
>> an address ID.
>>
>> When an address ID is used, mptcp_userspace_pm_lookup_addr_by_id() helper
>> is used to look up the address entry in the local address list instead of
>> using mptcp_userspace_pm_lookup_addr().
> 
> Mmh, I'm still not sure about that. As I was saying in [1], if I'm not
> mistaken, with the userspace PM, it is possible not to find any entries
> here, e.g.: if a subflow using this address has not been added or the
> address has not been announced. (I guess the initial address is not
> there then).
> 
> Do you think this patch is worth it? Setting by ID for the in-kernel PM
> makes sense: unique ID for the netns, easier to type the ID than the
> full address. While for the userspace PM, it will be managed by a daemon
> that will have to track addresses anyway.
> 
> Or in other words, do you have a use-case for this? To me, it looks like
> "yes, you can only set the ID, but it might not always work". Then maybe
> better to always set the full address, no?
> 
> [1]
> https://lore.kernel.org/mptcp/d01d0e8a-5606-4152-aabe-32e4402adeeb@kernel.org/

Note: if we drop this patch (I think it is better), maybe patch 8/8 is
not worth it: not to have a "common" section with plenty of 'if
(token)', no? Or do you really need them for the BPF PM?

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

Re: [PATCH mptcp-next v3 5/8] mptcp: userspace pm set_flags id support

[Geliang Tang]

Hi Matt,

Thanks for the review!

On Wed, 2025-01-08 at 19:51 +0100, Matthieu Baerts wrote:
> On 08/01/2025 19:47, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > On 08/01/2025 05:21, Geliang Tang wrote:
> > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > 
> > > Similar to in-kernel PM, this patch adds address ID support to
> > > set_flags()
> > > interface of userspace PM, allowing it to work with either an
> > > address or
> > > an address ID.
> > > 
> > > When an address ID is used,
> > > mptcp_userspace_pm_lookup_addr_by_id() helper
> > > is used to look up the address entry in the local address list
> > > instead of
> > > using mptcp_userspace_pm_lookup_addr().
> > 
> > Mmh, I'm still not sure about that. As I was saying in [1], if I'm
> > not
> > mistaken, with the userspace PM, it is possible not to find any
> > entries
> > here, e.g.: if a subflow using this address has not been added or
> > the
> > address has not been announced. (I guess the initial address is not
> > there then).

The previous version (in [1]) did have this issue and userspace_pm.sh
tests would fail because of it, but this new version has fixed it.

 mptcp_pm_nl_mp_prio_send_ack(msk,
                              entry ? &entry->addr : &local->addr,
                              remote, bkup);

When the entry is not found, we continue to pass local->addr to ensure
the same behavior as before.

> > 
> > Do you think this patch is worth it? Setting by ID for the in-
> > kernel PM
> > makes sense: unique ID for the netns, easier to type the ID than
> > the
> > full address. While for the userspace PM, it will be managed by a
> > daemon
> > that will have to track addresses anyway.

I think it's still useful to extend this functionality while the
original behavior is not affected, at least it doesn't hurt.

We cannot assume that userspace PM is always managed by a daemon. We
have exported its interfaces to BPF. We allow users to customize path
managers. That means we also allow users to use their own userspace PM
in any way.

Another consideration is that we need to maintain the consistency
between in-kernel PM and userspace PM. For ease of maintenance, we need
to make these two PMs use the same code as much as possible, and only
abstract their differences through PM interfaces such as get_addr,
dump_addr, set_flags, etc. At present, the biggest difference between
the two is that they use different linked lists (pernet-
>local_addr_list vs. msk->pm.userspace_pm_local_addr_list) to store
address entries, so we only need to put the code for operating the
linked lists into the interfaces of each PM. This is also the goal of
adjusting the pm interfaces in this series.

> > 
> > Or in other words, do you have a use-case for this? To me, it looks
> > like
> > "yes, you can only set the ID, but it might not always work". Then
> > maybe
> > better to always set the full address, no?

If you're worried that this functionality isn't covered by tests, I've
added a test that covers it in BPF path manager selftests:

        err = userspace_pm_set_flags(token, addr, "backup");
        if (!ASSERT_OK(err, "userspace_pm_set_flags backup"))
                goto close_accept;

	...

        err = userspace_pm_set_flags_by_id(token, 100, "nobackup");
        if (!ASSERT_OK(err, "userspace_pm_set_flags_by_id nobackup"))
                goto close_accept;

> > 
> > [1]
> > https://lore.kernel.org/mptcp/d01d0e8a-5606-4152-aabe-32e4402adeeb@kernel.org/
> 
> Note: if we drop this patch (I think it is better), maybe patch 8/8
> is
> not worth it: not to have a "common" section with plenty of 'if
> (token)', no? Or do you really need them for the BPF PM?

Here we are only adjusting set_flags interface of in-kernel PM and
userspace PM, which has nothing to do with the BPF PM implementation.

It seems that moving the code in mptcp_pm_nl_set_flags_doit() to
mptcp_pm_set_flags() can remove these 'if (token)':

int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info
*info)
{
        return mptcp_pm_set_flags(info);
}

static int mptcp_pm_set_flags(struct genl_info *info)
{
        struct mptcp_pm_addr_entry loc = { .addr = { .family =
AF_UNSPEC }, };
        struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
        struct nlattr *attr_loc, *attr_rem;
        int ret;

        if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
                return -EINVAL;

        attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
        ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
        if (ret < 0)
                return ret;

        if (info->attrs[MPTCP_PM_ATTR_TOKEN]) {
                if (GENL_REQ_ATTR_CHECK(info,
MPTCP_PM_ATTR_ADDR_REMOTE))
                        return -EINVAL;

                attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
                ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
                if (ret < 0)
                        return ret;

                if (rem.family == AF_UNSPEC) {
                        NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
                                            "invalid remote address
family");
                        return -EINVAL;
                }

                return mptcp_userspace_pm_set_flags(&loc, &rem, info);
        }

        if (loc.addr.family == AF_UNSPEC) {
                if (!loc.addr.id) {
                        NL_SET_ERR_MSG_ATTR(info->extack, attr_loc,
                                            "missing address ID");
                        return -EOPNOTSUPP;
		}
        }

        return mptcp_pm_nl_set_flags(&loc, info);
}

WDYT?

-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v3 5/8] mptcp: userspace pm set_flags id support

[Matthieu Baerts]

Hi Geliang,

Thank you for your reply!

On 09/01/2025 04:40, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for the review!
> 
> On Wed, 2025-01-08 at 19:51 +0100, Matthieu Baerts wrote:
>> On 08/01/2025 19:47, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> On 08/01/2025 05:21, Geliang Tang wrote:
>>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>>
>>>> Similar to in-kernel PM, this patch adds address ID support to
>>>> set_flags()
>>>> interface of userspace PM, allowing it to work with either an
>>>> address or
>>>> an address ID.
>>>>
>>>> When an address ID is used,
>>>> mptcp_userspace_pm_lookup_addr_by_id() helper
>>>> is used to look up the address entry in the local address list
>>>> instead of
>>>> using mptcp_userspace_pm_lookup_addr().
>>>
>>> Mmh, I'm still not sure about that. As I was saying in [1], if I'm
>>> not
>>> mistaken, with the userspace PM, it is possible not to find any
>>> entries
>>> here, e.g.: if a subflow using this address has not been added or
>>> the
>>> address has not been announced. (I guess the initial address is not
>>> there then).
> 
> The previous version (in [1]) did have this issue and userspace_pm.sh
> tests would fail because of it, but this new version has fixed it.
> 
>  mptcp_pm_nl_mp_prio_send_ack(msk,
>                               entry ? &entry->addr : &local->addr,
>                               remote, bkup);
> 
> When the entry is not found, we continue to pass local->addr to ensure
> the same behavior as before.

Yes indeed, the tests are fixed, but if 'entry' is NULL, the address you
will give will be empty, so it will not be able to find any subflow to
send the MP_PRIO, right?

>>> Do you think this patch is worth it? Setting by ID for the in-
>>> kernel PM
>>> makes sense: unique ID for the netns, easier to type the ID than
>>> the
>>> full address. While for the userspace PM, it will be managed by a
>>> daemon
>>> that will have to track addresses anyway.
> 
> I think it's still useful to extend this functionality while the
> original behavior is not affected, at least it doesn't hurt.

I'm sorry, I think it is not that simple: if we extend this
functionality, it means we will have to maintain it. Here, the interface
looks buggy because it will not work with all addresses: the initial
ones, the ones not announced but implicitly used, etc.

If the interface does not always work, I don't think we will recommend
using it, then why do we need to maintain it?

> We cannot assume that userspace PM is always managed by a daemon. We
> have exported its interfaces to BPF. We allow users to customize path
> managers. That means we also allow users to use their own userspace PM
> in any way.

I think the BPF PM is different: it is a different interface.

To interact with the userspace PM, it is required to monitor the MPTCP
events sent via Netlink, e.g. to get the token. When a new subflow is
created, the userspace will know which addresses (including the ID) it
is linked to. In this case, why only setting the ID in the address
structure if it doesn't always work, while setting the address will
always work as expected.

> Another consideration is that we need to maintain the consistency
> between in-kernel PM and userspace PM.

Not really: when they can do the same thing, yes, but the two interfaces
are different. We don't have to keep the consistency if it doesn't make
sense to do so.

> For ease of maintenance, we need
> to make these two PMs use the same code as much as possible, and only
> abstract their differences through PM interfaces such as get_addr,
> dump_addr, set_flags, etc.

Yes but there are some limits: if some code is shared between multiple
interfaces, it is important not to break one of them when changing the
code. In other words, if the behaviour is very similar (e.g. get_addr),
that's fine. But if they start to be too different, you have complex
common code where you need to think "OK, this one acts like that, but
the other one like that", and complexity is not good for the
maintenance. In this case, it sounds better to keep them separated.

> At present, the biggest difference between
> the two is that they use different linked lists (pernet-
>> local_addr_list vs. msk->pm.userspace_pm_local_addr_list) to store
> address entries, so we only need to put the code for operating the
> linked lists into the interfaces of each PM. This is also the goal of
> adjusting the pm interfaces in this series.

Yes, but that's not the only difference, because the interfaces are
different.

With the in-kernel PM, we act per netns, while with the userspace PM, it
is per connection. Because of that, addresses lists are managed
differently, leading to different concept, e.g. the list not having all
addresses, the addresses not having ID 0 in one, but OK in the other,
etc. With shared code that acts for both of them, you need to keep
thinking about these differences when reading or writing code, and
that's a source of error I think.

>>> Or in other words, do you have a use-case for this? To me, it looks
>>> like
>>> "yes, you can only set the ID, but it might not always work". Then
>>> maybe
>>> better to always set the full address, no?
> 
> If you're worried that this functionality isn't covered by tests, I've
> added a test that covers it in BPF path manager selftests:
> 
>         err = userspace_pm_set_flags(token, addr, "backup");
>         if (!ASSERT_OK(err, "userspace_pm_set_flags backup"))
>                 goto close_accept;
> 
> 	...
> 
>         err = userspace_pm_set_flags_by_id(token, 100, "nobackup");
>         if (!ASSERT_OK(err, "userspace_pm_set_flags_by_id nobackup"))
>                 goto close_accept;

I would need to check the BPF PM interface, but for me the userspace PM
and BPF PM interfaces don't have to be the same, e.g. why having a dump
if the BPF PM can directly access data from the kernel? Same here for
the ID: it depends if all IDs are tracked in the corresponding list,
e.g. it might not be the case with an "announced" list.

But also yes, if something is exposed to userspace (via Netlink), it
should be covered by a test (using the userspace Netlink interface)

>>>
>>> [1]
>>> https://lore.kernel.org/mptcp/d01d0e8a-5606-4152-aabe-32e4402adeeb@kernel.org/
>>
>> Note: if we drop this patch (I think it is better), maybe patch 8/8
>> is
>> not worth it: not to have a "common" section with plenty of 'if
>> (token)', no? Or do you really need them for the BPF PM?
> 
> Here we are only adjusting set_flags interface of in-kernel PM and
> userspace PM, which has nothing to do with the BPF PM implementation.
> 
> It seems that moving the code in mptcp_pm_nl_set_flags_doit() to
> mptcp_pm_set_flags() can remove these 'if (token)':
> 
> int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info
> *info)
> {
>         return mptcp_pm_set_flags(info);

I'm not sure whether it is useful to have one function simply calling
another function that is only used once.

> }
> 
> static int mptcp_pm_set_flags(struct genl_info *info)
> {
>         struct mptcp_pm_addr_entry loc = { .addr = { .family =
> AF_UNSPEC }, };
>         struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
>         struct nlattr *attr_loc, *attr_rem;
>         int ret;
> 
>         if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
>                 return -EINVAL;
> 
>         attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
>         ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
>         if (ret < 0)
>                 return ret;
> 
>         if (info->attrs[MPTCP_PM_ATTR_TOKEN]) {
>                 if (GENL_REQ_ATTR_CHECK(info,
> MPTCP_PM_ATTR_ADDR_REMOTE))
>                         return -EINVAL;
> 
>                 attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
>                 ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
>                 if (ret < 0)
>                         return ret;
> 
>                 if (rem.family == AF_UNSPEC) {
>                         NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
>                                             "invalid remote address
> family");
>                         return -EINVAL;
>                 }
> 
>                 return mptcp_userspace_pm_set_flags(&loc, &rem, info);
>         }

The problem is the same: ↑ is specific to the userspace PM, why moving
the code here in the common section then?

Same for the code ↓.

So at the end, the only common code is the parsing of the local address,
so just GENL_REQ_ATTR_CHECK(MPTCP_PM_ATTR_ADDR) and
mptcp_pm_parse_entry(MPTCP_PM_ATTR_ADDR). Is it worth it?

So if we want to share code, all we can get I think is this:

  int mptcp_pm_nl_set_flags_doit(...)
  {
      (...)

      if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
          return -EINVAL;

      attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
      ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
      if (ret < 0)
          return ret;

      if (info->attrs[MPTCP_PM_ATTR_TOKEN])
           return mptcp_userspace_pm_set_flags(&loc, info);

      return mptcp_pm_nl_set_flags(&loc, info);
  }

Not a lot to share, but at least there is nothing PM specific here,
except to pick the interface to continue with. And yes, that's something
that could be done, but that's not much...

> 
>         if (loc.addr.family == AF_UNSPEC) {
>                 if (!loc.addr.id) {
>                         NL_SET_ERR_MSG_ATTR(info->extack, attr_loc,
>                                             "missing address ID");
>                         return -EOPNOTSUPP;
> 		}
>         }
> 
>         return mptcp_pm_nl_set_flags(&loc, info);
> }
> 
> WDYT?
> 
> -Geliang
> 
>>
>> Cheers,
>> Matt
> 

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

Re: [PATCH mptcp-next v3 5/8] mptcp: userspace pm set_flags id support

[Geliang Tang]

Hi Matt,

On Thu, 2025-01-09 at 13:20 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for your reply!
> 
> On 09/01/2025 04:40, Geliang Tang wrote:
> > Hi Matt,
> > 
> > Thanks for the review!
> > 
> > On Wed, 2025-01-08 at 19:51 +0100, Matthieu Baerts wrote:
> > > On 08/01/2025 19:47, Matthieu Baerts wrote:
> > > > Hi Geliang,
> > > > 
> > > > On 08/01/2025 05:21, Geliang Tang wrote:
> > > > > From: Geliang Tang <tanggeliang@kylinos.cn>
> > > > > 
> > > > > Similar to in-kernel PM, this patch adds address ID support
> > > > > to
> > > > > set_flags()
> > > > > interface of userspace PM, allowing it to work with either an
> > > > > address or
> > > > > an address ID.
> > > > > 
> > > > > When an address ID is used,
> > > > > mptcp_userspace_pm_lookup_addr_by_id() helper
> > > > > is used to look up the address entry in the local address
> > > > > list
> > > > > instead of
> > > > > using mptcp_userspace_pm_lookup_addr().
> > > > 
> > > > Mmh, I'm still not sure about that. As I was saying in [1], if
> > > > I'm
> > > > not
> > > > mistaken, with the userspace PM, it is possible not to find any
> > > > entries
> > > > here, e.g.: if a subflow using this address has not been added
> > > > or
> > > > the
> > > > address has not been announced. (I guess the initial address is
> > > > not
> > > > there then).
> > 
> > The previous version (in [1]) did have this issue and
> > userspace_pm.sh
> > tests would fail because of it, but this new version has fixed it.
> > 
> >  mptcp_pm_nl_mp_prio_send_ack(msk,
> >                               entry ? &entry->addr : &local->addr,
> >                               remote, bkup);
> > 
> > When the entry is not found, we continue to pass local->addr to
> > ensure
> > the same behavior as before.
> 
> Yes indeed, the tests are fixed, but if 'entry' is NULL, the address
> you
> will give will be empty, so it will not be able to find any subflow
> to
> send the MP_PRIO, right?
> 
> > > > Do you think this patch is worth it? Setting by ID for the in-
> > > > kernel PM
> > > > makes sense: unique ID for the netns, easier to type the ID
> > > > than
> > > > the
> > > > full address. While for the userspace PM, it will be managed by
> > > > a
> > > > daemon
> > > > that will have to track addresses anyway.
> > 
> > I think it's still useful to extend this functionality while the
> > original behavior is not affected, at least it doesn't hurt.
> 
> I'm sorry, I think it is not that simple: if we extend this
> functionality, it means we will have to maintain it. Here, the
> interface
> looks buggy because it will not work with all addresses: the initial
> ones, the ones not announced but implicitly used, etc.
> 
> If the interface does not always work, I don't think we will
> recommend
> using it, then why do we need to maintain it?
> 
> > We cannot assume that userspace PM is always managed by a daemon.
> > We
> > have exported its interfaces to BPF. We allow users to customize
> > path
> > managers. That means we also allow users to use their own userspace
> > PM
> > in any way.
> 
> I think the BPF PM is different: it is a different interface.
> 
> To interact with the userspace PM, it is required to monitor the
> MPTCP
> events sent via Netlink, e.g. to get the token. When a new subflow is
> created, the userspace will know which addresses (including the ID)
> it
> is linked to. In this case, why only setting the ID in the address
> structure if it doesn't always work, while setting the address will
> always work as expected.
> 
> > Another consideration is that we need to maintain the consistency
> > between in-kernel PM and userspace PM.
> 
> Not really: when they can do the same thing, yes, but the two
> interfaces
> are different. We don't have to keep the consistency if it doesn't
> make
> sense to do so.
> 
> > For ease of maintenance, we need
> > to make these two PMs use the same code as much as possible, and
> > only
> > abstract their differences through PM interfaces such as get_addr,
> > dump_addr, set_flags, etc.
> 
> Yes but there are some limits: if some code is shared between
> multiple
> interfaces, it is important not to break one of them when changing
> the
> code. In other words, if the behaviour is very similar (e.g.
> get_addr),
> that's fine. But if they start to be too different, you have complex
> common code where you need to think "OK, this one acts like that, but
> the other one like that", and complexity is not good for the
> maintenance. In this case, it sounds better to keep them separated.
> 
> > At present, the biggest difference between
> > the two is that they use different linked lists (pernet-
> > > local_addr_list vs. msk->pm.userspace_pm_local_addr_list) to
> > > store
> > address entries, so we only need to put the code for operating the
> > linked lists into the interfaces of each PM. This is also the goal
> > of
> > adjusting the pm interfaces in this series.
> 
> Yes, but that's not the only difference, because the interfaces are
> different.
> 
> With the in-kernel PM, we act per netns, while with the userspace PM,
> it
> is per connection. Because of that, addresses lists are managed
> differently, leading to different concept, e.g. the list not having
> all
> addresses, the addresses not having ID 0 in one, but OK in the other,
> etc. With shared code that acts for both of them, you need to keep
> thinking about these differences when reading or writing code, and
> that's a source of error I think.
> 
> > > > Or in other words, do you have a use-case for this? To me, it
> > > > looks
> > > > like
> > > > "yes, you can only set the ID, but it might not always work".
> > > > Then
> > > > maybe
> > > > better to always set the full address, no?
> > 
> > If you're worried that this functionality isn't covered by tests,
> > I've
> > added a test that covers it in BPF path manager selftests:
> > 
> >         err = userspace_pm_set_flags(token, addr, "backup");
> >         if (!ASSERT_OK(err, "userspace_pm_set_flags backup"))
> >                 goto close_accept;
> > 
> > 	...
> > 
> >         err = userspace_pm_set_flags_by_id(token, 100, "nobackup");
> >         if (!ASSERT_OK(err, "userspace_pm_set_flags_by_id
> > nobackup"))
> >                 goto close_accept;
> 
> I would need to check the BPF PM interface, but for me the userspace
> PM
> and BPF PM interfaces don't have to be the same, e.g. why having a
> dump
> if the BPF PM can directly access data from the kernel? Same here for
> the ID: it depends if all IDs are tracked in the corresponding list,
> e.g. it might not be the case with an "announced" list.
> 
> But also yes, if something is exposed to userspace (via Netlink), it
> should be covered by a test (using the userspace Netlink interface)
> 
> > > > 
> > > > [1]
> > > > https://lore.kernel.org/mptcp/d01d0e8a-5606-4152-aabe-32e4402adeeb@kernel.org/
> > > 
> > > Note: if we drop this patch (I think it is better), maybe patch
> > > 8/8
> > > is
> > > not worth it: not to have a "common" section with plenty of 'if
> > > (token)', no? Or do you really need them for the BPF PM?
> > 
> > Here we are only adjusting set_flags interface of in-kernel PM and
> > userspace PM, which has nothing to do with the BPF PM
> > implementation.
> > 
> > It seems that moving the code in mptcp_pm_nl_set_flags_doit() to
> > mptcp_pm_set_flags() can remove these 'if (token)':
> > 
> > int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct
> > genl_info
> > *info)
> > {
> >         return mptcp_pm_set_flags(info);
> 
> I'm not sure whether it is useful to have one function simply calling
> another function that is only used once.

I kept this function in v4 to make it consistent with mptcp_pm_get_addr
and mptcp_pm_dump_addr.

And "mptcp: userspace pm set_flags id support" is moved out of this set
in v4.

Thanks,
-Geliang

> 
> > }
> > 
> > static int mptcp_pm_set_flags(struct genl_info *info)
> > {
> >         struct mptcp_pm_addr_entry loc = { .addr = { .family =
> > AF_UNSPEC }, };
> >         struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
> >         struct nlattr *attr_loc, *attr_rem;
> >         int ret;
> > 
> >         if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
> >                 return -EINVAL;
> > 
> >         attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
> >         ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
> >         if (ret < 0)
> >                 return ret;
> > 
> >         if (info->attrs[MPTCP_PM_ATTR_TOKEN]) {
> >                 if (GENL_REQ_ATTR_CHECK(info,
> > MPTCP_PM_ATTR_ADDR_REMOTE))
> >                         return -EINVAL;
> > 
> >                 attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
> >                 ret = mptcp_pm_parse_addr(attr_rem, info, &rem);
> >                 if (ret < 0)
> >                         return ret;
> > 
> >                 if (rem.family == AF_UNSPEC) {
> >                         NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
> >                                             "invalid remote address
> > family");
> >                         return -EINVAL;
> >                 }
> > 
> >                 return mptcp_userspace_pm_set_flags(&loc, &rem,
> > info);
> >         }
> 
> The problem is the same: ↑ is specific to the userspace PM, why
> moving
> the code here in the common section then?
> 
> Same for the code ↓.
> 
> So at the end, the only common code is the parsing of the local
> address,
> so just GENL_REQ_ATTR_CHECK(MPTCP_PM_ATTR_ADDR) and
> mptcp_pm_parse_entry(MPTCP_PM_ATTR_ADDR). Is it worth it?
> 
> So if we want to share code, all we can get I think is this:
> 
>   int mptcp_pm_nl_set_flags_doit(...)
>   {
>       (...)
> 
>       if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
>           return -EINVAL;
> 
>       attr_loc = info->attrs[MPTCP_PM_ATTR_ADDR];
>       ret = mptcp_pm_parse_entry(attr_loc, info, false, &loc);
>       if (ret < 0)
>           return ret;
> 
>       if (info->attrs[MPTCP_PM_ATTR_TOKEN])
>            return mptcp_userspace_pm_set_flags(&loc, info);
> 
>       return mptcp_pm_nl_set_flags(&loc, info);
>   }
> 
> Not a lot to share, but at least there is nothing PM specific here,
> except to pick the interface to continue with. And yes, that's
> something
> that could be done, but that's not much...
> 
> > 
> >         if (loc.addr.family == AF_UNSPEC) {
> >                 if (!loc.addr.id) {
> >                         NL_SET_ERR_MSG_ATTR(info->extack, attr_loc,
> >                                             "missing address ID");
> >                         return -EOPNOTSUPP;
> > 		}
> >         }
> > 
> >         return mptcp_pm_nl_set_flags(&loc, info);
> > }
> > 
> > WDYT?
> > 
> > -Geliang
> > 
> > > 
> > > Cheers,
> > > Matt
> > 
> 
> Cheers,
> Matt