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

On Thu, Jan 27, 2022, Nick Desaulniers wrote:

On Thu, Jan 27, 2022 at 3:33 PM Sean Christopherson [off-list ref] wrote:

quoted

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.

quoted

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.