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

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

[Nick Desaulniers]

On Thu, Jan 27, 2022 at 3:33 PM Sean Christopherson <seanjc@google.com> wrote:
>
> +Nick

(well, you asked the right person; you probably wont like the answer
much though)

>
> On Thu, Jan 27, 2022, Peter Zijlstra wrote:
> > On Thu, Jan 27, 2022 at 06:36:19AM +0000, Sean Christopherson wrote:
> > > On Thu, Jan 27, 2022, Sean Christopherson wrote:
> > > > Doh, I should have specified that KVM needs 8-byte CMPXCHG on 32-bit kernels due
> > > > to using it to atomically update guest PAE PTEs and LTR descriptors (yay).
> > > >
> > > > Also, KVM's use case isn't a tight loop, how gross would it be to add a slightly
> > > > less unsafe version that does __uaccess_begin_nospec()?  KVM pre-checks the address
> > > > way ahead of time, so the access_ok() check can be omitted.  Alternatively, KVM
> > > > could add its own macro, but that seems a little silly.  E.g. somethign like this,
> > > > though I don't think this is correct
> > >
> > > *sigh*
> > >
> > > Finally realized I forgot to add back the page offset after converting from guest
> > > page frame to host virtual address.  Anyways, this is what I ended up with, will
> > > test more tomorrow.
> >
> > Looks about right :-) (famous last words etc..)
>
> And it was right, but clang-13 ruined the party :-/
>
> clang barfs on asm goto with a "+m" input/output.  Change the "+m" to "=m" and
> clang is happy.  Remove usage of the label, clang is happy.

Yep, sorry, we only recently noticed that this was broken.  I fixed
this very recently (over the holidays) in clang-14, and is too risky
and late to backport to clang-13 IMO.
https://reviews.llvm.org/rG4edb9983cb8c8b850083ee5941468f5d0ef6fe2c

>
> I tried a bunch of different variants to see if anything would squeak by, but
> clang found a way to die on everything I threw at it.
>
> $ clang --version
>
>   Debian clang version 13.0.0-9+build1
>   Target: x86_64-pc-linux-gnu
>   Thread model: posix
>   InstalledDir: /usr/bin
>
> As written, with a named label param, clang yields:
>
>   $ echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null
>   <stdin>:1:29: error: invalid operand in inline asm: '.long (${1:l}) - .'
>   int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }
>                             ^
>   <stdin>:1:29: error: unknown token in expression
>   <inline asm>:1:9: note: instantiated into assembly here
>           .long () - .
>                ^
>   2 errors generated.
>
> While clang is perfectly happy switching "+m" to "=m":
>
>   $ echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "=m"(*x) ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null

= constraints should work with older clang releases; + constraints are
what was broken (Not generally, only when using asm goto with
outputs), fixed in clang-14.

>
> Referencing the label with a numbered param yields either the original error:
>
>   $ echo 'int foo(int *x) { asm goto (".long (%l1) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null
>   <stdin>:1:29: error: invalid operand in inline asm: '.long (${1:l}) - .'
>   int foo(int *x) { asm goto (".long (%l1) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }
>                             ^
>   <stdin>:1:29: error: unknown token in expression
>   <inline asm>:1:9: note: instantiated into assembly here
>           .long () - .
>                  ^
>    2 errors generated.

^ That case should not work in either compilers, more info below...

>
> Bumping the param number (more below) yields a different error (I tried defining
> tmp1, that didn't work :-) ).
>
>   $ echo 'int foo(int *x) { asm goto (".long (%l2) - .\n": "+m"(*x) ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null
>   error: Undefined temporary symbol .Ltmp1
>   1 error generated.

"Bumping the param number" will be required when using numbered
references, more info below...

>
> 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."
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, 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).

>
> Options #2 and #3 are quite gross, but I _think_ would be ok since the sequence
> is tagged as clobbering memory anyways?

-- 
Thanks,
~Nick Desaulniers

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

[Sean Christopherson]

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.