diff --git a/Makefile b/Makefile index 3727a6e11..c55a93d9c 100644 --- a/Makefile +++ b/Makefile @@ -44,6 +44,7 @@ TARGETS := \ testdata/invalid-kfunc \ testdata/kfunc-kmod \ testdata/constants \ + testdata/errors \ btf/testdata/relocs \ btf/testdata/relocs_read \ btf/testdata/relocs_read_tgt \ diff --git a/btf/btf_test.go b/btf/btf_test.go index ab0464eb4..222c0de9f 100644 --- a/btf/btf_test.go +++ b/btf/btf_test.go @@ -308,7 +308,9 @@ func TestLoadSpecFromElf(t *testing.T) { } func TestVerifierError(t *testing.T) { - _, err := NewHandle(&Builder{}) + b, err := NewBuilder([]Type{&Int{Encoding: 255}}) + qt.Assert(t, qt.IsNil(err)) + _, err = NewHandle(b) testutils.SkipIfNotSupported(t, err) var ve *internal.VerifierError if !errors.As(err, &ve) { diff --git a/btf/core.go b/btf/core.go index dc5baf55e..d53cf9168 100644 --- a/btf/core.go +++ b/btf/core.go @@ -15,6 +15,11 @@ import ( // Code in this file is derived from libbpf, which is available under a BSD // 2-Clause license. +// A constant used when CO-RE relocation has to remove instructions. +// +// Taken from libbpf. +const COREBadRelocationSentinel = 0xbad2310 + // COREFixup is the result of computing a CO-RE relocation for a target. type COREFixup struct { kind coreKind @@ -41,17 +46,26 @@ func (f *COREFixup) String() string { func (f *COREFixup) Apply(ins *asm.Instruction) error { if f.poison { - const badRelo = 0xbad2310 - // Relocation is poisoned, replace the instruction with an invalid one. - if ins.OpCode.IsDWordLoad() { // Replace a dword load with a invalid dword load to preserve instruction size. - *ins = asm.LoadImm(asm.R10, badRelo, asm.DWord) + *ins = asm.LoadImm(asm.R10, COREBadRelocationSentinel, asm.DWord) } else { // Replace all single size instruction with a invalid call instruction. - *ins = asm.BuiltinFunc(badRelo).Call() + *ins = asm.BuiltinFunc(COREBadRelocationSentinel).Call() } + + // Add context to the kernel verifier output. + if source := ins.Source(); source != nil { + *ins = ins.WithSource(&Line{ + line: fmt.Sprintf("instruction poisoned by CO-RE: %s", source), + }) + } else { + *ins = ins.WithSource(&Line{ + line: "instruction poisoned by CO-RE", + }) + } + return nil } diff --git a/btf/core_reloc_test.go b/btf/core_reloc_test.go index 20e86d984..39b21002a 100644 --- a/btf/core_reloc_test.go +++ b/btf/core_reloc_test.go @@ -1,8 +1,10 @@ package btf_test import ( + "bytes" "io" "os" + "slices" "strings" "testing" @@ -10,6 +12,8 @@ import ( "github.com/cilium/ebpf/btf" "github.com/cilium/ebpf/internal" "github.com/cilium/ebpf/internal/testutils" + + "github.com/go-quicktest/qt" ) func TestCORERelocationLoad(t *testing.T) { @@ -121,3 +125,42 @@ func TestLD64IMMReloc(t *testing.T) { } defer coll.Close() } + +func TestCOREPoisonLineInfo(t *testing.T) { + testutils.SkipOnOldKernel(t, "5.0", "program ext_infos") + + spec, err := ebpf.LoadCollectionSpec(testutils.NativeFile(t, "../testdata/errors-%s.elf")) + qt.Assert(t, qt.IsNil(err)) + + var b btf.Builder + raw, err := b.Marshal(nil, nil) + qt.Assert(t, qt.IsNil(err)) + empty, err := btf.LoadSpecFromReader(bytes.NewReader(raw)) + qt.Assert(t, qt.IsNil(err)) + + for _, test := range []struct { + name string + }{ + {"poisoned_single"}, + {"poisoned_double"}, + } { + progSpec := spec.Programs[test.name] + qt.Assert(t, qt.IsNotNil(progSpec)) + + t.Run(test.name, func(t *testing.T) { + t.Log(progSpec.Instructions) + _, err := ebpf.NewProgramWithOptions(progSpec, ebpf.ProgramOptions{ + KernelTypes: empty, + }) + testutils.SkipIfNotSupported(t, err) + + var ve *ebpf.VerifierError + qt.Assert(t, qt.ErrorAs(err, &ve)) + found := slices.ContainsFunc(ve.Log, func(line string) bool { + return strings.HasPrefix(line, "; instruction poisoned by CO-RE") + }) + qt.Assert(t, qt.IsTrue(found)) + t.Logf("%-5v", ve) + }) + } +} diff --git a/btf/marshal.go b/btf/marshal.go index bbf2cfdd1..f14cfa6e9 100644 --- a/btf/marshal.go +++ b/btf/marshal.go @@ -20,14 +20,17 @@ type MarshalOptions struct { StripFuncLinkage bool // Replace Enum64 with a placeholder for compatibility with <6.0 kernels. ReplaceEnum64 bool + // Prevent the "No type found" error when loading BTF without any types. + PreventNoTypeFound bool } // KernelMarshalOptions will generate BTF suitable for the current kernel. func KernelMarshalOptions() *MarshalOptions { return &MarshalOptions{ - Order: internal.NativeEndian, - StripFuncLinkage: haveFuncLinkage() != nil, - ReplaceEnum64: haveEnum64() != nil, + Order: internal.NativeEndian, + StripFuncLinkage: haveFuncLinkage() != nil, + ReplaceEnum64: haveEnum64() != nil, + PreventNoTypeFound: true, // All current kernels require this. } } @@ -94,9 +97,9 @@ func NewBuilder(types []Type) (*Builder, error) { return b, nil } -// Empty returns true if [Add] has not been invoked on the builder. +// Empty returns true if neither types nor strings have been added. func (b *Builder) Empty() bool { - return len(b.types) == 0 + return len(b.types) == 0 && (b.strings == nil || b.strings.Length() == 0) } // Add a Type and allocate a stable ID for it. @@ -169,10 +172,24 @@ func (b *Builder) Marshal(buf []byte, opts *MarshalOptions) ([]byte, error) { ids: maps.Clone(b.stableIDs), } + if e.ids == nil { + e.ids = make(map[Type]TypeID) + } + + types := b.types + if len(types) == 0 && stb.Length() > 0 && opts.PreventNoTypeFound { + // We have strings that need to be written out, + // but no types (besides the implicit Void). + // Kernels as recent as v6.7 refuse to load such BTF + // with a "No type found" error in the log. + // Fix this by adding a dummy type. + types = []Type{&Int{Size: 0}} + } + // Ensure that types are marshaled in the exact order they were Add()ed. // Otherwise the ID returned from Add() won't match. - e.pending.Grow(len(b.types)) - for _, typ := range b.types { + e.pending.Grow(len(types)) + for _, typ := range types { e.pending.Push(typ) } diff --git a/btf/testdata/relocs_read-eb.elf b/btf/testdata/relocs_read-eb.elf index 1f99deb9b..480dcca84 100644 Binary files a/btf/testdata/relocs_read-eb.elf and b/btf/testdata/relocs_read-eb.elf differ diff --git a/btf/testdata/relocs_read-el.elf b/btf/testdata/relocs_read-el.elf index 8a9d45eeb..7e081a8eb 100644 Binary files a/btf/testdata/relocs_read-el.elf and b/btf/testdata/relocs_read-el.elf differ diff --git a/btf/testdata/relocs_read.c b/btf/testdata/relocs_read.c index 09677a59d..1882b9f3c 100644 --- a/btf/testdata/relocs_read.c +++ b/btf/testdata/relocs_read.c @@ -1,8 +1,6 @@ #include "../../testdata/common.h" #include "bpf_core_read.h" -#define core_access __builtin_preserve_access_index - // Struct with the members declared in the wrong order. Accesses need // a successful CO-RE relocation against the type in relocs_read_tgt.c // for the test below to pass. diff --git a/btf/workarounds_test.go b/btf/workarounds_test.go index 1553b43d8..b7b5bf4a5 100644 --- a/btf/workarounds_test.go +++ b/btf/workarounds_test.go @@ -7,6 +7,8 @@ import ( "github.com/cilium/ebpf/internal" "github.com/cilium/ebpf/internal/testutils" + + "github.com/go-quicktest/qt" ) func TestDatasecResolveWorkaround(t *testing.T) { @@ -64,3 +66,15 @@ func TestDatasecResolveWorkaround(t *testing.T) { }) } } + +func TestEmptyBTFWithStringTableWorkaround(t *testing.T) { + var b Builder + + _, err := b.addString("foo") + qt.Assert(t, qt.IsNil(err)) + + h, err := NewHandle(&b) + testutils.SkipIfNotSupported(t, err) + qt.Assert(t, qt.IsNil(err)) + qt.Assert(t, qt.IsNil(h.Close())) +} diff --git a/prog.go b/prog.go index d479a3881..a5e78d2c2 100644 --- a/prog.go +++ b/prog.go @@ -24,6 +24,18 @@ import ( // ErrNotSupported is returned whenever the kernel doesn't support a feature. var ErrNotSupported = internal.ErrNotSupported +// errBadRelocation is returned when the verifier rejects a program due to a +// bad CO-RE relocation. +// +// This error is detected based on heuristics and therefore may not be reliable. +var errBadRelocation = errors.New("bad CO-RE relocation") + +// errUnknownKfunc is returned when the verifier rejects a program due to an +// unknown kfunc. +// +// This error is detected based on heuristics and therefore may not be reliable. +var errUnknownKfunc = errors.New("unknown kfunc") + // ProgramID represents the unique ID of an eBPF program. type ProgramID uint32 @@ -228,6 +240,15 @@ func NewProgramWithOptions(spec *ProgramSpec, opts ProgramOptions) (*Program, er return prog, err } +var ( + coreBadLoad = []byte(fmt.Sprintf("(18) r10 = 0x%x\n", btf.COREBadRelocationSentinel)) + // This log message was introduced by ebb676daa1a3 ("bpf: Print function name in + // addition to function id") which first appeared in v4.10 and has remained + // unchanged since. + coreBadCall = []byte(fmt.Sprintf("invalid func unknown#%d\n", btf.COREBadRelocationSentinel)) + kfuncBadCall = []byte(fmt.Sprintf("invalid func unknown#%d\n", kfuncCallPoisonBase)) +) + func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions) (*Program, error) { if len(spec.Instructions) == 0 { return nil, errors.New("instructions cannot be empty") @@ -416,6 +437,12 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions) (*Program, er _, err2 = sys.ProgLoad(attr) } + end := bytes.IndexByte(logBuf, 0) + if end < 0 { + end = len(logBuf) + } + + tail := logBuf[max(end-256, 0):end] switch { case errors.Is(err, unix.EPERM): if len(logBuf) > 0 && logBuf[0] == 0 { @@ -424,18 +451,32 @@ func newProgramWithOptions(spec *ProgramSpec, opts ProgramOptions) (*Program, er return nil, fmt.Errorf("load program: %w (MEMLOCK may be too low, consider rlimit.RemoveMemlock)", err) } - fallthrough - case errors.Is(err, unix.EINVAL): - if hasFunctionReferences(spec.Instructions) { - if err := haveBPFToBPFCalls(); err != nil { - return nil, fmt.Errorf("load program: %w", err) - } - } - if opts.LogSize > maxVerifierLogSize { return nil, fmt.Errorf("load program: %w (ProgramOptions.LogSize exceeds maximum value of %d)", err, maxVerifierLogSize) } + + if bytes.Contains(tail, coreBadCall) { + err = errBadRelocation + break + } else if bytes.Contains(tail, kfuncBadCall) { + err = errUnknownKfunc + break + } + + case errors.Is(err, unix.EACCES): + if bytes.Contains(tail, coreBadLoad) { + err = errBadRelocation + break + } + } + + // hasFunctionReferences may be expensive, so check it last. + if (errors.Is(err, unix.EINVAL) || errors.Is(err, unix.EPERM)) && + hasFunctionReferences(spec.Instructions) { + if err := haveBPFToBPFCalls(); err != nil { + return nil, fmt.Errorf("load program: %w", err) + } } truncated := errors.Is(err, unix.ENOSPC) || errors.Is(err2, unix.ENOSPC) diff --git a/prog_test.go b/prog_test.go index 5af2fa8dd..0e1ff3c24 100644 --- a/prog_test.go +++ b/prog_test.go @@ -952,6 +952,45 @@ func TestProgramInstructions(t *testing.T) { } } +func TestProgramLoadErrors(t *testing.T) { + testutils.SkipOnOldKernel(t, "4.10", "stable verifier log output") + + spec, err := LoadCollectionSpec(testutils.NativeFile(t, "testdata/errors-%s.elf")) + qt.Assert(t, qt.IsNil(err)) + + var b btf.Builder + raw, err := b.Marshal(nil, nil) + qt.Assert(t, qt.IsNil(err)) + empty, err := btf.LoadSpecFromReader(bytes.NewReader(raw)) + qt.Assert(t, qt.IsNil(err)) + + for _, test := range []struct { + name string + want error + }{ + {"poisoned_single", errBadRelocation}, + {"poisoned_double", errBadRelocation}, + {"poisoned_kfunc", errUnknownKfunc}, + } { + progSpec := spec.Programs[test.name] + qt.Assert(t, qt.IsNotNil(progSpec)) + + t.Run(test.name, func(t *testing.T) { + t.Log(progSpec.Instructions) + _, err := NewProgramWithOptions(progSpec, ProgramOptions{ + KernelTypes: empty, + }) + testutils.SkipIfNotSupported(t, err) + + qt.Assert(t, qt.ErrorIs(err, test.want)) + + var ve *VerifierError + qt.Assert(t, qt.ErrorAs(err, &ve)) + t.Logf("%-5v", ve) + }) + } +} + func BenchmarkNewProgram(b *testing.B) { testutils.SkipOnOldKernel(b, "5.18", "kfunc support") spec, err := LoadCollectionSpec(testutils.NativeFile(b, "testdata/kfunc-%s.elf")) diff --git a/testdata/common.h b/testdata/common.h index 95a13a567..0a31d1324 100644 --- a/testdata/common.h +++ b/testdata/common.h @@ -30,6 +30,8 @@ enum libbpf_tristate { !!sym; \ }) +#define core_access __builtin_preserve_access_index + #define BPF_MAP_TYPE_HASH (1) #define BPF_MAP_TYPE_ARRAY (2) #define BPF_MAP_TYPE_PROG_ARRAY (3) diff --git a/testdata/errors-eb.elf b/testdata/errors-eb.elf new file mode 100644 index 000000000..0cf48c021 Binary files /dev/null and b/testdata/errors-eb.elf differ diff --git a/testdata/errors-el.elf b/testdata/errors-el.elf new file mode 100644 index 000000000..7d12bae96 Binary files /dev/null and b/testdata/errors-el.elf differ diff --git a/testdata/errors.c b/testdata/errors.c new file mode 100644 index 000000000..30815ae48 --- /dev/null +++ b/testdata/errors.c @@ -0,0 +1,25 @@ +#include "common.h" +#include "../btf/testdata/bpf_core_read.h" + +struct nonexist { + int non_exist; +}; + +enum nonexist_enum { NON_EXIST = 1 }; + +__section("socket") int poisoned_single() { + struct nonexist ne; + return core_access(ne.non_exist); +} + +__section("socket") int poisoned_double() { + return bpf_core_enum_value(enum nonexist_enum, NON_EXIST); +} + +extern int invalid_kfunc(void) __ksym __weak; + +__section("socket") int poisoned_kfunc() { + // NB: This doesn't go via CO-RE but uses a similar mechanism to generate + // an invalid instruction. We test it here for convenience. + return invalid_kfunc(); +}