Re: [PATCH bpf-next] libbpf: support module BTF for BPF_TYPE_ID_TARGET CO-RE relocation

[Alan Maguire <alan.maguire@oracle.com>]

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)) {
-- 
1.8.3.1