Re: [PATCH] Add support for BPF

[Richard Henderson <rth at redhat dot com>]

On 06/07/2016 06:19 AM, Mark Wielaard wrote:
> Hi Richard,
> 
> On Fri, 2016-06-03 at 13:48 -0700, Richard Henderson wrote:
>> I'm using the following to aid in debugging objects similar to what's
>> produced by the llvm-bpf backend.
> 
> Thanks. Is there a document/URL on BPF data structures and how it gets
> embedded into ELF files to include in the source comments for reference?

No.  The best I could do is point you at

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/samples/bpf/bpf_load.c

and then you can read the code.  ;-)

> Also can I trick you into submitting a GNU-style ChangeLog for it too?
> Personally I like those because they review by listing what to expect.
> Although there seem to be more and more abolitionists out there these
> days.

Heh.  And I'm one of them.  But yes, I can write such a thing.

> Nice :) Normally we just take the latest elf.h from glibc.
> Would you mind submitting that part to libc-alpha@sourceware.org for
> inclusion? That would also make sure that other projects pick up the
> same EM constant as standard for BPF ELF files.

Yeah, I'll submit it for binutils and llvm too.

>> What's lacking so far are test cases.  Although there needn't be too
>> many of those, considering the simplicity of the target.
> 
> A simple testcase would be great. If only for those of us who have never
> actually seen an BPF ELF file. Note that we do allow binary files in the
> testsuite for tests that only read data. But please add a note in the
> test harness how to regenerate them.
> 
> I think we need a configure check to see if we have <linux/bpf.h>. Or we
> could create our own constants file and struct bpf_insn to be
> independent from the specific kernel headers installed.
> 
>> +static const char class_string[8][8] = {
>> +  [BPF_LD]    = "ld",
>> +  [BPF_LDX]   = "ldx",
>> +  [BPF_ST]    = "st",
>> +  [BPF_STX]   = "stx",
>> +  [BPF_ALU]   = "alu",
>> +  [BPF_JMP]   = "jmp",
>> +  [BPF_RET]   = "6",
>> +  [BPF_ALU64] = "alu64",
>> +};
> 
> Why is RET 6?

The "old" bpf format, from *bsd, used opcode 6 for return.  In the new ebpf
format, it's completely unused.

>> +      /* ??? We really should pass in CTX, so that we can detect
>> +	 wrong endianness and do some swapping.  */
> 
> Note that the disasm EBL hook is an internal API, so you should feel
> free to just pass in the ctx, ebl or elf to the function if you need it
> to determine whether the ELF was big or little endian. All users are
> internal.

Ok.

>> +      code = i.code;
>> +      class = BPF_CLASS(code);
>> +      op = BPF_OP(code) | BPF_SRC(code);
> 
> Do the various BPF_XXX macros clamp the values so that the fmt array
> accesses below don't overflow? We don't want crashes even if someone
> feeds bogus code (struct bpf_insn) values.

Yes, they do mask values.

> Of course if we have a small BPF testcase ELF file we can always just
> run a fuzzer on it to make sure.

Fair enough.  It would be small enough to just add all 256 values that fill the
opcode byte.


r~