[PATCH mptcp-next v4 0/7] BPF path manager, part 3

[PATCH mptcp-next v4 0/7] BPF path manager, part 3

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v4:
 - add a new patch, "add a build check for userspace_pm_dump_addr" as
   Matt suggested.
 - patch 4, fix a warning here reported by CI:
   WARNING: line length of 84 exceeds 80 columns
   #27: FILE: net/mptcp/pm_userspace.c:468:

v3:
 - drop "mptcp: update address type of get_local_id".

v2:
 - a new patch "hold msk lock before removing id 0 address"

Drop the patch "mptcp: reuse sending nlmsg code in dump_addr", add a new
helper mptcp_pm_genl_fill_addr() to save the duplicated code.

Define struct mptcp_pm_addr_id_bitmap in protocol.h, instead of defining
mptcp_pm_addr_id_bitmap_t in include/net/mptcp.h, since get_addr() and
dump_addr() interfaces of BPF userspace pm is dropped as Matt suggested.

Geliang Tang (7):
  mptcp: add mptcp_pm_genl_fill_addr helper
  mptcp: add a build check for userspace_pm_dump_addr
  mptcp: add struct mptcp_pm_addr_id_bitmap
  mptcp: drop inet6_sk in mptcp_nl_find_ssk
  mptcp: drop match in userspace_pm_append_new_local_addr
  mptcp: hold msk lock before removing id 0 address
  mptcp: change is_backup interfaces as get_flags

 net/mptcp/pm.c           | 28 ++++++++++++++--
 net/mptcp/pm_netlink.c   | 20 +++--------
 net/mptcp/pm_userspace.c | 71 ++++++++++++++--------------------------
 net/mptcp/protocol.h     | 11 +++++--
 4 files changed, 65 insertions(+), 65 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v4 1/7] mptcp: add mptcp_pm_genl_fill_addr helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

To save some redundant code in dump_addr() interfaces of both the
netlink PM and userspace PM, the code that calls netlink message
helpers (genlmsg_put/cancel/end) and mptcp_nl_fill_addr() is wrapped
into a new helper mptcp_pm_genl_fill_addr().

This helper will also be used in BPF path managers.

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index b1f36dc1a091..16cacce6c10f 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -489,6 +489,27 @@ int mptcp_pm_nl_get_addr_doit(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
+int mptcp_pm_genl_fill_addr(struct sk_buff *msg,
+			    struct netlink_callback *cb,
+			    struct mptcp_pm_addr_entry *entry)
+{
+	void *hdr;
+
+	hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
+			  cb->nlh->nlmsg_seq, &mptcp_genl_family,
+			  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
+	if (!hdr)
+		return -EINVAL;
+
+	if (mptcp_nl_fill_addr(msg, entry) < 0) {
+		genlmsg_cancel(msg, hdr);
+		return -EINVAL;
+	}
+
+	genlmsg_end(msg, hdr);
+	return 0;
+}
+
 static int mptcp_pm_dump_addr(struct sk_buff *msg, struct netlink_callback *cb)
 {
 	const struct genl_info *info = genl_info_dump(cb);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index fef01692eaed..afd517ff260c 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1798,7 +1798,6 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
 	int id = cb->args[0];
-	void *hdr;
 	int i;
 
 	pernet = pm_nl_get_pernet(net);
@@ -1813,19 +1812,10 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			if (entry->addr.id <= id)
 				continue;
 
-			hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
-					  cb->nlh->nlmsg_seq, &mptcp_genl_family,
-					  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
-			if (!hdr)
+			if (mptcp_pm_genl_fill_addr(msg, cb, entry))
 				break;
 
-			if (mptcp_nl_fill_addr(msg, entry) < 0) {
-				genlmsg_cancel(msg, hdr);
-				break;
-			}
-
 			id = entry->addr.id;
-			genlmsg_end(msg, hdr);
 		}
 	}
 	rcu_read_unlock();
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 277cf092a870..b50462b527bd 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -641,7 +641,6 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 	struct mptcp_sock *msk;
 	int ret = -EINVAL;
 	struct sock *sk;
-	void *hdr;
 
 	bitmap = (struct id_bitmap *)cb->ctx;
 
@@ -657,19 +656,10 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 		if (test_bit(entry->addr.id, bitmap->map))
 			continue;
 
-		hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
-				  cb->nlh->nlmsg_seq, &mptcp_genl_family,
-				  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
-		if (!hdr)
+		if (mptcp_pm_genl_fill_addr(msg, cb, entry))
 			break;
 
-		if (mptcp_nl_fill_addr(msg, entry) < 0) {
-			genlmsg_cancel(msg, hdr);
-			break;
-		}
-
 		__set_bit(entry->addr.id, bitmap->map);
-		genlmsg_end(msg, hdr);
 	}
 	spin_unlock_bh(&msk->pm.lock);
 	release_sock(sk);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 6fb536de9981..20941405a1ea 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1060,6 +1060,9 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
 					      struct request_sock *req);
 int mptcp_nl_fill_addr(struct sk_buff *skb,
 		       struct mptcp_pm_addr_entry *entry);
+int mptcp_pm_genl_fill_addr(struct sk_buff *msg,
+			    struct netlink_callback *cb,
+			    struct mptcp_pm_addr_entry *entry);
 
 static inline bool mptcp_pm_should_add_signal(struct mptcp_sock *msk)
 {
-- 
2.43.0

Re: [PATCH mptcp-next v4 1/7] mptcp: add mptcp_pm_genl_fill_addr helper

[Matthieu Baerts]

Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To save some redundant code in dump_addr() interfaces of both the
> netlink PM and userspace PM, the code that calls netlink message
> helpers (genlmsg_put/cancel/end) and mptcp_nl_fill_addr() is wrapped
> into a new helper mptcp_pm_genl_fill_addr().

I'm OK with the suggested modification here below (just one small
detail, please see below), but ...

> This helper will also be used in BPF path managers.

... that doesn't sound right: I don't see why a BPF PM has to dump
addresses via Netlink. In your cover-letter, you mentioned the
dump_addr() will not be implemented in the BPF PM, so I guess you left
this line by accident, right?

If not: with the in-kernel PM, it is needed to dump the endpoints to
know what has been set. With the userspace PM, the dump is useful for a
userspace PM daemon to recover from a restart. For these two PM, Netlink
is used between the userspace and the kernelspace, so that makes sense
to continue to do so. With the BPF PM, I don't really see the point: all
the communication (configuration, status, decisions, etc.) should be
done via BPF, no?

(...)

> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index fef01692eaed..afd517ff260c 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1798,7 +1798,6 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  	struct mptcp_pm_addr_entry *entry;
>  	struct pm_nl_pernet *pernet;
>  	int id = cb->args[0];
> -	void *hdr;
>  	int i;
>  
>  	pernet = pm_nl_get_pernet(net);
> @@ -1813,19 +1812,10 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  			if (entry->addr.id <= id)
>  				continue;
>  
> -			hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
> -					  cb->nlh->nlmsg_seq, &mptcp_genl_family,
> -					  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
> -			if (!hdr)
> +			if (mptcp_pm_genl_fill_addr(msg, cb, entry))

Even if technically, it is correct, it feels "strange" to see:

  if (foo())
      /* error path */

It feels a bit like having:

  if (success())
      /* error */

If the goal is to stop in case of error, better being explicit about that:

  if (foo() < 0)
      /* error path */

>  				break;
>  
> -			if (mptcp_nl_fill_addr(msg, entry) < 0) {
> -				genlmsg_cancel(msg, hdr);
> -				break;
> -			}
> -
>  			id = entry->addr.id;
> -			genlmsg_end(msg, hdr);
>  		}
>  	}
>  	rcu_read_unlock();
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 277cf092a870..b50462b527bd 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -641,7 +641,6 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  	struct mptcp_sock *msk;
>  	int ret = -EINVAL;
>  	struct sock *sk;
> -	void *hdr;
>  
>  	bitmap = (struct id_bitmap *)cb->ctx;
>  
> @@ -657,19 +656,10 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
>  		if (test_bit(entry->addr.id, bitmap->map))
>  			continue;
>  
> -		hdr = genlmsg_put(msg, NETLINK_CB(cb->skb).portid,
> -				  cb->nlh->nlmsg_seq, &mptcp_genl_family,
> -				  NLM_F_MULTI, MPTCP_PM_CMD_GET_ADDR);
> -		if (!hdr)
> +		if (mptcp_pm_genl_fill_addr(msg, cb, entry))

Same here: < 0

>  			break;
>  
> -		if (mptcp_nl_fill_addr(msg, entry) < 0) {
> -			genlmsg_cancel(msg, hdr);
> -			break;
> -		}
> -
>  		__set_bit(entry->addr.id, bitmap->map);
> -		genlmsg_end(msg, hdr);
>  	}
>  	spin_unlock_bh(&msk->pm.lock);
>  	release_sock(sk);

(...)

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

[PATCH mptcp-next v4 2/7] mptcp: add a build check for userspace_pm_dump_addr

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a build check for mptcp_userspace_pm_dump_addr() to make
sure there is enough space in 'cb->ctx' to store an address id bitmap.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index b50462b527bd..540b2080f02b 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -642,6 +642,8 @@ int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 	int ret = -EINVAL;
 	struct sock *sk;
 
+	BUILD_BUG_ON(sizeof(struct id_bitmap) > sizeof(cb->ctx));
+
 	bitmap = (struct id_bitmap *)cb->ctx;
 
 	msk = mptcp_userspace_pm_get_sock(info);
-- 
2.43.0

Re: [PATCH mptcp-next v4 2/7] mptcp: add a build check for userspace_pm_dump_addr

[Matthieu Baerts]

Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a build check for mptcp_userspace_pm_dump_addr() to make
> sure there is enough space in 'cb->ctx' to store an address id bitmap.

This looks good to me, thanks!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

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

[PATCH mptcp-next v4 3/7] mptcp: add struct mptcp_pm_addr_id_bitmap

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

To simplify the use of bitmap in BPF, a new type for MPTCP userspace
pm id bitmap, struct mptcp_pm_addr_id_bitmap is defined. Because
there's no way to use DECLARE_BITMAP macro in BPF program, and it's
not easy to reimplement it in BPF.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 540b2080f02b..5c7a8817595c 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -47,7 +47,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 						    struct mptcp_pm_addr_entry *entry,
 						    bool needs_id)
 {
-	DECLARE_BITMAP(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
+	struct mptcp_pm_addr_id_bitmap id_bitmap;
 	struct mptcp_pm_addr_entry *match = NULL;
 	struct sock *sk = (struct sock *)msk;
 	struct mptcp_pm_addr_entry *e;
@@ -55,7 +55,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 	bool id_match = false;
 	int ret = -EINVAL;
 
-	bitmap_zero(id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);
+	bitmap_zero(id_bitmap.map, MPTCP_PM_MAX_ADDR_ID + 1);
 
 	spin_lock_bh(&msk->pm.lock);
 	mptcp_for_each_userspace_pm_addr(msk, e) {
@@ -69,7 +69,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 		} else if (addr_match || id_match) {
 			break;
 		}
-		__set_bit(e->addr.id, id_bitmap);
+		__set_bit(e->addr.id, id_bitmap.map);
 	}
 
 	if (!match && !addr_match && !id_match) {
@@ -84,7 +84,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 
 		*e = *entry;
 		if (!e->addr.id && needs_id)
-			e->addr.id = find_next_zero_bit(id_bitmap,
+			e->addr.id = find_next_zero_bit(id_bitmap.map,
 							MPTCP_PM_MAX_ADDR_ID + 1,
 							1);
 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
@@ -633,18 +633,17 @@ int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
 int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
 				 struct netlink_callback *cb)
 {
-	struct id_bitmap {
-		DECLARE_BITMAP(map, MPTCP_PM_MAX_ADDR_ID + 1);
-	} *bitmap;
 	const struct genl_info *info = genl_info_dump(cb);
+	struct mptcp_pm_addr_id_bitmap *bitmap;
 	struct mptcp_pm_addr_entry *entry;
 	struct mptcp_sock *msk;
 	int ret = -EINVAL;
 	struct sock *sk;
 
-	BUILD_BUG_ON(sizeof(struct id_bitmap) > sizeof(cb->ctx));
+	BUILD_BUG_ON(sizeof(struct mptcp_pm_addr_id_bitmap) >
+		     sizeof(cb->ctx));
 
-	bitmap = (struct id_bitmap *)cb->ctx;
+	bitmap = (struct mptcp_pm_addr_id_bitmap *)cb->ctx;
 
 	msk = mptcp_userspace_pm_get_sock(info);
 	if (!msk)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 20941405a1ea..aca3de2baf81 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -212,6 +212,10 @@ enum mptcp_addr_signal_status {
 /* max value of mptcp_addr_info.id */
 #define MPTCP_PM_MAX_ADDR_ID		U8_MAX
 
+struct mptcp_pm_addr_id_bitmap {
+	DECLARE_BITMAP(map, MPTCP_PM_MAX_ADDR_ID + 1);
+};
+
 struct mptcp_pm_data {
 	struct mptcp_addr_info local;
 	struct mptcp_addr_info remote;
-- 
2.43.0

Re: [PATCH mptcp-next v4 3/7] mptcp: add struct mptcp_pm_addr_id_bitmap

[Matthieu Baerts]

Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> To simplify the use of bitmap in BPF, a new type for MPTCP userspace
> pm id bitmap, struct mptcp_pm_addr_id_bitmap is defined. Because
> there's no way to use DECLARE_BITMAP macro in BPF program, and it's
> not easy to reimplement it in BPF.

OK, I got that this is needed in BPF, but: why do you need it in the BPF PM?

I guess it is not for the dump_addr interface, right? If yes, then maybe
you don't need that, see my comment on the previous patch.

If not, what is it for?
I can understand that it might be needed to have a bitmap per msk to
handle the different address IDs. But then why is it needed to change
the code in pm_userspace.c, especially in mptcp_userspace_pm_dump_addr?

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

[PATCH mptcp-next v4 4/7] mptcp: drop inet6_sk in mptcp_nl_find_ssk

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In mptcp_nl_find_ssk(), 'issk' has already been got through inet_sk(). No
need to use inet6_sk() to get 'pinfo' again, just use issk->pinet6 instead.
This patch also drops this 'pinfo' variable.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 5c7a8817595c..bd09a637049d 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -465,9 +465,8 @@ static struct sock *mptcp_nl_find_ssk(struct mptcp_sock *msk,
 			break;
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 		case AF_INET6: {
-			const struct ipv6_pinfo *pinfo = inet6_sk(ssk);
-
-			if (!ipv6_addr_equal(&local->addr6, &pinfo->saddr) ||
+			if (!ipv6_addr_equal(&local->addr6,
+					     &issk->pinet6->saddr) ||
 			    !ipv6_addr_equal(&remote->addr6, &ssk->sk_v6_daddr))
 				continue;
 			break;
-- 
2.43.0

Re: [PATCH mptcp-next v4 4/7] mptcp: drop inet6_sk in mptcp_nl_find_ssk

[Matthieu Baerts]

Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> In mptcp_nl_find_ssk(), 'issk' has already been got through inet_sk(). No
> need to use inet6_sk() to get 'pinfo' again, just use issk->pinet6 instead.
> This patch also drops this 'pinfo' variable.

This looks good to me, thanks!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

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

[PATCH mptcp-next v4 5/7] mptcp: drop match in userspace_pm_append_new_local_addr

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The variable 'match' in mptcp_userspace_pm_append_new_local_addr() is a
redundant one, and this patch drops it.

No need to define 'match' as 'struct mptcp_pm_addr_entry *' type. In this
function, it's only used to check whether it's NULL. It can be defined as
a Boolean one.

Also other variables 'addr_match' and 'id_match' make 'match' a redundant
one, which can be replaced by directly checking 'addr_match && id_match'.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index bd09a637049d..8a0202d8bfa4 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -48,7 +48,6 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 						    bool needs_id)
 {
 	struct mptcp_pm_addr_id_bitmap id_bitmap;
-	struct mptcp_pm_addr_entry *match = NULL;
 	struct sock *sk = (struct sock *)msk;
 	struct mptcp_pm_addr_entry *e;
 	bool addr_match = false;
@@ -63,16 +62,12 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 		if (addr_match && entry->addr.id == 0 && needs_id)
 			entry->addr.id = e->addr.id;
 		id_match = (e->addr.id == entry->addr.id);
-		if (addr_match && id_match) {
-			match = e;
+		if (addr_match || id_match)
 			break;
-		} else if (addr_match || id_match) {
-			break;
-		}
 		__set_bit(e->addr.id, id_bitmap.map);
 	}
 
-	if (!match && !addr_match && !id_match) {
+	if (!addr_match && !id_match) {
 		/* Memory for the entry is allocated from the
 		 * sock option buffer.
 		 */
@@ -90,7 +85,7 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
 		msk->pm.local_addr_used++;
 		ret = e->addr.id;
-	} else if (match) {
+	} else if (addr_match && id_match) {
 		ret = entry->addr.id;
 	}
 
-- 
2.43.0

Re: [PATCH mptcp-next v4 5/7] mptcp: drop match in userspace_pm_append_new_local_addr

[Matthieu Baerts]

Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> The variable 'match' in mptcp_userspace_pm_append_new_local_addr() is a
> redundant one, and this patch drops it.
> 
> No need to define 'match' as 'struct mptcp_pm_addr_entry *' type. In this
> function, it's only used to check whether it's NULL. It can be defined as
> a Boolean one.
> 
> Also other variables 'addr_match' and 'id_match' make 'match' a redundant
> one, which can be replaced by directly checking 'addr_match && id_match'.

This looks good to me, thanks!

Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

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

[PATCH mptcp-next v4 6/7] mptcp: hold msk lock before removing id 0 address

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In mptcp_pm_nl_remove_doit(), move 'lock_sock(sk)' before invoking the
function mptcp_userspace_pm_remove_id_zero_address(). Then no need to
hold and release this lock in this function. And 'sk' and 'err' variables
and 'remove_err' label in it can be dropped.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 8a0202d8bfa4..9a6c17948d7e 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -256,11 +256,8 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk)
 {
 	struct mptcp_rm_list list = { .nr = 0 };
 	struct mptcp_subflow_context *subflow;
-	struct sock *sk = (struct sock *)msk;
 	bool has_id_0 = false;
-	int err = -EINVAL;
 
-	lock_sock(sk);
 	mptcp_for_each_subflow(msk, subflow) {
 		if (READ_ONCE(subflow->local_id) == 0) {
 			has_id_0 = true;
@@ -268,7 +265,7 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk)
 		}
 	}
 	if (!has_id_0)
-		goto remove_err;
+		return -EINVAL;
 
 	list.ids[list.nr++] = 0;
 
@@ -276,11 +273,7 @@ static int mptcp_userspace_pm_remove_id_zero_address(struct mptcp_sock *msk)
 	mptcp_pm_remove_addr(msk, &list);
 	spin_unlock_bh(&msk->pm.lock);
 
-	err = 0;
-
-remove_err:
-	release_sock(sk);
-	return err;
+	return 0;
 }
 
 void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
@@ -324,13 +317,14 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
 
 	sk = (struct sock *)msk;
 
+	lock_sock(sk);
+
 	if (id_val == 0) {
 		err = mptcp_userspace_pm_remove_id_zero_address(msk);
+		release_sock(sk);
 		goto out;
 	}
 
-	lock_sock(sk);
-
 	spin_lock_bh(&msk->pm.lock);
 	match = mptcp_userspace_pm_lookup_addr_by_id(msk, id_val);
 	if (!match) {
-- 
2.43.0

Re: [PATCH mptcp-next v4 6/7] mptcp: hold msk lock before removing id 0 address

[Matthieu Baerts]

Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> In mptcp_pm_nl_remove_doit(), move 'lock_sock(sk)' before invoking the
> function mptcp_userspace_pm_remove_id_zero_address(). Then no need to
> hold and release this lock in this function. And 'sk' and 'err' variables
> and 'remove_err' label in it can be dropped.

Sorry, but I'm not sure to understand the reasons behind this. Is it
only to reduce a bit the number of lines? Or because you plan to re-use
the same helper elsewhere?

Please also note that, usually, helpers that can only be used under some
conditions -- e.g. while some resources are locked -- are prefixed with
two underscores. Best to do that here too then.

So if there are reasons to keep this patch (e.g. helper going to be
reused elsewhere), then I think we should rename the helper, e.g.

  __remove_id_zero_address()

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

[PATCH mptcp-next v4 7/7] mptcp: change is_backup interfaces as get_flags

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

is_backup() interface of the path managers is not very common. A more
common approach is to add a get_flags() interface to obtain the flags
value of a given address. Then is_backup() can be implemented through
get_flags() by test whether backup flag is set in the flags value.

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 16cacce6c10f..e275be73b963 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -425,13 +425,16 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
 {
 	struct mptcp_addr_info skc_local;
+	u8 flags;
 
 	mptcp_local_address((struct sock_common *)skc, &skc_local);
 
 	if (mptcp_pm_is_userspace(msk))
-		return mptcp_userspace_pm_is_backup(msk, &skc_local);
+		flags = mptcp_userspace_pm_get_flags(msk, &skc_local);
+	else
+		flags = mptcp_pm_nl_get_flags(msk, &skc_local);
 
-	return mptcp_pm_nl_is_backup(msk, &skc_local);
+	return !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 }
 
 static int mptcp_pm_get_addr(u8 id, struct mptcp_pm_addr_entry *addr,
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index afd517ff260c..c0971b09c5f6 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1172,18 +1172,18 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	return ret;
 }
 
-bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
+u8 mptcp_pm_nl_get_flags(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
 {
 	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
 	struct mptcp_pm_addr_entry *entry;
-	bool backup;
+	u8 flags;
 
 	rcu_read_lock();
 	entry = __lookup_addr(pernet, skc);
-	backup = entry && !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+	flags = entry ? entry->flags : 0;
 	rcu_read_unlock();
 
-	return backup;
+	return flags;
 }
 
 #define MPTCP_PM_CMD_GRP_OFFSET       0
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 9a6c17948d7e..5e9b95270942 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -154,18 +154,18 @@ int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
 	return mptcp_userspace_pm_append_new_local_addr(msk, &new_entry, true);
 }
 
-bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
-				  struct mptcp_addr_info *skc)
+u8 mptcp_userspace_pm_get_flags(struct mptcp_sock *msk,
+				struct mptcp_addr_info *skc)
 {
 	struct mptcp_pm_addr_entry *entry;
-	bool backup;
+	u8 flags;
 
 	spin_lock_bh(&msk->pm.lock);
 	entry = mptcp_userspace_pm_lookup_addr(msk, skc);
-	backup = entry && !!(entry->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+	flags = entry ? entry->flags : 0;
 	spin_unlock_bh(&msk->pm.lock);
 
-	return backup;
+	return flags;
 }
 
 static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *info)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index aca3de2baf81..821d9d51e27c 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1132,8 +1132,8 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
 int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
 int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
 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);
+u8 mptcp_pm_nl_get_flags(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
+u8 mptcp_userspace_pm_get_flags(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
 int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
 			  struct netlink_callback *cb);
 int mptcp_userspace_pm_dump_addr(struct sk_buff *msg,
-- 
2.43.0

Re: [PATCH mptcp-next v4 7/7] mptcp: change is_backup interfaces as get_flags

[Matthieu Baerts]

Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> is_backup() interface of the path managers is not very common. A more
> common approach is to add a get_flags() interface to obtain the flags
> value of a given address. Then is_backup() can be implemented through
> get_flags() by test whether backup flag is set in the flags value.

Mmh, I don't know: the different PMs handle the flags differently, e.g.
on the userspace side, there is no concept of 'subflow', 'signal',
'implicit' and 'fullmesh' like we have with the in-kernel one. In fact,
only 'backup' is used, which is why only 'backup' is common to each others.

In that way, having a dedicated 'is_backup()' interface seems to make
more sense than 'get_flags()'. Plus these 'get_flags()' helpers are only
used in the 'mptcp_pm_is_backup()' interface, so why not only using
_is_backup()?

Do you think we need this modification then?

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

Re: [PATCH mptcp-next v4 0/7] BPF path manager, part 3

[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/12804233622

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


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 v4 0/7] BPF path manager, part 3

[Matthieu Baerts]

Hi Geliang,

On 16/01/2025 08:26, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> v4:
>  - add a new patch, "add a build check for userspace_pm_dump_addr" as
>    Matt suggested.
>  - patch 4, fix a warning here reported by CI:
>    WARNING: line length of 84 exceeds 80 columns
>    #27: FILE: net/mptcp/pm_userspace.c:468:
> 
> v3:
>  - drop "mptcp: update address type of get_local_id".
> 
> v2:
>  - a new patch "hold msk lock before removing id 0 address"
> 
> Drop the patch "mptcp: reuse sending nlmsg code in dump_addr", add a new
> helper mptcp_pm_genl_fill_addr() to save the duplicated code.
> 
> Define struct mptcp_pm_addr_id_bitmap in protocol.h, instead of defining
> mptcp_pm_addr_id_bitmap_t in include/net/mptcp.h, since get_addr() and
> dump_addr() interfaces of BPF userspace pm is dropped as Matt suggested.

Thank you for the patches!

I have a few suggestions, please see my comments on the individual patches.

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