linux-kernel - Re: objtool warnings for kernel/trace/trace_selftest

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]

Message-ID: <alpine.LSU.2.21.1812181022570.9644@pobox.suse.cz>
Date:   Tue, 18 Dec 2018 10:28:56 +0100 (CET)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
cc:     Andi Kleen <ak@...ux.intel.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Arnd Bergmann <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        the arch/x86 maintainers <x86@...nel.org>, mjambor@...e.cz
Subject: Re: objtool warnings for kernel/trace/trace_selftest_dynamic.o

On Mon, 17 Dec 2018, Josh Poimboeuf wrote:

> On Mon, Dec 17, 2018 at 04:06:18PM -0800, Andi Kleen wrote:
> > On Mon, Dec 17, 2018 at 05:36:44PM -0500, Steven Rostedt wrote:
> > > On Mon, 17 Dec 2018 15:31:26 -0600
> > > Josh Poimboeuf <jpoimboe@...hat.com> wrote:
> > > 
> > > > On Mon, Dec 17, 2018 at 08:29:38PM +0100, Peter Zijlstra wrote:
> > > > > On Mon, Dec 17, 2018 at 12:16:38PM -0600, Josh Poimboeuf wrote:
> > > > >   
> > > > > > > Yes LTO causes the to be treated like static functions.
> > > > > > > 
> > > > > > > I guess noclone is unlikely to be really needed here because these
> > > > > > > functions are unlikely to be cloned.
> > > > > > > 
> > > > > > > So as a workaround it could be removed.
> > > > > > > 
> > > > > > > But note we have other noclone functions in the tree (like in KVM)
> > > > > > > which actually need it.  
> > > > > > 
> > > > > > How about we just use the __used attribute then?  It seems to have the
> > > > > > same result of preventing IPA optimizations (without the weird side
> > > > > > effect of missing frame pointers).  
> > > > > 
> > > > > AFAIK we don't have any in-tree LTO, so it can all go in the bin.
> > > > > 
> > > > > When/if we get the LTO trainwreck sorted -- which very much includes
> > > > > getting that memory-order-consume fixed -- we can revisit all that.  
> > > > 
> > > > Ok, then if there are no objections I'll just send a revert of:
> > > > 
> > > >   dd3dad0d716d ("ftrace: Mark function tracer test functions noinline/noclone")
> 
> Sorry for suggesting this prematurely, my email client stopped syncing
> and I missed your later replies to Peter about this.
> 
> > > Should it be reverted, or just remove the noclone, and keep the
> > > noinline?
> > 
> > It should not be touched for now, until it is properly debugged.
> > 
> > IMHO Josh's explanation doesn't make much sense and there
> > was a lot of handwaving 
> > 
> > And just fixing one case isn't good enough because there are other
> > noclone functions in the tree.
> > 
> > It the problem is the plugin the plugin needs to be fixed.
> > 
> > If the problem is gcc we need a gcc test case and bug, with 
> > some analysis, and then based on that select the proper workaround.
> 
> The plugin is only used for older versions of GCC.  Newer versions have
> the same functionality builtin with -fsanitize-coverage=trace-pc.
> 
> So the problem is GCC.  We're using a function attribute which at least
> oneGCC developer doesn't recommend.  If you want to keep the LTO support
> then '__used' seems like a much better choice.

Martin added to CC.

Martin, the thread starts here 
http://lkml.kernel.org/r/CAK8P3a2K1K21ePBFbApaTKPCk+=Bqj0LyWoK1MdFb1s9ZwjfPg@mail.gmail.com

Can you explain the background of noclone vs. used attributes, please? 
We discussed it yesterday and I understood that maybe we should not rely 
on noclone that much. However it is used in the kernel. Should we avoid 
it in general and replace it with something else (used)? 

It definitely makes sense in our livepatching samples which Josh mentioned 
previously in the thread.

Thanks,
Miroslav