* [PATCH] x86/cpu: Fix SMAP check in PVOPS environments @ 2015-06-03 9:31 Andrew Cooper 2015-06-04 6:38 ` H. Peter Anvin ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Andrew Cooper @ 2015-06-03 9:31 UTC (permalink / raw) To: Xen-devel Cc: David Vrabel, Rusty Russell, Andrew Cooper, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Konrad Rzeszutek Wilk, Boris Ostrovsky, lguest, stable There appears to be no formal statement of what pv_irq_ops.save_fl() is supposed to return precisely. Native returns the full flags, while lguest and Xen only return the Interrupt Flag, and both have comments by the implementations stating that only the Interrupt Flag is looked at. This may have been true when initially implemented, but no longer is. To make matters worse, the Xen PVOP leaves the upper bits undefined, making the BUG_ON() undefined behaviour. Experimentally, this now trips for 32bit PV guests on Broadwell hardware. The BUG_ON() is consistent for an individual build, but not consistent for all builds. It has also been a sitting timebomb since SMAP support was introduced. Use native_save_fl() instead, which will obtain an accurate view of the AC flag. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: David Vrabel <david.vrabel@citrix.com> Tested-by: Rusty Russell <rusty@rustcorp.com.au> CC: Thomas Gleixner <tglx@linutronix.de> CC: Ingo Molnar <mingo@redhat.com> CC: H. Peter Anvin <hpa@zytor.com> CC: x86@kernel.org CC: linux-kernel@vger.kernel.org CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> CC: xen-devel <xen-devel@lists.xen.org> CC: lguest@lists.ozlabs.org CC: stable@vger.kernel.org --- arch/x86/kernel/cpu/common.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index a62cf04..4f2fded 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -291,10 +291,9 @@ __setup("nosmap", setup_disable_smap); static __always_inline void setup_smap(struct cpuinfo_x86 *c) { - unsigned long eflags; + unsigned long eflags = native_save_fl(); /* This should have been cleared long ago */ - raw_local_save_flags(eflags); BUG_ON(eflags & X86_EFLAGS_AC); if (cpu_has(c, X86_FEATURE_SMAP)) { -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-03 9:31 [PATCH] x86/cpu: Fix SMAP check in PVOPS environments Andrew Cooper @ 2015-06-04 6:38 ` H. Peter Anvin 2015-06-04 8:58 ` Andrew Cooper 2015-11-17 14:59 ` Andrew Cooper 2015-11-19 10:12 ` [tip:x86/urgent] " tip-bot for Andrew Cooper 2 siblings, 1 reply; 10+ messages in thread From: H. Peter Anvin @ 2015-06-04 6:38 UTC (permalink / raw) To: Andrew Cooper, Xen-devel Cc: David Vrabel, Rusty Russell, Thomas Gleixner, Ingo Molnar, x86, linux-kernel, Konrad Rzeszutek Wilk, Boris Ostrovsky, lguest, stable On 06/03/2015 02:31 AM, Andrew Cooper wrote: > There appears to be no formal statement of what pv_irq_ops.save_fl() is > supposed to return precisely. Native returns the full flags, while lguest and > Xen only return the Interrupt Flag, and both have comments by the > implementations stating that only the Interrupt Flag is looked at. This may > have been true when initially implemented, but no longer is. > > To make matters worse, the Xen PVOP leaves the upper bits undefined, making > the BUG_ON() undefined behaviour. Experimentally, this now trips for 32bit PV > guests on Broadwell hardware. The BUG_ON() is consistent for an individual > build, but not consistent for all builds. It has also been a sitting timebomb > since SMAP support was introduced. > > Use native_save_fl() instead, which will obtain an accurate view of the AC > flag. Could we fix the Xen pvops wrapper instead to not do things like this? -hpa ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-04 6:38 ` H. Peter Anvin @ 2015-06-04 8:58 ` Andrew Cooper 2015-06-04 19:55 ` Rusty Russell 0 siblings, 1 reply; 10+ messages in thread From: Andrew Cooper @ 2015-06-04 8:58 UTC (permalink / raw) To: H. Peter Anvin, Xen-devel Cc: David Vrabel, Rusty Russell, Thomas Gleixner, Ingo Molnar, x86, linux-kernel, Konrad Rzeszutek Wilk, Boris Ostrovsky, lguest, stable On 04/06/15 07:38, H. Peter Anvin wrote: > On 06/03/2015 02:31 AM, Andrew Cooper wrote: >> There appears to be no formal statement of what pv_irq_ops.save_fl() is >> supposed to return precisely. Native returns the full flags, while lguest and >> Xen only return the Interrupt Flag, and both have comments by the >> implementations stating that only the Interrupt Flag is looked at. This may >> have been true when initially implemented, but no longer is. >> >> To make matters worse, the Xen PVOP leaves the upper bits undefined, making >> the BUG_ON() undefined behaviour. Experimentally, this now trips for 32bit PV >> guests on Broadwell hardware. The BUG_ON() is consistent for an individual >> build, but not consistent for all builds. It has also been a sitting timebomb >> since SMAP support was introduced. >> >> Use native_save_fl() instead, which will obtain an accurate view of the AC >> flag. > Could we fix the Xen pvops wrapper instead to not do things like this? > > -hpa > > We could, and I have a patch for that, but the check would still then be broken in lguest, and it makes a hotpath rather longer. Either pv_irq_ops.save_fl() gets defined to handle all flags, and Xen & lguest need correcting in this regard, or save_fl() gets defined to handle the interrupt flag only, and this becomes the single problematic caller in the codebase. The problem with expanding save_fl() to handle all flags is that restore_fl() should follow suit, and there are a number of system flags are inapplicable in such a situation. ~Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-04 8:58 ` Andrew Cooper @ 2015-06-04 19:55 ` Rusty Russell 2015-06-04 20:29 ` H. Peter Anvin 0 siblings, 1 reply; 10+ messages in thread From: Rusty Russell @ 2015-06-04 19:55 UTC (permalink / raw) To: Andrew Cooper, H. Peter Anvin, Xen-devel Cc: David Vrabel, Thomas Gleixner, Ingo Molnar, x86, linux-kernel, Konrad Rzeszutek Wilk, Boris Ostrovsky, lguest, stable Andrew Cooper <andrew.cooper3@citrix.com> writes: > On 04/06/15 07:38, H. Peter Anvin wrote: >> On 06/03/2015 02:31 AM, Andrew Cooper wrote: >>> There appears to be no formal statement of what pv_irq_ops.save_fl() is >>> supposed to return precisely. Native returns the full flags, while lguest and >>> Xen only return the Interrupt Flag, and both have comments by the >>> implementations stating that only the Interrupt Flag is looked at. This may >>> have been true when initially implemented, but no longer is. >>> >>> To make matters worse, the Xen PVOP leaves the upper bits undefined, making >>> the BUG_ON() undefined behaviour. Experimentally, this now trips for 32bit PV >>> guests on Broadwell hardware. The BUG_ON() is consistent for an individual >>> build, but not consistent for all builds. It has also been a sitting timebomb >>> since SMAP support was introduced. >>> >>> Use native_save_fl() instead, which will obtain an accurate view of the AC >>> flag. >> Could we fix the Xen pvops wrapper instead to not do things like this? >> >> -hpa >> >> > > We could, and I have a patch for that, but the check would still then be > broken in lguest, and it makes a hotpath rather longer. > > Either pv_irq_ops.save_fl() gets defined to handle all flags, and Xen & > lguest need correcting in this regard, or save_fl() gets defined to > handle the interrupt flag only, and this becomes the single problematic > caller in the codebase. > > The problem with expanding save_fl() to handle all flags is that > restore_fl() should follow suit, and there are a number of system flags > are inapplicable in such a situation. Yeah, hard cases make bad law. I'm not too unhappy with this fix; ideally we'd rename save_fl and restore_fl to save_eflags_if and restore_eflags_if too. Cheers, Rusty. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-04 19:55 ` Rusty Russell @ 2015-06-04 20:29 ` H. Peter Anvin 2015-06-05 2:58 ` Rusty Russell 0 siblings, 1 reply; 10+ messages in thread From: H. Peter Anvin @ 2015-06-04 20:29 UTC (permalink / raw) To: Rusty Russell, Andrew Cooper, Xen-devel Cc: David Vrabel, Thomas Gleixner, Ingo Molnar, x86, linux-kernel, Konrad Rzeszutek Wilk, Boris Ostrovsky, lguest, stable On 06/04/2015 12:55 PM, Rusty Russell wrote: > > Yeah, hard cases make bad law. > > I'm not too unhappy with this fix; ideally we'd rename save_fl and > restore_fl to save_eflags_if and restore_eflags_if too. > I would be fine with this... but please document what the bloody semantics of pvops is actually supposed to be. -hpa ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-04 20:29 ` H. Peter Anvin @ 2015-06-05 2:58 ` Rusty Russell 2015-06-05 7:13 ` Ingo Molnar 2015-06-05 9:33 ` [Xen-devel] " David Vrabel 0 siblings, 2 replies; 10+ messages in thread From: Rusty Russell @ 2015-06-05 2:58 UTC (permalink / raw) To: H. Peter Anvin, Andrew Cooper, Xen-devel Cc: David Vrabel, Thomas Gleixner, Ingo Molnar, x86, linux-kernel, Konrad Rzeszutek Wilk, Boris Ostrovsky, lguest, stable "H. Peter Anvin" <hpa@zytor.com> writes: > On 06/04/2015 12:55 PM, Rusty Russell wrote: >> >> Yeah, hard cases make bad law. >> >> I'm not too unhappy with this fix; ideally we'd rename save_fl and >> restore_fl to save_eflags_if and restore_eflags_if too. >> > > I would be fine with this... but please document what the bloody > semantics of pvops is actually supposed to be. *cough*. Lightly compile tested... Subject: x86: rename save_fl/restore_fl paravirt ops to highlight eflags. From: Rusty Russell <rusty@rustcorp.com.au> As the comment in arch/x86/include/asm/paravirt_types.h says: * Get/set interrupt state. save_fl and restore_fl are only * expected to use X86_EFLAGS_IF; all other bits * returned from save_fl are undefined, and may be ignored by * restore_fl. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h index 8957810ad7d1..5ad7b0e62a77 100644 --- a/arch/x86/include/asm/paravirt.h +++ b/arch/x86/include/asm/paravirt.h @@ -801,12 +801,12 @@ static __always_inline void __ticket_unlock_kick(struct arch_spinlock *lock, static inline notrace unsigned long arch_local_save_flags(void) { - return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl); + return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_eflags_if); } static inline notrace void arch_local_irq_restore(unsigned long f) { - PVOP_VCALLEE1(pv_irq_ops.restore_fl, f); + PVOP_VCALLEE1(pv_irq_ops.restore_eflags_if, f); } static inline notrace void arch_local_irq_disable(void) diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index f7b0b5c112f2..05425c8544f1 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -204,8 +204,8 @@ struct pv_irq_ops { * NOTE: These functions callers expect the callee to preserve * more registers than the standard C calling convention. */ - struct paravirt_callee_save save_fl; - struct paravirt_callee_save restore_fl; + struct paravirt_callee_save save_eflags_if; + struct paravirt_callee_save restore_eflags_if; struct paravirt_callee_save irq_disable; struct paravirt_callee_save irq_enable; diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c index c614dd492f5f..d42e33b66ee6 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -321,8 +321,8 @@ struct pv_time_ops pv_time_ops = { }; __visible struct pv_irq_ops pv_irq_ops = { - .save_fl = __PV_IS_CALLEE_SAVE(native_save_fl), - .restore_fl = __PV_IS_CALLEE_SAVE(native_restore_fl), + .save_eflags_if = __PV_IS_CALLEE_SAVE(native_save_fl), + .restore_eflags_if = __PV_IS_CALLEE_SAVE(native_restore_fl), .irq_disable = __PV_IS_CALLEE_SAVE(native_irq_disable), .irq_enable = __PV_IS_CALLEE_SAVE(native_irq_enable), .safe_halt = native_safe_halt, diff --git a/arch/x86/kernel/paravirt_patch_32.c b/arch/x86/kernel/paravirt_patch_32.c index d9f32e6d6ab6..cf20c1b37f43 100644 --- a/arch/x86/kernel/paravirt_patch_32.c +++ b/arch/x86/kernel/paravirt_patch_32.c @@ -2,8 +2,8 @@ DEF_NATIVE(pv_irq_ops, irq_disable, "cli"); DEF_NATIVE(pv_irq_ops, irq_enable, "sti"); -DEF_NATIVE(pv_irq_ops, restore_fl, "push %eax; popf"); -DEF_NATIVE(pv_irq_ops, save_fl, "pushf; pop %eax"); +DEF_NATIVE(pv_irq_ops, restore_eflags_if, "push %eax; popf"); +DEF_NATIVE(pv_irq_ops, save_eflags_if, "pushf; pop %eax"); DEF_NATIVE(pv_cpu_ops, iret, "iret"); DEF_NATIVE(pv_cpu_ops, irq_enable_sysexit, "sti; sysexit"); DEF_NATIVE(pv_mmu_ops, read_cr2, "mov %cr2, %eax"); @@ -38,8 +38,8 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf, switch (type) { PATCH_SITE(pv_irq_ops, irq_disable); PATCH_SITE(pv_irq_ops, irq_enable); - PATCH_SITE(pv_irq_ops, restore_fl); - PATCH_SITE(pv_irq_ops, save_fl); + PATCH_SITE(pv_irq_ops, restore_eflags_if); + PATCH_SITE(pv_irq_ops, save_eflags_if); PATCH_SITE(pv_cpu_ops, iret); PATCH_SITE(pv_cpu_ops, irq_enable_sysexit); PATCH_SITE(pv_mmu_ops, read_cr2); diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c index a1da6737ba5b..24eaaa5f9aa6 100644 --- a/arch/x86/kernel/paravirt_patch_64.c +++ b/arch/x86/kernel/paravirt_patch_64.c @@ -4,8 +4,8 @@ DEF_NATIVE(pv_irq_ops, irq_disable, "cli"); DEF_NATIVE(pv_irq_ops, irq_enable, "sti"); -DEF_NATIVE(pv_irq_ops, restore_fl, "pushq %rdi; popfq"); -DEF_NATIVE(pv_irq_ops, save_fl, "pushfq; popq %rax"); +DEF_NATIVE(pv_irq_ops, restore_eflags_if, "pushq %rdi; popfq"); +DEF_NATIVE(pv_irq_ops, save_eflags_if, "pushfq; popq %rax"); DEF_NATIVE(pv_mmu_ops, read_cr2, "movq %cr2, %rax"); DEF_NATIVE(pv_mmu_ops, read_cr3, "movq %cr3, %rax"); DEF_NATIVE(pv_mmu_ops, write_cr3, "movq %rdi, %cr3"); @@ -45,8 +45,8 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf, end = end_##ops##_##x; \ goto patch_site switch(type) { - PATCH_SITE(pv_irq_ops, restore_fl); - PATCH_SITE(pv_irq_ops, save_fl); + PATCH_SITE(pv_irq_ops, restore_eflags_if); + PATCH_SITE(pv_irq_ops, save_eflags_if); PATCH_SITE(pv_irq_ops, irq_enable); PATCH_SITE(pv_irq_ops, irq_disable); PATCH_SITE(pv_cpu_ops, irq_enable_sysexit); diff --git a/arch/x86/kernel/vsmp_64.c b/arch/x86/kernel/vsmp_64.c index ee22c1d93ae5..c7f09f3fb31d 100644 --- a/arch/x86/kernel/vsmp_64.c +++ b/arch/x86/kernel/vsmp_64.c @@ -78,8 +78,8 @@ static unsigned __init_or_module vsmp_patch(u8 type, u16 clobbers, void *ibuf, switch (type) { case PARAVIRT_PATCH(pv_irq_ops.irq_enable): case PARAVIRT_PATCH(pv_irq_ops.irq_disable): - case PARAVIRT_PATCH(pv_irq_ops.save_fl): - case PARAVIRT_PATCH(pv_irq_ops.restore_fl): + case PARAVIRT_PATCH(pv_irq_ops.save_eflags_if): + case PARAVIRT_PATCH(pv_irq_ops.restore_eflags_if): return paravirt_patch_default(type, clobbers, ibuf, addr, len); default: return native_patch(type, clobbers, ibuf, addr, len); @@ -119,8 +119,8 @@ static void __init set_vsmp_pv_ops(void) /* Setup irq ops and turn on vSMP IRQ fastpath handling */ pv_irq_ops.irq_disable = PV_CALLEE_SAVE(vsmp_irq_disable); pv_irq_ops.irq_enable = PV_CALLEE_SAVE(vsmp_irq_enable); - pv_irq_ops.save_fl = PV_CALLEE_SAVE(vsmp_save_fl); - pv_irq_ops.restore_fl = PV_CALLEE_SAVE(vsmp_restore_fl); + pv_irq_ops.save_eflags_if = PV_CALLEE_SAVE(vsmp_save_fl); + pv_irq_ops.restore_eflags_if = PV_CALLEE_SAVE(vsmp_restore_fl); pv_init_ops.patch = vsmp_patch; ctl &= ~(1 << 4); } diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c index 8f9a133cc099..5f35bf3ff0b0 100644 --- a/arch/x86/lguest/boot.c +++ b/arch/x86/lguest/boot.c @@ -1426,8 +1426,8 @@ __init void lguest_init(void) */ /* Interrupt-related operations */ - pv_irq_ops.save_fl = PV_CALLEE_SAVE(lguest_save_fl); - pv_irq_ops.restore_fl = __PV_IS_CALLEE_SAVE(lg_restore_fl); + pv_irq_ops.save_eflags_if = PV_CALLEE_SAVE(lguest_save_fl); + pv_irq_ops.restore_eflags_if = __PV_IS_CALLEE_SAVE(lg_restore_fl); pv_irq_ops.irq_disable = PV_CALLEE_SAVE(lguest_irq_disable); pv_irq_ops.irq_enable = __PV_IS_CALLEE_SAVE(lg_irq_enable); pv_irq_ops.safe_halt = lguest_safe_halt; diff --git a/arch/x86/lguest/head_32.S b/arch/x86/lguest/head_32.S index d5ae63f5ec5d..03b94d38fbd7 100644 --- a/arch/x86/lguest/head_32.S +++ b/arch/x86/lguest/head_32.S @@ -64,7 +64,7 @@ LGUEST_PATCH(pushf, movl lguest_data+LGUEST_DATA_irq_enabled, %eax) /*G:033 * But using those wrappers is inefficient (we'll see why that doesn't matter - * for save_fl and irq_disable later). If we write our routines carefully in + * for lguest_save_fl and irq_disable later). If we write our routines carefully in * assembler, we can avoid clobbering any registers and avoid jumping through * the wrapper functions. * diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index 46957ead3060..d9511df0d63c 100644 --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -1074,8 +1074,8 @@ void xen_setup_vcpu_info_placement(void) * percpu area for all cpus, so make use of it. Note that for * PVH we want to use native IRQ mechanism. */ if (have_vcpu_info_placement && !xen_pvh_domain()) { - pv_irq_ops.save_fl = __PV_IS_CALLEE_SAVE(xen_save_fl_direct); - pv_irq_ops.restore_fl = __PV_IS_CALLEE_SAVE(xen_restore_fl_direct); + pv_irq_ops.save_eflags_if = __PV_IS_CALLEE_SAVE(xen_save_fl_direct); + pv_irq_ops.restore_eflags_if = __PV_IS_CALLEE_SAVE(xen_restore_fl_direct); pv_irq_ops.irq_disable = __PV_IS_CALLEE_SAVE(xen_irq_disable_direct); pv_irq_ops.irq_enable = __PV_IS_CALLEE_SAVE(xen_irq_enable_direct); pv_mmu_ops.read_cr2 = xen_read_cr2_direct; @@ -1102,8 +1102,8 @@ static unsigned xen_patch(u8 type, u16 clobbers, void *insnbuf, switch (type) { SITE(pv_irq_ops, irq_enable); SITE(pv_irq_ops, irq_disable); - SITE(pv_irq_ops, save_fl); - SITE(pv_irq_ops, restore_fl); + SITE(pv_irq_ops, save_eflags_if); + SITE(pv_irq_ops, restore_eflags_if); #undef SITE patch_site: diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c index a1207cb6472a..a7f58c48c2e1 100644 --- a/arch/x86/xen/irq.c +++ b/arch/x86/xen/irq.c @@ -115,8 +115,8 @@ static void xen_halt(void) } static const struct pv_irq_ops xen_irq_ops __initconst = { - .save_fl = PV_CALLEE_SAVE(xen_save_fl), - .restore_fl = PV_CALLEE_SAVE(xen_restore_fl), + .save_eflags_if = PV_CALLEE_SAVE(xen_save_fl), + .restore_eflags_if = PV_CALLEE_SAVE(xen_restore_fl), .irq_disable = PV_CALLEE_SAVE(xen_irq_disable), .irq_enable = PV_CALLEE_SAVE(xen_irq_enable), ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-05 2:58 ` Rusty Russell @ 2015-06-05 7:13 ` Ingo Molnar 2015-06-05 9:33 ` [Xen-devel] " David Vrabel 1 sibling, 0 replies; 10+ messages in thread From: Ingo Molnar @ 2015-06-05 7:13 UTC (permalink / raw) To: Rusty Russell Cc: H. Peter Anvin, Andrew Cooper, Xen-devel, David Vrabel, Thomas Gleixner, Ingo Molnar, x86, linux-kernel, Konrad Rzeszutek Wilk, Boris Ostrovsky, lguest, stable * Rusty Russell <rusty@rustcorp.com.au> wrote: > As the comment in arch/x86/include/asm/paravirt_types.h says: > > * Get/set interrupt state. save_fl and restore_fl are only > * expected to use X86_EFLAGS_IF; all other bits > * returned from save_fl are undefined, and may be ignored by > * restore_fl. There should be no 'may' about this: under CONFIG_PARAVIRT_DEBUG=y the reminder of the flags should be cleared (or all bits should be set: that will instantly crash if restored verbatim), so that we trip up generic code that relies on non-IF restoration? Thanks, Ingo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Xen-devel] [PATCH] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-05 2:58 ` Rusty Russell 2015-06-05 7:13 ` Ingo Molnar @ 2015-06-05 9:33 ` David Vrabel 1 sibling, 0 replies; 10+ messages in thread From: David Vrabel @ 2015-06-05 9:33 UTC (permalink / raw) To: Rusty Russell, H. Peter Anvin, Andrew Cooper, Xen-devel Cc: lguest, x86, linux-kernel, stable, Ingo Molnar, David Vrabel, Thomas Gleixner, Boris Ostrovsky On 05/06/15 03:58, Rusty Russell wrote: > > Subject: x86: rename save_fl/restore_fl paravirt ops to highlight eflags. > From: Rusty Russell <rusty@rustcorp.com.au> > > As the comment in arch/x86/include/asm/paravirt_types.h says: > > * Get/set interrupt state. save_fl and restore_fl are only > * expected to use X86_EFLAGS_IF; all other bits > * returned from save_fl are undefined, and may be ignored by > * restore_fl. > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> [...] > --- a/arch/x86/xen/enlighten.c > +++ b/arch/x86/xen/enlighten.c > @@ -1074,8 +1074,8 @@ void xen_setup_vcpu_info_placement(void) > * percpu area for all cpus, so make use of it. Note that for > * PVH we want to use native IRQ mechanism. */ > if (have_vcpu_info_placement && !xen_pvh_domain()) { > - pv_irq_ops.save_fl = __PV_IS_CALLEE_SAVE(xen_save_fl_direct); > - pv_irq_ops.restore_fl = __PV_IS_CALLEE_SAVE(xen_restore_fl_direct); > + pv_irq_ops.save_eflags_if = __PV_IS_CALLEE_SAVE(xen_save_fl_direct); > + pv_irq_ops.restore_eflags_if = __PV_IS_CALLEE_SAVE(xen_restore_fl_direct); > pv_irq_ops.irq_disable = __PV_IS_CALLEE_SAVE(xen_irq_disable_direct); > pv_irq_ops.irq_enable = __PV_IS_CALLEE_SAVE(xen_irq_enable_direct); > pv_mmu_ops.read_cr2 = xen_read_cr2_direct; > @@ -1102,8 +1102,8 @@ static unsigned xen_patch(u8 type, u16 clobbers, void *insnbuf, > switch (type) { > SITE(pv_irq_ops, irq_enable); > SITE(pv_irq_ops, irq_disable); > - SITE(pv_irq_ops, save_fl); > - SITE(pv_irq_ops, restore_fl); > + SITE(pv_irq_ops, save_eflags_if); > + SITE(pv_irq_ops, restore_eflags_if); > #undef SITE > > patch_site: > diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c Acked-by: David Vrabel <david.vrabel@citrix.com> Thanks. David ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-03 9:31 [PATCH] x86/cpu: Fix SMAP check in PVOPS environments Andrew Cooper 2015-06-04 6:38 ` H. Peter Anvin @ 2015-11-17 14:59 ` Andrew Cooper 2015-11-19 10:12 ` [tip:x86/urgent] " tip-bot for Andrew Cooper 2 siblings, 0 replies; 10+ messages in thread From: Andrew Cooper @ 2015-11-17 14:59 UTC (permalink / raw) Cc: Xen-devel, David Vrabel, Rusty Russell, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Konrad Rzeszutek Wilk, Boris Ostrovsky, lguest, stable Ping? None of the discussion on this thread altered the contents of this patch, and the bug is still present. ~Andrew On 03/06/15 10:31, Andrew Cooper wrote: > There appears to be no formal statement of what pv_irq_ops.save_fl() is > supposed to return precisely. Native returns the full flags, while lguest and > Xen only return the Interrupt Flag, and both have comments by the > implementations stating that only the Interrupt Flag is looked at. This may > have been true when initially implemented, but no longer is. > > To make matters worse, the Xen PVOP leaves the upper bits undefined, making > the BUG_ON() undefined behaviour. Experimentally, this now trips for 32bit PV > guests on Broadwell hardware. The BUG_ON() is consistent for an individual > build, but not consistent for all builds. It has also been a sitting timebomb > since SMAP support was introduced. > > Use native_save_fl() instead, which will obtain an accurate view of the AC > flag. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: David Vrabel <david.vrabel@citrix.com> > Tested-by: Rusty Russell <rusty@rustcorp.com.au> > CC: Thomas Gleixner <tglx@linutronix.de> > CC: Ingo Molnar <mingo@redhat.com> > CC: H. Peter Anvin <hpa@zytor.com> > CC: x86@kernel.org > CC: linux-kernel@vger.kernel.org > CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > CC: Boris Ostrovsky <boris.ostrovsky@oracle.com> > CC: xen-devel <xen-devel@lists.xen.org> > CC: lguest@lists.ozlabs.org > CC: stable@vger.kernel.org > --- > arch/x86/kernel/cpu/common.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c > index a62cf04..4f2fded 100644 > --- a/arch/x86/kernel/cpu/common.c > +++ b/arch/x86/kernel/cpu/common.c > @@ -291,10 +291,9 @@ __setup("nosmap", setup_disable_smap); > > static __always_inline void setup_smap(struct cpuinfo_x86 *c) > { > - unsigned long eflags; > + unsigned long eflags = native_save_fl(); > > /* This should have been cleared long ago */ > - raw_local_save_flags(eflags); > BUG_ON(eflags & X86_EFLAGS_AC); > > if (cpu_has(c, X86_FEATURE_SMAP)) { ^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip:x86/urgent] x86/cpu: Fix SMAP check in PVOPS environments 2015-06-03 9:31 [PATCH] x86/cpu: Fix SMAP check in PVOPS environments Andrew Cooper 2015-06-04 6:38 ` H. Peter Anvin 2015-11-17 14:59 ` Andrew Cooper @ 2015-11-19 10:12 ` tip-bot for Andrew Cooper 2 siblings, 0 replies; 10+ messages in thread From: tip-bot for Andrew Cooper @ 2015-11-19 10:12 UTC (permalink / raw) To: linux-tip-commits Cc: rusty, linux-kernel, mingo, david.vrabel, tglx, xen-devel, boris.ostrovsky, hpa, lguest, konrad.wilk, andrew.cooper3 Commit-ID: 581b7f158fe0383b492acd1ce3fb4e99d4e57808 Gitweb: http://git.kernel.org/tip/581b7f158fe0383b492acd1ce3fb4e99d4e57808 Author: Andrew Cooper <andrew.cooper3@citrix.com> AuthorDate: Wed, 3 Jun 2015 10:31:14 +0100 Committer: Thomas Gleixner <tglx@linutronix.de> CommitDate: Thu, 19 Nov 2015 11:07:49 +0100 x86/cpu: Fix SMAP check in PVOPS environments There appears to be no formal statement of what pv_irq_ops.save_fl() is supposed to return precisely. Native returns the full flags, while lguest and Xen only return the Interrupt Flag, and both have comments by the implementations stating that only the Interrupt Flag is looked at. This may have been true when initially implemented, but no longer is. To make matters worse, the Xen PVOP leaves the upper bits undefined, making the BUG_ON() undefined behaviour. Experimentally, this now trips for 32bit PV guests on Broadwell hardware. The BUG_ON() is consistent for an individual build, but not consistent for all builds. It has also been a sitting timebomb since SMAP support was introduced. Use native_save_fl() instead, which will obtain an accurate view of the AC flag. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: David Vrabel <david.vrabel@citrix.com> Tested-by: Rusty Russell <rusty@rustcorp.com.au> Cc: Rusty Russell <rusty@rustcorp.com.au> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> Cc: <lguest@lists.ozlabs.org> Cc: Xen-devel <xen-devel@lists.xen.org> CC: stable@vger.kernel.org Link: http://lkml.kernel.org/r/1433323874-6927-1-git-send-email-andrew.cooper3@citrix.com Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- arch/x86/kernel/cpu/common.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c index 4ddd780..c2b7522 100644 --- a/arch/x86/kernel/cpu/common.c +++ b/arch/x86/kernel/cpu/common.c @@ -273,10 +273,9 @@ __setup("nosmap", setup_disable_smap); static __always_inline void setup_smap(struct cpuinfo_x86 *c) { - unsigned long eflags; + unsigned long eflags = native_save_fl(); /* This should have been cleared long ago */ - raw_local_save_flags(eflags); BUG_ON(eflags & X86_EFLAGS_AC); if (cpu_has(c, X86_FEATURE_SMAP)) { ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-11-19 10:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-06-03 9:31 [PATCH] x86/cpu: Fix SMAP check in PVOPS environments Andrew Cooper 2015-06-04 6:38 ` H. Peter Anvin 2015-06-04 8:58 ` Andrew Cooper 2015-06-04 19:55 ` Rusty Russell 2015-06-04 20:29 ` H. Peter Anvin 2015-06-05 2:58 ` Rusty Russell 2015-06-05 7:13 ` Ingo Molnar 2015-06-05 9:33 ` [Xen-devel] " David Vrabel 2015-11-17 14:59 ` Andrew Cooper 2015-11-19 10:12 ` [tip:x86/urgent] " tip-bot for Andrew Cooper
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).