diff mbox series

[v2,bpf-next] libbpf: support module BTF for BPF_TYPE_ID_TARGET CO-RE relocation

Message ID 20201208235332.354826-1-andrii@kernel.org (mailing list archive)
State New, archived
Delegated to: BPF
Headers show
Series [v2,bpf-next] libbpf: support module BTF for BPF_TYPE_ID_TARGET CO-RE relocation | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 50 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Andrii Nakryiko Dec. 8, 2020, 11:53 p.m. UTC
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(-)

Comments

Alan Maguire Dec. 9, 2020, 11:08 p.m. UTC | #1
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)) {
Andrii Nakryiko Dec. 10, 2020, 12:11 a.m. UTC | #2
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 mbox series

Patch

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: