| Message ID | 20200925000337.3853598-1-kafai@fb.com |
|---|---|
| Headers | show |
| Series | bpf: Enable bpf_skc_to_* sock casting helper to networking prog type | expand |
On Fri, 25 Sep 2020 at 01:03, Martin KaFai Lau <kafai@fb.com> wrote: > > check_reg_type() checks whether a reg can be used as an arg of a > func_proto. For PTR_TO_BTF_ID, the check is actually not > completely done until the reg->btf_id is pointing to a > kernel struct that is acceptable by the func_proto. > > Thus, this patch moves the btf_id check into check_reg_type(). > "arg_type" and "arg_btf_id" are passed to check_reg_type() instead of > "compatible". The compatible_reg_types[] usage is localized in > check_reg_type() now. > > The "if (!btf_id) verbose(...); " is also removed since it won't happen. > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> Acked-by: Lorenz Bauer <lmb@cloudflare.com> > --- > kernel/bpf/verifier.c | 60 ++++++++++++++++++++++--------------------- > 1 file changed, 31 insertions(+), 29 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 42dee5dcbc74..945fa2b4d096 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -4028,19 +4028,27 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > }; > > static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > - const struct bpf_reg_types *compatible) > + enum bpf_arg_type arg_type, > + const u32 *arg_btf_id) > { > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > enum bpf_reg_type expected, type = reg->type; > + const struct bpf_reg_types *compatible; > int i, j; > > + compatible = compatible_reg_types[arg_type]; > + if (!compatible) { > + verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); > + return -EFAULT; > + } > + > for (i = 0; i < ARRAY_SIZE(compatible->types); i++) { > expected = compatible->types[i]; > if (expected == NOT_INIT) > break; > > if (type == expected) > - return 0; > + goto found; > } > > verbose(env, "R%d type=%s expected=", regno, reg_type_str[type]); > @@ -4048,6 +4056,25 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > verbose(env, "%s, ", reg_type_str[compatible->types[j]]); > verbose(env, "%s\n", reg_type_str[compatible->types[j]]); > return -EACCES; > + > +found: > + if (type == PTR_TO_BTF_ID) { > + if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, > + *arg_btf_id)) { > + verbose(env, "R%d is of type %s but %s is expected\n", > + regno, kernel_type_name(reg->btf_id), > + kernel_type_name(*arg_btf_id)); > + return -EACCES; > + } > + > + if (!tnum_is_const(reg->var_off) || reg->var_off.value) { > + verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n", > + regno); > + return -EACCES; > + } > + } > + > + return 0; > } > > static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > @@ -4057,7 +4084,6 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > u32 regno = BPF_REG_1 + arg; > struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; > enum bpf_arg_type arg_type = fn->arg_type[arg]; > - const struct bpf_reg_types *compatible; > enum bpf_reg_type type = reg->type; > int err = 0; > > @@ -4097,35 +4123,11 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > */ > goto skip_type_check; > > - compatible = compatible_reg_types[arg_type]; > - if (!compatible) { > - verbose(env, "verifier internal error: unsupported arg type %d\n", arg_type); > - return -EFAULT; > - } > - > - err = check_reg_type(env, regno, compatible); > + err = check_reg_type(env, regno, arg_type, fn->arg_btf_id[arg]); > if (err) > return err; > > - if (type == PTR_TO_BTF_ID) { > - const u32 *btf_id = fn->arg_btf_id[arg]; > - > - if (!btf_id) { > - verbose(env, "verifier internal error: missing BTF ID\n"); > - return -EFAULT; > - } > - > - if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, *btf_id)) { > - verbose(env, "R%d is of type %s but %s is expected\n", > - regno, kernel_type_name(reg->btf_id), kernel_type_name(*btf_id)); > - return -EACCES; > - } > - if (!tnum_is_const(reg->var_off) || reg->var_off.value) { > - verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n", > - regno); > - return -EACCES; > - } > - } else if (type == PTR_TO_CTX) { > + if (type == PTR_TO_CTX) { > err = check_ctx_reg(env, reg, regno); > if (err < 0) > return err; > -- > 2.24.1 >
On Fri, 25 Sep 2020 at 01:04, Martin KaFai Lau <kafai@fb.com> wrote: > > There is a constant need to add more fields into the bpf_tcp_sock > for the bpf programs running at tc, sock_ops...etc. > > A current workaround could be to use bpf_probe_read_kernel(). However, > other than making another helper call for reading each field and missing > CO-RE, it is also not as intuitive to use as directly reading > "tp->lsndtime" for example. While already having perfmon cap to do > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > directly read from the tcp_sock. > > This patch tries to do that by using the existing casting-helpers > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > kernel "struct tcp_sock". > > These helpers are also added to is_ptr_cast_function(). > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > That will keep the ref-tracking works properly. > > The bpf_skc_to_* helpers are made available to most of the bpf prog > types in filter.c. The bpf_skc_to_* helpers will be limited by > perfmon cap. > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > they will accept pointer obtained from skb->sk. > > Instead of specifying both arg_type and arg_btf_id in the same func_proto > which is how the current ARG_PTR_TO_BTF_ID does, the arg_btf_id of > the new ARG_PTR_TO_BTF_ID_SOCK_COMMON is specified in the > compatible_reg_types[] in verifier.c. The reason is the arg_btf_id is > always the same. Discussion in this thread: > https://lore.kernel.org/bpf/20200922070422.1917351-1-kafai@fb.com/ > > The ARG_PTR_TO_BTF_ID_ part gives a clear expectation that the helper is > expecting a PTR_TO_BTF_ID which could be NULL. This is the same > behavior as the existing helper taking ARG_PTR_TO_BTF_ID. > > The _SOCK_COMMON part means the helper is also expecting the legacy > SOCK_COMMON pointer. > > By excluding the _OR_NULL part, the bpf prog cannot call helper > with a literal NULL which doesn't make sense in most cases. > e.g. bpf_skc_to_tcp_sock(NULL) will be rejected. All PTR_TO_*_OR_NULL > reg has to do a NULL check first before passing into the helper or else > the bpf prog will be rejected. This behavior is nothing new and > consistent with the current expectation during bpf-prog-load. > > [ ARG_PTR_TO_BTF_ID_SOCK_COMMON will be used to replace > ARG_PTR_TO_SOCK* of other existing helpers later such that > those existing helpers can take the PTR_TO_BTF_ID returned by > the bpf_skc_to_*() helpers. > > The only special case is bpf_sk_lookup_assign() which can accept a > literal NULL ptr. It has to be handled specially in another follow > up patch if there is a need (e.g. by renaming ARG_PTR_TO_SOCKET_OR_NULL > to ARG_PTR_TO_BTF_ID_SOCK_COMMON_OR_NULL). ] > > [ When converting the older helpers that take ARG_PTR_TO_SOCK* in > the later patch, if the kernel does not support BTF, > ARG_PTR_TO_BTF_ID_SOCK_COMMON will behave like ARG_PTR_TO_SOCK_COMMON > because no reg->type could have PTR_TO_BTF_ID in this case. > > It is not a concern for the newer-btf-only helper like the bpf_skc_to_*() > here though because these helpers must require BTF vmlinux to begin > with. ] > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > include/linux/bpf.h | 1 + > kernel/bpf/verifier.c | 34 +++++++++++++++++++-- > net/core/filter.c | 69 ++++++++++++++++++++++++++++++------------- > 3 files changed, 82 insertions(+), 22 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index fc5c901c7542..d0937f1d2980 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -292,6 +292,7 @@ enum bpf_arg_type { > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ > + ARG_PTR_TO_BTF_ID_SOCK_COMMON, /* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */ > __BPF_ARG_TYPE_MAX, > }; > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 945fa2b4d096..d4ba29fb17a6 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -486,7 +486,12 @@ static bool is_acquire_function(enum bpf_func_id func_id, > static bool is_ptr_cast_function(enum bpf_func_id func_id) > { > return func_id == BPF_FUNC_tcp_sock || > - func_id == BPF_FUNC_sk_fullsock; > + func_id == BPF_FUNC_sk_fullsock || > + func_id == BPF_FUNC_skc_to_tcp_sock || > + func_id == BPF_FUNC_skc_to_tcp6_sock || > + func_id == BPF_FUNC_skc_to_udp6_sock || > + func_id == BPF_FUNC_skc_to_tcp_timewait_sock || > + func_id == BPF_FUNC_skc_to_tcp_request_sock; > } > > /* string representation of 'enum bpf_reg_type' */ > @@ -3953,6 +3958,7 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env, > > struct bpf_reg_types { > const enum bpf_reg_type types[10]; > + u32 *btf_id; > }; > > static const struct bpf_reg_types map_key_value_types = { > @@ -3973,6 +3979,17 @@ static const struct bpf_reg_types sock_types = { > }, > }; > > +static const struct bpf_reg_types btf_id_sock_common_types = { > + .types = { > + PTR_TO_SOCK_COMMON, > + PTR_TO_SOCKET, > + PTR_TO_TCP_SOCK, > + PTR_TO_XDP_SOCK, > + PTR_TO_BTF_ID, > + }, > + .btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], > +}; > + > static const struct bpf_reg_types mem_types = { > .types = { > PTR_TO_STACK, > @@ -4014,6 +4031,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > [ARG_PTR_TO_CTX] = &context_types, > [ARG_PTR_TO_CTX_OR_NULL] = &context_types, > [ARG_PTR_TO_SOCK_COMMON] = &sock_types, > + [ARG_PTR_TO_BTF_ID_SOCK_COMMON] = &btf_id_sock_common_types, > [ARG_PTR_TO_SOCKET] = &fullsock_types, > [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, > [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > @@ -4059,6 +4077,14 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > > found: > if (type == PTR_TO_BTF_ID) { > + if (!arg_btf_id) { > + if (!compatible->btf_id) { > + verbose(env, "verifier internal error: missing arg compatible BTF ID\n"); > + return -EFAULT; > + } > + arg_btf_id = compatible->btf_id; > + } > + > if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, > *arg_btf_id)) { > verbose(env, "R%d is of type %s but %s is expected\n", > @@ -4575,10 +4601,14 @@ static bool check_btf_id_ok(const struct bpf_func_proto *fn) > { > int i; > > - for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) > + for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) { > if (fn->arg_type[i] == ARG_PTR_TO_BTF_ID && !fn->arg_btf_id[i]) > return false; > > + if (fn->arg_type[i] != ARG_PTR_TO_BTF_ID && fn->arg_btf_id[i]) > + return false; > + } > + This is a hold over from the previous patchset? > return true; > } > > diff --git a/net/core/filter.c b/net/core/filter.c > index 706f8db0ccf8..6d1864f2bd51 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -77,6 +77,9 @@ > #include <net/transp_v6.h> > #include <linux/btf_ids.h> > > +static const struct bpf_func_proto * > +bpf_sk_base_func_proto(enum bpf_func_id func_id); > + > int copy_bpf_fprog_from_user(struct sock_fprog *dst, sockptr_t src, int len) > { > if (in_compat_syscall()) { > @@ -6620,7 +6623,7 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return NULL; > } > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -6639,7 +6642,7 @@ sk_filter_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > case BPF_FUNC_perf_event_output: > return &bpf_skb_event_output_proto; > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -6800,7 +6803,7 @@ tc_cls_act_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_sk_assign_proto; > #endif > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -6841,7 +6844,7 @@ xdp_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_tcp_gen_syncookie_proto; > #endif > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -6883,7 +6886,7 @@ sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_tcp_sock_proto; > #endif /* CONFIG_INET */ > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -6929,7 +6932,7 @@ sk_msg_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_get_cgroup_classid_curr_proto; > #endif > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -6971,7 +6974,7 @@ sk_skb_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > return &bpf_skc_lookup_tcp_proto; > #endif > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -6982,7 +6985,7 @@ flow_dissector_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > case BPF_FUNC_skb_load_bytes: > return &bpf_flow_dissector_load_bytes_proto; > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -7009,7 +7012,7 @@ lwt_out_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > case BPF_FUNC_skb_under_cgroup: > return &bpf_skb_under_cgroup_proto; > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -9746,7 +9749,7 @@ sk_lookup_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > case BPF_FUNC_sk_release: > return &bpf_sk_release_proto; > default: > - return bpf_base_func_proto(func_id); > + return bpf_sk_base_func_proto(func_id); > } > } > > @@ -9913,8 +9916,7 @@ const struct bpf_func_proto bpf_skc_to_tcp6_sock_proto = { > .func = bpf_skc_to_tcp6_sock, > .gpl_only = false, > .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, > - .arg1_type = ARG_PTR_TO_BTF_ID, > - .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, > .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP6], > }; > > @@ -9930,8 +9932,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_sock_proto = { > .func = bpf_skc_to_tcp_sock, > .gpl_only = false, > .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, > - .arg1_type = ARG_PTR_TO_BTF_ID, > - .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, > .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP], > }; > > @@ -9954,8 +9955,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_timewait_sock_proto = { > .func = bpf_skc_to_tcp_timewait_sock, > .gpl_only = false, > .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, > - .arg1_type = ARG_PTR_TO_BTF_ID, > - .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, > .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_TW], > }; > > @@ -9978,8 +9978,7 @@ const struct bpf_func_proto bpf_skc_to_tcp_request_sock_proto = { > .func = bpf_skc_to_tcp_request_sock, > .gpl_only = false, > .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, > - .arg1_type = ARG_PTR_TO_BTF_ID, > - .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, > .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_TCP_REQ], > }; > > @@ -10000,7 +9999,37 @@ const struct bpf_func_proto bpf_skc_to_udp6_sock_proto = { > .func = bpf_skc_to_udp6_sock, > .gpl_only = false, > .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, > - .arg1_type = ARG_PTR_TO_BTF_ID, > - .arg1_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], > + .arg1_type = ARG_PTR_TO_BTF_ID_SOCK_COMMON, > .ret_btf_id = &btf_sock_ids[BTF_SOCK_TYPE_UDP6], > }; > + > +static const struct bpf_func_proto * > +bpf_sk_base_func_proto(enum bpf_func_id func_id) > +{ > + const struct bpf_func_proto *func; > + > + switch (func_id) { > + case BPF_FUNC_skc_to_tcp6_sock: > + func = &bpf_skc_to_tcp6_sock_proto; > + break; > + case BPF_FUNC_skc_to_tcp_sock: > + func = &bpf_skc_to_tcp_sock_proto; > + break; > + case BPF_FUNC_skc_to_tcp_timewait_sock: > + func = &bpf_skc_to_tcp_timewait_sock_proto; > + break; > + case BPF_FUNC_skc_to_tcp_request_sock: > + func = &bpf_skc_to_tcp_request_sock_proto; > + break; > + case BPF_FUNC_skc_to_udp6_sock: > + func = &bpf_skc_to_udp6_sock_proto; > + break; > + default: > + return bpf_base_func_proto(func_id); > + } > + > + if (!perfmon_capable()) > + return NULL; > + > + return func; > +} > -- > 2.24.1 > -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
On Fri, 25 Sep 2020 at 01:03, Martin KaFai Lau <kafai@fb.com> wrote: > > This set allows networking prog type to directly read fields from > the in-kernel socket type, e.g. "struct tcp_sock". > > Patch 2 has the details on the use case. > > v3: > - Pass arg_btf_id instead of fn into check_reg_type() in Patch 1 (Lorenz) > - Move arg_btf_id from func_proto to struct bpf_reg_types in Patch 2 (Lorenz) > - Remove test_sock_fields from .gitignore in Patch 8 (Andrii) > - Add tests to have better coverage on the modified helpers (Alexei) > Patch 13 is added. > - Use "void *sk" as the helper argument in UAPI bpf.h > > v3: > - ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted in v2. The _OR_NULL was > needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL > PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly > gives an expectation that the helper can take a scalar NULL which does > not make sense in most (except one) helpers. Passing scalar NULL > should be rejected at the verification time. > > Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the > helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but > not scalar NULL. It requires the func_proto to explicitly specify the > arg_btf_id such that there is a very clear expectation that the helper > can handle a NULL PTR_TO_BTF_ID. > > v2: > - Add ARG_PTR_TO_SOCK_COMMON_OR_NULL (Lorenz) > > Martin KaFai Lau (13): > bpf: Move the PTR_TO_BTF_ID check to check_reg_type() > bpf: Enable bpf_skc_to_* sock casting helper to networking prog type > bpf: Change bpf_sk_release and bpf_sk_*cgroup_id to accept > ARG_PTR_TO_BTF_ID_SOCK_COMMON > bpf: Change bpf_sk_storage_*() to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON > bpf: Change bpf_tcp_*_syncookie to accept > ARG_PTR_TO_BTF_ID_SOCK_COMMON > bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON > bpf: selftest: Add ref_tracking verifier test for bpf_skc casting > bpf: selftest: Move sock_fields test into test_progs > bpf: selftest: Adapt sock_fields test to use skel and global variables > bpf: selftest: Use network_helpers in the sock_fields test > bpf: selftest: Use bpf_skc_to_tcp_sock() in the sock_fields test > bpf: selftest: Remove enum tcp_ca_state from bpf_tcp_helpers.h > bpf: selftest: Add test_btf_skc_cls_ingress LGTM, thanks!
On Fri, Sep 25, 2020 at 09:26:36AM +0100, Lorenz Bauer wrote: > On Fri, 25 Sep 2020 at 01:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > There is a constant need to add more fields into the bpf_tcp_sock > > for the bpf programs running at tc, sock_ops...etc. > > > > A current workaround could be to use bpf_probe_read_kernel(). However, > > other than making another helper call for reading each field and missing > > CO-RE, it is also not as intuitive to use as directly reading > > "tp->lsndtime" for example. While already having perfmon cap to do > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > > directly read from the tcp_sock. > > > > This patch tries to do that by using the existing casting-helpers > > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > > kernel "struct tcp_sock". > > > > These helpers are also added to is_ptr_cast_function(). > > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > > That will keep the ref-tracking works properly. > > > > The bpf_skc_to_* helpers are made available to most of the bpf prog > > types in filter.c. The bpf_skc_to_* helpers will be limited by > > perfmon cap. > > > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > > they will accept pointer obtained from skb->sk. > > > > Instead of specifying both arg_type and arg_btf_id in the same func_proto > > which is how the current ARG_PTR_TO_BTF_ID does, the arg_btf_id of > > the new ARG_PTR_TO_BTF_ID_SOCK_COMMON is specified in the > > compatible_reg_types[] in verifier.c. The reason is the arg_btf_id is > > always the same. Discussion in this thread: > > https://lore.kernel.org/bpf/20200922070422.1917351-1-kafai@fb.com/ > > > > The ARG_PTR_TO_BTF_ID_ part gives a clear expectation that the helper is > > expecting a PTR_TO_BTF_ID which could be NULL. This is the same > > behavior as the existing helper taking ARG_PTR_TO_BTF_ID. > > > > The _SOCK_COMMON part means the helper is also expecting the legacy > > SOCK_COMMON pointer. > > > > By excluding the _OR_NULL part, the bpf prog cannot call helper > > with a literal NULL which doesn't make sense in most cases. > > e.g. bpf_skc_to_tcp_sock(NULL) will be rejected. All PTR_TO_*_OR_NULL > > reg has to do a NULL check first before passing into the helper or else > > the bpf prog will be rejected. This behavior is nothing new and > > consistent with the current expectation during bpf-prog-load. > > > > [ ARG_PTR_TO_BTF_ID_SOCK_COMMON will be used to replace > > ARG_PTR_TO_SOCK* of other existing helpers later such that > > those existing helpers can take the PTR_TO_BTF_ID returned by > > the bpf_skc_to_*() helpers. > > > > The only special case is bpf_sk_lookup_assign() which can accept a > > literal NULL ptr. It has to be handled specially in another follow > > up patch if there is a need (e.g. by renaming ARG_PTR_TO_SOCKET_OR_NULL > > to ARG_PTR_TO_BTF_ID_SOCK_COMMON_OR_NULL). ] > > > > [ When converting the older helpers that take ARG_PTR_TO_SOCK* in > > the later patch, if the kernel does not support BTF, > > ARG_PTR_TO_BTF_ID_SOCK_COMMON will behave like ARG_PTR_TO_SOCK_COMMON > > because no reg->type could have PTR_TO_BTF_ID in this case. > > > > It is not a concern for the newer-btf-only helper like the bpf_skc_to_*() > > here though because these helpers must require BTF vmlinux to begin > > with. ] > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > --- > > include/linux/bpf.h | 1 + > > kernel/bpf/verifier.c | 34 +++++++++++++++++++-- > > net/core/filter.c | 69 ++++++++++++++++++++++++++++++------------- > > 3 files changed, 82 insertions(+), 22 deletions(-) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index fc5c901c7542..d0937f1d2980 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -292,6 +292,7 @@ enum bpf_arg_type { > > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ > > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ > > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ > > + ARG_PTR_TO_BTF_ID_SOCK_COMMON, /* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */ > > __BPF_ARG_TYPE_MAX, > > }; > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 945fa2b4d096..d4ba29fb17a6 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -486,7 +486,12 @@ static bool is_acquire_function(enum bpf_func_id func_id, > > static bool is_ptr_cast_function(enum bpf_func_id func_id) > > { > > return func_id == BPF_FUNC_tcp_sock || > > - func_id == BPF_FUNC_sk_fullsock; > > + func_id == BPF_FUNC_sk_fullsock || > > + func_id == BPF_FUNC_skc_to_tcp_sock || > > + func_id == BPF_FUNC_skc_to_tcp6_sock || > > + func_id == BPF_FUNC_skc_to_udp6_sock || > > + func_id == BPF_FUNC_skc_to_tcp_timewait_sock || > > + func_id == BPF_FUNC_skc_to_tcp_request_sock; > > } > > > > /* string representation of 'enum bpf_reg_type' */ > > @@ -3953,6 +3958,7 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env, > > > > struct bpf_reg_types { > > const enum bpf_reg_type types[10]; > > + u32 *btf_id; > > }; > > > > static const struct bpf_reg_types map_key_value_types = { > > @@ -3973,6 +3979,17 @@ static const struct bpf_reg_types sock_types = { > > }, > > }; > > > > +static const struct bpf_reg_types btf_id_sock_common_types = { > > + .types = { > > + PTR_TO_SOCK_COMMON, > > + PTR_TO_SOCKET, > > + PTR_TO_TCP_SOCK, > > + PTR_TO_XDP_SOCK, > > + PTR_TO_BTF_ID, > > + }, > > + .btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], > > +}; > > + > > static const struct bpf_reg_types mem_types = { > > .types = { > > PTR_TO_STACK, > > @@ -4014,6 +4031,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > > [ARG_PTR_TO_CTX] = &context_types, > > [ARG_PTR_TO_CTX_OR_NULL] = &context_types, > > [ARG_PTR_TO_SOCK_COMMON] = &sock_types, > > + [ARG_PTR_TO_BTF_ID_SOCK_COMMON] = &btf_id_sock_common_types, > > [ARG_PTR_TO_SOCKET] = &fullsock_types, > > [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, > > [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > > @@ -4059,6 +4077,14 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > > > > found: > > if (type == PTR_TO_BTF_ID) { > > + if (!arg_btf_id) { > > + if (!compatible->btf_id) { > > + verbose(env, "verifier internal error: missing arg compatible BTF ID\n"); > > + return -EFAULT; > > + } > > + arg_btf_id = compatible->btf_id; > > + } > > + > > if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, > > *arg_btf_id)) { > > verbose(env, "R%d is of type %s but %s is expected\n", > > @@ -4575,10 +4601,14 @@ static bool check_btf_id_ok(const struct bpf_func_proto *fn) > > { > > int i; > > > > - for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) > > + for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) { > > if (fn->arg_type[i] == ARG_PTR_TO_BTF_ID && !fn->arg_btf_id[i]) > > return false; > > > > + if (fn->arg_type[i] != ARG_PTR_TO_BTF_ID && fn->arg_btf_id[i]) > > + return false; > > + } > > + > > This is a hold over from the previous patchset? hmm... what do you mean?
Martin KaFai Lau wrote: > check_reg_type() checks whether a reg can be used as an arg of a > func_proto. For PTR_TO_BTF_ID, the check is actually not > completely done until the reg->btf_id is pointing to a > kernel struct that is acceptable by the func_proto. > > Thus, this patch moves the btf_id check into check_reg_type(). > "arg_type" and "arg_btf_id" are passed to check_reg_type() instead of > "compatible". The compatible_reg_types[] usage is localized in > check_reg_type() now. > > The "if (!btf_id) verbose(...); " is also removed since it won't happen. > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- Acked-by: John Fastabend <john.fastabend@gmail.com>
On Fri, 25 Sep 2020 at 14:18, Martin KaFai Lau <kafai@fb.com> wrote: > > On Fri, Sep 25, 2020 at 09:26:36AM +0100, Lorenz Bauer wrote: > > On Fri, 25 Sep 2020 at 01:04, Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > There is a constant need to add more fields into the bpf_tcp_sock > > > for the bpf programs running at tc, sock_ops...etc. > > > > > > A current workaround could be to use bpf_probe_read_kernel(). However, > > > other than making another helper call for reading each field and missing > > > CO-RE, it is also not as intuitive to use as directly reading > > > "tp->lsndtime" for example. While already having perfmon cap to do > > > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > > > directly read from the tcp_sock. > > > > > > This patch tries to do that by using the existing casting-helpers > > > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > > > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > > > kernel "struct tcp_sock". > > > > > > These helpers are also added to is_ptr_cast_function(). > > > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > > > That will keep the ref-tracking works properly. > > > > > > The bpf_skc_to_* helpers are made available to most of the bpf prog > > > types in filter.c. The bpf_skc_to_* helpers will be limited by > > > perfmon cap. > > > > > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > > > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > > > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > > > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > > > they will accept pointer obtained from skb->sk. > > > > > > Instead of specifying both arg_type and arg_btf_id in the same func_proto > > > which is how the current ARG_PTR_TO_BTF_ID does, the arg_btf_id of > > > the new ARG_PTR_TO_BTF_ID_SOCK_COMMON is specified in the > > > compatible_reg_types[] in verifier.c. The reason is the arg_btf_id is > > > always the same. Discussion in this thread: > > > https://lore.kernel.org/bpf/20200922070422.1917351-1-kafai@fb.com/ > > > > > > The ARG_PTR_TO_BTF_ID_ part gives a clear expectation that the helper is > > > expecting a PTR_TO_BTF_ID which could be NULL. This is the same > > > behavior as the existing helper taking ARG_PTR_TO_BTF_ID. > > > > > > The _SOCK_COMMON part means the helper is also expecting the legacy > > > SOCK_COMMON pointer. > > > > > > By excluding the _OR_NULL part, the bpf prog cannot call helper > > > with a literal NULL which doesn't make sense in most cases. > > > e.g. bpf_skc_to_tcp_sock(NULL) will be rejected. All PTR_TO_*_OR_NULL > > > reg has to do a NULL check first before passing into the helper or else > > > the bpf prog will be rejected. This behavior is nothing new and > > > consistent with the current expectation during bpf-prog-load. > > > > > > [ ARG_PTR_TO_BTF_ID_SOCK_COMMON will be used to replace > > > ARG_PTR_TO_SOCK* of other existing helpers later such that > > > those existing helpers can take the PTR_TO_BTF_ID returned by > > > the bpf_skc_to_*() helpers. > > > > > > The only special case is bpf_sk_lookup_assign() which can accept a > > > literal NULL ptr. It has to be handled specially in another follow > > > up patch if there is a need (e.g. by renaming ARG_PTR_TO_SOCKET_OR_NULL > > > to ARG_PTR_TO_BTF_ID_SOCK_COMMON_OR_NULL). ] > > > > > > [ When converting the older helpers that take ARG_PTR_TO_SOCK* in > > > the later patch, if the kernel does not support BTF, > > > ARG_PTR_TO_BTF_ID_SOCK_COMMON will behave like ARG_PTR_TO_SOCK_COMMON > > > because no reg->type could have PTR_TO_BTF_ID in this case. > > > > > > It is not a concern for the newer-btf-only helper like the bpf_skc_to_*() > > > here though because these helpers must require BTF vmlinux to begin > > > with. ] > > > > > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > > > --- > > > include/linux/bpf.h | 1 + > > > kernel/bpf/verifier.c | 34 +++++++++++++++++++-- > > > net/core/filter.c | 69 ++++++++++++++++++++++++++++++------------- > > > 3 files changed, 82 insertions(+), 22 deletions(-) > > > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > > index fc5c901c7542..d0937f1d2980 100644 > > > --- a/include/linux/bpf.h > > > +++ b/include/linux/bpf.h > > > @@ -292,6 +292,7 @@ enum bpf_arg_type { > > > ARG_PTR_TO_ALLOC_MEM, /* pointer to dynamically allocated memory */ > > > ARG_PTR_TO_ALLOC_MEM_OR_NULL, /* pointer to dynamically allocated memory or NULL */ > > > ARG_CONST_ALLOC_SIZE_OR_ZERO, /* number of allocated bytes requested */ > > > + ARG_PTR_TO_BTF_ID_SOCK_COMMON, /* pointer to in-kernel sock_common or bpf-mirrored bpf_sock */ > > > __BPF_ARG_TYPE_MAX, > > > }; > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index 945fa2b4d096..d4ba29fb17a6 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -486,7 +486,12 @@ static bool is_acquire_function(enum bpf_func_id func_id, > > > static bool is_ptr_cast_function(enum bpf_func_id func_id) > > > { > > > return func_id == BPF_FUNC_tcp_sock || > > > - func_id == BPF_FUNC_sk_fullsock; > > > + func_id == BPF_FUNC_sk_fullsock || > > > + func_id == BPF_FUNC_skc_to_tcp_sock || > > > + func_id == BPF_FUNC_skc_to_tcp6_sock || > > > + func_id == BPF_FUNC_skc_to_udp6_sock || > > > + func_id == BPF_FUNC_skc_to_tcp_timewait_sock || > > > + func_id == BPF_FUNC_skc_to_tcp_request_sock; > > > } > > > > > > /* string representation of 'enum bpf_reg_type' */ > > > @@ -3953,6 +3958,7 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env, > > > > > > struct bpf_reg_types { > > > const enum bpf_reg_type types[10]; > > > + u32 *btf_id; > > > }; > > > > > > static const struct bpf_reg_types map_key_value_types = { > > > @@ -3973,6 +3979,17 @@ static const struct bpf_reg_types sock_types = { > > > }, > > > }; > > > > > > +static const struct bpf_reg_types btf_id_sock_common_types = { > > > + .types = { > > > + PTR_TO_SOCK_COMMON, > > > + PTR_TO_SOCKET, > > > + PTR_TO_TCP_SOCK, > > > + PTR_TO_XDP_SOCK, > > > + PTR_TO_BTF_ID, > > > + }, > > > + .btf_id = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON], > > > +}; > > > + > > > static const struct bpf_reg_types mem_types = { > > > .types = { > > > PTR_TO_STACK, > > > @@ -4014,6 +4031,7 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = { > > > [ARG_PTR_TO_CTX] = &context_types, > > > [ARG_PTR_TO_CTX_OR_NULL] = &context_types, > > > [ARG_PTR_TO_SOCK_COMMON] = &sock_types, > > > + [ARG_PTR_TO_BTF_ID_SOCK_COMMON] = &btf_id_sock_common_types, > > > [ARG_PTR_TO_SOCKET] = &fullsock_types, > > > [ARG_PTR_TO_SOCKET_OR_NULL] = &fullsock_types, > > > [ARG_PTR_TO_BTF_ID] = &btf_ptr_types, > > > @@ -4059,6 +4077,14 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > > > > > > found: > > > if (type == PTR_TO_BTF_ID) { > > > + if (!arg_btf_id) { > > > + if (!compatible->btf_id) { > > > + verbose(env, "verifier internal error: missing arg compatible BTF ID\n"); > > > + return -EFAULT; > > > + } > > > + arg_btf_id = compatible->btf_id; > > > + } > > > + > > > if (!btf_struct_ids_match(&env->log, reg->off, reg->btf_id, > > > *arg_btf_id)) { > > > verbose(env, "R%d is of type %s but %s is expected\n", > > > @@ -4575,10 +4601,14 @@ static bool check_btf_id_ok(const struct bpf_func_proto *fn) > > > { > > > int i; > > > > > > - for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) > > > + for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) { > > > if (fn->arg_type[i] == ARG_PTR_TO_BTF_ID && !fn->arg_btf_id[i]) > > > return false; > > > > > > + if (fn->arg_type[i] != ARG_PTR_TO_BTF_ID && fn->arg_btf_id[i]) > > > + return false; > > > + } > > > + > > > > This is a hold over from the previous patchset? > hmm... what do you mean? Sorry, I misread the patch!
Martin KaFai Lau wrote: > There is a constant need to add more fields into the bpf_tcp_sock > for the bpf programs running at tc, sock_ops...etc. > > A current workaround could be to use bpf_probe_read_kernel(). However, > other than making another helper call for reading each field and missing > CO-RE, it is also not as intuitive to use as directly reading > "tp->lsndtime" for example. While already having perfmon cap to do > bpf_probe_read_kernel(), it will be much easier if the bpf prog can > directly read from the tcp_sock. > > This patch tries to do that by using the existing casting-helpers > bpf_skc_to_*() whose func_proto returns a btf_id. For example, the > func_proto of bpf_skc_to_tcp_sock returns the btf_id of the > kernel "struct tcp_sock". > > These helpers are also added to is_ptr_cast_function(). > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id. > That will keep the ref-tracking works properly. > > The bpf_skc_to_* helpers are made available to most of the bpf prog > types in filter.c. The bpf_skc_to_* helpers will be limited by > perfmon cap. > > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON. The helper accepting > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]) > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON). The bpf_skc_to_*() > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that > they will accept pointer obtained from skb->sk. > > Instead of specifying both arg_type and arg_btf_id in the same func_proto > which is how the current ARG_PTR_TO_BTF_ID does, the arg_btf_id of > the new ARG_PTR_TO_BTF_ID_SOCK_COMMON is specified in the > compatible_reg_types[] in verifier.c. The reason is the arg_btf_id is > always the same. Discussion in this thread: > https://lore.kernel.org/bpf/20200922070422.1917351-1-kafai@fb.com/ > > The ARG_PTR_TO_BTF_ID_ part gives a clear expectation that the helper is > expecting a PTR_TO_BTF_ID which could be NULL. This is the same > behavior as the existing helper taking ARG_PTR_TO_BTF_ID. > > The _SOCK_COMMON part means the helper is also expecting the legacy > SOCK_COMMON pointer. > > By excluding the _OR_NULL part, the bpf prog cannot call helper > with a literal NULL which doesn't make sense in most cases. > e.g. bpf_skc_to_tcp_sock(NULL) will be rejected. All PTR_TO_*_OR_NULL > reg has to do a NULL check first before passing into the helper or else > the bpf prog will be rejected. This behavior is nothing new and > consistent with the current expectation during bpf-prog-load. > > [ ARG_PTR_TO_BTF_ID_SOCK_COMMON will be used to replace > ARG_PTR_TO_SOCK* of other existing helpers later such that > those existing helpers can take the PTR_TO_BTF_ID returned by > the bpf_skc_to_*() helpers. > > The only special case is bpf_sk_lookup_assign() which can accept a > literal NULL ptr. It has to be handled specially in another follow > up patch if there is a need (e.g. by renaming ARG_PTR_TO_SOCKET_OR_NULL > to ARG_PTR_TO_BTF_ID_SOCK_COMMON_OR_NULL). ] > > [ When converting the older helpers that take ARG_PTR_TO_SOCK* in > the later patch, if the kernel does not support BTF, > ARG_PTR_TO_BTF_ID_SOCK_COMMON will behave like ARG_PTR_TO_SOCK_COMMON > because no reg->type could have PTR_TO_BTF_ID in this case. > > It is not a concern for the newer-btf-only helper like the bpf_skc_to_*() > here though because these helpers must require BTF vmlinux to begin > with. ] > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- LGTM it took a bit of looking around to convince myself that we have ret_type set to PTR_TO_SOCK_*_OR_NULL types in the sk lookup helpers so that we force a null check before passing these into the skc_to_* helpers here, but I didn't see any holes. Thanks. Acked-by: John Fastabend <john.fastabend@gmail.com> > @@ -4575,10 +4601,14 @@ static bool check_btf_id_ok(const struct bpf_func_proto *fn) > { > int i; > > - for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) > + for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) { > if (fn->arg_type[i] == ARG_PTR_TO_BTF_ID && !fn->arg_btf_id[i]) > return false; > > + if (fn->arg_type[i] != ARG_PTR_TO_BTF_ID && fn->arg_btf_id[i]) > + return false; > + } > + I guess this case is harmless? If some other arg has a btf_id its setup wrong, so nice to fail here I think. > return true; > } >
On 9/25/20 6:50 AM, Lorenz Bauer wrote: >>>> - for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) >>>> + for (i = 0; i < ARRAY_SIZE(fn->arg_type); i++) { >>>> if (fn->arg_type[i] == ARG_PTR_TO_BTF_ID && !fn->arg_btf_id[i]) >>>> return false; >>>> >>>> + if (fn->arg_type[i] != ARG_PTR_TO_BTF_ID && fn->arg_btf_id[i]) >>>> + return false; >>>> + } >>>> + >>> This is a hold over from the previous patchset? >> hmm... what do you mean? > Sorry, I misread the patch! Folks, please trim your replies. You could have quoted just few lines above instead of most of the patch. Scrolling takes time for those of us who used tbird, mutt, etc.
Martin KaFai Lau wrote: > This patch attaches a classifier prog to the ingress filter. > It exercises the following helpers with different socket pointer > types in different logical branches: > 1. bpf_sk_release() > 2. bpf_sk_assign() > 3. bpf_skc_to_tcp_request_sock(), bpf_skc_to_tcp_sock() > 4. bpf_tcp_gen_syncookie, bpf_tcp_check_syncookie > > Signed-off-by: Martin KaFai Lau <kafai@fb.com> > --- > tools/testing/selftests/bpf/bpf_tcp_helpers.h | 5 + > .../bpf/prog_tests/btf_skc_cls_ingress.c | 234 ++++++++++++++++++ > .../bpf/progs/test_btf_skc_cls_ingress.c | 174 +++++++++++++ > 3 files changed, 413 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c > create mode 100644 tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c > Hi Martin, One piece I'm missing is how does this handle null pointer dereferences from network side when reading BTF objects? I've not got through all the code yet so maybe I'm just not there yet. For example, > diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > index a0e8b3758bd7..2915664c335d 100644 > --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h > +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > @@ -16,6 +16,7 @@ BPF_PROG(name, args) > > struct sock_common { > unsigned char skc_state; > + __u16 skc_num; > } __attribute__((preserve_access_index)); > > enum sk_pacing { > @@ -45,6 +46,10 @@ struct inet_connection_sock { > __u64 icsk_ca_priv[104 / sizeof(__u64)]; > } __attribute__((preserve_access_index)); > > +struct request_sock { > + struct sock_common __req_common; > +} __attribute__((preserve_access_index)); > + > struct tcp_sock { > struct inet_connection_sock inet_conn; add some pointer from tcp_sock which is likely not set so should be NULL, struct tcp_fastopen_request *fastopen_req; [...] > + if (bpf_skc->state == BPF_TCP_NEW_SYN_RECV) { > + struct request_sock *req_sk; > + > + req_sk = (struct request_sock *)bpf_skc_to_tcp_request_sock(bpf_skc); > + if (!req_sk) { > + LOG(); > + goto release; > + } > + > + if (bpf_sk_assign(skb, req_sk, 0)) { > + LOG(); > + goto release; > + } > + > + req_sk_sport = req_sk->__req_common.skc_num; > + > + bpf_sk_release(req_sk); > + return TC_ACT_OK; > + } else if (bpf_skc->state == BPF_TCP_LISTEN) { > + struct tcp_sock *tp; > + > + tp = bpf_skc_to_tcp_sock(bpf_skc); > + if (!tp) { > + LOG(); > + goto release; > + } > + > + if (bpf_sk_assign(skb, tp, 0)) { > + LOG(); > + goto release; > + } > + Then use it here without a null check in the BPF program, fastopen = tp->fastopen_req; if (fastopen->size > 0x1234) (do something) > + listen_tp_sport = tp->inet_conn.icsk_inet.sk.__sk_common.skc_num; > + > + test_syncookie_helper(ip6h, th, tp, skb); > + bpf_sk_release(tp); > + return TC_ACT_OK; > + } My quick check shows this didn't actually fault and the xlated looks like it did the read and dereference. Must be missing something? We shouldn't have fault_handler set for cls_ingress. Perhaps a comment in the cover letter about this would be helpful? Or if I'm just being dense this morning let me know as well. ;) > + > + if (bpf_sk_assign(skb, bpf_skc, 0)) > + LOG(); > + > +release: > + bpf_sk_release(bpf_skc); > + return TC_ACT_OK; > +}
On Fri, Sep 25, 2020 at 09:24:02AM -0700, John Fastabend wrote: [ ... ] > Hi Martin, > > One piece I'm missing is how does this handle null pointer dereferences > from network side when reading BTF objects? I've not got through all the > code yet so maybe I'm just not there yet. > > For example, > > > diff --git a/tools/testing/selftests/bpf/bpf_tcp_helpers.h b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > index a0e8b3758bd7..2915664c335d 100644 > > --- a/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > +++ b/tools/testing/selftests/bpf/bpf_tcp_helpers.h > > @@ -16,6 +16,7 @@ BPF_PROG(name, args) > > > > struct sock_common { > > unsigned char skc_state; > > + __u16 skc_num; > > } __attribute__((preserve_access_index)); > > > > enum sk_pacing { > > @@ -45,6 +46,10 @@ struct inet_connection_sock { > > __u64 icsk_ca_priv[104 / sizeof(__u64)]; > > } __attribute__((preserve_access_index)); > > > > +struct request_sock { > > + struct sock_common __req_common; > > +} __attribute__((preserve_access_index)); > > + > > struct tcp_sock { > > struct inet_connection_sock inet_conn; > > add some pointer from tcp_sock which is likely not set so should be NULL, > > struct tcp_fastopen_request *fastopen_req; > > [...] > > > + if (bpf_skc->state == BPF_TCP_NEW_SYN_RECV) { > > + struct request_sock *req_sk; > > + > > + req_sk = (struct request_sock *)bpf_skc_to_tcp_request_sock(bpf_skc); > > + if (!req_sk) { > > + LOG(); > > + goto release; > > + } > > + > > + if (bpf_sk_assign(skb, req_sk, 0)) { > > + LOG(); > > + goto release; > > + } > > + > > + req_sk_sport = req_sk->__req_common.skc_num; > > + > > + bpf_sk_release(req_sk); > > + return TC_ACT_OK; > > + } else if (bpf_skc->state == BPF_TCP_LISTEN) { > > + struct tcp_sock *tp; > > + > > + tp = bpf_skc_to_tcp_sock(bpf_skc); > > + if (!tp) { > > + LOG(); > > + goto release; > > + } > > + > > + if (bpf_sk_assign(skb, tp, 0)) { > > + LOG(); > > + goto release; > > + } > > + > > > Then use it here without a null check in the BPF program, > > fastopen = tp->fastopen_req; fastopen is in PTR_TO_BTF_ID here. > if (fastopen->size > 0x1234) This load will be marked with BPF_PROBE_MEM. > (do something) > > > + listen_tp_sport = tp->inet_conn.icsk_inet.sk.__sk_common.skc_num; > > + > > + test_syncookie_helper(ip6h, th, tp, skb); > > + bpf_sk_release(tp); > > + return TC_ACT_OK; > > + } > > My quick check shows this didn't actually fault and the xlated > looks like it did the read and dereference. Must be missing > something? We shouldn't have fault_handler set for cls_ingress. By xlated, do you mean the interpreter mode? The LDX_PROBE_MEM is done by bpf_probe_read_kernel() in bpf/core.c. I don't think the handling of PTR_TO_BTF_ID is depending on prog->type. > > Perhaps a comment in the cover letter about this would be > helpful? Or if I'm just being dense this morning let me know > as well. ;) >
On Thu, Sep 24, 2020 at 5:03 PM Martin KaFai Lau <kafai@fb.com> wrote: > > This set allows networking prog type to directly read fields from > the in-kernel socket type, e.g. "struct tcp_sock". Applied. Thanks
This set allows networking prog type to directly read fields from the in-kernel socket type, e.g. "struct tcp_sock". Patch 2 has the details on the use case. v3: - Pass arg_btf_id instead of fn into check_reg_type() in Patch 1 (Lorenz) - Move arg_btf_id from func_proto to struct bpf_reg_types in Patch 2 (Lorenz) - Remove test_sock_fields from .gitignore in Patch 8 (Andrii) - Add tests to have better coverage on the modified helpers (Alexei) Patch 13 is added. - Use "void *sk" as the helper argument in UAPI bpf.h v3: - ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted in v2. The _OR_NULL was needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL PTR_TO_BTF_ID is not a scalar NULL to the verifier. "_OR_NULL" implicitly gives an expectation that the helper can take a scalar NULL which does not make sense in most (except one) helpers. Passing scalar NULL should be rejected at the verification time. Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but not scalar NULL. It requires the func_proto to explicitly specify the arg_btf_id such that there is a very clear expectation that the helper can handle a NULL PTR_TO_BTF_ID. v2: - Add ARG_PTR_TO_SOCK_COMMON_OR_NULL (Lorenz) Martin KaFai Lau (13): bpf: Move the PTR_TO_BTF_ID check to check_reg_type() bpf: Enable bpf_skc_to_* sock casting helper to networking prog type bpf: Change bpf_sk_release and bpf_sk_*cgroup_id to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON bpf: Change bpf_sk_storage_*() to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON bpf: Change bpf_tcp_*_syncookie to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON bpf: Change bpf_sk_assign to accept ARG_PTR_TO_BTF_ID_SOCK_COMMON bpf: selftest: Add ref_tracking verifier test for bpf_skc casting bpf: selftest: Move sock_fields test into test_progs bpf: selftest: Adapt sock_fields test to use skel and global variables bpf: selftest: Use network_helpers in the sock_fields test bpf: selftest: Use bpf_skc_to_tcp_sock() in the sock_fields test bpf: selftest: Remove enum tcp_ca_state from bpf_tcp_helpers.h bpf: selftest: Add test_btf_skc_cls_ingress include/linux/bpf.h | 1 + include/net/bpf_sk_storage.h | 2 - include/uapi/linux/bpf.h | 15 +- kernel/bpf/bpf_lsm.c | 4 +- kernel/bpf/verifier.c | 94 ++-- net/core/bpf_sk_storage.c | 29 +- net/core/filter.c | 111 ++-- net/ipv4/bpf_tcp_ca.c | 23 +- tools/include/uapi/linux/bpf.h | 15 +- tools/testing/selftests/bpf/.gitignore | 1 - tools/testing/selftests/bpf/Makefile | 2 +- tools/testing/selftests/bpf/bpf_tcp_helpers.h | 13 +- .../bpf/prog_tests/btf_skc_cls_ingress.c | 234 +++++++++ .../selftests/bpf/prog_tests/sock_fields.c | 382 ++++++++++++++ tools/testing/selftests/bpf/progs/bpf_cubic.c | 2 + tools/testing/selftests/bpf/progs/bpf_dctcp.c | 2 + .../bpf/progs/test_btf_skc_cls_ingress.c | 174 +++++++ ..._sock_fields_kern.c => test_sock_fields.c} | 176 ++++--- .../testing/selftests/bpf/test_sock_fields.c | 482 ------------------ .../selftests/bpf/verifier/ref_tracking.c | 47 ++ 20 files changed, 1093 insertions(+), 716 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/btf_skc_cls_ingress.c create mode 100644 tools/testing/selftests/bpf/prog_tests/sock_fields.c create mode 100644 tools/testing/selftests/bpf/progs/test_btf_skc_cls_ingress.c rename tools/testing/selftests/bpf/progs/{test_sock_fields_kern.c => test_sock_fields.c} (61%) delete mode 100644 tools/testing/selftests/bpf/test_sock_fields.c