| Message ID | 20201208235332.354826-1-andrii@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,bpf-next] libbpf: support module BTF for BPF_TYPE_ID_TARGET CO-RE relocation | expand |
On Tue, 8 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. > > v1->v2: > - fix crash on failing to resolve target spec (Alan). > > Cc: Alan Maguire <alan.maguire@oracle.com> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Thanks for this! Can confirm the segmentation fault has gone away. I tested with the veth_stats_rx program (though will switch to btf_test module later), and I still see the issue with a local kind of fwd for veth_stats leading to an inability to find the target kind in the module BTF: libbpf: sec 'kprobe/veth_stats_rx': found 5 CO-RE relocations libbpf: prog 'veth_stats_rx': relo #0: kind <target_type_id> (7), spec is [20] fwd veth_stats libbpf: prog 'veth_stats_rx': relo #0: no matching targets found libbpf: prog 'veth_stats_rx': relo #0: patched insn #3 (LDIMM64) imm64 20 -> 0 libbpf: prog 'veth_stats_rx': relo #1: kind <target_type_id> (7), spec is [20] fwd veth_stats libbpf: prog 'veth_stats_rx': relo #1: no matching targets found libbpf: prog 'veth_stats_rx': relo #1: patched insn #5 (LDIMM64) imm64 20 -> 0 Here's the same debug info with a patch on top of yours that loosens the constraints on kind matching such that a fwd local type will match a struct target type: libbpf: prog 'veth_stats_rx': relo #0: kind <target_type_id> (7), spec is [20] fwd veth_stats libbpf: CO-RE relocating [0] fwd veth_stats: found target candidate [91516] struct veth_stats in [veth] libbpf: prog 'veth_stats_rx': relo #0: matching candidate #0 [91516] struct veth_stats libbpf: prog 'veth_stats_rx': relo #0: patched insn #3 (LDIMM64) imm64 20 -> 450971657596 libbpf: prog 'veth_stats_rx': relo #1: kind <target_type_id> (7), spec is [20] fwd veth_stats libbpf: prog 'veth_stats_rx': relo #1: matching candidate #0 [91516] struct veth_stats libbpf: prog 'veth_stats_rx': relo #1: patched insn #5 (LDIMM64) imm64 20 -> 450971657596 Patch is below; if it makes sense to support loosening constraints on kind matching like this feel free to roll it into your patch or I can send a follow-up, whatever's easiest. Thanks! diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 2fb9824..9ead5b3 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -4673,6 +4673,23 @@ static void bpf_core_free_cands(struct core_cand_list *ca free(cands); } +/* module-specific structs may have relo kind set to fwd, so as + * well as handling exact matches, a fwd kind has to match + * a target struct kind. + */ +static bool kind_matches_target(const struct btf_type *local, + const struct btf_type *target) +{ + __u8 local_kind = btf_kind(local); + __u8 target_kind = btf_kind(target); + + if (local_kind == target_kind) + return true; + if (local_kind == BTF_KIND_FWD && target_kind == 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 +4706,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 (!kind_matches_target(local_cand->t, t)) continue; targ_name = btf__name_by_offset(targ_btf, t->name_off); @@ -5057,7 +5074,7 @@ static int bpf_core_types_are_compat(const struct btf *loc /* 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 (!kind_matches_target(local_type, targ_type)) return 0; recur: @@ -5070,7 +5087,7 @@ static int bpf_core_types_are_compat(const struct btf *loc if (!local_type || !targ_type) return -EINVAL; - if (btf_kind(local_type) != btf_kind(targ_type)) + if (!kind_matches_target(local_type, targ_type)) return 0; switch (btf_kind(local_type)) {
On Wed, Dec 9, 2020 at 3:10 PM Alan Maguire <alan.maguire@oracle.com> wrote: > > On Tue, 8 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. > > > > v1->v2: > > - fix crash on failing to resolve target spec (Alan). > > > > Cc: Alan Maguire <alan.maguire@oracle.com> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > Thanks for this! > > Can confirm the segmentation fault has gone away. I tested with the > veth_stats_rx program (though will switch to btf_test module later), > and I still see the issue with a local kind of fwd for veth_stats > leading to an inability to find the target kind in the module BTF: Yes, this patch wasn't intended to change that part of libbpf logic. For the FWD -> STRUCT/UNION (you forgot about union, btw) "resolution". For your use case, where does the veth_stats forward declaration come from? Is it coming from vmlinux.h as forward-declared? You can easily "work around" that by defining struct on your own, even the empty one: struct veth_stats {} That should make local veth_stats definition a concrete struct, not fwd. The idea behind this strictness was that you have a local expected *concrete* definition of the type you need to match against the kernel type, not just its name (which is the only thing that is recorded with FWD). So I'm still hesitant about that FWD -> STRUCT/UNION resolution. BTF dedup, btw, doesn't match just by name when it resolves FWD to STRUCT/UNION, it has considerably more complicated logic to do the resolution. So just make sure you have non-fwd declaration of the type you work with. > > libbpf: sec 'kprobe/veth_stats_rx': found 5 CO-RE relocations > libbpf: prog 'veth_stats_rx': relo #0: kind <target_type_id> (7), spec is > [20] fwd veth_stats > libbpf: prog 'veth_stats_rx': relo #0: no matching targets found > libbpf: prog 'veth_stats_rx': relo #0: patched insn #3 (LDIMM64) imm64 20 > -> 0 > libbpf: prog 'veth_stats_rx': relo #1: kind <target_type_id> (7), spec is > [20] fwd veth_stats > libbpf: prog 'veth_stats_rx': relo #1: no matching targets found > libbpf: prog 'veth_stats_rx': relo #1: patched insn #5 (LDIMM64) imm64 20 > -> 0 > > Here's the same debug info with a patch on top of yours that loosens the > constraints on kind matching such that a fwd local type will match a struct > target type: > > libbpf: prog 'veth_stats_rx': relo #0: kind <target_type_id> (7), spec is > [20] fwd veth_stats > libbpf: CO-RE relocating [0] fwd veth_stats: found target candidate > [91516] struct veth_stats in [veth] > libbpf: prog 'veth_stats_rx': relo #0: matching candidate #0 [91516] > struct veth_stats > libbpf: prog 'veth_stats_rx': relo #0: patched insn #3 (LDIMM64) imm64 20 > -> 450971657596 > libbpf: prog 'veth_stats_rx': relo #1: kind <target_type_id> (7), spec is > [20] fwd veth_stats > libbpf: prog 'veth_stats_rx': relo #1: matching candidate #0 [91516] > struct veth_stats > libbpf: prog 'veth_stats_rx': relo #1: patched insn #5 (LDIMM64) imm64 20 > -> 450971657596 > > Patch is below; if it makes sense to support loosening constraints on kind > matching like this feel free to roll it into your patch or I can send a > follow-up, whatever's easiest. Thanks! > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 2fb9824..9ead5b3 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -4673,6 +4673,23 @@ static void bpf_core_free_cands(struct > core_cand_list *ca > free(cands); > } > > +/* module-specific structs may have relo kind set to fwd, so as > + * well as handling exact matches, a fwd kind has to match > + * a target struct kind. > + */ > +static bool kind_matches_target(const struct btf_type *local, > + const struct btf_type *target) > +{ > + __u8 local_kind = btf_kind(local); > + __u8 target_kind = btf_kind(target); > + > + if (local_kind == target_kind) > + return true; > + if (local_kind == BTF_KIND_FWD && target_kind == 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 +4706,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 (!kind_matches_target(local_cand->t, t)) > continue; > > targ_name = btf__name_by_offset(targ_btf, t->name_off); > @@ -5057,7 +5074,7 @@ static int bpf_core_types_are_compat(const struct > btf *loc > /* 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 (!kind_matches_target(local_type, targ_type)) > return 0; > > recur: > @@ -5070,7 +5087,7 @@ static int bpf_core_types_are_compat(const struct > btf *loc > if (!local_type || !targ_type) > return -EINVAL; > > - if (btf_kind(local_type) != btf_kind(targ_type)) > + if (!kind_matches_target(local_type, targ_type)) > return 0; > > switch (btf_kind(local_type)) { > >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 9be88a90a4aa..2fb9824bf9bf 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 && 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. v1->v2: - fix crash on failing to resolve target spec (Alan). 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(-)