From b3fc4647b1d39ebe0003faa06b390a802f66d4cf Mon Sep 17 00:00:00 2001 From: Patrick Pichler Date: Fri, 4 Oct 2024 22:50:19 +0200 Subject: [PATCH] fixup! feat: add support for untyped extern __ksym variables Address PR comments Signed-off-by: Patrick Pichler --- btf/btf.go | 3 +- collection.go | 1 + elf_reader.go | 39 +++++++++++++++++++------ elf_reader_test.go | 45 ++++++++++++++++++++++++----- internal/kallsyms/kallsyms.go | 22 +++++--------- internal/kallsyms/kallsyms_test.go | 40 +++++++++++-------------- linker.go | 27 ++++++----------- prog.go | 6 ++-- testdata/ksym-eb.elf | Bin 0 -> 3160 bytes testdata/ksym-el.elf | Bin 0 -> 3160 bytes testdata/ksym.c | 18 ++++++++++-- 11 files changed, 124 insertions(+), 77 deletions(-) create mode 100644 testdata/ksym-eb.elf create mode 100644 testdata/ksym-el.elf diff --git a/btf/btf.go b/btf/btf.go index 63130429b..eb7e61b23 100644 --- a/btf/btf.go +++ b/btf/btf.go @@ -443,7 +443,8 @@ func fixupDatasec(types []Type, sectionSizes map[string]uint32, offsets map[symb // Some Datasecs are virtual and don't have corresponding ELF sections. switch name { case ".ksyms": - // .ksyms describes forward declarations of kfunc signatures. + // .ksyms describes forward declarations of kfunc signatures, as wel as + // references to kernel symbols. // Nothing to fix up, all sizes and offsets are 0. for _, vsi := range ds.Vars { switch t := vsi.Type.(type) { diff --git a/collection.go b/collection.go index 0512d10f2..c36264270 100644 --- a/collection.go +++ b/collection.go @@ -315,6 +315,7 @@ func (cs *CollectionSpec) LoadAndAssign(to interface{}, opts *CollectionOptions) return nil } + // Collection is a collection of Programs and Maps associated // with their symbols type Collection struct { diff --git a/elf_reader.go b/elf_reader.go index e6d5f86c1..72d96ebe4 100644 --- a/elf_reader.go +++ b/elf_reader.go @@ -15,6 +15,7 @@ import ( "github.com/cilium/ebpf/asm" "github.com/cilium/ebpf/btf" "github.com/cilium/ebpf/internal" + "github.com/cilium/ebpf/internal/kallsyms" "github.com/cilium/ebpf/internal/sys" ) @@ -35,7 +36,13 @@ type kfuncMeta struct { type ksymMetaKey struct{} type ksymMeta struct { - name string + VarSpec *KsymVarSpec + Binding elf.SymBind +} + +type KsymVarSpec struct { + Var *btf.Var + Addr uint64 } // elfCode is a convenience to reduce the amount of arguments that have to @@ -49,7 +56,7 @@ type elfCode struct { extInfo *btf.ExtInfos maps map[string]*MapSpec vars map[string]*VariableSpec - ksymVars map[string]*btf.Var + ksymVars map[string]*KsymVarSpec kfuncs map[string]*btf.Func kconfig *MapSpec } @@ -143,7 +150,7 @@ func LoadCollectionSpecFromReader(rd io.ReaderAt) (*CollectionSpec, error) { maps: make(map[string]*MapSpec), vars: make(map[string]*VariableSpec), kfuncs: make(map[string]*btf.Func), - ksymVars: make(map[string]*btf.Var), + ksymVars: make(map[string]*KsymVarSpec), } symbols, err := f.Symbols() @@ -634,8 +641,6 @@ func (ec *elfCode) relocateInstruction(ins *asm.Instruction, rel elf.Symbol) err return fmt.Errorf("asm relocation: %s: unsupported type %s", name, typ) } - // TODO(patrick.pichler): handle me - kf := ec.kfuncs[name] ksymsVar := ec.ksymVars[name] @@ -660,12 +665,12 @@ func (ec *elfCode) relocateInstruction(ins *asm.Instruction, rel elf.Symbol) err ins.Constant = 0 case ksymsVar != nil && ins.OpCode.IsDWordLoad(): - if bind != elf.STB_GLOBAL { + if bind != elf.STB_GLOBAL && bind != elf.STB_WEAK { return fmt.Errorf("asm relocation: %s: %w: %s", name, errUnsupportedBinding, bind) } - println("==> set") ins.Metadata.Set(ksymMetaKey{}, &ksymMeta{ - rel.Name, + VarSpec: ksymsVar, + Binding: bind, }) // If no kconfig map is found, this must be a symbol reference from inline @@ -1305,7 +1310,9 @@ func (ec *elfCode) loadKsymsSection() error { case *btf.Func: ec.kfuncs[t.TypeName()] = t case *btf.Var: - ec.ksymVars[t.TypeName()] = t + ec.ksymVars[t.TypeName()] = &KsymVarSpec{ + Var: t, + } // This case should never happen, as we already checked that only vars and funcs // are present in the vars. default: @@ -1313,6 +1320,20 @@ func (ec *elfCode) loadKsymsSection() error { } } + symbolsToLoad := make([]string, 0, len(ec.ksymVars)) + for k := range ec.ksymVars { + symbolsToLoad = append(symbolsToLoad, k) + } + + ksymAddrs, err := kallsyms.LoadSymbolAddresses(symbolsToLoad) + if err != nil { + return fmt.Errorf("error while loading ksym addresses: %w", err) + } + + for ksym, addr := range ksymAddrs { + ec.ksymVars[ksym].Addr = addr + } + return nil } diff --git a/elf_reader_test.go b/elf_reader_test.go index 3cd332006..1fa6981d2 100644 --- a/elf_reader_test.go +++ b/elf_reader_test.go @@ -774,10 +774,6 @@ func TestKconfigConfig(t *testing.T) { } func TestKsym(t *testing.T) { - if !haveTestmod(t) { - t.Skip("bpf_testmod not loaded") - } - file := testutils.NativeFile(t, "testdata/ksym-%s.elf") spec, err := LoadCollectionSpec(file) if err != nil { @@ -803,19 +799,54 @@ func TestKsym(t *testing.T) { t.Fatal(err) } + ksyms, err := kallsyms.LoadSymbolAddresses([]string{ + "socket_file_ops", + "tty_fops", + }) + if err != nil { + t.Fatal(err) + } + var value uint64 err = obj.ArrayMap.Lookup(uint32(0), &value) if err != nil { t.Fatal(err) } - ksyms, err := kallsyms.LoadSymbolAddresses() + qt.Assert(t, qt.Equals(value, ksyms["socket_file_ops"])) + + err = obj.ArrayMap.Lookup(uint32(1), &value) + if err != nil { + t.Fatal(err) + } + qt.Assert(t, qt.Equals(value, ksyms["tty_fops"])) +} + +func TestKsymWeakMissing(t *testing.T) { + file := testutils.NativeFile(t, "testdata/ksym-%s.elf") + spec, err := LoadCollectionSpec(file) + if err != nil { + t.Fatal(err) + } + + var obj struct { + Main *Program `ebpf:"ksym_missing_test"` + } + + err = spec.LoadAndAssign(&obj, nil) + testutils.SkipIfNotSupported(t, err) + if err != nil { + t.Fatal(err) + } + defer obj.Main.Close() + + res, _, err := obj.Main.Test(internal.EmptyBPFContext) + testutils.SkipIfNotSupported(t, err) if err != nil { t.Fatal(err) } - // socket_file_ops must be present and the value we have in kallsyms - qt.Assert(t, qt.Not(qt.Equals(value, ksyms["socket_file_ops __ksym"]))) + qt.Assert(t, qt.Equals(res, 1)) } func TestKfunc(t *testing.T) { diff --git a/internal/kallsyms/kallsyms.go b/internal/kallsyms/kallsyms.go index 9b2dee620..5206f4ce0 100644 --- a/internal/kallsyms/kallsyms.go +++ b/internal/kallsyms/kallsyms.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "os" + "slices" "sync" ) @@ -79,23 +80,13 @@ func loadKernelModuleMapping(f io.Reader) (map[string]string, error) { return mods, nil } -func LoadSymbolAddresses() (map[string]uint64, error) { - kernelSymbols.RLock() - ksyms := kernelSymbols.ksyms - kernelSymbols.RUnlock() - - if ksyms == nil { - kernelSymbols.Lock() - defer kernelSymbols.Unlock() - ksyms = kernelSymbols.ksyms - } - +func LoadSymbolAddresses(symbols []string) (map[string]uint64, error) { f, err := os.Open("/proc/kallsyms") if err != nil { return nil, err } - ksyms, err = loadSymbolAddresses(f) + ksyms, err := loadSymbolAddresses(f, symbols) if err != nil { return nil, err } @@ -103,10 +94,11 @@ func LoadSymbolAddresses() (map[string]uint64, error) { return ksyms, nil } -func loadSymbolAddresses(f io.Reader) (map[string]uint64, error) { +func loadSymbolAddresses(f io.Reader, symbols []string) (map[string]uint64, error) { scan := bufio.NewScanner(f) result := make(map[string]uint64) + slices.Sort(symbols) for scan.Scan() { var ( @@ -121,7 +113,9 @@ func loadSymbolAddresses(f io.Reader) (map[string]uint64, error) { if err != nil { return nil, err } - result[symbol] = addr + if _, found := slices.BinarySearch(symbols, symbol); found { + result[symbol] = addr + } } if scan.Err() != nil { diff --git a/internal/kallsyms/kallsyms_test.go b/internal/kallsyms/kallsyms_test.go index 32d374600..97c657062 100644 --- a/internal/kallsyms/kallsyms_test.go +++ b/internal/kallsyms/kallsyms_test.go @@ -7,18 +7,19 @@ import ( "github.com/go-quicktest/qt" ) +var kallsyms = []byte(`0000000000000000 t hid_generic_probe [hid_generic] +00000000000000A0 T tcp_connect +00000000000000B0 B empty_zero_page +00000000000000C0 D kimage_vaddr +00000000000000D0 R __start_pci_fixups_early +00000000000000E0 V hv_root_partition +00000000000000F0 W calibrate_delay_is_known +A0000000000000AA a nft_counter_seq [nft_counter] +A0000000000000BA b bootconfig_found +A0000000000000CA d __func__.10 +A0000000000000DA r __ksymtab_LZ4_decompress_fast`) + func TestKernelModule(t *testing.T) { - kallsyms := []byte(`0000000000000000 t hid_generic_probe [hid_generic] -0000000000000000 T tcp_connect -0000000000000000 B empty_zero_page -0000000000000000 D kimage_vaddr -0000000000000000 R __start_pci_fixups_early -0000000000000000 V hv_root_partition -0000000000000000 W calibrate_delay_is_known -0000000000000000 a nft_counter_seq [nft_counter] -0000000000000000 b bootconfig_found -0000000000000000 d __func__.10 -0000000000000000 r __ksymtab_LZ4_decompress_fast`) krdr := bytes.NewBuffer(kallsyms) kmods, err := loadKernelModuleMapping(krdr) qt.Assert(t, qt.IsNil(err)) @@ -39,19 +40,12 @@ func TestKernelModule(t *testing.T) { } func TestLoadSymbolAddresses(t *testing.T) { - kallsyms := []byte(`0000000000000000 t hid_generic_probe [hid_generic] -00000000000000A0 T tcp_connect -00000000000000B0 B empty_zero_page -00000000000000C0 D kimage_vaddr -00000000000000D0 R __start_pci_fixups_early -00000000000000E0 V hv_root_partition -00000000000000F0 W calibrate_delay_is_known -A0000000000000AA a nft_counter_seq [nft_counter] -A0000000000000BA b bootconfig_found -A0000000000000CA d __func__.10 -A0000000000000DA r __ksymtab_LZ4_decompress_fast`) krdr := bytes.NewBuffer(kallsyms) - ksyms, err := loadSymbolAddresses(krdr) + ksyms, err := loadSymbolAddresses(krdr, []string{ + "hid_generic_probe", + "tcp_connect", + "bootconfig_found", + }) qt.Assert(t, qt.IsNil(err)) qt.Assert(t, qt.Equals(ksyms["hid_generic_probe"], 0)) diff --git a/linker.go b/linker.go index cf142b12b..0438732ea 100644 --- a/linker.go +++ b/linker.go @@ -14,7 +14,6 @@ import ( "github.com/cilium/ebpf/asm" "github.com/cilium/ebpf/btf" "github.com/cilium/ebpf/internal" - "github.com/cilium/ebpf/internal/kallsyms" ) // handles stores handle objects to avoid gc cleanup @@ -461,10 +460,8 @@ func resolveKconfigReferences(insns asm.Instructions) (_ *Map, err error) { } func resolveKsymReferences(insns asm.Instructions) error { - var ksymMap map[string]uint64 - var err error - iter := insns.Iterate() + var missingKsyms []string for iter.Next() { @@ -472,23 +469,17 @@ func resolveKsymReferences(insns asm.Instructions) error { if meta == nil { continue } - // We load the kallsyms map only when really needed - if ksymMap == nil { - ksymMap, err = kallsyms.LoadSymbolAddresses() - if err != nil { - return fmt.Errorf("error whild trying to load kallsyms: %v", err) - } - } - addr, found := ksymMap[meta.name] - if !found { - if !slices.Contains(missingKsyms, meta.name) { - missingKsyms = append(missingKsyms, meta.name) + switch { + case meta.VarSpec.Addr != 0: + iter.Ins.Constant = int64(meta.VarSpec.Addr) + case meta.VarSpec.Addr == 0 && meta.Binding == elf.STB_WEAK: + iter.Ins.Constant = 0 + default: + if slices.Contains(missingKsyms, meta.VarSpec.Var.TypeName()) { + missingKsyms = append(missingKsyms, meta.VarSpec.Var.TypeName()) } } - - iter.Ins.OpCode = asm.LoadImmOp(asm.DWord) - iter.Ins.Constant = int64(addr) } if len(missingKsyms) > 0 { diff --git a/prog.go b/prog.go index f7a0127f3..b1c4ab803 100644 --- a/prog.go +++ b/prog.go @@ -337,10 +337,10 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions) (*Program, er } defer kconfig.Close() - err = resolveKsymReferences(insns) - if err != nil { + err = resolveKsymReferences(insns) + if err != nil { return nil, fmt.Errorf("resolve .ksyms: %w", err) - } + } if err := fixupAndValidate(insns); err != nil { return nil, err diff --git a/testdata/ksym-eb.elf b/testdata/ksym-eb.elf new file mode 100644 index 0000000000000000000000000000000000000000..76702ad65d0e7326b8ae588633fc45ec57136435 GIT binary patch literal 3160 zcmbtWO=w(I6h1F)+Qc?(s;xv2eFO;!gqfrk6^bwoO>8kjr~&&U+`P=ZB+tBgGt7Ha z604Yv5_cj9E)-MgLdAs$5_jpc%PzajvLHxyc2OA5ci*`$*PDqhJaFzk-}$}wynFA= zC)2YtJv|;3SNA5Oq48 zr#l~NNXMHR@40z)MsoYk>-oH0$>&e=sYmjACchhR*5T^^GjE=szLI%6pAYQw++f}# zu^0ZA+d=HVb9KJ_*4(V5_}A%=?{us4zLNKdTMzyfK|Oio5uQBxAV+!jp@vcjZbz9h z_ZmNzeP)y67>h6TLBXGZdH4{&ezujx?-`cg5I?9Y%qWCfKJyH-nCWeiD?Go)+{?Vd ze2W>e_nCuJJTRSL)^P(G>Lsq8%yn9|ulHUS{b@D_QVhM&OSF1$hEri(9O^AeooDkp zUQ;ltagV$W2Y>G1Z@HLBeHKAXD1A;#x%~XT?A^oS1oLt=`K~$Iu!7famC#e}_J%>Ppb!2FFn~ zOwv%cUhIWTRZ1F(3fH1ELnZFSVk4#!lBQJR4W?u=5Xl|oZ>s4<9>G5|KhT=|JhoZJ z9R%S>?K0yP1jB~=1p0{iOQP$zlfK~Jh~|MO1RMJm!8=6nBlm{jKZ$Da-xU0WXa#sn zF#E^wf0$#B&v6F4D|j*wrN@F5(KqG{v#LAsr7-uxcKDHj=)FG6aF#Ei>%YtP%VD0 z9~_MJ+v|DY;710ZIDO{iz^OqhW#L*zrB)c1rsii#0Qv^&_vyf0){gRO6*ty-mgJfi zZ%_9d9mlupDyY?3{HR*bLMhE!S+GbYzVLdMh0*NP)TBDktN;J>1*dS&zhv!O7U37C zj5%LWLpIpG7Zo^kA#+|}GG_$xA@SZIj287-@&wB`gP@8YivfQjG|lmd-#oKV6OX)` z%x3+@&MJ_HIFNn+fQ>)oc?-Mo3vgia^|(Kn`q12Cw)5?Kk++e9Bi~y}h<5+B7$3HO zvwpLG(-2c1`!D|Iu-|zjeMnY2a@%jZnk!Ce6uT;Y&kCC*#aWQX=YSk4RZ8m^WMoqvDu1F`6Y#LOa$aNjCgt zTfPw{%|?~=&6+FaI4)OL?Am=l4ogY7Rvj6#>lzy6WV~aMBct9O`B+M;K?4Jh%B8Rx zhr0D_FOqhZYOU&r>*Y9MCro0o7D<6cV<}*Rv7{A5ct-h7NiQqkgM3H>h~Ib<%x{_0 z2i^nsE7QmGMEVH)CAjl_F*S*QCteVlfXw@674Jf}e`wDX{eOZxmqfPI|2Off$hKk= zABj9td=}qgdt%qb{7GOY@2lSjZu@)BgKtI3F0@MQfxHF9<&jHPA#XRr^XHzf4jT?1s#HSdsFgDmDfhA|o9 z_PDxjgO2>1A&LGj@HQ0?FDBNzm&44zpTmrQq?obnx1YnjkH^3y`sunz07~(!g&D?Y zWhnKpI(Xf|TMqu(!9O|pzJnh+__2deogV1v?dz9960RpwXoOK=YGI~8U{0{PpN^en z6R5XVc4D1tNsnoE_q0CIQFOcF2TMy0T&lTip%5pHBv_OJ4!n`NFqxa0n)EN|CjN)` z0=eeL$*Hl@WevYNc75%cm6&{VBCM-u#jz*fcb@p2JT1T9X{gJ}O7J}K`zvM^Rrn7C zXx<9pZyxuV=J%_83z99|8O&?nR_9Msjac}P{9Bsu<%6N8{ZsQ@D9#p*Yg3KiI}3B? zw~gpl`=4*If11+%>HeMnb4+hOUgIGL?OTyO-TtiEs{gk3f1sJ1KjYl~Jm(#Ahc$>z ia7MQ@P|IX?cUHUy(%-Wz2ui