Re: [GIT PULL] x86/asm updates for v5.10

[Arvind Sankar <nivedita@alum.mit.edu>]

On Mon, Oct 12, 2020 at 11:11:35AM -0700, Linus Torvalds wrote:
> On Mon, Oct 12, 2020 at 4:06 AM Borislav Petkov <bp@suse.de> wrote:
> >
> > * Use XORL instead of XORQ to avoid a REX prefix and save some bytes in
> > the .fixup section, by Uros Bizjak.
> 
> I think this one is actually buggy.
> 
> For the 1-byte case, it does this:
> 
>      __get_user_asm(x_u8__, ptr, retval, "b", "=q");
> 
> and ends up doing "xorl" on a register that we told the compiler is a
> byte register (with that "=q")

It's not the "q", but the size of the l-value specified that tells the
compiler what to use. So x_u8__ does make it use a byte register, and it
would even with an "r" constraint. I think "q" is there only in case you
want to access the low byte of a bigger operand, to force the compiler
to use only a,b,c,d in 32-bit mode.

> 
> Yes, it uses "%k[output]" to turn that byte register into the word
> version of the register, but there's no fundamental reason why the
> register might not be something like "%ah".
> 
> Does the "xorl" work? Does it build? Yes, and yes.
> 
> But maybe %al contains SOMETHING ELSE, and it now clears that too,
> because the asm is basically doing something completely different than
> what we told the compiler it would do.
> 
> Now, afaik, gcc (and presumably clang) basically almost never use the
> high byte registers. But I still think this patch is fundamentally
> wrong and conceptually completely buggy, even if it might work in
> practice.
> 
> Also, I'm going to uninline this nasty __get_user() function anyway
> for 5.10, so the patch ends up being not just wrong, but pointless.
> This is not some kind of hot code that should be optimized, and the
> extra byte is not a lot to worry about.
> 
> Annoying. Because the other patch in this pull request is fine, and
> people want it.
> 
> But I'm going to skip this pull request, because I really think it's
> dangerously and subtly buggy even if there might not be any case that
> matters in reality.
> 
>                    Linus