Re: [PATCH] x86/entry: Fix AC assertion
On 24.08.20 17:58, Andrew Cooper wrote: On 24/08/2020 16:21, pet...@infradead.org wrote: On Mon, Aug 24, 2020 at 03:22:06PM +0100, Andrew Cooper wrote: On 24/08/2020 11:14, pet...@infradead.org wrote: The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further improve user entry sanity checks") unconditionally triggers on my IVB machine because it does not support SMAP. For !SMAP hardware we patch out CLAC/STAC instructions and thus if userspace sets AC, we'll still have it set after entry. Technically, you don't patch in, rather than patch out. True. Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry sanity checks") Signed-off-by: Peter Zijlstra (Intel) Acked-by: Andy Lutomirski --- arch/x86/include/asm/entry-common.h | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) --- a/arch/x86/include/asm/entry-common.h +++ b/arch/x86/include/asm/entry-common.h @@ -18,8 +18,15 @@ static __always_inline void arch_check_u * state, not the interrupt state as imagined by Xen. */ unsigned long flags = native_save_fl(); - WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF | - X86_EFLAGS_NT)); + unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT; + + /* +* For !SMAP hardware we patch out CLAC on entry. +*/ + if (boot_cpu_has(X86_FEATURE_SMAP)) + mask |= X86_EFLAGS_AC; The Xen PV ABI clears AC on entry for 64bit guests, because Linux is actually running in Ring 3, and therefore susceptible to #AC's which wouldn't occur natively. So do you then want it to be something like: if (boot_cpu_has(X86_FEATURE_SMAP) || (IS_ENABLED(CONFIG_64_BIT) && boot_cpu_has(X86_FEATURE_XENPV))) ? Or are you fine with the proposed? Dealers choice, but this option would be slightly better overall. (Are there any other cases where Linux will be running in Ring 3? I haven't been paying attention to recent changes in PVOps.) I'm not aware of any other case running kernel code in ring 3. Juergen
Re: [PATCH] x86/entry: Fix AC assertion
On 24/08/2020 16:21, pet...@infradead.org wrote: > On Mon, Aug 24, 2020 at 03:22:06PM +0100, Andrew Cooper wrote: >> On 24/08/2020 11:14, pet...@infradead.org wrote: >>> The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further >>> improve user entry sanity checks") unconditionally triggers on my IVB >>> machine because it does not support SMAP. >>> >>> For !SMAP hardware we patch out CLAC/STAC instructions and thus if >>> userspace sets AC, we'll still have it set after entry. >> Technically, you don't patch in, rather than patch out. > True. > >>> Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry >>> sanity checks") >>> Signed-off-by: Peter Zijlstra (Intel) >>> Acked-by: Andy Lutomirski >>> --- >>> arch/x86/include/asm/entry-common.h | 11 +-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> --- a/arch/x86/include/asm/entry-common.h >>> +++ b/arch/x86/include/asm/entry-common.h >>> @@ -18,8 +18,15 @@ static __always_inline void arch_check_u >>> * state, not the interrupt state as imagined by Xen. >>> */ >>> unsigned long flags = native_save_fl(); >>> - WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF | >>> - X86_EFLAGS_NT)); >>> + unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT; >>> + >>> + /* >>> +* For !SMAP hardware we patch out CLAC on entry. >>> +*/ >>> + if (boot_cpu_has(X86_FEATURE_SMAP)) >>> + mask |= X86_EFLAGS_AC; >> The Xen PV ABI clears AC on entry for 64bit guests, because Linux is >> actually running in Ring 3, and therefore susceptible to #AC's which >> wouldn't occur natively. > So do you then want it to be something like: > > if (boot_cpu_has(X86_FEATURE_SMAP) || > (IS_ENABLED(CONFIG_64_BIT) && boot_cpu_has(X86_FEATURE_XENPV))) > > ? Or are you fine with the proposed? Dealers choice, but this option would be slightly better overall. (Are there any other cases where Linux will be running in Ring 3? I haven't been paying attention to recent changes in PVOps.) ~Andrew
Re: [PATCH] x86/entry: Fix AC assertion
On Mon, Aug 24, 2020 at 03:22:06PM +0100, Andrew Cooper wrote: > On 24/08/2020 11:14, pet...@infradead.org wrote: > > The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further > > improve user entry sanity checks") unconditionally triggers on my IVB > > machine because it does not support SMAP. > > > > For !SMAP hardware we patch out CLAC/STAC instructions and thus if > > userspace sets AC, we'll still have it set after entry. > > Technically, you don't patch in, rather than patch out. True. > > Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry > > sanity checks") > > Signed-off-by: Peter Zijlstra (Intel) > > Acked-by: Andy Lutomirski > > --- > > arch/x86/include/asm/entry-common.h | 11 +-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > --- a/arch/x86/include/asm/entry-common.h > > +++ b/arch/x86/include/asm/entry-common.h > > @@ -18,8 +18,15 @@ static __always_inline void arch_check_u > > * state, not the interrupt state as imagined by Xen. > > */ > > unsigned long flags = native_save_fl(); > > - WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF | > > - X86_EFLAGS_NT)); > > + unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT; > > + > > + /* > > +* For !SMAP hardware we patch out CLAC on entry. > > +*/ > > + if (boot_cpu_has(X86_FEATURE_SMAP)) > > + mask |= X86_EFLAGS_AC; > > The Xen PV ABI clears AC on entry for 64bit guests, because Linux is > actually running in Ring 3, and therefore susceptible to #AC's which > wouldn't occur natively. So do you then want it to be something like: if (boot_cpu_has(X86_FEATURE_SMAP) || (IS_ENABLED(CONFIG_64_BIT) && boot_cpu_has(X86_FEATURE_XENPV))) ? Or are you fine with the proposed?
Re: [PATCH] x86/entry: Fix AC assertion
On 24/08/2020 11:14, pet...@infradead.org wrote: > The WARN added in commit 3c73b81a9164 ("x86/entry, selftests: Further > improve user entry sanity checks") unconditionally triggers on my IVB > machine because it does not support SMAP. > > For !SMAP hardware we patch out CLAC/STAC instructions and thus if > userspace sets AC, we'll still have it set after entry. Technically, you don't patch in, rather than patch out. > > Fixes: 3c73b81a9164 ("x86/entry, selftests: Further improve user entry sanity > checks") > Signed-off-by: Peter Zijlstra (Intel) > Acked-by: Andy Lutomirski > --- > arch/x86/include/asm/entry-common.h | 11 +-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > --- a/arch/x86/include/asm/entry-common.h > +++ b/arch/x86/include/asm/entry-common.h > @@ -18,8 +18,15 @@ static __always_inline void arch_check_u >* state, not the interrupt state as imagined by Xen. >*/ > unsigned long flags = native_save_fl(); > - WARN_ON_ONCE(flags & (X86_EFLAGS_AC | X86_EFLAGS_DF | > - X86_EFLAGS_NT)); > + unsigned long mask = X86_EFLAGS_DF | X86_EFLAGS_NT; > + > + /* > + * For !SMAP hardware we patch out CLAC on entry. > + */ > + if (boot_cpu_has(X86_FEATURE_SMAP)) > + mask |= X86_EFLAGS_AC; The Xen PV ABI clears AC on entry for 64bit guests, because Linux is actually running in Ring 3, and therefore susceptible to #AC's which wouldn't occur natively. ~Andrew