[PATCH mptcp-next v4] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper"

Geliang Tang posted 1 patch 2 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/multipath-tcp/mptcp_net-next tags/patchew/4a1d63bb7976c1d61308a875132107e17272c8f8.1645519631.git.geliang.tang@suse.com
Maintainers: "David S. Miller" <davem@davemloft.net>, Jakub Kicinski <kuba@kernel.org>, Andrii Nakryiko <andrii@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Alexei Starovoitov <ast@kernel.org>, Yonghong Song <yhs@fb.com>, John Fastabend <john.fastabend@gmail.com>, Mat Martineau <mathew.j.martineau@linux.intel.com>, Matthieu Baerts <matthieu.baerts@tessares.net>, Martin KaFai Lau <kafai@fb.com>, KP Singh <kpsingh@kernel.org>, Song Liu <songliubraving@fb.com>
include/linux/bpf.h     | 20 ------------------
include/linux/btf_ids.h |  3 ++-
include/linux/net.h     |  1 +
kernel/bpf/verifier.c   | 20 ------------------
net/core/filter.c       | 13 ++++++++++++
net/mptcp/bpf.c         | 46 ++++++-----------------------------------
6 files changed, 22 insertions(+), 81 deletions(-)
[PATCH mptcp-next v4] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper"
Posted by Geliang Tang 2 years, 2 months ago
This patch addressed to the comments in [1] from BPF maintainer Alexei:

'''
On Fri, Sep 18, 2020 at 02:10:42PM +0200, Nicolas Rybowski wrote:
> +
> +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);

Could you add !sk check here as well?
See commit 8c33dadc3e0e ("bpf: Bpf_skc_to_* casting helpers require a NULL check on sk")
It's not strictly necessary yet, but see below.
'''

I added this !sk check as he suggested.

'''
I think we shouldn't extend the verifier with PTR_TO_MPTCP_SOCK and similar concept anymore.
This approach doesn't scale and we have better way to handle such field access with BTF.

> +     }
> +     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,

In this particular case you can do:
+       .ret_type       = RET_PTR_TO_BTF_ID_OR_NULL,

Then bpf_mptcp_sock_convert_ctx_access() will no longer be necessary
and bpf prog will be able to access all mptcp_sock fields right away.
Will that work for your use case?
'''

I dropped bpf_mptcp_sock_convert_ctx_access() and use RET_PTR_TO_BTF_ID_OR_NULL
instead of RET_PTR_TO_MPTCP_SOCK_OR_NULL as he suggested..

Both 'bpf selftests' and 'bpf exampe' work with this patch.

[1]
https://lore.kernel.org/netdev/20200922040830.3iis6xiavhvpfq3v@ast-mbp.dhcp.thefacebook.com/

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.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 include/linux/bpf.h     | 20 ------------------
 include/linux/btf_ids.h |  3 ++-
 include/linux/net.h     |  1 +
 kernel/bpf/verifier.c   | 20 ------------------
 net/core/filter.c       | 13 ++++++++++++
 net/mptcp/bpf.c         | 46 ++++++-----------------------------------
 6 files changed, 22 insertions(+), 81 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5a17c1e3a6bc..f1b1f44c68ab 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;
@@ -2364,12 +2359,6 @@ static inline u32 bpf_xdp_sock_convert_ctx_access(enum bpf_access_type type,
 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,
@@ -2377,15 +2366,6 @@ static inline bool bpf_mptcp_sock_is_valid_access(int off, int size,
 {
 	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 {
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/linux/net.h b/include/linux/net.h
index ba736b457a06..9c1a634e5ac5 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -238,6 +238,7 @@ int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
 struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
 struct socket *sockfd_lookup(int fd, int *err);
 struct socket *sock_from_file(struct file *file);
+struct sock *msk_from_subflow(struct sock *sk);
 #define		     sockfd_put(sock) fput(sock->file)
 int net_ratelimit(void);
 
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..43fbdd5e602a 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -7395,6 +7395,19 @@ static const struct bpf_func_proto bpf_sock_ops_reserve_hdr_opt_proto = {
 	.arg3_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_1(bpf_mptcp_sock, struct sock *, sk)
+{
+	return (unsigned long)msk_from_subflow(sk);
+}
+
+static const struct bpf_func_proto bpf_mptcp_sock_proto = {
+	.func		= bpf_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],
+};
+
 #endif /* CONFIG_INET */
 
 bool bpf_helper_changes_pkt_data(void *func)
diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 5332469fbb28..6877fa5b5c66 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -26,47 +26,13 @@ bool bpf_mptcp_sock_is_valid_access(int off, int size, enum bpf_access_type type
 	}
 }
 
-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 sock *msk_from_subflow(struct sock *sk)
 {
-	struct bpf_insn *insn = insn_buf;
+	if (sk && sk_fullsock(sk) && sk->sk_protocol == IPPROTO_TCP && sk_is_mptcp(sk)) {
+		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
 
-#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 subflow->conn;
 	}
-
-	return insn - insn_buf;
+	return NULL;
 }
-
-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,
-};
+EXPORT_SYMBOL(msk_from_subflow);
-- 
2.34.1


Re: [PATCH mptcp-next v4] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper"
Posted by Matthieu Baerts 2 years, 1 month ago
Hi Geliang,

On 22/02/2022 09:52, Geliang Tang wrote:
> This patch addressed to the comments in [1] from BPF maintainer Alexei:

(...)

> diff --git a/include/linux/net.h b/include/linux/net.h
> index ba736b457a06..9c1a634e5ac5 100644
> --- a/include/linux/net.h
> +++ b/include/linux/net.h
> @@ -238,6 +238,7 @@ int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
>  struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
>  struct socket *sockfd_lookup(int fd, int *err);
>  struct socket *sock_from_file(struct file *file);
> +struct sock *msk_from_subflow(struct sock *sk);

Would it not be better to expose this in include/net/mptcp.h instead?
And prepend it with 'mptcp_' as it is exposed to the "outside world"?

  mptcp_sock_from_subflow()

But when thinking about that, maybe better to name it:

  bpf_mptcp_sock_from_subflow()

as the implementation is quite specific to BPF.

In this case, it should go in include/linux/bpf.h with the other
bpf_mptcp_ functions (with the #if/else CONFIG_MPTCP you need to avoid
compilation errors).

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

Re: [PATCH mptcp-next v4] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper"
Posted by Geliang Tang 2 years, 1 month ago
On Thu, Mar 03, 2022 at 09:44:52AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 22/02/2022 09:52, Geliang Tang wrote:
> > This patch addressed to the comments in [1] from BPF maintainer Alexei:
> 
> (...)
> 
> > diff --git a/include/linux/net.h b/include/linux/net.h
> > index ba736b457a06..9c1a634e5ac5 100644
> > --- a/include/linux/net.h
> > +++ b/include/linux/net.h
> > @@ -238,6 +238,7 @@ int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
> >  struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
> >  struct socket *sockfd_lookup(int fd, int *err);
> >  struct socket *sock_from_file(struct file *file);
> > +struct sock *msk_from_subflow(struct sock *sk);
> 
> Would it not be better to expose this in include/net/mptcp.h instead?
> And prepend it with 'mptcp_' as it is exposed to the "outside world"?
> 
>   mptcp_sock_from_subflow()
> 
> But when thinking about that, maybe better to name it:
> 
>   bpf_mptcp_sock_from_subflow()
> 
> as the implementation is quite specific to BPF.
> 
> In this case, it should go in include/linux/bpf.h with the other
> bpf_mptcp_ functions (with the #if/else CONFIG_MPTCP you need to avoid
> compilation errors).

Thanks Matt, I'll update this in v5.

But v4 still needs to be improved. The token value is incorrect in it,
since bpf_mptcp_sock_convert_ctx_access is dropped. We should do this
convert in this BTF_ID mode too, but I don't know how to do it. I mean
how to invoke this convert function when we use
RET_PTR_TO_BTF_ID_OR_NULL.

Thanks,
-Geliang

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


Re: [PATCH mptcp-next v4] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper"
Posted by Matthieu Baerts 2 years, 1 month ago
Hi Geliang,

On 03/03/2022 10:12, Geliang Tang wrote:
> On Thu, Mar 03, 2022 at 09:44:52AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> On 22/02/2022 09:52, Geliang Tang wrote:
>>> This patch addressed to the comments in [1] from BPF maintainer Alexei:
>>
>> (...)
>>
>>> diff --git a/include/linux/net.h b/include/linux/net.h
>>> index ba736b457a06..9c1a634e5ac5 100644
>>> --- a/include/linux/net.h
>>> +++ b/include/linux/net.h
>>> @@ -238,6 +238,7 @@ int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
>>>  struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
>>>  struct socket *sockfd_lookup(int fd, int *err);
>>>  struct socket *sock_from_file(struct file *file);
>>> +struct sock *msk_from_subflow(struct sock *sk);
>>
>> Would it not be better to expose this in include/net/mptcp.h instead?
>> And prepend it with 'mptcp_' as it is exposed to the "outside world"?
>>
>>   mptcp_sock_from_subflow()
>>
>> But when thinking about that, maybe better to name it:
>>
>>   bpf_mptcp_sock_from_subflow()
>>
>> as the implementation is quite specific to BPF.
>>
>> In this case, it should go in include/linux/bpf.h with the other
>> bpf_mptcp_ functions (with the #if/else CONFIG_MPTCP you need to avoid
>> compilation errors).
> 
> Thanks Matt, I'll update this in v5.
> 
> But v4 still needs to be improved. The token value is incorrect in it,
> since bpf_mptcp_sock_convert_ctx_access is dropped. We should do this
> convert in this BTF_ID mode too, but I don't know how to do it. I mean
> how to invoke this convert function when we use
> RET_PTR_TO_BTF_ID_OR_NULL.

I just sent a reply in 'selftests: bpf: exercise bpf_mptcp_sock()​' thread.

In short, now bpf_mptcp_sock() returns a "struct mptcp_sock *" and no
longer a "struct bpf_mptcp_sock *" with only the token in it. No?

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

Re: [PATCH mptcp-next v4] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper"
Posted by Matthieu Baerts 2 years, 2 months ago
Hi Geliang,

On 22/02/2022 09:52, Geliang Tang wrote:
> This patch addressed to the comments in [1] from BPF maintainer Alexei:

Thank you for this v4!

Just to be sure it is clear: this patch is marked as "Changes Requested"
on patchwork because it fails to build:

https://github.com/multipath-tcp/mptcp_net-next/runs/5285534493?check_suite_focus=true#step:5:3060

  ld: net/core/filter.o: in function `bpf_mptcp_sock':
  filter.c:(.text+0xca07): undefined reference to `msk_from_subflow'
  make: *** [Makefile:1155: vmlinux] Error 1

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

Re: [PATCH mptcp-next v4] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper"
Posted by Geliang Tang 2 years, 2 months ago
Matthieu Baerts <matthieu.baerts@tessares.net> 于2022年2月24日周四 22:16写道:
>
> Hi Geliang,
>
> On 22/02/2022 09:52, Geliang Tang wrote:
> > This patch addressed to the comments in [1] from BPF maintainer Alexei:
>
> Thank you for this v4!
>
> Just to be sure it is clear: this patch is marked as "Changes Requested"
> on patchwork because it fails to build:
>
> https://github.com/multipath-tcp/mptcp_net-next/runs/5285534493?check_suite_focus=true#step:5:3060
>
>   ld: net/core/filter.o: in function `bpf_mptcp_sock':
>   filter.c:(.text+0xca07): undefined reference to `msk_from_subflow'
>   make: *** [Makefile:1155: vmlinux] Error 1

Thanks Matt, I'll fix it in v5.

-Geliang

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