From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCH] ebpf: emit correct src_reg for conditional jumps Date: Fri, 11 Sep 2015 11:28:24 +0200 Message-ID: <55F29EB8.50805@iogearbox.net> References: <1441931107-17673-1-git-send-email-tycho.andersen@canonical.com> <55F294A9.9020005@iogearbox.net> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org To: Tycho Andersen , Alexei Starovoitov , "David S. Miller" Return-path: Received: from www62.your-server.de ([213.133.104.62]:43774 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751499AbbIKJ23 (ORCPT ); Fri, 11 Sep 2015 05:28:29 -0400 In-Reply-To: <55F294A9.9020005@iogearbox.net> Sender: netdev-owner@vger.kernel.org List-ID: 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 >> CC: Alexei Starovoitov >> CC: Daniel Borkmann > > 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