| Message ID | 20201205025140.443115-1-andrii@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [bpf-next] libbpf: support module BTF for BPF_TYPE_ID_TARGET CO-RE relocation | expand |
On Fri, 4 Dec 2020, Andrii Nakryiko wrote: > When Clang emits ldimm64 instruction for BPF_TYPE_ID_TARGET CO-RE relocation, > put module BTF FD, containing target type, into upper 32 bits of imm64. > > Because this FD is internal to libbpf, it's very cumbersome to test this in > selftests. Manual testing was performed with debug log messages sprinkled > across selftests and libbpf, confirming expected values are substituted. > Better testing will be performed as part of the work adding module BTF types > support to bpf_snprintf_btf() helpers. > > Cc: Alan Maguire <alan.maguire@oracle.com> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Thanks so much for doing this Andrii! When I tested, I ran into a problem; it turns out when a module struct such as "veth_stats" is used, it's classified as BTF_KIND_FWD, and as a result when we iterate over the modules and look in the veth module, "struct veth_stats" does not match since its module kind (BTF_KIND_STRUCT) does not match the candidate kind (BTF_KIND_FWD). I'm kind of out of my depth here, but the below patch (on top of your patch) worked. However without it - when we find 0 candidate matches - as well as not substituting the module object id/type id - we hit a segfault: Program terminated with signal 11, Segmentation fault. #0 0x0000000000480bf9 in bpf_core_calc_relo (prog=0x4d6ba40, relo=0x4d70e7c, relo_idx=0, local_spec=0x7ffe2cf17b00, targ_spec=0x0, res=0x7ffe2cf17ae0) at libbpf.c:4408 4408 switch (kind) { Missing separate debuginfos, use: debuginfo-install elfutils-libelf-0.172-2.el7.x86_64 glibc-2.17-196.el7.x86_64 libattr-2.4.46-13.el7.x86_64 libcap-2.22-9.el7.x86_64 libgcc-4.8.5-36.0.1.el7_6.2.x86_64 zlib-1.2.7-18.el7.x86_64 (gdb) bt #0 0x0000000000480bf9 in bpf_core_calc_relo (prog=0x4d6ba40, relo=0x4d70e7c, relo_idx=0, local_spec=0x7ffe2cf17b00, targ_spec=0x0, res=0x7ffe2cf17ae0) at libbpf.c:4408 The dereferences of targ_spec in bpf_core_recalc_relo() seem to be the cause; that function is called with a NULL targ_spec when 0 candidates are found, so it's possible we'd need to guard those accesses for cases where a bogus type was passed in and no candidates were found. If the below looks good would it make sense to roll it into your patch or will I add it to my v3 patch series? Thanks again for your help with this! Alan From 08040730dbff6c5d7636927777ac85a71c10827f Mon Sep 17 00:00:00 2001 From: Alan Maguire <alan.maguire@oracle.com> Date: Sun, 6 Dec 2020 01:10:28 +0100 Subject: [PATCH] libbpf: handle fwd kinds when checking candidate relocations for modules when a struct belonging to a module is being assessed, it will be designated a fwd kind (BTF_KIND_FWD); when matching candidate types constraints on exact type matching need to be relaxed to ensure that such structures are found successfully. Introduce kinds_match() function to handle this comparison. Signed-off-by: Alan Maguire <alan.maguire@oracle.com> --- tools/lib/bpf/libbpf.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 539956f..00fdb30 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -4673,6 +4673,24 @@ static void bpf_core_free_cands(struct core_cand_list *cands) free(cands); } +/* module-specific structs will have relo kind set to fwd, so as + * well as handling exact matches, struct has to match fwd kind. + */ +static bool kinds_match(const struct btf_type *type1, + const struct btf_type *type2) +{ + __u8 kind1 = btf_kind(type1); + __u8 kind2 = btf_kind(type2); + + if (kind1 == kind2) + return true; + if (kind1 == BTF_KIND_STRUCT && kind2 == BTF_KIND_FWD) + return true; + if (kind1 == BTF_KIND_FWD && kind2 == BTF_KIND_STRUCT) + return true; + return false; +} + static int bpf_core_add_cands(struct core_cand *local_cand, size_t local_essent_len, const struct btf *targ_btf, @@ -4689,7 +4707,7 @@ static int bpf_core_add_cands(struct core_cand *local_cand, n = btf__get_nr_types(targ_btf); for (i = targ_start_id; i <= n; i++) { t = btf__type_by_id(targ_btf, i); - if (btf_kind(t) != btf_kind(local_cand->t)) + if (!kinds_match(t, local_cand->t)) continue; targ_name = btf__name_by_offset(targ_btf, t->name_off); @@ -5057,7 +5075,7 @@ static int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id /* caller made sure that names match (ignoring flavor suffix) */ local_type = btf__type_by_id(local_btf, local_id); targ_type = btf__type_by_id(targ_btf, targ_id); - if (btf_kind(local_type) != btf_kind(targ_type)) + if (!kinds_match(local_type, targ_type)) return 0; recur: @@ -5070,7 +5088,7 @@ static int bpf_core_types_are_compat(const struct btf *local_btf, __u32 local_id if (!local_type || !targ_type) return -EINVAL; - if (btf_kind(local_type) != btf_kind(targ_type)) + if (!kinds_match(local_type, targ_type)) return 0; switch (btf_kind(local_type)) {
On Fri, 4 Dec 2020, Andrii Nakryiko wrote: > When Clang emits ldimm64 instruction for BPF_TYPE_ID_TARGET CO-RE relocation, > put module BTF FD, containing target type, into upper 32 bits of imm64. > > Because this FD is internal to libbpf, it's very cumbersome to test this in > selftests. Manual testing was performed with debug log messages sprinkled > across selftests and libbpf, confirming expected values are substituted. > Better testing will be performed as part of the work adding module BTF types > support to bpf_snprintf_btf() helpers. > > Cc: Alan Maguire <alan.maguire@oracle.com> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > tools/lib/bpf/libbpf.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 9be88a90a4aa..539956f7920a 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -4795,6 +4795,7 @@ static int load_module_btfs(struct bpf_object *obj) > > mod_btf = &obj->btf_modules[obj->btf_module_cnt++]; > > + btf__set_fd(btf, fd); > mod_btf->btf = btf; > mod_btf->id = id; > mod_btf->fd = fd; > @@ -5445,6 +5446,10 @@ struct bpf_core_relo_res > __u32 orig_type_id; > __u32 new_sz; > __u32 new_type_id; > + /* FD of the module BTF containing the target candidate, or 0 for > + * vmlinux BTF > + */ > + int btf_obj_fd; > }; > > /* Calculate original and target relocation values, given local and target > @@ -5469,6 +5474,7 @@ static int bpf_core_calc_relo(const struct bpf_program *prog, > res->fail_memsz_adjust = false; > res->orig_sz = res->new_sz = 0; > res->orig_type_id = res->new_type_id = 0; > + res->btf_obj_fd = 0; > > if (core_relo_is_field_based(relo->kind)) { > err = bpf_core_calc_field_relo(prog, relo, local_spec, > @@ -5519,6 +5525,9 @@ static int bpf_core_calc_relo(const struct bpf_program *prog, > } else if (core_relo_is_type_based(relo->kind)) { > err = bpf_core_calc_type_relo(relo, local_spec, &res->orig_val); > err = err ?: bpf_core_calc_type_relo(relo, targ_spec, &res->new_val); > + if (!err && relo->kind == BPF_TYPE_ID_TARGET && > + targ_spec->btf != prog->obj->btf_vmlinux) > + res->btf_obj_fd = btf__fd(targ_spec->btf); Sorry about this Andrii, but I'm a bit stuck here. I'm struggling to get tests working where the obj fd is used to designate the module BTF. Unless I'm missing something there are a few problems: - the fd association is removed by libbpf when the BPF program has loaded; the module fds are closed and the module BTF is discarded. However even if that isn't done (and as you mentioned, we could hold onto BTF that is in use, and I commented out the code that does that to test) - there's another problem: - I can't see a way to use the object fd value we set here later in BPF program context; btf_get_by_fd() returns -EBADF as the fd is associated with the module BTF in the test's process context, not necessarily in the context that the BPF program is running. Would it be possible in this case to use object id? Or is there another way to handle the fd->module BTF association that we need to make in BPF program context that I'm missing? - A more long-term issue; if we use fds to specify module BTFs and write the object fd into the program, we can pin the BPF program such that it outlives fds that refer to its associated BTF. So unless we pinned the BTF too, any code that assumed the BTF fd-> module mapping was valid would start to break once the user-space side went away and the pinned program persisted. Maybe there are solutions to these problems that I'm missing of course, but for now I'm not sure how to get things working. Thanks again for your help with this! Alan
On Mon, Dec 07, 2020 at 04:38:16PM +0000, Alan Maguire wrote: > On Fri, 4 Dec 2020, Andrii Nakryiko wrote: > > > When Clang emits ldimm64 instruction for BPF_TYPE_ID_TARGET CO-RE relocation, > > put module BTF FD, containing target type, into upper 32 bits of imm64. > > > > Because this FD is internal to libbpf, it's very cumbersome to test this in > > selftests. Manual testing was performed with debug log messages sprinkled > > across selftests and libbpf, confirming expected values are substituted. > > Better testing will be performed as part of the work adding module BTF types > > support to bpf_snprintf_btf() helpers. > > > > Cc: Alan Maguire <alan.maguire@oracle.com> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > tools/lib/bpf/libbpf.c | 19 ++++++++++++++++--- > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > index 9be88a90a4aa..539956f7920a 100644 > > --- a/tools/lib/bpf/libbpf.c > > +++ b/tools/lib/bpf/libbpf.c > > @@ -4795,6 +4795,7 @@ static int load_module_btfs(struct bpf_object *obj) > > > > mod_btf = &obj->btf_modules[obj->btf_module_cnt++]; > > > > + btf__set_fd(btf, fd); > > mod_btf->btf = btf; > > mod_btf->id = id; > > mod_btf->fd = fd; > > @@ -5445,6 +5446,10 @@ struct bpf_core_relo_res > > __u32 orig_type_id; > > __u32 new_sz; > > __u32 new_type_id; > > + /* FD of the module BTF containing the target candidate, or 0 for > > + * vmlinux BTF > > + */ > > + int btf_obj_fd; > > }; > > > > /* Calculate original and target relocation values, given local and target > > @@ -5469,6 +5474,7 @@ static int bpf_core_calc_relo(const struct bpf_program *prog, > > res->fail_memsz_adjust = false; > > res->orig_sz = res->new_sz = 0; > > res->orig_type_id = res->new_type_id = 0; > > + res->btf_obj_fd = 0; > > > > if (core_relo_is_field_based(relo->kind)) { > > err = bpf_core_calc_field_relo(prog, relo, local_spec, > > @@ -5519,6 +5525,9 @@ static int bpf_core_calc_relo(const struct bpf_program *prog, > > } else if (core_relo_is_type_based(relo->kind)) { > > err = bpf_core_calc_type_relo(relo, local_spec, &res->orig_val); > > err = err ?: bpf_core_calc_type_relo(relo, targ_spec, &res->new_val); > > + if (!err && relo->kind == BPF_TYPE_ID_TARGET && > > + targ_spec->btf != prog->obj->btf_vmlinux) > > + res->btf_obj_fd = btf__fd(targ_spec->btf); > > Sorry about this Andrii, but I'm a bit stuck here. > > I'm struggling to get tests working where the obj fd is used to designate > the module BTF. Unless I'm missing something there are a few problems: > > - the fd association is removed by libbpf when the BPF program has loaded; > the module fds are closed and the module BTF is discarded. However even if > that isn't done (and as you mentioned, we could hold onto BTF that is in > use, and I commented out the code that does that to test) - there's > another problem: > - I can't see a way to use the object fd value we set here later in BPF > program context; btf_get_by_fd() returns -EBADF as the fd is associated > with the module BTF in the test's process context, not necessarily in > the context that the BPF program is running. Would it be possible in this > case to use object id? Or is there another way to handle the fd->module > BTF association that we need to make in BPF program context that I'm > missing? > - A more long-term issue; if we use fds to specify module BTFs and write > the object fd into the program, we can pin the BPF program such that it > outlives fds that refer to its associated BTF. So unless we pinned the > BTF too, any code that assumed the BTF fd-> module mapping was valid would > start to break once the user-space side went away and the pinned program > persisted. All of the above are not issues. They are features of FD based approach. When the program refers to btf via fd the verifier needs to increment btf's refcnt so it won't go away while the prog is running. For module's BTF it means that the module can be unloaded, but its BTF may stay around if there is a prog that needs to access it. I think the missing piece in the above is that btf_get_by_fd() should be done at load time instead of program run-time. Everything FD based needs to behave similar to map_fds where ld_imm64 insn contains map_fd that gets converted to map_ptr by the verifier at load time. In this case single ld_imm64 with 32-bit FD + 32-bit btf_id is not enough. So either libbpf or the verifier need to insert additional instruction. I'm not sure yet how to extend 'struct btf_ptr' cleanly, so it looks good from C side. In the other patch I saw: struct btf_ptr { void *ptr; __u32 type_id; - __u32 flags; /* BTF ptr flags; unused at present. */ + __u32 obj_id; /* BTF object; vmlinux if 0 */ }; The removal of flags cannot be done, since it will break progs. Probably something like this: struct btf_ptr { void *ptr; __u32 type_id; __u32 flags; __u64 btf_obj_fd; /* this is 32-bit FD for libbpf which will become pointer after load */ }; would be the most convenient from the bpf prog side. The ld_imm64 init of btf_obj_fd will be replaced with absolute btf pointer by the verifier. So when bpf_snprintf_btf() is called the prog will pass the kernel internal pointer of struct btf to the helper. No extra run-time checks needed. bpf_snprintf_btf() would print that type_id within given struct btf object. libbpf would need to deal with two relos. One to store btf_id from bpf_core_type_id_kernel() into type_id. And another to find module's BTF and store its FD into btf_obj_fd with ld_imm64. I'm still thinking to how to frame that cleanly from C side. Other ideas?
On Sat, Dec 5, 2020 at 4:38 PM Alan Maguire <alan.maguire@oracle.com> wrote: > > On Fri, 4 Dec 2020, Andrii Nakryiko wrote: > > > When Clang emits ldimm64 instruction for BPF_TYPE_ID_TARGET CO-RE relocation, > > put module BTF FD, containing target type, into upper 32 bits of imm64. > > > > Because this FD is internal to libbpf, it's very cumbersome to test this in > > selftests. Manual testing was performed with debug log messages sprinkled > > across selftests and libbpf, confirming expected values are substituted. > > Better testing will be performed as part of the work adding module BTF types > > support to bpf_snprintf_btf() helpers. > > > > Cc: Alan Maguire <alan.maguire@oracle.com> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > Thanks so much for doing this Andrii! When I tested, I ran into a problem; > it turns out when a module struct such as "veth_stats" is used, it's > classified as BTF_KIND_FWD, and as a result when we iterate over > the modules and look in the veth module, "struct veth_stats" does not > match since its module kind (BTF_KIND_STRUCT) does not match the candidate > kind (BTF_KIND_FWD). I'm kind of out of my depth here, but the below > patch (on top of your patch) worked. I'm not quite clear on the situation. BTF_KIND_FWD is for the local type or the remote type? Maybe a small example would help, before we go straight to assuming FWD can be always resolved into a concrete STRUCT/UNION. > However without it - when we find > 0 candidate matches - as well as not substituting the module object > id/type id - we hit a segfault: Yep, I missed the null check in: targ_spec->btf != prog->obj->btf_vmlinux I'll fix that. > > Program terminated with signal 11, Segmentation fault. > #0 0x0000000000480bf9 in bpf_core_calc_relo (prog=0x4d6ba40, > relo=0x4d70e7c, > relo_idx=0, local_spec=0x7ffe2cf17b00, targ_spec=0x0, > res=0x7ffe2cf17ae0) > at libbpf.c:4408 > 4408 switch (kind) { > Missing separate debuginfos, use: debuginfo-install > elfutils-libelf-0.172-2.el7.x86_64 glibc-2.17-196.el7.x86_64 > libattr-2.4.46-13.el7.x86_64 libcap-2.22-9.el7.x86_64 > libgcc-4.8.5-36.0.1.el7_6.2.x86_64 zlib-1.2.7-18.el7.x86_64 > (gdb) bt > #0 0x0000000000480bf9 in bpf_core_calc_relo (prog=0x4d6ba40, > relo=0x4d70e7c, > relo_idx=0, local_spec=0x7ffe2cf17b00, targ_spec=0x0, > res=0x7ffe2cf17ae0) > at libbpf.c:4408 > > > The dereferences of targ_spec in bpf_core_recalc_relo() seem > to be the cause; that function is called with a NULL targ_spec > when 0 candidates are found, so it's possible we'd need to > guard those accesses for cases where a bogus type was passed > in and no candidates were found. If the below looks good would > it make sense to roll it into your patch or will I add it to my > v3 patch series? > > Thanks again for your help with this! > > Alan > > From 08040730dbff6c5d7636927777ac85a71c10827f Mon Sep 17 00:00:00 2001 > From: Alan Maguire <alan.maguire@oracle.com> > Date: Sun, 6 Dec 2020 01:10:28 +0100 > Subject: [PATCH] libbpf: handle fwd kinds when checking candidate relocations > for modules > > when a struct belonging to a module is being assessed, it will be > designated a fwd kind (BTF_KIND_FWD); when matching candidate > types constraints on exact type matching need to be relaxed to > ensure that such structures are found successfully. Introduce > kinds_match() function to handle this comparison. > > Signed-off-by: Alan Maguire <alan.maguire@oracle.com> > --- > tools/lib/bpf/libbpf.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > [...]
On Mon, Dec 7, 2020 at 7:12 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Dec 07, 2020 at 04:38:16PM +0000, Alan Maguire wrote: > > On Fri, 4 Dec 2020, Andrii Nakryiko wrote: > > > > > When Clang emits ldimm64 instruction for BPF_TYPE_ID_TARGET CO-RE relocation, > > > put module BTF FD, containing target type, into upper 32 bits of imm64. > > > > > > Because this FD is internal to libbpf, it's very cumbersome to test this in > > > selftests. Manual testing was performed with debug log messages sprinkled > > > across selftests and libbpf, confirming expected values are substituted. > > > Better testing will be performed as part of the work adding module BTF types > > > support to bpf_snprintf_btf() helpers. > > > > > > Cc: Alan Maguire <alan.maguire@oracle.com> > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > tools/lib/bpf/libbpf.c | 19 ++++++++++++++++--- > > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > > > index 9be88a90a4aa..539956f7920a 100644 > > > --- a/tools/lib/bpf/libbpf.c > > > +++ b/tools/lib/bpf/libbpf.c > > > @@ -4795,6 +4795,7 @@ static int load_module_btfs(struct bpf_object *obj) > > > > > > mod_btf = &obj->btf_modules[obj->btf_module_cnt++]; > > > > > > + btf__set_fd(btf, fd); > > > mod_btf->btf = btf; > > > mod_btf->id = id; > > > mod_btf->fd = fd; > > > @@ -5445,6 +5446,10 @@ struct bpf_core_relo_res > > > __u32 orig_type_id; > > > __u32 new_sz; > > > __u32 new_type_id; > > > + /* FD of the module BTF containing the target candidate, or 0 for > > > + * vmlinux BTF > > > + */ > > > + int btf_obj_fd; > > > }; > > > > > > /* Calculate original and target relocation values, given local and target > > > @@ -5469,6 +5474,7 @@ static int bpf_core_calc_relo(const struct bpf_program *prog, > > > res->fail_memsz_adjust = false; > > > res->orig_sz = res->new_sz = 0; > > > res->orig_type_id = res->new_type_id = 0; > > > + res->btf_obj_fd = 0; > > > > > > if (core_relo_is_field_based(relo->kind)) { > > > err = bpf_core_calc_field_relo(prog, relo, local_spec, > > > @@ -5519,6 +5525,9 @@ static int bpf_core_calc_relo(const struct bpf_program *prog, > > > } else if (core_relo_is_type_based(relo->kind)) { > > > err = bpf_core_calc_type_relo(relo, local_spec, &res->orig_val); > > > err = err ?: bpf_core_calc_type_relo(relo, targ_spec, &res->new_val); > > > + if (!err && relo->kind == BPF_TYPE_ID_TARGET && > > > + targ_spec->btf != prog->obj->btf_vmlinux) > > > + res->btf_obj_fd = btf__fd(targ_spec->btf); > > > > Sorry about this Andrii, but I'm a bit stuck here. > > > > I'm struggling to get tests working where the obj fd is used to designate > > the module BTF. Unless I'm missing something there are a few problems: > > > > - the fd association is removed by libbpf when the BPF program has loaded; > > the module fds are closed and the module BTF is discarded. However even if > > that isn't done (and as you mentioned, we could hold onto BTF that is in > > use, and I commented out the code that does that to test) - there's > > another problem: > > - I can't see a way to use the object fd value we set here later in BPF > > program context; btf_get_by_fd() returns -EBADF as the fd is associated > > with the module BTF in the test's process context, not necessarily in > > the context that the BPF program is running. Would it be possible in this > > case to use object id? Or is there another way to handle the fd->module > > BTF association that we need to make in BPF program context that I'm > > missing? > > - A more long-term issue; if we use fds to specify module BTFs and write > > the object fd into the program, we can pin the BPF program such that it > > outlives fds that refer to its associated BTF. So unless we pinned the > > BTF too, any code that assumed the BTF fd-> module mapping was valid would > > start to break once the user-space side went away and the pinned program > > persisted. > > All of the above are not issues. They are features of FD based approach. > When the program refers to btf via fd the verifier needs to increment btf's refcnt > so it won't go away while the prog is running. For module's BTF it means > that the module can be unloaded, but its BTF may stay around if there is a prog > that needs to access it. > I think the missing piece in the above is that btf_get_by_fd() should be > done at load time instead of program run-time. > Everything FD based needs to behave similar to map_fds where ld_imm64 insn > contains map_fd that gets converted to map_ptr by the verifier at load time. Right. I was going to extend verifier to do the same for all used BTF objects as part of ksym support for module BTFs. So totally agree. Just didn't need it so far. > In this case single ld_imm64 with 32-bit FD + 32-bit btf_id is not enough. > So either libbpf or the verifier need to insert additional instruction. So this part I haven't investigated in detail yet. But, if we are just talking about keeping struct btf * pointer + BTF type id (u32) in a single ldimm64, we actually have enough space by using both off + imm fields in both parts of ldimm64 instruction. Gives exactly 8 + 4 bytes. But I don't know if the problem you are referring to is in the JIT part. Also, for the ldimm64 instruction generated by __builtin_btf_type_id(), btf fd + btf type id are always the same, regardless of code path, so we can easily use bpf_insn_aux_data to keep any extra data there, no? > I'm not sure yet how to extend 'struct btf_ptr' cleanly, so it looks good > from C side. > In the other patch I saw: > struct btf_ptr { > void *ptr; > __u32 type_id; > - __u32 flags; /* BTF ptr flags; unused at present. */ > + __u32 obj_id; /* BTF object; vmlinux if 0 */ > }; > The removal of flags cannot be done, since it will break progs. This was something that I suggested to avoid extra logic based on the size of btf_ptr. Not super critical. The idea was that flags so far were always enforced to be zero, which make it backwards compatible and we can now re-use it instead for module BTF fd. If we need flags later, then we can extend it. But as I said, it's not a critical part of the design, so I won't fight that :) > Probably something like this: > struct btf_ptr { > void *ptr; > __u32 type_id; > __u32 flags; > __u64 btf_obj_fd; /* this is 32-bit FD for libbpf which will become pointer after load */ > }; > would be the most convenient from the bpf prog side. The ld_imm64 init of > btf_obj_fd will be replaced with absolute btf pointer by the verifier. So when > bpf_snprintf_btf() is called the prog will pass the kernel internal pointer > of struct btf to the helper. No extra run-time checks needed. > bpf_snprintf_btf() would print that type_id within given struct btf object. > libbpf would need to deal with two relos. One to store btf_id from > bpf_core_type_id_kernel() into type_id. And another to find module's BTF and > store its FD into btf_obj_fd with ld_imm64. I'm still thinking to how to frame So the latter we can do as yet another type of type-based CO-RE relocation, if needed. But if we do that, we should probably revert current __builtin_type_id(TYPE_ID_REMOTE) to just emit 32-bit register assignment (no ldimm64). As for how the verifier would translate such FD into struct btf * pointer. We have something similar today with ldimm64 with BPF_PSEUDO_BTF_ID, which resolves into kernel variables. Let's think if we can re-use that, or we can just add another BPF_PSEUDO_xxx "flavor"? > that cleanly from C side. > Other ideas? I'll need to think a bit more about this. But some thoughts I've got so far.
On Mon, 7 Dec 2020, Andrii Nakryiko wrote: > On Sat, Dec 5, 2020 at 4:38 PM Alan Maguire <alan.maguire@oracle.com> wrote: > > Thanks so much for doing this Andrii! When I tested, I ran into a problem; > > it turns out when a module struct such as "veth_stats" is used, it's > > classified as BTF_KIND_FWD, and as a result when we iterate over > > the modules and look in the veth module, "struct veth_stats" does not > > match since its module kind (BTF_KIND_STRUCT) does not match the candidate > > kind (BTF_KIND_FWD). I'm kind of out of my depth here, but the below > > patch (on top of your patch) worked. > > I'm not quite clear on the situation. BTF_KIND_FWD is for the local > type or the remote type? Maybe a small example would help, before we > go straight to assuming FWD can be always resolved into a concrete > STRUCT/UNION. > The local type was BTF_KIND_FWD, and the target type was BTF_KIND_STRUCT IIRC; I'll try and get some libbpf debug output for you showing the relocation info. If it helps, I think the situation was this; I was referencing __builtin_btf_type_id(struct veth_stats), and hadn't included a BTF-generated veth header, so I'm guessing libbpf classified it as a fwd declaration. My patch was a bit too general I suspect in that it assumed that either target or local could be BTF_KIND_FWD and should match BTF_KIND_STRUCT in local/target, wheres I _think_ the local only should permit BTF_KIND_FWD. Does that make sense? > > > However without it - when we find > > 0 candidate matches - as well as not substituting the module object > > id/type id - we hit a segfault: > > Yep, I missed the null check in: > > targ_spec->btf != prog->obj->btf_vmlinux > > I'll fix that. > Thanks! I think the core_reloc selftests trigger the segfault also if you need a test case to verify. Alan
On Mon, 7 Dec 2020, Andrii Nakryiko wrote: > On Mon, Dec 7, 2020 at 7:12 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Mon, Dec 07, 2020 at 04:38:16PM +0000, Alan Maguire wrote: > > > Sorry about this Andrii, but I'm a bit stuck here. > > > > > > I'm struggling to get tests working where the obj fd is used to designate > > > the module BTF. Unless I'm missing something there are a few problems: > > > > > > - the fd association is removed by libbpf when the BPF program has loaded; > > > the module fds are closed and the module BTF is discarded. However even if > > > that isn't done (and as you mentioned, we could hold onto BTF that is in > > > use, and I commented out the code that does that to test) - there's > > > another problem: > > > - I can't see a way to use the object fd value we set here later in BPF > > > program context; btf_get_by_fd() returns -EBADF as the fd is associated > > > with the module BTF in the test's process context, not necessarily in > > > the context that the BPF program is running. Would it be possible in this > > > case to use object id? Or is there another way to handle the fd->module > > > BTF association that we need to make in BPF program context that I'm > > > missing? > > > - A more long-term issue; if we use fds to specify module BTFs and write > > > the object fd into the program, we can pin the BPF program such that it > > > outlives fds that refer to its associated BTF. So unless we pinned the > > > BTF too, any code that assumed the BTF fd-> module mapping was valid would > > > start to break once the user-space side went away and the pinned program > > > persisted. > > > > All of the above are not issues. They are features of FD based approach. > > When the program refers to btf via fd the verifier needs to increment btf's refcnt > > so it won't go away while the prog is running. For module's BTF it means > > that the module can be unloaded, but its BTF may stay around if there is a prog > > that needs to access it. > > I think the missing piece in the above is that btf_get_by_fd() should be > > done at load time instead of program run-time. > > Everything FD based needs to behave similar to map_fds where ld_imm64 insn > > contains map_fd that gets converted to map_ptr by the verifier at load time. > > Right. I was going to extend verifier to do the same for all used BTF > objects as part of ksym support for module BTFs. So totally agree. > Just didn't need it so far. > Does this approach prevent more complex run-time specification of BTF object fd though? For example, I've been working on a simple tracer focused on kernel debugging; it uses a BPF map entry for each kernel function that is traced. User-space populates the map entry with BTF type ids for the function arguments/return value, and when the BPF program runs it uses the instruction pointer to look up the map entry for that function, and uses bpf_snprintf_btf() to write the string representations of the function arguments/return values. I'll send out an RFC soon, but longer-term I was hoping to extend it to support module-specific types. Would a dynamic case like that - where the BTF module fd is looked up in a map entry during program execution (rather than derived via __btf_builtin_type_id()) work too? Thanks! Alan
On Tue, Dec 08, 2020 at 10:13:35PM +0000, Alan Maguire wrote: > On Mon, 7 Dec 2020, Andrii Nakryiko wrote: > > > On Mon, Dec 7, 2020 at 7:12 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Mon, Dec 07, 2020 at 04:38:16PM +0000, Alan Maguire wrote: > > > > Sorry about this Andrii, but I'm a bit stuck here. > > > > > > > > I'm struggling to get tests working where the obj fd is used to designate > > > > the module BTF. Unless I'm missing something there are a few problems: > > > > > > > > - the fd association is removed by libbpf when the BPF program has loaded; > > > > the module fds are closed and the module BTF is discarded. However even if > > > > that isn't done (and as you mentioned, we could hold onto BTF that is in > > > > use, and I commented out the code that does that to test) - there's > > > > another problem: > > > > - I can't see a way to use the object fd value we set here later in BPF > > > > program context; btf_get_by_fd() returns -EBADF as the fd is associated > > > > with the module BTF in the test's process context, not necessarily in > > > > the context that the BPF program is running. Would it be possible in this > > > > case to use object id? Or is there another way to handle the fd->module > > > > BTF association that we need to make in BPF program context that I'm > > > > missing? > > > > - A more long-term issue; if we use fds to specify module BTFs and write > > > > the object fd into the program, we can pin the BPF program such that it > > > > outlives fds that refer to its associated BTF. So unless we pinned the > > > > BTF too, any code that assumed the BTF fd-> module mapping was valid would > > > > start to break once the user-space side went away and the pinned program > > > > persisted. > > > > > > All of the above are not issues. They are features of FD based approach. > > > When the program refers to btf via fd the verifier needs to increment btf's refcnt > > > so it won't go away while the prog is running. For module's BTF it means > > > that the module can be unloaded, but its BTF may stay around if there is a prog > > > that needs to access it. > > > I think the missing piece in the above is that btf_get_by_fd() should be > > > done at load time instead of program run-time. > > > Everything FD based needs to behave similar to map_fds where ld_imm64 insn > > > contains map_fd that gets converted to map_ptr by the verifier at load time. > > > > Right. I was going to extend verifier to do the same for all used BTF > > objects as part of ksym support for module BTFs. So totally agree. > > Just didn't need it so far. > > > > Does this approach prevent more complex run-time specification of BTF > object fd though? For example, I've been working on a simple tracer > focused on kernel debugging; it uses a BPF map entry for each kernel > function that is traced. User-space populates the map entry with BTF type > ids for the function arguments/return value, and when the BPF program > runs it uses the instruction pointer to look up the map entry for that > function, and uses bpf_snprintf_btf() to write the string representations > of the function arguments/return values. I'll send out an RFC soon, > but longer-term I was hoping to extend it to support module-specific > types. Would a dynamic case like that - where the BTF module fd is looked > up in a map entry during program execution (rather than derived via > __btf_builtin_type_id()) work too? Thanks! fd has to be resolved in the process context. bpf prog can read fd number from the map, but that number is meaningless. Say we allow using btf_obj_id+btf_id, how user space will know these two numbers? Some new libbpf api that searches for it? An extension to libbpf_find_vmlinux_btf_id() ? I was hoping that this api will stay semi-internal. But say it's extended. The user space will store a pair of numbers into a map and what program are going to do with it? If it's printing struct veth_stats contents it should have attached to a corresponding function in the veth module via fentry or something. The prog has hard coded logic in C with specific pointer to print. The prog has its type right there. Why would the prog take a pointer from one place, but it's type_id from the map? That's not realistic. Where it would potentially make sense is what I think you're descring where single kprobe style prog attached to many places and args of those places are stored in a map and the prog selects them with map_lookup with key=PT_REGS_IP ? And passes pointers into bpf_snprintf_btf() from PT_REGS_PARM1() ? I see why that is useful, but it's so racy. By the time the map is populated those btf_obj_id+btf_id could be invalid. I think instead of doing this in user space the program needs an access to vmlinux+mods BTFs. Sort-of like proposed bpf helper to return ksym based on IP there could be a helper to figure out btf_id+btf_obj_POINTER based on IP. Then there will no need for external map to populate. Would that solve your use case?
On Tue, 8 Dec 2020, Alexei Starovoitov wrote: > On Tue, Dec 08, 2020 at 10:13:35PM +0000, Alan Maguire wrote: > > > > Does this approach prevent more complex run-time specification of BTF > > object fd though? For example, I've been working on a simple tracer > > focused on kernel debugging; it uses a BPF map entry for each kernel > > function that is traced. User-space populates the map entry with BTF type > > ids for the function arguments/return value, and when the BPF program > > runs it uses the instruction pointer to look up the map entry for that > > function, and uses bpf_snprintf_btf() to write the string representations > > of the function arguments/return values. I'll send out an RFC soon, > > but longer-term I was hoping to extend it to support module-specific > > types. Would a dynamic case like that - where the BTF module fd is looked > > up in a map entry during program execution (rather than derived via > > __btf_builtin_type_id()) work too? Thanks! > > fd has to be resolved in the process context. bpf prog can read fd > number from the map, but that number is meaningless. > Say we allow using btf_obj_id+btf_id, how user space will know these > two numbers? Some new libbpf api that searches for it? > An extension to libbpf_find_vmlinux_btf_id() ? I was hoping that this api > will stay semi-internal. But say it's extended. > The user space will store a pair of numbers into a map and > what program are going to do with it? > If it's printing struct veth_stats contents it should have attached to > a corresponding function in the veth module via fentry or something. > The prog has hard coded logic in C with specific pointer to print. > The prog has its type right there. Why would the prog take a pointer > from one place, but it's type_id from the map? That's not realistic. > Where it would potentially make sense is what I think you're descring > where single kprobe style prog attached to many places and args of > those places are stored in a map and the prog selects them with > map_lookup with key=PT_REGS_IP ? Right, that's exactly it. A pair of generic tracing BPF programs are used, and they attach to kprobe/kretprobes, and when they run they use the arguments plus the map details about BTF ids of those arguments to run bpf_snprintf_btf(), and send perf events to userspace containing the results. > And passes pointers into bpf_snprintf_btf() from PT_REGS_PARM1() ? Exactly. > I see why that is useful, but it's so racy. By the time the map > is populated those btf_obj_id+btf_id could be invalid. > I think instead of doing this in user space the program needs an access > to vmlinux+mods BTFs. Sort-of like proposed bpf helper to return ksym > based on IP there could be a helper to figure out btf_id+btf_obj_POINTER > based on IP. Then there will no need for external map to populate. > Would that solve your use case? That would be fantastic! We could do that from the context passed into a kprobe program as the IP in struct pt_regs points at the function. kretprobes seems a bit trickier as in that case the IP in struct pt_regs is actually set to kretprobe_trampoline rather than the function we're returning from due to how kretprobes work; maybe there's another way to get it in that case though.. Alan
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 9be88a90a4aa..539956f7920a 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -4795,6 +4795,7 @@ static int load_module_btfs(struct bpf_object *obj) mod_btf = &obj->btf_modules[obj->btf_module_cnt++]; + btf__set_fd(btf, fd); mod_btf->btf = btf; mod_btf->id = id; mod_btf->fd = fd; @@ -5445,6 +5446,10 @@ struct bpf_core_relo_res __u32 orig_type_id; __u32 new_sz; __u32 new_type_id; + /* FD of the module BTF containing the target candidate, or 0 for + * vmlinux BTF + */ + int btf_obj_fd; }; /* Calculate original and target relocation values, given local and target @@ -5469,6 +5474,7 @@ static int bpf_core_calc_relo(const struct bpf_program *prog, res->fail_memsz_adjust = false; res->orig_sz = res->new_sz = 0; res->orig_type_id = res->new_type_id = 0; + res->btf_obj_fd = 0; if (core_relo_is_field_based(relo->kind)) { err = bpf_core_calc_field_relo(prog, relo, local_spec, @@ -5519,6 +5525,9 @@ static int bpf_core_calc_relo(const struct bpf_program *prog, } else if (core_relo_is_type_based(relo->kind)) { err = bpf_core_calc_type_relo(relo, local_spec, &res->orig_val); err = err ?: bpf_core_calc_type_relo(relo, targ_spec, &res->new_val); + if (!err && relo->kind == BPF_TYPE_ID_TARGET && + targ_spec->btf != prog->obj->btf_vmlinux) + res->btf_obj_fd = btf__fd(targ_spec->btf); } else if (core_relo_is_enumval_based(relo->kind)) { err = bpf_core_calc_enumval_relo(relo, local_spec, &res->orig_val); err = err ?: bpf_core_calc_enumval_relo(relo, targ_spec, &res->new_val); @@ -5725,10 +5734,14 @@ static int bpf_core_patch_insn(struct bpf_program *prog, } insn[0].imm = new_val; - insn[1].imm = 0; /* currently only 32-bit values are supported */ - pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %u\n", + /* btf_obj_fd is zero for all relos but BPF_TYPE_ID_TARGET + * with target type in the kernel module BTF + */ + insn[1].imm = res->btf_obj_fd; + pr_debug("prog '%s': relo #%d: patched insn #%d (LDIMM64) imm64 %llu -> %llu\n", prog->name, relo_idx, insn_idx, - (unsigned long long)imm, new_val); + (unsigned long long)imm, + ((unsigned long long)res->btf_obj_fd << 32) | new_val); break; } default:
When Clang emits ldimm64 instruction for BPF_TYPE_ID_TARGET CO-RE relocation, put module BTF FD, containing target type, into upper 32 bits of imm64. Because this FD is internal to libbpf, it's very cumbersome to test this in selftests. Manual testing was performed with debug log messages sprinkled across selftests and libbpf, confirming expected values are substituted. Better testing will be performed as part of the work adding module BTF types support to bpf_snprintf_btf() helpers. Cc: Alan Maguire <alan.maguire@oracle.com> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- tools/lib/bpf/libbpf.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)