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 10:52:02 -0700
From:   Randy Dunlap <rdunlap@...radead.org>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        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>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH] bpf: Tweak BPF jump table optimizations for objtool
 compatibility

On 5/5/20 10:43 AM, Alexei Starovoitov wrote:
> On Sat, May 02, 2020 at 02:21:05PM -0500, Josh Poimboeuf wrote:
>>
>> Ideally we would get rid of that label and just change all the 'goto
>> select_insn' to 'goto *jumptable[insn->code]'.  That allows objtool to
>> follow the code in both retpoline and non-retpoline cases.  It also
>> simplifies the code flow and (IMO) makes it easier for GCC to find
>> optimizations.
> 
> No. It's the opposite. It's not simplifying the code. It pessimizes
> compilers.
> 
>>
>> However, for the RETPOLINE=y case, that simplification actually would
>> cause GCC to grow the function text size by 40%.  
> 
> It pessimizes and causes text increase, since the version of gcc
> you're testing with cannot combine indirect gotos back into direct.
> 
>> I thought we were in
>> agreement that significant text growth would be universally bad,
>> presumably because of i-cache locality/pressure issues.  
> 
> No. As I explained before the extra code could give performance
> increase depending on how branch predictor is designed in HW.
> 
>> 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.
> 
> 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.
> 

Hi,

I see the objtool warning:
kernel/bpf/core.o: warning: objtool: ___bpf_prog_run()+0x33: call without frame pointer save/setup

when using:
gcc (SUSE Linux) 9.3.1 20200406 [revision 6db837a5288ee3ca5ec504fbd5a765817e556ac2]

with the attached config file.

thanks.
-- 
~Randy


View attachment "config-objtl-bpf" of type "text/plain" (117448 bytes)