netdev - Re: [PATCH bpf-next 07/19] bpf: verifier: remove dead code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 21 Dec 2018 17:24:01 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Yonghong Song <yhs@...com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "oss-drivers@...ronome.com" <oss-drivers@...ronome.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Martin Lau <kafai@...com>
Subject: Re: [PATCH bpf-next 07/19] bpf: verifier: remove dead code

On Fri, Dec 21, 2018 at 03:46:32PM -0800, Jakub Kicinski wrote:
> On Thu, 20 Dec 2018 07:19:06 +0000, Yonghong Song wrote:
> > > I think this will break func_info, since it's not adjusted here.
> > > Also iirc line_info is relying on first insn to always have line_info.
> > > If first insn is dead, second insn might not have a line_info generated
> > > and things won't go well.  
> > 
> > Right, func_info needs to be adjusted as well. The first line_info for 
> > each func must have insn_off = 0. In case of dead code elimination from
> > the start, if the first remaining insn has a line_info, just use it.
> > Otherwise, you can use the old first line_info.
> 
> I think I got it (see the incremental diff below).  I want to also
> tackle the JIT linfo offsets for offloads while at it and post an RFC
> (unless you're handling that already Martin?)

that may work, but I think it's too much extra complexity for
artificial corner case.
The first insn of the program and subprog will be valid insn
otherwise check_cfg would have rejected the prog earlier.
The only way it can become 'dead' is if the user manually created
bpf prog with 'ja pc+0' as the first insn just to triger this dead
code optimization pass. In such case I think it's better to leave
that insn as-is instead going out of the way adjusting prog_info, line_info
and other things that may depend on the first insn in the future.

I'm having seconds thoughts regarding the whole idea of dead code elimination.
We are relying on user space to optimize BPF code. Due to this choice we
avoided adding optimization passes to the kernel (few optimizations that
we do in the verifier and JITs cannot be done by the user space).
The way 'dead code' is used now is kinda border line.
May be we should teach libbpf to do this instead?
I'm happy to be convinced that kernel really needs to do it,
but I want to see examples where libbpf approach cannot fit.