[PATCH mptcp-next v6 0/4] add skc_to_mptcp_sock

[PATCH mptcp-next v6 0/4] add skc_to_mptcp_sock

[Geliang Tang]

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 (4):
  Revert "selftests: bpf: add bpf_mptcp_sock() verifier tests"
  Revert "bpf: add 'bpf_mptcp_sock' structure and helper"
  bpf: add skc_to_mptcp_sock helper
  selftests: bpf: add skc_to_mptcp_sock test

 include/linux/bpf.h                           | 31 +--------
 include/linux/btf_ids.h                       |  3 +-
 include/uapi/linux/bpf.h                      | 14 ++--
 kernel/bpf/verifier.c                         | 20 ------
 net/core/filter.c                             | 21 ++++--
 net/mptcp/bpf.c                               | 67 ++-----------------
 scripts/bpf_doc.py                            |  4 +-
 tools/include/uapi/linux/bpf.h                | 14 ++--
 tools/testing/selftests/bpf/bpf_tcp_helpers.h |  6 ++
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 27 ++++++--
 tools/testing/selftests/bpf/progs/mptcp.c     | 23 +++++++
 tools/testing/selftests/bpf/verifier/sock.c   | 63 -----------------
 12 files changed, 89 insertions(+), 204 deletions(-)

-- 
2.34.1

[PATCH mptcp-next v6 1/4] Revert "selftests: bpf: add bpf_mptcp_sock() verifier tests"

[Geliang Tang]

This reverts commit f48f0143286db0d8dbded9542d9a2535533dd3b9.
---
 tools/testing/selftests/bpf/verifier/sock.c | 63 ---------------------
 1 file changed, 63 deletions(-)

diff --git a/tools/testing/selftests/bpf/verifier/sock.c b/tools/testing/selftests/bpf/verifier/sock.c
index 1f85588091ab..8c224eac93df 100644
--- a/tools/testing/selftests/bpf/verifier/sock.c
+++ b/tools/testing/selftests/bpf/verifier/sock.c
@@ -731,66 +731,3 @@
 	.result_unpriv = REJECT,
 	.errstr_unpriv = "unknown func",
 },
-{
-	"bpf_mptcp_sock(skops->sk): no !skops->sk check",
-	.insns = {
-	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, offsetof(struct bpf_sock_ops, sk)),
-	BPF_EMIT_CALL(BPF_FUNC_mptcp_sock),
-	BPF_MOV64_IMM(BPF_REG_0, 0),
-	BPF_EXIT_INSN(),
-	},
-	.prog_type = BPF_PROG_TYPE_SOCK_OPS,
-	.result = REJECT,
-	.errstr = "type=sock_or_null expected=sock_common",
-},
-{
-	"bpf_mptcp_sock(skops->sk): no NULL check on ret",
-	.insns = {
-	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, offsetof(struct bpf_sock_ops, sk)),
-	BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 2),
-	BPF_MOV64_IMM(BPF_REG_0, 0),
-	BPF_EXIT_INSN(),
-	BPF_EMIT_CALL(BPF_FUNC_mptcp_sock),
-	BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, offsetof(struct bpf_mptcp_sock, token)),
-	BPF_MOV64_IMM(BPF_REG_0, 0),
-	BPF_EXIT_INSN(),
-	},
-	.prog_type = BPF_PROG_TYPE_SOCK_OPS,
-	.result = REJECT,
-	.errstr = "invalid mem access 'mptcp_sock_or_null'",
-},
-{
-	"bpf_mptcp_sock(skops->sk): msk->token",
-	.insns = {
-	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, offsetof(struct bpf_sock_ops, sk)),
-	BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 2),
-	BPF_MOV64_IMM(BPF_REG_0, 0),
-	BPF_EXIT_INSN(),
-	BPF_EMIT_CALL(BPF_FUNC_mptcp_sock),
-	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
-	BPF_EXIT_INSN(),
-	BPF_LDX_MEM(BPF_W, BPF_REG_0, BPF_REG_0, offsetof(struct bpf_mptcp_sock, token)),
-	BPF_MOV64_IMM(BPF_REG_0, 0),
-	BPF_EXIT_INSN(),
-	},
-	.prog_type = BPF_PROG_TYPE_SOCK_OPS,
-	.result = ACCEPT,
-},
-{
-	"bpf_mptcp_sock(skops->sk): msk->token cannot be modified",
-	.insns = {
-	BPF_LDX_MEM(BPF_DW, BPF_REG_1, BPF_REG_1, offsetof(struct bpf_sock_ops, sk)),
-	BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, 2),
-	BPF_MOV64_IMM(BPF_REG_0, 0),
-	BPF_EXIT_INSN(),
-	BPF_EMIT_CALL(BPF_FUNC_mptcp_sock),
-	BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 1),
-	BPF_EXIT_INSN(),
-	BPF_ST_MEM(BPF_W, BPF_REG_0, offsetof(struct bpf_mptcp_sock, token), 0x2a),
-	BPF_MOV64_IMM(BPF_REG_0, 0),
-	BPF_EXIT_INSN(),
-	},
-	.prog_type = BPF_PROG_TYPE_SOCK_OPS,
-	.result = REJECT,
-	.errstr = "cannot write into mptcp_sock",
-},
-- 
2.34.1

[PATCH mptcp-next v6 2/4] Revert "bpf: add 'bpf_mptcp_sock' structure and helper"

[Geliang Tang]

This reverts commit 2ee22ce38ed4c7a416ce887eb6379aaf157634f9.
---
 include/linux/bpf.h            | 34 ----------------
 include/uapi/linux/bpf.h       | 13 ------
 kernel/bpf/verifier.c          | 20 ----------
 net/core/filter.c              |  4 --
 net/mptcp/Makefile             |  2 -
 net/mptcp/bpf.c                | 72 ----------------------------------
 scripts/bpf_doc.py             |  2 -
 tools/include/uapi/linux/bpf.h | 13 ------
 8 files changed, 160 deletions(-)
 delete mode 100644 net/mptcp/bpf.c

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5a17c1e3a6bc..f19abc59b6cd 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -407,7 +407,6 @@ enum bpf_return_type {
 	RET_PTR_TO_MAP_VALUE,		/* returns a pointer to map elem value */
 	RET_PTR_TO_SOCKET,		/* returns a pointer to a socket */
 	RET_PTR_TO_TCP_SOCK,		/* returns a pointer to a tcp_sock */
-	RET_PTR_TO_MPTCP_SOCK,		/* returns a pointer to mptcp_sock */
 	RET_PTR_TO_SOCK_COMMON,		/* returns a pointer to a sock_common */
 	RET_PTR_TO_ALLOC_MEM,		/* returns a pointer to dynamically allocated memory */
 	RET_PTR_TO_MEM_OR_BTF_ID,	/* returns a pointer to a valid memory or a btf_id */
@@ -418,7 +417,6 @@ enum bpf_return_type {
 	RET_PTR_TO_MAP_VALUE_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_MAP_VALUE,
 	RET_PTR_TO_SOCKET_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_SOCKET,
 	RET_PTR_TO_TCP_SOCK_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_TCP_SOCK,
-	RET_PTR_TO_MPTCP_SOCK_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_MPTCP_SOCK,
 	RET_PTR_TO_SOCK_COMMON_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_SOCK_COMMON,
 	RET_PTR_TO_ALLOC_MEM_OR_NULL	= PTR_MAYBE_NULL | MEM_ALLOC | RET_PTR_TO_ALLOC_MEM,
 	RET_PTR_TO_BTF_ID_OR_NULL	= PTR_MAYBE_NULL | RET_PTR_TO_BTF_ID,
@@ -499,7 +497,6 @@ enum bpf_reg_type {
 	PTR_TO_SOCKET,		 /* reg points to struct bpf_sock */
 	PTR_TO_SOCK_COMMON,	 /* reg points to sock_common */
 	PTR_TO_TCP_SOCK,	 /* reg points to struct tcp_sock */
-	PTR_TO_MPTCP_SOCK,	 /* reg points to struct mptcp_sock */
 	PTR_TO_TP_BUFFER,	 /* reg points to a writable raw tp's buffer */
 	PTR_TO_XDP_SOCK,	 /* reg points to struct xdp_sock */
 	/* PTR_TO_BTF_ID points to a kernel struct that does not need
@@ -528,7 +525,6 @@ enum bpf_reg_type {
 	PTR_TO_SOCKET_OR_NULL		= PTR_MAYBE_NULL | PTR_TO_SOCKET,
 	PTR_TO_SOCK_COMMON_OR_NULL	= PTR_MAYBE_NULL | PTR_TO_SOCK_COMMON,
 	PTR_TO_TCP_SOCK_OR_NULL		= PTR_MAYBE_NULL | PTR_TO_TCP_SOCK,
-	PTR_TO_MPTCP_SOCK_OR_NULL	= PTR_MAYBE_NULL | PTR_TO_MPTCP_SOCK,
 	PTR_TO_BTF_ID_OR_NULL		= PTR_MAYBE_NULL | PTR_TO_BTF_ID,
 
 	/* This must be the last entry. Its purpose is to ensure the enum is
@@ -2221,7 +2217,6 @@ extern const struct bpf_func_proto bpf_get_local_storage_proto;
 extern const struct bpf_func_proto bpf_strtol_proto;
 extern const struct bpf_func_proto bpf_strtoul_proto;
 extern const struct bpf_func_proto bpf_tcp_sock_proto;
-extern const struct bpf_func_proto bpf_mptcp_sock_proto;
 extern const struct bpf_func_proto bpf_jiffies64_proto;
 extern const struct bpf_func_proto bpf_get_ns_current_pid_tgid_proto;
 extern const struct bpf_func_proto bpf_event_output_data_proto;
@@ -2309,7 +2304,6 @@ struct sk_reuseport_kern {
 	u32 reuseport_id;
 	bool bind_inany;
 };
-
 bool bpf_tcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
 				  struct bpf_insn_access_aux *info);
 
@@ -2360,34 +2354,6 @@ static inline u32 bpf_xdp_sock_convert_ctx_access(enum bpf_access_type type,
 }
 #endif /* CONFIG_INET */
 
-#ifdef CONFIG_MPTCP
-bool bpf_mptcp_sock_is_valid_access(int off, int size,
-				    enum bpf_access_type type,
-				    struct bpf_insn_access_aux *info);
-
-u32 bpf_mptcp_sock_convert_ctx_access(enum bpf_access_type type,
-				      const struct bpf_insn *si,
-				      struct bpf_insn *insn_buf,
-				      struct bpf_prog *prog,
-				      u32 *target_size);
-#else /* CONFIG_MPTCP */
-static inline bool bpf_mptcp_sock_is_valid_access(int off, int size,
-						  enum bpf_access_type type,
-						  struct bpf_insn_access_aux *info)
-{
-	return false;
-}
-
-static inline u32 bpf_mptcp_sock_convert_ctx_access(enum bpf_access_type type,
-						    const struct bpf_insn *si,
-						    struct bpf_insn *insn_buf,
-						    struct bpf_prog *prog,
-						    u32 *target_size)
-{
-	return 0;
-}
-#endif /* CONFIG_MPTCP */
-
 enum bpf_text_poke_type {
 	BPF_MOD_CALL,
 	BPF_MOD_JUMP,
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f2e5c7bd4f10..3f6dcdf4b915 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5086,14 +5086,6 @@ union bpf_attr {
  *	Return
  *		0 on success, or a negative error in case of failure. On error
  *		*dst* buffer is zeroed out.
- *
- * struct bpf_mptcp_sock *bpf_mptcp_sock(struct bpf_sock *sk)
- *	Description
- *		This helper gets a **struct bpf_mptcp_sock** pointer from a
- *		**struct bpf_sock** pointer.
- *	Return
- *		A **struct bpf_mptcp_sock** pointer on success, or **NULL** in
- *		case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5288,7 +5280,6 @@ union bpf_attr {
 	FN(xdp_load_bytes),		\
 	FN(xdp_store_bytes),		\
 	FN(copy_from_user_task),	\
-	FN(mptcp_sock),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
@@ -5637,10 +5628,6 @@ struct bpf_tcp_sock {
 	__u32 is_mptcp;		/* Is MPTCP subflow? */
 };
 
-struct bpf_mptcp_sock {
-	__u32 token;		/* msk token */
-};
-
 struct bpf_sock_tuple {
 	union {
 		struct {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 1bb1b5d0419d..d7473fee247c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -430,7 +430,6 @@ static bool type_is_sk_pointer(enum bpf_reg_type type)
 	return type == PTR_TO_SOCKET ||
 		type == PTR_TO_SOCK_COMMON ||
 		type == PTR_TO_TCP_SOCK ||
-		type == PTR_TO_MPTCP_SOCK ||
 		type == PTR_TO_XDP_SOCK;
 }
 
@@ -438,7 +437,6 @@ static bool reg_type_not_null(enum bpf_reg_type type)
 {
 	return type == PTR_TO_SOCKET ||
 		type == PTR_TO_TCP_SOCK ||
-		type == PTR_TO_MPTCP_SOCK ||
 		type == PTR_TO_MAP_VALUE ||
 		type == PTR_TO_MAP_KEY ||
 		type == PTR_TO_SOCK_COMMON;
@@ -454,7 +452,6 @@ static bool reg_type_may_be_refcounted_or_null(enum bpf_reg_type type)
 {
 	return base_type(type) == PTR_TO_SOCKET ||
 		base_type(type) == PTR_TO_TCP_SOCK ||
-		base_type(type) == PTR_TO_MPTCP_SOCK ||
 		base_type(type) == PTR_TO_MEM ||
 		base_type(type) == PTR_TO_BTF_ID;
 }
@@ -554,7 +551,6 @@ static const char *reg_type_str(struct bpf_verifier_env *env,
 		[PTR_TO_SOCKET]		= "sock",
 		[PTR_TO_SOCK_COMMON]	= "sock_common",
 		[PTR_TO_TCP_SOCK]	= "tcp_sock",
-		[PTR_TO_MPTCP_SOCK]	= "mptcp_sock",
 		[PTR_TO_TP_BUFFER]	= "tp_buffer",
 		[PTR_TO_XDP_SOCK]	= "xdp_sock",
 		[PTR_TO_BTF_ID]		= "ptr_",
@@ -2778,7 +2774,6 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
 	case PTR_TO_SOCKET:
 	case PTR_TO_SOCK_COMMON:
 	case PTR_TO_TCP_SOCK:
-	case PTR_TO_MPTCP_SOCK:
 	case PTR_TO_XDP_SOCK:
 	case PTR_TO_BTF_ID:
 	case PTR_TO_BUF:
@@ -3671,9 +3666,6 @@ static int check_sock_access(struct bpf_verifier_env *env, int insn_idx,
 	case PTR_TO_TCP_SOCK:
 		valid = bpf_tcp_sock_is_valid_access(off, size, t, &info);
 		break;
-	case PTR_TO_MPTCP_SOCK:
-		valid = bpf_mptcp_sock_is_valid_access(off, size, t, &info);
-		break;
 	case PTR_TO_XDP_SOCK:
 		valid = bpf_xdp_sock_is_valid_access(off, size, t, &info);
 		break;
@@ -3830,9 +3822,6 @@ static int check_ptr_alignment(struct bpf_verifier_env *env,
 	case PTR_TO_TCP_SOCK:
 		pointer_desc = "tcp_sock ";
 		break;
-	case PTR_TO_MPTCP_SOCK:
-		pointer_desc = "mptcp_sock ";
-		break;
 	case PTR_TO_XDP_SOCK:
 		pointer_desc = "xdp_sock ";
 		break;
@@ -6762,9 +6751,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 	} else if (base_type(ret_type) == RET_PTR_TO_TCP_SOCK) {
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].type = PTR_TO_TCP_SOCK | ret_flag;
-	} else if (base_type(ret_type) == RET_PTR_TO_MPTCP_SOCK) {
-		mark_reg_known_zero(env, regs, BPF_REG_0);
-		regs[BPF_REG_0].type = PTR_TO_MPTCP_SOCK | ret_flag;
 	} else if (base_type(ret_type) == RET_PTR_TO_ALLOC_MEM) {
 		mark_reg_known_zero(env, regs, BPF_REG_0);
 		regs[BPF_REG_0].type = PTR_TO_MEM | ret_flag;
@@ -7479,7 +7465,6 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 	case PTR_TO_SOCKET:
 	case PTR_TO_SOCK_COMMON:
 	case PTR_TO_TCP_SOCK:
-	case PTR_TO_MPTCP_SOCK:
 	case PTR_TO_XDP_SOCK:
 		verbose(env, "R%d pointer arithmetic on %s prohibited\n",
 			dst, reg_type_str(env, ptr_reg->type));
@@ -10854,7 +10839,6 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold,
 	case PTR_TO_SOCKET:
 	case PTR_TO_SOCK_COMMON:
 	case PTR_TO_TCP_SOCK:
-	case PTR_TO_MPTCP_SOCK:
 	case PTR_TO_XDP_SOCK:
 		/* Only valid matches are exact, which memcmp() above
 		 * would have accepted
@@ -11385,7 +11369,6 @@ static bool reg_type_mismatch_ok(enum bpf_reg_type type)
 	case PTR_TO_SOCKET:
 	case PTR_TO_SOCK_COMMON:
 	case PTR_TO_TCP_SOCK:
-	case PTR_TO_MPTCP_SOCK:
 	case PTR_TO_XDP_SOCK:
 	case PTR_TO_BTF_ID:
 		return false;
@@ -12810,9 +12793,6 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		case PTR_TO_TCP_SOCK:
 			convert_ctx_access = bpf_tcp_sock_convert_ctx_access;
 			break;
-		case PTR_TO_MPTCP_SOCK:
-			convert_ctx_access = bpf_mptcp_sock_convert_ctx_access;
-			break;
 		case PTR_TO_XDP_SOCK:
 			convert_ctx_access = bpf_xdp_sock_convert_ctx_access;
 			break;
diff --git a/net/core/filter.c b/net/core/filter.c
index a07b28997ad3..f64454722a7e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7850,10 +7850,6 @@ sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 	case BPF_FUNC_tcp_sock:
 		return &bpf_tcp_sock_proto;
 #endif /* CONFIG_INET */
-#ifdef CONFIG_MPTCP
-	case BPF_FUNC_mptcp_sock:
-		return &bpf_mptcp_sock_proto;
-#endif /* CONFIG_MPTCP */
 	default:
 		return bpf_sk_base_func_proto(func_id);
 	}
diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
index 0a0608b6b4b4..48a9d978aaeb 100644
--- a/net/mptcp/Makefile
+++ b/net/mptcp/Makefile
@@ -11,5 +11,3 @@ 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
deleted file mode 100644
index 5332469fbb28..000000000000
--- a/net/mptcp/bpf.c
+++ /dev/null
@@ -1,72 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/* Multipath TCP
- *
- * Copyright (c) 2020, Tessares SA.
- *
- * Author: Nicolas Rybowski <nicolas.rybowski@tessares.net>
- *
- */
-
-#include <linux/bpf.h>
-
-#include "protocol.h"
-
-bool bpf_mptcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
-				    struct bpf_insn_access_aux *info)
-{
-	if (off < 0 || off >= offsetofend(struct bpf_mptcp_sock, token))
-		return false;
-
-	if (off % size != 0)
-		return false;
-
-	switch (off) {
-	default:
-		return size == sizeof(__u32);
-	}
-}
-
-u32 bpf_mptcp_sock_convert_ctx_access(enum bpf_access_type type,
-				      const struct bpf_insn *si,
-				      struct bpf_insn *insn_buf,
-				      struct bpf_prog *prog, u32 *target_size)
-{
-	struct bpf_insn *insn = insn_buf;
-
-#define BPF_MPTCP_SOCK_GET_COMMON(FIELD)							\
-	do {											\
-		BUILD_BUG_ON(sizeof_field(struct mptcp_sock, FIELD) >				\
-				sizeof_field(struct bpf_mptcp_sock, FIELD));			\
-		*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct mptcp_sock, FIELD),		\
-							si->dst_reg, si->src_reg,		\
-							offsetof(struct mptcp_sock, FIELD));	\
-	} while (0)
-
-	if (insn > insn_buf)
-		return insn - insn_buf;
-
-	switch (si->off) {
-	case offsetof(struct bpf_mptcp_sock, token):
-		BPF_MPTCP_SOCK_GET_COMMON(token);
-		break;
-	}
-
-	return insn - insn_buf;
-}
-
-BPF_CALL_1(bpf_mptcp_sock, struct sock *, sk)
-{
-	if (sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk)) {
-		struct mptcp_subflow_context *mptcp_sfc = mptcp_subflow_ctx(sk);
-
-		return (unsigned long)mptcp_sfc->conn;
-	}
-	return (unsigned long)NULL;
-}
-
-const struct bpf_func_proto bpf_mptcp_sock_proto = {
-	.func           = bpf_mptcp_sock,
-	.gpl_only       = false,
-	.ret_type       = RET_PTR_TO_MPTCP_SOCK_OR_NULL,
-	.arg1_type      = ARG_PTR_TO_SOCK_COMMON,
-};
diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py
index 6ef5c44a2a9e..096625242475 100755
--- a/scripts/bpf_doc.py
+++ b/scripts/bpf_doc.py
@@ -623,7 +623,6 @@ class PrinterHelpers(Printer):
             'struct udp6_sock',
             'struct unix_sock',
             'struct task_struct',
-            'struct bpf_mptcp_sock',
 
             'struct __sk_buff',
             'struct sk_msg_md',
@@ -683,7 +682,6 @@ class PrinterHelpers(Printer):
             'struct socket',
             'struct file',
             'struct bpf_timer',
-            'struct bpf_mptcp_sock',
     }
     mapped_types = {
             'u8': '__u8',
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index f2e5c7bd4f10..3f6dcdf4b915 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5086,14 +5086,6 @@ union bpf_attr {
  *	Return
  *		0 on success, or a negative error in case of failure. On error
  *		*dst* buffer is zeroed out.
- *
- * struct bpf_mptcp_sock *bpf_mptcp_sock(struct bpf_sock *sk)
- *	Description
- *		This helper gets a **struct bpf_mptcp_sock** pointer from a
- *		**struct bpf_sock** pointer.
- *	Return
- *		A **struct bpf_mptcp_sock** pointer on success, or **NULL** in
- *		case of failure.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -5288,7 +5280,6 @@ union bpf_attr {
 	FN(xdp_load_bytes),		\
 	FN(xdp_store_bytes),		\
 	FN(copy_from_user_task),	\
-	FN(mptcp_sock),			\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
@@ -5637,10 +5628,6 @@ struct bpf_tcp_sock {
 	__u32 is_mptcp;		/* Is MPTCP subflow? */
 };
 
-struct bpf_mptcp_sock {
-	__u32 token;		/* msk token */
-};
-
 struct bpf_sock_tuple {
 	union {
 		struct {
-- 
2.34.1

[PATCH mptcp-next v6 3/4] bpf: add skc_to_mptcp_sock helper

[Geliang Tang]

This patch implemented bpf_skc_to_mptcp_sock helper. Defined a new bpf_id
BTF_SOCK_TYPE_MPTCP, and added a new helper bpf_mptcp_sock_from_subflow()
to get struct bpf_mptcp_sock from a given subflow socket.

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              | 17 +++++++++++++++++
 net/mptcp/Makefile             |  2 ++
 net/mptcp/bpf.c                | 17 +++++++++++++++++
 scripts/bpf_doc.py             |  2 ++
 tools/include/uapi/linux/bpf.h |  7 +++++++
 8 files changed, 63 insertions(+), 1 deletion(-)
 create mode 100644 net/mptcp/bpf.c

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index f19abc59b6cd..b28b3d8c3df2 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -2354,6 +2354,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 3f6dcdf4b915..a88aac4457a9 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5086,6 +5086,12 @@ union bpf_attr {
  *	Return
  *		0 on success, or a negative error in case of failure. On error
  *		*dst* buffer is zeroed out.
+ *
+ * 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),			\
@@ -5280,6 +5286,7 @@ union bpf_attr {
 	FN(xdp_load_bytes),		\
 	FN(xdp_store_bytes),		\
 	FN(copy_from_user_task),	\
+	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 f64454722a7e..538b6db4fd76 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11121,6 +11121,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);
@@ -11163,6 +11176,10 @@ 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:
+		return &bpf_skc_to_mptcp_sock_proto;
+#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..d140979181f9
--- /dev/null
+++ b/net/mptcp/bpf.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Multipath TCP
+ *
+ * Copyright (c) 2022, SUSE.
+ */
+
+#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 3f6dcdf4b915..a88aac4457a9 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5086,6 +5086,12 @@ union bpf_attr {
  *	Return
  *		0 on success, or a negative error in case of failure. On error
  *		*dst* buffer is zeroed out.
+ *
+ * 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),			\
@@ -5280,6 +5286,7 @@ union bpf_attr {
 	FN(xdp_load_bytes),		\
 	FN(xdp_store_bytes),		\
 	FN(copy_from_user_task),	\
+	FN(skc_to_mptcp_sock),		\
 	/* */
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
-- 
2.34.1

Re: [PATCH mptcp-next v6 3/4] bpf: add skc_to_mptcp_sock helper

[Matthieu Baerts]

Hi Geliang,

On 06/03/2022 02:01, Geliang Tang wrote:
> This patch implemented bpf_skc_to_mptcp_sock helper. Defined a new bpf_id
> BTF_SOCK_TYPE_MPTCP, and added a new helper bpf_mptcp_sock_from_subflow()
> to get struct bpf_mptcp_sock from a given subflow socket.

Thank you for the new version. It looks more like what I had in mind
after having re-read Alexei's review!

Patches 1-3/4 looks good to me but I just have two small suggestions for
this patch 3/4. My review for patch 4/4 will follow.

Do you mind if I re-add Nicolas' SoB as a co-developer? It is fine for
me to change the commiter name because after the recent review
iterations we had recently, the code has been almost fully rewritten.
But still, I would prefer to keep him as Co-dev as a "reward" for what
he did as a student :)

So we would have:

  Co-developed-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
  Signed-off-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
  Signed-off-by: Geliang Tang <geliang.tang@suse.com>

No objection from you?

(...)

> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> new file mode 100644
> index 000000000000..d140979181f9
> --- /dev/null
> +++ b/net/mptcp/bpf.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Multipath TCP
> + *
> + * Copyright (c) 2022, SUSE.

I understand your modification but is it OK if I re-add the previous
before the new one?

  Copyright (c) 2020, Tessares SA.

(I would prefer to keep it not to forget about very small companies
contributing to the Linux kernel and I'm not sure it can be easily
replaced :) )

I can do these modifications when applying the patches if there are no
objections.

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

Re: [PATCH mptcp-next v6 3/4] bpf: add skc_to_mptcp_sock helper

[Geliang Tang]

Hi Matt,

On Mon, Mar 07, 2022 at 03:04:29PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/03/2022 02:01, Geliang Tang wrote:
> > This patch implemented bpf_skc_to_mptcp_sock helper. Defined a new bpf_id
> > BTF_SOCK_TYPE_MPTCP, and added a new helper bpf_mptcp_sock_from_subflow()
> > to get struct bpf_mptcp_sock from a given subflow socket.
> 
> Thank you for the new version. It looks more like what I had in mind
> after having re-read Alexei's review!
> 
> Patches 1-3/4 looks good to me but I just have two small suggestions for
> this patch 3/4. My review for patch 4/4 will follow.
> 
> Do you mind if I re-add Nicolas' SoB as a co-developer? It is fine for
> me to change the commiter name because after the recent review
> iterations we had recently, the code has been almost fully rewritten.
> But still, I would prefer to keep him as Co-dev as a "reward" for what
> he did as a student :)
> 
> So we would have:
> 
>   Co-developed-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
>   Signed-off-by: Nicolas Rybowski <nicolas.rybowski@tessares.net>
>   Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> 
> No objection from you?

Sure! Please add Nicolas' SoB tag and your Suggested-by tag. I should
have added them. :)

> 
> (...)
> 
> > diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> > new file mode 100644
> > index 000000000000..d140979181f9
> > --- /dev/null
> > +++ b/net/mptcp/bpf.c
> > @@ -0,0 +1,17 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Multipath TCP
> > + *
> > + * Copyright (c) 2022, SUSE.
> 
> I understand your modification but is it OK if I re-add the previous
> before the new one?
> 
>   Copyright (c) 2020, Tessares SA.
> 
> (I would prefer to keep it not to forget about very small companies
> contributing to the Linux kernel and I'm not sure it can be easily
> replaced :) )
> 
> I can do these modifications when applying the patches if there are no
> objections.

I agree, please re-add:

// SPDX-License-Identifier: GPL-2.0
/* Multipath TCP
 *
 * Copyright (c) 2020, Tessares SA.
 *
 * Author: Nicolas Rybowski <nicolas.rybowski@tessares.net>
 *
 */

Thanks,
-Geliang

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

[PATCH mptcp-next v6 4/4] selftests: bpf: add skc_to_mptcp_sock test

[Geliang Tang]

This patch extended the MPTCP test base, to exercise bpf_skc_to_mptcp_sock()
from C test as Alexei suggested in v3.

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  | 27 +++++++++++++++----
 tools/testing/selftests/bpf/progs/mptcp.c     | 23 ++++++++++++++++
 3 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
index b1ede6f0b821..05f62f81cc4d 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 04aef0f147dc..ba856956f9c3 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -6,9 +6,11 @@
 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 int verify_sk(int map_fd, int client_fd, const char *msg,
+		     __u32 is_mptcp, __u32 token)
 {
 	int err = 0, cfd = client_fd;
 	struct mptcp_storage val;
@@ -19,8 +21,23 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
 	 * does not trigger sockops events.
 	 * We silently pass this situation at the moment.
 	 */
-	if (is_mptcp == 1)
-		return 0;
+	if (is_mptcp == 1) {
+		if (token <= 0)
+			return 0;
+
+		if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
+			perror("Failed to read socket storage");
+			return -1;
+		}
+
+		if (val.token <= 0) {
+			log_err("%s: unexpected bpf_mptcp_sock.token %d %d",
+				msg, val.token, token);
+			err++;
+		}
+
+		return err;
+	}
 
 	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
 		perror("Failed to read socket storage");
@@ -76,8 +93,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_sk(map_fd, client_fd, "MPTCP subflow socket", 1, 1) :
+			  verify_sk(map_fd, client_fd, "plain TCP socket", 0, 0);
 
 close_client_fd:
 	close(client_fd);
diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
index be5ee8dac2b3..212e9341b877 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
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
+#include "bpf_tcp_helpers.h"
 
 char _license[] SEC("license") = "GPL";
 __u32 _version SEC("version") = 1;
@@ -8,6 +9,7 @@ __u32 _version SEC("version") = 1;
 struct mptcp_storage {
 	__u32 invoked;
 	__u32 is_mptcp;
+	__u32 token;
 };
 
 struct {
@@ -20,6 +22,7 @@ struct {
 SEC("sockops")
 int _sockops(struct bpf_sock_ops *ctx)
 {
+	char fmt[] = "invoked=%u is_mptcp=%u token=%u\n";
 	struct mptcp_storage *storage;
 	struct bpf_tcp_sock *tcp_sk;
 	int op = (int)ctx->op;
@@ -43,6 +46,26 @@ int _sockops(struct bpf_sock_ops *ctx)
 
 	storage->invoked++;
 	storage->is_mptcp = tcp_sk->is_mptcp;
+	storage->token = 0;
+
+	if (tcp_sk->is_mptcp) {
+		struct mptcp_sock *msk;
+
+		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->invoked++;
+		storage->token = msk->token;
+		storage->is_mptcp = 1;
+	}
+
+	bpf_trace_printk(fmt, sizeof(fmt),
+			 storage->invoked, storage->is_mptcp, storage->token);
 
 	return 1;
 }
-- 
2.34.1

Re: selftests: bpf: add skc_to_mptcp_sock test: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- {"code":404,"message":
  - "HTTP 404 Not Found"}:
  - Task: https://cirrus-ci.com/task/6291582512005120
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6291582512005120/summary/summary.txt

- {"code":404,"message":
  - "HTTP 404 Not Found"}:
  - Task: https://cirrus-ci.com/task/5386436542201856
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5386436542201856/summary/summary.txt

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

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)

Re: selftests: bpf: add skc_to_mptcp_sock test: Tests Results

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- {"code":404,"message":
  - "HTTP 404 Not Found"}:
  - Task: https://cirrus-ci.com/task/6291582512005120
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6291582512005120/summary/summary.txt

- {"code":404,"message":
  - "HTTP 404 Not Found"}:
  - Task: https://cirrus-ci.com/task/5386436542201856
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5386436542201856/summary/summary.txt

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

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)

Re: selftests: bpf: add skc_to_mptcp_sock test: 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/6330772855455744
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6330772855455744/summary/summary.txt

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

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

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)

Re: [PATCH mptcp-next v6 4/4] selftests: bpf: add skc_to_mptcp_sock test

[Matthieu Baerts]

Hi Geliang,

On 06/03/2022 02:01, Geliang Tang wrote:
> This patch extended the MPTCP test base, to exercise bpf_skc_to_mptcp_sock()
> from C test as Alexei suggested in v3.
> 
> 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  | 27 +++++++++++++++----
>  tools/testing/selftests/bpf/progs/mptcp.c     | 23 ++++++++++++++++
>  3 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> index b1ede6f0b821..05f62f81cc4d 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));

Is it really OK to do that when 'struct mptcp_sock' is not declared in
the 'include' directory?
I guess the compiler can find where 'struct mptcp_sock' is actually
defined but I'm surprised it doesn't need more assistance here.

In your tests, did you check that the token seen in the kernel --
outside the BPF part but in MPTCP code -- is the same as what the
userspace program can see after a capture from BPF side? I mean: the
current test only check the token is different from 0 but it could also
be set at a wrong value (any random value except 0) and we would not
notice the issue, no?

> +
>  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 04aef0f147dc..ba856956f9c3 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -6,9 +6,11 @@
>  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 int verify_sk(int map_fd, int client_fd, const char *msg,
> +		     __u32 is_mptcp, __u32 token)
>  {
>  	int err = 0, cfd = client_fd;
>  	struct mptcp_storage val;
> @@ -19,8 +21,23 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
>  	 * does not trigger sockops events.
>  	 * We silently pass this situation at the moment.
>  	 */
> -	if (is_mptcp == 1)
> -		return 0;
> +	if (is_mptcp == 1) {
> +		if (token <= 0)

It looks like you no longer need 'token': it always has the same value
as "is_mptcp", which makes sense: if it is an MPTCP connection, it
should have a token. So maybe good not to add this new 'token' variable,
WDYT?

(it can be added later if it is needed to manage different cases)

> +			return 0;
> +
> +		if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
> +			perror("Failed to read socket storage");
> +			return -1;
> +		}
> +
> +		if (val.token <= 0) {
> +			log_err("%s: unexpected bpf_mptcp_sock.token %d %d",
> +				msg, val.token, token);
> +			err++;
> +		}
> +
> +		return err;
> +	}
>  
>  	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
>  		perror("Failed to read socket storage");
> @@ -76,8 +93,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_sk(map_fd, client_fd, "MPTCP subflow socket", 1, 1) :
> +			  verify_sk(map_fd, client_fd, "plain TCP socket", 0, 0);
>  
>  close_client_fd:
>  	close(client_fd);
> diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
> index be5ee8dac2b3..212e9341b877 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
>  #include <linux/bpf.h>
>  #include <bpf/bpf_helpers.h>
> +#include "bpf_tcp_helpers.h"
>  
>  char _license[] SEC("license") = "GPL";
>  __u32 _version SEC("version") = 1;
> @@ -8,6 +9,7 @@ __u32 _version SEC("version") = 1;
>  struct mptcp_storage {
>  	__u32 invoked;
>  	__u32 is_mptcp;
> +	__u32 token;
>  };
>  
>  struct {
> @@ -20,6 +22,7 @@ struct {
>  SEC("sockops")
>  int _sockops(struct bpf_sock_ops *ctx)
>  {
> +	char fmt[] = "invoked=%u is_mptcp=%u token=%u\n";
>  	struct mptcp_storage *storage;
>  	struct bpf_tcp_sock *tcp_sk;
>  	int op = (int)ctx->op;
> @@ -43,6 +46,26 @@ int _sockops(struct bpf_sock_ops *ctx)
>  
>  	storage->invoked++;
>  	storage->is_mptcp = tcp_sk->is_mptcp;
> +	storage->token = 0;
> +
> +	if (tcp_sk->is_mptcp) {
> +		struct mptcp_sock *msk;
> +
> +		msk = bpf_skc_to_mptcp_sock(sk);
> +		if (!msk)
> +			return 1;

(detail) if you have to edit this commit, please add a new empty line
here after the 'return'.

> +		storage = bpf_sk_storage_get(&socket_storage_map, msk, 0,
> +					     BPF_SK_STORAGE_GET_F_CREATE);
> +		if (!storage)
> +			return 1;
> +
> +		storage->invoked++;
> +		storage->token = msk->token;

Linked to my previous comment about checking if 'token' had the right
value: it is not enough to compare this 'msk->token' printed with
'bpf_trace_printk' here below with the value you have in
'prog_tests/mptcp.c' because it is supposed to be the same one if the
BPF MAP storage does its job (I guess it does).

Instead, we should compare with either what you can see with 'ss' or 'ip
mptcp monitor' or with the code in 'net/mptcp/' dir (pr_debug's print it
at some points I guess). But doing that is not enough: that will not be
part of the automated tests, just a manual check. But still good to do :)

My point is that the token is supposed to be random: if you read the
wrong address in the memory, it will also appear as a random value
because it is unlikely to read '0'.

Maybe it would be good to also check other values from 'mptcp_sock'. But
I don't know which one can be known in advanced in this structure appart
from booleans: fully_established, cork, etc. But I guess we should avoid
using booleans: one bit, we could be lucky to have the good value.

Or maybe interesting to use 'struct sock *first' (or *last_snd or
*subflow)? I mean it would be a good test case to keep a ref to a
subflow. Then from the userspace, we could get info from the MSK and
then get info about the subflow.
But I don't know if it is possible: typically from the usespace, we get
a sk from a fd. Can we get a sk from its pointer?
I guess it will be needed to iterate over the different subflows from a msk.

Cheers,
Matt


> +		storage->is_mptcp = 1;
> +	}
> +
> +	bpf_trace_printk(fmt, sizeof(fmt),
> +			 storage->invoked, storage->is_mptcp, storage->token);
>  
>  	return 1;
>  }

-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next v6 4/4] selftests: bpf: add skc_to_mptcp_sock test

[Geliang Tang]

Hi Matt,

On Mon, Mar 07, 2022 at 03:15:46PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 06/03/2022 02:01, Geliang Tang wrote:
> > This patch extended the MPTCP test base, to exercise bpf_skc_to_mptcp_sock()
> > from C test as Alexei suggested in v3.
> > 
> > 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  | 27 +++++++++++++++----
> >  tools/testing/selftests/bpf/progs/mptcp.c     | 23 ++++++++++++++++
> >  3 files changed, 51 insertions(+), 5 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
> > index b1ede6f0b821..05f62f81cc4d 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));
> 
> Is it really OK to do that when 'struct mptcp_sock' is not declared in
> the 'include' directory?
> I guess the compiler can find where 'struct mptcp_sock' is actually
> defined but I'm surprised it doesn't need more assistance here.

Without this struct mptcp_sock declaration, we'll get the following build
errors:

progs/mptcp.c:64:23: error: progs/mptcp.cincomplete definition of type 'struct mptcp_sock':
64:23: error: incomplete definition of type 'struct mptcp_sock'
                storage->token = msk->token;
                                 ~~~^
                storage->token = msk->token;
                                 ~~~^
/home/tgl/mptcp_net-next/tools/testing/selftests/bpf/tools/include/bpf/bpf_helper_defs.h:32:8: note: forward declaration of 'struct mptcp_sock'
/home/tgl/mptcp_net-next/tools/testing/selftests/bpf/tools/include/bpf/bpf_helper_defs.h:struct mptcp_sock;32
:8       ^:
 note: forward declaration of 'struct mptcp_sock'
struct mptcp_sock;
       ^
11 error error generated generated.
.
make: *** [Makefile:487: /home/tgl/mptcp_net-next/tools/testing/selftests/bpf/mptcp.o] Error 1
make: *** Waiting for unfinished jobs....
make: *** [Makefile:492: /home/tgl/mptcp_net-next/tools/testing/selftests/bpf/no_alu32/mptcp.o] Error 1

> 
> In your tests, did you check that the token seen in the kernel --
> outside the BPF part but in MPTCP code -- is the same as what the
> userspace program can see after a capture from BPF side? I mean: the
> current test only check the token is different from 0 but it could also
> be set at a wrong value (any random value except 0) and we would not
> notice the issue, no?

Yes, in my tests, the token in the userspace (both bpf and ip mptcp monitor)
is the same as it printed in the kernel:

> sudo dmesg | grep mptcp_sock
[ 3057.842339] bpf_mptcp_sock_from_subflow sk=000000004370e862 sk_fullsock=1 sk_protocol=1 sk_is_mptcp=1
[ 3057.842341] bpf_mptcp_sock_from_subflow subflow->conn=00000000f16c62fd msk=00000000f16c62fd token=3420716048

# cat /sys/kernel/debug/tracing/trace_pipe
      test_progs-22898   [007] d..21  3057.858031: bpf_trace_printk: is_mptcp=0 token=0(0x0)
      test_progs-22898   [007] d..21  3057.872544: bpf_trace_printk: is_mptcp=1 token=3420716048(0xcbe3fc10)

> sudo ip mptcp monitor
[       CREATED] token=cbe3fc10 remid=0 locid=0 saddr4=127.0.0.1 daddr4=127.0.0.1 sport=54646 dport=60123
[       CREATED] token=68f336ad remid=0 locid=0 saddr4=127.0.0.1 daddr4=127.0.0.1 sport=60123 dport=54646
[        CLOSED] token=cbe3fc10

> 
> > +
> >  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 04aef0f147dc..ba856956f9c3 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > @@ -6,9 +6,11 @@
> >  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 int verify_sk(int map_fd, int client_fd, const char *msg,
> > +		     __u32 is_mptcp, __u32 token)
> >  {
> >  	int err = 0, cfd = client_fd;
> >  	struct mptcp_storage val;
> > @@ -19,8 +21,23 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
> >  	 * does not trigger sockops events.
> >  	 * We silently pass this situation at the moment.
> >  	 */
> > -	if (is_mptcp == 1)
> > -		return 0;
> > +	if (is_mptcp == 1) {
> > +		if (token <= 0)
> 
> It looks like you no longer need 'token': it always has the same value
> as "is_mptcp", which makes sense: if it is an MPTCP connection, it
> should have a token. So maybe good not to add this new 'token' variable,
> WDYT?
> 
> (it can be added later if it is needed to manage different cases)

agree.

> 
> > +			return 0;
> > +
> > +		if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
> > +			perror("Failed to read socket storage");
> > +			return -1;
> > +		}
> > +
> > +		if (val.token <= 0) {
> > +			log_err("%s: unexpected bpf_mptcp_sock.token %d %d",
> > +				msg, val.token, token);
> > +			err++;
> > +		}
> > +
> > +		return err;
> > +	}
> >  
> >  	if (CHECK_FAIL(bpf_map_lookup_elem(map_fd, &cfd, &val) < 0)) {
> >  		perror("Failed to read socket storage");
> > @@ -76,8 +93,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_sk(map_fd, client_fd, "MPTCP subflow socket", 1, 1) :
> > +			  verify_sk(map_fd, client_fd, "plain TCP socket", 0, 0);
> >  
> >  close_client_fd:
> >  	close(client_fd);
> > diff --git a/tools/testing/selftests/bpf/progs/mptcp.c b/tools/testing/selftests/bpf/progs/mptcp.c
> > index be5ee8dac2b3..212e9341b877 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
> >  #include <linux/bpf.h>
> >  #include <bpf/bpf_helpers.h>
> > +#include "bpf_tcp_helpers.h"
> >  
> >  char _license[] SEC("license") = "GPL";
> >  __u32 _version SEC("version") = 1;
> > @@ -8,6 +9,7 @@ __u32 _version SEC("version") = 1;
> >  struct mptcp_storage {
> >  	__u32 invoked;
> >  	__u32 is_mptcp;
> > +	__u32 token;
> >  };
> >  
> >  struct {
> > @@ -20,6 +22,7 @@ struct {
> >  SEC("sockops")
> >  int _sockops(struct bpf_sock_ops *ctx)
> >  {
> > +	char fmt[] = "invoked=%u is_mptcp=%u token=%u\n";
> >  	struct mptcp_storage *storage;
> >  	struct bpf_tcp_sock *tcp_sk;
> >  	int op = (int)ctx->op;
> > @@ -43,6 +46,26 @@ int _sockops(struct bpf_sock_ops *ctx)
> >  
> >  	storage->invoked++;
> >  	storage->is_mptcp = tcp_sk->is_mptcp;
> > +	storage->token = 0;
> > +
> > +	if (tcp_sk->is_mptcp) {
> > +		struct mptcp_sock *msk;
> > +
> > +		msk = bpf_skc_to_mptcp_sock(sk);
> > +		if (!msk)
> > +			return 1;
> 
> (detail) if you have to edit this commit, please add a new empty line
> here after the 'return'.

Agree.

> 
> > +		storage = bpf_sk_storage_get(&socket_storage_map, msk, 0,
> > +					     BPF_SK_STORAGE_GET_F_CREATE);
> > +		if (!storage)
> > +			return 1;
> > +
> > +		storage->invoked++;
> > +		storage->token = msk->token;
> 
> Linked to my previous comment about checking if 'token' had the right
> value: it is not enough to compare this 'msk->token' printed with
> 'bpf_trace_printk' here below with the value you have in
> 'prog_tests/mptcp.c' because it is supposed to be the same one if the
> BPF MAP storage does its job (I guess it does).
> 
> Instead, we should compare with either what you can see with 'ss' or 'ip
> mptcp monitor' or with the code in 'net/mptcp/' dir (pr_debug's print it
> at some points I guess). But doing that is not enough: that will not be
> part of the automated tests, just a manual check. But still good to do :)
> 
> My point is that the token is supposed to be random: if you read the
> wrong address in the memory, it will also appear as a random value
> because it is unlikely to read '0'.

I did these manual checks. The all token values are the same in both
user space and kernel space. I'll try to add the automated check in the
next version, using netlink (like 'ip mptcp monitor') to get the token
values from kernel space, and compare it in bpf.

> 
> Maybe it would be good to also check other values from 'mptcp_sock'. But
> I don't know which one can be known in advanced in this structure appart
> from booleans: fully_established, cork, etc. But I guess we should avoid
> using booleans: one bit, we could be lucky to have the good value.
> 
> Or maybe interesting to use 'struct sock *first' (or *last_snd or
> *subflow)? I mean it would be a good test case to keep a ref to a
> subflow. Then from the userspace, we could get info from the MSK and
> then get info about the subflow.
> But I don't know if it is possible: typically from the usespace, we get
> a sk from a fd. Can we get a sk from its pointer?
> I guess it will be needed to iterate over the different subflows from a msk.

I'll try to check other more 'mptcp_sock' members later.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> 
> 
> > +		storage->is_mptcp = 1;
> > +	}
> > +
> > +	bpf_trace_printk(fmt, sizeof(fmt),
> > +			 storage->invoked, storage->is_mptcp, storage->token);
> >  
> >  	return 1;
> >  }
> 
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

Re: [PATCH mptcp-next v6 4/4] selftests: bpf: add skc_to_mptcp_sock test

[Matthieu Baerts]

Hi Geliang,

On 07/03/2022 16:58, Geliang Tang wrote:
> Hi Matt,
> 
> On Mon, Mar 07, 2022 at 03:15:46PM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 06/03/2022 02:01, Geliang Tang wrote:
>>> This patch extended the MPTCP test base, to exercise bpf_skc_to_mptcp_sock()
>>> from C test as Alexei suggested in v3.
>>>
>>> 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  | 27 +++++++++++++++----
>>>  tools/testing/selftests/bpf/progs/mptcp.c     | 23 ++++++++++++++++
>>>  3 files changed, 51 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h
>>> index b1ede6f0b821..05f62f81cc4d 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));
>>
>> Is it really OK to do that when 'struct mptcp_sock' is not declared in
>> the 'include' directory?
>> I guess the compiler can find where 'struct mptcp_sock' is actually
>> defined but I'm surprised it doesn't need more assistance here.
> 
> Without this struct mptcp_sock declaration, we'll get the following build
> errors:

I agree that we need it. But I was just surprised GCC/CLang knows the
'token' entry is not available at:

    [mptcp_sock address] + sizeof(struct inet_connection_sock)

like we could think it is when reading this structure here but instead
it is available at:

    [mptcp_sock address] + proper 'offsetof'

by looking at the structure defined in net/mptcp/protocol.h.

So good to know the "preserve_access_index" attribute works very well :)

>> In your tests, did you check that the token seen in the kernel --
>> outside the BPF part but in MPTCP code -- is the same as what the
>> userspace program can see after a capture from BPF side? I mean: the
>> current test only check the token is different from 0 but it could also
>> be set at a wrong value (any random value except 0) and we would not
>> notice the issue, no?
> 
> Yes, in my tests, the token in the userspace (both bpf and ip mptcp monitor)
> is the same as it printed in the kernel:
> 
>> sudo dmesg | grep mptcp_sock
> [ 3057.842339] bpf_mptcp_sock_from_subflow sk=000000004370e862 sk_fullsock=1 sk_protocol=1 sk_is_mptcp=1
> [ 3057.842341] bpf_mptcp_sock_from_subflow subflow->conn=00000000f16c62fd msk=00000000f16c62fd token=3420716048
> 
> # cat /sys/kernel/debug/tracing/trace_pipe
>       test_progs-22898   [007] d..21  3057.858031: bpf_trace_printk: is_mptcp=0 token=0(0x0)
>       test_progs-22898   [007] d..21  3057.872544: bpf_trace_printk: is_mptcp=1 token=3420716048(0xcbe3fc10)

(detail: maybe good to use "token=0x%x" instead of using "%u" in your
bpf_trace_printk's format you added in
tools/testing/selftests/bpf/progs/mptcp.c)


>> sudo ip mptcp monitor
> [       CREATED] token=cbe3fc10 remid=0 locid=0 saddr4=127.0.0.1 daddr4=127.0.0.1 sport=54646 dport=60123
> [       CREATED] token=68f336ad remid=0 locid=0 saddr4=127.0.0.1 daddr4=127.0.0.1 sport=60123 dport=54646
> [        CLOSED] token=cbe3fc10

Great, thank you for having checked!


>>> +		storage = bpf_sk_storage_get(&socket_storage_map, msk, 0,
>>> +					     BPF_SK_STORAGE_GET_F_CREATE);
>>> +		if (!storage)
>>> +			return 1;
>>> +
>>> +		storage->invoked++;
>>> +		storage->token = msk->token;
>>
>> Linked to my previous comment about checking if 'token' had the right
>> value: it is not enough to compare this 'msk->token' printed with
>> 'bpf_trace_printk' here below with the value you have in
>> 'prog_tests/mptcp.c' because it is supposed to be the same one if the
>> BPF MAP storage does its job (I guess it does).
>>
>> Instead, we should compare with either what you can see with 'ss' or 'ip
>> mptcp monitor' or with the code in 'net/mptcp/' dir (pr_debug's print it
>> at some points I guess). But doing that is not enough: that will not be
>> part of the automated tests, just a manual check. But still good to do :)
>>
>> My point is that the token is supposed to be random: if you read the
>> wrong address in the memory, it will also appear as a random value
>> because it is unlikely to read '0'.
> 
> I did these manual checks. The all token values are the same in both
> user space and kernel space. I'll try to add the automated check in the
> next version, using netlink (like 'ip mptcp monitor') to get the token
> values from kernel space, and compare it in bpf.

That would be nice but hopefuly not too complex. It can also be done in
an additional patch.

> 
>>
>> Maybe it would be good to also check other values from 'mptcp_sock'. But
>> I don't know which one can be known in advanced in this structure appart
>> from booleans: fully_established, cork, etc. But I guess we should avoid
>> using booleans: one bit, we could be lucky to have the good value.
>>
>> Or maybe interesting to use 'struct sock *first' (or *last_snd or
>> *subflow)? I mean it would be a good test case to keep a ref to a
>> subflow. Then from the userspace, we could get info from the MSK and
>> then get info about the subflow.
>> But I don't know if it is possible: typically from the usespace, we get
>> a sk from a fd. Can we get a sk from its pointer?
>> I guess it will be needed to iterate over the different subflows from a msk.
> 
> I'll try to check other more 'mptcp_sock' members later.

Thanks!
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net