From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:10:47
hi,
saga continues.. ;-) previous post is in here [1]
After another discussion with Steven, he mentioned that if we fix
the ftrace graph problem with direct functions, he'd be open to
add batch interface for direct ftrace functions.
He already had prove of concept fix for that, which I took and broke
up into several changes. I added the ftrace direct batch interface
and bpf new interface on top of that.
It's not so many patches after all, so I thought having them all
together will help the review, because they are all connected.
However I can break this up into separate patchsets if necessary.
This patchset contains:
1) patches (1-4) that fix the ftrace graph tracing over the function
with direct trampolines attached
2) patches (5-8) that add batch interface for ftrace direct function
register/unregister/modify
3) patches (9-19) that add support to attach BPF program to multiple
functions
In nutshell:
Ad 1) moves the graph tracing setup before the direct trampoline
prepares the stack, so they don't clash
Ad 2) uses ftrace_ops interface to register direct function with
all functions in ftrace_ops filter.
Ad 3) creates special program and trampoline type to allow attachment
of multiple functions to single program.
There're more detailed desriptions in related changelogs.
I have working bpftrace multi attachment code on top this. I briefly
checked retsnoop and I think it could use the new API as well.
Also available at:
https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
bpf/batch
thanks,
jirka
[1] https://lore.kernel.org/bpf/20210413121516.1467989-1-jolsa@kernel.org/
---
Jiri Olsa (17):
x86/ftrace: Remove extra orig rax move
tracing: Add trampoline/graph selftest
ftrace: Add ftrace_add_rec_direct function
ftrace: Add multi direct register/unregister interface
ftrace: Add multi direct modify interface
ftrace/samples: Add multi direct interface test module
bpf, x64: Allow to use caller address from stack
bpf: Allow to store caller's ip as argument
bpf: Add support to load multi func tracing program
bpf: Add bpf_trampoline_alloc function
bpf: Add support to link multi func tracing program
libbpf: Add btf__find_by_pattern_kind function
libbpf: Add support to link multi func tracing program
selftests/bpf: Add fentry multi func test
selftests/bpf: Add fexit multi func test
selftests/bpf: Add fentry/fexit multi func test
selftests/bpf: Temporary fix for fentry_fexit_multi_test
Steven Rostedt (VMware) (2):
x86/ftrace: Remove fault protection code in prepare_ftrace_return
x86/ftrace: Make function graph use ftrace directly
arch/x86/include/asm/ftrace.h | 9 ++++--
arch/x86/kernel/ftrace.c | 71 ++++++++++++++++++++++-----------------------
arch/x86/kernel/ftrace_64.S | 30 +------------------
arch/x86/net/bpf_jit_comp.c | 31 ++++++++++++++------
include/linux/bpf.h | 14 +++++++++
include/linux/ftrace.h | 22 ++++++++++++++
include/uapi/linux/bpf.h | 12 ++++++++
kernel/bpf/btf.c | 5 ++++
kernel/bpf/syscall.c | 220 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
kernel/bpf/trampoline.c | 83 ++++++++++++++++++++++++++++++++++++++---------------
kernel/bpf/verifier.c | 3 +-
kernel/trace/fgraph.c | 8 ++++--
kernel/trace/ftrace.c | 211 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----------------
kernel/trace/trace_selftest.c | 49 ++++++++++++++++++++++++++++++-
samples/ftrace/Makefile | 1 +
samples/ftrace/ftrace-direct-multi.c | 52 +++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 12 ++++++++
tools/lib/bpf/bpf.c | 11 ++++++-
tools/lib/bpf/bpf.h | 4 ++-
tools/lib/bpf/btf.c | 68 +++++++++++++++++++++++++++++++++++++++++++
tools/lib/bpf/btf.h | 3 ++
tools/lib/bpf/libbpf.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/multi_check.h | 53 ++++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/prog_tests/fentry_fexit_multi_test.c | 52 +++++++++++++++++++++++++++++++++
tools/testing/selftests/bpf/prog_tests/fentry_multi_test.c | 43 +++++++++++++++++++++++++++
tools/testing/selftests/bpf/prog_tests/fexit_multi_test.c | 44 ++++++++++++++++++++++++++++
tools/testing/selftests/bpf/progs/fentry_fexit_multi_test.c | 31 ++++++++++++++++++++
tools/testing/selftests/bpf/progs/fentry_multi_test.c | 20 +++++++++++++
tools/testing/selftests/bpf/progs/fexit_multi_test.c | 22 ++++++++++++++
29 files changed, 1121 insertions(+), 135 deletions(-)
create mode 100644 samples/ftrace/ftrace-direct-multi.c
create mode 100644 tools/testing/selftests/bpf/multi_check.h
create mode 100644 tools/testing/selftests/bpf/prog_tests/fentry_fexit_multi_test.c
create mode 100644 tools/testing/selftests/bpf/prog_tests/fentry_multi_test.c
create mode 100644 tools/testing/selftests/bpf/prog_tests/fexit_multi_test.c
create mode 100644 tools/testing/selftests/bpf/progs/fentry_fexit_multi_test.c
create mode 100644 tools/testing/selftests/bpf/progs/fentry_multi_test.c
create mode 100644 tools/testing/selftests/bpf/progs/fexit_multi_test.c
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:10:56
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
We don't need special hook for graph tracer entry point,
but instead we can use graph_ops::func function to install
the return_hooker.
This moves the graph tracing setup _before_ the direct
trampoline prepares the stack, so the return_hooker will
be called when the direct trampoline is finished.
This simplifies the code, because we don't need to take into
account the direct trampoline setup when preparing the graph
tracer hooker and we can allow function graph tracer on entries
registered with direct trampoline.
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/ftrace.h | 9 +++++++--
arch/x86/kernel/ftrace.c | 37 ++++++++++++++++++++++++++++++++---
arch/x86/kernel/ftrace_64.S | 29 +--------------------------
include/linux/ftrace.h | 6 ++++++
kernel/trace/fgraph.c | 8 +++++---
5 files changed, 53 insertions(+), 36 deletions(-)
@@ -65,8 +72,6 @@ struct dyn_arch_ftrace {/* No extra data needed for x86 */};-#define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR-#endif /* CONFIG_DYNAMIC_FTRACE */#endif /* __ASSEMBLY__ */#endif /* CONFIG_FUNCTION_TRACER */
@@ -115,6 +115,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,{structftrace_graph_enttrace;+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS/**Skipgraphtracingifthereturnlocationisservedbydirecttrampoline,*sincecallsequenceandreturnaddressesareunpredictableanyway.
@@ -124,6 +125,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,if(ftrace_direct_func_count&&ftrace_find_rec_direct(ret-MCOUNT_INSN_SIZE))return-EBUSY;+#endiftrace.func=func;trace.depth=++current->curr_ret_depth;
@@ -333,10 +335,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,#endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */staticstructftrace_opsgraph_ops={-.func=ftrace_stub,+.func=ftrace_graph_func,.flags=FTRACE_OPS_FL_INITIALIZED|-FTRACE_OPS_FL_PID|-FTRACE_OPS_FL_STUB,+FTRACE_OPS_FL_PID+FTRACE_OPS_GRAPH_STUB,#ifdef FTRACE_GRAPH_TRAMP_ADDR.trampoline=FTRACE_GRAPH_TRAMP_ADDR,/* trampoline_size is only needed for dynamically allocated tramps */
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:01
Adding selftest for checking that direct trampoline can
co-exist together with graph tracer on same function.
This is supported for CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
config option, which is defined only for x86_64 for now.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/trace/trace_selftest.c | 49 ++++++++++++++++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
@@ -808,8 +811,52 @@ trace_selftest_startup_function_graph(struct tracer *trace,gotoout;}-/* Don't test dynamic tracing, the function tracer already did */+#ifdef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS+tracing_reset_online_cpus(&tr->array_buffer);+set_graph_array(tr);+/*+*Somearchs*cough*PowerPC*cough*addcharacterstothe+*startofthefunctionnames.Wesimplyputa'*'to+*accommodatethem.+*/+func_name="*"__stringify(DYN_FTRACE_TEST_NAME);+ftrace_set_global_filter(func_name,strlen(func_name),1);++/*+*Registerdirectfunctiontogetherwithgraphtracer+*andmakesurewegetgraphtrace.+*/+ret=register_ftrace_direct((unsignedlong)DYN_FTRACE_TEST_NAME,+(unsignedlong)trace_direct_tramp);+if(ret)+gotoout;++ret=register_ftrace_graph(&fgraph_ops);+if(ret){+warn_failed_init_tracer(trace,ret);+gotoout;+}++DYN_FTRACE_TEST_NAME();++count=0;++tracing_stop();+/* check the trace buffer */+ret=trace_test_buffer(&tr->array_buffer,&count);++unregister_ftrace_graph(&fgraph_ops);++tracing_start();++if(!ret&&!count){+ret=-1;+gotoout;+}+#endif++/* Don't test dynamic tracing, the function tracer already did */out:/* Stop it if we failed */if(ret)
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:01
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
Removing the fault protection code when writing return_hooker
to stack. As Steven noted:
That protection was there from the beginning due to being "paranoid",
considering ftrace was bricking network cards. But that protection
would not have even protected against that.
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:02
Factor out the code that adds (ip, addr) tuple to direct_functions
hash in new ftrace_add_rec_direct function. It will be used in
following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/trace/ftrace.c | 60 ++++++++++++++++++++++++++-----------------
1 file changed, 36 insertions(+), 24 deletions(-)
@@ -2388,6 +2388,39 @@ unsigned long ftrace_find_rec_direct(unsigned long ip)returnentry->direct;}+staticstructftrace_func_entry*+ftrace_add_rec_direct(unsignedlongip,unsignedlongaddr,+structftrace_hash**free_hash)+{+structftrace_func_entry*entry;++if(ftrace_hash_empty(direct_functions)||+direct_functions->count>2*(1<<direct_functions->size_bits)){+structftrace_hash*new_hash;+intsize=ftrace_hash_empty(direct_functions)?0:+direct_functions->count+1;++if(size<32)+size=32;++new_hash=dup_hash(direct_functions,size);+if(!new_hash)+returnNULL;++*free_hash=direct_functions;+direct_functions=new_hash;+}++entry=kmalloc(sizeof(*entry),GFP_KERNEL);+if(!entry)+returnNULL;++entry->ip=ip;+entry->direct=addr;+__add_hash_entry(direct_functions,entry);+returnentry;+}+staticvoidcall_direct_funcs(unsignedlongip,unsignedlongpip,structftrace_ops*ops,structftrace_regs*fregs){
@@ -5105,27 +5138,6 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)}ret=-ENOMEM;-if(ftrace_hash_empty(direct_functions)||-direct_functions->count>2*(1<<direct_functions->size_bits)){-structftrace_hash*new_hash;-intsize=ftrace_hash_empty(direct_functions)?0:-direct_functions->count+1;--if(size<32)-size=32;--new_hash=dup_hash(direct_functions,size);-if(!new_hash)-gotoout_unlock;--free_hash=direct_functions;-direct_functions=new_hash;-}--entry=kmalloc(sizeof(*entry),GFP_KERNEL);-if(!entry)-gotoout_unlock;-direct=ftrace_find_direct_func(addr);if(!direct){direct=ftrace_alloc_direct_func(addr);
@@ -5135,9 +5147,9 @@ int register_ftrace_direct(unsigned long ip, unsigned long addr)}}-entry->ip=ip;-entry->direct=addr;-__add_hash_entry(direct_functions,entry);+entry=ftrace_add_rec_direct(ip,addr,&free_hash);+if(!entry)+gotoout_unlock;ret=ftrace_set_filter_ip(&direct_ops,ip,0,0);if(ret)
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:08
Adding interface to register multiple direct functions
within single call. Adding following functions:
register_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
unregister_ftrace_direct_multi(struct ftrace_ops *ops)
The register_ftrace_direct_multi registers direct function (addr)
with all functions in ops filter. The ops filter can be updated
before with ftrace_set_filter_ip calls.
All requested functions must not have direct function currently
registered, otherwise register_ftrace_direct_multi will fail.
The unregister_ftrace_direct_multi unregisters ops related direct
functions.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/ftrace.h | 10 ++++
kernel/trace/ftrace.c | 108 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 118 insertions(+)
@@ -5402,6 +5402,114 @@ int modify_ftrace_direct(unsigned long ip,returnret;}EXPORT_SYMBOL_GPL(modify_ftrace_direct);++#define MULTI_FLAGS (FTRACE_OPS_FL_IPMODIFY | FTRACE_OPS_FL_DIRECT | \+FTRACE_OPS_FL_SAVE_REGS)++staticintcheck_direct_multi(structftrace_ops*ops)+{+if(!(ops->flags&FTRACE_OPS_FL_INITIALIZED))+return-EINVAL;+if((ops->flags&MULTI_FLAGS)!=MULTI_FLAGS)+return-EINVAL;+return0;+}++intregister_ftrace_direct_multi(structftrace_ops*ops,unsignedlongaddr)+{+structftrace_hash*hash=ops->func_hash->filter_hash;+structftrace_func_entry*entry,*new;+structftrace_hash*free_hash=NULL;+interr=-EBUSY,size,i;++if(ops->func||ops->trampoline)+return-EINVAL;+if(ops->flags&FTRACE_OPS_FL_ENABLED)+return-EINVAL;+if(ftrace_hash_empty(hash))+return-EINVAL;++mutex_lock(&direct_mutex);++/* Make sure requested entries are not already registered.. */+size=1<<hash->size_bits;+for(i=0;i<size;i++){+hlist_for_each_entry(entry,&hash->buckets[i],hlist){+if(ftrace_find_rec_direct(entry->ip))+gotoout_unlock;+}+}++/* ... and insert them to direct_functions hash. */+err=-ENOMEM;+for(i=0;i<size;i++){+hlist_for_each_entry(entry,&hash->buckets[i],hlist){+new=ftrace_add_rec_direct(entry->ip,addr,&free_hash);+if(!new)+gotoout_remove;+entry->direct=addr;+}+}++ops->func=call_direct_funcs;+ops->flags=MULTI_FLAGS;+ops->trampoline=FTRACE_REGS_ADDR;++err=register_ftrace_function(ops);++out_remove:+if(err){+for(i=0;i<size;i++){+hlist_for_each_entry(entry,&hash->buckets[i],hlist){+new=__ftrace_lookup_ip(direct_functions,entry->ip);+if(new){+remove_hash_entry(direct_functions,new);+kfree(new);+}+}+}+}++out_unlock:+mutex_unlock(&direct_mutex);++if(free_hash){+synchronize_rcu_tasks();+free_ftrace_hash(free_hash);+}+returnerr;+}+EXPORT_SYMBOL_GPL(register_ftrace_direct_multi);++intunregister_ftrace_direct_multi(structftrace_ops*ops)+{+structftrace_hash*hash=ops->func_hash->filter_hash;+structftrace_func_entry*entry,*new;+interr,size,i;++if(check_direct_multi(ops))+return-EINVAL;+if(!(ops->flags&FTRACE_OPS_FL_ENABLED))+return-EINVAL;++mutex_lock(&direct_mutex);+err=unregister_ftrace_function(ops);++size=1<<hash->size_bits;+for(i=0;i<size;i++){+hlist_for_each_entry(entry,&hash->buckets[i],hlist){+new=__ftrace_lookup_ip(direct_functions,entry->ip);+if(new){+remove_hash_entry(direct_functions,new);+kfree(new);+}+}+}++mutex_unlock(&direct_mutex);+returnerr;+}+EXPORT_SYMBOL_GPL(unregister_ftrace_direct_multi);#endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS *//**
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:08
Adding interface to modify registered direct function
for ftrace_ops. Adding following function:
modify_ftrace_direct_multi(struct ftrace_ops *ops, unsigned long addr)
The function changes the currently registered direct
function for all attached functions.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/ftrace.h | 6 ++++++
kernel/trace/ftrace.c | 43 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 49 insertions(+)
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:10
Adding simple module that uses multi direct interface:
register_ftrace_direct_multi
unregister_ftrace_direct_multi
The init function registers trampoline for 2 functions,
and exit function unregisters them.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
samples/ftrace/Makefile | 1 +
samples/ftrace/ftrace-direct-multi.c | 52 ++++++++++++++++++++++++++++
2 files changed, 53 insertions(+)
create mode 100644 samples/ftrace/ftrace-direct-multi.c
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:14
Currently we call the original function by using the absolute address
given at the JIT generation. That's not usable when having trampoline
attached to multiple functions. In this case we need to take the
return address from the stack.
Adding support to retrieve the original function address from the stack
by adding new BPF_TRAMP_F_ORIG_STACK flag for arch_prepare_bpf_trampoline
function.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 13 +++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 14 insertions(+), 4 deletions(-)
@@ -2013,10 +2013,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *iif(flags&BPF_TRAMP_F_CALL_ORIG){restore_regs(m,&prog,nr_args,stack_size);-/* call original function */-if(emit_call(&prog,orig_call,prog)){-ret=-EINVAL;-gotocleanup;+if(flags&BPF_TRAMP_F_ORIG_STACK){+emit_ldx(&prog,BPF_DW,BPF_REG_0,BPF_REG_FP,8);+EMIT2(0xff,0xd0);/* call *rax */+}else{+/* call original function */+if(emit_call(&prog,orig_call,prog)){+ret=-EINVAL;+gotocleanup;+}}/* remember return value in a stack for bpf prog to access */emit_stx(&prog,BPF_DW,BPF_REG_FP,BPF_REG_0,-8);
@@ -554,6 +554,11 @@ struct btf_func_model {*/#define BPF_TRAMP_F_SKIP_FRAME BIT(2)+/* Get original function from stack instead of from provided direct address.+*Makessenseforfexitprogramsonly.+*/+#define BPF_TRAMP_F_ORIG_STACK BIT(3)+/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50*bytesonx86.PickanumbertofitintoBPF_IMAGE_SIZE/2*/
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:19
When we will have multiple functions attached to trampoline
we need to propagate the function's address to the bpf program.
Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
function that will store origin caller's address before function's
arguments.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 19 insertions(+), 4 deletions(-)
@@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i}if(flags&BPF_TRAMP_F_RESTORE_REGS)-restore_regs(m,&prog,nr_args,stack_size);+restore_regs(m,&prog,nr_args,stack_size-ip_arg);/* This needs to be done regardless. If there were fmod_ret programs,*thereturnvalueisonlyupdatedonthestackandstillneedstobe
@@ -559,6 +559,11 @@ struct btf_func_model {*/#define BPF_TRAMP_F_ORIG_STACK BIT(3)+/* First argument is IP address of the caller. Makes sense for fentry/fexit+*programsonly.+*/+#define BPF_TRAMP_F_IP_ARG BIT(4)+/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50*bytesonx86.PickanumbertofitintoBPF_IMAGE_SIZE/2*/
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:25
Adding support to load tracing program with new BPF_F_MULTI_FUNC flag,
that allows the program to be loaded without specific function to be
attached to.
The verifier assumes the program is using all (6) available arguments
as unsigned long values. We can't add extra ip argument at this time,
because JIT on x86 would fail to process this function. Instead we
allow to access extra first 'ip' argument in btf_ctx_access.
Such program will be allowed to be attached to multiple functions
in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 7 +++++++
kernel/bpf/btf.c | 5 +++++
kernel/bpf/syscall.c | 35 +++++++++++++++++++++++++++++-----
kernel/bpf/verifier.c | 3 ++-
tools/include/uapi/linux/bpf.h | 7 +++++++
6 files changed, 52 insertions(+), 6 deletions(-)
@@ -845,6 +845,7 @@ struct bpf_prog_aux {boolsleepable;booltail_call_reachable;structhlist_nodetramp_hlist;+boolmulti_func;/* BTF_KIND_FUNC_PROTO for valid attach_btf_id */conststructbtf_type*attach_func_proto;/* function name for valid attach_btf_id */
@@ -1109,6 +1109,13 @@ enum bpf_link_type {*/#define BPF_F_SLEEPABLE (1U << 4)+/* If BPF_F_MULTI_FUNC is used in BPF_PROG_LOAD command, the verifier does+*notexpectBTFIDfortheprogram,insteaditassumesit'sfunction+*with6u64arguments.Notrampolineiscreatedfortheprogram.Such+*programcanbeattachedtomultiplefunctions.+*/+#define BPF_F_MULTI_FUNC (1U << 5)+/* When BPF ldimm64's insn[0].src_reg != 0 then this can have*thefollowingextensions:*
@@ -2114,6 +2124,16 @@ static bool is_perfmon_prog_type(enum bpf_prog_type prog_type)}}+#define DEFINE_BPF_MULTI_FUNC(args...) \+externintbpf_multi_func(args);\+int__initbpf_multi_func(args){return0;}++DEFINE_BPF_MULTI_FUNC(unsignedlonga1,unsignedlonga2,+unsignedlonga3,unsignedlonga4,+unsignedlonga5,unsignedlonga6)++BTF_ID_LIST_SINGLE(bpf_multi_func_btf_id,func,bpf_multi_func)+/* last field in 'union bpf_attr' used by this command */#define BPF_PROG_LOAD_LAST_FIELD fd_array
@@ -2164,6 +2186,8 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)if(is_perfmon_prog_type(type)&&!perfmon_capable())return-EPERM;+multi_func=attr->prog_flags&BPF_F_MULTI_FUNC;+/* attach_prog_fd/attach_btf_obj_fd can specify fd of either bpf_prog*orbtf,weneedtocheckwhichoneitis*/
@@ -2182,7 +2206,7 @@ static int bpf_prog_load(union bpf_attr *attr, bpfptr_t uattr)return-ENOTSUPP;}}-}elseif(attr->attach_btf_id){+}elseif(attr->attach_btf_id||multi_func){/* fall back to vmlinux BTF, if BTF type ID is specified */attach_btf=bpf_get_btf_vmlinux();if(IS_ERR(attach_btf))
@@ -1109,6 +1109,13 @@ enum bpf_link_type {*/#define BPF_F_SLEEPABLE (1U << 4)+/* If BPF_F_MULTI_FUNC is used in BPF_PROG_LOAD command, the verifier does+*notexpectBTFIDfortheprogram,insteaditassumesit'sfunction+*with6u64arguments.Notrampolineiscreatedfortheprogram.Such+*programcanbeattachedtomultiplefunctions.+*/+#define BPF_F_MULTI_FUNC (1U << 5)+/* When BPF ldimm64's insn[0].src_reg != 0 then this can have*thefollowingextensions:*
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:26
Factor out the bpf_trampoline_alloc function. It will
be used to allocate trampoline for multi-func programs
in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/bpf/trampoline.c | 34 ++++++++++++++++++++++------------
1 file changed, 22 insertions(+), 12 deletions(-)
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:32
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
The new bpf_trampoline will store and pass to bpf program the highest
number of arguments from all given functions.
New programs (fentry or fexit) can be added to the existing trampoline
through the link_update interface via new_prog_fd descriptor.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/bpf.h | 3 +
include/uapi/linux/bpf.h | 5 +
kernel/bpf/syscall.c | 185 ++++++++++++++++++++++++++++++++-
kernel/bpf/trampoline.c | 53 +++++++---
tools/include/uapi/linux/bpf.h | 5 +
5 files changed, 237 insertions(+), 14 deletions(-)
@@ -343,14 +344,16 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)structbpf_tramp_image*im;structbpf_tramp_progs*tprogs;u32flags=BPF_TRAMP_F_RESTORE_REGS;-interr,total;+boolupdate=!tr->multi;+interr=0,total;tprogs=bpf_trampoline_get_progs(tr,&total);if(IS_ERR(tprogs))returnPTR_ERR(tprogs);if(total==0){-err=unregister_fentry(tr,tr->cur_image->image);+if(update)+err=unregister_fentry(tr,tr->cur_image->image);bpf_tramp_image_put(tr->cur_image);tr->cur_image=NULL;tr->selector=0;
@@ -363,9 +366,15 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)gotoout;}+if(tr->multi)+flags|=BPF_TRAMP_F_IP_ARG;+if(tprogs[BPF_TRAMP_FEXIT].nr_progs||-tprogs[BPF_TRAMP_MODIFY_RETURN].nr_progs)+tprogs[BPF_TRAMP_MODIFY_RETURN].nr_progs){flags=BPF_TRAMP_F_CALL_ORIG|BPF_TRAMP_F_SKIP_FRAME;+if(tr->multi)+flags|=BPF_TRAMP_F_ORIG_STACK|BPF_TRAMP_F_IP_ARG;+}err=arch_prepare_bpf_trampoline(im,im->image,im->image+PAGE_SIZE,&tr->func.model,flags,tprogs,
@@ -373,16 +382,19 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr)if(err<0)gotoout;+err=0;WARN_ON(tr->cur_image&&tr->selector==0);WARN_ON(!tr->cur_image&&tr->selector);-if(tr->cur_image)-/* progs already running at this address */-err=modify_fentry(tr,tr->cur_image->image,im->image);-else-/* first time registering */-err=register_fentry(tr,im->image);-if(err)-gotoout;+if(update){+if(tr->cur_image)+/* progs already running at this address */+err=modify_fentry(tr,tr->cur_image->image,im->image);+else+/* first time registering */+err=register_fentry(tr,im->image);+if(err)+gotoout;+}if(tr->cur_image)bpf_tramp_image_put(tr->cur_image);tr->cur_image=im;
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:32
Adding support to link multi func tracing program
through link_create interface.
Adding special types for multi func programs:
fentry.multi
fexit.multi
so you can define multi func programs like:
SEC("fentry.multi/bpf_fentry_test*")
int BPF_PROG(test1, unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f)
that defines test1 to be attached to bpf_fentry_test* functions,
and able to attach ip and 6 arguments.
If functions are not specified the program needs to be attached
manually.
Adding new btf id related fields to bpf_link_create_opts and
bpf_link_create to use them.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/bpf.c | 11 ++++++-
tools/lib/bpf/bpf.h | 4 ++-
tools/lib/bpf/libbpf.c | 72 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 2 deletions(-)
@@ -674,7 +674,8 @@ int bpf_link_create(int prog_fd, int target_fd,enumbpf_attach_typeattach_type,conststructbpf_link_create_opts*opts){-__u32target_btf_id,iter_info_len;+__u32target_btf_id,iter_info_len,multi_btf_ids_cnt;+__s32*multi_btf_ids;unionbpf_attrattr;intfd;
@@ -687,6 +688,9 @@ int bpf_link_create(int prog_fd, int target_fd,if(iter_info_len&&target_btf_id)returnlibbpf_err(-EINVAL);+multi_btf_ids=OPTS_GET(opts,multi_btf_ids,0);+multi_btf_ids_cnt=OPTS_GET(opts,multi_btf_ids_cnt,0);+memset(&attr,0,sizeof(attr));attr.link_create.prog_fd=prog_fd;attr.link_create.target_fd=target_fd;
@@ -701,6 +705,11 @@ int bpf_link_create(int prog_fd, int target_fd,attr.link_create.target_btf_id=target_btf_id;}+if(multi_btf_ids&&multi_btf_ids_cnt){+attr.link_create.multi_btf_ids=(__u64)multi_btf_ids;+attr.link_create.multi_btf_ids_cnt=multi_btf_ids_cnt;+}+fd=sys_bpf(BPF_LINK_CREATE,&attr,sizeof(attr));returnlibbpf_err_errno(fd);}
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:33
Adding btf__find_by_pattern_kind function that returns
array of BTF ids for given function name pattern.
Using libc's regex.h support for that.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/btf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
tools/lib/bpf/btf.h | 3 ++
2 files changed, 71 insertions(+)
@@ -711,6 +713,72 @@ __s32 btf__find_by_name_kind(const struct btf *btf, const char *type_name,returnlibbpf_err(-ENOENT);}+staticboolis_wildcard(charc)+{+staticconstchar*wildchars="*?[|";++returnstrchr(wildchars,c);+}++intbtf__find_by_pattern_kind(conststructbtf*btf,+constchar*type_pattern,__u32kind,+__s32**__ids)+{+__u32i,nr_types=btf__get_nr_types(btf);+__s32*ids=NULL;+intcnt=0,alloc=0,ret;+regex_tregex;+char*pattern;++if(kind==BTF_KIND_UNKN||!strcmp(type_pattern,"void"))+return0;++/* When the pattern does not start with wildcard, treat it as+*ifwe'dwanttomatchitfromthebeginningofthestring.+*/+asprintf(&pattern,"%s%s",+is_wildcard(type_pattern[0])?"^":"",+type_pattern);++ret=regcomp(®ex,pattern,REG_EXTENDED);+if(ret){+pr_warn("failed to compile regex\n");+free(pattern);+return-EINVAL;+}++free(pattern);++for(i=1;i<=nr_types;i++){+conststructbtf_type*t=btf__type_by_id(btf,i);+constchar*name;+__s32*p;++if(btf_kind(t)!=kind)+continue;+name=btf__name_by_offset(btf,t->name_off);+if(name&®exec(®ex,name,0,NULL,0))+continue;+if(cnt==alloc){+alloc=max(100,alloc*3/2);+p=realloc(ids,alloc*sizeof(__u32));+if(!p){+free(ids);+regfree(®ex);+return-ENOMEM;+}+ids=p;+}++ids[cnt]=i;+cnt++;+}++regfree(®ex);+*__ids=ids;+returncnt?:-ENOENT;+}+staticboolbtf_is_modifiable(conststructbtf*btf){return(void*)btf->hdr!=btf->raw_data;
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:47
Adding selftest for fexit multi func test that attaches
to bpf_fentry_test* functions and checks argument values
based on the processed function.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../bpf/prog_tests/fexit_multi_test.c | 44 +++++++++++++++++++
.../selftests/bpf/progs/fexit_multi_test.c | 20 +++++++++
2 files changed, 64 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/fexit_multi_test.c
create mode 100644 tools/testing/selftests/bpf/progs/fexit_multi_test.c
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:50
Adding selftest for fentry/fexit multi func test that attaches
to bpf_fentry_test* functions and checks argument values based
on the processed function.
When multi_arg_check is used from 2 different places I'm getting
compilation fail, which I did not deciphered yet:
$ CLANG=/opt/clang/bin/clang LLC=/opt/clang/bin/llc make
CLNG-BPF [test_maps] fentry_fexit_multi_test.o
progs/fentry_fexit_multi_test.c:18:2: error: too many args to t24: i64 = \
GlobalAddress<void (i64, i64, i64, i64, i64, i64, i64, i64*)* @multi_arg_check> 0, \
progs/fentry_fexit_multi_test.c:18:2 @[ progs/fentry_fexit_multi_test.c:16:5 ]
multi_arg_check(ip, a, b, c, d, e, f, &test1_arg_result);
^
progs/fentry_fexit_multi_test.c:25:2: error: too many args to t32: i64 = \
GlobalAddress<void (i64, i64, i64, i64, i64, i64, i64, i64*)* @multi_arg_check> 0, \
progs/fentry_fexit_multi_test.c:25:2 @[ progs/fentry_fexit_multi_test.c:23:5 ]
multi_arg_check(ip, a, b, c, d, e, f, &test2_arg_result);
^
In file included from progs/fentry_fexit_multi_test.c:5:
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
void multi_arg_check(unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f, __u64 *test_result)
^
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
5 errors generated.
make: *** [Makefile:470: /home/jolsa/linux-qemu/tools/testing/selftests/bpf/fentry_fexit_multi_test.o] Error 1
I can fix that by defining 2 separate multi_arg_check functions
with different names, which I did in follow up temporaary patch.
Not sure I'm hitting some clang/bpf limitation in here?
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../bpf/prog_tests/fentry_fexit_multi_test.c | 52 +++++++++++++++++++
.../bpf/progs/fentry_fexit_multi_test.c | 28 ++++++++++
2 files changed, 80 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/fentry_fexit_multi_test.c
create mode 100644 tools/testing/selftests/bpf/progs/fentry_fexit_multi_test.c
From: Jiri Olsa <jolsa@kernel.org> Date: 2021-06-05 11:11:52
When multi_arg_check is used from 2 different places I'm getting
compilation fail, which I did not deciphered yet:
$ CLANG=/opt/clang/bin/clang LLC=/opt/clang/bin/llc make
CLNG-BPF [test_maps] fentry_fexit_multi_test.o
progs/fentry_fexit_multi_test.c:18:2: error: too many args to t24: i64 = \
GlobalAddress<void (i64, i64, i64, i64, i64, i64, i64, i64*)* @multi_arg_check> 0, \
progs/fentry_fexit_multi_test.c:18:2 @[ progs/fentry_fexit_multi_test.c:16:5 ]
multi_arg_check(ip, a, b, c, d, e, f, &test1_arg_result);
^
progs/fentry_fexit_multi_test.c:25:2: error: too many args to t32: i64 = \
GlobalAddress<void (i64, i64, i64, i64, i64, i64, i64, i64*)* @multi_arg_check> 0, \
progs/fentry_fexit_multi_test.c:25:2 @[ progs/fentry_fexit_multi_test.c:23:5 ]
multi_arg_check(ip, a, b, c, d, e, f, &test2_arg_result);
^
In file included from progs/fentry_fexit_multi_test.c:5:
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
void multi_arg_check(unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f, __u64 *test_result)
^
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
5 errors generated.
make: *** [Makefile:470: /home/jolsa/linux-qemu/tools/testing/selftests/bpf/fentry_fexit_multi_test.o] Error 1
As a temporary fix adding 2 instaces of multi_arg_check
function, one for each caller.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/testing/selftests/bpf/multi_check.h | 41 ++++++++++---------
.../bpf/progs/fentry_fexit_multi_test.c | 7 +++-
.../selftests/bpf/progs/fentry_multi_test.c | 4 +-
.../selftests/bpf/progs/fexit_multi_test.c | 4 +-
4 files changed, 32 insertions(+), 24 deletions(-)
From: Yonghong Song <hidden> Date: 2021-06-07 03:08:57
On 6/5/21 4:10 AM, Jiri Olsa wrote:
Currently we call the original function by using the absolute address
given at the JIT generation. That's not usable when having trampoline
attached to multiple functions. In this case we need to take the
return address from the stack.
Here, it is mentioned to take the return address from the stack.
Adding support to retrieve the original function address from the stack
Here, it is said to take original funciton address from the stack.
quoted hunk
by adding new BPF_TRAMP_F_ORIG_STACK flag for arch_prepare_bpf_trampoline
function.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 13 +++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 14 insertions(+), 4 deletions(-)
@@ -2013,10 +2013,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *iif(flags&BPF_TRAMP_F_CALL_ORIG){restore_regs(m,&prog,nr_args,stack_size);-/* call original function */-if(emit_call(&prog,orig_call,prog)){-ret=-EINVAL;-gotocleanup;+if(flags&BPF_TRAMP_F_ORIG_STACK){+emit_ldx(&prog,BPF_DW,BPF_REG_0,BPF_REG_FP,8);
This is load double from base_pointer + 8 which should be func return
address for x86, yet we try to call it.
I guess I must have missed something
here. Could you give some explanation?
quoted hunk
+ EMIT2(0xff, 0xd0); /* call *rax */
+ } else {
+ /* call original function */
+ if (emit_call(&prog, orig_call, prog)) {
+ ret = -EINVAL;
+ goto cleanup;
+ }
}
/* remember return value in a stack for bpf prog to access */
emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
@@ -554,6 +554,11 @@ struct btf_func_model {*/#define BPF_TRAMP_F_SKIP_FRAME BIT(2)+/* Get original function from stack instead of from provided direct address.+*Makessenseforfexitprogramsonly.+*/+#define BPF_TRAMP_F_ORIG_STACK BIT(3)+/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50*bytesonx86.PickanumbertofitintoBPF_IMAGE_SIZE/2*/
From: Yonghong Song <hidden> Date: 2021-06-07 03:22:43
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted hunk
When we will have multiple functions attached to trampoline
we need to propagate the function's address to the bpf program.
Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
function that will store origin caller's address before function's
arguments.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 19 insertions(+), 4 deletions(-)
@@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *iEMIT4(0x48,0x83,0xEC,stack_size);/* sub rsp, stack_size */EMIT1(0x53);/* push rbx */-save_regs(m,&prog,nr_args,stack_size);+if(flags&BPF_TRAMP_F_IP_ARG){+emit_ldx(&prog,BPF_DW,BPF_REG_0,BPF_REG_FP,8);+EMIT4(0x48,0x83,0xe8,X86_PATCH_SIZE);/* sub $X86_PATCH_SIZE,%rax*/
Could you explain what the above EMIT4 is for? I am not quite familiar
with this piece of code and hence the question. Some comments here
should help too.
@@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i } if (flags & BPF_TRAMP_F_RESTORE_REGS)- restore_regs(m, &prog, nr_args, stack_size);+ restore_regs(m, &prog, nr_args, stack_size - ip_arg); /* This needs to be done regardless. If there were fmod_ret programs, * the return value is only updated on the stack and still needs to be
From: Yonghong Song <hidden> Date: 2021-06-07 03:58:56
On 6/5/21 4:10 AM, Jiri Olsa wrote:
Adding support to load tracing program with new BPF_F_MULTI_FUNC flag,
that allows the program to be loaded without specific function to be
attached to.
The verifier assumes the program is using all (6) available arguments
Is this a verifier failure or it is due to the check in the
beginning of function arch_prepare_bpf_trampoline()?
/* x86-64 supports up to 6 arguments. 7+ can be added in the
future */
if (nr_args > 6)
return -ENOTSUPP;
If it is indeed due to arch_prepare_bpf_trampoline() maybe we
can improve it instead of specially processing the first argument
"ip" in quite some places?
quoted hunk
as unsigned long values. We can't add extra ip argument at this time,
because JIT on x86 would fail to process this function. Instead we
allow to access extra first 'ip' argument in btf_ctx_access.
Such program will be allowed to be attached to multiple functions
in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 7 +++++++
kernel/bpf/btf.c | 5 +++++
kernel/bpf/syscall.c | 35 +++++++++++++++++++++++++++++-----
kernel/bpf/verifier.c | 3 ++-
tools/include/uapi/linux/bpf.h | 7 +++++++
6 files changed, 52 insertions(+), 6 deletions(-)
@@ -1109,6 +1109,13 @@ enum bpf_link_type {*/#define BPF_F_SLEEPABLE (1U << 4)+/* If BPF_F_MULTI_FUNC is used in BPF_PROG_LOAD command, the verifier does+*notexpectBTFIDfortheprogram,insteaditassumesit'sfunction+*with6u64arguments.Notrampolineiscreatedfortheprogram.Such+*programcanbeattachedtomultiplefunctions.+*/+#define BPF_F_MULTI_FUNC (1U << 5)+/* When BPF ldimm64's insn[0].src_reg != 0 then this can have*thefollowingextensions:*
From: Yonghong Song <hidden> Date: 2021-06-07 05:37:56
On 6/5/21 4:10 AM, Jiri Olsa wrote:
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
I am not sure how severe such a limitation could be in practice.
It is possible in production some non-multi fentry/fexit program
may run continuously. Does kprobe program impact this as well?
The new bpf_trampoline will store and pass to bpf program the highest
number of arguments from all given functions.
New programs (fentry or fexit) can be added to the existing trampoline
through the link_update interface via new_prog_fd descriptor.
Looks we do not support replacing old programs. Do we support
removing old programs?
@@ -3043,6 +3222,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type) case BPF_CGROUP_SETSOCKOPT: return BPF_PROG_TYPE_CGROUP_SOCKOPT; case BPF_TRACE_ITER:+ case BPF_TRACE_FENTRY:+ case BPF_TRACE_FEXIT: return BPF_PROG_TYPE_TRACING; case BPF_SK_LOOKUP: return BPF_PROG_TYPE_SK_LOOKUP;
@@ -4099,6 +4280,8 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr, if (prog->expected_attach_type == BPF_TRACE_ITER) return bpf_iter_link_attach(attr, uattr, prog);+ else if (prog->aux->multi_func)+ return bpf_tracing_multi_attach(prog, attr); else if (prog->type == BPF_PROG_TYPE_EXT) return bpf_tracing_prog_attach(prog, attr->link_create.target_fd,
@@ -4106,7 +4289,7 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr, return -EINVAL; }-#define BPF_LINK_CREATE_LAST_FIELD link_create.iter_info_len+#define BPF_LINK_CREATE_LAST_FIELD link_create.multi_btf_ids_cnt
It is okay that we don't change this. link_create.iter_info_len
has the same effect since it is a union.
From: Yonghong Song <hidden> Date: 2021-06-07 05:50:25
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted hunk
Adding support to link multi func tracing program
through link_create interface.
Adding special types for multi func programs:
fentry.multi
fexit.multi
so you can define multi func programs like:
SEC("fentry.multi/bpf_fentry_test*")
int BPF_PROG(test1, unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f)
that defines test1 to be attached to bpf_fentry_test* functions,
and able to attach ip and 6 arguments.
If functions are not specified the program needs to be attached
manually.
Adding new btf id related fields to bpf_link_create_opts and
bpf_link_create to use them.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/bpf.c | 11 ++++++-
tools/lib/bpf/bpf.h | 4 ++-
tools/lib/bpf/libbpf.c | 72 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 2 deletions(-)
@@ -674,7 +674,8 @@ int bpf_link_create(int prog_fd, int target_fd,enumbpf_attach_typeattach_type,conststructbpf_link_create_opts*opts){-__u32target_btf_id,iter_info_len;+__u32target_btf_id,iter_info_len,multi_btf_ids_cnt;+__s32*multi_btf_ids;unionbpf_attrattr;intfd;
[...]
quoted hunk
@@ -9584,6 +9597,9 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, if (!name) return -EINVAL;+ if (prog->prog_flags & BPF_F_MULTI_FUNC)+ return 0;+ for (i = 0; i < ARRAY_SIZE(section_defs); i++) { if (!section_defs[i].is_attach_btf) continue;
From: Yonghong Song <hidden> Date: 2021-06-07 06:07:03
On 6/5/21 4:10 AM, Jiri Olsa wrote:
Adding selftest for fentry multi func test that attaches
to bpf_fentry_test* functions and checks argument values
based on the processed function.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/testing/selftests/bpf/multi_check.h | 52 +++++++++++++++++++
Should we put this file under selftests/bpf/progs directory?
It is included only by bpf programs.
From: Jiri Olsa <hidden> Date: 2021-06-07 18:13:18
On Sun, Jun 06, 2021 at 08:07:44PM -0700, Yonghong Song wrote:
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
Currently we call the original function by using the absolute address
given at the JIT generation. That's not usable when having trampoline
attached to multiple functions. In this case we need to take the
return address from the stack.
Here, it is mentioned to take the return address from the stack.
quoted
Adding support to retrieve the original function address from the stack
Here, it is said to take original funciton address from the stack.
sorry if the description is confusing as always, the idea
is to take the function's return address from fentry call:
function
call fentry
xxxx <---- this address
and use it to call the original function body before fexit handler
jirka
quoted
by adding new BPF_TRAMP_F_ORIG_STACK flag for arch_prepare_bpf_trampoline
function.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 13 +++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 14 insertions(+), 4 deletions(-)
@@ -2013,10 +2013,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *iif(flags&BPF_TRAMP_F_CALL_ORIG){restore_regs(m,&prog,nr_args,stack_size);-/* call original function */-if(emit_call(&prog,orig_call,prog)){-ret=-EINVAL;-gotocleanup;+if(flags&BPF_TRAMP_F_ORIG_STACK){+emit_ldx(&prog,BPF_DW,BPF_REG_0,BPF_REG_FP,8);
This is load double from base_pointer + 8 which should be func return
address for x86, yet we try to call it.
I guess I must have missed something
here. Could you give some explanation?
quoted
+ EMIT2(0xff, 0xd0); /* call *rax */
+ } else {
+ /* call original function */
+ if (emit_call(&prog, orig_call, prog)) {
+ ret = -EINVAL;
+ goto cleanup;
+ }
}
/* remember return value in a stack for bpf prog to access */
emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_0, -8);
@@ -554,6 +554,11 @@ struct btf_func_model {*/#define BPF_TRAMP_F_SKIP_FRAME BIT(2)+/* Get original function from stack instead of from provided direct address.+*Makessenseforfexitprogramsonly.+*/+#define BPF_TRAMP_F_ORIG_STACK BIT(3)+/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50*bytesonx86.PickanumbertofitintoBPF_IMAGE_SIZE/2*/
From: Jiri Olsa <hidden> Date: 2021-06-07 18:15:46
On Sun, Jun 06, 2021 at 08:21:51PM -0700, Yonghong Song wrote:
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
When we will have multiple functions attached to trampoline
we need to propagate the function's address to the bpf program.
Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
function that will store origin caller's address before function's
arguments.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 19 insertions(+), 4 deletions(-)
@@ -1982,7 +1985,14 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *iEMIT4(0x48,0x83,0xEC,stack_size);/* sub rsp, stack_size */EMIT1(0x53);/* push rbx */-save_regs(m,&prog,nr_args,stack_size);+if(flags&BPF_TRAMP_F_IP_ARG){+emit_ldx(&prog,BPF_DW,BPF_REG_0,BPF_REG_FP,8);+EMIT4(0x48,0x83,0xe8,X86_PATCH_SIZE);/* sub $X86_PATCH_SIZE,%rax*/
Could you explain what the above EMIT4 is for? I am not quite familiar with
this piece of code and hence the question. Some comments here
should help too.
it's there to generate the 'sub $X86_PATCH_SIZE,%rax' instruction
to get the real IP address of the traced function, and it's stored
to stack on the next line
I'll put more comments in there
jirka
@@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i } if (flags & BPF_TRAMP_F_RESTORE_REGS)- restore_regs(m, &prog, nr_args, stack_size);+ restore_regs(m, &prog, nr_args, stack_size - ip_arg); /* This needs to be done regardless. If there were fmod_ret programs, * the return value is only updated on the stack and still needs to be
From: Jiri Olsa <hidden> Date: 2021-06-07 18:19:20
On Sun, Jun 06, 2021 at 08:56:47PM -0700, Yonghong Song wrote:
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
Adding support to load tracing program with new BPF_F_MULTI_FUNC flag,
that allows the program to be loaded without specific function to be
attached to.
The verifier assumes the program is using all (6) available arguments
Is this a verifier failure or it is due to the check in the
beginning of function arch_prepare_bpf_trampoline()?
/* x86-64 supports up to 6 arguments. 7+ can be added in the future
*/
if (nr_args > 6)
return -ENOTSUPP;
yes, that's the limit.. it allows the traced program to
touch 6 arguments, because it's the maximum for JIT
If it is indeed due to arch_prepare_bpf_trampoline() maybe we
can improve it instead of specially processing the first argument
"ip" in quite some places?
do you mean to teach JIT to process more than 6 arguments?
quoted
as unsigned long values. We can't add extra ip argument at this time,
because JIT on x86 would fail to process this function. Instead we
allow to access extra first 'ip' argument in btf_ctx_access.
Such program will be allowed to be attached to multiple functions
in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 7 +++++++
kernel/bpf/btf.c | 5 +++++
kernel/bpf/syscall.c | 35 +++++++++++++++++++++++++++++-----
kernel/bpf/verifier.c | 3 ++-
tools/include/uapi/linux/bpf.h | 7 +++++++
6 files changed, 52 insertions(+), 6 deletions(-)
@@ -1109,6 +1109,13 @@ enum bpf_link_type {*/#define BPF_F_SLEEPABLE (1U << 4)+/* If BPF_F_MULTI_FUNC is used in BPF_PROG_LOAD command, the verifier does+*notexpectBTFIDfortheprogram,insteaditassumesit'sfunction+*with6u64arguments.Notrampolineiscreatedfortheprogram.Such+*programcanbeattachedtomultiplefunctions.+*/+#define BPF_F_MULTI_FUNC (1U << 5)+/* When BPF ldimm64's insn[0].src_reg != 0 then this can have*thefollowingextensions:*
From: Jiri Olsa <hidden> Date: 2021-06-07 18:26:01
On Sun, Jun 06, 2021 at 10:36:57PM -0700, Yonghong Song wrote:
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
I am not sure how severe such a limitation could be in practice.
It is possible in production some non-multi fentry/fexit program
may run continuously. Does kprobe program impact this as well?
I did not find a way how to combine current trampolines with the
new ones for multiple programs.. what you described is a limitation
of the current approach
I'm not sure about kprobes and trampolines, but the limitation
should be same as we do have for current trampolines.. I'll check
quoted
The new bpf_trampoline will store and pass to bpf program the highest
number of arguments from all given functions.
New programs (fentry or fexit) can be added to the existing trampoline
through the link_update interface via new_prog_fd descriptor.
Looks we do not support replacing old programs. Do we support
removing old programs?
we don't.. it's not what bpftrace would do, it just adds programs
to trace and close all when it's done.. I think interface for removal
could be added if you think it's needed
@@ -3043,6 +3222,8 @@ attach_type_to_prog_type(enum bpf_attach_type attach_type) case BPF_CGROUP_SETSOCKOPT: return BPF_PROG_TYPE_CGROUP_SOCKOPT; case BPF_TRACE_ITER:+ case BPF_TRACE_FENTRY:+ case BPF_TRACE_FEXIT: return BPF_PROG_TYPE_TRACING; case BPF_SK_LOOKUP: return BPF_PROG_TYPE_SK_LOOKUP;
@@ -4099,6 +4280,8 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr, if (prog->expected_attach_type == BPF_TRACE_ITER) return bpf_iter_link_attach(attr, uattr, prog);+ else if (prog->aux->multi_func)+ return bpf_tracing_multi_attach(prog, attr); else if (prog->type == BPF_PROG_TYPE_EXT) return bpf_tracing_prog_attach(prog, attr->link_create.target_fd,
@@ -4106,7 +4289,7 @@ static int tracing_bpf_link_attach(const union bpf_attr *attr, bpfptr_t uattr, return -EINVAL; }-#define BPF_LINK_CREATE_LAST_FIELD link_create.iter_info_len+#define BPF_LINK_CREATE_LAST_FIELD link_create.multi_btf_ids_cnt
It is okay that we don't change this. link_create.iter_info_len
has the same effect since it is a union.
From: Jiri Olsa <hidden> Date: 2021-06-07 18:28:57
On Sun, Jun 06, 2021 at 10:49:16PM -0700, Yonghong Song wrote:
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
Adding support to link multi func tracing program
through link_create interface.
Adding special types for multi func programs:
fentry.multi
fexit.multi
so you can define multi func programs like:
SEC("fentry.multi/bpf_fentry_test*")
int BPF_PROG(test1, unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f)
that defines test1 to be attached to bpf_fentry_test* functions,
and able to attach ip and 6 arguments.
If functions are not specified the program needs to be attached
manually.
Adding new btf id related fields to bpf_link_create_opts and
bpf_link_create to use them.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/bpf.c | 11 ++++++-
tools/lib/bpf/bpf.h | 4 ++-
tools/lib/bpf/libbpf.c | 72 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 2 deletions(-)
@@ -674,7 +674,8 @@ int bpf_link_create(int prog_fd, int target_fd,enumbpf_attach_typeattach_type,conststructbpf_link_create_opts*opts){-__u32target_btf_id,iter_info_len;+__u32target_btf_id,iter_info_len,multi_btf_ids_cnt;+__s32*multi_btf_ids;unionbpf_attrattr;intfd;
[...]
quoted
@@ -9584,6 +9597,9 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, if (!name) return -EINVAL;+ if (prog->prog_flags & BPF_F_MULTI_FUNC)+ return 0;+ for (i = 0; i < ARRAY_SIZE(section_defs); i++) { if (!section_defs[i].is_attach_btf) continue;
From: Jiri Olsa <hidden> Date: 2021-06-07 18:42:38
On Sun, Jun 06, 2021 at 11:06:14PM -0700, Yonghong Song wrote:
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
Adding selftest for fentry multi func test that attaches
to bpf_fentry_test* functions and checks argument values
based on the processed function.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/testing/selftests/bpf/multi_check.h | 52 +++++++++++++++++++
Should we put this file under selftests/bpf/progs directory?
It is included only by bpf programs.
From: Yonghong Song <hidden> Date: 2021-06-07 19:35:59
On 6/7/21 11:18 AM, Jiri Olsa wrote:
On Sun, Jun 06, 2021 at 08:56:47PM -0700, Yonghong Song wrote:
quoted
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
Adding support to load tracing program with new BPF_F_MULTI_FUNC flag,
that allows the program to be loaded without specific function to be
attached to.
The verifier assumes the program is using all (6) available arguments
Is this a verifier failure or it is due to the check in the
beginning of function arch_prepare_bpf_trampoline()?
/* x86-64 supports up to 6 arguments. 7+ can be added in the future
*/
if (nr_args > 6)
return -ENOTSUPP;
yes, that's the limit.. it allows the traced program to
touch 6 arguments, because it's the maximum for JIT
quoted
If it is indeed due to arch_prepare_bpf_trampoline() maybe we
can improve it instead of specially processing the first argument
"ip" in quite some places?
do you mean to teach JIT to process more than 6 arguments?
Yes. Not sure how hard it is. If it is doable with reasonable
complexity, I think it will be worth it as it will benefit this
case to avoid special tweaks of the first argument, but also
benefit other cases e.g., attaching to a kernel function with
7 or more arguments.
quoted
quoted
as unsigned long values. We can't add extra ip argument at this time,
because JIT on x86 would fail to process this function. Instead we
allow to access extra first 'ip' argument in btf_ctx_access.
Such program will be allowed to be attached to multiple functions
in following patches.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/bpf.h | 1 +
include/uapi/linux/bpf.h | 7 +++++++
kernel/bpf/btf.c | 5 +++++
kernel/bpf/syscall.c | 35 +++++++++++++++++++++++++++++-----
kernel/bpf/verifier.c | 3 ++-
tools/include/uapi/linux/bpf.h | 7 +++++++
6 files changed, 52 insertions(+), 6 deletions(-)
From: Yonghong Song <hidden> Date: 2021-06-07 19:40:44
On 6/7/21 11:25 AM, Jiri Olsa wrote:
On Sun, Jun 06, 2021 at 10:36:57PM -0700, Yonghong Song wrote:
quoted
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
I am not sure how severe such a limitation could be in practice.
It is possible in production some non-multi fentry/fexit program
may run continuously. Does kprobe program impact this as well?
I did not find a way how to combine current trampolines with the
new ones for multiple programs.. what you described is a limitation
of the current approach
I'm not sure about kprobes and trampolines, but the limitation
should be same as we do have for current trampolines.. I'll check
quoted
quoted
The new bpf_trampoline will store and pass to bpf program the highest
number of arguments from all given functions.
New programs (fentry or fexit) can be added to the existing trampoline
through the link_update interface via new_prog_fd descriptor.
Looks we do not support replacing old programs. Do we support
removing old programs?
we don't.. it's not what bpftrace would do, it just adds programs
to trace and close all when it's done.. I think interface for removal
could be added if you think it's needed
This can be a followup patch. Indeed removing selective old programs
probably not a common use case.
From: Yonghong Song <hidden> Date: 2021-06-07 19:43:43
On 6/7/21 11:28 AM, Jiri Olsa wrote:
On Sun, Jun 06, 2021 at 10:49:16PM -0700, Yonghong Song wrote:
quoted
On 6/5/21 4:10 AM, Jiri Olsa wrote:
quoted
Adding support to link multi func tracing program
through link_create interface.
Adding special types for multi func programs:
fentry.multi
fexit.multi
so you can define multi func programs like:
SEC("fentry.multi/bpf_fentry_test*")
int BPF_PROG(test1, unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f)
that defines test1 to be attached to bpf_fentry_test* functions,
and able to attach ip and 6 arguments.
If functions are not specified the program needs to be attached
manually.
Adding new btf id related fields to bpf_link_create_opts and
bpf_link_create to use them.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/bpf.c | 11 ++++++-
tools/lib/bpf/bpf.h | 4 ++-
tools/lib/bpf/libbpf.c | 72 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 2 deletions(-)
@@ -674,7 +674,8 @@ int bpf_link_create(int prog_fd, int target_fd,enumbpf_attach_typeattach_type,conststructbpf_link_create_opts*opts){-__u32target_btf_id,iter_info_len;+__u32target_btf_id,iter_info_len,multi_btf_ids_cnt;+__s32*multi_btf_ids;unionbpf_attrattr;intfd;
[...]
quoted
@@ -9584,6 +9597,9 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, if (!name) return -EINVAL;+ if (prog->prog_flags & BPF_F_MULTI_FUNC)+ return 0;+ for (i = 0; i < ARRAY_SIZE(section_defs); i++) { if (!section_defs[i].is_attach_btf) continue;
On Sat, Jun 5, 2021 at 4:11 AM Jiri Olsa [off-list ref] wrote:
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
Overall the api makes sense to me.
The restriction of multi vs non-multi is too severe though.
The multi trampoline can serve normal fentry/fexit too.
If ip is moved to the end (instead of start) the trampoline
will be able to call into multi and normal fentry/fexit progs. Right?
From: Jiri Olsa <hidden> Date: 2021-06-08 18:17:15
On Tue, Jun 08, 2021 at 08:42:32AM -0700, Alexei Starovoitov wrote:
On Sat, Jun 5, 2021 at 4:11 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
Overall the api makes sense to me.
The restriction of multi vs non-multi is too severe though.
The multi trampoline can serve normal fentry/fexit too.
so multi trampoline gets called from all the registered functions,
so there would need to be filter for specific ip before calling the
standard program.. single cmp/jnz might not be that bad, I'll check
If ip is moved to the end (instead of start) the trampoline
will be able to call into multi and normal fentry/fexit progs. Right?
we could just skip ip arg when generating entry for normal programs
and start from first argument address for %rdi
and it'd need to be transparent for current trampolines user API,
so I wonder there will be some hiccup ;-) let's see
thanks,
jirka
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted hunk
From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
We don't need special hook for graph tracer entry point,
but instead we can use graph_ops::func function to install
the return_hooker.
This moves the graph tracing setup _before_ the direct
trampoline prepares the stack, so the return_hooker will
be called when the direct trampoline is finished.
This simplifies the code, because we don't need to take into
account the direct trampoline setup when preparing the graph
tracer hooker and we can allow function graph tracer on entries
registered with direct trampoline.
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/include/asm/ftrace.h | 9 +++++++--
arch/x86/kernel/ftrace.c | 37 ++++++++++++++++++++++++++++++++---
arch/x86/kernel/ftrace_64.S | 29 +--------------------------
include/linux/ftrace.h | 6 ++++++
kernel/trace/fgraph.c | 8 +++++---
5 files changed, 53 insertions(+), 36 deletions(-)
@@ -65,8 +72,6 @@ struct dyn_arch_ftrace {/* No extra data needed for x86 */};-#define FTRACE_GRAPH_TRAMP_ADDR FTRACE_GRAPH_ADDR-#endif /* CONFIG_DYNAMIC_FTRACE */#endif /* __ASSEMBLY__ */#endif /* CONFIG_FUNCTION_TRACER */
@@ -115,6 +115,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,{structftrace_graph_enttrace;+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS/**Skipgraphtracingifthereturnlocationisservedbydirecttrampoline,*sincecallsequenceandreturnaddressesareunpredictableanyway.
@@ -124,6 +125,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,if(ftrace_direct_func_count&&ftrace_find_rec_direct(ret-MCOUNT_INSN_SIZE))return-EBUSY;+#endiftrace.func=func;trace.depth=++current->curr_ret_depth;
@@ -333,10 +335,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,#endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */staticstructftrace_opsgraph_ops={-.func=ftrace_stub,+.func=ftrace_graph_func,.flags=FTRACE_OPS_FL_INITIALIZED|-FTRACE_OPS_FL_PID|-FTRACE_OPS_FL_STUB,+FTRACE_OPS_FL_PID+FTRACE_OPS_GRAPH_STUB,
nit: this looks so weird... Why not define FTRACE_OPS_GRAPH_STUB as
zero in case of #ifdef ftrace_graph_func? Then it will be natural and
correctly looking | FTRACE_OPS_GRAPH_STUB?
#ifdef FTRACE_GRAPH_TRAMP_ADDR
.trampoline = FTRACE_GRAPH_TRAMP_ADDR,
/* trampoline_size is only needed for dynamically allocated tramps */
--
2.31.1
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted hunk
When we will have multiple functions attached to trampoline
we need to propagate the function's address to the bpf program.
Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
function that will store origin caller's address before function's
arguments.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 19 insertions(+), 4 deletions(-)
similarly (and symmetrically), pass flags into restore_regs() to
handle that ip_arg transparently?
quoted hunk
if (flags & BPF_TRAMP_F_ORIG_STACK) {
emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
@@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i } if (flags & BPF_TRAMP_F_RESTORE_REGS)- restore_regs(m, &prog, nr_args, stack_size);+ restore_regs(m, &prog, nr_args, stack_size - ip_arg); /* This needs to be done regardless. If there were fmod_ret programs, * the return value is only updated on the stack and still needs to be
@@ -559,6 +559,11 @@ struct btf_func_model {*/#define BPF_TRAMP_F_ORIG_STACK BIT(3)+/* First argument is IP address of the caller. Makes sense for fentry/fexit+*programsonly.+*/+#define BPF_TRAMP_F_IP_ARG BIT(4)+/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50*bytesonx86.PickanumbertofitintoBPF_IMAGE_SIZE/2*/--
@@ -115,6 +115,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,{structftrace_graph_enttrace;+#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS/**Skipgraphtracingifthereturnlocationisservedbydirecttrampoline,*sincecallsequenceandreturnaddressesareunpredictableanyway.
@@ -124,6 +125,7 @@ int function_graph_enter(unsigned long ret, unsigned long func,if(ftrace_direct_func_count&&ftrace_find_rec_direct(ret-MCOUNT_INSN_SIZE))return-EBUSY;+#endiftrace.func=func;trace.depth=++current->curr_ret_depth;
@@ -333,10 +335,10 @@ unsigned long ftrace_graph_ret_addr(struct task_struct *task, int *idx,#endif /* HAVE_FUNCTION_GRAPH_RET_ADDR_PTR */staticstructftrace_opsgraph_ops={-.func=ftrace_stub,+.func=ftrace_graph_func,.flags=FTRACE_OPS_FL_INITIALIZED|-FTRACE_OPS_FL_PID|-FTRACE_OPS_FL_STUB,+FTRACE_OPS_FL_PID+FTRACE_OPS_GRAPH_STUB,
nit: this looks so weird... Why not define FTRACE_OPS_GRAPH_STUB as
zero in case of #ifdef ftrace_graph_func? Then it will be natural and
correctly looking | FTRACE_OPS_GRAPH_STUB?
ok, I can change that
thanks,
jirka
quoted
#ifdef FTRACE_GRAPH_TRAMP_ADDR
.trampoline = FTRACE_GRAPH_TRAMP_ADDR,
/* trampoline_size is only needed for dynamically allocated tramps */
--
2.31.1
On Tue, Jun 08, 2021 at 08:17:00PM +0200, Jiri Olsa wrote:
On Tue, Jun 08, 2021 at 08:42:32AM -0700, Alexei Starovoitov wrote:
quoted
On Sat, Jun 5, 2021 at 4:11 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
Overall the api makes sense to me.
The restriction of multi vs non-multi is too severe though.
The multi trampoline can serve normal fentry/fexit too.
so multi trampoline gets called from all the registered functions,
so there would need to be filter for specific ip before calling the
standard program.. single cmp/jnz might not be that bad, I'll check
You mean reusing the same multi trampoline for all IPs and regenerating
it with a bunch of cmp/jnz checks? There should be a better way to scale.
Maybe clone multi trampoline instead?
IPs[1-10] will point to multi.
IP[11] will point to a clone of multi that serves multi prog and
fentry/fexit progs specific for that IP.
From: Steven Rostedt <rostedt@goodmis.org> Date: 2021-06-08 19:20:43
On Tue, 8 Jun 2021 20:51:25 +0200
Jiri Olsa [off-list ref] wrote:
quoted
quoted
+ FTRACE_OPS_FL_PID
+ FTRACE_OPS_GRAPH_STUB,
nit: this looks so weird... Why not define FTRACE_OPS_GRAPH_STUB as
zero in case of #ifdef ftrace_graph_func? Then it will be natural and
correctly looking | FTRACE_OPS_GRAPH_STUB?
I have no idea why I did that :-/ But it was a while ago when I wrote
this code. I think there was a reason for it, but with various updates,
that reason disappeared.
From: Jiri Olsa <hidden> Date: 2021-06-08 20:58:42
On Tue, Jun 08, 2021 at 11:49:31AM -0700, Andrii Nakryiko wrote:
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
When we will have multiple functions attached to trampoline
we need to propagate the function's address to the bpf program.
Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
function that will store origin caller's address before function's
arguments.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 19 insertions(+), 4 deletions(-)
similarly (and symmetrically), pass flags into restore_regs() to
handle that ip_arg transparently?
so you mean something like:
if (flags & BPF_TRAMP_F_IP_ARG)
stack_size -= 8;
in both save_regs and restore_regs function, right?
jirka
quoted
if (flags & BPF_TRAMP_F_ORIG_STACK) {
emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
@@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i } if (flags & BPF_TRAMP_F_RESTORE_REGS)- restore_regs(m, &prog, nr_args, stack_size);+ restore_regs(m, &prog, nr_args, stack_size - ip_arg); /* This needs to be done regardless. If there were fmod_ret programs, * the return value is only updated on the stack and still needs to be
@@ -559,6 +559,11 @@ struct btf_func_model {*/#define BPF_TRAMP_F_ORIG_STACK BIT(3)+/* First argument is IP address of the caller. Makes sense for fentry/fexit+*programsonly.+*/+#define BPF_TRAMP_F_IP_ARG BIT(4)+/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50*bytesonx86.PickanumbertofitintoBPF_IMAGE_SIZE/2*/--
On Tue, Jun 8, 2021 at 1:58 PM Jiri Olsa [off-list ref] wrote:
On Tue, Jun 08, 2021 at 11:49:31AM -0700, Andrii Nakryiko wrote:
quoted
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
When we will have multiple functions attached to trampoline
we need to propagate the function's address to the bpf program.
Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
function that will store origin caller's address before function's
arguments.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 19 insertions(+), 4 deletions(-)
similarly (and symmetrically), pass flags into restore_regs() to
handle that ip_arg transparently?
so you mean something like:
if (flags & BPF_TRAMP_F_IP_ARG)
stack_size -= 8;
in both save_regs and restore_regs function, right?
yes, but for save_regs it will do more (emit_ldx and stuff)
jirka
quoted
quoted
if (flags & BPF_TRAMP_F_ORIG_STACK) {
emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
@@ -2052,7 +2062,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i } if (flags & BPF_TRAMP_F_RESTORE_REGS)- restore_regs(m, &prog, nr_args, stack_size);+ restore_regs(m, &prog, nr_args, stack_size - ip_arg); /* This needs to be done regardless. If there were fmod_ret programs, * the return value is only updated on the stack and still needs to be
@@ -559,6 +559,11 @@ struct btf_func_model {*/#define BPF_TRAMP_F_ORIG_STACK BIT(3)+/* First argument is IP address of the caller. Makes sense for fentry/fexit+*programsonly.+*/+#define BPF_TRAMP_F_IP_ARG BIT(4)+/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50*bytesonx86.PickanumbertofitintoBPF_IMAGE_SIZE/2*/--
From: Jiri Olsa <hidden> Date: 2021-06-08 21:07:56
On Tue, Jun 08, 2021 at 11:49:03AM -0700, Alexei Starovoitov wrote:
On Tue, Jun 08, 2021 at 08:17:00PM +0200, Jiri Olsa wrote:
quoted
On Tue, Jun 08, 2021 at 08:42:32AM -0700, Alexei Starovoitov wrote:
quoted
On Sat, Jun 5, 2021 at 4:11 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
Overall the api makes sense to me.
The restriction of multi vs non-multi is too severe though.
The multi trampoline can serve normal fentry/fexit too.
so multi trampoline gets called from all the registered functions,
so there would need to be filter for specific ip before calling the
standard program.. single cmp/jnz might not be that bad, I'll check
You mean reusing the same multi trampoline for all IPs and regenerating
it with a bunch of cmp/jnz checks? There should be a better way to scale.
Maybe clone multi trampoline instead?
IPs[1-10] will point to multi.
IP[11] will point to a clone of multi that serves multi prog and
fentry/fexit progs specific for that IP.
ok, so we'd clone multi trampoline if there's request to attach
standard trampoline to some IP from multi trampoline
.. and transform currently attached standard trampoline for IP
into clone of multi trampoline, if there's request to create
multi trampoline that covers that IP
jirka
From: Jiri Olsa <hidden> Date: 2021-06-08 21:11:27
On Tue, Jun 08, 2021 at 02:02:56PM -0700, Andrii Nakryiko wrote:
On Tue, Jun 8, 2021 at 1:58 PM Jiri Olsa [off-list ref] wrote:
quoted
On Tue, Jun 08, 2021 at 11:49:31AM -0700, Andrii Nakryiko wrote:
quoted
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
When we will have multiple functions attached to trampoline
we need to propagate the function's address to the bpf program.
Adding new BPF_TRAMP_F_IP_ARG flag to arch_prepare_bpf_trampoline
function that will store origin caller's address before function's
arguments.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
arch/x86/net/bpf_jit_comp.c | 18 ++++++++++++++----
include/linux/bpf.h | 5 +++++
2 files changed, 19 insertions(+), 4 deletions(-)
On Tue, Jun 8, 2021 at 2:07 PM Jiri Olsa [off-list ref] wrote:
On Tue, Jun 08, 2021 at 11:49:03AM -0700, Alexei Starovoitov wrote:
quoted
On Tue, Jun 08, 2021 at 08:17:00PM +0200, Jiri Olsa wrote:
quoted
On Tue, Jun 08, 2021 at 08:42:32AM -0700, Alexei Starovoitov wrote:
quoted
On Sat, Jun 5, 2021 at 4:11 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
Overall the api makes sense to me.
The restriction of multi vs non-multi is too severe though.
The multi trampoline can serve normal fentry/fexit too.
so multi trampoline gets called from all the registered functions,
so there would need to be filter for specific ip before calling the
standard program.. single cmp/jnz might not be that bad, I'll check
You mean reusing the same multi trampoline for all IPs and regenerating
it with a bunch of cmp/jnz checks? There should be a better way to scale.
Maybe clone multi trampoline instead?
IPs[1-10] will point to multi.
IP[11] will point to a clone of multi that serves multi prog and
fentry/fexit progs specific for that IP.
ok, so we'd clone multi trampoline if there's request to attach
standard trampoline to some IP from multi trampoline
.. and transform currently attached standard trampoline for IP
into clone of multi trampoline, if there's request to create
multi trampoline that covers that IP
yep. For every IP==btf_id there will be only two possible trampolines.
Should be easy enough to track and transition between them.
The standard fentry/fexit will only get negligible slowdown from
going through multi.
multi+fexit and fmod_ret needs to be thought through as well.
That's why I thought that 'ip' at the end should simplify things.
Only multi will have access to it.
But we can store it first too. fentry/fexit will see ctx=r1 with +8 offset
and will have normal args in ctx. Like ip isn't even there.
While multi trampoline is always doing ip, arg1,arg2, .., arg6
and passes ctx = &ip into multi prog and ctx = &arg1 into fentry/fexit.
'ret' for fexit is problematic though. hmm.
Maybe such clone multi trampoline for specific ip with 2 args will do:
ip, arg1, arg2, ret, 0, 0, 0, ret.
Then multi will have 6 args, though 3rd is actually ret.
Then fexit will have ret in the right place and multi prog will have
it as 7th arg.
On Tue, Jun 8, 2021 at 4:07 PM Alexei Starovoitov
[off-list ref] wrote:
On Tue, Jun 8, 2021 at 2:07 PM Jiri Olsa [off-list ref] wrote:
quoted
On Tue, Jun 08, 2021 at 11:49:03AM -0700, Alexei Starovoitov wrote:
quoted
On Tue, Jun 08, 2021 at 08:17:00PM +0200, Jiri Olsa wrote:
quoted
On Tue, Jun 08, 2021 at 08:42:32AM -0700, Alexei Starovoitov wrote:
quoted
On Sat, Jun 5, 2021 at 4:11 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
Overall the api makes sense to me.
The restriction of multi vs non-multi is too severe though.
The multi trampoline can serve normal fentry/fexit too.
so multi trampoline gets called from all the registered functions,
so there would need to be filter for specific ip before calling the
standard program.. single cmp/jnz might not be that bad, I'll check
You mean reusing the same multi trampoline for all IPs and regenerating
it with a bunch of cmp/jnz checks? There should be a better way to scale.
Maybe clone multi trampoline instead?
IPs[1-10] will point to multi.
IP[11] will point to a clone of multi that serves multi prog and
fentry/fexit progs specific for that IP.
ok, so we'd clone multi trampoline if there's request to attach
standard trampoline to some IP from multi trampoline
.. and transform currently attached standard trampoline for IP
into clone of multi trampoline, if there's request to create
multi trampoline that covers that IP
yep. For every IP==btf_id there will be only two possible trampolines.
Should be easy enough to track and transition between them.
The standard fentry/fexit will only get negligible slowdown from
going through multi.
multi+fexit and fmod_ret needs to be thought through as well.
That's why I thought that 'ip' at the end should simplify things.
Putting ip at the end has downsides. We might support >6 arguments
eventually, at which point it will be super weird to have 6 args, ip,
then the rest of arguments?..
Would it be too bad to put IP at -8 offset relative to ctx? That will
also work for normal fentry/fexit, for which it's useful to have ip
passed in as well, IMO. So no special casing for multi/non-multi, and
it's backwards compatible.
Ideally, I'd love it to be actually retrievable through a new BPF
helper, something like bpf_caller_ip(ctx), but I'm not sure if we can
implement this sanely, so I don't hold high hopes.
Only multi will have access to it.
But we can store it first too. fentry/fexit will see ctx=r1 with +8 offset
and will have normal args in ctx. Like ip isn't even there.
While multi trampoline is always doing ip, arg1,arg2, .., arg6
and passes ctx = &ip into multi prog and ctx = &arg1 into fentry/fexit.
'ret' for fexit is problematic though. hmm.
Maybe such clone multi trampoline for specific ip with 2 args will do:
ip, arg1, arg2, ret, 0, 0, 0, ret.
Then multi will have 6 args, though 3rd is actually ret.
Then fexit will have ret in the right place and multi prog will have
it as 7th arg.
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
So I'm not sure why we added a new load flag instead of just using a
new BPF program type or expected attach type? We have different
trampolines and different kinds of links for them, so why not be
consistent and use the new type of BPF program?.. It does change BPF
verifier's treatment of input arguments, so it's not just a slight
variation, it's quite different type of program.
quoted hunk
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
The new bpf_trampoline will store and pass to bpf program the highest
number of arguments from all given functions.
New programs (fentry or fexit) can be added to the existing trampoline
through the link_update interface via new_prog_fd descriptor.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
include/linux/bpf.h | 3 +
include/uapi/linux/bpf.h | 5 +
kernel/bpf/syscall.c | 185 ++++++++++++++++++++++++++++++++-
kernel/bpf/trampoline.c | 53 +++++++---
tools/include/uapi/linux/bpf.h | 5 +
5 files changed, 237 insertions(+), 14 deletions(-)
@@ -1454,6 +1455,10 @@ union bpf_attr {__aligned_u64iter_info;/* extra bpf_iter_link_info */__u32iter_info_len;/* iter_info length */};+struct{+__aligned_u64multi_btf_ids;/* addresses to attach */+__u32multi_btf_ids_cnt;/* addresses count */+};
let's do what bpf_link-based TC-BPF API is doing, put it into a named
field (I'd do the same for iter_info/iter_info_len above as well, I'm
not sure why we did this flat naming scheme, we now it's inconvenient
when extending stuff).
struct {
__aligned_u64 btf_ids;
__u32 btf_ids_cnt;
} multi;
BPF_LINK_UPDATE command supports passing old_fd and extra flags. We
can use that to implement both updating existing BPF program in-place
(by passing BPF_F_REPLACE and old_fd) or adding the program to the
list of programs, if old_fd == 0. WDYT?
On Sat, Jun 5, 2021 at 4:14 AM Jiri Olsa [off-list ref] wrote:
quoted hunk
Adding btf__find_by_pattern_kind function that returns
array of BTF ids for given function name pattern.
Using libc's regex.h support for that.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/btf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
tools/lib/bpf/btf.h | 3 ++
2 files changed, 71 insertions(+)
@@ -711,6 +713,72 @@ __s32 btf__find_by_name_kind(const struct btf *btf, const char *type_name,returnlibbpf_err(-ENOENT);}+staticboolis_wildcard(charc)+{+staticconstchar*wildchars="*?[|";++returnstrchr(wildchars,c);+}++intbtf__find_by_pattern_kind(conststructbtf*btf,+constchar*type_pattern,__u32kind,+__s32**__ids)+{+__u32i,nr_types=btf__get_nr_types(btf);+__s32*ids=NULL;+intcnt=0,alloc=0,ret;+regex_tregex;+char*pattern;++if(kind==BTF_KIND_UNKN||!strcmp(type_pattern,"void"))+return0;++/* When the pattern does not start with wildcard, treat it as+*ifwe'dwanttomatchitfromthebeginningofthestring.+*/
This assumption is absolutely atrocious. If we say it's regexp, then
it has to always be regexp, not something based on some random
heuristic based on the first character.
Taking a step back, though. Do we really need to provide this API? Why
applications can't implement it on their own, given regexp
functionality is provided by libc. Which I didn't know, actually, so
that's pretty nice, assuming that it's also available in more minimal
implementations like musl.
+ asprintf(&pattern, "%s%s",
+ is_wildcard(type_pattern[0]) ? "^" : "",
+ type_pattern);
+
+ ret = regcomp(®ex, pattern, REG_EXTENDED);
+ if (ret) {
+ pr_warn("failed to compile regex\n");
+ free(pattern);
+ return -EINVAL;
+ }
+
+ free(pattern);
+
+ for (i = 1; i <= nr_types; i++) {
+ const struct btf_type *t = btf__type_by_id(btf, i);
+ const char *name;
+ __s32 *p;
+
+ if (btf_kind(t) != kind)
+ continue;
+ name = btf__name_by_offset(btf, t->name_off);
+ if (name && regexec(®ex, name, 0, NULL, 0))
+ continue;
+ if (cnt == alloc) {
+ alloc = max(100, alloc * 3 / 2);
+ p = realloc(ids, alloc * sizeof(__u32));
this memory allocation and re-allocation on behalf of users is another
argument against this API
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted hunk
Adding support to link multi func tracing program
through link_create interface.
Adding special types for multi func programs:
fentry.multi
fexit.multi
so you can define multi func programs like:
SEC("fentry.multi/bpf_fentry_test*")
int BPF_PROG(test1, unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f)
that defines test1 to be attached to bpf_fentry_test* functions,
and able to attach ip and 6 arguments.
If functions are not specified the program needs to be attached
manually.
Adding new btf id related fields to bpf_link_create_opts and
bpf_link_create to use them.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/bpf.c | 11 ++++++-
tools/lib/bpf/bpf.h | 4 ++-
tools/lib/bpf/libbpf.c | 72 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 2 deletions(-)
@@ -674,7 +674,8 @@ int bpf_link_create(int prog_fd, int target_fd,enumbpf_attach_typeattach_type,conststructbpf_link_create_opts*opts){-__u32target_btf_id,iter_info_len;+__u32target_btf_id,iter_info_len,multi_btf_ids_cnt;+__s32*multi_btf_ids;unionbpf_attrattr;intfd;
@@ -687,6 +688,9 @@ int bpf_link_create(int prog_fd, int target_fd,if(iter_info_len&&target_btf_id)
here we check that mutually exclusive options are not specified, we
should do the same for multi stuff
@@ -9584,6 +9597,9 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, if (!name) return -EINVAL;+ if (prog->prog_flags & BPF_F_MULTI_FUNC)+ return 0;+ for (i = 0; i < ARRAY_SIZE(section_defs); i++) { if (!section_defs[i].is_attach_btf) continue;
I wonder if it would be better to just support a simplified glob
patterns like "prefix*", "*suffix", "exactmatch", and "*substring*"?
That should be sufficient for majority of cases. For the cases where
user needs something more nuanced, they can just construct BTF ID list
with custom code and do manual attach.
wait, that's a regexp syntax that libc supports?.. Not .*? We should
definitely not provide btf__find_by_pattern_kind() API, I'd like to
avoid explaining what flavors of regexps libbpf supports.
+int BPF_PROG(test, unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f)
+{
+ multi_arg_check(ip, a, b, c, d, e, f, &test_result);
+ return 0;
+}
--
2.31.1
On Sat, Jun 5, 2021 at 4:13 AM Jiri Olsa [off-list ref] wrote:
Adding selftest for fentry/fexit multi func test that attaches
to bpf_fentry_test* functions and checks argument values based
on the processed function.
When multi_arg_check is used from 2 different places I'm getting
compilation fail, which I did not deciphered yet:
$ CLANG=/opt/clang/bin/clang LLC=/opt/clang/bin/llc make
CLNG-BPF [test_maps] fentry_fexit_multi_test.o
progs/fentry_fexit_multi_test.c:18:2: error: too many args to t24: i64 = \
GlobalAddress<void (i64, i64, i64, i64, i64, i64, i64, i64*)* @multi_arg_check> 0, \
progs/fentry_fexit_multi_test.c:18:2 @[ progs/fentry_fexit_multi_test.c:16:5 ]
multi_arg_check(ip, a, b, c, d, e, f, &test1_arg_result);
^
progs/fentry_fexit_multi_test.c:25:2: error: too many args to t32: i64 = \
GlobalAddress<void (i64, i64, i64, i64, i64, i64, i64, i64*)* @multi_arg_check> 0, \
progs/fentry_fexit_multi_test.c:25:2 @[ progs/fentry_fexit_multi_test.c:23:5 ]
multi_arg_check(ip, a, b, c, d, e, f, &test2_arg_result);
^
In file included from progs/fentry_fexit_multi_test.c:5:
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
void multi_arg_check(unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f, __u64 *test_result)
^
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
5 errors generated.
make: *** [Makefile:470: /home/jolsa/linux-qemu/tools/testing/selftests/bpf/fentry_fexit_multi_test.o] Error 1
I can fix that by defining 2 separate multi_arg_check functions
with different names, which I did in follow up temporaary patch.
Not sure I'm hitting some clang/bpf limitation in here?
don't know about clang limitations, but we should use static linking
proper anyways
From: Jiri Olsa <hidden> Date: 2021-06-09 13:33:47
On Tue, Jun 08, 2021 at 04:05:29PM -0700, Alexei Starovoitov wrote:
On Tue, Jun 8, 2021 at 2:07 PM Jiri Olsa [off-list ref] wrote:
quoted
On Tue, Jun 08, 2021 at 11:49:03AM -0700, Alexei Starovoitov wrote:
quoted
On Tue, Jun 08, 2021 at 08:17:00PM +0200, Jiri Olsa wrote:
quoted
On Tue, Jun 08, 2021 at 08:42:32AM -0700, Alexei Starovoitov wrote:
quoted
On Sat, Jun 5, 2021 at 4:11 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
Overall the api makes sense to me.
The restriction of multi vs non-multi is too severe though.
The multi trampoline can serve normal fentry/fexit too.
so multi trampoline gets called from all the registered functions,
so there would need to be filter for specific ip before calling the
standard program.. single cmp/jnz might not be that bad, I'll check
You mean reusing the same multi trampoline for all IPs and regenerating
it with a bunch of cmp/jnz checks? There should be a better way to scale.
Maybe clone multi trampoline instead?
IPs[1-10] will point to multi.
IP[11] will point to a clone of multi that serves multi prog and
fentry/fexit progs specific for that IP.
ok, so we'd clone multi trampoline if there's request to attach
standard trampoline to some IP from multi trampoline
.. and transform currently attached standard trampoline for IP
into clone of multi trampoline, if there's request to create
multi trampoline that covers that IP
yep. For every IP==btf_id there will be only two possible trampolines.
Should be easy enough to track and transition between them.
The standard fentry/fexit will only get negligible slowdown from
going through multi.
multi+fexit and fmod_ret needs to be thought through as well.
That's why I thought that 'ip' at the end should simplify things.
Only multi will have access to it.
But we can store it first too. fentry/fexit will see ctx=r1 with +8 offset
and will have normal args in ctx. Like ip isn't even there.
While multi trampoline is always doing ip, arg1,arg2, .., arg6
and passes ctx = &ip into multi prog and ctx = &arg1 into fentry/fexit.
'ret' for fexit is problematic though. hmm.
Maybe such clone multi trampoline for specific ip with 2 args will do:
ip, arg1, arg2, ret, 0, 0, 0, ret.
we could call multi progs first and setup new args
and call non-multi progs with that
jirka
Then multi will have 6 args, though 3rd is actually ret.
Then fexit will have ret in the right place and multi prog will have
it as 7th arg.
From: Jiri Olsa <hidden> Date: 2021-06-09 13:42:52
On Tue, Jun 08, 2021 at 10:08:32PM -0700, Andrii Nakryiko wrote:
On Tue, Jun 8, 2021 at 4:07 PM Alexei Starovoitov
[off-list ref] wrote:
quoted
On Tue, Jun 8, 2021 at 2:07 PM Jiri Olsa [off-list ref] wrote:
quoted
On Tue, Jun 08, 2021 at 11:49:03AM -0700, Alexei Starovoitov wrote:
quoted
On Tue, Jun 08, 2021 at 08:17:00PM +0200, Jiri Olsa wrote:
quoted
On Tue, Jun 08, 2021 at 08:42:32AM -0700, Alexei Starovoitov wrote:
quoted
On Sat, Jun 5, 2021 at 4:11 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
The new link_create interface creates new BPF_LINK_TYPE_TRACING_MULTI
link type, which creates separate bpf_trampoline and registers it
as direct function for all specified btf ids.
The new bpf_trampoline is out of scope (bpf_trampoline_lookup) of
standard trampolines, so all registered functions need to be free
of direct functions, otherwise the link fails.
Overall the api makes sense to me.
The restriction of multi vs non-multi is too severe though.
The multi trampoline can serve normal fentry/fexit too.
so multi trampoline gets called from all the registered functions,
so there would need to be filter for specific ip before calling the
standard program.. single cmp/jnz might not be that bad, I'll check
You mean reusing the same multi trampoline for all IPs and regenerating
it with a bunch of cmp/jnz checks? There should be a better way to scale.
Maybe clone multi trampoline instead?
IPs[1-10] will point to multi.
IP[11] will point to a clone of multi that serves multi prog and
fentry/fexit progs specific for that IP.
ok, so we'd clone multi trampoline if there's request to attach
standard trampoline to some IP from multi trampoline
.. and transform currently attached standard trampoline for IP
into clone of multi trampoline, if there's request to create
multi trampoline that covers that IP
yep. For every IP==btf_id there will be only two possible trampolines.
Should be easy enough to track and transition between them.
The standard fentry/fexit will only get negligible slowdown from
going through multi.
multi+fexit and fmod_ret needs to be thought through as well.
That's why I thought that 'ip' at the end should simplify things.
Putting ip at the end has downsides. We might support >6 arguments
eventually, at which point it will be super weird to have 6 args, ip,
then the rest of arguments?..
Would it be too bad to put IP at -8 offset relative to ctx? That will
also work for normal fentry/fexit, for which it's useful to have ip
passed in as well, IMO. So no special casing for multi/non-multi, and
it's backwards compatible.
I think Alexei is ok with that, as he said below
Ideally, I'd love it to be actually retrievable through a new BPF
helper, something like bpf_caller_ip(ctx), but I'm not sure if we can
implement this sanely, so I don't hold high hopes.
we could always store it in ctx-8 and have the helper to get it
from there.. that might also ease up handling that extra first
ip argument for multi-func programs in verifier
jirka
quoted
Only multi will have access to it.
But we can store it first too. fentry/fexit will see ctx=r1 with +8 offset
and will have normal args in ctx. Like ip isn't even there.
While multi trampoline is always doing ip, arg1,arg2, .., arg6
and passes ctx = &ip into multi prog and ctx = &arg1 into fentry/fexit.
'ret' for fexit is problematic though. hmm.
Maybe such clone multi trampoline for specific ip with 2 args will do:
ip, arg1, arg2, ret, 0, 0, 0, ret.
Then multi will have 6 args, though 3rd is actually ret.
Then fexit will have ret in the right place and multi prog will have
it as 7th arg.
From: Jiri Olsa <hidden> Date: 2021-06-09 13:53:12
On Tue, Jun 08, 2021 at 10:18:21PM -0700, Andrii Nakryiko wrote:
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to attach multiple functions to tracing program
by using the link_create/link_update interface.
Adding multi_btf_ids/multi_btf_ids_cnt pair to link_create struct
API, that define array of functions btf ids that will be attached
to prog_fd.
The prog_fd needs to be multi prog tracing program (BPF_F_MULTI_FUNC).
So I'm not sure why we added a new load flag instead of just using a
new BPF program type or expected attach type? We have different
trampolines and different kinds of links for them, so why not be
consistent and use the new type of BPF program?.. It does change BPF
verifier's treatment of input arguments, so it's not just a slight
variation, it's quite different type of program.
ok, makes sense ... BPF_PROG_TYPE_TRACING_MULTI ?
SNIP
@@ -1454,6 +1455,10 @@ union bpf_attr {__aligned_u64iter_info;/* extra bpf_iter_link_info */__u32iter_info_len;/* iter_info length */};+struct{+__aligned_u64multi_btf_ids;/* addresses to attach */+__u32multi_btf_ids_cnt;/* addresses count */+};
let's do what bpf_link-based TC-BPF API is doing, put it into a named
field (I'd do the same for iter_info/iter_info_len above as well, I'm
not sure why we did this flat naming scheme, we now it's inconvenient
when extending stuff).
struct {
__aligned_u64 btf_ids;
__u32 btf_ids_cnt;
} multi;
BPF_LINK_UPDATE command supports passing old_fd and extra flags. We
can use that to implement both updating existing BPF program in-place
(by passing BPF_F_REPLACE and old_fd) or adding the program to the
list of programs, if old_fd == 0. WDYT?
From: Jiri Olsa <hidden> Date: 2021-06-09 13:59:58
On Tue, Jun 08, 2021 at 10:29:19PM -0700, Andrii Nakryiko wrote:
On Sat, Jun 5, 2021 at 4:14 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding btf__find_by_pattern_kind function that returns
array of BTF ids for given function name pattern.
Using libc's regex.h support for that.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/btf.c | 68 +++++++++++++++++++++++++++++++++++++++++++++
tools/lib/bpf/btf.h | 3 ++
2 files changed, 71 insertions(+)
@@ -711,6 +713,72 @@ __s32 btf__find_by_name_kind(const struct btf *btf, const char *type_name,returnlibbpf_err(-ENOENT);}+staticboolis_wildcard(charc)+{+staticconstchar*wildchars="*?[|";++returnstrchr(wildchars,c);+}++intbtf__find_by_pattern_kind(conststructbtf*btf,+constchar*type_pattern,__u32kind,+__s32**__ids)+{+__u32i,nr_types=btf__get_nr_types(btf);+__s32*ids=NULL;+intcnt=0,alloc=0,ret;+regex_tregex;+char*pattern;++if(kind==BTF_KIND_UNKN||!strcmp(type_pattern,"void"))+return0;++/* When the pattern does not start with wildcard, treat it as+*ifwe'dwanttomatchitfromthebeginningofthestring.+*/
This assumption is absolutely atrocious. If we say it's regexp, then
it has to always be regexp, not something based on some random
heuristic based on the first character.
Taking a step back, though. Do we really need to provide this API? Why
applications can't implement it on their own, given regexp
functionality is provided by libc. Which I didn't know, actually, so
that's pretty nice, assuming that it's also available in more minimal
implementations like musl.
so the only purpose for this function is to support wildcards in
tests like:
SEC("fentry.multi/bpf_fentry_test*")
so the generic skeleton attach function can work.. but that can be
removed and the test programs can be attached manually through some
other attach function that will have list of functions as argument
jirka
quoted
+ asprintf(&pattern, "%s%s",
+ is_wildcard(type_pattern[0]) ? "^" : "",
+ type_pattern);
+
+ ret = regcomp(®ex, pattern, REG_EXTENDED);
+ if (ret) {
+ pr_warn("failed to compile regex\n");
+ free(pattern);
+ return -EINVAL;
+ }
+
+ free(pattern);
+
+ for (i = 1; i <= nr_types; i++) {
+ const struct btf_type *t = btf__type_by_id(btf, i);
+ const char *name;
+ __s32 *p;
+
+ if (btf_kind(t) != kind)
+ continue;
+ name = btf__name_by_offset(btf, t->name_off);
+ if (name && regexec(®ex, name, 0, NULL, 0))
+ continue;
+ if (cnt == alloc) {
+ alloc = max(100, alloc * 3 / 2);
+ p = realloc(ids, alloc * sizeof(__u32));
this memory allocation and re-allocation on behalf of users is another
argument against this API
From: Jiri Olsa <hidden> Date: 2021-06-09 14:17:44
On Tue, Jun 08, 2021 at 10:34:11PM -0700, Andrii Nakryiko wrote:
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to link multi func tracing program
through link_create interface.
Adding special types for multi func programs:
fentry.multi
fexit.multi
so you can define multi func programs like:
SEC("fentry.multi/bpf_fentry_test*")
int BPF_PROG(test1, unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f)
that defines test1 to be attached to bpf_fentry_test* functions,
and able to attach ip and 6 arguments.
If functions are not specified the program needs to be attached
manually.
Adding new btf id related fields to bpf_link_create_opts and
bpf_link_create to use them.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/bpf.c | 11 ++++++-
tools/lib/bpf/bpf.h | 4 ++-
tools/lib/bpf/libbpf.c | 72 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 2 deletions(-)
@@ -674,7 +674,8 @@ int bpf_link_create(int prog_fd, int target_fd,enumbpf_attach_typeattach_type,conststructbpf_link_create_opts*opts){-__u32target_btf_id,iter_info_len;+__u32target_btf_id,iter_info_len,multi_btf_ids_cnt;+__s32*multi_btf_ids;unionbpf_attrattr;intfd;
@@ -687,6 +688,9 @@ int bpf_link_create(int prog_fd, int target_fd,if(iter_info_len&&target_btf_id)
here we check that mutually exclusive options are not specified, we
should do the same for multi stuff
@@ -9584,6 +9597,9 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, if (!name) return -EINVAL;+ if (prog->prog_flags & BPF_F_MULTI_FUNC)+ return 0;+ for (i = 0; i < ARRAY_SIZE(section_defs); i++) { if (!section_defs[i].is_attach_btf) continue;
I wonder if it would be better to just support a simplified glob
patterns like "prefix*", "*suffix", "exactmatch", and "*substring*"?
That should be sufficient for majority of cases. For the cases where
user needs something more nuanced, they can just construct BTF ID list
with custom code and do manual attach.
as I wrote earlier the function is just for the purpose of the test,
and we can always do the manual attach
I don't mind adding that simplified matching you described
jirka
From: Jiri Olsa <hidden> Date: 2021-06-09 14:19:40
On Wed, Jun 09, 2021 at 03:59:47PM +0200, Jiri Olsa wrote:
SNIP
quoted
quoted
+
+ /* When the pattern does not start with wildcard, treat it as
+ * if we'd want to match it from the beginning of the string.
+ */
This assumption is absolutely atrocious. If we say it's regexp, then
it has to always be regexp, not something based on some random
heuristic based on the first character.
Taking a step back, though. Do we really need to provide this API? Why
applications can't implement it on their own, given regexp
functionality is provided by libc. Which I didn't know, actually, so
that's pretty nice, assuming that it's also available in more minimal
implementations like musl.
so the only purpose for this function is to support wildcards in
tests like:
SEC("fentry.multi/bpf_fentry_test*")
so the generic skeleton attach function can work.. but that can be
removed and the test programs can be attached manually through some
other attach function that will have list of functions as argument
nah, no other attach function is needed, we have that support now in
link_create ready to use ;-) sry
jirka
wait, that's a regexp syntax that libc supports?.. Not .*? We should
definitely not provide btf__find_by_pattern_kind() API, I'd like to
avoid explaining what flavors of regexps libbpf supports.
From: Jiri Olsa <hidden> Date: 2021-06-09 14:29:54
On Tue, Jun 08, 2021 at 10:41:37PM -0700, Andrii Nakryiko wrote:
On Sat, Jun 5, 2021 at 4:13 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding selftest for fentry/fexit multi func test that attaches
to bpf_fentry_test* functions and checks argument values based
on the processed function.
When multi_arg_check is used from 2 different places I'm getting
compilation fail, which I did not deciphered yet:
$ CLANG=/opt/clang/bin/clang LLC=/opt/clang/bin/llc make
CLNG-BPF [test_maps] fentry_fexit_multi_test.o
progs/fentry_fexit_multi_test.c:18:2: error: too many args to t24: i64 = \
GlobalAddress<void (i64, i64, i64, i64, i64, i64, i64, i64*)* @multi_arg_check> 0, \
progs/fentry_fexit_multi_test.c:18:2 @[ progs/fentry_fexit_multi_test.c:16:5 ]
multi_arg_check(ip, a, b, c, d, e, f, &test1_arg_result);
^
progs/fentry_fexit_multi_test.c:25:2: error: too many args to t32: i64 = \
GlobalAddress<void (i64, i64, i64, i64, i64, i64, i64, i64*)* @multi_arg_check> 0, \
progs/fentry_fexit_multi_test.c:25:2 @[ progs/fentry_fexit_multi_test.c:23:5 ]
multi_arg_check(ip, a, b, c, d, e, f, &test2_arg_result);
^
In file included from progs/fentry_fexit_multi_test.c:5:
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
void multi_arg_check(unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f, __u64 *test_result)
^
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
/home/jolsa/linux-qemu/tools/testing/selftests/bpf/multi_check.h:9:6: error: defined with too many args
5 errors generated.
make: *** [Makefile:470: /home/jolsa/linux-qemu/tools/testing/selftests/bpf/fentry_fexit_multi_test.o] Error 1
I can fix that by defining 2 separate multi_arg_check functions
with different names, which I did in follow up temporaary patch.
Not sure I'm hitting some clang/bpf limitation in here?
don't know about clang limitations, but we should use static linking
proper anyways
we have a proper static linking now, we don't have to use header
inclusion hacks, let's do this properly?
ok, will change
quoted
quoted
@@ -0,0 +1,52 @@+/* SPDX-License-Identifier: GPL-2.0 */++#ifndef __MULTI_CHECK_H+#define __MULTI_CHECK_H++extern unsigned long long bpf_fentry_test[8];++static __attribute__((unused)) inline+void multi_arg_check(unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f, __u64 *test_result)+{+ if (ip == bpf_fentry_test[0]) {+ *test_result += (int) a == 1;+ } else if (ip == bpf_fentry_test[1]) {+ *test_result += (int) a == 2 && (__u64) b == 3;+ } else if (ip == bpf_fentry_test[2]) {+ *test_result += (char) a == 4 && (int) b == 5 && (__u64) c == 6;+ } else if (ip == bpf_fentry_test[3]) {+ *test_result += (void *) a == (void *) 7 && (char) b == 8 && (int) c == 9 && (__u64) d == 10;+ } else if (ip == bpf_fentry_test[4]) {+ *test_result += (__u64) a == 11 && (void *) b == (void *) 12 && (short) c == 13 && (int) d == 14 && (__u64) e == 15;+ } else if (ip == bpf_fentry_test[5]) {+ *test_result += (__u64) a == 16 && (void *) b == (void *) 17 && (short) c == 18 && (int) d == 19 && (void *) e == (void *) 20 && (__u64) f == 21;+ } else if (ip == bpf_fentry_test[6]) {+ *test_result += 1;+ } else if (ip == bpf_fentry_test[7]) {+ *test_result += 1;+ }
why not use switch? and why the casting?
hum, for switch I'd need constants right?
doh, of course :)
but! you don't need to fill out bpf_fentry_test[] array from
user-space, just use extern const void variables to get addresses of
those functions:
extern const void bpf_fentry_test1 __ksym;
extern const void bpf_fentry_test2 __ksym;
...
casting is extra ;-) wanted to check the actual argument types,
but probably makes no sense
probably doesn't given you already declared it u64 and use integer
values for comparison
wait, that's a regexp syntax that libc supports?.. Not .*? We should
definitely not provide btf__find_by_pattern_kind() API, I'd like to
avoid explaining what flavors of regexps libbpf supports.
On Wed, Jun 9, 2021 at 7:17 AM Jiri Olsa [off-list ref] wrote:
On Tue, Jun 08, 2021 at 10:34:11PM -0700, Andrii Nakryiko wrote:
quoted
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
Adding support to link multi func tracing program
through link_create interface.
Adding special types for multi func programs:
fentry.multi
fexit.multi
so you can define multi func programs like:
SEC("fentry.multi/bpf_fentry_test*")
int BPF_PROG(test1, unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f)
that defines test1 to be attached to bpf_fentry_test* functions,
and able to attach ip and 6 arguments.
If functions are not specified the program needs to be attached
manually.
Adding new btf id related fields to bpf_link_create_opts and
bpf_link_create to use them.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/bpf.c | 11 ++++++-
tools/lib/bpf/bpf.h | 4 ++-
tools/lib/bpf/libbpf.c | 72 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 2 deletions(-)
@@ -674,7 +674,8 @@ int bpf_link_create(int prog_fd, int target_fd,enumbpf_attach_typeattach_type,conststructbpf_link_create_opts*opts){-__u32target_btf_id,iter_info_len;+__u32target_btf_id,iter_info_len,multi_btf_ids_cnt;+__s32*multi_btf_ids;unionbpf_attrattr;intfd;
@@ -687,6 +688,9 @@ int bpf_link_create(int prog_fd, int target_fd,if(iter_info_len&&target_btf_id)
here we check that mutually exclusive options are not specified, we
should do the same for multi stuff
@@ -9584,6 +9597,9 @@ static int libbpf_find_attach_btf_id(struct bpf_program *prog, int *btf_obj_fd, if (!name) return -EINVAL;+ if (prog->prog_flags & BPF_F_MULTI_FUNC)+ return 0;+ for (i = 0; i < ARRAY_SIZE(section_defs); i++) { if (!section_defs[i].is_attach_btf) continue;
I wonder if it would be better to just support a simplified glob
patterns like "prefix*", "*suffix", "exactmatch", and "*substring*"?
That should be sufficient for majority of cases. For the cases where
user needs something more nuanced, they can just construct BTF ID list
with custom code and do manual attach.
as I wrote earlier the function is just for the purpose of the test,
and we can always do the manual attach
I don't mind adding that simplified matching you described
I use that in retsnoop and that seems to be simple but flexible enough
for all the purposes, so far. It matches typical file globbing rules
(with extra limitations, of course), so it's also intuitive.
But I still am not sure about making it a public API, because in a lot
of cases you'll want a list of patterns (both allowing and denying
different patterns), so it should be generalized to something like
btf__find_by_glob_kind(btf, allow_patterns, deny_patterns, ids)
which gets pretty unwieldy. I'd start with telling users to just
iterate BTF on their own and apply whatever custom filtering they
need. For simple cases libbpf will just initially support a simple and
single glob filter declaratively (e.g, SEC("fentry.multi/bpf_*")).
we have a proper static linking now, we don't have to use header
inclusion hacks, let's do this properly?
ok, will change
quoted
quoted
@@ -0,0 +1,52 @@+/* SPDX-License-Identifier: GPL-2.0 */++#ifndef __MULTI_CHECK_H+#define __MULTI_CHECK_H++extern unsigned long long bpf_fentry_test[8];++static __attribute__((unused)) inline+void multi_arg_check(unsigned long ip, __u64 a, __u64 b, __u64 c, __u64 d, __u64 e, __u64 f, __u64 *test_result)+{+ if (ip == bpf_fentry_test[0]) {+ *test_result += (int) a == 1;+ } else if (ip == bpf_fentry_test[1]) {+ *test_result += (int) a == 2 && (__u64) b == 3;+ } else if (ip == bpf_fentry_test[2]) {+ *test_result += (char) a == 4 && (int) b == 5 && (__u64) c == 6;+ } else if (ip == bpf_fentry_test[3]) {+ *test_result += (void *) a == (void *) 7 && (char) b == 8 && (int) c == 9 && (__u64) d == 10;+ } else if (ip == bpf_fentry_test[4]) {+ *test_result += (__u64) a == 11 && (void *) b == (void *) 12 && (short) c == 13 && (int) d == 14 && (__u64) e == 15;+ } else if (ip == bpf_fentry_test[5]) {+ *test_result += (__u64) a == 16 && (void *) b == (void *) 17 && (short) c == 18 && (int) d == 19 && (void *) e == (void *) 20 && (__u64) f == 21;+ } else if (ip == bpf_fentry_test[6]) {+ *test_result += 1;+ } else if (ip == bpf_fentry_test[7]) {+ *test_result += 1;+ }
why not use switch? and why the casting?
hum, for switch I'd need constants right?
doh, of course :)
but! you don't need to fill out bpf_fentry_test[] array from
user-space, just use extern const void variables to get addresses of
those functions:
extern const void bpf_fentry_test1 __ksym;
extern const void bpf_fentry_test2 __ksym;
...
I wonder if it would be better to just support a simplified glob
patterns like "prefix*", "*suffix", "exactmatch", and "*substring*"?
That should be sufficient for majority of cases. For the cases where
user needs something more nuanced, they can just construct BTF ID list
with custom code and do manual attach.
as I wrote earlier the function is just for the purpose of the test,
and we can always do the manual attach
I don't mind adding that simplified matching you described
I use that in retsnoop and that seems to be simple but flexible enough
for all the purposes, so far. It matches typical file globbing rules
(with extra limitations, of course), so it's also intuitive.
But I still am not sure about making it a public API, because in a lot
of cases you'll want a list of patterns (both allowing and denying
different patterns), so it should be generalized to something like
btf__find_by_glob_kind(btf, allow_patterns, deny_patterns, ids)
which gets pretty unwieldy. I'd start with telling users to just
iterate BTF on their own and apply whatever custom filtering they
need. For simple cases libbpf will just initially support a simple and
single glob filter declaratively (e.g, SEC("fentry.multi/bpf_*")).
ok, I'll scan retsnoop and see what I can steal ;-)
jirka
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
hi,
saga continues.. ;-) previous post is in here [1]
After another discussion with Steven, he mentioned that if we fix
the ftrace graph problem with direct functions, he'd be open to
add batch interface for direct ftrace functions.
He already had prove of concept fix for that, which I took and broke
up into several changes. I added the ftrace direct batch interface
and bpf new interface on top of that.
It's not so many patches after all, so I thought having them all
together will help the review, because they are all connected.
However I can break this up into separate patchsets if necessary.
This patchset contains:
1) patches (1-4) that fix the ftrace graph tracing over the function
with direct trampolines attached
2) patches (5-8) that add batch interface for ftrace direct function
register/unregister/modify
3) patches (9-19) that add support to attach BPF program to multiple
functions
In nutshell:
Ad 1) moves the graph tracing setup before the direct trampoline
prepares the stack, so they don't clash
Ad 2) uses ftrace_ops interface to register direct function with
all functions in ftrace_ops filter.
Ad 3) creates special program and trampoline type to allow attachment
of multiple functions to single program.
There're more detailed desriptions in related changelogs.
I have working bpftrace multi attachment code on top this. I briefly
checked retsnoop and I think it could use the new API as well.
Ok, so I had a bit of time and enthusiasm to try that with retsnoop.
The ugly code is at [0] if you'd like to see what kind of changes I
needed to make to use this (it won't work if you check it out because
it needs your libbpf changes synced into submodule, which I only did
locally). But here are some learnings from that experiment both to
emphasize how important it is to make this work and how restrictive
are some of the current limitations.
First, good news. Using this mass-attach API to attach to almost 1000
kernel functions goes from
Plain fentry/fexit:
===================
real 0m27.321s
user 0m0.352s
sys 0m20.919s
to
Mass-attach fentry/fexit:
=========================
real 0m2.728s
user 0m0.329s
sys 0m2.380s
It's a 10x speed up. And a good chunk of those 2.7 seconds is in some
preparatory steps not related to fentry/fexit stuff.
It's not exactly apples-to-apples, though, because the limitations you
have right now prevents attaching both fentry and fexit programs to
the same set of kernel functions. This makes it pretty useless for a
lot of cases, in particular for retsnoop. So I haven't really tested
retsnoop end-to-end, I only verified that I do see fentries triggered,
but can't have matching fexits. So the speed-up might be smaller due
to additional fexit mass-attach (once that is allowed), but it's still
a massive difference. So we absolutely need to get this optimization
in.
Few more thoughts, if you'd like to plan some more work ahead ;)
1. We need similar mass-attach functionality for kprobe/kretprobe, as
there are use cases where kprobe are more useful than fentry (e.g., >6
args funcs, or funcs with input arguments that are not supported by
BPF verifier, like struct-by-value). It's not clear how to best
represent this, given currently we attach kprobe through perf_event,
but we'll need to think about this for sure.
2. To make mass-attach fentry/fexit useful for practical purposes, it
would be really great to have an ability to fetch traced function's
IP. I.e., if we fentry/fexit func kern_func_abc, bpf_get_func_ip()
would return IP of that functions that matches the one in
/proc/kallsyms. Right now I do very brittle hacks to do that.
So all-in-all, super excited about this, but I hope all those issues
are addressed to make retsnoop possible and fast.
[0] https://github.com/anakryiko/retsnoop/commit/8a07bc4d8c47d025f755c108f92f0583e3fda6d8
Also available at:
https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
bpf/batch
thanks,
jirka
[1] https://lore.kernel.org/bpf/20210413121516.1467989-1-jolsa@kernel.org/
---
Jiri Olsa (17):
x86/ftrace: Remove extra orig rax move
tracing: Add trampoline/graph selftest
ftrace: Add ftrace_add_rec_direct function
ftrace: Add multi direct register/unregister interface
ftrace: Add multi direct modify interface
ftrace/samples: Add multi direct interface test module
bpf, x64: Allow to use caller address from stack
bpf: Allow to store caller's ip as argument
bpf: Add support to load multi func tracing program
bpf: Add bpf_trampoline_alloc function
bpf: Add support to link multi func tracing program
libbpf: Add btf__find_by_pattern_kind function
libbpf: Add support to link multi func tracing program
selftests/bpf: Add fentry multi func test
selftests/bpf: Add fexit multi func test
selftests/bpf: Add fentry/fexit multi func test
selftests/bpf: Temporary fix for fentry_fexit_multi_test
Steven Rostedt (VMware) (2):
x86/ftrace: Remove fault protection code in prepare_ftrace_return
x86/ftrace: Make function graph use ftrace directly
From: Jiri Olsa <hidden> Date: 2021-06-19 08:33:38
On Thu, Jun 17, 2021 at 01:29:45PM -0700, Andrii Nakryiko wrote:
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
hi,
saga continues.. ;-) previous post is in here [1]
After another discussion with Steven, he mentioned that if we fix
the ftrace graph problem with direct functions, he'd be open to
add batch interface for direct ftrace functions.
He already had prove of concept fix for that, which I took and broke
up into several changes. I added the ftrace direct batch interface
and bpf new interface on top of that.
It's not so many patches after all, so I thought having them all
together will help the review, because they are all connected.
However I can break this up into separate patchsets if necessary.
This patchset contains:
1) patches (1-4) that fix the ftrace graph tracing over the function
with direct trampolines attached
2) patches (5-8) that add batch interface for ftrace direct function
register/unregister/modify
3) patches (9-19) that add support to attach BPF program to multiple
functions
In nutshell:
Ad 1) moves the graph tracing setup before the direct trampoline
prepares the stack, so they don't clash
Ad 2) uses ftrace_ops interface to register direct function with
all functions in ftrace_ops filter.
Ad 3) creates special program and trampoline type to allow attachment
of multiple functions to single program.
There're more detailed desriptions in related changelogs.
I have working bpftrace multi attachment code on top this. I briefly
checked retsnoop and I think it could use the new API as well.
Ok, so I had a bit of time and enthusiasm to try that with retsnoop.
The ugly code is at [0] if you'd like to see what kind of changes I
needed to make to use this (it won't work if you check it out because
it needs your libbpf changes synced into submodule, which I only did
locally). But here are some learnings from that experiment both to
emphasize how important it is to make this work and how restrictive
are some of the current limitations.
First, good news. Using this mass-attach API to attach to almost 1000
kernel functions goes from
Plain fentry/fexit:
===================
real 0m27.321s
user 0m0.352s
sys 0m20.919s
to
Mass-attach fentry/fexit:
=========================
real 0m2.728s
user 0m0.329s
sys 0m2.380s
I did not meassured the bpftrace speedup, because the new code
attached instantly ;-)
It's a 10x speed up. And a good chunk of those 2.7 seconds is in some
preparatory steps not related to fentry/fexit stuff.
It's not exactly apples-to-apples, though, because the limitations you
have right now prevents attaching both fentry and fexit programs to
the same set of kernel functions. This makes it pretty useless for a
hum, you could do link_update with fexit program on the link fd,
like in the selftest, right?
lot of cases, in particular for retsnoop. So I haven't really tested
retsnoop end-to-end, I only verified that I do see fentries triggered,
but can't have matching fexits. So the speed-up might be smaller due
to additional fexit mass-attach (once that is allowed), but it's still
a massive difference. So we absolutely need to get this optimization
in.
Few more thoughts, if you'd like to plan some more work ahead ;)
1. We need similar mass-attach functionality for kprobe/kretprobe, as
there are use cases where kprobe are more useful than fentry (e.g., >6
args funcs, or funcs with input arguments that are not supported by
BPF verifier, like struct-by-value). It's not clear how to best
represent this, given currently we attach kprobe through perf_event,
but we'll need to think about this for sure.
I'm fighting with the '2 trampolines concept' at the moment, but the
mass attach for kprobes seems interesting ;-) will check
2. To make mass-attach fentry/fexit useful for practical purposes, it
would be really great to have an ability to fetch traced function's
IP. I.e., if we fentry/fexit func kern_func_abc, bpf_get_func_ip()
would return IP of that functions that matches the one in
/proc/kallsyms. Right now I do very brittle hacks to do that.
so I hoped that we could store ip always in ctx-8 and have
the bpf_get_func_ip helper to access that, but the BPF_PROG
macro does not pass ctx value to the program, just args
we could perhaps somehow store the ctx in BPF_PROG before calling
the bpf program, but I did not get to try that yet
From: Yonghong Song <hidden> Date: 2021-06-19 16:20:52
On 6/19/21 1:33 AM, Jiri Olsa wrote:
On Thu, Jun 17, 2021 at 01:29:45PM -0700, Andrii Nakryiko wrote:
quoted
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
hi,
saga continues.. ;-) previous post is in here [1]
After another discussion with Steven, he mentioned that if we fix
the ftrace graph problem with direct functions, he'd be open to
add batch interface for direct ftrace functions.
He already had prove of concept fix for that, which I took and broke
up into several changes. I added the ftrace direct batch interface
and bpf new interface on top of that.
It's not so many patches after all, so I thought having them all
together will help the review, because they are all connected.
However I can break this up into separate patchsets if necessary.
This patchset contains:
1) patches (1-4) that fix the ftrace graph tracing over the function
with direct trampolines attached
2) patches (5-8) that add batch interface for ftrace direct function
register/unregister/modify
3) patches (9-19) that add support to attach BPF program to multiple
functions
In nutshell:
Ad 1) moves the graph tracing setup before the direct trampoline
prepares the stack, so they don't clash
Ad 2) uses ftrace_ops interface to register direct function with
all functions in ftrace_ops filter.
Ad 3) creates special program and trampoline type to allow attachment
of multiple functions to single program.
There're more detailed desriptions in related changelogs.
I have working bpftrace multi attachment code on top this. I briefly
checked retsnoop and I think it could use the new API as well.
Ok, so I had a bit of time and enthusiasm to try that with retsnoop.
The ugly code is at [0] if you'd like to see what kind of changes I
needed to make to use this (it won't work if you check it out because
it needs your libbpf changes synced into submodule, which I only did
locally). But here are some learnings from that experiment both to
emphasize how important it is to make this work and how restrictive
are some of the current limitations.
First, good news. Using this mass-attach API to attach to almost 1000
kernel functions goes from
Plain fentry/fexit:
===================
real 0m27.321s
user 0m0.352s
sys 0m20.919s
to
Mass-attach fentry/fexit:
=========================
real 0m2.728s
user 0m0.329s
sys 0m2.380s
I did not meassured the bpftrace speedup, because the new code
attached instantly ;-)
quoted
It's a 10x speed up. And a good chunk of those 2.7 seconds is in some
preparatory steps not related to fentry/fexit stuff.
It's not exactly apples-to-apples, though, because the limitations you
have right now prevents attaching both fentry and fexit programs to
the same set of kernel functions. This makes it pretty useless for a
hum, you could do link_update with fexit program on the link fd,
like in the selftest, right?
quoted
lot of cases, in particular for retsnoop. So I haven't really tested
retsnoop end-to-end, I only verified that I do see fentries triggered,
but can't have matching fexits. So the speed-up might be smaller due
to additional fexit mass-attach (once that is allowed), but it's still
a massive difference. So we absolutely need to get this optimization
in.
Few more thoughts, if you'd like to plan some more work ahead ;)
1. We need similar mass-attach functionality for kprobe/kretprobe, as
there are use cases where kprobe are more useful than fentry (e.g., >6
args funcs, or funcs with input arguments that are not supported by
BPF verifier, like struct-by-value). It's not clear how to best
represent this, given currently we attach kprobe through perf_event,
but we'll need to think about this for sure.
I'm fighting with the '2 trampolines concept' at the moment, but the
mass attach for kprobes seems interesting ;-) will check
quoted
2. To make mass-attach fentry/fexit useful for practical purposes, it
would be really great to have an ability to fetch traced function's
IP. I.e., if we fentry/fexit func kern_func_abc, bpf_get_func_ip()
would return IP of that functions that matches the one in
/proc/kallsyms. Right now I do very brittle hacks to do that.
so I hoped that we could store ip always in ctx-8 and have
the bpf_get_func_ip helper to access that, but the BPF_PROG
macro does not pass ctx value to the program, just args
ctx does pass to the bpf program. You can check BPF_PROG
macro definition.
we could perhaps somehow store the ctx in BPF_PROG before calling
the bpf program, but I did not get to try that yet
From: Jiri Olsa <hidden> Date: 2021-06-19 17:09:37
On Sat, Jun 19, 2021 at 09:19:57AM -0700, Yonghong Song wrote:
On 6/19/21 1:33 AM, Jiri Olsa wrote:
quoted
On Thu, Jun 17, 2021 at 01:29:45PM -0700, Andrii Nakryiko wrote:
quoted
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
hi,
saga continues.. ;-) previous post is in here [1]
After another discussion with Steven, he mentioned that if we fix
the ftrace graph problem with direct functions, he'd be open to
add batch interface for direct ftrace functions.
He already had prove of concept fix for that, which I took and broke
up into several changes. I added the ftrace direct batch interface
and bpf new interface on top of that.
It's not so many patches after all, so I thought having them all
together will help the review, because they are all connected.
However I can break this up into separate patchsets if necessary.
This patchset contains:
1) patches (1-4) that fix the ftrace graph tracing over the function
with direct trampolines attached
2) patches (5-8) that add batch interface for ftrace direct function
register/unregister/modify
3) patches (9-19) that add support to attach BPF program to multiple
functions
In nutshell:
Ad 1) moves the graph tracing setup before the direct trampoline
prepares the stack, so they don't clash
Ad 2) uses ftrace_ops interface to register direct function with
all functions in ftrace_ops filter.
Ad 3) creates special program and trampoline type to allow attachment
of multiple functions to single program.
There're more detailed desriptions in related changelogs.
I have working bpftrace multi attachment code on top this. I briefly
checked retsnoop and I think it could use the new API as well.
Ok, so I had a bit of time and enthusiasm to try that with retsnoop.
The ugly code is at [0] if you'd like to see what kind of changes I
needed to make to use this (it won't work if you check it out because
it needs your libbpf changes synced into submodule, which I only did
locally). But here are some learnings from that experiment both to
emphasize how important it is to make this work and how restrictive
are some of the current limitations.
First, good news. Using this mass-attach API to attach to almost 1000
kernel functions goes from
Plain fentry/fexit:
===================
real 0m27.321s
user 0m0.352s
sys 0m20.919s
to
Mass-attach fentry/fexit:
=========================
real 0m2.728s
user 0m0.329s
sys 0m2.380s
I did not meassured the bpftrace speedup, because the new code
attached instantly ;-)
quoted
It's a 10x speed up. And a good chunk of those 2.7 seconds is in some
preparatory steps not related to fentry/fexit stuff.
It's not exactly apples-to-apples, though, because the limitations you
have right now prevents attaching both fentry and fexit programs to
the same set of kernel functions. This makes it pretty useless for a
hum, you could do link_update with fexit program on the link fd,
like in the selftest, right?
quoted
lot of cases, in particular for retsnoop. So I haven't really tested
retsnoop end-to-end, I only verified that I do see fentries triggered,
but can't have matching fexits. So the speed-up might be smaller due
to additional fexit mass-attach (once that is allowed), but it's still
a massive difference. So we absolutely need to get this optimization
in.
Few more thoughts, if you'd like to plan some more work ahead ;)
1. We need similar mass-attach functionality for kprobe/kretprobe, as
there are use cases where kprobe are more useful than fentry (e.g., >6
args funcs, or funcs with input arguments that are not supported by
BPF verifier, like struct-by-value). It's not clear how to best
represent this, given currently we attach kprobe through perf_event,
but we'll need to think about this for sure.
I'm fighting with the '2 trampolines concept' at the moment, but the
mass attach for kprobes seems interesting ;-) will check
quoted
2. To make mass-attach fentry/fexit useful for practical purposes, it
would be really great to have an ability to fetch traced function's
IP. I.e., if we fentry/fexit func kern_func_abc, bpf_get_func_ip()
would return IP of that functions that matches the one in
/proc/kallsyms. Right now I do very brittle hacks to do that.
so I hoped that we could store ip always in ctx-8 and have
the bpf_get_func_ip helper to access that, but the BPF_PROG
macro does not pass ctx value to the program, just args
ctx does pass to the bpf program. You can check BPF_PROG
macro definition.
ah right, should have checked it.. so how about we change
trampoline code to store ip in ctx-8 and make bpf_get_func_ip(ctx)
to return [ctx-8]
I'll need to check if it's ok for the tracing helper to take
ctx as argument
thanks,
jirka
From: Yonghong Song <hidden> Date: 2021-06-20 16:57:14
On 6/19/21 10:09 AM, Jiri Olsa wrote:
On Sat, Jun 19, 2021 at 09:19:57AM -0700, Yonghong Song wrote:
quoted
On 6/19/21 1:33 AM, Jiri Olsa wrote:
quoted
On Thu, Jun 17, 2021 at 01:29:45PM -0700, Andrii Nakryiko wrote:
quoted
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
hi,
saga continues.. ;-) previous post is in here [1]
After another discussion with Steven, he mentioned that if we fix
the ftrace graph problem with direct functions, he'd be open to
add batch interface for direct ftrace functions.
He already had prove of concept fix for that, which I took and broke
up into several changes. I added the ftrace direct batch interface
and bpf new interface on top of that.
It's not so many patches after all, so I thought having them all
together will help the review, because they are all connected.
However I can break this up into separate patchsets if necessary.
This patchset contains:
1) patches (1-4) that fix the ftrace graph tracing over the function
with direct trampolines attached
2) patches (5-8) that add batch interface for ftrace direct function
register/unregister/modify
3) patches (9-19) that add support to attach BPF program to multiple
functions
In nutshell:
Ad 1) moves the graph tracing setup before the direct trampoline
prepares the stack, so they don't clash
Ad 2) uses ftrace_ops interface to register direct function with
all functions in ftrace_ops filter.
Ad 3) creates special program and trampoline type to allow attachment
of multiple functions to single program.
There're more detailed desriptions in related changelogs.
I have working bpftrace multi attachment code on top this. I briefly
checked retsnoop and I think it could use the new API as well.
Ok, so I had a bit of time and enthusiasm to try that with retsnoop.
The ugly code is at [0] if you'd like to see what kind of changes I
needed to make to use this (it won't work if you check it out because
it needs your libbpf changes synced into submodule, which I only did
locally). But here are some learnings from that experiment both to
emphasize how important it is to make this work and how restrictive
are some of the current limitations.
First, good news. Using this mass-attach API to attach to almost 1000
kernel functions goes from
Plain fentry/fexit:
===================
real 0m27.321s
user 0m0.352s
sys 0m20.919s
to
Mass-attach fentry/fexit:
=========================
real 0m2.728s
user 0m0.329s
sys 0m2.380s
I did not meassured the bpftrace speedup, because the new code
attached instantly ;-)
quoted
It's a 10x speed up. And a good chunk of those 2.7 seconds is in some
preparatory steps not related to fentry/fexit stuff.
It's not exactly apples-to-apples, though, because the limitations you
have right now prevents attaching both fentry and fexit programs to
the same set of kernel functions. This makes it pretty useless for a
hum, you could do link_update with fexit program on the link fd,
like in the selftest, right?
quoted
lot of cases, in particular for retsnoop. So I haven't really tested
retsnoop end-to-end, I only verified that I do see fentries triggered,
but can't have matching fexits. So the speed-up might be smaller due
to additional fexit mass-attach (once that is allowed), but it's still
a massive difference. So we absolutely need to get this optimization
in.
Few more thoughts, if you'd like to plan some more work ahead ;)
1. We need similar mass-attach functionality for kprobe/kretprobe, as
there are use cases where kprobe are more useful than fentry (e.g., >6
args funcs, or funcs with input arguments that are not supported by
BPF verifier, like struct-by-value). It's not clear how to best
represent this, given currently we attach kprobe through perf_event,
but we'll need to think about this for sure.
I'm fighting with the '2 trampolines concept' at the moment, but the
mass attach for kprobes seems interesting ;-) will check
quoted
2. To make mass-attach fentry/fexit useful for practical purposes, it
would be really great to have an ability to fetch traced function's
IP. I.e., if we fentry/fexit func kern_func_abc, bpf_get_func_ip()
would return IP of that functions that matches the one in
/proc/kallsyms. Right now I do very brittle hacks to do that.
so I hoped that we could store ip always in ctx-8 and have
the bpf_get_func_ip helper to access that, but the BPF_PROG
macro does not pass ctx value to the program, just args
ctx does pass to the bpf program. You can check BPF_PROG
macro definition.
ah right, should have checked it.. so how about we change
trampoline code to store ip in ctx-8 and make bpf_get_func_ip(ctx)
to return [ctx-8]
This should work. Thanks!
I'll need to check if it's ok for the tracing helper to take
ctx as argument
thanks,
jirka
On Sun, Jun 20, 2021 at 8:47 PM Alexei Starovoitov
[off-list ref] wrote:
On Sun, Jun 20, 2021 at 9:57 AM Yonghong Song [off-list ref] wrote:
quoted
quoted
ah right, should have checked it.. so how about we change
trampoline code to store ip in ctx-8 and make bpf_get_func_ip(ctx)
to return [ctx-8]
This should work. Thanks!
+1
and pls make it always inline into single LDX insn in the verifier.
For both mass attach and normal fentry/fexit.
Yep.
And we should do it for kprobes (trivial, PT_REGS_IP(ctx)) and
kretprobe (less trivial but simple from inside the kernel, Masami
showed how to do it in one of the previous emails). I hope BPF infra
allows inlining of helpers for some program types but not the others.
On Sat, Jun 19, 2021 at 11:33 AM Jiri Olsa [off-list ref] wrote:
On Thu, Jun 17, 2021 at 01:29:45PM -0700, Andrii Nakryiko wrote:
quoted
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
hi,
saga continues.. ;-) previous post is in here [1]
After another discussion with Steven, he mentioned that if we fix
the ftrace graph problem with direct functions, he'd be open to
add batch interface for direct ftrace functions.
He already had prove of concept fix for that, which I took and broke
up into several changes. I added the ftrace direct batch interface
and bpf new interface on top of that.
It's not so many patches after all, so I thought having them all
together will help the review, because they are all connected.
However I can break this up into separate patchsets if necessary.
This patchset contains:
1) patches (1-4) that fix the ftrace graph tracing over the function
with direct trampolines attached
2) patches (5-8) that add batch interface for ftrace direct function
register/unregister/modify
3) patches (9-19) that add support to attach BPF program to multiple
functions
In nutshell:
Ad 1) moves the graph tracing setup before the direct trampoline
prepares the stack, so they don't clash
Ad 2) uses ftrace_ops interface to register direct function with
all functions in ftrace_ops filter.
Ad 3) creates special program and trampoline type to allow attachment
of multiple functions to single program.
There're more detailed desriptions in related changelogs.
I have working bpftrace multi attachment code on top this. I briefly
checked retsnoop and I think it could use the new API as well.
Ok, so I had a bit of time and enthusiasm to try that with retsnoop.
The ugly code is at [0] if you'd like to see what kind of changes I
needed to make to use this (it won't work if you check it out because
it needs your libbpf changes synced into submodule, which I only did
locally). But here are some learnings from that experiment both to
emphasize how important it is to make this work and how restrictive
are some of the current limitations.
First, good news. Using this mass-attach API to attach to almost 1000
kernel functions goes from
Plain fentry/fexit:
===================
real 0m27.321s
user 0m0.352s
sys 0m20.919s
to
Mass-attach fentry/fexit:
=========================
real 0m2.728s
user 0m0.329s
sys 0m2.380s
I did not meassured the bpftrace speedup, because the new code
attached instantly ;-)
quoted
It's a 10x speed up. And a good chunk of those 2.7 seconds is in some
preparatory steps not related to fentry/fexit stuff.
It's not exactly apples-to-apples, though, because the limitations you
have right now prevents attaching both fentry and fexit programs to
the same set of kernel functions. This makes it pretty useless for a
hum, you could do link_update with fexit program on the link fd,
like in the selftest, right?
Hm... I didn't realize we can attach two different prog FDs to the
same link, honestly (and was too lazy to look through selftests
again). I can try that later. But it's actually quite a
counter-intuitive API (I honestly assumed that link_update can be used
to add more BTF IDs, but not change prog_fd). Previously bpf_link was
always associated with single BPF prog FD. It would be good to keep
that property in the final version, but we can get back to that later.
quoted
lot of cases, in particular for retsnoop. So I haven't really tested
retsnoop end-to-end, I only verified that I do see fentries triggered,
but can't have matching fexits. So the speed-up might be smaller due
to additional fexit mass-attach (once that is allowed), but it's still
a massive difference. So we absolutely need to get this optimization
in.
Few more thoughts, if you'd like to plan some more work ahead ;)
1. We need similar mass-attach functionality for kprobe/kretprobe, as
there are use cases where kprobe are more useful than fentry (e.g., >6
args funcs, or funcs with input arguments that are not supported by
BPF verifier, like struct-by-value). It's not clear how to best
represent this, given currently we attach kprobe through perf_event,
but we'll need to think about this for sure.
I'm fighting with the '2 trampolines concept' at the moment, but the
mass attach for kprobes seems interesting ;-) will check
quoted
2. To make mass-attach fentry/fexit useful for practical purposes, it
would be really great to have an ability to fetch traced function's
IP. I.e., if we fentry/fexit func kern_func_abc, bpf_get_func_ip()
would return IP of that functions that matches the one in
/proc/kallsyms. Right now I do very brittle hacks to do that.
so I hoped that we could store ip always in ctx-8 and have
the bpf_get_func_ip helper to access that, but the BPF_PROG
macro does not pass ctx value to the program, just args
we could perhaps somehow store the ctx in BPF_PROG before calling
the bpf program, but I did not get to try that yet
On Sun, Jun 20, 2021 at 11:50 PM Andrii Nakryiko
[off-list ref] wrote:
On Sat, Jun 19, 2021 at 11:33 AM Jiri Olsa [off-list ref] wrote:
quoted
On Thu, Jun 17, 2021 at 01:29:45PM -0700, Andrii Nakryiko wrote:
quoted
On Sat, Jun 5, 2021 at 4:12 AM Jiri Olsa [off-list ref] wrote:
quoted
hi,
saga continues.. ;-) previous post is in here [1]
After another discussion with Steven, he mentioned that if we fix
the ftrace graph problem with direct functions, he'd be open to
add batch interface for direct ftrace functions.
He already had prove of concept fix for that, which I took and broke
up into several changes. I added the ftrace direct batch interface
and bpf new interface on top of that.
It's not so many patches after all, so I thought having them all
together will help the review, because they are all connected.
However I can break this up into separate patchsets if necessary.
This patchset contains:
1) patches (1-4) that fix the ftrace graph tracing over the function
with direct trampolines attached
2) patches (5-8) that add batch interface for ftrace direct function
register/unregister/modify
3) patches (9-19) that add support to attach BPF program to multiple
functions
In nutshell:
Ad 1) moves the graph tracing setup before the direct trampoline
prepares the stack, so they don't clash
Ad 2) uses ftrace_ops interface to register direct function with
all functions in ftrace_ops filter.
Ad 3) creates special program and trampoline type to allow attachment
of multiple functions to single program.
There're more detailed desriptions in related changelogs.
I have working bpftrace multi attachment code on top this. I briefly
checked retsnoop and I think it could use the new API as well.
Ok, so I had a bit of time and enthusiasm to try that with retsnoop.
The ugly code is at [0] if you'd like to see what kind of changes I
needed to make to use this (it won't work if you check it out because
it needs your libbpf changes synced into submodule, which I only did
locally). But here are some learnings from that experiment both to
emphasize how important it is to make this work and how restrictive
are some of the current limitations.
First, good news. Using this mass-attach API to attach to almost 1000
kernel functions goes from
Plain fentry/fexit:
===================
real 0m27.321s
user 0m0.352s
sys 0m20.919s
to
Mass-attach fentry/fexit:
=========================
real 0m2.728s
user 0m0.329s
sys 0m2.380s
I did not meassured the bpftrace speedup, because the new code
attached instantly ;-)
quoted
It's a 10x speed up. And a good chunk of those 2.7 seconds is in some
preparatory steps not related to fentry/fexit stuff.
It's not exactly apples-to-apples, though, because the limitations you
have right now prevents attaching both fentry and fexit programs to
the same set of kernel functions. This makes it pretty useless for a
hum, you could do link_update with fexit program on the link fd,
like in the selftest, right?
Hm... I didn't realize we can attach two different prog FDs to the
same link, honestly (and was too lazy to look through selftests
again). I can try that later. But it's actually quite a
counter-intuitive API (I honestly assumed that link_update can be used
to add more BTF IDs, but not change prog_fd). Previously bpf_link was
always associated with single BPF prog FD. It would be good to keep
that property in the final version, but we can get back to that later.
Ok, I'm back from PTO and as a warm-up did a two-line change to make
retsnoop work end-to-end using this bpf_link_update() approach. See
[0]. I still think it's a completely confusing API to do
bpf_link_update() to have both fexit and fentry, but it worked for
this experiment.
BTW, adding ~900 fexit attachments is barely noticeable, which is
great, means that attachment is instantaneous.
real 0m2.739s
user 0m0.351s
sys 0m2.370s
[0] https://github.com/anakryiko/retsnoop/commit/c915d729d6e98f83601e432e61cb1bdf476ceefb
quoted
quoted
lot of cases, in particular for retsnoop. So I haven't really tested
retsnoop end-to-end, I only verified that I do see fentries triggered,
but can't have matching fexits. So the speed-up might be smaller due
to additional fexit mass-attach (once that is allowed), but it's still
a massive difference. So we absolutely need to get this optimization
in.
Few more thoughts, if you'd like to plan some more work ahead ;)
1. We need similar mass-attach functionality for kprobe/kretprobe, as
there are use cases where kprobe are more useful than fentry (e.g., >6
args funcs, or funcs with input arguments that are not supported by
BPF verifier, like struct-by-value). It's not clear how to best
represent this, given currently we attach kprobe through perf_event,
but we'll need to think about this for sure.
I'm fighting with the '2 trampolines concept' at the moment, but the
mass attach for kprobes seems interesting ;-) will check
quoted
2. To make mass-attach fentry/fexit useful for practical purposes, it
would be really great to have an ability to fetch traced function's
IP. I.e., if we fentry/fexit func kern_func_abc, bpf_get_func_ip()
would return IP of that functions that matches the one in
/proc/kallsyms. Right now I do very brittle hacks to do that.
so I hoped that we could store ip always in ctx-8 and have
the bpf_get_func_ip helper to access that, but the BPF_PROG
macro does not pass ctx value to the program, just args
we could perhaps somehow store the ctx in BPF_PROG before calling
the bpf program, but I did not get to try that yet