linux-kernel - Re: [PATCH v2 15/28] init: lto: ensure initcall ordering

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: <20200908211638.GC1060586@google.com>
Date:   Tue, 8 Sep 2020 14:16:38 -0700
From:   Sami Tolvanen <samitolvanen@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Masahiro Yamada <masahiroy@...nel.org>,
        Will Deacon <will@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Paul E. McKenney" <paulmck@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        clang-built-linux@...glegroups.com,
        kernel-hardening@...ts.openwall.com, linux-arch@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kbuild@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        x86@...nel.org
Subject: Re: [PATCH v2 15/28] init: lto: ensure initcall ordering

On Thu, Sep 03, 2020 at 03:40:31PM -0700, Kees Cook wrote:
> On Thu, Sep 03, 2020 at 01:30:40PM -0700, Sami Tolvanen wrote:
> > With LTO, the compiler doesn't necessarily obey the link order for
> > initcalls, and initcall variables need globally unique names to avoid
> > collisions at link time.
> > 
> > This change exports __KBUILD_MODNAME and adds the initcall_id() macro,
> > which uses it together with __COUNTER__ and __LINE__ to help ensure
> > these variables have unique names, and moves each variable to its own
> > section when LTO is enabled, so the correct order can be specified using
> > a linker script.
> > 
> > The generate_initcall_ordering.pl script uses nm to find initcalls from
> > the object files passed to the linker, and generates a linker script
> > that specifies the intended order. With LTO, the script is called in
> > link-vmlinux.sh.
> 
> I think I asked before about this being made unconditional, but the hit
> on final link time was noticeable. Am I remembering that right? If so,
> sure, let's keep it separate.

Yes, it was noticeable when compiling on systems with fewer CPU cores,
so I would prefer to keep it separate.

> > +## forks a child to process each file passed in the command line and collects
> > +## the results
> > +sub process_files {
> > +	my $index = 0;
> > +	my $njobs = get_online_processors();
> > +	my $select = IO::Select->new();
> > +
> > +	while (my $file = shift(@ARGV)) {
> > +		# fork a child process and read it's stdout
> > +		my $pid = open(my $fh, '-|');
> 
> /me makes noises about make -jN and the jobserver and not using all
> processors on a machine if we were asked nicely not to.
> 
> I wrote a jobserver aware tool for the documentation builds, but it's in
> python (scripts/jobserver-exec). Instead of reinventing that wheel (and
> in Perl), we could:
> 
> 1) ignore this problem and assume anyone using LTO is fine with using all CPUs
> 
> 2) implement a jobserver-aware Perl script to do this
> 
> 3) make Python a build dependency of CONFIG_LTO and re-use scripts/jobserver-exec

I'm fine with any of these options, although I'm not sure why anyone
would want to compile an LTO kernel without using all the available
cores... :)

Using jobserver-exec seems like the easiest option if we want to limit
the number of cores used here. Any preferences?

> >  # If CONFIG_LTO_CLANG is selected, collect generated symbol versions into
> >  # .tmp_symversions.lds
> >  gen_symversions()
> > @@ -74,6 +84,9 @@ modpost_link()
> >  		--end-group"
> >  
> >  	if [ -n "${CONFIG_LTO_CLANG}" ]; then
> > +		gen_initcalls
> > +		lds="-T .tmp_initcalls.lds"
> 
> Oh, I think lds should be explicitly a "local" at the start of this
> function, perhaps back in the symversions patch that touches this?

It's already local, that part is just not visible in this patch.

Sami