netdev - Re: [PATCH] bpf: Tweak BPF jump table optimizations for objtool compatibility

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:   Tue, 5 May 2020 12:53:20 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        bpf@...r.kernel.org, linux-kernel@...r.kernel.org, x86@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] bpf: Tweak BPF jump table optimizations for objtool
 compatibility

On Tue, May 05, 2020 at 01:11:08PM -0500, Josh Poimboeuf wrote:
> On Tue, May 05, 2020 at 10:43:00AM -0700, Alexei Starovoitov wrote:
> > > Or, if you want to minimize the patch's impact on other arches, and keep
> > > the current patch the way it is (with bug fixed and changed patch
> > > description), that's fine too.  I can change the patch description
> > > accordingly.
> > > 
> > > Or if you want me to measure the performance impact of the +40% code
> > > growth, and *then* decide what to do, that's also fine.  But you'd need
> > > to tell me what tests to run.
> > 
> > I'd like to minimize the risk and avoid code churn,
> > so how about we step back and debug it first?
> > Which version of gcc are you using and what .config?
> > I've tried:
> > Linux version 5.7.0-rc2 (gcc version 10.0.1 20200505 (prerelease) (GCC)
> > CONFIG_UNWINDER_ORC=y
> > # CONFIG_RETPOLINE is not set
> > 
> > and objtool didn't complain.
> > I would like to reproduce it first before making any changes.
> 
> Revert
> 
>   3193c0836f20 ("bpf: Disable GCC -fgcse optimization for ___bpf_prog_run()")
> 
> and compile with retpolines off (and either ORC or FP, doesn't matter).
> 
> I'm using GCC 9.3.1:
> 
>   kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x8dc: sibling call from callable instruction with modified stack frame
> 
> That's the original issue described in that commit.

I see something different.
With gcc 8, 9, and 10 and CCONFIG_UNWINDER_FRAME_POINTER=y
I see:
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x4837: call without frame pointer save/setup
and sure enough assembly code for ___bpf_prog_run does not countain frame setup
though -fno-omit-frame-pointer flag was passed at command line.
Then I did:
static u64 /*__no_fgcse*/ ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn, u64 *stack)
and the assembly had proper frame, but objtool wasn't happy:
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x480a: sibling call from callable instruction with modified stack frame

gcc 6.3 doesn't have objtool warning with and without -fno-gcse.

Looks like we have two issues here.
First gcc 8, 9 and 10 have a severe bug with __attribute__((optimize("")))
In this particular case passing -fno-gcse somehow overruled -fno-omit-frame-pointer
which is serious issue. powerpc is using __nostackprotector. I don't understand
how it can keep working with newer gcc-s. May be got lucky.
Plenty of other projects use various __attribute__((optimize("")))
they all have to double check that their vesion of GCC produces correct code.
Can somebody reach out to gcc folks for explanation?

The second objtool issue is imo minor one. It can be worked around for now
and fixed for real later.

> > Also since objtool cannot follow the optimizations compiler is doing
> > how about admit the design failure and teach objtool to build ORC
> > (and whatever else it needs to build) based on dwarf for the functions where
> > it cannot understand the assembly code ?
> > Otherwise objtool will forever be playing whackamole with compilers.
> 
> I agree it's not a good long term approach.  But DWARF has its own
> issues and we can't rely on it for live patching.

Curious what is the issue with dwarf and live patching ?
I'm sure dwarf is enough to build ORC tables.

> As I mentioned we have a plan to use a compiler plugin to annotate jump
> tables (including GCC switch tables).  But the approach taken by this
> patch should be good enough for now.

I don't have gcc 7 around. Could you please test the workaround with gcc 7,8,9,10
and several clang versions? With ORC and with FP ? and retpoline on/off ?
I don't see any issues with ORC=y. objtool complains with FP=y only for my configs.
I want to make sure the workaround is actually effective.