Re: [RFC][PATCH v2 4/5] x86/uaccess: Implement unsafe_try_cmpxchg_user()

From: Sean Christopherson <seanjc@google.com>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	mingo@redhat.com, tglx@linutronix.de, juri.lelli@redhat.com,
	vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
	rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
	bristot@redhat.com, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, linux-api@vger.kernel.org, x86@kernel.org,
	pjt@google.com, posk@google.com, avagin@google.com,
	jannh@google.com, tdelisle@uwaterloo.ca, mark.rutland@arm.com,
	posk@posk.io, James Y Knight <jyknight@google.com>,
	llvm@lists.linux.dev
Subject: Re: [RFC][PATCH v2 4/5] x86/uaccess: Implement unsafe_try_cmpxchg_user()
Date: Fri, 28 Jan 2022 16:29:37 +0000	[thread overview]
Message-ID: <YfQZ8cXPDraUqLMN@google.com> (raw)
In-Reply-To: <CAKwvOdmPkCuYuMRBSeK7DaK-wrdH5+C0gL63eqJ1buHsmueFsg@mail.gmail.com>

On Thu, Jan 27, 2022, Nick Desaulniers wrote:
> On Thu, Jan 27, 2022 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
> > Regarding the param number, gcc also appears to have a goof with asm goto and "+m",
> > but bumping the param number in that case remedies its woes.
> >
> >   $echo 'int foo(int *x) { asm goto (".long (%l1) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | gcc -x c - -c -o /dev/null
> >   <stdin>: In function ‘foo’:
> >   <stdin>:1:19: error: invalid 'asm': '%l' operand isn't a label
> >
> >   $ echo 'int foo(int *x) { asm goto (".long (%l2) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | gcc -x c - -c -o /dev/null
> 
> Right, so in fixing the above issue with tied outputs, I noticed that
> the GCC implementation of asm goto with outputs had different
> behavior. I changed clang's implementation in clang-14 (same patch
> series) to match:
> https://reviews.llvm.org/rG5c562f62a4ee15592f5d764d0710553a4b07a6f2
> This comment summarizes most of my thoughts on the issue:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98096#c7
> Specifically the quote "It appears to me that the GCC decision here
> was accidental, and that when pointed out, the bug was simply
> documented rather than fixed."

I guess that makes a certain amount of sense, but wow that is subtle, confusing,
and potentially dangerous.  Looks like the hidden inputs are numbered after all
explicit inputs, otherwise there would be broken code left and right, but it means
that a typo like so:

  echo 'int foo(int x) { asm ("xor %0, %0; xor %2, %2" : "+a"(x) : "b"(x)); return x; return 0; }' | clang -x c - -c -o tmp.o

will compile cleanly.

> Though I think compatibility between compilers is ultimately more
> important.  There's no standards bodies involved in these extension,
> which is simultaneously more flexible, yet can also lead to
> differences in implementations like this. Thanks for attending my TED
> talk.
> 
> >
> >
> > So my immediate question: how do we want to we deal with this in the kernel?  Keeping
> > in mind that I'd really like to send this to stable@ to fix the KVM mess.
> >
> > I can think of few options that are varying degrees of gross.
> >
> >   1) Use a more complex sequence for probing CC_HAS_ASM_GOTO_OUTPUT.
> >
> >   2) Use an output-only "=m" operand.
> >
> >   3) Use an input register param.
> >
> > Option #1 has the obvious downside of the fancier asm goto for  __get_user_asm()
> > and friends being collateral damage.  The biggest benefit is it'd reduce the
> > likelihood of someone else having to debug similar errors, which was quite painful.
> 
> Right; I assumed we'd hit this at some point, as soon as people wanted
> to used tied outputs with asm goto.  I'd rather have a different
> Kconfig test for working tied outputs, 

Is it all tied outputs, or just "+m"?  E.g. using "+r" compiles cleanly.

  echo 'int foo(int x) { asm goto ("xor %0, %0;.long (%l[bar]) - .\n": "+r"(x) ::: bar); return x; bar: return 0; }' | clang -x c - -c -o /dev/null

It might be a moot point as I can't find any instances of "+"-anything in conjunction
with asm_volatile_goto, i.e. adding CC_HAS_ASM_GOTO_OUTPUT_TIED_OUTPUTS won't create
an inconsistency with existing code.

Regardless, I like that idea.

> and that all uses in the kernels used the symbolic references which are much
> more readable and less confusing than the rules for numbered references
> (which are bug prone IMO).

100% agree, even though it takes me twice as long to write because I can never
remember the syntax :-)  Converting all existing usage is probably a fools errand,
but adding a checkpatch rule would get us going in the right direction.