From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3C9D6C433F5 for ; Fri, 28 Jan 2022 00:17:33 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4758E6B0071; Thu, 27 Jan 2022 19:17:32 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 4231C6B0072; Thu, 27 Jan 2022 19:17:32 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2EB106B0073; Thu, 27 Jan 2022 19:17:32 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0124.hostedemail.com [216.40.44.124]) by kanga.kvack.org (Postfix) with ESMTP id 1EB6A6B0071 for ; Thu, 27 Jan 2022 19:17:32 -0500 (EST) Received: from smtpin31.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id D474895293 for ; Fri, 28 Jan 2022 00:17:31 +0000 (UTC) X-FDA: 79077781902.31.7945BDD Received: from mail-lf1-f46.google.com (mail-lf1-f46.google.com [209.85.167.46]) by imf03.hostedemail.com (Postfix) with ESMTP id 4C7352000B for ; Fri, 28 Jan 2022 00:17:31 +0000 (UTC) Received: by mail-lf1-f46.google.com with SMTP id o12so8426731lfg.12 for ; Thu, 27 Jan 2022 16:17:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=IlSEJdjJrMjk0uBxQ1Hapn8YGZniC2X8+hlUSQJTZwM=; b=b0GAq2WgzrvcsiDSn/b3NReLAu13yP5fhdkdH7+7CT6UB+7fWmzEeAHsKA4dDNoeNo paizE2t2lPrMvJe9bILSyix2rdq0PRr2zY9MJG4pVC5F5WXz5wRRUk5hlzex7z7PLHob asTr+61XLgdhhPpPp0RKDRELmqf75iv7SoSyPKD9BNktRE6TCLbmzkjFgoJEVVA7g3zb 6CjiFZP8sW76YJ+0pW8jd9eDQvnRKX1hdSL940vNICgmeRzitsOZ4qJcnM++xAhIrNJX KLerT7V508LixrbrvdMjcwYUJsJzwlwqhT/00ZQBa3tUYwEQaJOlcy1E3kH0EaMhdcMY AcdA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=IlSEJdjJrMjk0uBxQ1Hapn8YGZniC2X8+hlUSQJTZwM=; b=cmCOKU+DVz30dwcVUtUDgZCe/upt13SLbqXxBbXfQr7vUe6TytNSS4drpEXsLeYLvn KuxGGOepG70GFYG5EMjrgPqqoHBGTZU6cvOnRmuwBHWazXd2uxsgoOyf9ZEMBVD7wGpI nGpjxhf6K7KysRyPVX6DKJ1wung3hYJ1++pIZIudXEu6a7U3mlgrnMfPDgmKFZhejPZ5 974GPO+O/dH0VPxkzOdLnBTyhg5P/ZVFFYxyih06OhdF2s0Czn5VIVGugIkxx1nSjmkW 83TgWJO17BBFj0o2m8QRjRDgpU708cFbHx5iFIMPgN8iKa+hwQEPH4iPi92H5XA0bwb+ 65qQ== X-Gm-Message-State: AOAM533gDxC5ADjv3vvM1Dm2m+SJhrdAV9wCHtixTW+pyQRSDP9QjM0m gnEfFsOmTZdhuGroslcwQXdhBz2BlBgkOzfCTT1N8Q== X-Google-Smtp-Source: ABdhPJwwq3RWKdq6zaQeBD4zp7BWV/yEz0bPSG5n3byJeJMyzqq7BkL9gHT/C/ZRY8175jaO1A1XGgvkI1yjZL5Hpfk= X-Received: by 2002:ac2:4c4c:: with SMTP id o12mr4346659lfk.523.1643329049299; Thu, 27 Jan 2022 16:17:29 -0800 (PST) MIME-Version: 1.0 References: <20220120155517.066795336@infradead.org> <20220120160822.852009966@infradead.org> In-Reply-To: From: Nick Desaulniers Date: Thu, 27 Jan 2022 16:17:17 -0800 Message-ID: Subject: Re: [RFC][PATCH v2 4/5] x86/uaccess: Implement unsafe_try_cmpxchg_user() To: Sean Christopherson Cc: Peter Zijlstra , 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 , llvm@lists.linux.dev Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 4C7352000B X-Rspam-User: nil Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=b0GAq2Wg; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf03.hostedemail.com: domain of ndesaulniers@google.com designates 209.85.167.46 as permitted sender) smtp.mailfrom=ndesaulniers@google.com X-Stat-Signature: eufxf1u5q9i6qubmpuksztpcqwqfndkn X-HE-Tag: 1643329051-724239 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Jan 27, 2022 at 3:33 PM Sean Christopherson wro= te: > > +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-bi= t 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 a= dd a slightly > > > > less unsafe version that does __uaccess_begin_nospec()? KVM pre-ch= ecks the address > > > > way ahead of time, so the access_ok() check can be omitted. Altern= atively, KVM > > > > could add its own macro, but that seems a little silly. E.g. somet= hign like this, > > > > though I don't think this is correct > > > > > > *sigh* > > > > > > Finally realized I forgot to add back the page offset after convertin= g 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 "= =3Dm" 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 > :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; } > ^ > :1:29: error: unknown token in expression > :1:9: note: instantiated into assembly here > .long () - . > ^ > 2 errors generated. > > While clang is perfectly happy switching "+m" to "=3Dm": > > $ echo 'int foo(int *x) { asm goto (".long (%l[bar]) - .\n": "=3Dm"(*x)= ::: bar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null =3D 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 er= ror: > > $ echo 'int foo(int *x) { asm goto (".long (%l1) - .\n": "+m"(*x) ::: b= ar); return *x; bar: return 0; }' | clang -x c - -c -o /dev/null > :1:29: error: invalid operand in inline asm: '.long (${1:l}) - .= ' > int foo(int *x) { asm goto (".long (%l1) - .\n": "+m"(*x) ::: bar); ret= urn *x; bar: return 0; } > ^ > :1:29: error: unknown token in expression > :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 d= efining > tmp1, that didn't work :-) ). > > $ echo 'int foo(int *x) { asm goto (".long (%l2) - .\n": "+m"(*x) ::: b= ar); 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) ::: ba= r); return *x; bar: return 0; }' | gcc -x c - -c -o /dev/null > : In function =E2=80=98foo=E2=80=99: > :1:19: error: invalid 'asm': '%l' operand isn't a label > > $ echo 'int foo(int *x) { asm goto (".long (%l2) - .\n": "+m"(*x) ::: b= ar); 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=3D98096#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 kern= el? 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 "=3Dm" operand. > > 3) Use an input register param. > > Option #1 has the obvious downside of the fancier asm goto for __get_use= r_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 quit= e 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 se= quence > is tagged as clobbering memory anyways? --=20 Thanks, ~Nick Desaulniers