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;
Date:   Wed, 02 Sep 2020 05:45:15 -0700
User-Agent: K-9 Mail for Android
In-Reply-To: <20200901161857.566142-1-namit@vmware.com>
References: <20200901161857.566142-1-namit@vmware.com>
MIME-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PATCH] x86/special_insn: reverse __force_order logic
To:     Nadav Amit <nadav.amit@gmail.com>, linux-kernel@vger.kernel.org,
        Thomas Gleixner <tglx@linutronix.de>
CC:     Nadav Amit <namit@vmware.com>, Ingo Molnar <mingo@redhat.com>,
        Borislav Petkov <bp@alien8.de>, x86@kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Andy Lutomirski <luto@kernel.org>,
        Kees Cook <keescook@chromium.org>, stable@vger.kernel.org
From:   hpa@zytor.com
Message-ID: <493788DD-A9BE-4D48-94D1-2E3B8AE6BA4E@zytor.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On September 1, 2020 9:18:57 AM PDT, Nadav Amit <nadav=2Eamit@gmail=2Ecom> =
wrote:
>From: Nadav Amit <namit@vmware=2Ecom>
>
>The __force_order logic seems to be inverted=2E __force_order is
>supposedly used to manipulate the compiler to use its memory
>dependencies analysis to enforce orders between CR writes and reads=2E
>Therefore, the memory should behave as a "CR": when the CR is read, the
>memory should be "read" by the inline assembly, and __force_order
>should
>be an output=2E When the CR is written, the memory should be "written"=2E
>
>This change should allow to remove the "volatile" qualifier from CR
>reads at a later patch=2E
>
>While at it, remove the extra new-line from the inline assembly, as it
>only confuses GCC when it estimates the cost of the inline assembly=2E
>
>Cc: Thomas Gleixner <tglx@linutronix=2Ede>
>Cc: Ingo Molnar <mingo@redhat=2Ecom>
>Cc: Borislav Petkov <bp@alien8=2Ede>
>Cc: x86@kernel=2Eorg
>Cc: "H=2E Peter Anvin" <hpa@zytor=2Ecom>
>Cc: Peter Zijlstra <peterz@infradead=2Eorg>
>Cc: Andy Lutomirski <luto@kernel=2Eorg>
>Cc: Kees Cook <keescook@chromium=2Eorg>
>Cc: stable@vger=2Ekernel=2Eorg
>Signed-off-by: Nadav Amit <namit@vmware=2Ecom>
>
>---
>
>Unless I misunderstand the logic, __force_order should also be used by
>rdpkru() and wrpkru() which do not have dependency on __force_order=2E I
>also did not understand why native_write_cr0() has R/W dependency on
>__force_order, and why native_write_cr4() no longer has any dependency
>on __force_order=2E
>---
> arch/x86/include/asm/special_insns=2Eh | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
>diff --git a/arch/x86/include/asm/special_insns=2Eh
>b/arch/x86/include/asm/special_insns=2Eh
>index 5999b0b3dd4a=2E=2Edff5e5b01a3c 100644
>--- a/arch/x86/include/asm/special_insns=2Eh
>+++ b/arch/x86/include/asm/special_insns=2Eh
>@@ -24,32 +24,32 @@ void native_write_cr0(unsigned long val);
> static inline unsigned long native_read_cr0(void)
> {
> 	unsigned long val;
>-	asm volatile("mov %%cr0,%0\n\t" : "=3Dr" (val), "=3Dm" (__force_order))=
;
>+	asm volatile("mov %%cr0,%0" : "=3Dr" (val) : "m" (__force_order));
> 	return val;
> }
>=20
> static __always_inline unsigned long native_read_cr2(void)
> {
> 	unsigned long val;
>-	asm volatile("mov %%cr2,%0\n\t" : "=3Dr" (val), "=3Dm" (__force_order))=
;
>+	asm volatile("mov %%cr2,%0" : "=3Dr" (val) : "m" (__force_order));
> 	return val;
> }
>=20
> static __always_inline void native_write_cr2(unsigned long val)
> {
>-	asm volatile("mov %0,%%cr2": : "r" (val), "m" (__force_order));
>+	asm volatile("mov %1,%%cr2" : "=3Dm" (__force_order) : "r" (val));
> }
>=20
> static inline unsigned long __native_read_cr3(void)
> {
> 	unsigned long val;
>-	asm volatile("mov %%cr3,%0\n\t" : "=3Dr" (val), "=3Dm" (__force_order))=
;
>+	asm volatile("mov %%cr3,%0" : "=3Dr" (val) : "m" (__force_order));
> 	return val;
> }
>=20
> static inline void native_write_cr3(unsigned long val)
> {
>-	asm volatile("mov %0,%%cr3": : "r" (val), "m" (__force_order));
>+	asm volatile("mov %1,%%cr3" : "=3Dm" (__force_order) : "r" (val));
> }
>=20
> static inline unsigned long native_read_cr4(void)
>@@ -64,10 +64,10 @@ static inline unsigned long native_read_cr4(void)
> 	asm volatile("1: mov %%cr4, %0\n"
> 		     "2:\n"
> 		     _ASM_EXTABLE(1b, 2b)
>-		     : "=3Dr" (val), "=3Dm" (__force_order) : "0" (0));
>+		     : "=3Dr" (val) : "m" (__force_order), "0" (0));
> #else
> 	/* CR4 always exists on x86_64=2E */
>-	asm volatile("mov %%cr4,%0\n\t" : "=3Dr" (val), "=3Dm" (__force_order))=
;
>+	asm volatile("mov %%cr4,%0" : "=3Dr" (val) : "m" (__force_order));
> #endif
> 	return val;
> }

This seems reasonable to me, and I fully agree with you that the logic see=
ms to be just plain wrong (and unnoticed since all volatile operations are =
strictly ordered anyway), but you better not remove the volatile from cr2 r=
ead=2E=2E=2E unlike the other CRs that one is written by hardware and so ge=
nuinely is volatile=2E

However, I do believe "=3Dm" is at least theoretically wrong=2E You are on=
ly writing *part* of the total state represented by this token (you can thi=
nk of it as equivalent to a bitop), so it should be "+m"=2E
--=20
Sent from my Android device with K-9 Mail=2E Please excuse my brevity=2E