`call -1` instructions no longer load without error

[brycekahle]

Describe the bug

f086705 breaks loading of programs with error:
decoding instructions for section <sectionname>: offset <offset>: invalid constant 0xffffffff

The actual instruction looks like 85 00 00 00 ff ff ff ff call -1

This is happening because we have call -1 instructions that we use as sentinels for patching before loading into the kernel. Since 0xffff is a valid 32 bit value, it shouldn't be considered an invalid constant or instruction. I'm not sure why it is being sign-extended to 64 bits in the error message.

How to reproduce

Write an ebpf program with the following:

#define PATCH_TARGET -1
static void *(*bpf_patch)(unsigned long, ...) = (void *)PATCH_TARGET;

and in your program use it

bpf_patch(0, 0);

then try to load the program using cilium/ebpf

Version information

n/a

[lmb]

@dylandreimerink could you take a look? I'll be out for the next week and a half.

[dylandreimerink]

Yes, will take a look

[dylandreimerink]

Hmm. I am able to repo this and found the cause. In f086705 I moved this line

ebpf/asm/instruction.go

Line 67 in df9ebe8

ins.Constant = int64(int32(bo.Uint32(data[4:8])))

from below the if statement to above it.

Before that change ins.Constant is always 0 when passed to BuiltinFuncForPlatform. So I think that logic didn't work as expected before it was accidentally "fixed" by this move.

BuiltinFuncForPlatform applies a tag in the upper 4 bits which is required for Windows support. The code we have asserts that no more than the lower 28 bits of the constant are used, but -1 of course uses all 32 bits due to the way its encoded.

6bba483 made it so 85 00 00 00 ff ff ff ff call -1 cannot be decoded anymore, however the new logic was never properly invoked.

Option one is to not throw an error anymore and overwrite the upper 4 bits. Side effect is that 85 00 00 00 ff ff ff ff would be decoded as call 268435455 (the sign bit zeroed out) which would likely also mess up your placeholder code.

To me it seems like we should store the platform info in the extra 32 bits of the constant normally reserved for double width loads or in a new metadata tag, and then defer tagging the constant until marshaling. That would mean that we can unmarshal into a spec, but we require the user to patch instructions where the constant (as uint32) is greater than 2^28. That way we still keep this error around to warn of bad encoding, but we tolerate a wider range of input.

@lmb what do you think?