[PATCH mptcp-next v3 0/5] add mptcp_subflow bpf_iter

[PATCH mptcp-next v3 0/5] add mptcp_subflow bpf_iter

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

v3:
 - drop bpf_iter__mptcp_subflow, __diag_push, __diag_pop and
 __diag_ignore_all
 - drop declarations for bpf kfuncs

v2:
 - update patch 1 as Martin and Andrii suggested.
 - fix warnings and errors reported by MPTCP CI.

This patch set adds a mptcp_subflow type bpf_iter, and self tests.

Geliang Tang (5):
  bpf: Add mptcp_subflow bpf_iter
  Squash to "mptcp: add sched_data helpers"
  bpf: Register mptcp tracing kfunc set
  selftests/bpf: Add mptcp_subflow bpf_iter test prog
  selftests/bpf: Add mptcp_subflow bpf_iter subtest

 net/mptcp/bpf.c                               | 54 ++++++++++--
 net/mptcp/protocol.h                          |  2 -
 .../testing/selftests/bpf/bpf_experimental.h  |  7 ++
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 85 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/mptcp_bpf.h |  4 +
 .../selftests/bpf/progs/mptcp_bpf_iter.c      | 38 +++++++++
 6 files changed, 183 insertions(+), 7 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_iter.c

-- 
2.43.0

[PATCH mptcp-next v3 1/5] bpf: Add mptcp_subflow bpf_iter

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

It's necessary to traverse all subflows on the conn_list of an MPTCP
socket and then call kfunc to modify the fields of each subflow. In
kernel space, mptcp_for_each_subflow() helper is used for this:

	mptcp_for_each_subflow(msk, subflow)
		kfunc(subflow);

But in the MPTCP BPF program, this has not yet been implemented. As
Martin suggested recently, this conn_list walking + modify-by-kfunc
usage fits the bpf_iter use case. So this patch adds a new bpf_iter
type named "mptcp_subflow" to do this and implements its helpers
bpf_iter_mptcp_subflow_new()/_next()/_destroy().

Then bpf_for_each() for mptcp_subflow can be used in BPF program like
this:

	bpf_rcu_read_lock();
	bpf_for_each(mptcp_subflow, subflow, msk)
		kfunc(subflow);
	bpf_rcu_read_unlock();

Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/bpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 4 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 6414824402e6..350672e24a3d 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -201,9 +201,48 @@ static const struct btf_kfunc_id_set bpf_mptcp_fmodret_set = {
 	.set   = &bpf_mptcp_fmodret_ids,
 };
 
-__diag_push();
-__diag_ignore_all("-Wmissing-prototypes",
-		  "kfuncs which will be used in BPF programs");
+struct bpf_iter_mptcp_subflow {
+	__u64 __opaque[2];
+} __attribute__((aligned(8)));
+
+struct bpf_iter_mptcp_subflow_kern {
+	struct mptcp_sock *msk;
+	struct list_head *pos;
+} __attribute__((aligned(8)));
+
+__bpf_kfunc_start_defs();
+
+__bpf_kfunc int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
+					   struct mptcp_sock *msk)
+{
+	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
+
+	if (!msk)
+		return -EINVAL;
+
+	kit->msk = msk;
+	kit->pos = &msk->conn_list;
+	return 0;
+}
+
+__bpf_kfunc struct mptcp_subflow_context *
+bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it)
+{
+	struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
+	struct mptcp_subflow_context *subflow;
+	struct mptcp_sock *msk = kit->msk;
+
+	subflow = list_entry((kit->pos)->next, struct mptcp_subflow_context, node);
+	if (!msk || list_entry_is_head(subflow, &msk->conn_list, node))
+		return NULL;
+
+	kit->pos = &subflow->node;
+	return subflow;
+}
+
+__bpf_kfunc void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it)
+{
+}
 
 __bpf_kfunc struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
@@ -218,7 +257,7 @@ __bpf_kfunc bool bpf_mptcp_subflow_queues_empty(struct sock *sk)
 	return tcp_rtx_queue_empty(sk);
 }
 
-__diag_pop();
+__bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(bpf_mptcp_sched_kfunc_ids)
 BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
-- 
2.43.0

[PATCH mptcp-next v3 2/5] Squash to "mptcp: add sched_data helpers"

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Drop bpf_mptcp_subflow_ctx_by_pos declaration, since
"-Wmissing-declarations" is ignored in __bpf_kfunc_start_defs.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/protocol.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 6e7d88ca7d42..aa0969d4440b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -722,8 +722,6 @@ void mptcp_sock_graft(struct sock *sk, struct socket *parent);
 u64 mptcp_wnd_end(const struct mptcp_sock *msk);
 void mptcp_set_timeout(struct sock *sk);
 bool bpf_mptcp_subflow_queues_empty(struct sock *sk);
-struct mptcp_subflow_context *
-bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos);
 struct sock *__mptcp_nmpc_sk(struct mptcp_sock *msk);
 bool __mptcp_close(struct sock *sk, long timeout);
 void mptcp_cancel_work(struct sock *sk);
-- 
2.43.0

[PATCH mptcp-next v3 3/5] bpf: Register mptcp tracing kfunc set

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

Since the kfuncs mptcp_subflow_active() and mptcp_subflow_set_scheduled()
are invoked in the mptcp_subflow bpf_iter test in a ftrace hook for
mptcp_sched_get_send(), it's necessary to register them into a
BPF_PROG_TYPE_TRACING type kfunc set together with the bpf_iter
mptcp_subflow helpers bpf_iter_mptcp_subflow_new()/_next()/_destroy().

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

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 350672e24a3d..0b7a0c2634c4 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -260,9 +260,12 @@ __bpf_kfunc bool bpf_mptcp_subflow_queues_empty(struct sock *sk)
 __bpf_kfunc_end_defs();
 
 BTF_KFUNCS_START(bpf_mptcp_sched_kfunc_ids)
+BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_new)
+BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_next)
+BTF_ID_FLAGS(func, bpf_iter_mptcp_subflow_destroy)
+BTF_ID_FLAGS(func, mptcp_subflow_active)
 BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
 BTF_ID_FLAGS(func, bpf_mptcp_subflow_ctx_by_pos)
-BTF_ID_FLAGS(func, mptcp_subflow_active)
 BTF_ID_FLAGS(func, mptcp_set_timeout)
 BTF_ID_FLAGS(func, mptcp_wnd_end)
 BTF_ID_FLAGS(func, tcp_stream_memory_free)
@@ -280,6 +283,8 @@ static int __init bpf_mptcp_kfunc_init(void)
 	int ret;
 
 	ret = register_btf_fmodret_id_set(&bpf_mptcp_fmodret_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING,
+					       &bpf_mptcp_sched_kfunc_set);
 	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS,
 					       &bpf_mptcp_sched_kfunc_set);
 #ifdef CONFIG_BPF_JIT
-- 
2.43.0

[PATCH mptcp-next v3 4/5] selftests/bpf: Add mptcp_subflow bpf_iter test prog

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a new mptcp bpf selftest program for the newly added
mptcp_subflow bpf_iter.

Export bpf_iter_mptcp_subflow_new/_next/_destroy into bpf_experimental.h
then use bpf_for_each(mptcp_subflow, subflow, msk) to walk the mptcp
subflow list.

Add a ftrace hook for mptcp_sched_get_send() to do this test and invoke
kfuncs mptcp_subflow_active() and mptcp_subflow_set_scheduled() in the
loops.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/bpf/bpf_experimental.h  |  7 ++++
 tools/testing/selftests/bpf/progs/mptcp_bpf.h |  4 ++
 .../selftests/bpf/progs/mptcp_bpf_iter.c      | 38 +++++++++++++++++++
 3 files changed, 49 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/mptcp_bpf_iter.c

diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index 828556cdc2f0..97aad95c90c6 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -549,6 +549,13 @@ extern int bpf_iter_css_new(struct bpf_iter_css *it,
 extern struct cgroup_subsys_state *bpf_iter_css_next(struct bpf_iter_css *it) __weak __ksym;
 extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym;
 
+struct bpf_iter_mptcp_subflow;
+extern int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
+				      struct mptcp_sock *msk) __weak __ksym;
+extern struct mptcp_subflow_context *
+bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
+extern void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it) __weak __ksym;
+
 extern int bpf_wq_init(struct bpf_wq *wq, void *p__map, unsigned int flags) __weak __ksym;
 extern int bpf_wq_start(struct bpf_wq *wq, unsigned int flags) __weak __ksym;
 extern int bpf_wq_set_callback_impl(struct bpf_wq *wq,
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
index 928a1e5ad8db..3280d4183beb 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
@@ -43,9 +43,13 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
 }
 
 /* ksym */
+extern bool mptcp_subflow_active(struct mptcp_subflow_context *subflow) __ksym;
 extern void mptcp_subflow_set_scheduled(struct mptcp_subflow_context *subflow,
 					bool scheduled) __ksym;
 
+extern void bpf_rcu_read_lock(void) __ksym;
+extern void bpf_rcu_read_unlock(void) __ksym;
+
 extern struct mptcp_subflow_context *
 bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos) __ksym;
 
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iter.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iter.c
new file mode 100644
index 000000000000..03168be3980b
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iter.c
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024, Kylin Software */
+
+/* vmlinux.h, bpf_helpers.h and other 'define' */
+#include "bpf_tracing_net.h"
+#include "mptcp_bpf.h"
+
+char _license[] SEC("license") = "GPL";
+int iter = 0;
+int pid;
+
+SEC("fentry/mptcp_sched_get_send")
+int BPF_PROG(trace_mptcp_sched_get_send, struct mptcp_sock *msk)
+{
+	struct mptcp_subflow_context *subflow;
+	int i = 0;
+
+	if (bpf_get_current_pid_tgid() >> 32 != pid)
+		return 0;
+
+	bpf_rcu_read_lock();
+	bpf_for_each(mptcp_subflow, subflow, msk) {
+		if (i++ >= MPTCP_SUBFLOWS_MAX)
+			break;
+
+		if (subflow->token != msk->token)
+			break;
+
+		if (!mptcp_subflow_active(subflow))
+			continue;
+
+		mptcp_subflow_set_scheduled(subflow, false);
+	}
+	bpf_rcu_read_unlock();
+
+	iter = i;
+	return 0;
+}
-- 
2.43.0

[PATCH mptcp-next v3 5/5] selftests/bpf: Add mptcp_subflow bpf_iter subtest

[Geliang Tang]

From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a subtest named test_bpf_iter to load and verify the newly
added mptcp_subflow type bpf_iter example in test_mptcp. Use endpoint_init()
to add a new subflow endpoint. A new helper recv_send() is invoked to trigger
the ftrace hook for mptcp_sched_get_send() after wait_for_new_subflows().
Check if skel->bss->iter equals 2 to verify whether the ftrace program loops
twice as expected.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 2476f0a083bc..4ecfa6570b85 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -10,6 +10,7 @@
 #include "mptcp_sock.skel.h"
 #include "mptcpify.skel.h"
 #include "mptcp_subflow.skel.h"
+#include "mptcp_bpf_iter.skel.h"
 #include "mptcp_bpf_first.skel.h"
 #include "mptcp_bpf_bkup.skel.h"
 #include "mptcp_bpf_rr.skel.h"
@@ -258,6 +259,34 @@ static void send_byte(int fd)
 	ASSERT_EQ(write(fd, &b, sizeof(b)), 1, "send single byte");
 }
 
+static int recv_send(int server_fd)
+{
+	char buf[1];
+	int ret, fd;
+	ssize_t n;
+
+	fd = accept(server_fd, NULL, NULL);
+	if (!ASSERT_OK_FD(fd, "accept"))
+		return -1;
+
+	n = recv(fd, buf, sizeof(buf), 0);
+	if (!ASSERT_GT(n, 0, "recv")) {
+		ret = -1;
+		goto close;
+	}
+
+	n = send(fd, buf, n, 0);
+	if (!ASSERT_GT(n, 0, "send")) {
+		ret = -1;
+		goto close;
+	}
+
+	ret = 0;
+close:
+	close(fd);
+	return ret;
+}
+
 static int verify_mptcpify(int server_fd, int client_fd)
 {
 	struct __mptcp_info info;
@@ -470,6 +499,60 @@ static void test_subflow(void)
 	close(cgroup_fd);
 }
 
+static void run_bpf_iter(void)
+{
+	int server_fd, client_fd;
+
+	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
+	if (!ASSERT_OK_FD(server_fd, "start_mptcp_server"))
+		return;
+
+	client_fd = connect_to_fd(server_fd, 0);
+	if (!ASSERT_OK_FD(client_fd, "connect_to_fd"))
+		goto close_server;
+
+	send_byte(client_fd);
+	wait_for_new_subflows(client_fd);
+	recv_send(server_fd);
+
+	close(client_fd);
+close_server:
+	close(server_fd);
+}
+
+static void test_bpf_iter(void)
+{
+	struct mptcp_bpf_iter *skel;
+	struct nstoken *nstoken;
+	int err;
+
+	skel = mptcp_bpf_iter__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_load: mptcp_iter"))
+		return;
+
+	skel->bss->pid = getpid();
+
+	err = mptcp_bpf_iter__attach(skel);
+	if (!ASSERT_OK(err, "skel_attach: mptcp_iter"))
+		goto skel_destroy;
+
+	nstoken = create_netns();
+	if (!ASSERT_OK_PTR(nstoken, "create_netns: mptcp_iter"))
+		goto skel_destroy;
+
+	if (endpoint_init("subflow") < 0)
+		goto close_netns;
+
+	run_bpf_iter();
+
+	ASSERT_EQ(skel->bss->iter, 2, "iter");
+
+close_netns:
+	cleanup_netns(nstoken);
+skel_destroy:
+	mptcp_bpf_iter__destroy(skel);
+}
+
 static struct nstoken *sched_init(char *flags, char *sched)
 {
 	struct nstoken *nstoken;
@@ -651,6 +734,8 @@ void test_mptcp(void)
 		test_mptcpify();
 	if (test__start_subtest("subflow"))
 		test_subflow();
+	if (test__start_subtest("bpf_iter"))
+		test_bpf_iter();
 	if (test__start_subtest("default"))
 		test_default();
 	if (test__start_subtest("first"))
-- 
2.43.0

Re: [PATCH mptcp-next v3 0/5] add mptcp_subflow bpf_iter

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

But sadly, our CI spotted some issues with it when trying to build it.

You can find more details there:

  https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10786552828

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

Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.

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

Re: [PATCH mptcp-next v3 0/5] add mptcp_subflow bpf_iter

[Geliang Tang]

On Tue, 2024-09-10 at 06:02 +0000, MPTCP CI wrote:
> Hi Geliang,
> 
> Thank you for your modifications, that's great!
> 
> But sadly, our CI spotted some issues with it when trying to build
> it.
> 
> You can find more details there:
> 
>  
> https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10786552828
> 
> Status: failure

These errors reported by CI should be ignored:

Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
net/mptcp/bpf.c:215:17: warning: symbol 'bpf_iter_mptcp_subflow_new'
was not declared. Should it be static?
Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
net/mptcp/bpf.c:228:42: warning: symbol 'bpf_iter_mptcp_subflow_next'
was not declared. Should it be static?
Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
net/mptcp/bpf.c:243:18: warning: symbol
'bpf_iter_mptcp_subflow_destroy' was not declared. Should it be static?
Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
net/mptcp/bpf.c:247:42: warning: symbol 'bpf_mptcp_subflow_ctx_by_pos'
was not declared. Should it be static?

Thanks,
-Geliang

> Initiator: Patchew Applier
> Commits:
> https://github.com/multipath-tcp/mptcp_net-next/commits/ed6d012aaff4
> Patchwork:
> https://patchwork.kernel.org/project/mptcp/list/?series=888713
> 
> Feel free to reply to this email if you cannot access logs, if you
> need
> some support to fix the error, if this doesn't seem to be caused by
> your
> modifications or if the error is a false positive one.
> 
> Cheers,
> MPTCP GH Action bot
> Bot operated by Matthieu Baerts (NGI0 Core)
> 

Re: [PATCH mptcp-next v3 0/5] add mptcp_subflow bpf_iter

[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 (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10786552827

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


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 0/5] add mptcp_subflow bpf_iter

[Matthieu Baerts]

Hi Geliang,

On 10/09/2024 08:30, Geliang Tang wrote:
> On Tue, 2024-09-10 at 06:02 +0000, MPTCP CI wrote:
>> Hi Geliang,
>>
>> Thank you for your modifications, that's great!
>>
>> But sadly, our CI spotted some issues with it when trying to build
>> it.
>>
>> You can find more details there:
>>
>>  
>> https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10786552828
>>
>> Status: failure
> 
> These errors reported by CI should be ignored:
> 
> Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
> net/mptcp/bpf.c:215:17: warning: symbol 'bpf_iter_mptcp_subflow_new'
> was not declared. Should it be static?
> Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
> net/mptcp/bpf.c:228:42: warning: symbol 'bpf_iter_mptcp_subflow_next'
> was not declared. Should it be static?
> Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
> net/mptcp/bpf.c:243:18: warning: symbol
> 'bpf_iter_mptcp_subflow_destroy' was not declared. Should it be static?
> Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
> net/mptcp/bpf.c:247:42: warning: symbol 'bpf_mptcp_subflow_ctx_by_pos'
> was not declared. Should it be static?

Can we not squash patch 1/5 and 3/5 together?

It is strange to add the helpers but not using them, then register them
in another patch.

Or did you split them to ease the review on BPF side?

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

Re: [PATCH mptcp-next v3 0/5] add mptcp_subflow bpf_iter

[Geliang Tang]

Hi Matt,

On Tue, 2024-09-10 at 09:56 +0200, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 10/09/2024 08:30, Geliang Tang wrote:
> > On Tue, 2024-09-10 at 06:02 +0000, MPTCP CI wrote:
> > > Hi Geliang,
> > > 
> > > Thank you for your modifications, that's great!
> > > 
> > > But sadly, our CI spotted some issues with it when trying to
> > > build
> > > it.
> > > 
> > > You can find more details there:
> > > 
> > >  
> > > https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10786552828
> > > 
> > > Status: failure
> > 
> > These errors reported by CI should be ignored:
> > 
> > Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
> > net/mptcp/bpf.c:215:17: warning: symbol
> > 'bpf_iter_mptcp_subflow_new'
> > was not declared. Should it be static?
> > Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
> > net/mptcp/bpf.c:228:42: warning: symbol
> > 'bpf_iter_mptcp_subflow_next'
> > was not declared. Should it be static?
> > Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
> > net/mptcp/bpf.c:243:18: warning: symbol
> > 'bpf_iter_mptcp_subflow_destroy' was not declared. Should it be
> > static?
> > Unable to compile mptcp source code with make C=1 net/mptcp/bpf.o:
> > net/mptcp/bpf.c:247:42: warning: symbol
> > 'bpf_mptcp_subflow_ctx_by_pos'
> > was not declared. Should it be static?
> 
> Can we not squash patch 1/5 and 3/5 together?
> 
> It is strange to add the helpers but not using them, then register
> them
> in another patch.

It's a bit strange indeed. It's better to register them in
common_btf_ids in kernel/bpf/helpers.c, not in
bpf_mptcp_sched_kfunc_ids in net/mptcp/bpf.c.

I'll send two squash-to patches to fix this.

Thanks,
-Geliang

> 
> Or did you split them to ease the review on BPF side?
> 
> Cheers,
> Matt

Re: [PATCH mptcp-next v3 1/5] bpf: Add mptcp_subflow bpf_iter

[Andrii Nakryiko]

On Mon, Sep 9, 2024 at 10:37 PM Geliang Tang <geliang@kernel.org> wrote:
>
> From: Geliang Tang <tanggeliang@kylinos.cn>
>
> It's necessary to traverse all subflows on the conn_list of an MPTCP
> socket and then call kfunc to modify the fields of each subflow. In
> kernel space, mptcp_for_each_subflow() helper is used for this:
>
>         mptcp_for_each_subflow(msk, subflow)
>                 kfunc(subflow);
>
> But in the MPTCP BPF program, this has not yet been implemented. As
> Martin suggested recently, this conn_list walking + modify-by-kfunc
> usage fits the bpf_iter use case. So this patch adds a new bpf_iter
> type named "mptcp_subflow" to do this and implements its helpers
> bpf_iter_mptcp_subflow_new()/_next()/_destroy().
>
> Then bpf_for_each() for mptcp_subflow can be used in BPF program like
> this:
>
>         bpf_rcu_read_lock();
>         bpf_for_each(mptcp_subflow, subflow, msk)
>                 kfunc(subflow);
>         bpf_rcu_read_unlock();
>
> Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/bpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 4 deletions(-)
>

Not sure why, but only this patch made it to the BPF mailing list? Did
you cc bpf@vger on all patches?

> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 6414824402e6..350672e24a3d 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -201,9 +201,48 @@ static const struct btf_kfunc_id_set bpf_mptcp_fmodret_set = {
>         .set   = &bpf_mptcp_fmodret_ids,
>  };
>
> -__diag_push();
> -__diag_ignore_all("-Wmissing-prototypes",
> -                 "kfuncs which will be used in BPF programs");
> +struct bpf_iter_mptcp_subflow {
> +       __u64 __opaque[2];
> +} __attribute__((aligned(8)));
> +
> +struct bpf_iter_mptcp_subflow_kern {
> +       struct mptcp_sock *msk;
> +       struct list_head *pos;
> +} __attribute__((aligned(8)));
> +
> +__bpf_kfunc_start_defs();
> +
> +__bpf_kfunc int bpf_iter_mptcp_subflow_new(struct bpf_iter_mptcp_subflow *it,
> +                                          struct mptcp_sock *msk)
> +{
> +       struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> +
> +       if (!msk)
> +               return -EINVAL;

you still need to initialize at least kit->msk to NULL to prevent next
implementation below doing the wrong thing

keep in mind, iterator constructor returning error doesn't prevent BPF
program from still calling next() and destroy(), so implementation has
to set iterator state such that next can return NULL if iterator was
never successfully initialized

pw-bot: cr

> +
> +       kit->msk = msk;
> +       kit->pos = &msk->conn_list;
> +       return 0;
> +}
> +
> +__bpf_kfunc struct mptcp_subflow_context *
> +bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it)
> +{
> +       struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> +       struct mptcp_subflow_context *subflow;
> +       struct mptcp_sock *msk = kit->msk;
> +

you should check if (!msk) early here, before accessing kit->pos->next below

> +       subflow = list_entry((kit->pos)->next, struct mptcp_subflow_context, node);

nit: why () around kit->pos?

> +       if (!msk || list_entry_is_head(subflow, &msk->conn_list, node))

as I mentioned, !msk check seems too late. Maybe list_entry_is_head()
is a bit too late as well?

> +               return NULL;
> +
> +       kit->pos = &subflow->node;
> +       return subflow;
> +}
> +
> +__bpf_kfunc void bpf_iter_mptcp_subflow_destroy(struct bpf_iter_mptcp_subflow *it)
> +{
> +}
>
>  __bpf_kfunc struct mptcp_subflow_context *
>  bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data, unsigned int pos)
> @@ -218,7 +257,7 @@ __bpf_kfunc bool bpf_mptcp_subflow_queues_empty(struct sock *sk)
>         return tcp_rtx_queue_empty(sk);
>  }
>
> -__diag_pop();
> +__bpf_kfunc_end_defs();
>
>  BTF_KFUNCS_START(bpf_mptcp_sched_kfunc_ids)
>  BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
> --
> 2.43.0
>
>

Re: [PATCH mptcp-next v3 1/5] bpf: Add mptcp_subflow bpf_iter

[Geliang Tang]

Hi Andrii,

On Wed, 2024-09-11 at 14:00 -0700, Andrii Nakryiko wrote:
> On Mon, Sep 9, 2024 at 10:37 PM Geliang Tang <geliang@kernel.org>
> wrote:
> > 
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > It's necessary to traverse all subflows on the conn_list of an
> > MPTCP
> > socket and then call kfunc to modify the fields of each subflow. In
> > kernel space, mptcp_for_each_subflow() helper is used for this:
> > 
> >         mptcp_for_each_subflow(msk, subflow)
> >                 kfunc(subflow);
> > 
> > But in the MPTCP BPF program, this has not yet been implemented. As
> > Martin suggested recently, this conn_list walking + modify-by-kfunc
> > usage fits the bpf_iter use case. So this patch adds a new bpf_iter
> > type named "mptcp_subflow" to do this and implements its helpers
> > bpf_iter_mptcp_subflow_new()/_next()/_destroy().
> > 
> > Then bpf_for_each() for mptcp_subflow can be used in BPF program
> > like
> > this:
> > 
> >         bpf_rcu_read_lock();
> >         bpf_for_each(mptcp_subflow, subflow, msk)
> >                 kfunc(subflow);
> >         bpf_rcu_read_unlock();
> > 
> > Suggested-by: Martin KaFai Lau <martin.lau@kernel.org>
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/bpf.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
> > --
> >  1 file changed, 43 insertions(+), 4 deletions(-)
> > 
> 
> Not sure why, but only this patch made it to the BPF mailing list?
> Did
> you cc bpf@vger on all patches?

This patch is for "mptcp-next" [1], it depends on the "new MPTCP
subflow subtest" which is under review on the bpf list. We will send it
to the bpf list very soon.

[1]
https://patchwork.kernel.org/project/mptcp/cover/cover.1726023577.git.tanggeliang@kylinos.cn/

> 
> > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> > index 6414824402e6..350672e24a3d 100644
> > --- a/net/mptcp/bpf.c
> > +++ b/net/mptcp/bpf.c
> > @@ -201,9 +201,48 @@ static const struct btf_kfunc_id_set
> > bpf_mptcp_fmodret_set = {
> >         .set   = &bpf_mptcp_fmodret_ids,
> >  };
> > 
> > -__diag_push();
> > -__diag_ignore_all("-Wmissing-prototypes",
> > -                 "kfuncs which will be used in BPF programs");
> > +struct bpf_iter_mptcp_subflow {
> > +       __u64 __opaque[2];
> > +} __attribute__((aligned(8)));
> > +
> > +struct bpf_iter_mptcp_subflow_kern {
> > +       struct mptcp_sock *msk;
> > +       struct list_head *pos;
> > +} __attribute__((aligned(8)));
> > +
> > +__bpf_kfunc_start_defs();
> > +
> > +__bpf_kfunc int bpf_iter_mptcp_subflow_new(struct
> > bpf_iter_mptcp_subflow *it,
> > +                                          struct mptcp_sock *msk)
> > +{
> > +       struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> > +
> > +       if (!msk)
> > +               return -EINVAL;
> 
> you still need to initialize at least kit->msk to NULL to prevent
> next
> implementation below doing the wrong thing
> 
> keep in mind, iterator constructor returning error doesn't prevent
> BPF
> program from still calling next() and destroy(), so implementation
> has
> to set iterator state such that next can return NULL if iterator was
> never successfully initialized
> 

I'll move "kit->msk = msk;" earlier like this:

{
        struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;

        kit->msk = msk;
        if (!msk)
                return -EINVAL;

        kit->pos = &msk->conn_list;
        return 0;
}

> pw-bot: cr
> 
> > +
> > +       kit->msk = msk;
> > +       kit->pos = &msk->conn_list;
> > +       return 0;
> > +}
> > +
> > +__bpf_kfunc struct mptcp_subflow_context *
> > +bpf_iter_mptcp_subflow_next(struct bpf_iter_mptcp_subflow *it)
> > +{
> > +       struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
> > +       struct mptcp_subflow_context *subflow;
> > +       struct mptcp_sock *msk = kit->msk;
> > +
> 
> you should check if (!msk) early here, before accessing kit->pos-
> >next below
> 
> > +       subflow = list_entry((kit->pos)->next, struct
> > mptcp_subflow_context, node);
> 
> nit: why () around kit->pos?
> 
> > +       if (!msk || list_entry_is_head(subflow, &msk->conn_list,
> > node))
> 
> as I mentioned, !msk check seems too late. Maybe list_entry_is_head()
> is a bit too late as well?

We can use list_is_last() to check kit->pos earlier. But here we use
list_entry_is_head(), it should be after list_entry().

I'll move "if (!msk)" check earlier like this:

{
        struct bpf_iter_mptcp_subflow_kern *kit = (void *)it;
        struct mptcp_subflow_context *subflow;
        struct mptcp_sock *msk = kit->msk;

        if (!msk)
                return NULL;

        subflow = list_entry(kit->pos->next, struct
mptcp_subflow_context, node);
        if (!subflow || list_entry_is_head(subflow, &msk->conn_list,
node))
                return NULL;

        kit->pos = &subflow->node;
        return subflow;
}

Thanks,
-Geliang

> 
> > +               return NULL;
> > +
> > +       kit->pos = &subflow->node;
> > +       return subflow;
> > +}
> > +
> > +__bpf_kfunc void bpf_iter_mptcp_subflow_destroy(struct
> > bpf_iter_mptcp_subflow *it)
> > +{
> > +}
> > 
> >  __bpf_kfunc struct mptcp_subflow_context *
> >  bpf_mptcp_subflow_ctx_by_pos(const struct mptcp_sched_data *data,
> > unsigned int pos)
> > @@ -218,7 +257,7 @@ __bpf_kfunc bool
> > bpf_mptcp_subflow_queues_empty(struct sock *sk)
> >         return tcp_rtx_queue_empty(sk);
> >  }
> > 
> > -__diag_pop();
> > +__bpf_kfunc_end_defs();
> > 
> >  BTF_KFUNCS_START(bpf_mptcp_sched_kfunc_ids)
> >  BTF_ID_FLAGS(func, mptcp_subflow_set_scheduled)
> > --
> > 2.43.0
> > 
> >