From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a "cgroup/getsockopt" program "iters_subflow" to test the
newly added mptcp_subflow bpf_iter.

Export mptcp_subflow helpers bpf_iter_mptcp_subflow_new/_next/_destroy
and other helpers into bpf_experimental.h.

Use bpf_for_each() to walk the subflow list of an msk. From there,
future MPTCP-specific kfunc can be called in the loop. Because they are
not there yet, this test doesn't do anything very "useful" for the
moment, but it focuses on validating the 'bpf_iter' part and the basic
MPTCP kfunc. That's why it simply adds all subflow ids to local variable
local_ids to make sure all subflows have been seen, then invoke
mptcp_subflow_tcp_sock() in the loop to pick the subflow context.

Out of the loop, use bpf_mptcp_subflow_ctx() to get the subflow context
of the picked subflow context and do some verifications. Finally, assign
local_ids to global variable ids so that the application can obtain this
value.

A related subtest called test_iters_subflow is added to load and verify
the newly added mptcp_subflow type bpf_iter example in test_mptcp. The
endpoint_init() helper is used to add 3 new subflow endpoints. Then one
byte of message is sent to trigger the creation of new subflows.
getsockopt() is invoked once the subflows have been created to trigger
the "cgroup/getsockopt" test program "iters_subflow". skel->bss->ids is
then checked to make sure it equals 10, the sum of each subflow ID: we
should have 4 subflows: 1 + 2 + 3 + 4 = 10. If that's the case, the
bpf_iter loop did the job as expected.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
 - v2:
   - explicit sk protocol checks are no longer needed, implicitly done
     in bpf_skc_to_mptcp_sock().
   - use bpf_skc_to_mptcp_sock() instead of bpf_mptcp_sk(), and
     mptcp_subflow_tcp_sock() instead of bpf_mptcp_subflow_tcp_sock().
   - bpf_mptcp_subflow_ctx() can now return NULL.
 - v3:
   - Use bpf_core_cast to get the msk instead of bpf_skc_to_mptcp_sock.
   - Drop bpf_mptcp_sock_acquire and bpf_mptcp_sock_release (Martin).
   - Adapt the commit message accordingly.
   - Remove no longer needed export to the mptcp_bpf.h file and adapt
     bpf_iter_mptcp_subflow_new parameter in bpf_experimental.h.
---
 tools/testing/selftests/bpf/bpf_experimental.h     |  8 +++
 tools/testing/selftests/bpf/prog_tests/mptcp.c     | 73 ++++++++++++++++++++++
 tools/testing/selftests/bpf/progs/mptcp_bpf.h      |  4 ++
 .../testing/selftests/bpf/progs/mptcp_bpf_iters.c  | 59 +++++++++++++++++
 4 files changed, 144 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
index cd8ecd39c3f3c68d40c6e3e1465b42ed66537027..6a96c56f0725a86ab6e83675ca0e474c3d668b10 100644
--- a/tools/testing/selftests/bpf/bpf_experimental.h
+++ b/tools/testing/selftests/bpf/bpf_experimental.h
@@ -575,6 +575,14 @@ 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 sock *sk) __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/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 85f3d4119802a85c86cde7b74a0b857252bad8b8..f37574b5ef68d8f32f8002df317869dfdf1d4b2d 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -11,6 +11,7 @@
 #include "mptcp_sock.skel.h"
 #include "mptcpify.skel.h"
 #include "mptcp_subflow.skel.h"
+#include "mptcp_bpf_iters.skel.h"
 
 #define NS_TEST "mptcp_ns"
 #define ADDR_1	"10.0.1.1"
@@ -33,6 +34,9 @@
 #ifndef MPTCP_INFO
 #define MPTCP_INFO		1
 #endif
+#ifndef TCP_IS_MPTCP
+#define TCP_IS_MPTCP		43	/* Is MPTCP being used? */
+#endif
 #ifndef MPTCP_INFO_FLAG_FALLBACK
 #define MPTCP_INFO_FLAG_FALLBACK		_BITUL(0)
 #endif
@@ -480,6 +484,73 @@ static void test_subflow(void)
 	close(cgroup_fd);
 }
 
+static void run_iters_subflow(void)
+{
+	int server_fd, client_fd;
+	int is_mptcp, err;
+	socklen_t len;
+
+	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);
+
+	len = sizeof(is_mptcp);
+	/* mainly to trigger the BPF program */
+	err = getsockopt(client_fd, SOL_TCP, TCP_IS_MPTCP, &is_mptcp, &len);
+	if (ASSERT_OK(err, "getsockopt(client_fd, TCP_IS_MPTCP)"))
+		ASSERT_EQ(is_mptcp, 1, "is_mptcp");
+
+	close(client_fd);
+close_server:
+	close(server_fd);
+}
+
+static void test_iters_subflow(void)
+{
+	struct mptcp_bpf_iters *skel;
+	struct netns_obj *netns;
+	int cgroup_fd;
+
+	cgroup_fd = test__join_cgroup("/iters_subflow");
+	if (!ASSERT_OK_FD(cgroup_fd, "join_cgroup: iters_subflow"))
+		return;
+
+	skel = mptcp_bpf_iters__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_load: iters_subflow"))
+		goto close_cgroup;
+
+	skel->links.iters_subflow = bpf_program__attach_cgroup(skel->progs.iters_subflow,
+							       cgroup_fd);
+	if (!ASSERT_OK_PTR(skel->links.iters_subflow, "attach getsockopt"))
+		goto skel_destroy;
+
+	netns = netns_new(NS_TEST, true);
+	if (!ASSERT_OK_PTR(netns, "netns_new: iters_subflow"))
+		goto skel_destroy;
+
+	if (endpoint_init("subflow", 4) < 0)
+		goto close_netns;
+
+	run_iters_subflow();
+
+	/* 1 + 2 + 3 + 4 = 10 */
+	ASSERT_EQ(skel->bss->ids, 10, "subflow ids");
+
+close_netns:
+	netns_free(netns);
+skel_destroy:
+	mptcp_bpf_iters__destroy(skel);
+close_cgroup:
+	close(cgroup_fd);
+}
+
 void test_mptcp(void)
 {
 	if (test__start_subtest("base"))
@@ -488,4 +559,6 @@ void test_mptcp(void)
 		test_mptcpify();
 	if (test__start_subtest("subflow"))
 		test_subflow();
+	if (test__start_subtest("iters_subflow"))
+		test_iters_subflow();
 }
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
index 3b188ccdcc4041acb4f7ed38ae8ddf5a7305466a..aa897074de6f377e8cddc859c3b2dc3751f14381 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
@@ -39,4 +39,8 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
 	return subflow->tcp_sock;
 }
 
+/* ksym */
+extern struct mptcp_subflow_context *
+bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
+
 #endif
diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
new file mode 100644
index 0000000000000000000000000000000000000000..a1d8f9b20259a9cbdc46d58e0d18157564fa5acd
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
@@ -0,0 +1,59 @@
+// 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 ids;
+
+#ifndef TCP_IS_MPTCP
+#define TCP_IS_MPTCP		43	/* Is MPTCP being used? */
+#endif
+
+SEC("cgroup/getsockopt")
+int iters_subflow(struct bpf_sockopt *ctx)
+{
+	struct mptcp_subflow_context *subflow;
+	struct bpf_sock *sk = ctx->sk;
+	struct sock *ssk = NULL;
+	struct mptcp_sock *msk;
+	int local_ids = 0;
+
+	if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
+		return 1;
+
+	msk = bpf_core_cast(sk, struct mptcp_sock);
+	if (!msk || msk->pm.server_side || !msk->pm.subflows)
+		return 1;
+
+	bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
+		/* Here MPTCP-specific packet scheduler kfunc can be called:
+		 * this test is not doing anything really useful, only to
+		 * verify the iteration works.
+		 */
+
+		local_ids += subflow->subflow_id;
+
+		/* only to check the following helper works */
+		ssk = mptcp_subflow_tcp_sock(subflow);
+	}
+
+	if (!ssk)
+		goto out;
+
+	/* assert: if not OK, something wrong on the kernel side */
+	if (ssk->sk_dport != ((struct sock *)msk)->sk_dport)
+		goto out;
+
+	/* only to check the following kfunc works */
+	subflow = bpf_mptcp_subflow_ctx(ssk);
+	if (!subflow || subflow->token != msk->token)
+		goto out;
+
+	ids = local_ids;
+
+out:
+	return 1;
+}

-- 
2.48.1

On 3/20/25 10:48 AM, Matthieu Baerts (NGI0) wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a "cgroup/getsockopt" program "iters_subflow" to test the
> newly added mptcp_subflow bpf_iter.
> 
> Export mptcp_subflow helpers bpf_iter_mptcp_subflow_new/_next/_destroy
> and other helpers into bpf_experimental.h.
> 
> Use bpf_for_each() to walk the subflow list of an msk. From there,
> future MPTCP-specific kfunc can be called in the loop. Because they are
> not there yet, this test doesn't do anything very "useful" for the
> moment, but it focuses on validating the 'bpf_iter' part and the basic
> MPTCP kfunc. That's why it simply adds all subflow ids to local variable
> local_ids to make sure all subflows have been seen, then invoke
> mptcp_subflow_tcp_sock() in the loop to pick the subflow context.
> 
> Out of the loop, use bpf_mptcp_subflow_ctx() to get the subflow context
> of the picked subflow context and do some verifications. Finally, assign
> local_ids to global variable ids so that the application can obtain this
> value.
> 
> A related subtest called test_iters_subflow is added to load and verify
> the newly added mptcp_subflow type bpf_iter example in test_mptcp. The
> endpoint_init() helper is used to add 3 new subflow endpoints. Then one
> byte of message is sent to trigger the creation of new subflows.
> getsockopt() is invoked once the subflows have been created to trigger
> the "cgroup/getsockopt" test program "iters_subflow". skel->bss->ids is
> then checked to make sure it equals 10, the sum of each subflow ID: we
> should have 4 subflows: 1 + 2 + 3 + 4 = 10. If that's the case, the
> bpf_iter loop did the job as expected.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>   - v2:
>     - explicit sk protocol checks are no longer needed, implicitly done
>       in bpf_skc_to_mptcp_sock().
>     - use bpf_skc_to_mptcp_sock() instead of bpf_mptcp_sk(), and
>       mptcp_subflow_tcp_sock() instead of bpf_mptcp_subflow_tcp_sock().
>     - bpf_mptcp_subflow_ctx() can now return NULL.
>   - v3:
>     - Use bpf_core_cast to get the msk instead of bpf_skc_to_mptcp_sock.
>     - Drop bpf_mptcp_sock_acquire and bpf_mptcp_sock_release (Martin).
>     - Adapt the commit message accordingly.
>     - Remove no longer needed export to the mptcp_bpf.h file and adapt
>       bpf_iter_mptcp_subflow_new parameter in bpf_experimental.h.
> ---
>   tools/testing/selftests/bpf/bpf_experimental.h     |  8 +++
>   tools/testing/selftests/bpf/prog_tests/mptcp.c     | 73 ++++++++++++++++++++++
>   tools/testing/selftests/bpf/progs/mptcp_bpf.h      |  4 ++
>   .../testing/selftests/bpf/progs/mptcp_bpf_iters.c  | 59 +++++++++++++++++
>   4 files changed, 144 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h
> index cd8ecd39c3f3c68d40c6e3e1465b42ed66537027..6a96c56f0725a86ab6e83675ca0e474c3d668b10 100644
> --- a/tools/testing/selftests/bpf/bpf_experimental.h
> +++ b/tools/testing/selftests/bpf/bpf_experimental.h
> @@ -575,6 +575,14 @@ 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 sock *sk) __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/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 85f3d4119802a85c86cde7b74a0b857252bad8b8..f37574b5ef68d8f32f8002df317869dfdf1d4b2d 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -11,6 +11,7 @@
>   #include "mptcp_sock.skel.h"
>   #include "mptcpify.skel.h"
>   #include "mptcp_subflow.skel.h"
> +#include "mptcp_bpf_iters.skel.h"
>   
>   #define NS_TEST "mptcp_ns"
>   #define ADDR_1	"10.0.1.1"
> @@ -33,6 +34,9 @@
>   #ifndef MPTCP_INFO
>   #define MPTCP_INFO		1
>   #endif
> +#ifndef TCP_IS_MPTCP
> +#define TCP_IS_MPTCP		43	/* Is MPTCP being used? */
> +#endif
>   #ifndef MPTCP_INFO_FLAG_FALLBACK
>   #define MPTCP_INFO_FLAG_FALLBACK		_BITUL(0)
>   #endif
> @@ -480,6 +484,73 @@ static void test_subflow(void)
>   	close(cgroup_fd);
>   }
>   
> +static void run_iters_subflow(void)
> +{
> +	int server_fd, client_fd;
> +	int is_mptcp, err;
> +	socklen_t len;
> +
> +	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);
> +
> +	len = sizeof(is_mptcp);
> +	/* mainly to trigger the BPF program */
> +	err = getsockopt(client_fd, SOL_TCP, TCP_IS_MPTCP, &is_mptcp, &len);
> +	if (ASSERT_OK(err, "getsockopt(client_fd, TCP_IS_MPTCP)"))
> +		ASSERT_EQ(is_mptcp, 1, "is_mptcp");
> +
> +	close(client_fd);
> +close_server:
> +	close(server_fd);
> +}
> +
> +static void test_iters_subflow(void)
> +{
> +	struct mptcp_bpf_iters *skel;
> +	struct netns_obj *netns;
> +	int cgroup_fd;
> +
> +	cgroup_fd = test__join_cgroup("/iters_subflow");
> +	if (!ASSERT_OK_FD(cgroup_fd, "join_cgroup: iters_subflow"))
> +		return;
> +
> +	skel = mptcp_bpf_iters__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open_load: iters_subflow"))
> +		goto close_cgroup;
> +
> +	skel->links.iters_subflow = bpf_program__attach_cgroup(skel->progs.iters_subflow,
> +							       cgroup_fd);
> +	if (!ASSERT_OK_PTR(skel->links.iters_subflow, "attach getsockopt"))
> +		goto skel_destroy;
> +
> +	netns = netns_new(NS_TEST, true);
> +	if (!ASSERT_OK_PTR(netns, "netns_new: iters_subflow"))
> +		goto skel_destroy;
> +
> +	if (endpoint_init("subflow", 4) < 0)
> +		goto close_netns;
> +
> +	run_iters_subflow();
> +
> +	/* 1 + 2 + 3 + 4 = 10 */
> +	ASSERT_EQ(skel->bss->ids, 10, "subflow ids");
> +
> +close_netns:
> +	netns_free(netns);
> +skel_destroy:
> +	mptcp_bpf_iters__destroy(skel);
> +close_cgroup:
> +	close(cgroup_fd);
> +}
> +
>   void test_mptcp(void)
>   {
>   	if (test__start_subtest("base"))
> @@ -488,4 +559,6 @@ void test_mptcp(void)
>   		test_mptcpify();
>   	if (test__start_subtest("subflow"))
>   		test_subflow();
> +	if (test__start_subtest("iters_subflow"))
> +		test_iters_subflow();
>   }
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf.h b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> index 3b188ccdcc4041acb4f7ed38ae8ddf5a7305466a..aa897074de6f377e8cddc859c3b2dc3751f14381 100644
> --- a/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf.h
> @@ -39,4 +39,8 @@ mptcp_subflow_tcp_sock(const struct mptcp_subflow_context *subflow)
>   	return subflow->tcp_sock;
>   }
>   
> +/* ksym */
> +extern struct mptcp_subflow_context *
> +bpf_mptcp_subflow_ctx(const struct sock *sk) __ksym;
> +
>   #endif
> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..a1d8f9b20259a9cbdc46d58e0d18157564fa5acd
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
> @@ -0,0 +1,59 @@
> +// 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 ids;
> +
> +#ifndef TCP_IS_MPTCP
> +#define TCP_IS_MPTCP		43	/* Is MPTCP being used? */
> +#endif
> +
> +SEC("cgroup/getsockopt")
> +int iters_subflow(struct bpf_sockopt *ctx)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct bpf_sock *sk = ctx->sk;
> +	struct sock *ssk = NULL;
> +	struct mptcp_sock *msk;
> +	int local_ids = 0;
> +
> +	if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
> +		return 1;
> +
> +	msk = bpf_core_cast(sk, struct mptcp_sock);
> +	if (!msk || msk->pm.server_side || !msk->pm.subflows)
> +		return 1;
> +
> +	bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
> +		/* Here MPTCP-specific packet scheduler kfunc can be called:
> +		 * this test is not doing anything really useful, only to

Lets fold the bpf_iter_mptcp_subflow addition into the future "mptcp_sched_ops" 
set (the github link that you mentioned in patch 2). Post them as one set to 
have a more practical example.

> +		 * verify the iteration works.
> +		 */
> +
> +		local_ids += subflow->subflow_id;
> +
> +		/* only to check the following helper works */
> +		ssk = mptcp_subflow_tcp_sock(subflow);
> +	}
> +
> +	if (!ssk)
> +		goto out;
> +
> +	/* assert: if not OK, something wrong on the kernel side */
> +	if (ssk->sk_dport != ((struct sock *)msk)->sk_dport)
> +		goto out;
> +
> +	/* only to check the following kfunc works */
> +	subflow = bpf_mptcp_subflow_ctx(ssk);

bpf_core_cast should be as good instead of adding a new bpf_mptcp_subflow_ctx() 
kfunc, so patch 1 should not be needed.

> +	if (!subflow || subflow->token != msk->token)
> +		goto out;
> +
> +	ids = local_ids;
> +
> +out:
> +	return 1;
> +}
>

Hi Martin,

On 17/05/2025 00:48, Martin KaFai Lau wrote:
> On 3/20/25 10:48 AM, Matthieu Baerts (NGI0) wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> This patch adds a "cgroup/getsockopt" program "iters_subflow" to test the
>> newly added mptcp_subflow bpf_iter.
>>
>> Export mptcp_subflow helpers bpf_iter_mptcp_subflow_new/_next/_destroy
>> and other helpers into bpf_experimental.h.
>>
>> Use bpf_for_each() to walk the subflow list of an msk. From there,
>> future MPTCP-specific kfunc can be called in the loop. Because they are
>> not there yet, this test doesn't do anything very "useful" for the
>> moment, but it focuses on validating the 'bpf_iter' part and the basic
>> MPTCP kfunc. That's why it simply adds all subflow ids to local variable
>> local_ids to make sure all subflows have been seen, then invoke
>> mptcp_subflow_tcp_sock() in the loop to pick the subflow context.
>>
>> Out of the loop, use bpf_mptcp_subflow_ctx() to get the subflow context
>> of the picked subflow context and do some verifications. Finally, assign
>> local_ids to global variable ids so that the application can obtain this
>> value.
>>
>> A related subtest called test_iters_subflow is added to load and verify
>> the newly added mptcp_subflow type bpf_iter example in test_mptcp. The
>> endpoint_init() helper is used to add 3 new subflow endpoints. Then one
>> byte of message is sent to trigger the creation of new subflows.
>> getsockopt() is invoked once the subflows have been created to trigger
>> the "cgroup/getsockopt" test program "iters_subflow". skel->bss->ids is
>> then checked to make sure it equals 10, the sum of each subflow ID: we
>> should have 4 subflows: 1 + 2 + 3 + 4 = 10. If that's the case, the
>> bpf_iter loop did the job as expected.

(...)

>> diff --git a/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c b/
>> tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
>> new file mode 100644
>> index
>> 0000000000000000000000000000000000000000..a1d8f9b20259a9cbdc46d58e0d18157564fa5acd
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/mptcp_bpf_iters.c
>> @@ -0,0 +1,59 @@
>> +// 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 ids;
>> +
>> +#ifndef TCP_IS_MPTCP
>> +#define TCP_IS_MPTCP        43    /* Is MPTCP being used? */
>> +#endif
>> +
>> +SEC("cgroup/getsockopt")
>> +int iters_subflow(struct bpf_sockopt *ctx)
>> +{
>> +    struct mptcp_subflow_context *subflow;
>> +    struct bpf_sock *sk = ctx->sk;
>> +    struct sock *ssk = NULL;
>> +    struct mptcp_sock *msk;
>> +    int local_ids = 0;
>> +
>> +    if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
>> +        return 1;
>> +
>> +    msk = bpf_core_cast(sk, struct mptcp_sock);
>> +    if (!msk || msk->pm.server_side || !msk->pm.subflows)
>> +        return 1;
>> +
>> +    bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
>> +        /* Here MPTCP-specific packet scheduler kfunc can be called:
>> +         * this test is not doing anything really useful, only to
> 
> Lets fold the bpf_iter_mptcp_subflow addition into the future
> "mptcp_sched_ops" set (the github link that you mentioned in patch 2).
> Post them as one set to have a more practical example.

Thank you for this suggestion. We can delay that if needed.

Note that we have two struct_ops in preparation: mptcp_sched_ops and
mptcp_pm_ops. We don't know which one will be ready first. They are both
"blocked" by internal API modifications we would like to do to ease the
maintenance later before "exposing" such API's via BPF. That's why we
suggested to upstream this common part first as it is ready. But we can
of course wait if you prefer.

>> +         * verify the iteration works.
>> +         */
>> +
>> +        local_ids += subflow->subflow_id;
>> +
>> +        /* only to check the following helper works */
>> +        ssk = mptcp_subflow_tcp_sock(subflow);
>> +    }
>> +
>> +    if (!ssk)
>> +        goto out;
>> +
>> +    /* assert: if not OK, something wrong on the kernel side */
>> +    if (ssk->sk_dport != ((struct sock *)msk)->sk_dport)
>> +        goto out;
>> +
>> +    /* only to check the following kfunc works */
>> +    subflow = bpf_mptcp_subflow_ctx(ssk);
> 
> bpf_core_cast should be as good instead of adding a new
> bpf_mptcp_subflow_ctx() kfunc, so patch 1 should not be needed.

OK, indeed, in this series we don't need it. We will need it later to
modify some fields from the "subflow" structure directly. We can do the
modification or drop this test when the new struct_ops will be ready.

>> +    if (!subflow || subflow->token != msk->token)
>> +        goto out;
>> +
>> +    ids = local_ids;
>> +
>> +out:
>> +    return 1;
>> +}
>>
> 
> 

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

On 5/19/25 3:04 AM, Matthieu Baerts wrote:
>>> +SEC("cgroup/getsockopt")
>>> +int iters_subflow(struct bpf_sockopt *ctx)
>>> +{
>>> +    struct mptcp_subflow_context *subflow;
>>> +    struct bpf_sock *sk = ctx->sk;
>>> +    struct sock *ssk = NULL;
>>> +    struct mptcp_sock *msk;
>>> +    int local_ids = 0;
>>> +
>>> +    if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
>>> +        return 1;
>>> +
>>> +    msk = bpf_core_cast(sk, struct mptcp_sock);
>>> +    if (!msk || msk->pm.server_side || !msk->pm.subflows)
>>> +        return 1;
>>> +
>>> +    bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
>>> +        /* Here MPTCP-specific packet scheduler kfunc can be called:
>>> +         * this test is not doing anything really useful, only to
>>
>> Lets fold the bpf_iter_mptcp_subflow addition into the future
>> "mptcp_sched_ops" set (the github link that you mentioned in patch 2).
>> Post them as one set to have a more practical example.
> 
> Thank you for this suggestion. We can delay that if needed.
> 
> Note that we have two struct_ops in preparation: mptcp_sched_ops and
> mptcp_pm_ops. We don't know which one will be ready first. They are both
> "blocked" by internal API modifications we would like to do to ease the
> maintenance later before "exposing" such API's via BPF. That's why we
> suggested to upstream this common part first as it is ready. But we can
> of course wait if you prefer.

This set is useful for discussing the questions you raised in patch 2.

I still don't see it useful to upstream patch 2 alone. The existing 
selftests/bpf/progs/mptcp_subflow.c has already shown a way to do similar 
iteration in SEC("cgroup/getsockopt") without patch 2.

I would prefer to wait for a fuller picture on the main struct_ops use case 
first to ensure that we didn't overlook things. iiuc, improving the iteration in 
SEC("cgroup/getsockopt") is not the main objective.

> 
>>> +         * verify the iteration works.
>>> +         */
>>> +
>>> +        local_ids += subflow->subflow_id;
>>> +
>>> +        /* only to check the following helper works */
>>> +        ssk = mptcp_subflow_tcp_sock(subflow);
>>> +    }
>>> +
>>> +    if (!ssk)
>>> +        goto out;
>>> +
>>> +    /* assert: if not OK, something wrong on the kernel side */
>>> +    if (ssk->sk_dport != ((struct sock *)msk)->sk_dport)
>>> +        goto out;
>>> +
>>> +    /* only to check the following kfunc works */
>>> +    subflow = bpf_mptcp_subflow_ctx(ssk);
>>
>> bpf_core_cast should be as good instead of adding a new
>> bpf_mptcp_subflow_ctx() kfunc, so patch 1 should not be needed.
> 
> OK, indeed, in this series we don't need it. We will need it later to
> modify some fields from the "subflow" structure directly. We can do the

The "ssk" here is not a trusted pointer. Note that in patch 1, the kfunc 
bpf_mptcp_subflow_ctx() does not specify KF_TRUSTED_ARGS. I suspect it should be 
KF_TRUSTED_ARGS based on what you described here.

Hi Martin,

On 21/05/2025 00:18, Martin KaFai Lau wrote:
> On 5/19/25 3:04 AM, Matthieu Baerts wrote:
>>>> +SEC("cgroup/getsockopt")
>>>> +int iters_subflow(struct bpf_sockopt *ctx)
>>>> +{
>>>> +    struct mptcp_subflow_context *subflow;
>>>> +    struct bpf_sock *sk = ctx->sk;
>>>> +    struct sock *ssk = NULL;
>>>> +    struct mptcp_sock *msk;
>>>> +    int local_ids = 0;
>>>> +
>>>> +    if (ctx->level != SOL_TCP || ctx->optname != TCP_IS_MPTCP)
>>>> +        return 1;
>>>> +
>>>> +    msk = bpf_core_cast(sk, struct mptcp_sock);
>>>> +    if (!msk || msk->pm.server_side || !msk->pm.subflows)
>>>> +        return 1;
>>>> +
>>>> +    bpf_for_each(mptcp_subflow, subflow, (struct sock *)sk) {
>>>> +        /* Here MPTCP-specific packet scheduler kfunc can be called:
>>>> +         * this test is not doing anything really useful, only to
>>>
>>> Lets fold the bpf_iter_mptcp_subflow addition into the future
>>> "mptcp_sched_ops" set (the github link that you mentioned in patch 2).
>>> Post them as one set to have a more practical example.
>>
>> Thank you for this suggestion. We can delay that if needed.
>>
>> Note that we have two struct_ops in preparation: mptcp_sched_ops and
>> mptcp_pm_ops. We don't know which one will be ready first. They are both
>> "blocked" by internal API modifications we would like to do to ease the
>> maintenance later before "exposing" such API's via BPF. That's why we
>> suggested to upstream this common part first as it is ready. But we can
>> of course wait if you prefer.
> 
> This set is useful for discussing the questions you raised in patch 2.
> 
> I still don't see it useful to upstream patch 2 alone. The existing
> selftests/bpf/progs/mptcp_subflow.c has already shown a way to do
> similar iteration in SEC("cgroup/getsockopt") without patch 2.
> 
> I would prefer to wait for a fuller picture on the main struct_ops use
> case first to ensure that we didn't overlook things. iiuc, improving the
> iteration in SEC("cgroup/getsockopt") is not the main objective.

I understand, that makes sense. When the rest will be ready, we will
upstream patches from this series, except this one ("useless" selftest),
and restricting bpf_iter_mptcp_subflow_* and other new kfuncs to
BPF_PROG_TYPE_STRUCT_OPS only. So not to BPF_PROG_TYPE_CGROUP_SOCKOPT
any more which was only needed for this new test. I don't think this
program type requires access to these new kfunc for useful use-cases.
This can be changed later if required anyway.

>>>> +         * verify the iteration works.
>>>> +         */
>>>> +
>>>> +        local_ids += subflow->subflow_id;
>>>> +
>>>> +        /* only to check the following helper works */
>>>> +        ssk = mptcp_subflow_tcp_sock(subflow);
>>>> +    }
>>>> +
>>>> +    if (!ssk)
>>>> +        goto out;
>>>> +
>>>> +    /* assert: if not OK, something wrong on the kernel side */
>>>> +    if (ssk->sk_dport != ((struct sock *)msk)->sk_dport)
>>>> +        goto out;
>>>> +
>>>> +    /* only to check the following kfunc works */
>>>> +    subflow = bpf_mptcp_subflow_ctx(ssk);
>>>
>>> bpf_core_cast should be as good instead of adding a new
>>> bpf_mptcp_subflow_ctx() kfunc, so patch 1 should not be needed.
>>
>> OK, indeed, in this series we don't need it. We will need it later to
>> modify some fields from the "subflow" structure directly. We can do the
> 
> The "ssk" here is not a trusted pointer. Note that in patch 1, the kfunc
> bpf_mptcp_subflow_ctx() does not specify KF_TRUSTED_ARGS. I suspect it
> should be KF_TRUSTED_ARGS based on what you described here.

Good point, I think this flag is indeed missing.

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