Re: [PATCH] ebpf: emit correct src_reg for conditional jumps

[Daniel Borkmann <daniel@iogearbox.net>]

On 09/11/2015 10:45 AM, Daniel Borkmann wrote:
> On 09/11/2015 02:25 AM, Tycho Andersen wrote:
>> Instead of always emitting BPF_REG_X, let's emit BPF_REG_X only when the
>> source actually is BPF_X. This causes programs generated by the classic
>> converter to not be importable via bpf(), as the eBPF verifier checks that
>> the src_reg is correct or 0. While not a problem yet, this will be a
>> problem when BPF_PROG_DUMP lands, and we can potentially dump and re-import
>> programs generated by the converter.
>>
>> Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
>> CC: Alexei Starovoitov <ast@kernel.org>
>> CC: Daniel Borkmann <daniel@iogearbox.net>
>
> I think the description at the beginning could have been a bit clearer. I.e.
> it's safe to zero insn->src_reg for BPF_SRC(fp->code) that is not X, because
> we can either have X or K for classic jump compares, and in case of K we're
> only interested in the immediate value, but not a possible src_reg, of course,
> so eBPF converter should have zeroed it.
>
> Yeah, it was never really the aim of the converter to cover something like
> that requirement you seem to have:
>
>    load classic BPF
>      -> transform into eBPF
>        -> dump that eBPF result to uspace
>          -> load that eBPF via bpf(2)

[off topic for this patch]

... this requirement also breaks down for cases where you have a single
classic BPF instruction that maps into 2 or more eBPF instructions, hitting
BPF_MAXINSNS early at the time when you try to call into bpf(2) again with
the dumped result. If I recall correctly, Chrome seems to use up quite a lot
of insns space on (classic) seccomp-BPF, so this could potentially be a real
issue, next to artificially crafted examples that would fail.

With regards to the latter, also classic programs that could have holes of
dead code where you jump over them (see lib/test_bpf.c for examples) are
unfortunately allowed on the ABI side, so while the classic -> eBPF converter
may translate this dead region 1:1, it will be rejected by the verifier when
you dump and try to reload that. ;) Anyway, it's perhaps a silly example, but
it shows that this use-case can only work on a 'best-effort' basis, and not
cover everything of the classic BPF ABI, as opposed to having an interface
that can dump/restore classic BPF instructions directly.

Do you need this for checkpoint/restore? Wouldn't this therefore be better
done as dump/restore classic<->classic and eBPF<->eBPF directly? In socket
filters we do this by keeping orig_prog around, I guess it's better to bite
the bullet of additional memory overhead in case of classic seccomp-BPF, too.

Cheers,
Daniel