cilium
cilium copied to clipboard
cilium
init.sh: migrate alignchecker to use BTF instead of DWARF
With cilium/ebpf's btf/ package now being public, there is no longer a reason to compile datapath ELFs with DWARF data and parsing it in userspace. We're compiling ELFs with -g since a few releases, so BTF presence can be assumed.
Additionally, we don't really need to build a separate alignchecker BPF ELF. Perform align checking on the actual datapath ELFs and remove bpf_alignchecker.c. This means we no longer need to keep the list up-to-date manually.
Hi @ti-mo I could help here but I also need guidance since I am new to cilium/ebpf, btf, and this whole alignment check thing :). I see this is part of deprecate init.sh, so I looked at bpf/init.sh
function bpf_compile()
{
IN=$1
OUT=$2
TYPE=$3
EXTRA_OPTS=$4
clang -O2 -target bpf -std=gnu89 -nostdinc -emit-llvm \
-g -Wall -Wextra -Werror -Wshadow \
-Wno-address-of-packed-member \
-Wno-unknown-warning-option \
-Wno-gnu-variable-sized-type-not-at-end \
-Wdeclaration-after-statement \
-Wimplicit-int-conversion -Wenum-conversion \
-I. -I$DIR -I$LIB -I$LIB/include \
-D__NR_CPUS__=$NR_CPUS \
-DENABLE_ARP_RESPONDER=1 \
$EXTRA_OPTS \
-c $LIB/$IN -o - | \
llc -march=bpf -mcpu=$MCPU -mattr=dwarfris -filetype=$TYPE -o $OUT
}
is this llc -mattr=dwarfris that you talk about to deprecate?
in addition, are pkg/alignchecker, pkg/datapath/alignchecker, and pkg/monitor/alignchecker you talk about to remove? I could get myself familiar with these part of codes first if that are the goal of this request
I did the removal part in my draft PR https://github.com/cilium/cilium/pull/20788, not sure how to add cilium/ebpf btf part for alignment, is it done automatically already when loader use cilium/ebpf to extract BTF info from BPF object ?
just trying to understand what you mean :)
With
cilium/ebpf's btf/ package now being public, there is no longer a reason to compile datapath ELFs with DWARF data and parsing it in userspace. We're compiling ELFs with-gsince a few releases, so BTF presence can be assumed.
I kind of understand to replace DWARF with BTF, it seems to me to replace getDWARFFromELF(f) and getStructInfosFromDWARF(d, toCheck) in CheckStructAlignments() in pkg/alignchecker/alignchecker.go
Additionally, we don't really need to build a separate alignchecker BPF ELF. Perform align checking on the actual datapath ELFs and remove
bpf_alignchecker.c. This means we no longer need to keep the list up-to-date manually.
if bpf_alignchecker.c is to be removed, which datapath ELF object file should alignchecker check? there will be no one single datapath ELF object like bpf_alignchecker.o to include all the cilium map struct for alignchecker to check alignment. I guess you have some idea that I am missing
pkg/datapath/loader/base.go
if l.canDisableDwarfRelocations {
// Validate alignments of C and Go equivalent structs
if err := alignchecker.CheckStructAlignments(defaults.AlignCheckerName); err != nil {
log.WithError(err).Fatal("C and Go structs alignment check failed")
}
} else {
log.Warning("Cannot check matching of C and Go common struct alignments due to old LLVM/clang version")
}
it appears bpf_alignchecker.o is missing quite a few STRUCTs like lb4_key for example from bpftool btf dump, simply replace DWARF with BTF would miss quite a few struct alignment check
bpftool btf dump file /var/run/cilium/state/bpf_alignchecker.o | grep STRUCT
[1] STRUCT '(anon)' size=40 vlen=5
[11] STRUCT '(anon)' size=48 vlen=6
[13] STRUCT 'endpoint_key' size=20 vlen=4
[15] STRUCT '(anon)' size=16 vlen=4
[19] STRUCT '(anon)' size=16 vlen=4
[20] STRUCT '(anon)' size=16 vlen=2
[29] STRUCT 'endpoint_info' size=48 vlen=7
[35] STRUCT '(anon)' size=48 vlen=6
[39] STRUCT 'metrics_key' size=8 vlen=4
[42] STRUCT 'metrics_value' size=16 vlen=2
[44] STRUCT '(anon)' size=48 vlen=6
[48] STRUCT 'ipcache_key' size=24 vlen=5
[49] STRUCT 'bpf_lpm_trie_key' size=4 vlen=2
[52] STRUCT '(anon)' size=16 vlen=4
[54] STRUCT 'remote_endpoint_info' size=12 vlen=3
[58] STRUCT '(anon)' size=40 vlen=5
[63] STRUCT 'encrypt_config' size=1 vlen=1
[65] STRUCT '(anon)' size=48 vlen=6
[67] STRUCT 'vtep_key' size=4 vlen=1
[69] STRUCT 'vtep_value' size=16 vlen=2
[71] STRUCT '(anon)' size=40 vlen=5
[73] STRUCT '(anon)' size=40 vlen=5
[78] STRUCT '(anon)' size=40 vlen=5
[80] STRUCT 'capture_cache' size=6 vlen=3
[83] STRUCT '(anon)' size=40 vlen=5
[87] STRUCT 'sock_key' size=44 vlen=7
[89] STRUCT '(anon)' size=16 vlen=4
[91] STRUCT '(anon)' size=16 vlen=4
[97] STRUCT '__sk_buff' size=184 vlen=32
[119] STRUCT 'bpf_elf_map' size=36 vlen=9
@vincentmli Not sure how did you compile the bpf_alignchecker.o, but I find it:
[103] STRUCT 'lb4_key' size=12 vlen=6
'address' type_id=99 bits_offset=0
'dport' type_id=82 bits_offset=32
'backend_slot' type_id=26 bits_offset=48
'proto' type_id=23 bits_offset=64
'scope' type_id=23 bits_offset=72
'pad' type_id=87 bits_offset=80
@brb do you have all off them declared in bpf_alignchecker.c? there is difference I noticed, make -C bpf and check bpf/bpf_alignchecker.o, it has lb4_key, but still some of them are missing. when actually deploy cilium agent in k8s node, the run time /var/run/cilium/state/bpf_alignchecker.o which is actually checked by pkg/alignchecker is missing lb4_key and other structs, I added log info in draft PR to confirm https://github.com/cilium/cilium/pull/20809/commits/fe345bcd6c2c4d826b9fd96bf6420136104c19e2
note the dwarf debug info has all the structs from /var/run/cilium/state/bpf_alignchecker.o
I double checked bpf/bpf_alignchecker.o compiled by make -C bpf, it indeed include all the structs. but at run time checking /var/run/cilium/state/bpf_alignchecker.o which I assume is compiled by bpf_compile from bpf/init.sh, it is missing quite a lot of struct from bpftool btf dump, I have not figured out the difference between make -C bpf where clang and llc flag is defined in bpf/Makefile.bpf and bpf_compile yet
# Compile dummy BPF file containing all shared struct definitions used by
# pkg/alignchecker to validate C and Go equivalent struct alignments
bpf_compile bpf_alignchecker.c bpf_alignchecker.o obj ""
and the question is if I remove bpf_compile bpf_alignchecker.c bpf_alignchecker.o obj "" from bpf/init.sh, how do I make sure the run time /var/run/cilium/state/bpf_alignchecker.o is compiled similar to make -C bpf?
I double checked bpf/bpf_alignchecker.o compiled by
make -C bpf, it indeed include all the structs. but at run time checking/var/run/cilium/state/bpf_alignchecker.owhich I assume is compiled bybpf_compilefrombpf/init.sh, it is missing quite a lot of struct from bpftool btf dump, I have not figured out the difference betweenmake -C bpfwhere clang and llc flag is defined inbpf/Makefile.bpfandbpf_compileyet
well, speak too early about make -C bpf having all structs, after I did make clean -C bpf; make -C bpf; bpftool btf dump file bpf/bpf_alignchecker.o | grep 'STRUCT' | grep -v 'anon'; structs are still missing bpf/bpf_alignchecker.o
make clean -C bpf
make: Entering directory '/home/go/src/github.com/vincentmli/cilium/bpf'
make -C sockops clean; make -C custom clean;
make[1]: Entering directory '/home/go/src/github.com/vincentmli/cilium/bpf/sockops'
rm -fr *.o *.ll *.i
make[1]: Leaving directory '/home/go/src/github.com/vincentmli/cilium/bpf/sockops'
make[1]: Entering directory '/home/go/src/github.com/vincentmli/cilium/bpf/custom'
rm -fr *.o *.ll *.i
make[1]: Leaving directory '/home/go/src/github.com/vincentmli/cilium/bpf/custom'
rm -fr *.o *.ll *.i *.s
rm -f cilium-probe-kernel-hz
make: Leaving directory '/home/go/src/github.com/vincentmli/cilium/bpf'
[root@centos-dev cilium]# ls -l bpf/bpf_alignchecker.o
ls: cannot access 'bpf/bpf_alignchecker.o': No such file or directory
[root@centos-dev cilium]# make -C bpf
make[1]: Leaving directory '/home/go/src/github.com/vincentmli/cilium/bpf/custom'
make: Leaving directory '/home/go/src/github.com/vincentmli/cilium/bpf'
[root@centos-dev cilium]#
[root@centos-dev cilium]#
[root@centos-dev cilium]#
[root@centos-dev cilium]#
[root@centos-dev cilium]# bpftool btf dump file bpf/bpf_alignchecker.o | grep 'STRUCT' | grep -v 'anon'
[12] STRUCT 'endpoint_key' size=20 vlen=4
[28] STRUCT 'endpoint_info' size=48 vlen=7
[38] STRUCT 'metrics_key' size=8 vlen=4
[41] STRUCT 'metrics_value' size=16 vlen=2
[47] STRUCT 'ipcache_key' size=24 vlen=5
[48] STRUCT 'bpf_lpm_trie_key' size=4 vlen=2
[53] STRUCT 'remote_endpoint_info' size=12 vlen=3
[62] STRUCT 'encrypt_config' size=1 vlen=1
[66] STRUCT 'vtep_key' size=4 vlen=1
[68] STRUCT 'vtep_value' size=16 vlen=2
[80] STRUCT 'lb6_reverse_nat' size=18 vlen=2
[85] STRUCT 'lb6_key' size=24 vlen=6
[88] STRUCT 'lb6_service' size=12 vlen=6
[93] STRUCT 'lb6_backend' size=20 vlen=4
[97] STRUCT 'lb4_reverse_nat' size=6 vlen=2
[102] STRUCT 'lb4_key' size=12 vlen=6
[104] STRUCT 'lb4_service' size=12 vlen=6
[109] STRUCT 'lb4_backend' size=8 vlen=4
[113] STRUCT 'capture_cache' size=6 vlen=3
[118] STRUCT 'capture4_wcard' size=16 vlen=8
[120] STRUCT 'capture_rule' size=8 vlen=3
[126] STRUCT 'capture6_wcard' size=40 vlen=8
[132] STRUCT 'sock_key' size=44 vlen=7
[143] STRUCT '__sk_buff' size=184 vlen=32
[171] STRUCT 'bpf_elf_map' size=36 vlen=9
[root@centos-dev cilium]# clang -v clang version 15.0.0 (https://github.com/llvm/llvm-project.git e91a73de24d60954700d7ac0293c050ab2cbe90b) Target: x86_64-unknown-linux-gnu Thread model: posix InstalledDir: /usr/local/bin Found candidate GCC installation: /usr/lib/gcc/x86_64-redhat-linux/8 Selected GCC installation: /usr/lib/gcc/x86_64-redhat-linux/8 Candidate multilib: .;@m64 Candidate multilib: 32;@m32 Selected multilib: .;@m64
and kernel version
[root@centos-dev cilium]# uname -a Linux centos-dev.localdomain 5.18.0 #98 SMP PREEMPT_DYNAMIC Sun Jul 3 21:42:54 EDT 2022 x86_64 x86_64 x86_64 GNU/Linux
a side note, I found that pahole -JV <object> could convert dwarf to btf as workaround
@vincentmli It is still not clear to me which struct definition in BTF is missing. It would be great to know.
Note that BTF generated by the llvm compiler with -target bpf may contain less types than pahole -JV <object> types. The reason is -target bpf only generates BTF with global variables, functions definitions and CO-RE related types, some types inside the program will not be generated for BTF. This is to only have needed types for bpf programs so far.
dwarf actually contains all types in the program so pahole -JV <object> may generate more types.
Just give a simple example,
$ cat t1.c
struct t {
int a;
};
void bar(void *);
int foo() {
struct t tmp;
bar(&tmp);
return 0;
}
$ clang -target bpf -g -O2 -c t1.c
$ bpftool btf dump file t1.o
[1] FUNC_PROTO '(anon)' ret_type_id=2 vlen=0
[2] INT 'int' size=4 bits_offset=0 nr_bits=32 encoding=SIGNED
[3] FUNC 'foo' type_id=1 linkage=global
[4] FUNC_PROTO '(anon)' ret_type_id=0 vlen=1
'(anon)' type_id=5
[5] PTR '(anon)' type_id=0
[6] FUNC 'bar' type_id=4 linkage=extern
In the above definition, there is no struct t definition. But the below pahole -JV does contain struct t:
$ pahole -JV t1.o
btf_encoder__new: 't1.o' doesn't have '.data..percpu' section
Found 0 per-CPU variables!
Found 1 functions!
File t1.o:
[1] PTR (anon) type_id=0
[2] INT int size=4 nr_bits=32 encoding=SIGNED
[3] STRUCT t size=4
a type_id=2 bits_offset=0
[4] FUNC_PROTO (anon) return=2 args=(void)
[5] FUNC foo type_id=4
thanks for the clarification, to my knowledge, I could not tell the difference between missing structs and non-missing structs :)
these are the missing structs for example, I did not list all the missing structs
egress_gw_policy_entry
egress_gw_policy_key
ipv4_ct_tuple
ipv4_frag_id
ipv4_frag_l4ports
ipv4_nat_entry
ipv4_revnat_entry
ipv4_revnat_tuple
ipv6_ct_tuple
ipv6_nat_entry
ipv6_revnat_entry
ipv6_revnat_tuple
all declared structs in bpf/bpf_alignchecker.c
grep 'DECLARE(struct' bpf/bpf_alignchecker.c | awk '{print $2;}' | sort
capture4_wcard);
capture6_wcard);
capture_rule);
ct_entry);
debug_capture_msg);
debug_msg);
drop_notify);
edt_id);
edt_info);
egress_gw_policy_entry);
egress_gw_policy_key);
endpoint_info);
endpoint_key);
ipcache_key);
ipv4_ct_tuple);
ipv4_frag_id);
ipv4_frag_l4ports);
ipv4_nat_entry);
ipv4_revnat_entry);
ipv4_revnat_tuple);
ipv6_ct_tuple);
ipv6_nat_entry);
ipv6_revnat_entry);
ipv6_revnat_tuple);
lb4_affinity_key);
lb4_backend);
lb4_key);
lb4_service);
lb4_src_range_key);
lb6_affinity_key);
lb6_backend);
lb6_key);
lb6_service);
lb6_src_range_key);
lb_affinity_match);
lb_affinity_val);
metrics_key);
metrics_value);
policy_entry);
policy_key);
policy_verdict_notify);
remote_endpoint_info);
sock_key);
srv6_policy_key4);
srv6_policy_key6);
srv6_vrf_key4);
srv6_vrf_key6);
trace_notify);
vtep_key);
vtep_value);
bpftool btf dump file bpf/bpf_alignchecker.o | grep 'STRUCT' | grep -v 'anon' | awk '{print $3;}' | sort
'bpf_elf_map'
'bpf_lpm_trie_key'
'capture4_wcard'
'capture6_wcard'
'capture_cache'
'capture_rule'
'encrypt_config'
'endpoint_info'
'endpoint_key'
'ipcache_key'
'lb4_backend'
'lb4_key'
'lb4_reverse_nat'
'lb4_service'
'lb6_backend'
'lb6_key'
'lb6_reverse_nat'
'lb6_service'
'metrics_key'
'metrics_value'
'remote_endpoint_info'
'__sk_buff'
'sock_key'
'vtep_key'
'vtep_value'
@vincentmli Based on the following, you might need to change DECLARE so that it defines e.g., global vars.
Note that BTF generated by the llvm compiler with -target bpf may contain less types than pahole -JV
@brb @yonghong-song sorry my C skill is rough and I tried to move all the DECLARE statement out of main(), think that might make all these structs global variable, but I got macro expand error, also tried to change DECLARE macro, still got syntax error due to my rough C skills, could you show an example of making the struct as global variable in bpf/bpf_alignchecker.c or example of changing macro DECLARE
for example, here is the probably the dumbest change I could think of and I suspect it seriously flawed :-), comment out the DECLARE macro, define a global variable for each struct, sort of work.
diff --git a/bpf/bpf_alignchecker.c b/bpf/bpf_alignchecker.c
index 46f5e1b7b2..0d6cec466d 100644
--- a/bpf/bpf_alignchecker.c
+++ b/bpf/bpf_alignchecker.c
@@ -32,17 +32,21 @@
* to the var to a BPF helper function which accepts a reference as
* an argument.
*/
+/*
#define DECLARE(type) \
{ \
type s = {}; \
trace_printk("%p", 1, &s); \
}
-
+*/
/* This function is a placeholder for C struct definitions shared with Go,
* it is never executed.
*/
+struct ipv4_ct_tuple a;
+struct ipv6_ct_tuple b;
int main(void)
{
+ /*
DECLARE(struct ipv4_ct_tuple);
DECLARE(struct ipv6_ct_tuple);
DECLARE(struct ct_entry);
@@ -94,6 +98,7 @@ int main(void)
DECLARE(struct srv6_vrf_key6);
DECLARE(struct srv6_policy_key4);
DECLARE(struct srv6_policy_key6);
+ */
return 0;
}
another observation, I noticed for specific feature related structs for example struct egress_gw_policy_key and struct egress_gw_policy_entry that are missing from BTF, add #define ENABLE_EGRESS_GATEWAY in bpf/bpf_alignchecker.c would resolve the missing BTF struct issue, seems like a simple workaround, but still for these common structs like struct ipv4_ct_tuple that are not specific to a feature and are missing from BTF, still an issue.
it seems to me cilium is required to do alignment check for cilium maps with key or value struct.
41 func CheckStructAlignments(path string) error {
42 // Validate alignments of C and Go equivalent structs
43 toCheck := map[string][]reflect.Type{
44 "ipv4_ct_tuple": {reflect.TypeOf(ctmap.CtKey4{}), reflect.TypeOf(ctmap.CtKey4Global{})},
45 "ipv6_ct_tuple": {reflect.TypeOf(ctmap.CtKey6{}), reflect.TypeOf(ctmap.CtKey6Global{})},
46 "ct_entry": {reflect.TypeOf(ctmap.CtEntry{})},
47 "ipcache_key": {reflect.TypeOf(ipcachemap.Key{})},
............
but I noticed the go struct ctmap.CtKey4{} does not have tag
// CtKey4 is needed to provide CtEntry type to Lookup values
// +k8s:deepcopy-gen=true
// +k8s:deepcopy-gen:interfaces=github.com/cilium/cilium/pkg/bpf.MapKey
type CtKey4 struct {
tuple.TupleKey4
}
the structs members without tag will be bypassed for alignment check
for i := 0; i < g.NumField(); i++ {
fieldName := g.Field(i).Tag.Get("align")
// Ignore fields without `align` struct tag. <=======
if fieldName == "" {
continue
}
goOffset := int64(g.Field(i).Offset)
cOffset := structs[name].fieldOffsets[fieldName]
if goOffset != cOffset {
return fmt.Errorf("%s.%s offset(%d) does not match %s.%s(%d)",
g, g.Field(i).Name, goOffset, name, fieldName, cOffset)
}
}
so it means cilium is not doing alignment check for struct ipv4_ct_tuple, is this bug or done on purpose? also the new SRV6 feature related go structs are also missing tags.
@brb @ti-mo @pchaigno @joestringer do you have idea?
I don't have a good solution for the mismatch between dwarf->btf and llvm btf types. You could have one global definition like
union __unused_union {
struct a a;
struct b b;
...
} __unused_var;
But this is just a workaround. Since these types are not used in bpf program. Not sure why cilium needs them. Probably there is a reason for this.
@yonghong-song I like your workaround, the bpf_aligncheck.o is solely used by cilium to check struct alignment between C struct and Go struct when cilium agent start up, it is not used in cilium datapath.
I guess @ti-mo idea is to get rid of bpf_alignchecker.o completely and let cilium alignment checker to check the actual datapath object files (bpf_lxc.o, bpf_host.o, bpf_sock.o...) at start up, so no need to manually add each struct in bpf_alignchecker.c, but pkg/datapath/alignchecker/alignchecker.go is to manually add Go struct anyway, if only remove the C struct manual part (bpf_alignchecker.o), but keep the Go struct manual part, in my opinion I don't see much benefit
Hi, just read up on the thread.
Not sure why you're seeing missing types in the objects you built. Declaring a global variable like struct ipv4_ct_tuple _1; in the top scope results in type info STRUCT 'ipv4_ct_tuple' being emitted. If a type is compiled out due missing #define ENABLE_EGRESS_GATEWAY, that results in a compilation error when trying to declare a variable of that type.
Some types are not checked, presumably because whoever added them did not add struct annotations.
My thinking was that eventually, we could lift alignment checking for, e.g. the ipcache map into the ipcache package.
On one hand, loading a map definition from an ELF automatically exposes its type info in MapSpec.Key and .Value, but we have many packages that create maps directly from Go, pin them, and rely on the loader to load programs from ELF and pick up pinned maps. The ipcache package doesn't handle an ELF at any point, so it doesn't have access to the C types to compare.
I have a few things in mind on how we could reconcile this, but let's focus on getting #20809 merged. Getting this out of init.sh and using BTF is already a great step.