Repository: cilium/ebpf
Pull request #145 (2020-10-01 16:40:05)

btf: fix LoadKernelSpec
The current code fails to find any kernel spec because the call to
Sprintf may contain too many or too few arguments. The result then
contains %!s(MISSING), etc. which of course triggers an error when
trying to open the path.

Another problem is that the function tries other filenames if reading
the BTF itself fails for some reason. In that case it will most likely
return ErrNotFound, and the user is none the wiser: we found a BTF
but couldn't load it.

Change the code so that we return any error from the BTF loading itself.

lmb: cc @brycekahle this is why CI fails. Now I'm mystified why it worked on the other kernels :shrug: 

Patch:
diff --git a/asm/instruction.go b/asm/instruction.go
index 8fbcf56..890dc00 100644
--- a/asm/instruction.go
+++ b/asm/instruction.go
@@ -12,6 +12,14 @@ import (
 // InstructionSize is the size of a BPF instruction in bytes
 const InstructionSize = 8
 
+// RawInstructionOffset is an offset in units of raw BPF instructions.
+type RawInstructionOffset uint64
+
+// Bytes returns the offset of an instruction in bytes.
+func (rio RawInstructionOffset) Bytes() uint64 {
+	return uint64(rio) * InstructionSize
+}
+
 // Instruction is a single eBPF instruction.
 type Instruction struct {
 	OpCode    OpCode
@@ -155,6 +163,13 @@ func (ins *Instruction) isLoadFromMap() bool {
 	return ins.OpCode == LoadImmOp(DWord) && (ins.Src == PseudoMapFD || ins.Src == PseudoMapValue)
 }
 
+// IsFunctionCall returns true if the instruction calls another BPF function.
+//
+// This is not the same thing as a BPF helper call.
+func (ins *Instruction) IsFunctionCall() bool {
+	return ins.OpCode.JumpOp() == Call && ins.Src == PseudoCall
+}
+
 // Format implements fmt.Formatter.
 func (ins Instruction) Format(f fmt.State, c rune) {
 	if c != 'v' {
@@ -310,28 +325,6 @@ func (insns Instructions) ReferenceOffsets() map[string][]int {
 	return offsets
 }
 
-func (insns Instructions) marshalledOffsets() (map[string]int, error) {
-	symbols := make(map[string]int)
-
-	marshalledPos := 0
-	for _, ins := range insns {
-		currentPos := marshalledPos
-		marshalledPos += ins.OpCode.marshalledInstructions()
-
-		if ins.Symbol == "" {
-			continue
-		}
-
-		if _, ok := symbols[ins.Symbol]; ok {
-			return nil, fmt.Errorf("duplicate symbol %s", ins.Symbol)
-		}
-
-		symbols[ins.Symbol] = currentPos
-	}
-
-	return symbols, nil
-}
-
 // Format implements fmt.Formatter.
 //
 // You can control indentation of symbols by
@@ -370,21 +363,17 @@ func (insns Instructions) Format(f fmt.State, c rune) {
 		symIndent = strings.Repeat(" ", symPadding)
 	}
 
-	// Figure out how many digits we need to represent the highest
-	// offset.
-	highestOffset := 0
-	for _, ins := range insns {
-		highestOffset += ins.OpCode.marshalledInstructions()
-	}
+	// Guess how many digits we need at most, by assuming that all instructions
+	// are double wide.
+	highestOffset := len(insns) * 2
 	offsetWidth := int(math.Ceil(math.Log10(float64(highestOffset))))
 
-	offset := 0
-	for _, ins := range insns {
-		if ins.Symbol != "" {
-			fmt.Fprintf(f, "%s%s:\n", symIndent, ins.Symbol)
+	iter := insns.Iterate()
+	for iter.Next() {
+		if iter.Ins.Symbol != "" {
+			fmt.Fprintf(f, "%s%s:\n", symIndent, iter.Ins.Symbol)
 		}
-		fmt.Fprintf(f, "%s%*d: %v\n", indent, offsetWidth, offset, ins)
-		offset += ins.OpCode.marshalledInstructions()
+		fmt.Fprintf(f, "%s%*d: %v\n", indent, offsetWidth, iter.Offset, iter.Ins)
 	}
 
 	return
@@ -392,43 +381,49 @@ func (insns Instructions) Format(f fmt.State, c rune) {
 
 // Marshal encodes a BPF program into the kernel format.
 func (insns Instructions) Marshal(w io.Writer, bo binary.ByteOrder) error {
-	absoluteOffsets, err := insns.marshalledOffsets()
-	if err != nil {
-		return err
-	}
-
-	num := 0
 	for i, ins := range insns {
-		switch {
-		case ins.OpCode.JumpOp() == Call && ins.Src == PseudoCall && ins.Constant == -1:
-			// Rewrite bpf to bpf call
-			offset, ok := absoluteOffsets[ins.Reference]
-			if !ok {
-				return fmt.Errorf("instruction %d: reference to missing symbol %s", i, ins.Reference)
-			}
-
-			ins.Constant = int64(offset - num - 1)
-
-		case ins.OpCode.Class() == JumpClass && ins.Offset == -1:
-			// Rewrite jump to label
-			offset, ok := absoluteOffsets[ins.Reference]
-			if !ok {
-				return fmt.Errorf("instruction %d: reference to missing symbol %s", i, ins.Reference)
-			}
-
-			ins.Offset = int16(offset - num - 1)
-		}
-
-		n, err := ins.Marshal(w, bo)
+		_, err := ins.Marshal(w, bo)
 		if err != nil {
 			return fmt.Errorf("instruction %d: %w", i, err)
 		}
-
-		num += int(n / InstructionSize)
 	}
 	return nil
 }
 
+// Iterate allows iterating a BPF program while keeping track of
+// various offsets.
+//
+// Modifying the instruction slice will lead to undefined behaviour.
+func (insns Instructions) Iterate() *InstructionIterator {
+	return &InstructionIterator{insns: insns}
+}
+
+// InstructionIterator iterates over a BPF program.
+type InstructionIterator struct {
+	insns Instructions
+	// The instruction in question.
+	Ins *Instruction
+	// The index of the instruction in the original instruction slice.
+	Index int
+	// The offset of the instruction in raw BPF instructions. This accounts
+	// for double-wide instructions.
+	Offset RawInstructionOffset
+}
+
+// Next returns true as long as there are any instructions remaining.
+func (iter *InstructionIterator) Next() bool {
+	if len(iter.insns) == 0 {
+		return false
+	}
+
+	if iter.Ins != nil {
+		iter.Offset += RawInstructionOffset(iter.Ins.OpCode.rawInstructions())
+	}
+	iter.Ins = &iter.insns[0]
+	iter.insns = iter.insns[1:]
+	return true
+}
+
 type bpfInstruction struct {
 	OpCode    OpCode
 	Registers bpfRegisters
diff --git a/asm/opcode.go b/asm/opcode.go
index c99b659..dc4564a 100644
--- a/asm/opcode.go
+++ b/asm/opcode.go
@@ -66,10 +66,10 @@ type OpCode uint8
 // InvalidOpCode is returned by setters on OpCode
 const InvalidOpCode OpCode = 0xff
 
-// marshalledInstructions returns the number of BPF instructions required
+// rawInstructions returns the number of BPF instructions required
 // to encode this opcode.
-func (op OpCode) marshalledInstructions() int {
-	if op == LoadImmOp(DWord) {
+func (op OpCode) rawInstructions() int {
+	if op.isDWordLoad() {
 		return 2
 	}
 	return 1
diff --git a/internal/btf/btf.go b/internal/btf/btf.go
index 57a8154..5e4a958 100644
--- a/internal/btf/btf.go
+++ b/internal/btf/btf.go
@@ -137,7 +137,7 @@ func findBtfSections(file *elf.File) (*elf.Section, *elf.Section, map[string]uin
 	return btfSection, btfExtSection, sectionSizes, nil
 }
 
-func loadNakedSpecFromReader(rd io.ReaderAt, sectionSizes map[string]uint32, variableOffsets map[variable]uint32) (*Spec, error) {
+func loadSpecFromVmlinux(rd io.ReaderAt) (*Spec, error) {
 	file, err := elf.NewFile(rd)
 	if err != nil {
 		return nil, err
@@ -148,7 +148,7 @@ func loadNakedSpecFromReader(rd io.ReaderAt, sectionSizes map[string]uint32, var
 	if btfSection == nil {
 		return nil, fmt.Errorf("unable to find .BTF ELF section")
 	}
-	return loadNakedSpec(btfSection.Open(), file.ByteOrder, sectionSizes, variableOffsets)
+	return loadNakedSpec(btfSection.Open(), file.ByteOrder, nil, nil)
 }
 
 func loadNakedSpec(btf io.ReadSeeker, bo binary.ByteOrder, sectionSizes map[string]uint32, variableOffsets map[variable]uint32) (*Spec, error) {
@@ -203,42 +203,35 @@ func loadKernelSpec() (*Spec, error) {
 		return nil, fmt.Errorf("can't read kernel release number: %w", err)
 	}
 
+	fh, err := os.Open("/sys/kernel/btf/vmlinux")
+	if err == nil {
+		defer fh.Close()
+
+		return loadNakedSpec(fh, internal.NativeEndian, nil, nil)
+	}
+
 	// use same list of locations as libbpf
 	// https://github.com/libbpf/libbpf/blob/9a3a42608dbe3731256a5682a125ac1e23bced8f/src/btf.c#L3114-L3122
-	locations := []struct {
-		pathFmt string
-		raw     bool
-	}{
-		/* try canonical vmlinux BTF through sysfs first */
-		{"/sys/kernel/btf/vmlinux", true /* raw BTF */},
-		/* fall back to trying to find vmlinux ELF on disk otherwise */
-		{"/boot/vmlinux-%s", false},
-		{"/lib/modules/%s/vmlinux-%s", false},
-		{"/lib/modules/%s/build/vmlinux", false},
-		{"/usr/lib/modules/%s/kernel/vmlinux", false},
-		{"/usr/lib/debug/boot/vmlinux-%s", false},
-		{"/usr/lib/debug/boot/vmlinux-%s.debug", false},
-		{"/usr/lib/debug/lib/modules/%s/vmlinux", false},
-	}
-
-	var spec *Spec
+	locations := []string{
+		"/boot/vmlinux-%s",
+		"/lib/modules/%s/vmlinux-%[1]s",
+		"/lib/modules/%s/build/vmlinux",
+		"/usr/lib/modules/%s/kernel/vmlinux",
+		"/usr/lib/debug/boot/vmlinux-%s",
+		"/usr/lib/debug/boot/vmlinux-%s.debug",
+		"/usr/lib/debug/lib/modules/%s/vmlinux",
+	}
+
 	for _, loc := range locations {
-		path := fmt.Sprintf(loc.pathFmt, release)
+		path := fmt.Sprintf(loc, release)
+
 		fh, err := os.Open(path)
 		if err != nil {
 			continue
 		}
 		defer fh.Close()
-		if loc.raw {
-			spec, err = loadNakedSpec(fh, internal.NativeEndian, nil, nil)
-		} else {
-			spec, err = loadNakedSpecFromReader(fh, nil, nil)
-		}
 
-		if err != nil {
-			continue
-		}
-		return spec, nil
+		return loadSpecFromVmlinux(fh)
 	}
 
 	return nil, fmt.Errorf("BTF for kernel version %s: %w", release, ErrNotFound)
diff --git a/linker.go b/linker.go
index 1bb8f61..fc4ad9d 100644
--- a/linker.go
+++ b/linker.go
@@ -84,3 +84,50 @@ func needSection(insns, section asm.Instructions) (bool, error) {
 	// None of the functions in the section are called.
 	return false, nil
 }
+
+func fixupJumpsAndCalls(insns asm.Instructions) error {
+	symbolOffsets := make(map[string]asm.RawInstructionOffset)
+	iter := insns.Iterate()
+	for iter.Next() {
+		ins := iter.Ins
+
+		if ins.Symbol == "" {
+			continue
+		}
+
+		if _, ok := symbolOffsets[ins.Symbol]; ok {
+			return fmt.Errorf("duplicate symbol %s", ins.Symbol)
+		}
+
+		symbolOffsets[ins.Symbol] = iter.Offset
+	}
+
+	iter = insns.Iterate()
+	for iter.Next() {
+		i := iter.Index
+		offset := iter.Offset
+		ins := iter.Ins
+
+		switch {
+		case ins.IsFunctionCall() && ins.Constant == -1:
+			// Rewrite bpf to bpf call
+			callOffset, ok := symbolOffsets[ins.Reference]
+			if !ok {
+				return fmt.Errorf("instruction %d: reference to missing symbol %s", i, ins.Reference)
+			}
+
+			ins.Constant = int64(callOffset - offset - 1)
+
+		case ins.OpCode.Class() == asm.JumpClass && ins.Offset == -1:
+			// Rewrite jump to label
+			jumpOffset, ok := symbolOffsets[ins.Reference]
+			if !ok {
+				return fmt.Errorf("instruction %d: reference to missing symbol %s", i, ins.Reference)
+			}
+
+			ins.Offset = int16(jumpOffset - offset - 1)
+		}
+	}
+
+	return nil
+}
diff --git a/prog.go b/prog.go
index b072be1..bf1c799 100644
--- a/prog.go
+++ b/prog.go
@@ -208,8 +208,15 @@ func convertProgramSpec(spec *ProgramSpec, handle *btf.Handle) (*bpfProgLoadAttr
 		return nil, fmt.Errorf("can't load %s program on %s", spec.ByteOrder, internal.NativeEndian)
 	}
 
+	insns := make(asm.Instructions, len(spec.Instructions))
+	copy(insns, spec.Instructions)
+
+	if err := fixupJumpsAndCalls(insns); err != nil {
+		return nil, err
+	}
+
 	buf := bytes.NewBuffer(make([]byte, 0, len(spec.Instructions)*asm.InstructionSize))
-	err := spec.Instructions.Marshal(buf, internal.NativeEndian)
+	err := insns.Marshal(buf, internal.NativeEndian)
 	if err != nil {
 		return nil, err
 	}


Test patch:
diff --git a/internal/btf/btf_test.go b/internal/btf/btf_test.go
index 39a547b..de8e022 100644
--- a/internal/btf/btf_test.go
+++ b/internal/btf/btf_test.go
@@ -134,6 +134,17 @@ func TestLoadSpecFromElf(t *testing.T) {
 	})
 }
 
+func TestLoadKernelSpec(t *testing.T) {
+	if _, err := os.Stat("/sys/kernel/btf/vmlinux"); os.IsNotExist(err) {
+		t.Skip("/sys/kernel/btf/vmlinux not present")
+	}
+
+	_, err := LoadKernelSpec()
+	if err != nil {
+		t.Fatal("Can't load kernel spec:", err)
+	}
+}
+
 func TestHaveBTF(t *testing.T) {
 	testutils.CheckFeatureTest(t, haveBTF)
 }