[PATCH mptcp-next v12 0/5] add skc_to_mptcp_sock

[PATCH mptcp-next v12 0/5] add skc_to_mptcp_sock

[Geliang Tang]

v12:
 patch 1:
  - merged the squash-to patch for v11.
  - rebased to export/20220324T054815.
 patch 2:
  - fix the libbpf 0.8 build error.
 patch 4:
  - use fgets instead of fread to get null terminated string.

v11:
 - merge "check more values from mptcp_sock" series into this series.
 - patch 2:
   - rebased.
 - patch 3:
   - rebased.
   - add 'Copyright' entry as Matt suggested in v9.
 - patch 4:
   - add test__start_subtest.
   - add some 'Copyright' entries.
 - patch 5:
   - change monitor_log_path to static
   - drop bpf_trace_printk()
 - patch 6:
   - use TCP_CA_NAME_MAX instead of strlen(val.ca_name) in strncmp().
   - use 'sysctl -b net.ipv4.tcp_congestion_control' to get ca_name.

v10:
 - merge the squash-to patch.
 - update patch 3 as Matt suggested.
 - add sync() in get_msk_token() before read().
 - add a comment for get_msk_token().
 - update subjects and commit logs.

v9:
 - update progs/mptcp.c in patch 4 and 5

v8:
 - update as Matt suggested.

v7:
 - parse msk token from the output of 'ip mptcp monitor'.
 - add Nicolas and Matt's SoB tags.

v6:
 - add skc_to_mptcp_sock helper and test

RESEND:
 - fix the CI Build Failure.
v5:
 - fix incorrect token value
 - verify the token in selftest

v4:
 - define bpf_mptcp_sock_proto as a static function, no longer export
   it in linux/bpf.h

v3:
 - use RET_PTR_TO_BTF_ID_OR_NULL instead of RET_PTR_TO_MPTCP_SOCK_OR_NULL
 - add a new bpf_id BTF_SOCK_TYPE_MPTCP

v2:
 - keep RET_PTR_TO_MPTCP_SOCK_OR_NULL. If we use RET_PTR_TO_BTF_ID_OR_NULL
instead of RET_PTR_TO_MPTCP_SOCK_OR_NULL as Alexei suggested, the
"userspace" tests developed by Nicolas will break.

Geliang Tang (5):
  bpf: add bpf_skc_to_mptcp_sock_proto
  Squash to "selftests: bpf: add MPTCP test base"
  selftests: bpf: test bpf_skc_to_mptcp_sock
  selftests: bpf: verify ca_name of struct mptcp_sock
  selftests: bpf: verify first subflow of mptcp_sock

 include/linux/bpf.h                           |   9 +
 include/linux/btf_ids.h                       |   3 +-
 include/uapi/linux/bpf.h                      |   7 +
 net/core/filter.c                             |  18 ++
 net/mptcp/Makefile                            |   2 +
 net/mptcp/bpf.c                               |  22 ++
 scripts/bpf_doc.py                            |   2 +
 tools/include/uapi/linux/bpf.h                |   7 +
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |  12 ++
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 192 +++++++++++++++---
 tools/testing/selftests/bpf/progs/mptcp.c     |  44 +++-
 11 files changed, 284 insertions(+), 34 deletions(-)
 create mode 100644 net/mptcp/bpf.c

-- 
2.34.1

[PATCH mptcp-next v12 1/5] bpf: add bpf_skc_to_mptcp_sock_proto

[Geliang Tang]

This patch implemented a new bpf_func_proto bpf_skc_to_mptcp_sock_proto.
Defined a new bpf_id BTF_SOCK_TYPE_MPTCP, implemented the helper
bpf_skc_to_mptcp_sock, and added a new helper bpf_mptcp_sock_from_subflow
to get struct mptcp_sock from a given subflow socket.

Co-developed-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
Signed-off-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
Co-developed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/linux/bpf.h            |  9 +++++++++
 include/linux/btf_ids.h        |  3 ++-
 include/uapi/linux/bpf.h       |  7 +++++++
 net/core/filter.c              | 18 ++++++++++++++++++
 net/mptcp/Makefile             |  2 ++
 net/mptcp/bpf.c                | 22 ++++++++++++++++++++++
 scripts/bpf_doc.py             |  2 ++
 tools/include/uapi/linux/bpf.h |  7 +++++++
 8 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 net/mptcp/bpf.c

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index bdb5298735ce..9752445aa266 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2372,6 +2372,15 @@ static inline u32 bpf_xdp_sock_convert_ctx_access(enum bpf_access_type type,
 }
 #endif /* CONFIG_INET */
 
+#ifdef CONFIG_MPTCP
+struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk);
+#else /* CONFIG_MPTCP */
+static inline struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
+{
+	return NULL;
+}
+#endif /* CONFIG_MPTCP */
+
 enum bpf_text_poke_type {
 	BPF_MOD_CALL,
 	BPF_MOD_JUMP,
diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h
index bc5d9cc34e4c..335a19092368 100644
--- a/include/linux/btf_ids.h
+++ b/include/linux/btf_ids.h
@@ -178,7 +178,8 @@ extern struct btf_id_set name;
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_TCP6, tcp6_sock)			\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP, udp_sock)			\
 	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UDP6, udp6_sock)			\
-	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UNIX, unix_sock)
+	BTF_SOCK_TYPE(BTF_SOCK_TYPE_UNIX, unix_sock)			\
+	BTF_SOCK_TYPE(BTF_SOCK_TYPE_MPTCP, mptcp_sock)
 
 enum {
 #define BTF_SOCK_TYPE(name, str) name,
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9ef1f3e1c22f..785f2cb15495 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5143,6 +5143,12 @@ union bpf_attr {
  *		The **hash_algo** is returned on success,
  *		**-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
  *		invalid arguments are passed.
+ *
+ * struct mptcp_sock *bpf_skc_to_mptcp_sock(void *sk)
+ *	Description
+ *		Dynamically cast a *sk* pointer to a *mptcp_sock* pointer.
+ *	Return
+ *		*sk* if casting is valid, or **NULL** otherwise.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5339,6 +5345,7 @@ union bpf_attr {
 	FN(copy_from_user_task),	\
 	FN(skb_set_tstamp),		\
 	FN(ima_file_hash),		\
+	FN(skc_to_mptcp_sock),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
diff --git a/net/core/filter.c b/net/core/filter.c
index 258821e58ec6..171941772a2e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11270,6 +11270,19 @@ const struct bpf_func_proto bpf_skc_to_unix_sock_proto = {
 	.ret_btf_id		= &btf_sock_ids[BTF_SOCK_TYPE_UNIX],
 };
 
+BPF_CALL_1(bpf_skc_to_mptcp_sock, struct sock *, sk)
+{
+	return (unsigned long)bpf_mptcp_sock_from_subflow(sk);
+}
+
+static const struct bpf_func_proto bpf_skc_to_mptcp_sock_proto = {
+	.func		= bpf_skc_to_mptcp_sock,
+	.gpl_only	= false,
+	.ret_type	= RET_PTR_TO_BTF_ID_OR_NULL,
+	.arg1_type	= ARG_PTR_TO_SOCK_COMMON,
+	.ret_btf_id	= &btf_sock_ids[BTF_SOCK_TYPE_MPTCP],
+};
+
 BPF_CALL_1(bpf_sock_from_file, struct file *, file)
 {
 	return (unsigned long)sock_from_file(file);
@@ -11312,6 +11325,11 @@ bpf_sk_base_func_proto(enum bpf_func_id func_id)
 	case BPF_FUNC_skc_to_unix_sock:
 		func = &bpf_skc_to_unix_sock_proto;
 		break;
+#ifdef CONFIG_MPTCP
+	case BPF_FUNC_skc_to_mptcp_sock:
+		func = &bpf_skc_to_mptcp_sock_proto;
+		break;
+#endif /* CONFIG_MPTCP */
 	case BPF_FUNC_ktime_get_coarse_ns:
 		return &bpf_ktime_get_coarse_ns_proto;
 	default:
diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
index 48a9d978aaeb..0a0608b6b4b4 100644
--- a/net/mptcp/Makefile
+++ b/net/mptcp/Makefile
@@ -11,3 +11,5 @@ obj-$(CONFIG_INET_MPTCP_DIAG) += mptcp_diag.o
 mptcp_crypto_test-objs := crypto_test.o
 mptcp_token_test-objs := token_test.o
 obj-$(CONFIG_MPTCP_KUNIT_TEST) += mptcp_crypto_test.o mptcp_token_test.o
+
+obj-$(CONFIG_BPF) += bpf.o
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
new file mode 100644
index 000000000000..535602ba2582
--- /dev/null
+++ b/net/mptcp/bpf.c
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Multipath TCP
+ *
+ * Copyright (c) 2020, Tessares SA.
+ * Copyright (c) 2022, SUSE.
+ *
+ * Author: Nicolas Rybowski <nicolas.rybowski@tessares.net>
+ */
+
+#define pr_fmt(fmt) "MPTCP: " fmt
+
+#include <linux/bpf.h>
+#include "protocol.h"
+
+struct mptcp_sock *bpf_mptcp_sock_from_subflow(struct sock *sk)
+{
+	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk))
+		return mptcp_sk(mptcp_subflow_ctx(sk)->conn);
+
+	return NULL;
+}
+EXPORT_SYMBOL(bpf_mptcp_sock_from_subflow);
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index 096625242475..0e5a9e69ae59 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -623,6 +623,7 @@ class PrinterHelpers(Printer):
             'struct udp6_sock',
             'struct unix_sock',
             'struct task_struct',
+            'struct mptcp_sock',
 
             'struct __sk_buff',
             'struct sk_msg_md',
@@ -682,6 +683,7 @@ class PrinterHelpers(Printer):
             'struct socket',
             'struct file',
             'struct bpf_timer',
+            'struct mptcp_sock',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 9ef1f3e1c22f..785f2cb15495 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5143,6 +5143,12 @@ union bpf_attr {
  *		The **hash_algo** is returned on success,
  *		**-EOPNOTSUP** if the hash calculation failed or **-EINVAL** if
  *		invalid arguments are passed.
+ *
+ * struct mptcp_sock *bpf_skc_to_mptcp_sock(void *sk)
+ *	Description
+ *		Dynamically cast a *sk* pointer to a *mptcp_sock* pointer.
+ *	Return
+ *		*sk* if casting is valid, or **NULL** otherwise.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5339,6 +5345,7 @@ union bpf_attr {
 	FN(copy_from_user_task),	\
 	FN(skb_set_tstamp),		\
 	FN(ima_file_hash),		\
+	FN(skc_to_mptcp_sock),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.34.1

[PATCH mptcp-next v12 2/5] Squash to "selftests: bpf: add MPTCP test base"

[Geliang Tang]

fix libbpf 0.8 build break:

prog_tests/mptcp.c:176:2: error: 'bpf_prog_load_xattr' is deprecated: libbpf v0.8+: use bpf_object__open() and bpf_object__load() instead [-Werror=deprecated-declarations]

Some cleanups.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 60 ++++++++++++-------
 tools/testing/selftests/bpf/progs/mptcp.c     | 16 ++---
 2 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 04aef0f147dc..5ec1cc4c2ef6 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -1,4 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020, Tessares SA. */
+
 #include <test_progs.h>
 #include "cgroup_helpers.h"
 #include "network_helpers.h"
@@ -13,12 +15,6 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
 	int err = 0, cfd = client_fd;
 	struct mptcp_storage val;
 
-	/* Currently there is no easy way to get back the subflow sk from the MPTCP
-	 * sk, thus we cannot access here the sk_storage associated to the subflow
-	 * sk. Also, there is no sk_storage associated with the MPTCP sk since it
-	 * does not trigger sockops events.
-	 * We silently pass this situation at the moment.
-	 */
 	if (is_mptcp == 1)
 		return 0;
 
@@ -28,14 +24,14 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
 	}
 
 	if (val.invoked != 1) {
-		log_err("%s: unexpected invoked count %d != %d",
-			msg, val.invoked, 1);
+		log_err("%s: unexpected invoked count %d != 1",
+			msg, val.invoked);
 		err++;
 	}
 
-	if (val.is_mptcp != is_mptcp) {
-		log_err("%s: unexpected bpf_tcp_sock.is_mptcp %d != %d",
-			msg, val.is_mptcp, is_mptcp);
+	if (val.is_mptcp != 0) {
+		log_err("%s: unexpected bpf_tcp_sock.is_mptcp %d != 0",
+			msg, val.is_mptcp);
 		err++;
 	}
 
@@ -44,28 +40,46 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
 
 static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 {
-	int client_fd, prog_fd, map_fd, err;
+	int client_fd, prog_fd, map_fd;
+	const char *file = "./mptcp.o";
+	struct bpf_program *prog;
 	struct bpf_object *obj;
 	struct bpf_map *map;
+	int err = 0;
 
-	struct bpf_prog_load_attr attr = {
-		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
-		.file = "./mptcp.o",
-		.expected_attach_type = BPF_CGROUP_SOCK_OPS,
-	};
+	obj = bpf_object__open(file);
+	if (libbpf_get_error(obj))
+		return -1;
 
-	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
+	err = bpf_object__load(obj);
 	if (err) {
 		log_err("Failed to load BPF object");
-		return -1;
+		err = -1;
+		goto close_bpf_object;
 	}
 
+	prog = bpf_object__next_program(obj, NULL);
+	if (!prog) {
+		log_err("Failed to get BPF program");
+		err = -1;
+		goto close_bpf_object;
+	}
+
+	prog_fd = bpf_program__fd(prog);
+
 	map = bpf_object__next_map(obj, NULL);
+	if (!map) {
+		log_err("Failed to get BPF map");
+		err = -1;
+		goto close_bpf_object;
+	}
+
 	map_fd = bpf_map__fd(map);
 
 	err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
 	if (err) {
 		log_err("Failed to attach BPF program");
+		err = -1;
 		goto close_bpf_object;
 	}
 
@@ -87,7 +101,7 @@ static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 	return err;
 }
 
-void test_mptcp(void)
+void test_base(void)
 {
 	int server_fd, cgroup_fd;
 
@@ -117,3 +131,9 @@ void test_mptcp(void)
 close_cgroup_fd:
 	close(cgroup_fd);
 }
+
+void test_mptcp(void)
+{
+	if (test__start_subtest("base"))
+		test_base();
+}
diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
index be5ee8dac2b3..0d65fb889d03 100644
--- a/tools/testing/selftests/bpf/progs/mptcp.c
+++ b/tools/testing/selftests/bpf/progs/mptcp.c
@@ -1,4 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2020, Tessares SA. */
+
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
@@ -25,22 +27,22 @@ int _sockops(struct bpf_sock_ops *ctx)
 	int op = (int)ctx->op;
 	struct bpf_sock *sk;
 
+	if (op != BPF_SOCK_OPS_TCP_CONNECT_CB)
+		return 1;
+
 	sk = ctx->sk;
 	if (!sk)
 		return 1;
 
+	tcp_sk = bpf_tcp_sock(sk);
+	if (!tcp_sk)
+		return 1;
+
 	storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
 				     BPF_SK_STORAGE_GET_F_CREATE);
 	if (!storage)
 		return 1;
 
-	if (op != BPF_SOCK_OPS_TCP_CONNECT_CB)
-		return 1;
-
-	tcp_sk = bpf_tcp_sock(sk);
-	if (!tcp_sk)
-		return 1;
-
 	storage->invoked++;
 	storage->is_mptcp = tcp_sk->is_mptcp;
 
-- 
2.34.1

Re: [PATCH mptcp-next v12 2/5] Squash to "selftests: bpf: add MPTCP test base"

[Matthieu Baerts]

Hi Geliang,

Thank you for this new version!

On 24/03/2022 14:28, Geliang Tang wrote:
> fix libbpf 0.8 build break:
> 
> prog_tests/mptcp.c:176:2: error: 'bpf_prog_load_xattr' is deprecated: libbpf v0.8+: use bpf_object__open() and bpf_object__load() instead [-Werror=deprecated-declarations]

These modifications seem good but while updating this test to sync with
what has done upstream recently, maybe it would also be good to use
macros that seem to be used in many places now, WDYT?
Please see below:

(...)

> @@ -44,28 +40,46 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
>  
>  static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
>  {
> -	int client_fd, prog_fd, map_fd, err;
> +	int client_fd, prog_fd, map_fd;
> +	const char *file = "./mptcp.o";
> +	struct bpf_program *prog;
>  	struct bpf_object *obj;
>  	struct bpf_map *map;
> +	int err = 0;
>  
> -	struct bpf_prog_load_attr attr = {
> -		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
> -		.file = "./mptcp.o",
> -		.expected_attach_type = BPF_CGROUP_SOCK_OPS,
> -	};
> +	obj = bpf_object__open(file);

Does bpf_object__open() not take two parameters?

> +	if (libbpf_get_error(obj))

I was surprised not to see a check 'obj != NULL', e.g.

  if (!ASSERT_OK_PTR(obj, "bpf_obj_open"))

like it is done in some other selftests but I see in other selftests
they use libbpf_get_error() like here.

I guess that's fine but I don't find a proper doc explaining what to use
here :)

I would still recommend to use ASSERT_OK_PTR(), at least to have a clear
error message.


> +		return -1;
>  
> -	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
> +	err = bpf_object__load(obj);
>  	if (err) {

Should you use ASSERT*() and CHECK() macros to include the 'log_err()'
below?

  if (!ASSERT_OK(err, "bpf_obj_load")) {

In 2020, these macro were not common but it looks like there are more
and more used now, e.g. check commit 186d1a86003d ("selftests/bpf:
Remove all the uses of deprecated bpf_prog_load_xattr()")

>  		log_err("Failed to load BPF object");
> -		return -1;
> +		err = -1;

I see some selftests returning "-EIO", maybe better here?
(same below)

> +		goto close_bpf_object;
>  	}
>  
> +	prog = bpf_object__next_program(obj, NULL);

It seems more common to use bpf_object__find_program_by_name() just in
case a new program is added later I guess. But maybe fine now?

> +	if (!prog) {> +		log_err("Failed to get BPF program");

Maybe good to use:

  CHECK(!prog, "find_prog", "mptcp prog not found\n")

> +		err = -1;
> +		goto close_bpf_object;
> +	}
> +
> +	prog_fd = bpf_program__fd(prog);
> +
>  	map = bpf_object__next_map(obj, NULL);

Similar here: maybe better to use bpf_object__find_map_by_name()?

> +	if (!map) {> +		log_err("Failed to get BPF map");

Maybe good to use CHECK() here?

> +		err = -1;
> +		goto close_bpf_object;
> +	}
> +
>  	map_fd = bpf_map__fd(map);
>  
>  	err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
>  	if (err) {
>  		log_err("Failed to attach BPF program");

And use CHECK() here as well?

> +		err = -1;

'err' should already be != 0 here. Do you need to set it explicitly to -1?


Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v12 2/5] Squash to "selftests: bpf: add MPTCP test base"

[Geliang Tang]

Hi Matt,

On Thu, Mar 24, 2022 at 06:05:03PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for this new version!
> 
> On 24/03/2022 14:28, Geliang Tang wrote:
> > fix libbpf 0.8 build break:
> > 
> > prog_tests/mptcp.c:176:2: error: 'bpf_prog_load_xattr' is deprecated: libbpf v0.8+: use bpf_object__open() and bpf_object__load() instead [-Werror=deprecated-declarations]
> 
> These modifications seem good but while updating this test to sync with
> what has done upstream recently, maybe it would also be good to use
> macros that seem to be used in many places now, WDYT?
> Please see below:
> 
> (...)
> 
> > @@ -44,28 +40,46 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
> >  
> >  static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
> >  {
> > -	int client_fd, prog_fd, map_fd, err;
> > +	int client_fd, prog_fd, map_fd;
> > +	const char *file = "./mptcp.o";
> > +	struct bpf_program *prog;
> >  	struct bpf_object *obj;
> >  	struct bpf_map *map;
> > +	int err = 0;
> >  
> > -	struct bpf_prog_load_attr attr = {
> > -		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
> > -		.file = "./mptcp.o",
> > -		.expected_attach_type = BPF_CGROUP_SOCK_OPS,
> > -	};
> > +	obj = bpf_object__open(file);
> 
> Does bpf_object__open() not take two parameters?

bpf_object__open_file() takes two parameters, this one takes one :)

> 
> > +	if (libbpf_get_error(obj))
> 
> I was surprised not to see a check 'obj != NULL', e.g.
> 
>   if (!ASSERT_OK_PTR(obj, "bpf_obj_open"))

This check had been done in libbpf_get_error().

> 
> like it is done in some other selftests but I see in other selftests
> they use libbpf_get_error() like here.
> 
> I guess that's fine but I don't find a proper doc explaining what to use
> here :)
> 
> I would still recommend to use ASSERT_OK_PTR(), at least to have a clear
> error message.
> 
> 
> > +		return -1;
> >  
> > -	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
> > +	err = bpf_object__load(obj);
> >  	if (err) {
> 
> Should you use ASSERT*() and CHECK() macros to include the 'log_err()'
> below?
> 
>   if (!ASSERT_OK(err, "bpf_obj_load")) {
> 
> In 2020, these macro were not common but it looks like there are more
> and more used now, e.g. check commit 186d1a86003d ("selftests/bpf:
> Remove all the uses of deprecated bpf_prog_load_xattr()")

Updated in v13.

> 
> >  		log_err("Failed to load BPF object");
> > -		return -1;
> > +		err = -1;
> 
> I see some selftests returning "-EIO", maybe better here?
> (same below)

Yes, -EIO is much better, updated in v13.

> 
> > +		goto close_bpf_object;
> >  	}
> >  
> > +	prog = bpf_object__next_program(obj, NULL);
> 
> It seems more common to use bpf_object__find_program_by_name() just in
> case a new program is added later I guess. But maybe fine now?

Agree, updated in v13.

> 
> > +	if (!prog) {> +		log_err("Failed to get BPF program");
> 
> Maybe good to use:
> 
>   CHECK(!prog, "find_prog", "mptcp prog not found\n")
> 
> > +		err = -1;
> > +		goto close_bpf_object;
> > +	}
> > +
> > +	prog_fd = bpf_program__fd(prog);
> > +
> >  	map = bpf_object__next_map(obj, NULL);
> 
> Similar here: maybe better to use bpf_object__find_map_by_name()?

Updated in v13.

> 
> > +	if (!map) {> +		log_err("Failed to get BPF map");
> 
> Maybe good to use CHECK() here?

Yes.

> 
> > +		err = -1;
> > +		goto close_bpf_object;
> > +	}
> > +
> >  	map_fd = bpf_map__fd(map);
> >  
> >  	err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
> >  	if (err) {
> >  		log_err("Failed to attach BPF program");
> 
> And use CHECK() here as well?

Yes.

> 
> > +		err = -1;
> 
> 'err' should already be != 0 here. Do you need to set it explicitly to -1?

No, updated in v13.

Thanks,
-Geliang

> 
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

[PATCH mptcp-next v12 3/5] selftests: bpf: test bpf_skc_to_mptcp_sock

[Geliang Tang]

This patch extended the MPTCP test base, to test the new helper
bpf_skc_to_mptcp_sock().

Added a new function verify_msk() to verify the msk token, and a new
function get_msk_token() to parse the msk token from the output of the
command 'ip mptcp monitor'.

Acked-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |   6 ++
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 102 ++++++++++++++++--
 tools/testing/selftests/bpf/progs/mptcp.c     |  25 ++++-
 3 files changed, 123 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
index b1ede6f0b821..9ea6687c8f4d 100644
--- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
@@ -83,6 +83,12 @@ struct tcp_sock {
 	__u64	tcp_mstamp;	/* most recent packet received/sent */
 } __attribute__((preserve_access_index));
 
+struct mptcp_sock {
+	struct inet_connection_sock	sk;
+
+	__u32		token;
+} __attribute__((preserve_access_index));
+
 static __always_inline struct inet_connection_sock *inet_csk(const struct sock *sk)
 {
 	return (struct inet_connection_sock *)sk;
diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 5ec1cc4c2ef6..770104dc5448 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -8,16 +8,17 @@
 struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
+	__u32 token;
 };
 
-static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
+static char monitor_log_path[64];
+
+static int verify_tsk(int map_fd, int client_fd)
 {
+	char *msg = "plain TCP socket";
 	int err = 0, cfd = client_fd;
 	struct mptcp_storage val;
 
-	if (is_mptcp == 1)
-		return 0;
-
 	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
 		perror("Failed to read socket storage");
 		return -1;
@@ -38,6 +39,85 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
 	return err;
 }
 
+/*
+ * Parse the token from the output of 'ip mptcp monitor':
+ *
+ * [       CREATED] token=3ca933d3 remid=0 locid=0 saddr4=127.0.0.1 ...
+ * [       CREATED] token=2ab57040 remid=0 locid=0 saddr4=127.0.0.1 ...
+ */
+static __u32 get_msk_token(void)
+{
+	char *prefix = "[       CREATED] token=";
+	char buf[BUFSIZ] = {};
+	__u32 token = 0;
+	ssize_t len;
+	int fd;
+
+	sync();
+
+	fd = open(monitor_log_path, O_RDONLY);
+	if (CHECK_FAIL(fd < 0)) {
+		log_err("Failed to open %s", monitor_log_path);
+		return token;
+	}
+
+	len = read(fd, buf, sizeof(buf));
+	if (CHECK_FAIL(len < 0)) {
+		log_err("Failed to read %s", monitor_log_path);
+		goto err;
+	}
+
+	if (strncmp(buf, prefix, strlen(prefix))) {
+		log_err("Invalid prefix %s", buf);
+		goto err;
+	}
+
+	token = strtol(buf + strlen(prefix), NULL, 16);
+
+err:
+	close(fd);
+	return token;
+}
+
+static int verify_msk(int map_fd, int client_fd)
+{
+	char *msg = "MPTCP subflow socket";
+	int err = 0, cfd = client_fd;
+	struct mptcp_storage val;
+	__u32 token;
+
+	token = get_msk_token();
+	if (token <= 0) {
+		log_err("Unexpected token %x", token);
+		return -1;
+	}
+
+	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
+		perror("Failed to read socket storage");
+		return -1;
+	}
+
+	if (val.invoked != 1) {
+		log_err("%s: unexpected invoked count %d != 1",
+			msg, val.invoked);
+		err++;
+	}
+
+	if (val.is_mptcp != 1) {
+		log_err("%s: unexpected bpf_tcp_sock.is_mptcp %d != 1",
+			msg, val.is_mptcp);
+		err++;
+	}
+
+	if (val.token != token) {
+		log_err("Unexpected mptcp_sock.token %x != %x",
+			val.token, token);
+		err++;
+	}
+
+	return err;
+}
+
 static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 {
 	int client_fd, prog_fd, map_fd;
@@ -90,8 +170,8 @@ static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 		goto close_client_fd;
 	}
 
-	err += is_mptcp ? verify_sk(map_fd, client_fd, "MPTCP subflow socket", 1) :
-			  verify_sk(map_fd, client_fd, "plain TCP socket", 0);
+	err += is_mptcp ? verify_msk(map_fd, client_fd) :
+			  verify_tsk(map_fd, client_fd);
 
 close_client_fd:
 	close(client_fd);
@@ -103,6 +183,7 @@ static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
 
 void test_base(void)
 {
+	char cmd[256], tmp_dir[] = "/tmp/XXXXXX";
 	int server_fd, cgroup_fd;
 
 	cgroup_fd = test__join_cgroup("/mptcp");
@@ -120,6 +201,13 @@ void test_base(void)
 
 with_mptcp:
 	/* with MPTCP */
+	if (CHECK_FAIL(!mkdtemp(tmp_dir)))
+		goto close_cgroup_fd;
+	snprintf(monitor_log_path, sizeof(monitor_log_path),
+		 "%s/ip_mptcp_monitor", tmp_dir);
+	snprintf(cmd, sizeof(cmd), "ip mptcp monitor > %s &", monitor_log_path);
+	if (CHECK_FAIL(system(cmd)))
+		goto close_cgroup_fd;
 	server_fd = start_mptcp_server(AF_INET, NULL, 0, 0);
 	if (CHECK_FAIL(server_fd < 0))
 		goto close_cgroup_fd;
@@ -127,6 +215,8 @@ void test_base(void)
 	CHECK_FAIL(run_test(cgroup_fd, server_fd, true));
 
 	close(server_fd);
+	snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp_dir);
+	system(cmd);
 
 close_cgroup_fd:
 	close(cgroup_fd);
diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
index 0d65fb889d03..afacea5ad9ca 100644
--- a/tools/testing/selftests/bpf/progs/mptcp.c
+++ b/tools/testing/selftests/bpf/progs/mptcp.c
@@ -3,6 +3,7 @@
 
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
+#include "bpf_tcp_helpers.h"
 
 char _license[] SEC("license") = "GPL";
 __u32 _version SEC("version") = 1;
@@ -10,6 +11,7 @@ __u32 _version SEC("version") = 1;
 struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
+	__u32 token;
 };
 
 struct {
@@ -24,6 +26,7 @@ int _sockops(struct bpf_sock_ops *ctx)
 {
 	struct mptcp_storage *storage;
 	struct bpf_tcp_sock *tcp_sk;
+	struct mptcp_sock *msk;
 	int op = (int)ctx->op;
 	struct bpf_sock *sk;
 
@@ -38,11 +41,25 @@ int _sockops(struct bpf_sock_ops *ctx)
 	if (!tcp_sk)
 		return 1;
 
-	storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
-				     BPF_SK_STORAGE_GET_F_CREATE);
-	if (!storage)
-		return 1;
+	if (!tcp_sk->is_mptcp) {
+		storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
+					     BPF_SK_STORAGE_GET_F_CREATE);
+		if (!storage)
+			return 1;
+
+		storage->token = 0;
+	} else {
+		msk = bpf_skc_to_mptcp_sock(sk);
+		if (!msk)
+			return 1;
+
+		storage = bpf_sk_storage_get(&socket_storage_map, msk, 0,
+					     BPF_SK_STORAGE_GET_F_CREATE);
+		if (!storage)
+			return 1;
 
+		storage->token = msk->token;
+	}
 	storage->invoked++;
 	storage->is_mptcp = tcp_sk->is_mptcp;
 
-- 
2.34.1

[PATCH mptcp-next v12 4/5] selftests: bpf: verify ca_name of struct mptcp_sock

[Geliang Tang]

This patch verified another member of struct mptcp_sock, ca_name. Added a
new function get_msk_ca_name() to read the sysctl tcp_congestion_control
and verified it in verify_msk().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |  5 ++++
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 24 +++++++++++++++++++
 tools/testing/selftests/bpf/progs/mptcp.c     |  4 ++++
 3 files changed, 33 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
index 9ea6687c8f4d..bebe382bcc7a 100644
--- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
@@ -16,6 +16,10 @@ BPF_PROG(name, args)
 #define SOL_TCP 6
 #endif
 
+#ifndef TCP_CA_NAME_MAX
+#define TCP_CA_NAME_MAX	16
+#endif
+
 #define tcp_jiffies32 ((__u32)bpf_jiffies64())
 
 struct sock_common {
@@ -87,6 +91,7 @@ struct mptcp_sock {
 	struct inet_connection_sock	sk;
 
 	__u32		token;
+	char		ca_name[TCP_CA_NAME_MAX];
 } __attribute__((preserve_access_index));
 
 static __always_inline struct inet_connection_sock *inet_csk(const struct sock *sk)
diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 770104dc5448..32425f9cd76c 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -5,10 +5,15 @@
 #include "cgroup_helpers.h"
 #include "network_helpers.h"
 
+#ifndef TCP_CA_NAME_MAX
+#define TCP_CA_NAME_MAX	16
+#endif
+
 struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
 	__u32 token;
+	char ca_name[TCP_CA_NAME_MAX];
 };
 
 static char monitor_log_path[64];
@@ -79,11 +84,22 @@ static __u32 get_msk_token(void)
 	return token;
 }
 
+void get_msk_ca_name(char ca_name[])
+{
+	FILE *stream = popen("sysctl -b net.ipv4.tcp_congestion_control", "r");
+
+	if (fgets(ca_name, TCP_CA_NAME_MAX, stream) == NULL)
+		log_err("Failed to read ca_name");
+
+	pclose(stream);
+}
+
 static int verify_msk(int map_fd, int client_fd)
 {
 	char *msg = "MPTCP subflow socket";
 	int err = 0, cfd = client_fd;
 	struct mptcp_storage val;
+	char ca_name[TCP_CA_NAME_MAX];
 	__u32 token;
 
 	token = get_msk_token();
@@ -92,6 +108,8 @@ static int verify_msk(int map_fd, int client_fd)
 		return -1;
 	}
 
+	get_msk_ca_name(ca_name);
+
 	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
 		perror("Failed to read socket storage");
 		return -1;
@@ -115,6 +133,12 @@ static int verify_msk(int map_fd, int client_fd)
 		err++;
 	}
 
+	if (strncmp(val.ca_name, ca_name, TCP_CA_NAME_MAX)) {
+		log_err("Unexpected mptcp_sock.ca_name %s != %s",
+			val.ca_name, ca_name);
+		err++;
+	}
+
 	return err;
 }
 
diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
index afacea5ad9ca..6e7d2abf6ce3 100644
--- a/tools/testing/selftests/bpf/progs/mptcp.c
+++ b/tools/testing/selftests/bpf/progs/mptcp.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2020, Tessares SA. */
 
+#include <string.h>
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 #include "bpf_tcp_helpers.h"
@@ -12,6 +13,7 @@ struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
 	__u32 token;
+	char ca_name[TCP_CA_NAME_MAX];
 };
 
 struct {
@@ -48,6 +50,7 @@ int _sockops(struct bpf_sock_ops *ctx)
 			return 1;
 
 		storage->token = 0;
+		bzero(storage->ca_name, TCP_CA_NAME_MAX);
 	} else {
 		msk = bpf_skc_to_mptcp_sock(sk);
 		if (!msk)
@@ -59,6 +62,7 @@ int _sockops(struct bpf_sock_ops *ctx)
 			return 1;
 
 		storage->token = msk->token;
+		memcpy(storage->ca_name, msk->ca_name, TCP_CA_NAME_MAX);
 	}
 	storage->invoked++;
 	storage->is_mptcp = tcp_sk->is_mptcp;
-- 
2.34.1

[PATCH mptcp-next v12 5/5] selftests: bpf: verify first subflow of mptcp_sock

[Geliang Tang]

This patch verified the 'first' struct member of struct mptcp_sock, which
pointed to the first subflow of msk. Saved 'sk' in mptcp_storage, and
verified it with 'first' in verify_msk().

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/bpf_tcp_helpers.h  | 1 +
 tools/testing/selftests/bpf/prog_tests/mptcp.c | 8 ++++++++
 tools/testing/selftests/bpf/progs/mptcp.c      | 5 +++++
 3 files changed, 14 insertions(+)

diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
index bebe382bcc7a..f92357597e63 100644
--- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
@@ -91,6 +91,7 @@ struct mptcp_sock {
 	struct inet_connection_sock	sk;
 
 	__u32		token;
+	struct sock	*first;
 	char		ca_name[TCP_CA_NAME_MAX];
 } __attribute__((preserve_access_index));
 
diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 32425f9cd76c..b7c230c4efef 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -12,7 +12,9 @@
 struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
+	struct sock *sk;
 	__u32 token;
+	struct sock *first;
 	char ca_name[TCP_CA_NAME_MAX];
 };
 
@@ -133,6 +135,12 @@ static int verify_msk(int map_fd, int client_fd)
 		err++;
 	}
 
+	if (val.first != val.sk) {
+		log_err("Unexpected mptcp_sock.first %p != %p",
+			val.first, val.sk);
+		err++;
+	}
+
 	if (strncmp(val.ca_name, ca_name, TCP_CA_NAME_MAX)) {
 		log_err("Unexpected mptcp_sock.ca_name %s != %s",
 			val.ca_name, ca_name);
diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
index 6e7d2abf6ce3..16055f553f29 100644
--- a/tools/testing/selftests/bpf/progs/mptcp.c
+++ b/tools/testing/selftests/bpf/progs/mptcp.c
@@ -12,7 +12,9 @@ __u32 _version SEC("version") = 1;
 struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
+	struct sock *sk;
 	__u32 token;
+	struct sock *first;
 	char ca_name[TCP_CA_NAME_MAX];
 };
 
@@ -51,6 +53,7 @@ int _sockops(struct bpf_sock_ops *ctx)
 
 		storage->token = 0;
 		bzero(storage->ca_name, TCP_CA_NAME_MAX);
+		storage->first = NULL;
 	} else {
 		msk = bpf_skc_to_mptcp_sock(sk);
 		if (!msk)
@@ -63,9 +66,11 @@ int _sockops(struct bpf_sock_ops *ctx)
 
 		storage->token = msk->token;
 		memcpy(storage->ca_name, msk->ca_name, TCP_CA_NAME_MAX);
+		storage->first = msk->first;
 	}
 	storage->invoked++;
 	storage->is_mptcp = tcp_sk->is_mptcp;
+	storage->sk = (struct sock *)sk;
 
 	return 1;
 }
-- 
2.34.1

Re: selftests: bpf: verify first subflow of mptcp_sock: Tests Results

[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! ✅:
  - Task: https://cirrus-ci.com/task/5025009943969792
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5025009943969792/summary/summary.txt

- KVM Validation: debug:
  - Unstable: 3 failed test(s): packetdrill_sockopts selftest_diag selftest_mptcp_join 🔴:
  - Task: https://cirrus-ci.com/task/6150909850812416
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6150909850812416/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/261123c9bf65

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 (Tessares)