| Message ID | 20210707214751.159713-4-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf, x86: Add bpf_get_func_ip helper | expand |
On Wed, Jul 7, 2021 at 2:53 PM Jiri Olsa <jolsa@redhat.com> wrote: > > Adding bpf_get_func_ip helper for BPF_PROG_TYPE_TRACING programs, > specifically for all trampoline attach types. > > The trampoline's caller IP address is stored in (ctx - 8) address. > so there's no reason to actually call the helper, but rather fixup > the call instruction and return [ctx - 8] value directly (suggested > by Alexei). > > [fixed has_get_func_ip wrong return type] > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/uapi/linux/bpf.h | 7 +++++ > kernel/bpf/verifier.c | 53 ++++++++++++++++++++++++++++++++++ > kernel/trace/bpf_trace.c | 15 ++++++++++ > tools/include/uapi/linux/bpf.h | 7 +++++ > 4 files changed, 82 insertions(+) > [...] > static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > int *insn_idx_p) > { > @@ -6225,6 +6256,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > if (func_id == BPF_FUNC_get_stackid || func_id == BPF_FUNC_get_stack) > env->prog->call_get_stack = true; > > + if (func_id == BPF_FUNC_get_func_ip) { > + if (has_get_func_ip(env)) from has_xxx name I'd expect it returns true/false, so this reads super confusing. check_get_func_ip would be a bit more consistent with other cases like this (still reads confusing to me, but that's ok) > + return -ENOTSUPP; > + env->prog->call_get_func_ip = true; > + } > + > if (changes_data) > clear_all_pkt_pointers(env); > return 0; [...]
On Wed, Jul 07, 2021 at 11:47:47PM +0200, Jiri Olsa wrote: > > +static bool allow_get_func_ip_tracing(struct bpf_verifier_env *env) > +{ > + return env->prog->jit_requested && IS_ENABLED(CONFIG_X86_64); Why does it have to be gated by 'jited && x86_64' ? It's gated by bpf trampoline and it's only implemented on x86_64 so far. The trampoline has plenty of features. I would expect bpf trampoline for arm64 to implement all of them. If not the func_ip would be just one of the trampoline features that couldn't be implemented and at that time we'd need a flag mask of a sort, but I'd rather push of feature equivalence between trampoline implementations. Then jited part also doesn't seem to be necessary. The trampoline passed pointer to a stack in R1. Interpreter should deal with BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, -8) insn the same way and it should work, since trampoline prepared it. What did I miss? > +static int has_get_func_ip(struct bpf_verifier_env *env) > +{ > + enum bpf_attach_type eatype = env->prog->expected_attach_type; > + enum bpf_prog_type type = resolve_prog_type(env->prog); > + int func_id = BPF_FUNC_get_func_ip; > + > + if (type == BPF_PROG_TYPE_TRACING) { > + if (eatype != BPF_TRACE_FENTRY && eatype != BPF_TRACE_FEXIT && > + eatype != BPF_MODIFY_RETURN) { > + verbose(env, "func %s#%d supported only for fentry/fexit/fmod_ret programs\n", > + func_id_name(func_id), func_id); > + return -ENOTSUPP; > + } > + if (!allow_get_func_ip_tracing(env)) { > + verbose(env, "func %s#%d for tracing programs supported only for JITed x86_64\n", > + func_id_name(func_id), func_id); > + return -ENOTSUPP; > + } > + return 0; > + } > + > + verbose(env, "func %s#%d not supported for program type %d\n", > + func_id_name(func_id), func_id, type); > + return -ENOTSUPP; > +} > + > static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > int *insn_idx_p) > { > @@ -6225,6 +6256,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > if (func_id == BPF_FUNC_get_stackid || func_id == BPF_FUNC_get_stack) > env->prog->call_get_stack = true; > > + if (func_id == BPF_FUNC_get_func_ip) { > + if (has_get_func_ip(env)) > + return -ENOTSUPP; > + env->prog->call_get_func_ip = true; > + } > + > if (changes_data) > clear_all_pkt_pointers(env); > return 0; > @@ -12369,6 +12406,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env) > { > struct bpf_prog *prog = env->prog; > bool expect_blinding = bpf_jit_blinding_enabled(prog); > + enum bpf_prog_type prog_type = resolve_prog_type(prog); > struct bpf_insn *insn = prog->insnsi; > const struct bpf_func_proto *fn; > const int insn_cnt = prog->len; > @@ -12702,6 +12740,21 @@ static int do_misc_fixups(struct bpf_verifier_env *env) > continue; > } > > + /* Implement bpf_get_func_ip inline. */ > + if (prog_type == BPF_PROG_TYPE_TRACING && > + insn->imm == BPF_FUNC_get_func_ip) { > + /* Load IP address from ctx - 8 */ > + insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, -8); > + > + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, 1); > + if (!new_prog) > + return -ENOMEM; > + > + env->prog = prog = new_prog; > + insn = new_prog->insnsi + i + delta; > + continue; > + } > + > patch_call_imm: > fn = env->ops->get_func_proto(insn->imm, env->prog); > /* all functions that have prototype and verifier allowed > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index 64bd2d84367f..9edd3b1a00ad 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -948,6 +948,19 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { > .arg5_type = ARG_ANYTHING, > }; > > +BPF_CALL_1(bpf_get_func_ip_tracing, void *, ctx) > +{ > + /* Stub, the helper call is inlined in the program. */ > + return 0; > +} may be add a WARN in here that it should never be executed ? Or may be add an actual implementation: return ((u64 *)ctx)[-1]; and check that it works without inlining by the verifier?
On Wed, Jul 07, 2021 at 07:11:23PM -0700, Alexei Starovoitov wrote: > On Wed, Jul 07, 2021 at 11:47:47PM +0200, Jiri Olsa wrote: > > > > +static bool allow_get_func_ip_tracing(struct bpf_verifier_env *env) > > +{ > > + return env->prog->jit_requested && IS_ENABLED(CONFIG_X86_64); > > Why does it have to be gated by 'jited && x86_64' ? > It's gated by bpf trampoline and it's only implemented on x86_64 so far. > The trampoline has plenty of features. I would expect bpf trampoline > for arm64 to implement all of them. If not the func_ip would be just > one of the trampoline features that couldn't be implemented and at that > time we'd need a flag mask of a sort, but I'd rather push of feature > equivalence between trampoline implementations. ok, check for trampoline's prog types should be enough > > Then jited part also doesn't seem to be necessary. > The trampoline passed pointer to a stack in R1. > Interpreter should deal with BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, -8) insn > the same way and it should work, since trampoline prepared it. > What did I miss? ah right.. will remove that SNIP > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index 64bd2d84367f..9edd3b1a00ad 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -948,6 +948,19 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { > > .arg5_type = ARG_ANYTHING, > > }; > > > > +BPF_CALL_1(bpf_get_func_ip_tracing, void *, ctx) > > +{ > > + /* Stub, the helper call is inlined in the program. */ > > + return 0; > > +} > > may be add a WARN in here that it should never be executed ? > Or may be add an actual implementation: > return ((u64 *)ctx)[-1]; > and check that it works without inlining by the verifier? > sure, but having tracing program with this helper, it will be always inlined, right? I can't see how it could be skipped thanks, jirka
On Wed, Jul 07, 2021 at 05:06:17PM -0700, Andrii Nakryiko wrote: > On Wed, Jul 7, 2021 at 2:53 PM Jiri Olsa <jolsa@redhat.com> wrote: > > > > Adding bpf_get_func_ip helper for BPF_PROG_TYPE_TRACING programs, > > specifically for all trampoline attach types. > > > > The trampoline's caller IP address is stored in (ctx - 8) address. > > so there's no reason to actually call the helper, but rather fixup > > the call instruction and return [ctx - 8] value directly (suggested > > by Alexei). > > > > [fixed has_get_func_ip wrong return type] > > Reported-by: kernel test robot <lkp@intel.com> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > > --- > > include/uapi/linux/bpf.h | 7 +++++ > > kernel/bpf/verifier.c | 53 ++++++++++++++++++++++++++++++++++ > > kernel/trace/bpf_trace.c | 15 ++++++++++ > > tools/include/uapi/linux/bpf.h | 7 +++++ > > 4 files changed, 82 insertions(+) > > > > [...] > > > static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn, > > int *insn_idx_p) > > { > > @@ -6225,6 +6256,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > > if (func_id == BPF_FUNC_get_stackid || func_id == BPF_FUNC_get_stack) > > env->prog->call_get_stack = true; > > > > + if (func_id == BPF_FUNC_get_func_ip) { > > + if (has_get_func_ip(env)) > > from has_xxx name I'd expect it returns true/false, so this reads > super confusing. check_get_func_ip would be a bit more consistent with > other cases like this (still reads confusing to me, but that's ok) ok, will change jirka > > > + return -ENOTSUPP; > > + env->prog->call_get_func_ip = true; > > + } > > + > > if (changes_data) > > clear_all_pkt_pointers(env); > > return 0; > > [...] >
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index bf9252c7381e..83e87ffdbb6e 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -4780,6 +4780,12 @@ union bpf_attr { * Execute close syscall for given FD. * Return * A syscall result. + * + * u64 bpf_get_func_ip(void *ctx) + * Description + * Get address of the traced function (for tracing programs). + * Return + * Address of the traced function. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -4951,6 +4957,7 @@ union bpf_attr { FN(sys_bpf), \ FN(btf_find_by_name_kind), \ FN(sys_close), \ + FN(get_func_ip), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index be38bb930bf1..f975a3aa9368 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5149,6 +5149,11 @@ static bool allow_tail_call_in_subprogs(struct bpf_verifier_env *env) return env->prog->jit_requested && IS_ENABLED(CONFIG_X86_64); } +static bool allow_get_func_ip_tracing(struct bpf_verifier_env *env) +{ + return env->prog->jit_requested && IS_ENABLED(CONFIG_X86_64); +} + static int check_map_func_compatibility(struct bpf_verifier_env *env, struct bpf_map *map, int func_id) { @@ -5955,6 +5960,32 @@ static int check_bpf_snprintf_call(struct bpf_verifier_env *env, return err; } +static int has_get_func_ip(struct bpf_verifier_env *env) +{ + enum bpf_attach_type eatype = env->prog->expected_attach_type; + enum bpf_prog_type type = resolve_prog_type(env->prog); + int func_id = BPF_FUNC_get_func_ip; + + if (type == BPF_PROG_TYPE_TRACING) { + if (eatype != BPF_TRACE_FENTRY && eatype != BPF_TRACE_FEXIT && + eatype != BPF_MODIFY_RETURN) { + verbose(env, "func %s#%d supported only for fentry/fexit/fmod_ret programs\n", + func_id_name(func_id), func_id); + return -ENOTSUPP; + } + if (!allow_get_func_ip_tracing(env)) { + verbose(env, "func %s#%d for tracing programs supported only for JITed x86_64\n", + func_id_name(func_id), func_id); + return -ENOTSUPP; + } + return 0; + } + + verbose(env, "func %s#%d not supported for program type %d\n", + func_id_name(func_id), func_id, type); + return -ENOTSUPP; +} + static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn, int *insn_idx_p) { @@ -6225,6 +6256,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn if (func_id == BPF_FUNC_get_stackid || func_id == BPF_FUNC_get_stack) env->prog->call_get_stack = true; + if (func_id == BPF_FUNC_get_func_ip) { + if (has_get_func_ip(env)) + return -ENOTSUPP; + env->prog->call_get_func_ip = true; + } + if (changes_data) clear_all_pkt_pointers(env); return 0; @@ -12369,6 +12406,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env) { struct bpf_prog *prog = env->prog; bool expect_blinding = bpf_jit_blinding_enabled(prog); + enum bpf_prog_type prog_type = resolve_prog_type(prog); struct bpf_insn *insn = prog->insnsi; const struct bpf_func_proto *fn; const int insn_cnt = prog->len; @@ -12702,6 +12740,21 @@ static int do_misc_fixups(struct bpf_verifier_env *env) continue; } + /* Implement bpf_get_func_ip inline. */ + if (prog_type == BPF_PROG_TYPE_TRACING && + insn->imm == BPF_FUNC_get_func_ip) { + /* Load IP address from ctx - 8 */ + insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_0, BPF_REG_1, -8); + + new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, 1); + if (!new_prog) + return -ENOMEM; + + env->prog = prog = new_prog; + insn = new_prog->insnsi + i + delta; + continue; + } + patch_call_imm: fn = env->ops->get_func_proto(insn->imm, env->prog); /* all functions that have prototype and verifier allowed diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index 64bd2d84367f..9edd3b1a00ad 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -948,6 +948,19 @@ const struct bpf_func_proto bpf_snprintf_btf_proto = { .arg5_type = ARG_ANYTHING, }; +BPF_CALL_1(bpf_get_func_ip_tracing, void *, ctx) +{ + /* Stub, the helper call is inlined in the program. */ + return 0; +} + +static const struct bpf_func_proto bpf_get_func_ip_proto_tracing = { + .func = bpf_get_func_ip_tracing, + .gpl_only = true, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_CTX, +}; + const struct bpf_func_proto * bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) { @@ -1058,6 +1071,8 @@ bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return &bpf_for_each_map_elem_proto; case BPF_FUNC_snprintf: return &bpf_snprintf_proto; + case BPF_FUNC_get_func_ip: + return &bpf_get_func_ip_proto_tracing; default: return NULL; } diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index bf9252c7381e..83e87ffdbb6e 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -4780,6 +4780,12 @@ union bpf_attr { * Execute close syscall for given FD. * Return * A syscall result. + * + * u64 bpf_get_func_ip(void *ctx) + * Description + * Get address of the traced function (for tracing programs). + * Return + * Address of the traced function. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -4951,6 +4957,7 @@ union bpf_attr { FN(sys_bpf), \ FN(btf_find_by_name_kind), \ FN(sys_close), \ + FN(get_func_ip), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper
Adding bpf_get_func_ip helper for BPF_PROG_TYPE_TRACING programs, specifically for all trampoline attach types. The trampoline's caller IP address is stored in (ctx - 8) address. so there's no reason to actually call the helper, but rather fixup the call instruction and return [ctx - 8] value directly (suggested by Alexei). [fixed has_get_func_ip wrong return type] Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/uapi/linux/bpf.h | 7 +++++ kernel/bpf/verifier.c | 53 ++++++++++++++++++++++++++++++++++ kernel/trace/bpf_trace.c | 15 ++++++++++ tools/include/uapi/linux/bpf.h | 7 +++++ 4 files changed, 82 insertions(+)