Message-ID: <alpine.LRH.2.23.451.2012092249520.26400@localhost>
Date:   Wed, 9 Dec 2020 23:08:02 +0000 (GMT)
From:   Alan Maguire <alan.maguire@...cle.com>
To:     Andrii Nakryiko <andrii@...nel.org>
cc:     bpf@...r.kernel.org, netdev@...r.kernel.org, ast@...com,
        daniel@...earbox.net, kernel-team@...com,
        Alan Maguire <alan.maguire@...cle.com>
Subject: Re: [PATCH v2 bpf-next] libbpf: support module BTF for BPF_TYPE_ID_TARGET
 CO-RE relocation

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@...cle.com>
> Signed-off-by: Andrii Nakryiko <andrii@...nel.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)) {

Powered by blists - more mailing lists