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;
Sender: Arvind Sankar <niveditas98@gmail.com>
From:   Arvind Sankar <nivedita@alum.mit.edu>
Date:   Mon, 12 Oct 2020 16:27:00 -0400
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Borislav Petkov <bp@suse.de>, Uros Bizjak <ubizjak@gmail.com>,
        x86-ml <x86@kernel.org>, lkml <linux-kernel@vger.kernel.org>
Subject: Re: [GIT PULL] x86/asm updates for v5.10
Message-ID: <20201012202700.GA818459@rani.riverdale.lan>
References: <20201012110557.GK25311@zn.tnic>
 <CAHk-=wiVoLoZS1v9SCQzH4mkpz6akE3pPrHPxM2hamOXPcaW9w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <CAHk-=wiVoLoZS1v9SCQzH4mkpz6akE3pPrHPxM2hamOXPcaW9w@mail.gmail.com>
Precedence: bulk

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