diff mbox series

[mptcp-next,v1,5/6] mptcp: pm: add remote parameter for set_flags

Message ID 10ce66a82baec7e37fcde50554c1a0f0f2a9da6a.1740638334.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded, archived
Delegated to: Matthieu Baerts
Headers show
Series BPF path manager, part 5 | expand

Checks

Context Check Description
matttbe/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 96 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build success Build and static analysis OK
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅

Commit Message

Geliang Tang Feb. 27, 2025, 6:43 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

The remote address of set_flags() interface is useful for userspace PM,
but unused in in-kernel PM.

But an additional "changed" parameter needs to be passed to set_flags() of
in-kernel PM. One option is to add a "u8 changed" parameter to set_flags()
interface:

	set_flags(struct mptcp_pm_addr_entry *local,
		  struct mptcp_addr_info *remote,
		  u8 changed)

A better option is to add a struct mptcp_pm_addr_entry "remote" parameter
for set_flags(), so that "remote->addr" can be used for userspace PM, and
"remote->flags" can be used for in-kernel PM to replace the additional
"changed" parameter.

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

Comments

Matthieu Baerts Feb. 27, 2025, 12:26 p.m. UTC | #1 
Hi Geliang,

On 27/02/2025 07:43, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The remote address of set_flags() interface is useful for userspace PM,
> but unused in in-kernel PM.
> 
> But an additional "changed" parameter needs to be passed to set_flags() of
> in-kernel PM. One option is to add a "u8 changed" parameter to set_flags()
> interface:
> 
> 	set_flags(struct mptcp_pm_addr_entry *local,
> 		  struct mptcp_addr_info *remote,
> 		  u8 changed)
> 
> A better option is to add a struct mptcp_pm_addr_entry "remote" parameter
> for set_flags(), so that "remote->addr" can be used for userspace PM, and
> "remote->flags" can be used for in-kernel PM to replace the additional
> "changed" parameter.

I don't think I understand why we need this modification.

The 'set_flags' netlink command is there to change the behaviour of the
in-kernel PM and the userspace PM. It will certainly not be useful for a
BPF PM because all its configurations will be done via BPF, not via
Netlink. If the userspace send this Netlink command when a BPF PM will
be used, nothing will happen:
- if a token is specified, the userspace PM should not do anything if it
is not the designated PM for this connection.
- if not, the in-kernel PM will only modify the connections handled by
the in-kernel PM.

The in-kernel and userspace PMs use the same command, but for different
things, hence requiring different parameters: e.g. the userspace one
requires the token because the modification will be only for one
connection, while the in-kernel will be for a specific endpoint.

So the current code looks OK: in the common part (pm.c), only the common
attributes are parsed. On the userspace side, extra parameter will be
parsed, that's fine. On the in-kernel side, 'changed' will be looked at
simply because there is no need to iterate over all the connections if
nothing has changed on the corresponding endpoint side. In other words,
I don't think the code should be changed, no?

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 1844107f6f93..adb2735c4131 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -531,7 +531,8 @@  int mptcp_pm_nl_get_addr_dumpit(struct sk_buff *msg,
 static int mptcp_pm_set_flags(struct genl_info *info)
 {
 	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
-	struct nlattr *attr_loc;
+	struct mptcp_pm_addr_entry rem = { .addr = { .family = AF_UNSPEC }, };
+	struct nlattr *attr_loc, *attr_rem;
 	int ret = -EINVAL;
 
 	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
@@ -542,9 +543,20 @@  static int mptcp_pm_set_flags(struct genl_info *info)
 	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);
+	if (info->attrs[MPTCP_PM_ATTR_TOKEN]) {
+		attr_rem = info->attrs[MPTCP_PM_ATTR_ADDR_REMOTE];
+		ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
+		if (ret < 0)
+			return ret;
+
+		if (rem.addr.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);
+	}
+	return mptcp_pm_nl_set_flags(&loc, &rem, info);
 }
 
 int mptcp_pm_nl_set_flags_doit(struct sk_buff *skb, struct genl_info *info)
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index e546388070b4..053f2bec9042 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1949,6 +1949,7 @@  static void mptcp_nl_set_flags(struct net *net,
 }
 
 int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
+			  struct mptcp_pm_addr_entry *remote,
 			  struct genl_info *info)
 {
 	struct nlattr *attr = info->attrs[MPTCP_PM_ATTR_ADDR];
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 05d59ad1a0bc..dddd2daed650 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -553,19 +553,16 @@  int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 }
 
 int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
+				 struct mptcp_pm_addr_entry *remote,
 				 struct genl_info *info)
 {
-	struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
 	struct mptcp_pm_addr_entry *entry;
-	struct nlattr *attr, *attr_rem;
 	struct mptcp_sock *msk;
+	struct nlattr *attr;
 	int ret = -EINVAL;
 	struct sock *sk;
 	u8 bkup = 0;
 
-	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
-		return ret;
-
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
 		return ret;
@@ -580,18 +577,6 @@  int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
 		goto set_flags_err;
 	}
 
-	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 (local->flags & MPTCP_PM_ADDR_FLAG_BACKUP)
 		bkup = 1;
 
@@ -606,7 +591,7 @@  int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
 	spin_unlock_bh(&msk->pm.lock);
 
 	lock_sock(sk);
-	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &local->addr, &rem, bkup);
+	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &local->addr, &remote->addr, bkup);
 	release_sock(sk);
 
 	/* mptcp_pm_nl_mp_prio_send_ack() only fails in one case */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 0b6695fbb645..5adb791898ec 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1038,8 +1038,10 @@  bool mptcp_lookup_subflow_by_saddr(const struct list_head *list,
 bool mptcp_remove_anno_list_by_saddr(struct mptcp_sock *msk,
 				     const struct mptcp_addr_info *addr);
 int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
+			  struct mptcp_pm_addr_entry *remote,
 			  struct genl_info *info);
 int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
+				 struct mptcp_pm_addr_entry *remote,
 				 struct genl_info *info);
 int mptcp_pm_announce_addr(struct mptcp_sock *msk,
 			   const struct mptcp_addr_info *addr,