Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make BTF for data sections optional #675

Merged
merged 7 commits into from May 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Expand Up @@ -13,7 +13,7 @@ UIDGID := $(shell stat -c '%u:%g' ${REPODIR})
# Prefer podman if installed, otherwise use docker.
# Note: Setting the var at runtime will always override.
CONTAINER_ENGINE ?= $(if $(shell command -v podman), podman, docker)
CONTAINER_RUN_ARGS ?= $(if $(filter ${CONTAINER_ENGINE}, podman),, --user "${UIDGID}")
CONTAINER_RUN_ARGS ?= $(if $(filter ${CONTAINER_ENGINE}, podman), --log-driver=none, --user "${UIDGID}")

IMAGE := $(shell cat ${REPODIR}/testdata/docker/IMAGE)
VERSION := $(shell cat ${REPODIR}/testdata/docker/VERSION)
Expand Down
73 changes: 53 additions & 20 deletions collection.go
Expand Up @@ -122,34 +122,67 @@ func (cs *CollectionSpec) RewriteMaps(maps map[string]*Map) error {
//
// Returns an error if a constant doesn't exist.
func (cs *CollectionSpec) RewriteConstants(consts map[string]interface{}) error {
rodata := cs.Maps[".rodata"]
if rodata == nil {
return errors.New("missing .rodata section")
}
replaced := make(map[string]bool)

if rodata.BTF == nil {
return errors.New(".rodata section has no BTF")
}
for name, spec := range cs.Maps {
if !strings.HasPrefix(name, ".rodata") {
continue
}

if n := len(rodata.Contents); n != 1 {
return fmt.Errorf("expected one key in .rodata, found %d", n)
}
b, ds, err := spec.dataSection()
if errors.Is(err, errMapNoBTFValue) {
// Data sections without a BTF Datasec are valid, but don't support
// constant replacements.
continue
}
if err != nil {
return fmt.Errorf("map %s: %w", name, err)
}

// MapSpec.Copy() performs a shallow copy. Fully copy the byte slice
// to avoid any changes affecting other copies of the MapSpec.
cpy := make([]byte, len(b))
copy(cpy, b)

for _, v := range ds.Vars {
vname := v.Type.TypeName()
replacement, ok := consts[vname]
if !ok {
continue
}

kv := rodata.Contents[0]
value, ok := kv.Value.([]byte)
if !ok {
return fmt.Errorf("first value in .rodata is %T not []byte", kv.Value)
if replaced[vname] {
return fmt.Errorf("section %s: duplicate variable %s", name, vname)
}

if int(v.Offset+v.Size) > len(cpy) {
return fmt.Errorf("section %s: offset %d(+%d) for variable %s is out of bounds", name, v.Offset, v.Size, vname)
}

b, err := marshalBytes(replacement, int(v.Size))
if err != nil {
return fmt.Errorf("marshaling constant replacement %s: %w", vname, err)
}

copy(cpy[v.Offset:v.Offset+v.Size], b)

replaced[vname] = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow replacing the same variable in multiple rodata sections? Seems wonky.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning behind not allowing it? Detecting compiler bugs? Technically, the same var being present in multiple rodata sections would also require having 2 equally-named ELF symbols with global vis, which is wonky indeed. So, we error here instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, added a check that makes sure we don't replace the same var twice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this could also happen when the caller copies .rodata from a CollectionSpec and adds it under a different key in Maps.

}

spec.Contents[0] = MapKV{Key: uint32(0), Value: cpy}
}

buf := make([]byte, len(value))
copy(buf, value)
var missing []string
for c := range consts {
if !replaced[c] {
missing = append(missing, c)
}
}

err := patchValue(buf, rodata.Value, consts)
if err != nil {
return err
if len(missing) != 0 {
return fmt.Errorf("spec is missing one or more constants: %s", strings.Join(missing, ","))
}

rodata.Contents[0] = MapKV{kv.Key, buf}
return nil
}

Expand Down
45 changes: 27 additions & 18 deletions elf_reader.go
Expand Up @@ -459,7 +459,7 @@ func (ec *elfCode) relocateInstruction(ins *asm.Instruction, rel elf.Symbol) err
switch typ {
case elf.STT_SECTION:
if bind != elf.STB_LOCAL {
return fmt.Errorf("direct load: %s: unsupported relocation %s", name, bind)
return fmt.Errorf("direct load: %s: unsupported section relocation %s", name, bind)
}

// This is really a reference to a static symbol, which clang doesn't
Expand All @@ -468,8 +468,17 @@ func (ec *elfCode) relocateInstruction(ins *asm.Instruction, rel elf.Symbol) err
offset = uint32(uint64(ins.Constant))

case elf.STT_OBJECT:
if bind != elf.STB_GLOBAL {
return fmt.Errorf("direct load: %s: unsupported relocation %s", name, bind)
// LLVM 9 emits OBJECT-LOCAL symbols for anonymous constants.
if bind != elf.STB_GLOBAL && bind != elf.STB_LOCAL {
return fmt.Errorf("direct load: %s: unsupported object relocation %s", name, bind)
}

offset = uint32(rel.Value)

case elf.STT_NOTYPE:
// LLVM 7 emits NOTYPE-LOCAL symbols for anonymous constants.
if bind != elf.STB_LOCAL {
return fmt.Errorf("direct load: %s: unsupported untyped relocation %s", name, bind)
}

offset = uint32(rel.Value)
Expand Down Expand Up @@ -1016,15 +1025,6 @@ func (ec *elfCode) loadDataSections(maps map[string]*MapSpec) error {
continue
}

if ec.btf == nil {
return errors.New("data sections require BTF, make sure all consts are marked as static")
}

var datasec *btf.Datasec
if err := ec.btf.TypeByName(sec.Name, &datasec); err != nil {
return fmt.Errorf("data section %s: can't get BTF: %w", sec.Name, err)
}

data, err := sec.Data()
if err != nil {
return fmt.Errorf("data section %s: can't get contents: %w", sec.Name, err)
Expand All @@ -1041,16 +1041,25 @@ func (ec *elfCode) loadDataSections(maps map[string]*MapSpec) error {
ValueSize: uint32(len(data)),
MaxEntries: 1,
Contents: []MapKV{{uint32(0), data}},
Key: &btf.Void{},
Value: datasec,
BTF: ec.btf,
}

switch sec.Name {
case ".rodata":
// It is possible for a data section to exist without a corresponding BTF Datasec
// if it only contains anonymous values like macro-defined arrays.
if ec.btf != nil {
var ds *btf.Datasec
if ec.btf.TypeByName(sec.Name, &ds) == nil {
// Assign the spec's key and BTF only if the Datasec lookup was successful.
mapSpec.BTF = ec.btf
mapSpec.Key = &btf.Void{}
mapSpec.Value = ds
}
}

switch n := sec.Name; {
case strings.HasPrefix(n, ".rodata"):
mapSpec.Flags = unix.BPF_F_RDONLY_PROG
mapSpec.Freeze = true
case ".bss":
case n == ".bss":
// The kernel already zero-initializes the map
mapSpec.Contents = nil
}
Expand Down
20 changes: 12 additions & 8 deletions elf_reader_test.go
Expand Up @@ -131,6 +131,12 @@ func TestLoadCollectionSpec(t *testing.T) {
SectionName: "static",
License: "MIT",
},
"anon_const": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there is no fix for the problem here, this means that the tests will fail on this commit? If yes, that's problematic when trying to bisect problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, didn't know we really cared about that, will respin the set to take this into account.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Name: "anon_const",
Type: SocketFilter,
SectionName: "socket/4",
License: "MIT",
},
},
}

Expand All @@ -148,13 +154,10 @@ func TestLoadCollectionSpec(t *testing.T) {
cmpopts.IgnoreFields(MapSpec{}, "Key", "Value"),
cmpopts.IgnoreUnexported(ProgramSpec{}),
cmpopts.IgnoreMapEntries(func(key string, _ *MapSpec) bool {
switch key {
case ".bss", ".data", ".rodata":
if key == ".bss" || key == ".data" || strings.HasPrefix(key, ".rodata") {
return true

default:
return false
}
return false
}),
}

Expand All @@ -171,9 +174,10 @@ func TestLoadCollectionSpec(t *testing.T) {
}

opts := defaultOpts
if have.Maps[".rodata"] != nil {
if have.Types != nil {
err := have.RewriteConstants(map[string]interface{}{
"arg": uint32(1),
"arg": uint32(1),
"arg2": uint32(2),
})
if err != nil {
t.Fatal("Can't rewrite constant:", err)
Expand Down Expand Up @@ -217,7 +221,7 @@ func TestLoadCollectionSpec(t *testing.T) {
t.Fatal("Can't run program:", err)
}

if ret != 5 {
if ret != 7 {
t.Error("Expected return value to be 5, got", ret)
}
})
Expand Down
81 changes: 26 additions & 55 deletions map.go
Expand Up @@ -8,7 +8,6 @@ import (
"math/rand"
"path/filepath"
"reflect"
"strings"
"time"
"unsafe"

Expand All @@ -25,6 +24,7 @@ var (
ErrKeyExist = errors.New("key already exists")
ErrIterationAborted = errors.New("iteration aborted")
ErrMapIncompatible = errors.New("map spec is incompatible with existing map")
errMapNoBTFValue = errors.New("map spec does not contain a BTF Value")
)

// MapOptions control loading a map into the kernel.
Expand Down Expand Up @@ -128,6 +128,31 @@ func (ms *MapSpec) clampPerfEventArraySize() error {
return nil
}

// dataSection returns the contents and BTF Datasec descriptor of the spec.
func (ms *MapSpec) dataSection() ([]byte, *btf.Datasec, error) {

if ms.Value == nil {
return nil, nil, errMapNoBTFValue
}

ds, ok := ms.Value.(*btf.Datasec)
if !ok {
return nil, nil, fmt.Errorf("map value BTF is a %T, not a *btf.Datasec", ms.Value)
}

if n := len(ms.Contents); n != 1 {
return nil, nil, fmt.Errorf("expected one key, found %d", n)
}

kv := ms.Contents[0]
value, ok := kv.Value.([]byte)
if !ok {
return nil, nil, fmt.Errorf("value at first map key is %T, not []byte", kv.Value)
}

return value, ds, nil
}

// MapKV is used to initialize the contents of a Map.
type MapKV struct {
Key interface{}
Expand Down Expand Up @@ -1282,60 +1307,6 @@ func marshalMap(m *Map, length int) ([]byte, error) {
return buf, nil
}

func patchValue(value []byte, typ btf.Type, replacements map[string]interface{}) error {
replaced := make(map[string]bool)
replace := func(name string, offset, size int, replacement interface{}) error {
if offset+size > len(value) {
return fmt.Errorf("%s: offset %d(+%d) is out of bounds", name, offset, size)
}

buf, err := marshalBytes(replacement, size)
if err != nil {
return fmt.Errorf("marshal %s: %w", name, err)
}

copy(value[offset:offset+size], buf)
replaced[name] = true
return nil
}

switch parent := typ.(type) {
case *btf.Datasec:
for _, secinfo := range parent.Vars {
name := string(secinfo.Type.(*btf.Var).Name)
replacement, ok := replacements[name]
if !ok {
continue
}

err := replace(name, int(secinfo.Offset), int(secinfo.Size), replacement)
if err != nil {
return err
}
}

default:
return fmt.Errorf("patching %T is not supported", typ)
}

if len(replaced) == len(replacements) {
return nil
}

var missing []string
for name := range replacements {
if !replaced[name] {
missing = append(missing, name)
}
}

if len(missing) == 1 {
return fmt.Errorf("unknown field: %s", missing[0])
}

return fmt.Errorf("unknown fields: %s", strings.Join(missing, ","))
}

// MapIterator iterates a Map.
//
// See Map.Iterate.
Expand Down
Binary file modified testdata/loader-clang-14-eb.elf
Binary file not shown.
Binary file modified testdata/loader-clang-14-el.elf
Binary file not shown.
Binary file modified testdata/loader-clang-7-eb.elf
Binary file not shown.
Binary file modified testdata/loader-clang-7-el.elf
Binary file not shown.
Binary file modified testdata/loader-clang-9-eb.elf
Binary file not shown.
Binary file modified testdata/loader-clang-9-el.elf
Binary file not shown.
30 changes: 28 additions & 2 deletions testdata/loader.c
Expand Up @@ -112,7 +112,9 @@ int __attribute__((noinline)) global_fn(uint32_t arg) {
static volatile unsigned int key1 = 0; // .bss
static volatile unsigned int key2 = 1; // .data
volatile const unsigned int key3 = 2; // .rodata
static volatile const uint32_t arg; // .rodata, rewritten by loader
static volatile const uint32_t arg; // .rodata, populated by loader
// custom .rodata section, populated by loader
static volatile const uint32_t arg2 __section(".rodata.test");
#endif

__section("xdp") int xdp_prog() {
Expand All @@ -121,11 +123,12 @@ __section("xdp") int xdp_prog() {
unsigned int key2 = 1;
unsigned int key3 = 2;
uint32_t arg = 1;
uint32_t arg2 = 2;
#endif
map_lookup_elem(&hash_map, (void *)&key1);
map_lookup_elem(&hash_map2, (void *)&key2);
map_lookup_elem(&hash_map2, (void *)&key3);
return static_fn(arg) + global_fn(arg);
return static_fn(arg) + global_fn(arg) + arg2;
}

// This function has no relocations, and is thus parsed differently.
Expand Down Expand Up @@ -166,3 +169,26 @@ __section("socket/3") int data_sections() {
return 0;
}
#endif

/*
* Up until LLVM 14, this program results in an .rodata.cst32 section
* that is accessed by 'return values[i]'. For this section, no BTF is
* emitted. 'values' cannot be rewritten, since there is no BTF info
* describing the data section.
*/
__section("socket/4") int anon_const() {
volatile int ctx = 0;

// 32 bytes wide results in a .rodata.cst32 section.
#define values \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we just fake a .rodata.foo via explicit section annotations? Might be more robust on newer versions of LLVM as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried all possible ways I could think of to force this to be emitted to a particular section, but haven't managed. Since it's not a real symbol, clang doesn't support setting the section attribute on it.

(uint64_t[]) { 0x0, 0x1, 0x2, 0x3 }

int i;
for (i = 0; i < 3; i++) {
if (ctx == values[i]) {
return values[i];
}
}

return 0;
}