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] [day] [month] [year] [list]

Message-ID: <CAJpBn1wjSasEW7cq9na6MSbuO+=sj2qwJ4dd14f2mja24_hxvg@mail.gmail.com>
Date:   Sat, 22 Dec 2018 14:12:17 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.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, 21 Dec 2018 17:24:01 -0800, Alexei Starovoitov wrote:
> 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 just use ja pc+0 because its simplest, but its possible that a
subprog will have a conditional jump based on an argument which is
always taken or not taken, and therefore can be removed.

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

I shouldn't have posted this incremental diff, it's quite messy, I
changed comments around and reshuffled things :(  I think the code ended
up reasonably clean and readable.

The case that prompted me to look at this was legit dead code, not
result of patching in constants.  Let me post the full set after
Christmas and perhaps you can judge the whole thing..

I quite like the ability to correct the line info and subprog info, for
code space constrained architectures removing code whenever possible is
really important.