[mptcp-next,v1,2/4] bpf: Export mptcp path manager kfuncs

Commit Message

Geliang Tang March 21, 2025, 1:49 a.m. UTC

From: Geliang Tang <[e-mail]>

This patch exports mptcp path manager helpers into BPF, adds these
kfunc names into mptcp common kfunc_set.

bpf_kmemdup_entry() and bpf_kfree_entry() are wrappers of kmemdup() and
kfree(), using to alloc and free an mptcp address entry.

bpf_set_bit() and bpf_bitmap_fill() are wrappers of __set_bit() and
bitmap_fill(), using for mptcp address ID bitmap.

bpf_spin_lock_bh() and bpf_spin_unlock_bh() are wrappers of spin_lock_bh()
and spin_unlock_bh(), using to lock and unlock the mptcp pm lock.

Signed-off-by: Geliang Tang <[e-mail]>
---
 net/mptcp/bpf.c       | 48 +++++++++++++++++++++++++++++++++++++++++++
 net/mptcp/pm_kernel.c | 27 ++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

Comments

Matthieu Baerts March 21, 2025, 11:11 a.m. UTC | #1

Hi Geliang,

On 21/03/2025 02:49, Geliang Tang wrote:
> From: Geliang Tang <[e-mail]>
> 
> This patch exports mptcp path manager helpers into BPF, adds these
> kfunc names into mptcp common kfunc_set.
> 
> bpf_kmemdup_entry() and bpf_kfree_entry() are wrappers of kmemdup() and
> kfree(), using to alloc and free an mptcp address entry.

That feels really wrong: a BPF program cannot crash or cause problems
(deadlock, memleaks, etc.), it should then not be able to reserve memory
except in a map or something that can be automatically freed when the
bpf program is removed.

> bpf_set_bit() and bpf_bitmap_fill() are wrappers of __set_bit() and
> bitmap_fill(), using for mptcp address ID bitmap.

Is it really needed? Are there not already some  it not store stuff in a
BPF map?

> bpf_spin_lock_bh() and bpf_spin_unlock_bh() are wrappers of spin_lock_bh()
> and spin_unlock_bh(), using to lock and unlock the mptcp pm lock.

Same here, a BPF should not be able to lock something and never unlock
it. I think there are some stuff in BPF to get a lock automatically.
(Polymorphisme?)

(...)

> @@ -564,6 +596,22 @@ BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock, KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS)
>  BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL)
>  BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY)
> +BTF_ID_FLAGS(func, bpf_kmemdup_entry)
> +BTF_ID_FLAGS(func, bpf_kfree_entry)
> +BTF_ID_FLAGS(func, bpf_set_bit)
> +BTF_ID_FLAGS(func, bpf_bitmap_fill)
> +BTF_ID_FLAGS(func, bpf_spin_lock_bh)
> +BTF_ID_FLAGS(func, bpf_spin_unlock_bh)

This should not be needed, a BPF program should not need them. Not sure
about the bitmap, but for the rest, something else should be used
(maps?) or automatically done (locks).

> +BTF_ID_FLAGS(func, mptcp_pm_nl_lookup_addr)
> +BTF_ID_FLAGS(func, mptcp_pm_nl_append_new_local_addr_msk)

That's specific to the in-kernel PM, that feels wrong. Addresses should
be stored in BPF maps or similar instead I guess.

> +BTF_ID_FLAGS(func, mptcp_pm_get_add_addr_signal_max)
> +BTF_ID_FLAGS(func, mptcp_pm_get_add_addr_accept_max)
> +BTF_ID_FLAGS(func, mptcp_pm_get_subflows_max)
> +BTF_ID_FLAGS(func, mptcp_pm_get_local_addr_max)

That feels wrong, it should not be needed, that's specific to the
in-kernel PM as well and set via netlink.

> +BTF_ID_FLAGS(func, mptcp_pm_add_addr_recv)

Should not be needed, see my comments in part 6.

> +BTF_ID_FLAGS(func, mptcp_pm_is_init_remote_addr)

Should it not be handled by the core (pm.c) or specific to in-kernel? (I
didn't check)

> +BTF_ID_FLAGS(func, mptcp_pm_create_subflow_or_signal_addr)

Specific to the in-kernel PM, that feels wrong.

> +BTF_ID_FLAGS(func, mptcp_pm_rm_addr_recv)

Maybe not needed, see my comments in part 6.

(...)

Cheers,
Matt

Patch

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 596574102b89..e411ae8382f2 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -540,6 +540,38 @@  bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it)
 {
 }
 
+__bpf_kfunc static struct mptcp_pm_addr_entry *
+bpf_kmemdup_entry(struct mptcp_pm_addr_entry *entry, int size, gfp_t priority)
+{
+	return kmemdup(entry, size, priority);
+}
+
+__bpf_kfunc static void
+bpf_kfree_entry(struct mptcp_pm_addr_entry *entry)
+{
+	kfree(entry);
+}
+
+__bpf_kfunc static void bpf_set_bit(unsigned long nr, unsigned long *addr__ign)
+{
+	__set_bit(nr, addr__ign);
+}
+
+__bpf_kfunc static void bpf_bitmap_fill(unsigned long *dst__ign, unsigned int nbits)
+{
+	bitmap_fill(dst__ign, nbits);
+}
+
+__bpf_kfunc static void bpf_spin_lock_bh(spinlock_t *lock)
+{
+	spin_lock_bh(lock);
+}
+
+__bpf_kfunc static void bpf_spin_unlock_bh(spinlock_t *lock)
+{
+	spin_unlock_bh(lock);
+}
+
 __bpf_kfunc static bool bpf_mptcp_subflow_queues_empty(struct sock *sk)
 {
 	return tcp_rtx_queue_empty(sk);
@@ -564,6 +596,22 @@  BTF_ID_FLAGS(func, bpf_mptcp_subflow_tcp_sock, KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new, KF_ITER_NEW | KF_TRUSTED_ARGS)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next, KF_ITER_NEXT | KF_RET_NULL)
 BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy, KF_ITER_DESTROY)
+BTF_ID_FLAGS(func, bpf_kmemdup_entry)
+BTF_ID_FLAGS(func, bpf_kfree_entry)
+BTF_ID_FLAGS(func, bpf_set_bit)
+BTF_ID_FLAGS(func, bpf_bitmap_fill)
+BTF_ID_FLAGS(func, bpf_spin_lock_bh)
+BTF_ID_FLAGS(func, bpf_spin_unlock_bh)
+BTF_ID_FLAGS(func, mptcp_pm_nl_lookup_addr)
+BTF_ID_FLAGS(func, mptcp_pm_nl_append_new_local_addr_msk)
+BTF_ID_FLAGS(func, mptcp_pm_get_add_addr_signal_max)
+BTF_ID_FLAGS(func, mptcp_pm_get_add_addr_accept_max)
+BTF_ID_FLAGS(func, mptcp_pm_get_subflows_max)
+BTF_ID_FLAGS(func, mptcp_pm_get_local_addr_max)
+BTF_ID_FLAGS(func, mptcp_pm_add_addr_recv)
+BTF_ID_FLAGS(func, mptcp_pm_is_init_remote_addr)
+BTF_ID_FLAGS(func, mptcp_pm_create_subflow_or_signal_addr)
+BTF_ID_FLAGS(func, mptcp_pm_rm_addr_recv)
 BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
 BTF_ID_FLAGS(func, mptcp_subflow_active)
 BTF_ID_FLAGS(func, mptcp_set_timeout)
diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
index 4f7b2e0e998d..3cf81986c70d 100644
--- a/net/mptcp/pm_kernel.c
+++ b/net/mptcp/pm_kernel.c
@@ -253,6 +253,9 @@  __lookup_addr(struct pm_nl_pernet *pernet, const struct mptcp_addr_info *info)
 	return NULL;
 }
 
+__bpf_kfunc_start_defs();
+
+__bpf_kfunc
 static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 {
 	struct sock *sk = (struct sock *)msk;
@@ -367,6 +370,8 @@  static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 	mptcp_pm_nl_check_work_pending(msk);
 }
 
+__bpf_kfunc_end_defs();
+
 static void mptcp_pm_kernel_established(struct mptcp_sock *msk)
 {
 	spin_lock_bh(&msk->pm.lock);
@@ -1493,3 +1498,25 @@  void __init mptcp_pm_kernel_register(void)
 
 	mptcp_pm_register(&mptcp_pm_kernel);
 }
+
+__bpf_kfunc_start_defs();
+
+__bpf_kfunc static struct mptcp_pm_addr_entry *
+mptcp_pm_nl_lookup_addr(struct mptcp_sock *msk, const struct mptcp_addr_info *info)
+{
+	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+
+	return __lookup_addr(pernet, info);
+}
+
+__bpf_kfunc static int
+mptcp_pm_nl_append_new_local_addr_msk(struct mptcp_sock *msk,
+				      struct mptcp_pm_addr_entry *entry,
+				      bool needs_id, bool replace)
+{
+	struct pm_nl_pernet *pernet = pm_nl_get_pernet_from_msk(msk);
+
+	return mptcp_pm_nl_append_new_local_addr(pernet, entry, needs_id, replace);
+}
+
+__bpf_kfunc_end_defs();