Re: [PATCH] compiler: enable CONFIG_OPTIMIZE_INLINING forcibly

From: Will Deacon <will@kernel.org>
To: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>,
	linux-arch <linux-arch@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Russell King <rmk+kernel@arm.linux.org.uk>,
	Stefan Wahren <wahrenst@gmx.net>, Kees Cook <keescook@google.com>
Subject: Re: [PATCH] compiler: enable CONFIG_OPTIMIZE_INLINING forcibly
Date: Mon, 30 Sep 2019 13:18:04 +0100	[thread overview]
Message-ID: <20190930121803.n34i63scet2ec7ll@willie-the-truck> (raw)
In-Reply-To: <CAK7LNASQZ82KSOrQW7+Wq1vFDCg2__maBEAPMLqUDqZMLuj1rA@mail.gmail.com>

On Mon, Sep 30, 2019 at 09:05:11PM +0900, Masahiro Yamada wrote:
> On Mon, Sep 30, 2019 at 8:26 PM Will Deacon <will@kernel.org> wrote:
> > On Fri, Sep 27, 2019 at 03:38:44PM -0700, Linus Torvalds wrote:
> > > Soem of that code is pretty subtle. They have fixed register usage
> > > (but the asm macros actually check them). And the inline asms clobber
> > > the link register, but they do seem to clearly _state_ that they
> > > clobber it, so who knows.
> > >
> > > Just based on the EFAULT, I'd _guess_ that it's some interaction with
> > > the domain access control register (so that get/set_domain() thing).
> > > But I'm not even sure that code is enabled for the Rpi2, so who
> > > knows..
> >
> > FWIW, we've run into issues with CONFIG_OPTIMIZE_INLINING and local
> > variables marked as 'register' where GCC would do crazy things and end
> > up corrupting data, so I suspect the use of fixed registers in the arm
> > uaccess functions is hitting something similar:
> >
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91111
> 
> No. Not similar at all.

They're similar in that enabling CONFIG_OPTIMIZE_INLINING causes register
variables to go wrong. I agree that the ARM code looks dodgy with
that call to uaccess_save_and_enable(), but there are __asmeq macros
in there to try to catch that, so it's still very fishy.

> I fixed it already. See
> https://lore.kernel.org/patchwork/patch/1132459/

You fixed the specific case above for 32-bit ARM, but the arm64 case
is due to a compiler bug. As it happens, we've reworked our atomics
in 5.4 so that particular issue no longer triggers, but the fact remains
that GCC has been shown to screw up explicit register allocation for
perfectly legitimate code when giving the flexibility to move code out
of line.

> The problems are fixable by writing correct code.

Right, in the compiler ;)

> I think we discussed this already.

We did?

> - There is nothing arch-specific in CONFIG_OPTIMIZE_INLINING

Apart from the bugs... and even then, that's just based on reports.

> - 'inline' is just a hint. It does not guarantee function inlining.
>   This is standard.
> 
> - The kernel macrofies 'inline' to add __attribute__((__always_inline__))
>   This terrible hack must end.

I'm all for getting rid of hacks, but not at the cost of correctness.

> -  __attribute__((__always_inline__)) takes aways compiler's freedom,
>    and prevents it from optimizing the code for -O2, -Os, or whatever.

s/whatever/miscompiling the code/

If it helps, here is more information about the arm64 failure which
triggered the GCC bugzilla:

https://www.spinics.net/lists/arm-kernel/msg730329.html

Will