This series adds support attaching freplace BPF programs to multiple targets.
This is needed to support incremental attachment of multiple XDP programs using
the libxdp dispatcher model.
The first patch fixes an issue that came up in review: The verifier will
currently allow MODIFY_RETURN tracing functions to attach to other BPF programs,
even though it is pretty clear from the commit messages introducing the
functionality that this was not the intention. This patch is included in the
series because the subsequent refactoring patches touch the same code.
The next three patches are refactoring patches: Patch 2 is a trivial change to
the logging in the verifier, split out to make the subsequent refactor easier to
read. Patch 3 refactors check_attach_btf_id() so that the checks on program and
target compatibility can be reused when attaching to a secondary location.
Patch 4 moves prog_aux->linked_prog and the trampoline to be embedded in
bpf_tracing_link on attach, and freed by the link release logic, and introduces
a mutex to protect the writing of the pointers in prog->aux.
Based on these refactorings, it becomes pretty straight-forward to support
multiple-attach for freplace programs (patch 5). This is simply a matter of
creating a second bpf_tracing_link if a target is supplied. However, for API
consistency with other types of link attach, this option is added to the
BPF_LINK_CREATE API instead of extending bpf_raw_tracepoint_open().
Patch 6 is a port of Jiri Olsa's patch to support fentry/fexit on freplace
programs. His approach of getting the target type from the target program
reference no longer works after we've gotten rid of linked_prog (because the
bpf_tracing_link reference disappears on attach). Instead, we used the saved
reference to the target prog type that is also used to verify compatibility on
secondary freplace attachment.
Patches 7 is the accompanying libbpf update, and patches 8-11 are selftests:
patch 8 tests for the multi-freplace functionality itself; patch 9 is Jiri's
previous selftest for the fentry-to-freplace fix; patch 10 is a test for the
change introduced in patch 1, blocking MODIFY_RETURN functions from attaching to
other BPF programs; and finally, patch 11 removes MODIFY_RETURN functions from
the benchmark and test_overhead programs in selftests, as these were never
supposed to work in the first place.
With this series, libxdp and xdp-tools can successfully attach multiple programs
one at a time. To play with this, use the 'freplace-multi-attach' branch of
xdp-tools:
$ git clone --recurse-submodules --branch freplace-multi-attach https://github.com/xdp-project/xdp-tools
$ cd xdp-tools/xdp-loader
$ make
$ sudo ./xdp-loader load veth0 ../lib/testing/xdp_drop.o
$ sudo ./xdp-loader load veth0 ../lib/testing/xdp_pass.o
$ sudo ./xdp-loader status
The series is also available here:
https://git.kernel.org/pub/scm/linux/kernel/git/toke/linux.git/log/?h=bpf-freplace-multi-attach-alt-08
Changelog:
v8:
- Add a separate error message when trying to attach FMOD_REPLACE to tgt_prog
- Better error messages in bpf_program__attach_freplace()
- Don't lock mutex when setting tgt_* pointers in prog create and verifier
- Remove fmod_ret programs from benchmarks in selftests (new patch 11)
- Fix a few other nits in selftests
v7:
- Add back missing ptype == prog->type check in link_create()
- Use tracing_bpf_link_attach() instead of separate freplace_bpf_link_attach()
- Don't break attachment of bpf_iters in libbpf (by clobbering link_create.iter_info)
v6:
- Rebase to latest bpf-next
- Simplify logic in bpf_tracing_prog_attach()
- Don't create a new attach_type for link_create(), disambiguate on prog->type
instead
- Use raw_tracepoint_open() in libbpf bpf_program__attach_ftrace() if called
with NULL target
- Switch bpf_program__attach_ftrace() to take function name as parameter
instead of btf_id
- Add a patch disallowing MODIFY_RETURN programs from attaching to other BPF
programs, and an accompanying selftest (patches 1 and 10)
v5:
- Fix typo in inline function definition of bpf_trampoline_get()
- Don't put bpf_tracing_link in prog->aux, use a mutex to protect tgt_prog and
trampoline instead, and move them to the link on attach.
- Restore Jiri as author of the last selftest patch
v4:
- Cleanup the refactored check_attach_btf_id() to make the logic easier to follow
- Fix cleanup paths for bpf_tracing_link
- Use xchg() for removing the bpf_tracing_link from prog->aux and restore on (some) failures
- Use BPF_LINK_CREATE operation to create link with target instead of extending raw_tracepoint_open
- Fold update of tools/ UAPI header into main patch
- Update arg dereference patch to use skeletons and set_attach_target()
v3:
- Get rid of prog_aux->linked_prog entirely in favour of a bpf_tracing_link
- Incorporate Jiri's fix for attaching fentry to freplace programs
v2:
- Drop the log arguments from bpf_raw_tracepoint_open
- Fix kbot errors
- Rebase to latest bpf-next
---
Jiri Olsa (1):
selftests/bpf: Adding test for arg dereference in extension trace
Toke Høiland-Jørgensen (10):
bpf: disallow attaching modify_return tracing functions to other BPF programs
bpf: change logging calls from verbose() to bpf_log() and use log pointer
bpf: verifier: refactor check_attach_btf_id()
bpf: move prog->aux->linked_prog and trampoline into bpf_link on attach
bpf: support attaching freplace programs to multiple attach points
bpf: Fix context type resolving for extension programs
libbpf: add support for freplace attachment in bpf_link_create
selftests: add test for multiple attachments of freplace program
selftests: Add selftest for disallowing modify_return attachment to freplace
selftests: Remove fmod_ret from benchmarks and test_overhead
include/linux/bpf.h | 26 +-
include/linux/bpf_verifier.h | 14 +-
include/uapi/linux/bpf.h | 9 +-
kernel/bpf/btf.c | 21 +-
kernel/bpf/core.c | 9 +-
kernel/bpf/syscall.c | 135 ++++++++--
kernel/bpf/trampoline.c | 32 ++-
kernel/bpf/verifier.c | 250 ++++++++++--------
tools/include/uapi/linux/bpf.h | 9 +-
tools/lib/bpf/bpf.c | 18 +-
tools/lib/bpf/bpf.h | 3 +-
tools/lib/bpf/libbpf.c | 44 ++-
tools/lib/bpf/libbpf.h | 3 +
tools/lib/bpf/libbpf.map | 1 +
tools/testing/selftests/bpf/bench.c | 5 -
.../selftests/bpf/benchs/bench_rename.c | 17 --
.../selftests/bpf/benchs/bench_trigger.c | 17 --
.../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 212 ++++++++++++---
.../selftests/bpf/prog_tests/test_overhead.c | 14 +-
.../selftests/bpf/prog_tests/trace_ext.c | 111 ++++++++
.../selftests/bpf/progs/fmod_ret_freplace.c | 14 +
.../bpf/progs/freplace_get_constant.c | 15 ++
.../selftests/bpf/progs/test_overhead.c | 6 -
.../selftests/bpf/progs/test_trace_ext.c | 18 ++
.../bpf/progs/test_trace_ext_tracing.c | 25 ++
.../selftests/bpf/progs/trigger_bench.c | 7 -
26 files changed, 771 insertions(+), 264 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
create mode 100644 tools/testing/selftests/bpf/progs/fmod_ret_freplace.c
create mode 100644 tools/testing/selftests/bpf/progs/freplace_get_constant.c
create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
From: Toke Høiland-Jørgensen <redacted>
The check_attach_btf_id() function really does three things:
1. It performs a bunch of checks on the program to ensure that the
attachment is valid.
2. It stores a bunch of state about the attachment being requested in
the verifier environment and struct bpf_prog objects.
3. It allocates a trampoline for the attachment.
This patch splits out (1.) and (3.) into separate functions in preparation
for reusing them when the actual attachment is happening (in the
raw_tracepoint_open syscall operation), which will allow tracing programs
to have multiple (compatible) attachments.
This also fixes a bug where a bunch of checks were skipped if a trampoline
already existed for the tracing target.
Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN")
Fixes: 1e6c62a88215 ("bpf: Introduce sleepable BPF programs")
Acked-by: Andrii Nakryiko <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/linux/bpf.h | 7 +
include/linux/bpf_verifier.h | 9 ++
kernel/bpf/trampoline.c | 20 ++++
kernel/bpf/verifier.c | 200 ++++++++++++++++++++++++------------------
4 files changed, 150 insertions(+), 86 deletions(-)
@@ -11215,43 +11215,29 @@ static int check_non_sleepable_error_inject(u32 btf_id)returnbtf_id_set_contains(&btf_non_sleepable_error_inject,btf_id);}-staticintcheck_attach_btf_id(structbpf_verifier_env*env)+intbpf_check_attach_target(structbpf_verifier_log*log,+conststructbpf_prog*prog,+conststructbpf_prog*tgt_prog,+u32btf_id,+structbtf_func_model*fmodel,+long*tgt_addr,+constchar**tgt_name,+conststructbtf_type**tgt_type){-structbpf_prog*prog=env->prog;boolprog_extension=prog->type==BPF_PROG_TYPE_EXT;-structbpf_prog*tgt_prog=prog->aux->linked_prog;-structbpf_verifier_log*log=&env->log;-u32btf_id=prog->aux->attach_btf_id;constcharprefix[]="btf_trace_";-structbtf_func_modelfmodel;intret=0,subprog=-1,i;-structbpf_trampoline*tr;conststructbtf_type*t;boolconservative=true;constchar*tname;structbtf*btf;-longaddr;-u64key;--if(prog->aux->sleepable&&prog->type!=BPF_PROG_TYPE_TRACING&&-prog->type!=BPF_PROG_TYPE_LSM){-verbose(env,"Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");-return-EINVAL;-}--if(prog->type==BPF_PROG_TYPE_STRUCT_OPS)-returncheck_struct_ops_btf_id(env);--if(prog->type!=BPF_PROG_TYPE_TRACING&&-prog->type!=BPF_PROG_TYPE_LSM&&-!prog_extension)-return0;+longaddr=0;if(!btf_id){bpf_log(log,"Tracing programs must provide btf_id\n");return-EINVAL;}-btf=bpf_prog_get_target_btf(prog);+btf=tgt_prog?tgt_prog->aux->btf:btf_vmlinux;if(!btf){bpf_log(log,"FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");
@@ -11291,8 +11277,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)"Extension programs should be JITed\n");return-EINVAL;}-env->ops=bpf_verifier_ops[tgt_prog->type];-prog->expected_attach_type=tgt_prog->expected_attach_type;}if(!tgt_prog->jited){bpf_log(log,"Can attach to only JITed progs\n");
@@ -11364,13 +11346,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)/* should never happen in valid vmlinux build */return-EINVAL;-/* remember two read only pointers that are valid for-*thelifetimeofthekernel-*/-prog->aux->attach_func_name=tname;-prog->aux->attach_func_proto=t;-prog->aux->attach_btf_trace=true;-return0;+break;caseBPF_TRACE_ITER:if(!btf_type_is_func(t)){bpf_log(log,"attach_btf_id %u is not a function\n",
@@ -11380,12 +11356,10 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)t=btf_type_by_id(btf,t->type);if(!btf_type_is_func_proto(t))return-EINVAL;-prog->aux->attach_func_name=tname;-prog->aux->attach_func_proto=t;-if(!bpf_iter_prog_supported(prog))-return-EINVAL;-ret=btf_distill_func_proto(log,btf,t,tname,&fmodel);-returnret;+ret=btf_distill_func_proto(log,btf,t,tname,fmodel);+if(ret)+returnret;+break;default:if(!prog_extension)return-EINVAL;
@@ -11394,13 +11368,6 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)caseBPF_LSM_MAC:caseBPF_TRACE_FENTRY:caseBPF_TRACE_FEXIT:-prog->aux->attach_func_name=tname;-if(prog->type==BPF_PROG_TYPE_LSM){-ret=bpf_lsm_verify_prog(log,prog);-if(ret<0)-returnret;-}-if(!btf_type_is_func(t)){bpf_log(log,"attach_btf_id %u is not a function\n",btf_id);
@@ -11412,24 +11379,14 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)t=btf_type_by_id(btf,t->type);if(!btf_type_is_func_proto(t))return-EINVAL;-tr=bpf_trampoline_lookup(key);-if(!tr)-return-ENOMEM;-/* t is either vmlinux type or another program's type */-prog->aux->attach_func_proto=t;-mutex_lock(&tr->mutex);-if(tr->func.addr){-prog->aux->trampoline=tr;-gotoout;-}-if(tgt_prog&&conservative){-prog->aux->attach_func_proto=NULL;++if(tgt_prog&&conservative)t=NULL;-}-ret=btf_distill_func_proto(log,btf,t,-tname,&tr->func.model);++ret=btf_distill_func_proto(log,btf,t,tname,fmodel);if(ret<0)-gotoout;+returnret;+if(tgt_prog){if(subprog==0)addr=(long)tgt_prog->bpf_func;
@@ -11441,8 +11398,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)bpf_log(log,"The address of function %s cannot be found\n",tname);-ret=-ENOENT;-gotoout;+return-ENOENT;}}
@@ -11467,30 +11423,102 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)default:break;}-if(ret)-bpf_log(log,"%s is not sleepable\n",-prog->aux->attach_func_name);+if(ret){+bpf_log(log,"%s is not sleepable\n",tname);+returnret;+}}elseif(prog->expected_attach_type==BPF_MODIFY_RETURN){if(tgt_prog){bpf_log(log,"can't modify return codes of BPF programs\n");-ret=-EINVAL;-gotoout;+return-EINVAL;+}+ret=check_attach_modify_return(prog,addr,tname);+if(ret){+bpf_log(log,"%s() is not modifiable\n",tname);+returnret;}-ret=check_attach_modify_return(prog,addr);-if(ret)-bpf_log(log,"%s() is not modifiable\n",-prog->aux->attach_func_name);}-if(ret)-gotoout;-tr->func.addr=(void*)addr;-prog->aux->trampoline=tr;-out:-mutex_unlock(&tr->mutex);-if(ret)-bpf_trampoline_put(tr);++break;+}+*tgt_addr=addr;+if(tgt_name)+*tgt_name=tname;+if(tgt_type)+*tgt_type=t;+return0;+}++staticintcheck_attach_btf_id(structbpf_verifier_env*env)+{+structbpf_prog*prog=env->prog;+structbpf_prog*tgt_prog=prog->aux->linked_prog;+u32btf_id=prog->aux->attach_btf_id;+structbtf_func_modelfmodel;+structbpf_trampoline*tr;+conststructbtf_type*t;+constchar*tname;+longaddr;+intret;+u64key;++if(prog->aux->sleepable&&prog->type!=BPF_PROG_TYPE_TRACING&&+prog->type!=BPF_PROG_TYPE_LSM){+verbose(env,"Only fentry/fexit/fmod_ret and lsm programs can be sleepable\n");+return-EINVAL;+}++if(prog->type==BPF_PROG_TYPE_STRUCT_OPS)+returncheck_struct_ops_btf_id(env);++if(prog->type!=BPF_PROG_TYPE_TRACING&&+prog->type!=BPF_PROG_TYPE_LSM&&+prog->type!=BPF_PROG_TYPE_EXT)+return0;++ret=bpf_check_attach_target(&env->log,prog,tgt_prog,btf_id,+&fmodel,&addr,&tname,&t);+if(ret)returnret;++if(tgt_prog){+if(prog->type==BPF_PROG_TYPE_EXT){+env->ops=bpf_verifier_ops[tgt_prog->type];+prog->expected_attach_type=+tgt_prog->expected_attach_type;+}+key=((u64)tgt_prog->aux->id)<<32|btf_id;+}else{+key=btf_id;}++/* remember two read only pointers that are valid for+*thelifetimeofthekernel+*/+prog->aux->attach_func_proto=t;+prog->aux->attach_func_name=tname;++if(prog->expected_attach_type==BPF_TRACE_RAW_TP){+prog->aux->attach_btf_trace=true;+return0;+}elseif(prog->expected_attach_type==BPF_TRACE_ITER){+if(!bpf_iter_prog_supported(prog))+return-EINVAL;+return0;+}++if(prog->type==BPF_PROG_TYPE_LSM){+ret=bpf_lsm_verify_prog(&env->log,prog);+if(ret<0)+returnret;+}++tr=bpf_trampoline_get(key,(void*)addr,&fmodel);+if(!tr)+return-ENOMEM;++prog->aux->trampoline=tr;+return0;}intbpf_check(structbpf_prog**prog,unionbpf_attr*attr,
From: Toke Høiland-Jørgensen <redacted>
Eelco reported we can't properly access arguments if the tracing
program is attached to extension program.
Having following program:
SEC("classifier/test_pkt_md_access")
int test_pkt_md_access(struct __sk_buff *skb)
with its extension:
SEC("freplace/test_pkt_md_access")
int test_pkt_md_access_new(struct __sk_buff *skb)
and tracing that extension with:
SEC("fentry/test_pkt_md_access_new")
int BPF_PROG(fentry, struct sk_buff *skb)
It's not possible to access skb argument in the fentry program,
with following error from verifier:
; int BPF_PROG(fentry, struct sk_buff *skb)
0: (79) r1 = *(u64 *)(r1 +0)
invalid bpf_context access off=0 size=8
The problem is that btf_ctx_access gets the context type for the
traced program, which is in this case the extension.
But when we trace extension program, we want to get the context
type of the program that the extension is attached to, so we can
access the argument properly in the trace program.
This version of the patch is tweaked slightly from Jiri's original one,
since the refactoring in the previous patches means we have to get the
target prog type from the new variable in prog->aux instead of directly
from the target prog.
Reported-by: Eelco Chaudron <echaudro@redhat.com>
Suggested-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Andrii Nakryiko <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
kernel/bpf/btf.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: Toke Høiland-Jørgensen <redacted>
This enables support for attaching freplace programs to multiple attach
points. It does this by amending the UAPI for bpf_link_Create with a target
btf ID that can be used to supply the new attachment point along with the
target program fd. The target must be compatible with the target that was
supplied at program load time.
The implementation reuses the checks that were factored out of
check_attach_btf_id() to ensure compatibility between the BTF types of the
old and new attachment. If these match, a new bpf_tracing_link will be
created for the new attach target, allowing multiple attachments to
co-exist simultaneously.
The code could theoretically support multiple-attach of other types of
tracing programs as well, but since I don't have a use case for any of
those, there is no API support for doing so.
Acked-by: Andrii Nakryiko <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/linux/bpf.h | 2 +
include/uapi/linux/bpf.h | 9 +++-
kernel/bpf/syscall.c | 102 +++++++++++++++++++++++++++++++++-------
kernel/bpf/verifier.c | 9 ++++
tools/include/uapi/linux/bpf.h | 9 +++-
5 files changed, 108 insertions(+), 23 deletions(-)
@@ -751,6 +751,8 @@ struct bpf_prog_aux {structmutextgt_mutex;/* protects tgt_* pointers below, *after* prog becomes visible */structbpf_prog*tgt_prog;structbpf_trampoline*tgt_trampoline;+enumbpf_prog_typetgt_prog_type;+enumbpf_attach_typetgt_attach_type;boolverifier_zext;/* Zero extensions has been inserted by verifier. */booloffload_requested;boolattach_btf_trace;/* true if attaching to BTF-enabled raw tp */
@@ -632,8 +632,13 @@ union bpf_attr {};__u32attach_type;/* attach type */__u32flags;/* extra flags */-__aligned_u64iter_info;/* extra bpf_iter_link_info */-__u32iter_info_len;/* iter_info length */+union{+__u32target_btf_id;/* btf_id of target to attach to */+struct{+__aligned_u64iter_info;/* extra bpf_iter_link_info */+__u32iter_info_len;/* iter_info length */+};+};}link_create;struct{/* struct used by BPF_LINK_UPDATE command */
@@ -2583,6 +2589,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)gotoout_put_prog;}+if(!!tgt_prog_fd!=!!btf_id){+err=-EINVAL;+gotoout_put_prog;+}++if(tgt_prog_fd){+/* For now we only allow new targets for BPF_PROG_TYPE_EXT */+if(prog->type!=BPF_PROG_TYPE_EXT){+err=-EINVAL;+gotoout_put_prog;+}++tgt_prog=bpf_prog_get(tgt_prog_fd);+if(IS_ERR(tgt_prog)){+err=PTR_ERR(tgt_prog);+tgt_prog=NULL;+gotoout_put_prog;+}++key=((u64)tgt_prog->aux->id)<<32|btf_id;+}+link=kzalloc(sizeof(*link),GFP_USER);if(!link){err=-ENOMEM;
@@ -2594,12 +2622,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)mutex_lock(&prog->aux->tgt_mutex);-if(!prog->aux->tgt_trampoline){+if(!prog->aux->tgt_trampoline&&!tgt_prog){err=-ENOENT;gotoout_unlock;}-tr=prog->aux->tgt_trampoline;-tgt_prog=prog->aux->tgt_prog;++if(!prog->aux->tgt_trampoline||+(key&&key!=prog->aux->tgt_trampoline->key)){++err=bpf_check_attach_target(NULL,prog,tgt_prog,btf_id,+&fmodel,&addr,NULL,NULL);+if(err)+gotoout_unlock;++tr=bpf_trampoline_get(key,(void*)addr,&fmodel);+if(!tr){+err=-ENOMEM;+gotoout_unlock;+}+}else{+tr=prog->aux->tgt_trampoline;+tgt_prog=prog->aux->tgt_prog;+}err=bpf_link_prime(&link->link,&link_primer);if(err)
@@ -2614,16 +2658,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog)link->tgt_prog=tgt_prog;link->trampoline=tr;--prog->aux->tgt_prog=NULL;-prog->aux->tgt_trampoline=NULL;+if(tr==prog->aux->tgt_trampoline){+/* if we got a new ref from syscall, drop existing one from prog */+if(tgt_prog_fd)+bpf_prog_put(prog->aux->tgt_prog);+prog->aux->tgt_trampoline=NULL;+prog->aux->tgt_prog=NULL;+}mutex_unlock(&prog->aux->tgt_mutex);returnbpf_link_settle(&link_primer);out_unlock:+if(tr&&tr!=prog->aux->tgt_trampoline)+bpf_trampoline_put(tr);mutex_unlock(&prog->aux->tgt_mutex);kfree(link);out_put_prog:+if(tgt_prog_fd&&tgt_prog)+bpf_prog_put(tgt_prog);bpf_prog_put(prog);returnerr;}
@@ -2737,7 +2789,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)tp_name=prog->aux->attach_func_name;break;}-returnbpf_tracing_prog_attach(prog);+returnbpf_tracing_prog_attach(prog,0,0);caseBPF_PROG_TYPE_RAW_TRACEPOINT:caseBPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:if(strncpy_from_user(buf,
@@ -3921,10 +3973,15 @@ static int bpf_map_do_batch(const union bpf_attr *attr,staticinttracing_bpf_link_attach(constunionbpf_attr*attr,structbpf_prog*prog){-if(attr->link_create.attach_type==BPF_TRACE_ITER&&-prog->expected_attach_type==BPF_TRACE_ITER)-returnbpf_iter_link_attach(attr,prog);+if(attr->link_create.attach_type!=prog->expected_attach_type)+return-EINVAL;+if(prog->expected_attach_type==BPF_TRACE_ITER)+returnbpf_iter_link_attach(attr,prog);+elseif(prog->type==BPF_PROG_TYPE_EXT)+returnbpf_tracing_prog_attach(prog,+attr->link_create.target_fd,+attr->link_create.target_btf_id);return-EINVAL;}
@@ -3938,18 +3995,25 @@ static int link_create(union bpf_attr *attr)if(CHECK_ATTR(BPF_LINK_CREATE))return-EINVAL;-ptype=attach_type_to_prog_type(attr->link_create.attach_type);-if(ptype==BPF_PROG_TYPE_UNSPEC)-return-EINVAL;--prog=bpf_prog_get_type(attr->link_create.prog_fd,ptype);+prog=bpf_prog_get(attr->link_create.prog_fd);if(IS_ERR(prog))returnPTR_ERR(prog);ret=bpf_prog_attach_check_attach_type(prog,attr->link_create.attach_type);if(ret)-gotoerr_out;+gotoout;++if(prog->type==BPF_PROG_TYPE_EXT){+ret=tracing_bpf_link_attach(attr,prog);+gotoout;+}++ptype=attach_type_to_prog_type(attr->link_create.attach_type);+if(ptype==BPF_PROG_TYPE_UNSPEC||ptype!=prog->type){+ret=-EINVAL;+gotoout;+}switch(ptype){caseBPF_PROG_TYPE_CGROUP_SKB:
@@ -3977,7 +4041,7 @@ static int link_create(union bpf_attr *attr)ret=-EINVAL;}-err_out:+out:if(ret<0)bpf_prog_put(prog);returnret;
@@ -632,8 +632,13 @@ union bpf_attr {};__u32attach_type;/* attach type */__u32flags;/* extra flags */-__aligned_u64iter_info;/* extra bpf_iter_link_info */-__u32iter_info_len;/* iter_info length */+union{+__u32target_btf_id;/* btf_id of target to attach to */+struct{+__aligned_u64iter_info;/* extra bpf_iter_link_info */+__u32iter_info_len;/* iter_info length */+};+};}link_create;struct{/* struct used by BPF_LINK_UPDATE command */
From: Toke Høiland-Jørgensen <redacted>
This adds a selftest that ensures that modify_return tracing programs
cannot be attached to freplace programs. The security_ prefix is added to
the freplace program because that would otherwise let it pass the check for
modify_return.
Acked-by: Andrii Nakryiko <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
.../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 56 ++++++++++++++++++++
.../selftests/bpf/progs/fmod_ret_freplace.c | 14 +++++
.../selftests/bpf/progs/freplace_get_constant.c | 2 -
3 files changed, 71 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/progs/fmod_ret_freplace.c
@@ -232,6 +232,60 @@ static void test_func_replace_multi(void)prog_name,true,test_second_attach);}+staticvoidtest_fmod_ret_freplace(void)+{+structbpf_object*freplace_obj=NULL,*pkt_obj,*fmod_obj=NULL;+constchar*freplace_name="./freplace_get_constant.o";+constchar*fmod_ret_name="./fmod_ret_freplace.o";+DECLARE_LIBBPF_OPTS(bpf_object_open_opts,opts);+constchar*tgt_name="./test_pkt_access.o";+structbpf_link*freplace_link=NULL;+structbpf_program*prog;+__u32duration=0;+interr,pkt_fd;++err=bpf_prog_load(tgt_name,BPF_PROG_TYPE_UNSPEC,+&pkt_obj,&pkt_fd);+/* the target prog should load fine */+if(CHECK(err,"tgt_prog_load","file %s err %d errno %d\n",+tgt_name,err,errno))+return;+opts.attach_prog_fd=pkt_fd;++freplace_obj=bpf_object__open_file(freplace_name,&opts);+if(CHECK(IS_ERR_OR_NULL(freplace_obj),"freplace_obj_open",+"failed to open %s: %ld\n",freplace_name,+PTR_ERR(freplace_obj)))+gotoout;++err=bpf_object__load(freplace_obj);+if(CHECK(err,"freplace_obj_load","err %d\n",err))+gotoout;++prog=bpf_program__next(NULL,freplace_obj);+freplace_link=bpf_program__attach_trace(prog);+if(CHECK(IS_ERR(freplace_link),"freplace_attach_trace","failed to link\n"))+gotoout;++opts.attach_prog_fd=bpf_program__fd(prog);+fmod_obj=bpf_object__open_file(fmod_ret_name,&opts);+if(CHECK(IS_ERR_OR_NULL(fmod_obj),"fmod_obj_open",+"failed to open %s: %ld\n",fmod_ret_name,+PTR_ERR(fmod_obj)))+gotoout;++err=bpf_object__load(fmod_obj);+if(CHECK(!err,"fmod_obj_load","loading fmod_ret should fail\n"))+gotoout;++out:+bpf_link__destroy(freplace_link);+bpf_object__close(freplace_obj);+bpf_object__close(fmod_obj);+bpf_object__close(pkt_obj);+}++staticvoidtest_func_sockmap_update(void){constchar*prog_name[]={
From: Toke Høiland-Jørgensen <redacted>
The benchmark code and the test_overhead prog_test included fmod_ret
programs that attached to various functions in the kernel. However, these
functions were never listed as allowed for return modification, so this
only worked because of the verifier skipping tests when a trampoline
already existed for the attach point. Now that the verifier checks have
been fixed, remove fmod_ret from the affected tests so they all work again.
Fixes: 4eaf0b5c5e04 ("selftest/bpf: Fmod_ret prog and implement test_overhead as part of bench")
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
tools/testing/selftests/bpf/bench.c | 5 -----
tools/testing/selftests/bpf/benchs/bench_rename.c | 17 -----------------
tools/testing/selftests/bpf/benchs/bench_trigger.c | 17 -----------------
.../selftests/bpf/prog_tests/test_overhead.c | 14 +-------------
tools/testing/selftests/bpf/progs/test_overhead.c | 6 ------
tools/testing/selftests/bpf/progs/trigger_bench.c | 7 -------
6 files changed, 1 insertion(+), 65 deletions(-)
From: Toke Høiland-Jørgensen <redacted>
In preparation for moving code around, change a bunch of references to
env->log (and the verbose() logging helper) to use bpf_log() and a direct
pointer to struct bpf_verifier_log. While we're touching the function
signature, mark the 'prog' argument to bpf_check_type_match() as const.
Also enhance the bpf_verifier_log_needed() check to handle NULL pointers
for the log struct so we can re-use the code with logging disabled.
Acked-by: Andrii Nakryiko <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/linux/bpf.h | 2 +-
include/linux/bpf_verifier.h | 5 +++-
kernel/bpf/btf.c | 6 +++--
kernel/bpf/verifier.c | 50 +++++++++++++++++++++---------------------
4 files changed, 32 insertions(+), 31 deletions(-)
@@ -1402,7 +1402,7 @@ int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,structbpf_reg_state*regs);intbtf_prepare_func_args(structbpf_verifier_env*env,intsubprog,structbpf_reg_state*reg);-intbtf_check_type_match(structbpf_verifier_env*env,structbpf_prog*prog,+intbtf_check_type_match(structbpf_verifier_log*log,conststructbpf_prog*prog,structbtf*btf,conststructbtf_type*t);structbpf_prog*bpf_prog_by_id(u32id);
@@ -4388,7 +4388,7 @@ static int btf_check_func_type_match(struct bpf_verifier_log *log,}/* Compare BTFs of given program with BTF of target program */-intbtf_check_type_match(structbpf_verifier_env*env,structbpf_prog*prog,+intbtf_check_type_match(structbpf_verifier_log*log,conststructbpf_prog*prog,structbtf*btf2,conststructbtf_type*t2){structbtf*btf1=prog->aux->btf;
@@ -4408,7 +4408,7 @@ int btf_check_type_match(struct bpf_verifier_env *env, struct bpf_prog *prog,if(!t1||!btf_type_is_func(t1))return-EFAULT;-returnbtf_check_func_type_match(&env->log,btf1,t1,btf2,t2);+returnbtf_check_func_type_match(log,btf1,t1,btf2,t2);}/* Compare BTF of a function with given bpf_reg_state.
@@ -11220,6 +11220,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)structbpf_prog*prog=env->prog;boolprog_extension=prog->type==BPF_PROG_TYPE_EXT;structbpf_prog*tgt_prog=prog->aux->linked_prog;+structbpf_verifier_log*log=&env->log;u32btf_id=prog->aux->attach_btf_id;constcharprefix[]="btf_trace_";structbtf_func_modelfmodel;
@@ -11247,23 +11248,23 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)return0;if(!btf_id){-verbose(env,"Tracing programs must provide btf_id\n");+bpf_log(log,"Tracing programs must provide btf_id\n");return-EINVAL;}btf=bpf_prog_get_target_btf(prog);if(!btf){-verbose(env,+bpf_log(log,"FENTRY/FEXIT program can only be attached to another program annotated with BTF\n");return-EINVAL;}t=btf_type_by_id(btf,btf_id);if(!t){-verbose(env,"attach_btf_id %u is invalid\n",btf_id);+bpf_log(log,"attach_btf_id %u is invalid\n",btf_id);return-EINVAL;}tname=btf_name_by_offset(btf,t->name_off);if(!tname){-verbose(env,"attach_btf_id %u doesn't have a name\n",btf_id);+bpf_log(log,"attach_btf_id %u doesn't have a name\n",btf_id);return-EINVAL;}if(tgt_prog){
@@ -11275,18 +11276,18 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)break;}if(subprog==-1){-verbose(env,"Subprog %s doesn't exist\n",tname);+bpf_log(log,"Subprog %s doesn't exist\n",tname);return-EINVAL;}conservative=aux->func_info_aux[subprog].unreliable;if(prog_extension){if(conservative){-verbose(env,+bpf_log(log,"Cannot replace static functions\n");return-EINVAL;}if(!prog->jit_requested){-verbose(env,+bpf_log(log,"Extension programs should be JITed\n");return-EINVAL;}
@@ -11294,7 +11295,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)prog->expected_attach_type=tgt_prog->expected_attach_type;}if(!tgt_prog->jited){-verbose(env,"Can attach to only JITed progs\n");+bpf_log(log,"Can attach to only JITed progs\n");return-EINVAL;}if(tgt_prog->type==prog->type){
@@ -11339,17 +11340,17 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)switch(prog->expected_attach_type){caseBPF_TRACE_RAW_TP:if(tgt_prog){-verbose(env,+bpf_log(log,"Only FENTRY/FEXIT progs are attachable to another BPF prog\n");return-EINVAL;}if(!btf_type_is_typedef(t)){-verbose(env,"attach_btf_id %u is not a typedef\n",+bpf_log(log,"attach_btf_id %u is not a typedef\n",btf_id);return-EINVAL;}if(strncmp(prefix,tname,sizeof(prefix)-1)){-verbose(env,"attach_btf_id %u points to wrong type name %s\n",+bpf_log(log,"attach_btf_id %u points to wrong type name %s\n",btf_id,tname);return-EINVAL;}
@@ -11372,7 +11373,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)return0;caseBPF_TRACE_ITER:if(!btf_type_is_func(t)){-verbose(env,"attach_btf_id %u is not a function\n",+bpf_log(log,"attach_btf_id %u is not a function\n",btf_id);return-EINVAL;}
@@ -11383,8 +11384,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)prog->aux->attach_func_proto=t;if(!bpf_iter_prog_supported(prog))return-EINVAL;-ret=btf_distill_func_proto(&env->log,btf,t,-tname,&fmodel);+ret=btf_distill_func_proto(log,btf,t,tname,&fmodel);returnret;default:if(!prog_extension)
@@ -11396,18 +11396,18 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)caseBPF_TRACE_FEXIT:prog->aux->attach_func_name=tname;if(prog->type==BPF_PROG_TYPE_LSM){-ret=bpf_lsm_verify_prog(&env->log,prog);+ret=bpf_lsm_verify_prog(log,prog);if(ret<0)returnret;}if(!btf_type_is_func(t)){-verbose(env,"attach_btf_id %u is not a function\n",+bpf_log(log,"attach_btf_id %u is not a function\n",btf_id);return-EINVAL;}if(prog_extension&&-btf_check_type_match(env,prog,btf,t))+btf_check_type_match(log,prog,btf,t))return-EINVAL;t=btf_type_by_id(btf,t->type);if(!btf_type_is_func_proto(t))
@@ -11426,7 +11426,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)prog->aux->attach_func_proto=NULL;t=NULL;}-ret=btf_distill_func_proto(&env->log,btf,t,+ret=btf_distill_func_proto(log,btf,t,tname,&tr->func.model);if(ret<0)gotoout;
@@ -11438,7 +11438,7 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)}else{addr=kallsyms_lookup_name(tname);if(!addr){-verbose(env,+bpf_log(log,"The address of function %s cannot be found\n",tname);ret=-ENOENT;
@@ -11468,17 +11468,17 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)break;}if(ret)-verbose(env,"%s is not sleepable\n",+bpf_log(log,"%s is not sleepable\n",prog->aux->attach_func_name);}elseif(prog->expected_attach_type==BPF_MODIFY_RETURN){if(tgt_prog){-verbose(env,"can't modify return codes of BPF programs\n");+bpf_log(log,"can't modify return codes of BPF programs\n");ret=-EINVAL;gotoout;}ret=check_attach_modify_return(prog,addr);if(ret)-verbose(env,"%s() is not modifiable\n",+bpf_log(log,"%s() is not modifiable\n",prog->aux->attach_func_name);}if(ret)
From: Jiri Olsa <jolsa@kernel.org>
Adding test that setup following program:
SEC("classifier/test_pkt_md_access")
int test_pkt_md_access(struct __sk_buff *skb)
with its extension:
SEC("freplace/test_pkt_md_access")
int test_pkt_md_access_new(struct __sk_buff *skb)
and tracing that extension with:
SEC("fentry/test_pkt_md_access_new")
int BPF_PROG(fentry, struct sk_buff *skb)
The test verifies that the tracing program can
dereference skb argument properly.
Acked-by: Andrii Nakryiko <redacted>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
tools/testing/selftests/bpf/prog_tests/trace_ext.c | 111 ++++++++++++++++++++
tools/testing/selftests/bpf/progs/test_trace_ext.c | 18 +++
.../selftests/bpf/progs/test_trace_ext_tracing.c | 25 +++++
3 files changed, 154 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/trace_ext.c
create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext.c
create mode 100644 tools/testing/selftests/bpf/progs/test_trace_ext_tracing.c
From: Toke Høiland-Jørgensen <redacted>
In preparation for allowing multiple attachments of freplace programs, move
the references to the target program and trampoline into the
bpf_tracing_link structure when that is created. To do this atomically,
introduce a new mutex in prog->aux to protect writing to the two pointers
to target prog and trampoline, and rename the members to make it clear that
they are related.
With this change, it is no longer possible to attach the same tracing
program multiple times (detaching in-between), since the reference from the
tracing program to the target disappears on the first attach. However,
since the next patch will let the caller supply an attach target, that will
also make it possible to attach to the same place multiple times.
Acked-by: Andrii Nakryiko <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
include/linux/bpf.h | 15 +++++++++------
kernel/bpf/btf.c | 6 +++---
kernel/bpf/core.c | 9 ++++++---
kernel/bpf/syscall.c | 47 +++++++++++++++++++++++++++++++++++++++--------
kernel/bpf/trampoline.c | 12 ++++--------
kernel/bpf/verifier.c | 7 +++----
6 files changed, 64 insertions(+), 32 deletions(-)
@@ -746,7 +748,9 @@ struct bpf_prog_aux {u32max_rdonly_access;u32max_rdwr_access;conststructbpf_ctx_arg_aux*ctx_arg_info;-structbpf_prog*linked_prog;+structmutextgt_mutex;/* protects tgt_* pointers below, *after* prog becomes visible */+structbpf_prog*tgt_prog;+structbpf_trampoline*tgt_trampoline;boolverifier_zext;/* Zero extensions has been inserted by verifier. */booloffload_requested;boolattach_btf_trace;/* true if attaching to BTF-enabled raw tp */
@@ -4559,7 +4559,7 @@ int btf_prepare_func_args(struct bpf_verifier_env *env, int subprog,return-EFAULT;}if(prog_type==BPF_PROG_TYPE_EXT)-prog_type=prog->aux->linked_prog->type;+prog_type=prog->aux->tgt_prog->type;t=btf_type_by_id(btf,t->type);if(!t||!btf_type_is_func_proto(t)){
@@ -301,7 +299,7 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)}hlist_add_head(&prog->aux->tramp_hlist,&tr->progs_hlist[kind]);tr->progs_cnt[kind]++;-err=bpf_trampoline_update(prog->aux->trampoline);+err=bpf_trampoline_update(tr);if(err){hlist_del(&prog->aux->tramp_hlist);tr->progs_cnt[kind]--;
@@ -312,13 +310,11 @@ int bpf_trampoline_link_prog(struct bpf_prog *prog)}/* bpf_trampoline_unlink_prog() should never fail. */-intbpf_trampoline_unlink_prog(structbpf_prog*prog)+intbpf_trampoline_unlink_prog(structbpf_prog*prog,structbpf_trampoline*tr){enumbpf_tramp_prog_typekind;-structbpf_trampoline*tr;interr;-tr=prog->aux->trampoline;kind=bpf_attach_type_to_tramp(prog);mutex_lock(&tr->mutex);if(kind==BPF_TRAMP_REPLACE){
@@ -330,7 +326,7 @@ int bpf_trampoline_unlink_prog(struct bpf_prog *prog)}hlist_del(&prog->aux->tramp_hlist);tr->progs_cnt[kind]--;-err=bpf_trampoline_update(prog->aux->trampoline);+err=bpf_trampoline_update(tr);out:mutex_unlock(&tr->mutex);returnerr;
From: Toke Høiland-Jørgensen <redacted>
This adds support for supplying a target btf ID for the bpf_link_create()
operation, and adds a new bpf_program__attach_freplace() high-level API for
attaching freplace functions with a target.
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
tools/lib/bpf/bpf.c | 18 +++++++++++++++---
tools/lib/bpf/bpf.h | 3 ++-
tools/lib/bpf/libbpf.c | 44 +++++++++++++++++++++++++++++++++++++++-----
tools/lib/bpf/libbpf.h | 3 +++
tools/lib/bpf/libbpf.map | 1 +
5 files changed, 60 insertions(+), 9 deletions(-)
@@ -586,19 +586,31 @@ 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;unionbpf_attrattr;if(!OPTS_VALID(opts,bpf_link_create_opts))return-EINVAL;+iter_info_len=OPTS_GET(opts,iter_info_len,0);+target_btf_id=OPTS_GET(opts,target_btf_id,0);++if(iter_info_len&&target_btf_id)+return-EINVAL;+memset(&attr,0,sizeof(attr));attr.link_create.prog_fd=prog_fd;attr.link_create.target_fd=target_fd;attr.link_create.attach_type=attach_type;attr.link_create.flags=OPTS_GET(opts,flags,0);-attr.link_create.iter_info=-ptr_to_u64(OPTS_GET(opts,iter_info,(void*)0));-attr.link_create.iter_info_len=OPTS_GET(opts,iter_info_len,0);++if(iter_info_len){+attr.link_create.iter_info=+ptr_to_u64(OPTS_GET(opts,iter_info,(void*)0));+attr.link_create.iter_info_len=iter_info_len;+}elseif(target_btf_id){+attr.link_create.target_btf_id=target_btf_id;+}returnsys_bpf(BPF_LINK_CREATE,&attr,sizeof(attr));}
@@ -9410,7 +9412,7 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,link->detach=&bpf_link__detach_fd;attach_type=bpf_program__get_expected_attach_type(prog);-link_fd=bpf_link_create(prog_fd,target_fd,attach_type,NULL);+link_fd=bpf_link_create(prog_fd,target_fd,attach_type,&opts);if(link_fd<0){link_fd=-errno;free(link);
@@ -9426,19 +9428,51 @@ bpf_program__attach_fd(struct bpf_program *prog, int target_fd,structbpf_link*bpf_program__attach_cgroup(structbpf_program*prog,intcgroup_fd){-returnbpf_program__attach_fd(prog,cgroup_fd,"cgroup");+returnbpf_program__attach_fd(prog,cgroup_fd,0,"cgroup");}structbpf_link*bpf_program__attach_netns(structbpf_program*prog,intnetns_fd){-returnbpf_program__attach_fd(prog,netns_fd,"netns");+returnbpf_program__attach_fd(prog,netns_fd,0,"netns");}structbpf_link*bpf_program__attach_xdp(structbpf_program*prog,intifindex){/* target_fd/target_ifindex use the same field in LINK_CREATE */-returnbpf_program__attach_fd(prog,ifindex,"xdp");+returnbpf_program__attach_fd(prog,ifindex,0,"xdp");+}++structbpf_link*bpf_program__attach_freplace(structbpf_program*prog,+inttarget_fd,+constchar*attach_func_name)+{+intbtf_id;++if(!!target_fd!=!!attach_func_name){+pr_warn("prog '%s': supply none or both of target_fd and attach_func_name\n",+prog->name);+returnERR_PTR(-EINVAL);+}++if(prog->type!=BPF_PROG_TYPE_EXT){+pr_warn("prog '%s': only BPF_PROG_TYPE_EXT can attach as freplace",+prog->name);+returnERR_PTR(-EINVAL);+}++if(target_fd){+btf_id=libbpf_find_prog_btf_id(attach_func_name,target_fd);+if(btf_id<0)+returnERR_PTR(btf_id);++returnbpf_program__attach_fd(prog,target_fd,btf_id,"freplace");+}else{+/* no target, so use raw_tracepoint_open for compatibility+*witholdkernels+*/+returnbpf_program__attach_trace(prog);+}}structbpf_link*
From: Toke Høiland-Jørgensen <redacted>
From the checks and commit messages for modify_return, it seems it was
never the intention that it should be possible to attach a tracing program
with expected_attach_type == BPF_MODIFY_RETURN to another BPF program.
However, check_attach_modify_return() will only look at the function name,
so if the target function starts with "security_", the attach will be
allowed even for bpf2bpf attachment.
Fix this oversight by also blocking the modification if a target program is
supplied.
Fixes: 18644cec714a ("bpf: Fix use-after-free in fmod_ret check")
Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN")
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
kernel/bpf/verifier.c | 5 +++++
1 file changed, 5 insertions(+)
@@ -11471,6 +11471,11 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)verbose(env,"%s is not sleepable\n",prog->aux->attach_func_name);}elseif(prog->expected_attach_type==BPF_MODIFY_RETURN){+if(tgt_prog){+verbose(env,"can't modify return codes of BPF programs\n");+ret=-EINVAL;+gotoout;+}ret=check_attach_modify_return(prog,addr);if(ret)verbose(env,"%s() is not modifiable\n",
On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen [off-list ref] wrote:
From: Toke Høiland-Jørgensen <redacted>
From the checks and commit messages for modify_return, it seems it was
never the intention that it should be possible to attach a tracing program
with expected_attach_type == BPF_MODIFY_RETURN to another BPF program.
However, check_attach_modify_return() will only look at the function name,
so if the target function starts with "security_", the attach will be
allowed even for bpf2bpf attachment.
Fix this oversight by also blocking the modification if a target program is
supplied.
Fixes: 18644cec714a ("bpf: Fix use-after-free in fmod_ret check")
Fixes: 6ba43b761c41 ("bpf: Attachment verification for BPF_MODIFY_RETURN")
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
@@ -11471,6 +11471,11 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)verbose(env,"%s is not sleepable\n",prog->aux->attach_func_name);}elseif(prog->expected_attach_type==BPF_MODIFY_RETURN){+if(tgt_prog){+verbose(env,"can't modify return codes of BPF programs\n");+ret=-EINVAL;+gotoout;+}ret=check_attach_modify_return(prog,addr);if(ret)verbose(env,"%s() is not modifiable\n",
On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen [off-list ref] wrote:
From: Toke Høiland-Jørgensen <redacted>
This adds support for supplying a target btf ID for the bpf_link_create()
operation, and adds a new bpf_program__attach_freplace() high-level API for
attaching freplace functions with a target.
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen [off-list ref] wrote:
From: Toke Høiland-Jørgensen <redacted>
The benchmark code and the test_overhead prog_test included fmod_ret
programs that attached to various functions in the kernel. However, these
functions were never listed as allowed for return modification, so this
only worked because of the verifier skipping tests when a trampoline
already existed for the attach point. Now that the verifier checks have
been fixed, remove fmod_ret from the affected tests so they all work again.
Fixes: 4eaf0b5c5e04 ("selftest/bpf: Fmod_ret prog and implement test_overhead as part of bench")
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
On Tue, Sep 22, 2020 at 11:39 AM Toke Høiland-Jørgensen [off-list ref] wrote:
quoted
From: Toke Høiland-Jørgensen <redacted>
This adds support for supplying a target btf ID for the bpf_link_create()
operation, and adds a new bpf_program__attach_freplace() high-level API for
attaching freplace functions with a target.
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
LGTM.
Acked-by: Andrii Nakryiko <redacted>
Awesome! Thanks again for your (as always) thorough review (for the
whole series, of course) :)
-Toke
On Tue, Sep 22, 2020 at 08:38:37PM +0200, Toke Høiland-Jørgensen wrote:
From: Toke Høiland-Jørgensen <redacted>
The check_attach_btf_id() function really does three things:
1. It performs a bunch of checks on the program to ensure that the
attachment is valid.
2. It stores a bunch of state about the attachment being requested in
the verifier environment and struct bpf_prog objects.
3. It allocates a trampoline for the attachment.
This patch splits out (1.) and (3.) into separate functions in preparation
for reusing them when the actual attachment is happening (in the
raw_tracepoint_open syscall operation), which will allow tracing programs
to have multiple (compatible) attachments.
raw_tp_open part is no longer correct.
Also could you re-phrase it that 'stores a bunch of state about the attechment'
is still the case. It doesn't store into bpf_prog directly, but returns instead.
This also fixes a bug where a bunch of checks were skipped if a trampoline
already existed for the tracing target.
This time we were lucky. When you see such selftests failures please debug
them before submitting the patches. The reviewers should not be pointing out
that the patch broke some tests.
If anything breaks please mention it in the cover letter.
-static int check_attach_modify_return(struct bpf_prog *prog, unsigned long addr)
+static int check_attach_modify_return(const struct bpf_prog *prog, unsigned long addr,
+ const char *func_name)
Since you're adding 'func_name' why keep 'prog' there? Pls drop it.
How about grouping the return args into
struct bpf_attach_target_info {
struct btf_func_model fmodel;
long tgt_addr;
const char *tgt_name;
const struct btf_type *tgt_type;
};
allocate it on stack in the caller and pass a pointer into this function?
The same way pass the whole &bpf_attach_target_info into bpf_trampoline_get().
It will use fmodel and tgt_addr out of it, but it doesn't hurt to pass
the whole thing.
Overall I like the refactoring, but this prototype and conditional
if (tgt_name) *tgt_name =; and if (tgt_type) makes it harder to comprehend.
quoted hunk
if (!tgt_prog->jited) {
bpf_log(log, "Can attach to only JITed progs\n");
Could you please refactor key computation into a helper as well?
Especially since it will be used out of verifier.c and from syscall.c.
Something like this for bpf.h
static inline u64 bpf_trampoline_compute_key(struct bpf_prog *tgt_prog, u32 btf_id)
{
if (tgt_prog) {
return ((u64)tgt_prog->aux->id) << 32 | btf_id;
} else {
return btf_id;
}
}
and here it would be:
if (tgt_prog && prog->type == BPF_PROG_TYPE_EXT) {
env->ops = bpf_verifier_ops[tgt_prog->type];
prog->expected_attach_type = tgt_prog->expected_attach_type;
}
key = bpf_trampoline_compute_key(tgt_prog, btf_id);
otherwise above 'if' groups two separate things.
It's not pretty in the existing code, no doubt, but since you're doing
nice cleanup let's make it clean here too.
+
+ /* remember two read only pointers that are valid for
+ * the life time of the kernel
+ */
Here this comment is not correct.
It was correct in the place you copy-pasted it from, but not here.
Please think it through and adjust accordingly.
This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.
I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.
+ struct mutex tgt_mutex; /* protects tgt_* pointers below, *after* prog becomes visible */
+ struct bpf_prog *tgt_prog;
+ struct bpf_trampoline *tgt_trampoline;
bool verifier_zext; /* Zero extensions has been inserted by verifier. */
bool offload_requested;
bool attach_btf_trace; /* true if attaching to BTF-enabled raw tp */
imo it's confusing to have 'tgt_prog' to mean two different things.
In prog->aux->tgt_prog it means target prog to attach to in the future.
Whereas here it means the existing prog that was used to attached to.
They kinda both 'target progs' but would be good to disambiguate.
May be keep it as 'tgt_prog' here and
rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?
I had to scratch my head quite a bit before I understood this NULL check.
Could you add a comment saying that tr_link->tgt_prog can be NULL
when trampoline is for kernel function ?
On Tue, Sep 22, 2020 at 08:38:39PM +0200, Toke Høiland-Jørgensen wrote:
+
+ if (tgt_prog_fd) {
+ /* For now we only allow new targets for BPF_PROG_TYPE_EXT */
+ if (prog->type != BPF_PROG_TYPE_EXT) {
+ err = -EINVAL;
+ goto out_put_prog;
+ }
+
+ tgt_prog = bpf_prog_get(tgt_prog_fd);
+ if (IS_ERR(tgt_prog)) {
+ err = PTR_ERR(tgt_prog);
+ tgt_prog = NULL;
+ goto out_put_prog;
+ }
+
+ key = ((u64)tgt_prog->aux->id) << 32 | btf_id;
key = bpf_trampoline_compute_key(tgt_prog, btf_id);
would be handy here.
quoted hunk
+ }
+
link = kzalloc(sizeof(*link), GFP_USER);
if (!link) {
err = -ENOMEM;
@@ -2594,12 +2622,28 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog) mutex_lock(&prog->aux->tgt_mutex);- if (!prog->aux->tgt_trampoline) {+ if (!prog->aux->tgt_trampoline && !tgt_prog) { err = -ENOENT; goto out_unlock; }
Could you add a comment explaining all cases, since it's hard to follow right
now and few month later no one will remember.
As far as I understood:
prog->aux->dest_trampoline != NULL -> the program was just loaded and not attached to anything.
prog->aux->dest_trampoline == NULL -> the program was loaded and raw_tp_open-ed.
tgt_prog != NULL only when sepcifying tgt_prog_fd + target_btf_id in link_create api.
tgt_prog == NULL when this function is called from raw_tp_open.
Only the case of both NULL is invalid.
This 'else' is the case when a prog was loaded and _not_ raw_tp_open-ed
and the user is doing link_create with tgt_prog_fd + target_btf_id
into exactly the same place as attach_btf_id during load?
So this is the alternative api to raw_tp_open, right?
Please explain this in commit log and in comments.
It's not some minor detail.
@@ -2614,16 +2658,24 @@ static int bpf_tracing_prog_attach(struct bpf_prog *prog) link->tgt_prog = tgt_prog; link->trampoline = tr;-- prog->aux->tgt_prog = NULL;- prog->aux->tgt_trampoline = NULL;+ if (tr == prog->aux->tgt_trampoline) {+ /* if we got a new ref from syscall, drop existing one from prog */+ if (tgt_prog_fd)+ bpf_prog_put(prog->aux->tgt_prog);+ prog->aux->tgt_trampoline = NULL;+ prog->aux->tgt_prog = NULL;+ }
What happens when the user did prog load with attach_btf_id into one tgt_prog
but then link_create into a different tgt_prog?
bpf_check_attach_target + bpf_trampoline_get will allocate new trampoline (potentially)
and tr != prog->aux->dest_trampoline,
so we won't trigger the above code.
prog->aux->dest_prog/dest_tramoline will still point to some prog.
Later raw_tp_open will succeed and prog will be attached to two places.
I would probably make it unconditional that both raw_tp_open
and link_create clear dest_prog/dest_trampoline, but can be convinced otherwise.
What use case do you have in mind to allow that?
Anyway it needs to be documented and tests written.
May be call them saved_tgt_prog_type and saved_tgt_attach_type ?
Since that's another variant of 'target' meaning. Here the prog type survives
the target prog, since it will be still valid even when the first prog it was
attached to will be unloaded.
@@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx)__sync_add_and_fetch(&hits,1);return0;}--SEC("fmod_ret/__x64_sys_getpgid")-intbench_trigger_fmodret(void*ctx)-{-__sync_add_and_fetch(&hits,1);-return-22;-}
why are you removing this? There is no problem here.
All syscalls are error-injectable.
I'm surprised Andrii acked this :(
Andrii didn't know that all syscalls are error-injectable, thanks for
catching :) after fmod_ret/__set_task_comm I just assumed that I've
been abusing fmod_ret all this time...
This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.
I did run the tests, and saw no more breakages after applying my patches
than before. Which didn't catch this, because this is the current state
of bpf-next selftests:
# ./test_progs | grep FAIL
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10 btf_map_in_map:FAIL
configure_stack:FAIL:BPF load failed; run with -vv for more info
#72 sk_assign:FAIL
test_test_bpffs:FAIL:bpffs test failed 255
#96 test_bpffs:FAIL
Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED
The test_bpffs failure happens because the umh is missing from the
.config; and when I tried to fix this I ended up with:
[..]
CC [M] kernel/bpf/preload/bpf_preload_kern.o
Auto-detecting system features:
... libelf: [ OFF ]
... zlib: [ OFF ]
... bpf: [ OFF ]
No libelf found
...which I just put down to random breakage, turned off the umh and
continued on my way (ignoring the failed test). Until you wrote this I
did not suspect this would be something I needed to pay attention to.
Now that you did mention it, I'll obviously go investigate some more, my
point is just that in this instance it's not accurate to assume I just
didn't run the tests... :)
I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.
That is completely fine if the tests are working in the first place. And
even when they're not (like in this case), pointing it out is fine, and
I'll obviously go investigate. But please at least reply to the email,
not all of us watch patchwork regularly.
(I'll fix all your other comments and respin; thanks!)
-Toke
On Thu, Sep 24, 2020 at 7:34 AM Toke Høiland-Jørgensen [off-list ref] wrote:
...which I just put down to random breakage, turned off the umh and
continued on my way (ignoring the failed test). Until you wrote this I
did not suspect this would be something I needed to pay attention to.
Now that you did mention it, I'll obviously go investigate some more, my
point is just that in this instance it's not accurate to assume I just
didn't run the tests... :)
Ignoring failures is the same as not running them.
I expect all developers to confirm that they see "0 FAILED" before
sending any patches.
quoted
I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.
That is completely fine if the tests are working in the first place. And
even when they're not (like in this case), pointing it out is fine, and
I'll obviously go investigate. But please at least reply to the email,
not all of us watch patchwork regularly.
Please see Documentation/bpf/bpf_devel_QA.rst.
patchwork status is the way we communicate the intent.
If the patch is not in the queue it won't be acted upon.
This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.
I did run the tests, and saw no more breakages after applying my patches
than before. Which didn't catch this, because this is the current state
of bpf-next selftests:
# ./test_progs | grep FAIL
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10 btf_map_in_map:FAIL
this failure suggests you are not running the latest kernel, btw
configure_stack:FAIL:BPF load failed; run with -vv for more info
#72 sk_assign:FAIL
test_test_bpffs:FAIL:bpffs test failed 255
#96 test_bpffs:FAIL
Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED
The test_bpffs failure happens because the umh is missing from the
.config; and when I tried to fix this I ended up with:
yeah, seems like selftests/bpf/config needs to be updated to mention
UMH-related config values:
CONFIG_BPF_PRELOAD=y
CONFIG_BPF_PRELOAD_UMD=m|y
with that test_bpffs shouldn't fail on master
[..]
CC [M] kernel/bpf/preload/bpf_preload_kern.o
Auto-detecting system features:
... libelf: [ OFF ]
... zlib: [ OFF ]
... bpf: [ OFF ]
No libelf found
might be worthwhile to look into why detection fails, might be
something with Makefiles or your environment
...which I just put down to random breakage, turned off the umh and
continued on my way (ignoring the failed test). Until you wrote this I
did not suspect this would be something I needed to pay attention to.
Now that you did mention it, I'll obviously go investigate some more, my
point is just that in this instance it's not accurate to assume I just
didn't run the tests... :)
Don't just assume some tests are always broken. Either ask or
investigate on your own. Such cases do happen from time to time while
we wait for a fix in bpf to get merged into bpf-next or vice versa,
but it's rare. We now have two different CI systems running selftests
all the time, in addition to running them locally as well, so any
permanent test failure is very apparent and annoying, so we fix them
quickly. So, when in doubt - ask or fix.
quoted
I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.
That is completely fine if the tests are working in the first place. And
They are and hopefully moving forward that would be your assumption.
even when they're not (like in this case), pointing it out is fine, and
I'll obviously go investigate. But please at least reply to the email,
not all of us watch patchwork regularly.
(I'll fix all your other comments and respin; thanks!)
-Toke
This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.
I did run the tests, and saw no more breakages after applying my patches
than before. Which didn't catch this, because this is the current state
of bpf-next selftests:
# ./test_progs | grep FAIL
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10 btf_map_in_map:FAIL
this failure suggests you are not running the latest kernel, btw
I did see that discussion (about the reverted patch), and figured that
was the case. So I did a 'git pull' just before testing, and still got
this.
$ git describe HEAD
v5.9-rc3-2681-g182bf3f3ddb6
so any other ideas? :)
quoted
configure_stack:FAIL:BPF load failed; run with -vv for more info
#72 sk_assign:FAIL
(and what about this one, now that I'm asking?)
quoted
test_test_bpffs:FAIL:bpffs test failed 255
#96 test_bpffs:FAIL
Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED
The test_bpffs failure happens because the umh is missing from the
.config; and when I tried to fix this I ended up with:
yeah, seems like selftests/bpf/config needs to be updated to mention
UMH-related config values:
CONFIG_BPF_PRELOAD=y
CONFIG_BPF_PRELOAD_UMD=m|y
with that test_bpffs shouldn't fail on master
Yup, did get that far, and got the below...
quoted
[..]
CC [M] kernel/bpf/preload/bpf_preload_kern.o
Auto-detecting system features:
... libelf: [ OFF ]
... zlib: [ OFF ]
... bpf: [ OFF ]
No libelf found
might be worthwhile to look into why detection fails, might be
something with Makefiles or your environment
I think it's actually another instance of the bug I fixed with this
commit:
1eb832ac2dee ("tools/bpf: build: Make sure resolve_btfids cleans up after itself")
which I finally remembered after being tickled by the error message
seeming familiar. And indeed, manually removing the 'feature' directory
in kernel/bpf/preload seems to fix the issue, so I'm planning to go fix
that Makefile as well...
quoted
...which I just put down to random breakage, turned off the umh and
continued on my way (ignoring the failed test). Until you wrote this I
did not suspect this would be something I needed to pay attention to.
Now that you did mention it, I'll obviously go investigate some more, my
point is just that in this instance it's not accurate to assume I just
didn't run the tests... :)
Don't just assume some tests are always broken. Either ask or
investigate on your own. Such cases do happen from time to time while
we wait for a fix in bpf to get merged into bpf-next or vice versa,
but it's rare. We now have two different CI systems running selftests
all the time, in addition to running them locally as well, so any
permanent test failure is very apparent and annoying, so we fix them
quickly. So, when in doubt - ask or fix.
That's good to know; and I do think the situation has improved
immensely. There was a time when the selftests broke every other week
(or so it felt, at least), and I guess I'm still a bit scarred from
that.
One thing that would be really useful would be to have a 'reference
config' or something like that. Missing config options are a common
reason for test failures (as we have just seen above), and it's not
always obvious which option is missing for each test. Even something
like grepping .config for BPF doesn't catch everything. If you already
have a CI running, just pointing to that config would be a good start
(especially if it has history). In an ideal world I think it would be
great if each test could detect whether the kernel has the right config
set for its features and abort with a clear error message if it isn't...
quoted
quoted
I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.
That is completely fine if the tests are working in the first place. And
They are and hopefully moving forward that would be your assumption.
Sure, with the exception of the two tests still failing that I mentioned
above. Which I'm hoping you can help figure out the reason for :)
-Toke
I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.
That is completely fine if the tests are working in the first place. And
even when they're not (like in this case), pointing it out is fine, and
I'll obviously go investigate. But please at least reply to the email,
not all of us watch patchwork regularly.
Please see Documentation/bpf/bpf_devel_QA.rst.
patchwork status is the way we communicate the intent.
If the patch is not in the queue it won't be acted upon.
I do realise that you guys use patchwork as the status tracker, but from
a submitter PoV, in practice a change there is coupled with an email
either requesting something change, or notifying of merge. Which is
fine, and I'm not asking you to do anything differently. I'm just
suggesting that if you start silently marking patches as 'changes
requested' without emailing the submitter explaining why, that will just
going to end up creating confusion, and you'll get questions and/or
identical resubmissions. So it won't actually solve anything...
(And to be clear, I'm not saying this because I plan to deliberately
submit patches with broken selftests in the future!)
-Toke
imo it's confusing to have 'tgt_prog' to mean two different things.
In prog->aux->tgt_prog it means target prog to attach to in the future.
Whereas here it means the existing prog that was used to attached to.
They kinda both 'target progs' but would be good to disambiguate.
May be keep it as 'tgt_prog' here and
rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?
I started changing this as you suggested, but I think it actually makes
the code weirder. We'll end up with a lot of 'tgt_prog =
prog->aux->dest_prog' assignments in the verifier, unless we also rename
all of the local variables, which I think is just code churn for very
little gain (the existing 'target' meaning is quite clear, I think).
I also think it's quite natural that the target moves; I mean, it's
literally the same pointer being re-assigned from prog->aux to the link.
We could rename the link member to 'attached_tgt_prog' or something like
that, but I'm not sure it helps (and I don't see much of a problem in
the first place).
WDYT?
-Toke
This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.
I did run the tests, and saw no more breakages after applying my patches
than before. Which didn't catch this, because this is the current state
of bpf-next selftests:
# ./test_progs | grep FAIL
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10 btf_map_in_map:FAIL
this failure suggests you are not running the latest kernel, btw
I did see that discussion (about the reverted patch), and figured that
was the case. So I did a 'git pull' just before testing, and still got
this.
$ git describe HEAD
v5.9-rc3-2681-g182bf3f3ddb6
so any other ideas? :)
That memory leak was fixed in 1d4e1eab456e ("bpf: Fix map leak in
HASH_OF_MAPS map") at the end of July. So while your git repo might be
checked out on a recent enough commit, could it be that the kernel
that you are running is not what you think you are running?
I specifically built kernel from the same commit and double-checked:
[vmuser@archvm bpf]$ uname -r
5.9.0-rc6-01779-g182bf3f3ddb6
[vmuser@archvm bpf]$ sudo ./test_progs -t map_in_map
#10/1 lookup_update:OK
#10/2 diff_size:OK
#10 btf_map_in_map:OK
Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
quoted
quoted
configure_stack:FAIL:BPF load failed; run with -vv for more info
#72 sk_assign:FAIL
(and what about this one, now that I'm asking?)
Did you run with -vv? Jakub Sitnicki (cc'd) might probably help, if
you provide a bit more details.
quoted
quoted
test_test_bpffs:FAIL:bpffs test failed 255
#96 test_bpffs:FAIL
Summary: 113/844 PASSED, 14 SKIPPED, 4 FAILED
The test_bpffs failure happens because the umh is missing from the
.config; and when I tried to fix this I ended up with:
yeah, seems like selftests/bpf/config needs to be updated to mention
UMH-related config values:
CONFIG_BPF_PRELOAD=y
CONFIG_BPF_PRELOAD_UMD=m|y
with that test_bpffs shouldn't fail on master
Yup, did get that far, and got the below...
quoted
quoted
[..]
CC [M] kernel/bpf/preload/bpf_preload_kern.o
Auto-detecting system features:
... libelf: [ OFF ]
... zlib: [ OFF ]
... bpf: [ OFF ]
No libelf found
might be worthwhile to look into why detection fails, might be
something with Makefiles or your environment
I think it's actually another instance of the bug I fixed with this
commit:
1eb832ac2dee ("tools/bpf: build: Make sure resolve_btfids cleans up after itself")
which I finally remembered after being tickled by the error message
seeming familiar. And indeed, manually removing the 'feature' directory
in kernel/bpf/preload seems to fix the issue, so I'm planning to go fix
that Makefile as well...
glad we got to the bottom of it
quoted
quoted
...which I just put down to random breakage, turned off the umh and
continued on my way (ignoring the failed test). Until you wrote this I
did not suspect this would be something I needed to pay attention to.
Now that you did mention it, I'll obviously go investigate some more, my
point is just that in this instance it's not accurate to assume I just
didn't run the tests... :)
Don't just assume some tests are always broken. Either ask or
investigate on your own. Such cases do happen from time to time while
we wait for a fix in bpf to get merged into bpf-next or vice versa,
but it's rare. We now have two different CI systems running selftests
all the time, in addition to running them locally as well, so any
permanent test failure is very apparent and annoying, so we fix them
quickly. So, when in doubt - ask or fix.
That's good to know; and I do think the situation has improved
immensely. There was a time when the selftests broke every other week
(or so it felt, at least), and I guess I'm still a bit scarred from
that.
One thing that would be really useful would be to have a 'reference
config' or something like that. Missing config options are a common
reason for test failures (as we have just seen above), and it's not
always obvious which option is missing for each test. Even something
like grepping .config for BPF doesn't catch everything. If you already
have a CI running, just pointing to that config would be a good start
(especially if it has history). In an ideal world I think it would be
great if each test could detect whether the kernel has the right config
set for its features and abort with a clear error message if it isn't...
so tools/testing/selftests/bpf/config is intended to list all the
config values necessary, but given we don't update them often we
forget to update them when selftests requiring extra kernel config are
added, unfortunately.
As for CI's config, check [0], that's what we use to build kernels.
Kernel config is intentionally pretty minimal and is running in a
single-user mode in pretty stripped down environment, so might not
work as is for full-blown VM. But you can still take a look.
[0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/latest.config
quoted
quoted
quoted
I think I will just start marking patches as changes-requested when I see that
they break tests without replying and without reviewing.
Please respect reviewer's time.
That is completely fine if the tests are working in the first place. And
They are and hopefully moving forward that would be your assumption.
Sure, with the exception of the two tests still failing that I mentioned
above. Which I'm hoping you can help figure out the reason for :)
-Toke
This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.
I did run the tests, and saw no more breakages after applying my patches
than before. Which didn't catch this, because this is the current state
of bpf-next selftests:
# ./test_progs | grep FAIL
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10 btf_map_in_map:FAIL
this failure suggests you are not running the latest kernel, btw
I did see that discussion (about the reverted patch), and figured that
was the case. So I did a 'git pull' just before testing, and still got
this.
$ git describe HEAD
v5.9-rc3-2681-g182bf3f3ddb6
so any other ideas? :)
That memory leak was fixed in 1d4e1eab456e ("bpf: Fix map leak in
HASH_OF_MAPS map") at the end of July. So while your git repo might be
checked out on a recent enough commit, could it be that the kernel
that you are running is not what you think you are running?
Nah, I'm running these in a one-shot virtual machine with virtme-run.
I specifically built kernel from the same commit and double-checked:
[vmuser@archvm bpf]$ uname -r
5.9.0-rc6-01779-g182bf3f3ddb6
[vmuser@archvm bpf]$ sudo ./test_progs -t map_in_map
#10/1 lookup_update:OK
#10/2 diff_size:OK
#10 btf_map_in_map:OK
Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
configure_stack:FAIL:BPF load failed; run with -vv for more info
#72 sk_assign:FAIL
(and what about this one, now that I'm asking?)
Did you run with -vv? Jakub Sitnicki (cc'd) might probably help, if
you provide a bit more details.
No, I didn't, silly me. Turned out that was also just a missing config
option - thanks! :)
quoted
One thing that would be really useful would be to have a 'reference
config' or something like that. Missing config options are a common
reason for test failures (as we have just seen above), and it's not
always obvious which option is missing for each test. Even something
like grepping .config for BPF doesn't catch everything. If you already
have a CI running, just pointing to that config would be a good start
(especially if it has history). In an ideal world I think it would be
great if each test could detect whether the kernel has the right config
set for its features and abort with a clear error message if it isn't...
so tools/testing/selftests/bpf/config is intended to list all the
config values necessary, but given we don't update them often we
forget to update them when selftests requiring extra kernel config are
added, unfortunately.
Ah, that's useful! I wonder how difficult it would be to turn this into
a 'make bpfconfig' top-level make target (similar to 'make defconfig')?
That way, it could be run automatically, and we would also catch
anything missing?
As for CI's config, check [0], that's what we use to build kernels.
Kernel config is intentionally pretty minimal and is running in a
single-user mode in pretty stripped down environment, so might not
work as is for full-blown VM. But you can still take a look.
[0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/latest.config
Well that's how I'm running my own tests (as mentioned above), so that
might be useful, actually! I'll go take a look, thanks :)
-Toke
This change breaks bpf_preload and selftests test_bpffs.
There is really no excuse not to run the selftests.
I did run the tests, and saw no more breakages after applying my patches
than before. Which didn't catch this, because this is the current state
of bpf-next selftests:
# ./test_progs | grep FAIL
test_lookup_update:FAIL:map1_leak inner_map1 leaked!
#10/1 lookup_update:FAIL
#10 btf_map_in_map:FAIL
this failure suggests you are not running the latest kernel, btw
I did see that discussion (about the reverted patch), and figured that
was the case. So I did a 'git pull' just before testing, and still got
this.
$ git describe HEAD
v5.9-rc3-2681-g182bf3f3ddb6
so any other ideas? :)
That memory leak was fixed in 1d4e1eab456e ("bpf: Fix map leak in
HASH_OF_MAPS map") at the end of July. So while your git repo might be
checked out on a recent enough commit, could it be that the kernel
that you are running is not what you think you are running?
Nah, I'm running these in a one-shot virtual machine with virtme-run.
quoted
I specifically built kernel from the same commit and double-checked:
[vmuser@archvm bpf]$ uname -r
5.9.0-rc6-01779-g182bf3f3ddb6
[vmuser@archvm bpf]$ sudo ./test_progs -t map_in_map
#10/1 lookup_update:OK
#10/2 diff_size:OK
#10 btf_map_in_map:OK
Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
Trying the same, while manually entering the VM:
[root@(none) bpf]# uname -r
5.9.0-rc6-02685-g64363ff12e8f
I don't see 64363ff12e8f sha in my repo, so I still don't know what
commit your kernel is built off of. But I believe that you have the
latest kernel, you'll just need to debug this on your own, though,
because this test was never flaky for me, I can't repro the failure.
try adding sleep(few seconds, enough for RCU grace period to pass)
here and see if that helps
if not, please printk() around to see why the inner_map1 wasn't freed
configure_stack:FAIL:BPF load failed; run with -vv for more info
#72 sk_assign:FAIL
(and what about this one, now that I'm asking?)
Did you run with -vv? Jakub Sitnicki (cc'd) might probably help, if
you provide a bit more details.
No, I didn't, silly me. Turned out that was also just a missing config
option - thanks! :)
ok, cool
quoted
quoted
One thing that would be really useful would be to have a 'reference
config' or something like that. Missing config options are a common
reason for test failures (as we have just seen above), and it's not
always obvious which option is missing for each test. Even something
like grepping .config for BPF doesn't catch everything. If you already
have a CI running, just pointing to that config would be a good start
(especially if it has history). In an ideal world I think it would be
great if each test could detect whether the kernel has the right config
set for its features and abort with a clear error message if it isn't...
so tools/testing/selftests/bpf/config is intended to list all the
config values necessary, but given we don't update them often we
forget to update them when selftests requiring extra kernel config are
added, unfortunately.
Ah, that's useful! I wonder how difficult it would be to turn this into
a 'make bpfconfig' top-level make target (similar to 'make defconfig')?
That way, it could be run automatically, and we would also catch
anything missing?
no idea, might be worth trying
quoted
As for CI's config, check [0], that's what we use to build kernels.
Kernel config is intentionally pretty minimal and is running in a
single-user mode in pretty stripped down environment, so might not
work as is for full-blown VM. But you can still take a look.
[0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/latest.config
Well that's how I'm running my own tests (as mentioned above), so that
might be useful, actually! I'll go take a look, thanks :)
try adding sleep(few seconds, enough for RCU grace period to pass)
here and see if that helps
if not, please printk() around to see why the inner_map1 wasn't freed
Aha, found it! It happened because my kernel was built with
PREEMPT_VOLUNTARY. Changing that to PREEMPT fixed the test, and got me
to:
Summary: 116/853 PASSED, 14 SKIPPED, 0 FAILED
So yay! Thanks for your help with debugging :)
-Toke
@@ -45,10 +45,3 @@ int bench_trigger_fentry_sleep(void *ctx)__sync_add_and_fetch(&hits,1);return0;}--SEC("fmod_ret/__x64_sys_getpgid")-intbench_trigger_fmodret(void*ctx)-{-__sync_add_and_fetch(&hits,1);-return-22;-}
why are you removing this? There is no problem here.
All syscalls are error-injectable.
I'm surprised Andrii acked this :(
Andrii didn't know that all syscalls are error-injectable, thanks for
catching :) after fmod_ret/__set_task_comm I just assumed that I've
been abusing fmod_ret all this time...
I didn't know that either. Shall I just drop your ACK from the next
version so you can take another look?
-Toke
imo it's confusing to have 'tgt_prog' to mean two different things.
In prog->aux->tgt_prog it means target prog to attach to in the future.
Whereas here it means the existing prog that was used to attached to.
They kinda both 'target progs' but would be good to disambiguate.
May be keep it as 'tgt_prog' here and
rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?
I started changing this as you suggested, but I think it actually makes
the code weirder. We'll end up with a lot of 'tgt_prog =
prog->aux->dest_prog' assignments in the verifier, unless we also rename
all of the local variables, which I think is just code churn for very
little gain (the existing 'target' meaning is quite clear, I think).
you mean "churn" just for this patch. that's fine.
But it will make names more accurate for everyone reading it afterwards.
Hence I prefer distinct and specific names where possible.
I also think it's quite natural that the target moves; I mean, it's
literally the same pointer being re-assigned from prog->aux to the link.
We could rename the link member to 'attached_tgt_prog' or something like
that, but I'm not sure it helps (and I don't see much of a problem in
the first place).
'attached_tgt_prog' will not be the correct name.
There is 'prog' inside the link already. That's 'attached' prog.
Not this one. This one is the 'attached_to' prog.
But such name would be too long.
imo calling it 'dest_prog' in aux is shorter and more obvious.
imo it's confusing to have 'tgt_prog' to mean two different things.
In prog->aux->tgt_prog it means target prog to attach to in the future.
Whereas here it means the existing prog that was used to attached to.
They kinda both 'target progs' but would be good to disambiguate.
May be keep it as 'tgt_prog' here and
rename to 'dest_prog' and 'dest_trampoline' in prog->aux ?
I started changing this as you suggested, but I think it actually makes
the code weirder. We'll end up with a lot of 'tgt_prog =
prog->aux->dest_prog' assignments in the verifier, unless we also rename
all of the local variables, which I think is just code churn for very
little gain (the existing 'target' meaning is quite clear, I think).
you mean "churn" just for this patch. that's fine.
But it will make names more accurate for everyone reading it afterwards.
Hence I prefer distinct and specific names where possible.
quoted
I also think it's quite natural that the target moves; I mean, it's
literally the same pointer being re-assigned from prog->aux to the link.
We could rename the link member to 'attached_tgt_prog' or something like
that, but I'm not sure it helps (and I don't see much of a problem in
the first place).
'attached_tgt_prog' will not be the correct name.
There is 'prog' inside the link already. That's 'attached' prog.
Not this one. This one is the 'attached_to' prog.
But such name would be too long.
imo calling it 'dest_prog' in aux is shorter and more obvious.
Meh, don't really see how it helps ('destination' and 'target' are
literally synonyms). But I don't care enough to bikeshed about it
either, so I'll just do a search/replace...
-Toke