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

[Tycho Andersen <tycho.andersen@canonical.com>]

On Fri, Sep 11, 2015 at 08:40:43AM -0700, Alexei Starovoitov wrote:
> On Fri, Sep 11, 2015 at 11:28:24AM +0200, Daniel Borkmann wrote:
> > [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.
> 
> I don't think so.
> When I played with libseccomp and chrome I saw that browser installed
> several bpf programs for every new tab. The longest program was 275
> classic insns which translated to slightly more eBPF insns
> (because in eBPF we don't have < and <= instructions, so converter
> needs to emit extra jump insns)
> Also they don't produce unreachable code.
> So getting over 4k limit and unreachable are rather hypothetical
> problems. I wouldn't want to have two interfaces to criu seccomp.
> eBPF is going to be used by seccomp as well, so having two is extra
> burden for user space criu.

I think the burden is not so huge once we have eBPF c/r in place (we
could just check the classic flag first, then dump the eBPF version or
something). However, it doesn't seem ideal to have to support two
interfaces.

> If we start hitting 4k limit for eBPF, we can easily bump it
> and/or add < and <= insns to eBPF (which was on my todo list anyway).

I was hoping you might say this (assuming you mean add < BPF_MAXINSNS
to the converter).

The dead code is certainly a problem, but perhaps we can wait on this
until there become some critical application that has this issue. I
was hoping we could get away without extra memory usage on the kernel
side (indeed, this patchset is mostly to try and avoid that).

Tycho