Messages in this thread | | | Date | Thu, 27 Jan 2022 23:33:12 +0000 | From | Sean Christopherson <> | Subject | Re: [RFC][PATCH v2 4/5] x86/uaccess: Implement unsafe_try_cmpxchg_user() |
| |
+Nick
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.
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
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.
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.
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
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.
Options #2 and #3 are quite gross, but I _think_ would be ok since the sequence is tagged as clobbering memory anyways?
| |