Repository: cilium/ebpf
Pull request #137 (2020-09-04 21:59:57)

Fix BTF map loading to use pointer target types and sizes
For BTF based map loading, the `key` and `value` members of a map definition should always be pointers. This is because the `__type` macro is defined as `#define __type(name, val) typeof(val) *name`. Surprisingly, the kernel expects the type IDs passed to the `bpf` syscall, to be the "target" types and not the pointer types themselves. The `key_size` and `value_size` values should also be based on the "target" types.

I was receiving verifier errors without these changes.

Supports #114 


Patch:
diff --git a/internal/btf/btf.go b/internal/btf/btf.go
index 3dd000a..e335fe6 100644
--- a/internal/btf/btf.go
+++ b/internal/btf/btf.go
@@ -402,9 +402,19 @@ func (s *Spec) Map(name string) (*Map, []Member, error) {
 		switch member.Name {
 		case "key":
 			key = member.Type
+			if pk, isPtr := key.(*Pointer); !isPtr {
+				return nil, nil, fmt.Errorf("key type is not a pointer: %T", key)
+			} else {
+				key = pk.Target
+			}
 
 		case "value":
 			value = member.Type
+			if vk, isPtr := value.(*Pointer); !isPtr {
+				return nil, nil, fmt.Errorf("value type is not a pointer: %T", value)
+			} else {
+				value = vk.Target
+			}
 		}
 	}
 
@@ -621,9 +631,7 @@ type bpfLoadBTFAttr struct {
 }
 
 func bpfLoadBTF(attr *bpfLoadBTFAttr) (*internal.FD, error) {
-	const _BTFLoad = 18
-
-	fd, err := internal.BPF(_BTFLoad, unsafe.Pointer(attr), unsafe.Sizeof(*attr))
+	fd, err := internal.BPF(internal.BPF_BTF_LOAD, unsafe.Pointer(attr), unsafe.Sizeof(*attr))
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/btf/types.go b/internal/btf/types.go
index 59121fc..8db5b66 100644
--- a/internal/btf/types.go
+++ b/internal/btf/types.go
@@ -23,6 +23,8 @@ type Type interface {
 	// Make a copy of the type, without copying Type members.
 	copy() Type
 
+	// Enumerate all nested Types. Repeated calls must visit nested
+	// types in the same order.
 	walk(*copyStack)
 }
 
@@ -223,36 +225,39 @@ func (td *Typedef) copy() Type {
 	return &cpy
 }
 
-// Volatile is a modifier.
+// Volatile is a qualifier.
 type Volatile struct {
 	TypeID
 	Type Type
 }
 
+func (v *Volatile) qualify() Type      { return v.Type }
 func (v *Volatile) walk(cs *copyStack) { cs.push(&v.Type) }
 func (v *Volatile) copy() Type {
 	cpy := *v
 	return &cpy
 }
 
-// Const is a modifier.
+// Const is a qualifier.
 type Const struct {
 	TypeID
 	Type Type
 }
 
+func (c *Const) qualify() Type      { return c.Type }
 func (c *Const) walk(cs *copyStack) { cs.push(&c.Type) }
 func (c *Const) copy() Type {
 	cpy := *c
 	return &cpy
 }
 
-// Restrict is a modifier.
+// Restrict is a qualifier.
 type Restrict struct {
 	TypeID
 	Type Type
 }
 
+func (r *Restrict) qualify() Type      { return r.Type }
 func (r *Restrict) walk(cs *copyStack) { cs.push(&r.Type) }
 func (r *Restrict) copy() Type {
 	cpy := *r
@@ -354,6 +359,16 @@ var (
 	_ sizer = (*Datasec)(nil)
 )
 
+type qualifier interface {
+	qualify() Type
+}
+
+var (
+	_ qualifier = (*Const)(nil)
+	_ qualifier = (*Restrict)(nil)
+	_ qualifier = (*Volatile)(nil)
+)
+
 // Sizeof returns the size of a type in bytes.
 //
 // Returns an error if the size can't be computed.
@@ -382,14 +397,9 @@ func Sizeof(typ Type) (int, error) {
 		case *Typedef:
 			typ = v.Type
 			continue
-		case *Volatile:
-			typ = v.Type
-			continue
-		case *Const:
-			typ = v.Type
-			continue
-		case *Restrict:
-			typ = v.Type
+
+		case qualifier:
+			typ = v.qualify()
 			continue
 
 		default:
@@ -439,7 +449,7 @@ func copyType(typ Type) Type {
 }
 
 // copyStack keeps track of pointers to types which still
-// need to be copied.
+// need to be visited.
 type copyStack []*Type
 
 // push adds a type to the stack.


Test patch:
diff --git a/internal/btf/types_test.go b/internal/btf/types_test.go
index 0f67282..45d119f 100644
--- a/internal/btf/types_test.go
+++ b/internal/btf/types_test.go
@@ -43,15 +43,7 @@ func TestCopyType(t *testing.T) {
 	}
 
 	t.Run("cyclical", func(t *testing.T) {
-		ptr := &Pointer{}
-		foo := &Struct{
-			Members: []Member{
-				{Type: ptr},
-			},
-		}
-		ptr.Target = foo
-
-		_ = copyType(foo)
+		_ = copyType(newCyclicalType(2))
 	})
 }
 
@@ -79,3 +71,75 @@ func ExampleType_validTypes() {
 	t = &Datasec{}
 	_ = t
 }
+
+func TestType(t *testing.T) {
+	types := []func() Type{
+		func() Type { return &Void{} },
+		func() Type { return &Int{} },
+		func() Type { return &Pointer{} },
+		func() Type { return &Array{} },
+		func() Type { return &Struct{} },
+		func() Type { return &Union{} },
+		func() Type { return &Enum{} },
+		func() Type { return &Fwd{} },
+		func() Type { return &Typedef{} },
+		func() Type { return &Volatile{} },
+		func() Type { return &Const{} },
+		func() Type { return &Restrict{} },
+		func() Type { return &Func{} },
+		func() Type { return &FuncProto{} },
+		func() Type { return &Var{} },
+		func() Type { return &Datasec{} },
+	}
+
+	for _, fn := range types {
+		typ := fn()
+		t.Run(fmt.Sprintf("%T", typ), func(t *testing.T) {
+			if typ == typ.copy() {
+				t.Error("Copy doesn't copy")
+			}
+
+			var first, second copyStack
+			typ.walk(&first)
+			typ.walk(&second)
+
+			a, b := first.pop(), second.pop()
+			for a != nil && b != nil {
+				if a != b {
+					t.Fatal("Order of walk is not the same")
+				}
+				a, b = first.pop(), second.pop()
+			}
+
+			if a != nil || b != nil {
+				t.Fatal("Number of types walked is not the same")
+			}
+		})
+	}
+}
+
+func newCyclicalType(n int) Type {
+	ptr := &Pointer{}
+	prev := Type(ptr)
+	for i := 0; i < n; i++ {
+		switch i % 5 {
+		case 0:
+			prev = &Struct{
+				Members: []Member{
+					{Type: prev},
+				},
+			}
+
+		case 1:
+			prev = &Const{Type: prev}
+		case 2:
+			prev = &Volatile{Type: prev}
+		case 3:
+			prev = &Typedef{Type: prev}
+		case 4:
+			prev = &Array{Type: prev}
+		}
+	}
+	ptr.Target = prev
+	return ptr
+}