[PATCH mptcp-next v1 0/6] BPF path manager, part 4

[PATCH mptcp-next v1 0/6] BPF path manager, part 4

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Some path manager related refactoring and cleanups.

Geliang Tang (6):
  mptcp: pm: in-kernel: avoid access entry without lock
  mptcp: pm: in-kernel: reduce parameters of set_flags
  mptcp: pm: use addr entry for get_local_id
  mptcp: pm: in-kernel: use kmemdup helper
  sock: add sock_kmemdup helper
  mptcp: pm: userspace: use sock_kmemdup helper

 include/net/sock.h       |  1 +
 net/core/sock.c          | 23 +++++++++++++++++++++++
 net/mptcp/pm.c           |  9 ++++++---
 net/mptcp/pm_netlink.c   | 30 +++++++++++++-----------------
 net/mptcp/pm_userspace.c | 20 +++++++-------------
 net/mptcp/protocol.h     |  6 ++++--
 6 files changed, 54 insertions(+), 35 deletions(-)

-- 
2.43.0

[PATCH mptcp-next v1 1/6] mptcp: pm: in-kernel: avoid access entry without lock

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

In mptcp_pm_nl_set_flags(), "entry" is copied to "local" when pernet->lock
is held to avoid direct access to entry without pernet->lock.

Therefore, "local->flags" should be passed to mptcp_nl_set_flags instead
of "entry->flags" when pernet->lock is not held, so as to avoid access to
entry.

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index d4328443d844..fb83eba041f1 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1983,7 +1983,7 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 	*local = *entry;
 	spin_unlock_bh(&pernet->lock);
 
-	mptcp_nl_set_flags(net, &local->addr, entry->flags, changed);
+	mptcp_nl_set_flags(net, &local->addr, local->flags, changed);
 	return 0;
 }
 
-- 
2.43.0

[PATCH mptcp-next v1 2/6] mptcp: pm: in-kernel: reduce parameters of set_flags

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The number of parameters in mptcp_nl_set_flags() can be reduced.
Only need to pass a "local" parameter to it instead of "local->addr"
and "local->flags".

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index fb83eba041f1..4bebc4963c42 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1907,11 +1907,12 @@ static void mptcp_pm_nl_fullmesh(struct mptcp_sock *msk,
 	spin_unlock_bh(&msk->pm.lock);
 }
 
-static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
-			       u8 flags, u8 changed)
+static void mptcp_nl_set_flags(struct net *net,
+			       struct mptcp_pm_addr_entry *local,
+			       u8 changed)
 {
-	u8 is_subflow = !!(flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
-	u8 bkup = !!(flags & MPTCP_PM_ADDR_FLAG_BACKUP);
+	u8 is_subflow = !!(local->flags & MPTCP_PM_ADDR_FLAG_SUBFLOW);
+	u8 bkup = !!(local->flags & MPTCP_PM_ADDR_FLAG_BACKUP);
 	long s_slot = 0, s_num = 0;
 	struct mptcp_sock *msk;
 
@@ -1926,10 +1927,10 @@ static void mptcp_nl_set_flags(struct net *net, struct mptcp_addr_info *addr,
 
 		lock_sock(sk);
 		if (changed & MPTCP_PM_ADDR_FLAG_BACKUP)
-			mptcp_pm_nl_mp_prio_send_ack(msk, addr, NULL, bkup);
+			mptcp_pm_nl_mp_prio_send_ack(msk, &local->addr, NULL, bkup);
 		/* Subflows will only be recreated if the SUBFLOW flag is set */
 		if (is_subflow && (changed & MPTCP_PM_ADDR_FLAG_FULLMESH))
-			mptcp_pm_nl_fullmesh(msk, addr);
+			mptcp_pm_nl_fullmesh(msk, &local->addr);
 		release_sock(sk);
 
 next:
@@ -1983,7 +1984,7 @@ int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
 	*local = *entry;
 	spin_unlock_bh(&pernet->lock);
 
-	mptcp_nl_set_flags(net, &local->addr, local->flags, changed);
+	mptcp_nl_set_flags(net, local, changed);
 	return 0;
 }
 
-- 
2.43.0

[PATCH mptcp-next v1 3/6] mptcp: pm: use addr entry for get_local_id

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

The following code in mptcp_userspace_pm_get_local_id() that assigns "skc"
to "new_entry" is not allowed in BPF if we use the same code to implement
the get_local_id() interface of a BFP path manager:

	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
	new_entry.addr = *skc;
	new_entry.addr.id = 0;
	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;

To solve the issue, this patch moves this assignment to "new_entry" forward
to mptcp_pm_get_local_id(), and then passing "new_entry" as a parameter to
both mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id().

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

diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 16cacce6c10f..ac7b39148bd3 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -403,7 +403,7 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 
 int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 {
-	struct mptcp_addr_info skc_local;
+	struct mptcp_pm_addr_entry skc_local = { 0 };
 	struct mptcp_addr_info msk_local;
 
 	if (WARN_ON_ONCE(!msk))
@@ -413,10 +413,13 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
 	 * addr
 	 */
 	mptcp_local_address((struct sock_common *)msk, &msk_local);
-	mptcp_local_address((struct sock_common *)skc, &skc_local);
-	if (mptcp_addresses_equal(&msk_local, &skc_local, false))
+	mptcp_local_address((struct sock_common *)skc, &skc_local.addr);
+	if (mptcp_addresses_equal(&msk_local, &skc_local.addr, false))
 		return 0;
 
+	skc_local.addr.id = 0;
+	skc_local.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
+
 	if (mptcp_pm_is_userspace(msk))
 		return mptcp_userspace_pm_get_local_id(msk, &skc_local);
 	return mptcp_pm_nl_get_local_id(msk, &skc_local);
diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 4bebc4963c42..033cba59023f 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1139,7 +1139,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
 	return err;
 }
 
-int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc)
+int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
+			     struct mptcp_pm_addr_entry *skc)
 {
 	struct mptcp_pm_addr_entry *entry;
 	struct pm_nl_pernet *pernet;
@@ -1148,7 +1149,7 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	pernet = pm_nl_get_pernet_from_msk(msk);
 
 	rcu_read_lock();
-	entry = __lookup_addr(pernet, skc);
+	entry = __lookup_addr(pernet, &skc->addr);
 	ret = entry ? entry->addr.id : -1;
 	rcu_read_unlock();
 	if (ret >= 0)
@@ -1159,12 +1160,8 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk, struct mptcp_addr_info *skc
 	if (!entry)
 		return -ENOMEM;
 
-	entry->addr = *skc;
-	entry->addr.id = 0;
+	*entry = *skc;
 	entry->addr.port = 0;
-	entry->ifindex = 0;
-	entry->flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
-	entry->lsk = NULL;
 	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry, true);
 	if (ret < 0)
 		kfree(entry);
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 6bf6a20ef7f3..5b3ee43130be 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -131,27 +131,22 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
 }
 
 int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
-				    struct mptcp_addr_info *skc)
+				    struct mptcp_pm_addr_entry *skc)
 {
-	struct mptcp_pm_addr_entry *entry = NULL, new_entry;
 	__be16 msk_sport =  ((struct inet_sock *)
 			     inet_sk((struct sock *)msk))->inet_sport;
+	struct mptcp_pm_addr_entry *entry;
 
 	spin_lock_bh(&msk->pm.lock);
-	entry = mptcp_userspace_pm_lookup_addr(msk, skc);
+	entry = mptcp_userspace_pm_lookup_addr(msk, &skc->addr);
 	spin_unlock_bh(&msk->pm.lock);
 	if (entry)
 		return entry->addr.id;
 
-	memset(&new_entry, 0, sizeof(struct mptcp_pm_addr_entry));
-	new_entry.addr = *skc;
-	new_entry.addr.id = 0;
-	new_entry.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
-
-	if (new_entry.addr.port == msk_sport)
-		new_entry.addr.port = 0;
+	if (skc->addr.port == msk_sport)
+		skc->addr.port = 0;
 
-	return mptcp_userspace_pm_append_new_local_addr(msk, &new_entry, true);
+	return mptcp_userspace_pm_append_new_local_addr(msk, skc, true);
 }
 
 bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3d72ca155322..ef1d43406f9b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -1126,8 +1126,10 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, const struct sk_buff *skb,
 bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
 			     struct mptcp_rm_list *rm_list);
 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);
+int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
+			     struct mptcp_pm_addr_entry *skc);
+int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
+				    struct mptcp_pm_addr_entry *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);
-- 
2.43.0

[PATCH mptcp-next v1 4/6] mptcp: pm: in-kernel: use kmemdup helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Instead of using kmalloc() or kzalloc() to allocate an entry and
then immediately duplicate another entry to the newly allocated
one, kmemdup() helper can be used to simplify the code.

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

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 033cba59023f..ee0cd92865cc 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1156,11 +1156,10 @@ int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
 		return ret;
 
 	/* address not found, add to local list */
-	entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
+	entry = kmemdup(skc, sizeof(*skc), GFP_ATOMIC);
 	if (!entry)
 		return -ENOMEM;
 
-	*entry = *skc;
 	entry->addr.port = 0;
 	ret = mptcp_pm_nl_append_new_local_addr(pernet, entry, true);
 	if (ret < 0)
@@ -1422,13 +1421,12 @@ int mptcp_pm_nl_add_addr_doit(struct sk_buff *skb, struct genl_info *info)
 		return -EINVAL;
 	}
 
-	entry = kzalloc(sizeof(*entry), GFP_KERNEL_ACCOUNT);
+	entry = kmemdup(&addr, sizeof(addr), GFP_KERNEL_ACCOUNT);
 	if (!entry) {
 		GENL_SET_ERR_MSG(info, "can't allocate addr");
 		return -ENOMEM;
 	}
 
-	*entry = addr;
 	if (entry->addr.port) {
 		ret = mptcp_pm_nl_create_listen_socket(skb->sk, entry);
 		if (ret) {
-- 
2.43.0

[PATCH mptcp-next v1 5/6] sock: add sock_kmemdup helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds the sock version of kmemdup() helper, named sock_kmemdup(),
to duplicate a memory block using the socket's option memory buffer.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 include/net/sock.h |  1 +
 net/core/sock.c    | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/net/sock.h b/include/net/sock.h
index edbb870e3f86..ffd757e7e329 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1793,6 +1793,7 @@ static inline struct sk_buff *sock_alloc_send_skb(struct sock *sk,
 }
 
 void *sock_kmalloc(struct sock *sk, int size, gfp_t priority);
+void *sock_kmemdup(struct sock *sk, const void *src, int size, gfp_t priority);
 void sock_kfree_s(struct sock *sk, void *mem, int size);
 void sock_kzfree_s(struct sock *sk, void *mem, int size);
 void sk_send_sigurg(struct sock *sk);
diff --git a/net/core/sock.c b/net/core/sock.c
index 0d385bf27b38..d09bd697c120 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2805,6 +2805,29 @@ void *sock_kmalloc(struct sock *sk, int size, gfp_t priority)
 }
 EXPORT_SYMBOL(sock_kmalloc);
 
+/*
+ * Duplicate a memory block using the socket's option memory buffer.
+ */
+void *sock_kmemdup(struct sock *sk, const void *src, int size, gfp_t priority)
+{
+	int optmem_max = READ_ONCE(sock_net(sk)->core.sysctl_optmem_max);
+
+	if ((unsigned int)size <= optmem_max &&
+	    atomic_read(&sk->sk_omem_alloc) + size < optmem_max) {
+		void *mem;
+		/* First do the add, to avoid the race if kmalloc
+		 * might sleep.
+		 */
+		atomic_add(size, &sk->sk_omem_alloc);
+		mem = kmemdup(src, size, priority);
+		if (mem)
+			return mem;
+		atomic_sub(size, &sk->sk_omem_alloc);
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(sock_kmemdup);
+
 /* Free an option memory block. Note, we actually want the inline
  * here as this allows gcc to detect the nullify and fold away the
  * condition entirely.
-- 
2.43.0

[PATCH mptcp-next v1 6/6] mptcp: pm: userspace: use sock_kmemdup helper

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Instead of using sock_kmalloc() to allocate an entry and then
immediately duplicate another entry to the newly allocated one,
sock_kmemdup() helper can be used to simplify the code.

More importantly, the code "*e = *entry;" that assigns "entry"
to "e" is not easy to implemented in BPF if we use the same code
to implement an append_new_local_addr() helper of a BFP path
manager. This patch avoids this type of memory assignment
operation.

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

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 5b3ee43130be..8c45eebe9bbc 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -71,13 +71,12 @@ static int mptcp_userspace_pm_append_new_local_addr(struct mptcp_sock *msk,
 		/* Memory for the entry is allocated from the
 		 * sock option buffer.
 		 */
-		e = sock_kmalloc(sk, sizeof(*e), GFP_ATOMIC);
+		e = sock_kmemdup(sk, entry, sizeof(*entry), GFP_ATOMIC);
 		if (!e) {
 			ret = -ENOMEM;
 			goto append_err;
 		}
 
-		*e = *entry;
 		if (!e->addr.id && needs_id)
 			e->addr.id = find_next_zero_bit(id_bitmap,
 							MPTCP_PM_MAX_ADDR_ID + 1,
-- 
2.43.0

Re: [PATCH mptcp-next v1 1/6] mptcp: pm: in-kernel: avoid access entry without lock

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 09:13, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> In mptcp_pm_nl_set_flags(), "entry" is copied to "local" when pernet->lock
> is held to avoid direct access to entry without pernet->lock.
> 
> Therefore, "local->flags" should be passed to mptcp_nl_set_flags instead
> of "entry->flags" when pernet->lock is not held, so as to avoid access to
> entry.

Good catch! I see that this is a fix for a patch that has been sent to
net-next, but not applied yet.

Fixes: TODO ("mptcp: pm: change to fullmesh only for 'subflow'")

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

Re: [PATCH mptcp-next v1 5/6] sock: add sock_kmemdup helper

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 09:13, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds the sock version of kmemdup() helper, named sock_kmemdup(),
> to duplicate a memory block using the socket's option memory buffer.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  include/net/sock.h |  1 +
>  net/core/sock.c    | 23 +++++++++++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/include/net/sock.h b/include/net/sock.h
> index edbb870e3f86..ffd757e7e329 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1793,6 +1793,7 @@ static inline struct sk_buff *sock_alloc_send_skb(struct sock *sk,
>  }
>  
>  void *sock_kmalloc(struct sock *sk, int size, gfp_t priority);
> +void *sock_kmemdup(struct sock *sk, const void *src, int size, gfp_t priority);
>  void sock_kfree_s(struct sock *sk, void *mem, int size);
>  void sock_kzfree_s(struct sock *sk, void *mem, int size);
>  void sk_send_sigurg(struct sock *sk);
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 0d385bf27b38..d09bd697c120 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2805,6 +2805,29 @@ void *sock_kmalloc(struct sock *sk, int size, gfp_t priority)
>  }
>  EXPORT_SYMBOL(sock_kmalloc);
>  
> +/*
> + * Duplicate a memory block using the socket's option memory buffer.
> + */
> +void *sock_kmemdup(struct sock *sk, const void *src, int size, gfp_t priority)
> +{
> +	int optmem_max = READ_ONCE(sock_net(sk)->core.sysctl_optmem_max);
> +
> +	if ((unsigned int)size <= optmem_max &&
> +	    atomic_read(&sk->sk_omem_alloc) + size < optmem_max) {
> +		void *mem;
> +		/* First do the add, to avoid the race if kmalloc
> +		 * might sleep.
> +		 */
> +		atomic_add(size, &sk->sk_omem_alloc);
> +		mem = kmemdup(src, size, priority);
> +		if (mem)
> +			return mem;
> +		atomic_sub(size, &sk->sk_omem_alloc);

I'm not really convinced by this helper: it is a duplication of
sock_kmalloc(), and it is only used once in MPTCP code.

Calling sock_kmalloc() + memset, and using this new helper in different
places in the net code might help. But still, I don't know if this would
be accepted, it is only saving one line (plus memcpy() will be used when
it is not needed, same for the previous patch at the end).

If you still want to propose that, I suggest sending a dedicated series
to netdev, not to block MPTCP (only) patches. WDYT?

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

Re: [PATCH mptcp-next v1 0/6] BPF path manager, part 4

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

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


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 v1 5/6] sock: add sock_kmemdup helper

[Geliang Tang]

Hi Matt,

Thanks for the review.

On Mon, 2025-02-24 at 09:54 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 24/02/2025 09:13, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > This patch adds the sock version of kmemdup() helper, named
> > sock_kmemdup(),
> > to duplicate a memory block using the socket's option memory
> > buffer.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  include/net/sock.h |  1 +
> >  net/core/sock.c    | 23 +++++++++++++++++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/include/net/sock.h b/include/net/sock.h
> > index edbb870e3f86..ffd757e7e329 100644
> > --- a/include/net/sock.h
> > +++ b/include/net/sock.h
> > @@ -1793,6 +1793,7 @@ static inline struct sk_buff
> > *sock_alloc_send_skb(struct sock *sk,
> >  }
> >  
> >  void *sock_kmalloc(struct sock *sk, int size, gfp_t priority);
> > +void *sock_kmemdup(struct sock *sk, const void *src, int size,
> > gfp_t priority);
> >  void sock_kfree_s(struct sock *sk, void *mem, int size);
> >  void sock_kzfree_s(struct sock *sk, void *mem, int size);
> >  void sk_send_sigurg(struct sock *sk);
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index 0d385bf27b38..d09bd697c120 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -2805,6 +2805,29 @@ void *sock_kmalloc(struct sock *sk, int
> > size, gfp_t priority)
> >  }
> >  EXPORT_SYMBOL(sock_kmalloc);
> >  
> > +/*
> > + * Duplicate a memory block using the socket's option memory
> > buffer.
> > + */
> > +void *sock_kmemdup(struct sock *sk, const void *src, int size,
> > gfp_t priority)
> > +{
> > + int optmem_max = READ_ONCE(sock_net(sk)->core.sysctl_optmem_max);
> > +
> > + if ((unsigned int)size <= optmem_max &&
> > +     atomic_read(&sk->sk_omem_alloc) + size < optmem_max) {
> > + void *mem;
> > + /* First do the add, to avoid the race if kmalloc
> > + * might sleep.
> > + */
> > + atomic_add(size, &sk->sk_omem_alloc);
> > + mem = kmemdup(src, size, priority);

I made some updates to this patch here.

      mem = src ? kmemdup(src, size, priority) :
                  kmalloc(size, priority);

> > + if (mem)
> > + return mem;
> > + atomic_sub(size, &sk->sk_omem_alloc);

Then sock_kmalloc() can be implemented through sock_kmemdup(), which
can reduce the duplication of code between them.

void *sock_kmalloc(struct sock *sk, int size, gfp_t priority)
{
      return sock_kmemdup(sk, NULL, size, priority);
}

> 
> I'm not really convinced by this helper: it is a duplication of
> sock_kmalloc(), and it is only used once in MPTCP code.

I found that this new helper can also be used in
mptcp_copy_ip_options() too.

> 
> Calling sock_kmalloc() + memset, and using this new helper in
> different
> places in the net code might help. But still, I don't know if this

And there are three other places in the net code where it can be used:

	ipv6_dup_options()
	sctp_v4_copy_ip_options()
	tcp_ao_copy_key()

That way, this helper can be used in five places.

> would
> be accepted, it is only saving one line (plus memcpy() will be used
> when
> it is not needed, same for the previous patch at the end).

What do you think of this new version?

> 
> If you still want to propose that, I suggest sending a dedicated
> series
> to netdev, not to block MPTCP (only) patches. WDYT?

Please remove the last two patches from this set if others are ready to
apply.

Thanks,
-Geliang

> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v1 5/6] sock: add sock_kmemdup helper

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 11:42, Geliang Tang wrote:
> Hi Matt,
> 
> Thanks for the review.
> 
> On Mon, 2025-02-24 at 09:54 +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 24/02/2025 09:13, Geliang Tang wrote:
>>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>>
>>> This patch adds the sock version of kmemdup() helper, named
>>> sock_kmemdup(),
>>> to duplicate a memory block using the socket's option memory
>>> buffer.
>>>
>>> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
>>> ---
>>>  include/net/sock.h |  1 +
>>>  net/core/sock.c    | 23 +++++++++++++++++++++++
>>>  2 files changed, 24 insertions(+)
>>>
>>> diff --git a/include/net/sock.h b/include/net/sock.h
>>> index edbb870e3f86..ffd757e7e329 100644
>>> --- a/include/net/sock.h
>>> +++ b/include/net/sock.h
>>> @@ -1793,6 +1793,7 @@ static inline struct sk_buff
>>> *sock_alloc_send_skb(struct sock *sk,
>>>  }
>>>  
>>>  void *sock_kmalloc(struct sock *sk, int size, gfp_t priority);
>>> +void *sock_kmemdup(struct sock *sk, const void *src, int size,
>>> gfp_t priority);
>>>  void sock_kfree_s(struct sock *sk, void *mem, int size);
>>>  void sock_kzfree_s(struct sock *sk, void *mem, int size);
>>>  void sk_send_sigurg(struct sock *sk);
>>> diff --git a/net/core/sock.c b/net/core/sock.c
>>> index 0d385bf27b38..d09bd697c120 100644
>>> --- a/net/core/sock.c
>>> +++ b/net/core/sock.c
>>> @@ -2805,6 +2805,29 @@ void *sock_kmalloc(struct sock *sk, int
>>> size, gfp_t priority)
>>>  }
>>>  EXPORT_SYMBOL(sock_kmalloc);
>>>  
>>> +/*
>>> + * Duplicate a memory block using the socket's option memory
>>> buffer.
>>> + */
>>> +void *sock_kmemdup(struct sock *sk, const void *src, int size,
>>> gfp_t priority)
>>> +{
>>> + int optmem_max = READ_ONCE(sock_net(sk)->core.sysctl_optmem_max);
>>> +
>>> + if ((unsigned int)size <= optmem_max &&
>>> +     atomic_read(&sk->sk_omem_alloc) + size < optmem_max) {
>>> + void *mem;
>>> + /* First do the add, to avoid the race if kmalloc
>>> + * might sleep.
>>> + */
>>> + atomic_add(size, &sk->sk_omem_alloc);
>>> + mem = kmemdup(src, size, priority);
> 
> I made some updates to this patch here.
> 
>       mem = src ? kmemdup(src, size, priority) :
>                   kmalloc(size, priority);
> 
>>> + if (mem)
>>> + return mem;
>>> + atomic_sub(size, &sk->sk_omem_alloc);
> 
> Then sock_kmalloc() can be implemented through sock_kmemdup(), which
> can reduce the duplication of code between them.
> 
> void *sock_kmalloc(struct sock *sk, int size, gfp_t priority)
> {
>       return sock_kmemdup(sk, NULL, size, priority);
> }

Yes, looks good.

I was thinking about not modifying sock_kmalloc() by calling it from
sock_kmemdup(), then calling memcpy(). So similar to what kmemdup() is
doing. But your version looks good.

>> I'm not really convinced by this helper: it is a duplication of
>> sock_kmalloc(), and it is only used once in MPTCP code.
> 
> I found that this new helper can also be used in
> mptcp_copy_ip_options() too.

Indeed.

>> Calling sock_kmalloc() + memset, and using this new helper in
>> different
>> places in the net code might help. But still, I don't know if this
> 
> And there are three other places in the net code where it can be used:
> 
> 	ipv6_dup_options()
> 	sctp_v4_copy_ip_options()
> 	tcp_ao_copy_key()
> 
> That way, this helper can be used in five places.

Indeed, that's what I saw when I quickly looked.

>> would
>> be accepted, it is only saving one line (plus memcpy() will be used
>> when
>> it is not needed, same for the previous patch at the end).
> 
> What do you think of this new version?

Yes, it might be OK, but I don't know if that kind of cleanup would be
accepted by netdev maintainers. Then, do you mind sending a dedicated
patch (or series) introducing this new helper and using it in the
different places to the netdev ML with the appropriated reviewers added
in cc, please?

>> If you still want to propose that, I suggest sending a dedicated
>> series
>> to netdev, not to block MPTCP (only) patches. WDYT?
> 
> Please remove the last two patches from this set if others are ready to
> apply.

Yes, I can do that.

I will not apply patch 4/6 as well, because the modification is similar.
I will first wait to see what kind of feedback the netdev maintainers
will give.

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

Re: [PATCH mptcp-next v1 1/6] mptcp: pm: in-kernel: avoid access entry without lock

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 09:31, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 24/02/2025 09:13, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> In mptcp_pm_nl_set_flags(), "entry" is copied to "local" when pernet->lock
>> is held to avoid direct access to entry without pernet->lock.
>>
>> Therefore, "local->flags" should be passed to mptcp_nl_set_flags instead
>> of "entry->flags" when pernet->lock is not held, so as to avoid access to
>> entry.
> 
> Good catch! I see that this is a fix for a patch that has been sent to
> net-next, but not applied yet.
> 
> Fixes: TODO ("mptcp: pm: change to fullmesh only for 'subflow'")

I will mark this patch and the two next ones as queued, and I will apply
them when the above patch will be applied in netdev, so I can add the
appropriated SHA.

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

Re: [PATCH mptcp-next v1 0/6] BPF path manager, part 4

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 09:13, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Some path manager related refactoring and cleanups.
> 
> Geliang Tang (6):
>   mptcp: pm: in-kernel: avoid access entry without lock
>   mptcp: pm: in-kernel: reduce parameters of set_flags
>   mptcp: pm: use addr entry for get_local_id

For these 3 patches above:

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

>   mptcp: pm: in-kernel: use kmemdup helper
>   sock: add sock_kmemdup helper
>   mptcp: pm: userspace: use sock_kmemdup helper
> 
>  include/net/sock.h       |  1 +
>  net/core/sock.c          | 23 +++++++++++++++++++++++
>  net/mptcp/pm.c           |  9 ++++++---
>  net/mptcp/pm_netlink.c   | 30 +++++++++++++-----------------
>  net/mptcp/pm_userspace.c | 20 +++++++-------------
>  net/mptcp/protocol.h     |  6 ++++--
>  6 files changed, 54 insertions(+), 35 deletions(-)
Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v1 0/6] BPF path manager, part 4

[Matthieu Baerts]

Hi Geliang,

On 24/02/2025 12:05, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 24/02/2025 09:13, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> Some path manager related refactoring and cleanups.
>>
>> Geliang Tang (6):
>>   mptcp: pm: in-kernel: avoid access entry without lock
>>   mptcp: pm: in-kernel: reduce parameters of set_flags
>>   mptcp: pm: use addr entry for get_local_id
> 
> For these 3 patches above:
> 
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

These 3 patches are now in our tree (feat. for next), with the proper
SHA in the Fixes tag for the first patch:

New patches for t/upstream:
- c8a2f8c2afac: mptcp: pm: in-kernel: avoid access entry without lock
- 91606b7d16da: mptcp: pm: in-kernel: reduce parameters of set_flags
- 29b6fd0cf693: mptcp: pm: use addr entry for get_local_id
- Results: 1238896935ea..3635c12a532c (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/100f5ffc0c317c10656b1397607f52d95d0d8911/checks

Note that there was a small conflict with "mptcp: fix 'scheduling while
atomic' in mptcp_pm_nl_append_new_local_addr".

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