linux-kernel - Re: [PATCH] compiler.h: Move instrumentation_begin()/end() into new <linux/instrumentation.h> header

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: <20200604081928.GA570386@gmail.com>
Date:   Thu, 4 Jun 2020 10:19:28 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] compiler.h: Move instrumentation_begin()/end() into new
 <linux/instrumentation.h> header


* Ingo Molnar <mingo@...nel.org> wrote:

> 
> * Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> 
> > On Mon, Jun 1, 2020 at 6:08 AM Ingo Molnar <mingo@...nel.org> wrote:
> > >
> > >  include/linux/compiler.h            | 53 +++++++++++++++++++++++
> > 
> > I have pulled this, but do we really want to add this to a header file
> > that is _so_ core that it gets included for basically every single
> > file built?
> > 
> > I don't even see those instrumentation_begin/end() things used
> > anywhere right now.
> > 
> > It seems excessive. That 53 lines is maybe not a lot, but it pushed
> > that header file to over 12kB, and while it's mostly comments, it's
> > extra IO and parsing basically for _every_ single file compiled in the
> > kernel.
> > 
> > For what appears to be absolutely zero upside right now, and I really
> > don't see why this should be in such a core header file!
> > 
> > I don't even see this as having anything at all to do with
> > "compiler.h" in the first place.
> > 
> > I really think we should think twice about making core header files
> > bigger like this. No, we're nowhere the disaster that C++ project
> > headers are, but tokenization and parsing is actually a pretty big
> > part of the build costs (which may surprise some people who think it's
> > all the fancy optimizations that cost a lot of CPU time).
> 
> Fully agreed - and I made the attached patch to address this.
> 
> The code got cleaner and better structured, but it didn't help much in 
> terms of inclusion count:
> 
>    2616  total .o files
> 
>    2447  <linux/types.h>
>    2436  <linux/compiler.h>
>    2404  <linux/bug.h>
> 
> The reason is that <linux/bug.h> is included almost everywhere as 
> well, and the instrumentation_begin()/end() annotations affect the 
> BUG*() and WARN*() primitives as well.
> 
> At least non-x86 would have less instrumentation related noise, for 
> now at least.

>  arch/x86/include/asm/bug.h       |  1 +
>  include/linux/compiler.h         | 53 -------------------------------------
>  include/linux/context_tracking.h |  2 ++
>  include/linux/instrumentation.h  | 57 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 60 insertions(+), 53 deletions(-)

The tested v2 version of the patch also needed the include in 
asm-generic/bug.h (see the fix attached below), because for 
completeness the generic version was annotated as well - even though 
only x86 has objtool support for now.

The readability improvement is real though.

Thanks,

	Ingo


diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 384b5c835ced..c43b5906e0dc 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -3,6 +3,7 @@
 #define _ASM_GENERIC_BUG_H
 
 #include <linux/compiler.h>
+#include <linux/instrumentation.h>
 
 #define CUT_HERE		"------------[ cut here ]------------\n"