Re: [PATCH] Add support for BPF

[Mark Wielaard <mjw at redhat dot com>]

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?

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.

> (Really, the only change I've made so far is to give the objects a
> proper e_machine value.  The stock llvm compiler produces e_machine = 0.
> I chose EM_BPF = 0xeb9f, as a sort-of leet-speak spelling of "ebpf".)

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.

> The most important part of the patch, for me, is the disassembler,
> since there isn't any published tool for that at the moment, much less
> one that could take its data out of the object file.  I followed the
> dumping code in linux/kernel/bpf/verifier.c in producing C-like output.
> (Though the kernel's output for 32-bit ALU ops is ugly so I cleaned
> that up, as well as tweaking the signed shift and compare ops.)

I do have to admit that our disassembler isn't very actively maintained.
This might be the first addition in the last 5 years.

> 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?

> +int
> +bpf_disasm (const uint8_t **startp, const uint8_t *end, GElf_Addr addr,
> +	    const char *fmt __attribute__((unused)),
> +	    DisasmOutputCB_t outcb,
> +	    DisasmGetSymCB_t symcb __attribute__((unused)),
> +	    void *outcbarg,
> +	    void *symcbarg __attribute__((unused)))
> +{
> +  const uint8_t *start = *startp;
> +  char buf[128];
> +  int len, retval = 0;
> +
> +  while (start + sizeof(struct bpf_insn) <= end)
> +    {
> +      struct bpf_insn i;
> +      unsigned code, class, op;
> +      const char *op_str;
> +
> +      memcpy(&i, start, sizeof(struct bpf_insn));
> +      start += sizeof(struct bpf_insn);
> +      addr += sizeof(struct bpf_insn);
> +
> +      /* ??? 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.

(And libasm.h itself isn't really a public interface itself. You aren't
actually able to initialize an DisasmCtx_t because that needs an Ebl
handle, which is only accessible through internal non-public functions.
Oops.)

> +      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.

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

Looks good.

Mark