Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"

From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: Chris J Arges <chris.j.arges@canonical.com>,
	Borislav Petkov <bp@suse.de>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H . Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, linux-kernel@vger.kernel.org,
	Douglas Anderson <dianders@chromium.org>,
	Michael Davidson <md@google.com>,
	Greg Hackmann <ghackmann@google.com>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Stephen Hines <srhines@google.com>,
	Kees Cook <keescook@chromium.org>, Arnd Bergmann <arnd@arndb.de>,
	Bernhard.Rosenkranzer@linaro.org
Subject: Re: [PATCH] Revert "x86/uaccess: Add stack frame output operand in get_user() inline asm"
Date: Wed, 12 Jul 2017 17:12:42 -0500	[thread overview]
Message-ID: <20170712221242.puv5illztsla4nph@treble> (raw)
In-Reply-To: <20170712212744.23660-1-mka@chromium.org>

On Wed, Jul 12, 2017 at 02:27:44PM -0700, Matthias Kaehlcke wrote:
> Commit f05058c4d652 supposedly "forces a stack frame to be created before
> the inline asm code if CONFIG_FRAME_POINTER is enabled by listing the
> stack pointer as an output operand for the get_user() inline assembly
> statement.". This doesn't work as intended, at least with gcc v4.9.2 and
> x86-64 the generated code is exactly the same with and without the patch.
> However clang adds an extra instruction that adjusts %rsp, which ends up
> causing double faults all over the place.

I don't think reverting it is the right approach, because that will
still break frame pointers in certain cases.

The original commit probably should have clarified:

 " ... forces a stack frame *if it doesn't already exist*."

In *most* cases it will have no effect, as you saw, because users of
get_user() tend to do other function calls beforehand, so they will have
already saved the frame pointer before calling it.

However, that isn't always the case.  We found that certain configs
change GCC's behavior such that, for certain get_user() call sites, the
containing function doesn't saved the frame pointer before inserting
get_user()'s inline asm.

GCC completely ignores inline asm, so it has no idea that it has a call
instruction in it.  So in general, *any* inline asm with a call
instruction needs this constraint, to force the frame pointer to be
saved, if it hasn't already.

This is admittedly an awkward way of achieving this goal, but it's the
only way I know how to do it with GCC.

What extra instruction does clang add?

-- 
Josh