Repository: cilium/ebpf
Pull request #1422 (2024-04-09 11:29:03)

btf: refuse reloTypeIDTarget for kmod types
reloTypeIDTarget is used to substitute the ID of an equivalent type in vmlinux. This is problematic when dealing with kmod types, since their IDs are defined to be sequential with vmlinux. If the last vmlinux type ID is 99, the first type in kmod a and b has ID 100. To disambiguate between these we need a (BTF ID, Type ID) tuple, which we currently don't support.

Poison any relocation which tries to use the target ID of a kmod type. This also gets rid of mergedSpec which didn't take overlapping ID ranges into account.


Patch:
diff --git a/btf/btf.go b/btf/btf.go
index c5def03..204757d 100644
--- a/btf/btf.go
+++ b/btf/btf.go
@@ -563,7 +563,7 @@ func (s *Spec) TypeByID(id TypeID) (Type, error) {
 
 // TypeID returns the ID for a given Type.
 //
-// Returns an error wrapping ErrNoFound if the type isn't part of the Spec.
+// Returns an error wrapping [ErrNotFound] if the type isn't part of the Spec.
 func (s *Spec) TypeID(typ Type) (TypeID, error) {
 	return s.mutableTypes.typeID(typ)
 }
diff --git a/btf/core.go b/btf/core.go
index f3b7e03..4a19791 100644
--- a/btf/core.go
+++ b/btf/core.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"math"
 	"reflect"
+	"slices"
 	"strconv"
 	"strings"
 
@@ -174,36 +175,12 @@ func (k coreKind) String() string {
 	}
 }
 
-type mergedSpec []*Spec
-
-func (s mergedSpec) TypeByID(id TypeID) (Type, error) {
-	for _, sp := range s {
-		t, err := sp.TypeByID(id)
-		if err != nil {
-			if errors.Is(err, ErrNotFound) {
-				continue
-			}
-			return nil, err
-		}
-		return t, nil
-	}
-	return nil, fmt.Errorf("look up type with ID %d (first ID is %d): %w", id, s[0].imm.firstTypeID, ErrNotFound)
-}
-
-func (s mergedSpec) NamedTypes(name essentialName) []TypeID {
-	var typeIDs []TypeID
-	for _, sp := range s {
-		namedTypes := sp.imm.namedTypes[name]
-		if len(namedTypes) > 0 {
-			typeIDs = append(typeIDs, namedTypes...)
-		}
-	}
-	return typeIDs
-}
-
 // CORERelocate calculates changes needed to adjust eBPF instructions for differences
 // in types.
 //
+// targets forms the set of types to relocate against. The first element has to be
+// BTF for vmlinux, the following must be types for kernel modules.
+//
 // resolveLocalTypeID is called for each local type which requires a stable TypeID.
 // Calling the function with the same type multiple times must produce the same
 // result. It is the callers responsibility to ensure that the relocated instructions
@@ -220,6 +197,10 @@ func CORERelocate(relos []*CORERelocation, targets []*Spec, bo binary.ByteOrder,
 		return nil, fmt.Errorf("targets must be provided")
 	}
 
+	// We can't encode type IDs that aren't for vmlinux into instructions at the
+	// moment.
+	resolveTargetTypeID := targets[0].TypeID
+
 	for _, target := range targets {
 		if bo != target.imm.byteOrder {
 			return nil, fmt.Errorf("can't relocate %s against %s", bo, target.imm.byteOrder)
@@ -265,15 +246,29 @@ func CORERelocate(relos []*CORERelocation, targets []*Spec, bo binary.ByteOrder,
 		group.indices = append(group.indices, i)
 	}
 
-	mergeTarget := mergedSpec(targets)
 	for localType, group := range relosByType {
 		localTypeName := localType.TypeName()
 		if localTypeName == "" {
 			return nil, fmt.Errorf("relocate unnamed or anonymous type %s: %w", localType, ErrNotSupported)
 		}
 
-		targets := mergeTarget.NamedTypes(newEssentialName(localTypeName))
-		fixups, err := coreCalculateFixups(group.relos, &mergeTarget, targets, bo)
+		essentialName := newEssentialName(localTypeName)
+
+		var targetTypes []Type
+		for _, target := range targets {
+			namedTypeIDs := target.imm.namedTypes[essentialName]
+			targetTypes = slices.Grow(targetTypes, len(namedTypeIDs))
+			for _, id := range namedTypeIDs {
+				typ, err := target.TypeByID(id)
+				if err != nil {
+					return nil, err
+				}
+
+				targetTypes = append(targetTypes, typ)
+			}
+		}
+
+		fixups, err := coreCalculateFixups(group.relos, targetTypes, bo, resolveTargetTypeID)
 		if err != nil {
 			return nil, fmt.Errorf("relocate %s: %w", localType, err)
 		}
@@ -296,19 +291,14 @@ var errIncompatibleTypes = errors.New("incompatible types")
 //
 // The best target is determined by scoring: the less poisoning we have to do
 // the better the target is.
-func coreCalculateFixups(relos []*CORERelocation, targetSpec *mergedSpec, targets []TypeID, bo binary.ByteOrder) ([]COREFixup, error) {
+func coreCalculateFixups(relos []*CORERelocation, targets []Type, bo binary.ByteOrder, resolveTargetTypeID func(Type) (TypeID, error)) ([]COREFixup, error) {
 	bestScore := len(relos)
 	var bestFixups []COREFixup
-	for _, targetID := range targets {
-		target, err := targetSpec.TypeByID(targetID)
-		if err != nil {
-			return nil, fmt.Errorf("look up target: %w", err)
-		}
-
+	for _, target := range targets {
 		score := 0 // lower is better
 		fixups := make([]COREFixup, 0, len(relos))
 		for _, relo := range relos {
-			fixup, err := coreCalculateFixup(relo, target, targetID, bo)
+			fixup, err := coreCalculateFixup(relo, target, bo, resolveTargetTypeID)
 			if err != nil {
 				return nil, fmt.Errorf("target %s: %s: %w", target, relo.kind, err)
 			}
@@ -359,9 +349,8 @@ func coreCalculateFixups(relos []*CORERelocation, targetSpec *mergedSpec, target
 
 var errNoSignedness = errors.New("no signedness")
 
-// coreCalculateFixup calculates the fixup for a single local type, target type
-// and relocation.
-func coreCalculateFixup(relo *CORERelocation, target Type, targetID TypeID, bo binary.ByteOrder) (COREFixup, error) {
+// coreCalculateFixup calculates the fixup given a relocation and a target type.
+func coreCalculateFixup(relo *CORERelocation, target Type, bo binary.ByteOrder, resolveTargetTypeID func(Type) (TypeID, error)) (COREFixup, error) {
 	fixup := func(local, target uint64) (COREFixup, error) {
 		return COREFixup{kind: relo.kind, local: local, target: target}, nil
 	}
@@ -397,6 +386,15 @@ func coreCalculateFixup(relo *CORERelocation, target Type, targetID TypeID, bo b
 			return fixup(1, 1)
 
 		case reloTypeIDTarget:
+			targetID, err := resolveTargetTypeID(target)
+			if errors.Is(err, ErrNotFound) {
+				// Probably a relocation trying to get the ID
+				// of a type from a kmod.
+				return poison()
+			}
+			if err != nil {
+				return zero, err
+			}
 			return fixup(uint64(relo.id), uint64(targetID))
 
 		case reloTypeSize:
diff --git a/prog.go b/prog.go
index a5e78d2..f4f3af7 100644
--- a/prog.go
+++ b/prog.go
@@ -659,7 +659,7 @@ type RunOptions struct {
 }
 
 // Test runs the Program in the kernel with the given input and returns the
-// value returned by the eBPF program. outLen may be zero.
+// value returned by the eBPF program.
 //
 // Note: the kernel expects at least 14 bytes input for an ethernet header for
 // XDP and SKB programs.


Test patch:
diff --git a/btf/core_test.go b/btf/core_test.go
index b5ce7ff..0c39d3f 100644
--- a/btf/core_test.go
+++ b/btf/core_test.go
@@ -588,7 +588,7 @@ func TestCOREReloFieldSigned(t *testing.T) {
 			relo := &CORERelocation{
 				typ, coreAccessor{0}, reloFieldSigned, 0,
 			}
-			fixup, err := coreCalculateFixup(relo, &Void{}, 0, internal.NativeEndian)
+			fixup, err := coreCalculateFixup(relo, &Void{}, internal.NativeEndian, dummyTypeID)
 			qt.Assert(t, qt.IsTrue(fixup.poison))
 			qt.Assert(t, qt.IsNil(err))
 		})
@@ -598,7 +598,7 @@ func TestCOREReloFieldSigned(t *testing.T) {
 		relo := &CORERelocation{
 			&Array{}, coreAccessor{0}, reloFieldSigned, 0,
 		}
-		_, err := coreCalculateFixup(relo, &Array{}, 0, internal.NativeEndian)
+		_, err := coreCalculateFixup(relo, &Array{}, internal.NativeEndian, dummyTypeID)
 		qt.Assert(t, qt.ErrorIs(err, errNoSignedness))
 	})
 }
@@ -615,12 +615,47 @@ func TestCOREReloFieldShiftU64(t *testing.T) {
 		{typ, coreAccessor{0, 0}, reloFieldLShiftU64, 1},
 	} {
 		t.Run(relo.kind.String(), func(t *testing.T) {
-			_, err := coreCalculateFixup(relo, typ, 1, internal.NativeEndian)
+			_, err := coreCalculateFixup(relo, typ, internal.NativeEndian, dummyTypeID)
 			qt.Assert(t, qt.ErrorIs(err, errUnsizedType))
 		})
 	}
 }
 
+func TestCORERelosKmodTypeID(t *testing.T) {
+	a := &Int{Name: "a"}
+	b := &Int{Name: "b"}
+
+	relos := []*CORERelocation{
+		{&Int{}, coreAccessor{0}, reloTypeIDTarget, 0},
+	}
+
+	typeID := func(t Type) (TypeID, error) {
+		if t == a {
+			return 42, nil
+		}
+		return 0, ErrNotFound
+	}
+
+	fixups, err := coreCalculateFixups(
+		relos,
+		[]Type{a, b},
+		internal.NativeEndian,
+		typeID,
+	)
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.IsFalse(fixups[0].poison))
+	qt.Assert(t, qt.Equals(fixups[0].target, 42))
+
+	fixups, err = coreCalculateFixups(
+		relos,
+		[]Type{b},
+		internal.NativeEndian,
+		typeID,
+	)
+	qt.Assert(t, qt.IsNil(err))
+	qt.Assert(t, qt.IsTrue(fixups[0].poison))
+}
+
 func BenchmarkCORESkBuff(b *testing.B) {
 	spec := vmlinuxTestdataSpec(b)
 
@@ -669,3 +704,8 @@ func BenchmarkCORESkBuff(b *testing.B) {
 		})
 	}
 }
+
+// dummyTypeID returns 0, nil for any passed type.
+func dummyTypeID(Type) (TypeID, error) {
+	return 0, nil
+}