* [PATCH mptcp-next v3] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper"
@ 2022-02-22 6:11 Geliang Tang
2022-02-22 6:18 ` Squash to "bpf: add 'bpf_mptcp_sock' structure and helper": Build Failure MPTCP CI
0 siblings, 1 reply; 2+ messages in thread
From: Geliang Tang @ 2022-02-22 6:11 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
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/
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
include/linux/bpf.h | 19 -----------------
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(+), 80 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 5a17c1e3a6bc..6f840ece0e96 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
@@ -2364,12 +2360,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 +2367,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..a143f7631bba 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -11143,6 +11143,19 @@ const struct bpf_func_proto bpf_sock_from_file_proto = {
.arg1_btf_id = &bpf_sock_from_file_btf_ids[1],
};
+BPF_CALL_1(bpf_mptcp_sock, struct sock *, sk)
+{
+ return (unsigned long)msk_from_subflow(sk);
+}
+
+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],
+};
+
static const struct bpf_func_proto *
bpf_sk_base_func_proto(enum bpf_func_id func_id)
{
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: Squash to "bpf: add 'bpf_mptcp_sock' structure and helper": Build Failure
2022-02-22 6:11 [PATCH mptcp-next v3] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper" Geliang Tang
@ 2022-02-22 6:18 ` MPTCP CI
0 siblings, 0 replies; 2+ messages in thread
From: MPTCP CI @ 2022-02-22 6:18 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
Hi Geliang,
Thank you for your modifications, that's great!
But sadly, our CI spotted some issues with it when trying to build it.
You can find more details there:
https://patchwork.kernel.org/project/mptcp/patch/a355682307154e5fc4f26fc0957f00f931460905.1645510267.git.geliang.tang@suse.com/
https://github.com/multipath-tcp/mptcp_net-next/actions/runs/1879950874
Status: failure
Initiator: MPTCPimporter
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/9af48d88e2c2
Feel free to reply to this email if you cannot access logs, if you need
some support to fix the error, if this doesn't seem to be caused by your
modifications or if the error is a false positive one.
Cheers,
MPTCP GH Action bot
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-02-22 6:19 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-22 6:11 [PATCH mptcp-next v3] Squash to "bpf: add 'bpf_mptcp_sock' structure and helper" Geliang Tang
2022-02-22 6:18 ` Squash to "bpf: add 'bpf_mptcp_sock' structure and helper": Build Failure MPTCP CI
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.