Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201023203154.27335-1-linux@rasmusvillemoes.dk>
In-Reply-To: <20201023203154.27335-1-linux@rasmusvillemoes.dk>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Fri, 23 Oct 2020 13:42:39 -0700
Message-ID: <CALCETrV2rzowdWqhEG7v-hmbUOpNHsBz-hR1RdxBFKuwU6rDTA@mail.gmail.com>
Subject: Re: [PATCH] x86/uaccess: fix code generation in put_user()
To:     Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        X86 ML <x86@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Naresh Kamboju <naresh.kamboju@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 23, 2020 at 1:32 PM Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> Quoting
> https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html:
>
>   You can define a local register variable and associate it with a
>   specified register...
>
>   The only supported use for this feature is to specify registers for
>   input and output operands when calling Extended asm (see Extended
>   Asm). This may be necessary if the constraints for a particular
>   machine don't provide sufficient control to select the desired
>   register.
>
> On 32-bit x86, this is used to ensure that gcc will put an 8-byte
> value into the %edx:%eax pair, while all other cases will just use the
> single register %eax (%rax on x86-64). While the _ASM_AX actually just
> expands to "%eax", note this comment next to get_user() which does
> something very similar:
>
>  * The use of _ASM_DX as the register specifier is a bit of a
>  * simplification, as gcc only cares about it as the starting point
>  * and not size: for a 64-bit value it will use %ecx:%edx on 32 bits
>  * (%ecx being the next register in gcc's x86 register sequence), and
>  * %rdx on 64 bits.
>
> However, getting this to work requires that there is no code between
> the assignment to the local register variable and its use as an input
> to the asm() which can possibly clobber any of the registers involved
> - including evaluation of the expressions making up other inputs.

This looks like the patch is an improvement, but this is still IMO
likely to be very fragile.  Can we just do the size-dependent "a" vs
"A" selection method instead?  Sure, it's a little more code, but it
will be Obviously Correct.  As it stands, I can easily see our code
failing on some gcc or clang version and the compiler authors telling
us that we're relying on unsupportable behavior.