On Mon, Oct 12, 2020 at 11:11:35AM -0700, Linus Torvalds wrote: > On Mon, Oct 12, 2020 at 4:06 AM Borislav Petkov <b...@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
