* Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
@ 2018-08-22 7:19 gregkh
2018-11-28 14:56 ` [Xen-devel] " David Woodhouse
2018-11-28 14:56 ` David Woodhouse
0 siblings, 2 replies; 23+ messages in thread
From: gregkh @ 2018-08-22 7:19 UTC (permalink / raw)
To: b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.luto,
boris.ostrovsky, bp, brgerst, dave.hansen, dvlasenk, gregkh, hpa,
jgross, jpoimboe, linux
Cc: stable-commits
This is a note to let you know that I've just added the patch titled
x86/entry/64: Remove %ebx handling from error_entry/exit
to the 4.9-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
The filename of the patch is:
x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch
and it can be found in the queue-4.9 subdirectory.
If you, or anyone else, feels it should not be added to the stable tree,
please let <stable@vger.kernel.org> know about it.
From b3681dd548d06deb2e1573890829dff4b15abf46 Mon Sep 17 00:00:00 2001
From: Andy Lutomirski <luto@kernel.org>
Date: Sun, 22 Jul 2018 11:05:09 -0700
Subject: x86/entry/64: Remove %ebx handling from error_entry/exit
From: Andy Lutomirski <luto@kernel.org>
commit b3681dd548d06deb2e1573890829dff4b15abf46 upstream.
error_entry and error_exit communicate the user vs. kernel status of
the frame using %ebx. This is unnecessary -- the information is in
regs->cs. Just use regs->cs.
This makes error_entry simpler and makes error_exit more robust.
It also fixes a nasty bug. Before all the Spectre nonsense, the
xen_failsafe_callback entry point returned like this:
ALLOC_PT_GPREGS_ON_STACK
SAVE_C_REGS
SAVE_EXTRA_REGS
ENCODE_FRAME_POINTER
jmp error_exit
And it did not go through error_entry. This was bogus: RBX
contained garbage, and error_exit expected a flag in RBX.
Fortunately, it generally contained *nonzero* garbage, so the
correct code path was used. As part of the Spectre fixes, code was
added to clear RBX to mitigate certain speculation attacks. Now,
depending on kernel configuration, RBX got zeroed and, when running
some Wine workloads, the kernel crashes. This was introduced by:
commit 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface")
With this patch applied, RBX is no longer needed as a flag, and the
problem goes away.
I suspect that malicious userspace could use this bug to crash the
kernel even without the offending patch applied, though.
[ Historical note: I wrote this patch as a cleanup before I was aware
of the bug it fixed. ]
[ Note to stable maintainers: this should probably get applied to all
kernels. If you're nervous about that, a more conservative fix to
add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
also fix the problem. ]
Reported-and-tested-by: M. Vefa Bicakci <m.v.b@runbox.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Dominik Brodowski <linux@dominikbrodowski.net>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
Fixes: 3ac6d8c787b8 ("x86/entry/64: Clear registers for exceptions/interrupts, to reduce speculation attack surface")
Link: http://lkml.kernel.org/r/b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sarah Newman <srn@prgmr.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
arch/x86/entry/entry_64.S | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -774,7 +774,7 @@ ENTRY(\sym)
call \do_sym
- jmp error_exit /* %ebx: no swapgs flag */
+ jmp error_exit
.endif
END(\sym)
.endm
@@ -1043,7 +1043,6 @@ END(paranoid_exit)
/*
* Save all registers in pt_regs, and switch gs if needed.
- * Return: EBX=0: came from user mode; EBX=1: otherwise
*/
ENTRY(error_entry)
cld
@@ -1056,7 +1055,6 @@ ENTRY(error_entry)
* the kernel CR3 here.
*/
SWITCH_KERNEL_CR3
- xorl %ebx, %ebx
testb $3, CS+8(%rsp)
jz .Lerror_kernelspace
@@ -1087,7 +1085,6 @@ ENTRY(error_entry)
* for these here too.
*/
.Lerror_kernelspace:
- incl %ebx
leaq native_irq_return_iret(%rip), %rcx
cmpq %rcx, RIP+8(%rsp)
je .Lerror_bad_iret
@@ -1119,28 +1116,19 @@ ENTRY(error_entry)
/*
* Pretend that the exception came from user mode: set up pt_regs
- * as if we faulted immediately after IRET and clear EBX so that
- * error_exit knows that we will be returning to user mode.
+ * as if we faulted immediately after IRET.
*/
mov %rsp, %rdi
call fixup_bad_iret
mov %rax, %rsp
- decl %ebx
jmp .Lerror_entry_from_usermode_after_swapgs
END(error_entry)
-
-/*
- * On entry, EBX is a "return to kernel mode" flag:
- * 1: already in kernel mode, don't need SWAPGS
- * 0: user gsbase is loaded, we need SWAPGS and standard preparation for return to usermode
- */
ENTRY(error_exit)
- movl %ebx, %eax
DISABLE_INTERRUPTS(CLBR_NONE)
TRACE_IRQS_OFF
- testl %eax, %eax
- jnz retint_kernel
+ testb $3, CS(%rsp)
+ jz retint_kernel
jmp retint_user
END(error_exit)
Patches currently in stable-queue which might be from luto@kernel.org are
queue-4.9/x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-08-22 7:19 Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree gregkh
@ 2018-11-28 14:56 ` David Woodhouse
2018-11-28 15:43 ` Sasha Levin
` (3 more replies)
2018-11-28 14:56 ` David Woodhouse
1 sibling, 4 replies; 23+ messages in thread
From: David Woodhouse @ 2018-11-28 14:56 UTC (permalink / raw)
To: gregkh,
b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.luto,
boris.ostrovsky, bp, brgerst, dave.hansen, dvlasenk, hpa, jgross,
jpoimboe, linux, luto, m.v.b, mingo, peterz, srn, tglx, torvalds,
xen-devel
Cc: stable
[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]
On Wed, 2018-08-22 at 09:19 +0200, gregkh@linuxfoundation.org wrote:
> This is a note to let you know that I've just added the patch titled
>
> x86/entry/64: Remove %ebx handling from error_entry/exit
>
> to the 4.9-stable tree which can be found at:
> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
> x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch
> and it can be found in the queue-4.9 subdirectory.
Can we have it for 4.4 too, please?
> [ Note to stable maintainers: this should probably get applied to all
> kernels. If you're nervous about that, a more conservative fix to
> add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
> also fix the problem. ]
Can we assume it's always from kernel? The Xen code definitely seems to
handle invoking this from both kernel and userspace contexts.
Shouldn't %ebx get set to !(regs->rsp & 3) ?
Either way, let's just do it in the stable tree exactly the same way
it's done upstream.
> - * On entry, EBX is a "return to kernel mode" flag:
Re-introduce the typo 'EBS' here, to make the patch apply cleanly to
4.4. It's only removing that line anyway.
Or just cherry-pick upstream commit 75ca5b22260ef7 first.
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-08-22 7:19 Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree gregkh
2018-11-28 14:56 ` [Xen-devel] " David Woodhouse
@ 2018-11-28 14:56 ` David Woodhouse
1 sibling, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2018-11-28 14:56 UTC (permalink / raw)
To: gregkh,
b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.luto,
boris.ostrovsky, bp, brgerst, dave.hansen, dvlasenk, hpa, jgross,
jpoimboe, linux
Cc: stable
[-- Attachment #1.1: Type: text/plain, Size: 1310 bytes --]
On Wed, 2018-08-22 at 09:19 +0200, gregkh@linuxfoundation.org wrote:
> This is a note to let you know that I've just added the patch titled
>
> x86/entry/64: Remove %ebx handling from error_entry/exit
>
> to the 4.9-stable tree which can be found at:
> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>
> The filename of the patch is:
> x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch
> and it can be found in the queue-4.9 subdirectory.
Can we have it for 4.4 too, please?
> [ Note to stable maintainers: this should probably get applied to all
> kernels. If you're nervous about that, a more conservative fix to
> add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
> also fix the problem. ]
Can we assume it's always from kernel? The Xen code definitely seems to
handle invoking this from both kernel and userspace contexts.
Shouldn't %ebx get set to !(regs->rsp & 3) ?
Either way, let's just do it in the stable tree exactly the same way
it's done upstream.
> - * On entry, EBX is a "return to kernel mode" flag:
Re-introduce the typo 'EBS' here, to make the patch apply cleanly to
4.4. It's only removing that line anyway.
Or just cherry-pick upstream commit 75ca5b22260ef7 first.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-11-28 14:56 ` [Xen-devel] " David Woodhouse
@ 2018-11-28 15:43 ` Sasha Levin
2018-11-28 15:43 ` Sasha Levin
` (2 subsequent siblings)
3 siblings, 0 replies; 23+ messages in thread
From: Sasha Levin @ 2018-11-28 15:43 UTC (permalink / raw)
To: David Woodhouse
Cc: gregkh, boris.ostrovsky, bp, brgerst, dave.hansen, dvlasenk, hpa,
jgross, jpoimboe, linux, luto, m.v.b, mingo, peterz, srn, tglx,
torvalds, xen-devel, stable
On Wed, Nov 28, 2018 at 02:56:32PM +0000, David Woodhouse wrote:
>On Wed, 2018-08-22 at 09:19 +0200, gregkh@linuxfoundation.org wrote:
>> This is a note to let you know that I've just added the patch titled
>>
>> x86/entry/64: Remove %ebx handling from error_entry/exit
>>
>> to the 4.9-stable tree which can be found at:
>> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>> x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch
>> and it can be found in the queue-4.9 subdirectory.
>
>Can we have it for 4.4 too, please?
>
>> [ Note to stable maintainers: this should probably get applied to all
>> kernels. If you're nervous about that, a more conservative fix to
>> add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>> also fix the problem. ]
>
>Can we assume it's always from kernel? The Xen code definitely seems to
>handle invoking this from both kernel and userspace contexts.
>
>Shouldn't %ebx get set to !(regs->rsp & 3) ?
>
>Either way, let's just do it in the stable tree exactly the same way
>it's done upstream.
>
>> - * On entry, EBX is a "return to kernel mode" flag:
>
>Re-introduce the typo 'EBS' here, to make the patch apply cleanly to
>4.4. It's only removing that line anyway.
>
>Or just cherry-pick upstream commit 75ca5b22260ef7 first.
Queued for 4.4. I've just grabbed the extra spellcheck fix as well.
--
Thanks,
Sasha
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-11-28 14:56 ` [Xen-devel] " David Woodhouse
2018-11-28 15:43 ` Sasha Levin
@ 2018-11-28 15:43 ` Sasha Levin
2018-11-28 16:44 ` Andy Lutomirski
2018-11-28 16:44 ` [Xen-devel] " Andy Lutomirski
3 siblings, 0 replies; 23+ messages in thread
From: Sasha Levin @ 2018-11-28 15:43 UTC (permalink / raw)
To: David Woodhouse
Cc: jgross, tglx, dvlasenk, jpoimboe, peterz, gregkh, dave.hansen,
m.v.b, linux, srn, torvalds, bp, stable, luto, brgerst,
xen-devel, boris.ostrovsky, hpa, mingo
On Wed, Nov 28, 2018 at 02:56:32PM +0000, David Woodhouse wrote:
>On Wed, 2018-08-22 at 09:19 +0200, gregkh@linuxfoundation.org wrote:
>> This is a note to let you know that I've just added the patch titled
>>
>> x86/entry/64: Remove %ebx handling from error_entry/exit
>>
>> to the 4.9-stable tree which can be found at:
>> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>> x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch
>> and it can be found in the queue-4.9 subdirectory.
>
>Can we have it for 4.4 too, please?
>
>> [ Note to stable maintainers: this should probably get applied to all
>> kernels. If you're nervous about that, a more conservative fix to
>> add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>> also fix the problem. ]
>
>Can we assume it's always from kernel? The Xen code definitely seems to
>handle invoking this from both kernel and userspace contexts.
>
>Shouldn't %ebx get set to !(regs->rsp & 3) ?
>
>Either way, let's just do it in the stable tree exactly the same way
>it's done upstream.
>
>> - * On entry, EBX is a "return to kernel mode" flag:
>
>Re-introduce the typo 'EBS' here, to make the patch apply cleanly to
>4.4. It's only removing that line anyway.
>
>Or just cherry-pick upstream commit 75ca5b22260ef7 first.
Queued for 4.4. I've just grabbed the extra spellcheck fix as well.
--
Thanks,
Sasha
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-11-28 14:56 ` [Xen-devel] " David Woodhouse
` (2 preceding siblings ...)
2018-11-28 16:44 ` Andy Lutomirski
@ 2018-11-28 16:44 ` Andy Lutomirski
2018-12-06 17:10 ` David Woodhouse
2018-12-06 17:10 ` Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree David Woodhouse
3 siblings, 2 replies; 23+ messages in thread
From: Andy Lutomirski @ 2018-11-28 16:44 UTC (permalink / raw)
To: David Woodhouse
Cc: gregkh,
b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.luto,
boris.ostrovsky, bp, brgerst, dave.hansen, dvlasenk, hpa, jgross,
jpoimboe, linux, luto, m.v.b, mingo, peterz, srn, tglx, torvalds,
xen-devel, stable
> On Nov 28, 2018, at 6:56 AM, David Woodhouse <dwmw2@infradead.org> wrote:
>
>> On Wed, 2018-08-22 at 09:19 +0200, gregkh@linuxfoundation.org wrote:
>> This is a note to let you know that I've just added the patch titled
>>
>> x86/entry/64: Remove %ebx handling from error_entry/exit
>>
>> to the 4.9-stable tree which can be found at:
>> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>> x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch
>> and it can be found in the queue-4.9 subdirectory.
>
> Can we have it for 4.4 too, please?
>
>> [ Note to stable maintainers: this should probably get applied to all
>> kernels. If you're nervous about that, a more conservative fix to
>> add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>> also fix the problem. ]
>
> Can we assume it's always from kernel? The Xen code definitely seems to
> handle invoking this from both kernel and userspace contexts.
I learned that my comment here was wrong shortly after the patch landed :(
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-11-28 14:56 ` [Xen-devel] " David Woodhouse
2018-11-28 15:43 ` Sasha Levin
2018-11-28 15:43 ` Sasha Levin
@ 2018-11-28 16:44 ` Andy Lutomirski
2018-11-28 16:44 ` [Xen-devel] " Andy Lutomirski
3 siblings, 0 replies; 23+ messages in thread
From: Andy Lutomirski @ 2018-11-28 16:44 UTC (permalink / raw)
To: David Woodhouse
Cc: jgross, tglx, dvlasenk,
b5010a090d3586b2d6e06c7ad3ec5542d1241c45.1532282627.git.luto,
jpoimboe, peterz, gregkh, dave.hansen, m.v.b, linux, srn,
torvalds, bp, stable
> On Nov 28, 2018, at 6:56 AM, David Woodhouse <dwmw2@infradead.org> wrote:
>
>> On Wed, 2018-08-22 at 09:19 +0200, gregkh@linuxfoundation.org wrote:
>> This is a note to let you know that I've just added the patch titled
>>
>> x86/entry/64: Remove %ebx handling from error_entry/exit
>>
>> to the 4.9-stable tree which can be found at:
>> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>
>> The filename of the patch is:
>> x86-entry-64-remove-ebx-handling-from-error_entry-exit.patch
>> and it can be found in the queue-4.9 subdirectory.
>
> Can we have it for 4.4 too, please?
>
>> [ Note to stable maintainers: this should probably get applied to all
>> kernels. If you're nervous about that, a more conservative fix to
>> add xorl %ebx,%ebx; incl %ebx before the jump to error_exit should
>> also fix the problem. ]
>
> Can we assume it's always from kernel? The Xen code definitely seems to
> handle invoking this from both kernel and userspace contexts.
I learned that my comment here was wrong shortly after the patch landed :(
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-11-28 16:44 ` [Xen-devel] " Andy Lutomirski
@ 2018-12-06 17:10 ` David Woodhouse
2018-12-06 17:36 ` Andrew Cooper
2018-12-06 17:36 ` [Xen-devel] " Andrew Cooper
2018-12-06 17:10 ` Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree David Woodhouse
1 sibling, 2 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-06 17:10 UTC (permalink / raw)
To: Andy Lutomirski
Cc: gregkh, boris.ostrovsky, bp, brgerst, dave.hansen, dvlasenk, hpa,
jgross, jpoimboe, linux, luto, m.v.b, mingo, peterz, srn, tglx,
torvalds, xen-devel, stable
[-- Attachment #1: Type: text/plain, Size: 2227 bytes --]
On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote:
> > Can we assume it's always from kernel? The Xen code definitely seems to
> > handle invoking this from both kernel and userspace contexts.
>
> I learned that my comment here was wrong shortly after the patch landed :(
Turns out the only place I see it getting called from is under
__context_switch().
#7 [ffff8801144a7cf0] new_xen_failsafe_callback at ffffffffa028028a [kmod_ebxfix]
#8 [ffff8801144a7d90] xen_hypercall_update_descriptor at ffffffff8100114a
#9 [ffff8801144a7db8] xen_hypercall_update_descriptor at ffffffff8100114a
#10 [ffff8801144a7df0] xen_mc_flush at ffffffff81006ab9
#11 [ffff8801144a7e30] xen_end_context_switch at ffffffff81004e12
#12 [ffff8801144a7e48] __switch_to at ffffffff81016582
#13 [ffff8801144a7ea0] __schedule at ffffffff815d2b37
That …114a in xen_hypercall_update_descriptor is the 'pop' instruction
right after the syscall; it's happening when Xen is preempting the
domain in the hypercall and then reloads the segment registers to run
that vCPU again later.
[ 44185.225289] WARN: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000abbd76060
The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0
respectively — it was setting a descriptor at that address, to zero.
Xen then failed to load the selector 0x63 into the %gs register (since
that descriptor has just been wiped?), leaving it zero.
[ 44185.225256] WARN: xen_failsafe_callback from xen_hypercall_update_descriptor+0xa/0x40
[ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63
This is on context switch from a 32-bit task to idle. So
xen_failsafe_callback is returning to the "faulting" instruction, with
a comment saying "Retry the IRET", but in fact is just continuing on
its merry way with %gs unexpectedly set to zero.
In fact I think this is probably fine in practice, since it's about to
get explicitly set a few lines further down in __context_switch(). But
it's odd enough, and far enough away from what's actually said by the
comments, that I'm utterly unsure.
In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the
32-bit kernel. Is that really right?
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-11-28 16:44 ` [Xen-devel] " Andy Lutomirski
2018-12-06 17:10 ` David Woodhouse
@ 2018-12-06 17:10 ` David Woodhouse
1 sibling, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-06 17:10 UTC (permalink / raw)
To: Andy Lutomirski
Cc: jgross, tglx, dvlasenk, jpoimboe, peterz, gregkh, dave.hansen,
m.v.b, linux, srn, torvalds, bp, stable, luto, brgerst,
xen-devel, boris.ostrovsky, hpa, mingo
[-- Attachment #1.1: Type: text/plain, Size: 2227 bytes --]
On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote:
> > Can we assume it's always from kernel? The Xen code definitely seems to
> > handle invoking this from both kernel and userspace contexts.
>
> I learned that my comment here was wrong shortly after the patch landed :(
Turns out the only place I see it getting called from is under
__context_switch().
#7 [ffff8801144a7cf0] new_xen_failsafe_callback at ffffffffa028028a [kmod_ebxfix]
#8 [ffff8801144a7d90] xen_hypercall_update_descriptor at ffffffff8100114a
#9 [ffff8801144a7db8] xen_hypercall_update_descriptor at ffffffff8100114a
#10 [ffff8801144a7df0] xen_mc_flush at ffffffff81006ab9
#11 [ffff8801144a7e30] xen_end_context_switch at ffffffff81004e12
#12 [ffff8801144a7e48] __switch_to at ffffffff81016582
#13 [ffff8801144a7ea0] __schedule at ffffffff815d2b37
That …114a in xen_hypercall_update_descriptor is the 'pop' instruction
right after the syscall; it's happening when Xen is preempting the
domain in the hypercall and then reloads the segment registers to run
that vCPU again later.
[ 44185.225289] WARN: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000abbd76060
The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0
respectively — it was setting a descriptor at that address, to zero.
Xen then failed to load the selector 0x63 into the %gs register (since
that descriptor has just been wiped?), leaving it zero.
[ 44185.225256] WARN: xen_failsafe_callback from xen_hypercall_update_descriptor+0xa/0x40
[ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63
This is on context switch from a 32-bit task to idle. So
xen_failsafe_callback is returning to the "faulting" instruction, with
a comment saying "Retry the IRET", but in fact is just continuing on
its merry way with %gs unexpectedly set to zero.
In fact I think this is probably fine in practice, since it's about to
get explicitly set a few lines further down in __context_switch(). But
it's odd enough, and far enough away from what's actually said by the
comments, that I'm utterly unsure.
In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the
32-bit kernel. Is that really right?
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-06 17:10 ` David Woodhouse
2018-12-06 17:36 ` Andrew Cooper
@ 2018-12-06 17:36 ` Andrew Cooper
2018-12-06 18:49 ` Andy Lutomirski
2018-12-06 18:49 ` [Xen-devel] " Andy Lutomirski
1 sibling, 2 replies; 23+ messages in thread
From: Andrew Cooper @ 2018-12-06 17:36 UTC (permalink / raw)
To: David Woodhouse, Andy Lutomirski
Cc: jgross, tglx, dvlasenk, jpoimboe, peterz, gregkh, dave.hansen,
m.v.b, linux, srn, torvalds, bp, stable, luto, brgerst,
xen-devel, boris.ostrovsky, hpa, mingo
On 06/12/2018 17:10, David Woodhouse wrote:
> On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote:
>>> Can we assume it's always from kernel? The Xen code definitely seems to
>>> handle invoking this from both kernel and userspace contexts.
>> I learned that my comment here was wrong shortly after the patch landed :(
> Turns out the only place I see it getting called from is under
> __context_switch().
>
> #7 [ffff8801144a7cf0] new_xen_failsafe_callback at ffffffffa028028a [kmod_ebxfix]
> #8 [ffff8801144a7d90] xen_hypercall_update_descriptor at ffffffff8100114a
> #9 [ffff8801144a7db8] xen_hypercall_update_descriptor at ffffffff8100114a
> #10 [ffff8801144a7df0] xen_mc_flush at ffffffff81006ab9
> #11 [ffff8801144a7e30] xen_end_context_switch at ffffffff81004e12
> #12 [ffff8801144a7e48] __switch_to at ffffffff81016582
> #13 [ffff8801144a7ea0] __schedule at ffffffff815d2b37
>
> That …114a in xen_hypercall_update_descriptor is the 'pop' instruction
> right after the syscall; it's happening when Xen is preempting the
> domain in the hypercall and then reloads the segment registers to run
> that vCPU again later.
>
> [ 44185.225289] WARN: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000abbd76060
>
> The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0
> respectively — it was setting a descriptor at that address, to zero.
>
> Xen then failed to load the selector 0x63 into the %gs register (since
> that descriptor has just been wiped?), leaving it zero.
>
> [ 44185.225256] WARN: xen_failsafe_callback from xen_hypercall_update_descriptor+0xa/0x40
> [ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63
>
> This is on context switch from a 32-bit task to idle. So
> xen_failsafe_callback is returning to the "faulting" instruction, with
> a comment saying "Retry the IRET", but in fact is just continuing on
> its merry way with %gs unexpectedly set to zero.
>
> In fact I think this is probably fine in practice, since it's about to
> get explicitly set a few lines further down in __context_switch(). But
> it's odd enough, and far enough away from what's actually said by the
> comments, that I'm utterly unsure.
>
> In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the
> 32-bit kernel. Is that really right?
Basically - what is happening is that xen_load_tls() is invalidating the
%gs selector while %gs is still non-NUL.
If this happens to intersect with a vcpu reschedule, %gs (being non-NUL)
takes precedence over KERNGSBASE, and faults when Xen tries to reload
it. This results in the failsafe callback being invoked.
I think the correct course of action is to use xen_load_gs_index(0)
(poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs)
before using update_descriptor() to invalidate the segment.
That will reset %gs to 0 without touching KERNGSBASE, and can be queued
in the same multicall as the update_descriptor() hypercall.
~Andrew
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-06 17:10 ` David Woodhouse
@ 2018-12-06 17:36 ` Andrew Cooper
2018-12-06 17:36 ` [Xen-devel] " Andrew Cooper
1 sibling, 0 replies; 23+ messages in thread
From: Andrew Cooper @ 2018-12-06 17:36 UTC (permalink / raw)
To: David Woodhouse, Andy Lutomirski
Cc: jgross, dvlasenk, srn, peterz, gregkh, dave.hansen, m.v.b,
brgerst, linux, mingo, bp, stable, luto, jpoimboe, xen-devel,
tglx, torvalds, boris.ostrovsky, hpa
On 06/12/2018 17:10, David Woodhouse wrote:
> On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote:
>>> Can we assume it's always from kernel? The Xen code definitely seems to
>>> handle invoking this from both kernel and userspace contexts.
>> I learned that my comment here was wrong shortly after the patch landed :(
> Turns out the only place I see it getting called from is under
> __context_switch().
>
> #7 [ffff8801144a7cf0] new_xen_failsafe_callback at ffffffffa028028a [kmod_ebxfix]
> #8 [ffff8801144a7d90] xen_hypercall_update_descriptor at ffffffff8100114a
> #9 [ffff8801144a7db8] xen_hypercall_update_descriptor at ffffffff8100114a
> #10 [ffff8801144a7df0] xen_mc_flush at ffffffff81006ab9
> #11 [ffff8801144a7e30] xen_end_context_switch at ffffffff81004e12
> #12 [ffff8801144a7e48] __switch_to at ffffffff81016582
> #13 [ffff8801144a7ea0] __schedule at ffffffff815d2b37
>
> That …114a in xen_hypercall_update_descriptor is the 'pop' instruction
> right after the syscall; it's happening when Xen is preempting the
> domain in the hypercall and then reloads the segment registers to run
> that vCPU again later.
>
> [ 44185.225289] WARN: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000abbd76060
>
> The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0
> respectively — it was setting a descriptor at that address, to zero.
>
> Xen then failed to load the selector 0x63 into the %gs register (since
> that descriptor has just been wiped?), leaving it zero.
>
> [ 44185.225256] WARN: xen_failsafe_callback from xen_hypercall_update_descriptor+0xa/0x40
> [ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63
>
> This is on context switch from a 32-bit task to idle. So
> xen_failsafe_callback is returning to the "faulting" instruction, with
> a comment saying "Retry the IRET", but in fact is just continuing on
> its merry way with %gs unexpectedly set to zero.
>
> In fact I think this is probably fine in practice, since it's about to
> get explicitly set a few lines further down in __context_switch(). But
> it's odd enough, and far enough away from what's actually said by the
> comments, that I'm utterly unsure.
>
> In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the
> 32-bit kernel. Is that really right?
Basically - what is happening is that xen_load_tls() is invalidating the
%gs selector while %gs is still non-NUL.
If this happens to intersect with a vcpu reschedule, %gs (being non-NUL)
takes precedence over KERNGSBASE, and faults when Xen tries to reload
it. This results in the failsafe callback being invoked.
I think the correct course of action is to use xen_load_gs_index(0)
(poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs)
before using update_descriptor() to invalidate the segment.
That will reset %gs to 0 without touching KERNGSBASE, and can be queued
in the same multicall as the update_descriptor() hypercall.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-06 17:36 ` [Xen-devel] " Andrew Cooper
2018-12-06 18:49 ` Andy Lutomirski
@ 2018-12-06 18:49 ` Andy Lutomirski
2018-12-06 20:27 ` David Woodhouse
2018-12-06 20:27 ` [Xen-devel] " David Woodhouse
1 sibling, 2 replies; 23+ messages in thread
From: Andy Lutomirski @ 2018-12-06 18:49 UTC (permalink / raw)
To: Andrew Cooper
Cc: David Woodhouse, Juergen Gross, Thomas Gleixner, Denys Vlasenko,
Josh Poimboeuf, Peter Zijlstra, Greg KH, Dave Hansen,
M. Vefa Bicakci, Dominik Brodowski, Sarah Newman, Linus Torvalds,
Borislav Petkov, stable, Andrew Lutomirski, Brian Gerst,
xen-devel, Boris Ostrovsky, H. Peter Anvin, Ingo Molnar
> On Dec 6, 2018, at 9:36 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
>> On 06/12/2018 17:10, David Woodhouse wrote:
>> On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote:
>>>> Can we assume it's always from kernel? The Xen code definitely seems to
>>>> handle invoking this from both kernel and userspace contexts.
>>> I learned that my comment here was wrong shortly after the patch landed :(
>> Turns out the only place I see it getting called from is under
>> __context_switch().
>>
>> #7 [ffff8801144a7cf0] new_xen_failsafe_callback at ffffffffa028028a [kmod_ebxfix]
>> #8 [ffff8801144a7d90] xen_hypercall_update_descriptor at ffffffff8100114a
>> #9 [ffff8801144a7db8] xen_hypercall_update_descriptor at ffffffff8100114a
>> #10 [ffff8801144a7df0] xen_mc_flush at ffffffff81006ab9
>> #11 [ffff8801144a7e30] xen_end_context_switch at ffffffff81004e12
>> #12 [ffff8801144a7e48] __switch_to at ffffffff81016582
>> #13 [ffff8801144a7ea0] __schedule at ffffffff815d2b37
>>
>> That …114a in xen_hypercall_update_descriptor is the 'pop' instruction
>> right after the syscall; it's happening when Xen is preempting the
>> domain in the hypercall and then reloads the segment registers to run
>> that vCPU again later.
>>
>> [ 44185.225289] WARN: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000abbd76060
>>
>> The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0
>> respectively — it was setting a descriptor at that address, to zero.
>>
>> Xen then failed to load the selector 0x63 into the %gs register (since
>> that descriptor has just been wiped?), leaving it zero.
>>
>> [ 44185.225256] WARN: xen_failsafe_callback from xen_hypercall_update_descriptor+0xa/0x40
>> [ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63
>>
>> This is on context switch from a 32-bit task to idle. So
>> xen_failsafe_callback is returning to the "faulting" instruction, with
>> a comment saying "Retry the IRET", but in fact is just continuing on
>> its merry way with %gs unexpectedly set to zero.
>>
>> In fact I think this is probably fine in practice, since it's about to
>> get explicitly set a few lines further down in __context_switch(). But
>> it's odd enough, and far enough away from what's actually said by the
>> comments, that I'm utterly unsure.
>>
>> In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the
>> 32-bit kernel. Is that really right?
>
> Basically - what is happening is that xen_load_tls() is invalidating the
> %gs selector while %gs is still non-NUL.
>
> If this happens to intersect with a vcpu reschedule, %gs (being non-NUL)
> takes precedence over KERNGSBASE, and faults when Xen tries to reload
> it. This results in the failsafe callback being invoked.
>
> I think the correct course of action is to use xen_load_gs_index(0)
> (poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs)
> before using update_descriptor() to invalidate the segment.
>
> That will reset %gs to 0 without touching KERNGSBASE, and can be queued
> in the same multicall as the update_descriptor() hypercall.
Sounds good to me as long as we skip it on native.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-06 17:36 ` [Xen-devel] " Andrew Cooper
@ 2018-12-06 18:49 ` Andy Lutomirski
2018-12-06 18:49 ` [Xen-devel] " Andy Lutomirski
1 sibling, 0 replies; 23+ messages in thread
From: Andy Lutomirski @ 2018-12-06 18:49 UTC (permalink / raw)
To: Andrew Cooper
Cc: Juergen Gross, Denys Vlasenko, Sarah Newman, Peter Zijlstra,
Greg KH, Dave Hansen, M. Vefa Bicakci, Brian Gerst,
Dominik Brodowski, Ingo Molnar, Linus Torvalds, Borislav Petkov,
stable, Andrew Lutomirski, Josh Poimboeuf, xen-devel,
Thomas Gleixner, David Woodhouse, Boris Ostrovsky,
H. Peter Anvin
> On Dec 6, 2018, at 9:36 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
>> On 06/12/2018 17:10, David Woodhouse wrote:
>> On Wed, 2018-11-28 at 08:44 -0800, Andy Lutomirski wrote:
>>>> Can we assume it's always from kernel? The Xen code definitely seems to
>>>> handle invoking this from both kernel and userspace contexts.
>>> I learned that my comment here was wrong shortly after the patch landed :(
>> Turns out the only place I see it getting called from is under
>> __context_switch().
>>
>> #7 [ffff8801144a7cf0] new_xen_failsafe_callback at ffffffffa028028a [kmod_ebxfix]
>> #8 [ffff8801144a7d90] xen_hypercall_update_descriptor at ffffffff8100114a
>> #9 [ffff8801144a7db8] xen_hypercall_update_descriptor at ffffffff8100114a
>> #10 [ffff8801144a7df0] xen_mc_flush at ffffffff81006ab9
>> #11 [ffff8801144a7e30] xen_end_context_switch at ffffffff81004e12
>> #12 [ffff8801144a7e48] __switch_to at ffffffff81016582
>> #13 [ffff8801144a7ea0] __schedule at ffffffff815d2b37
>>
>> That …114a in xen_hypercall_update_descriptor is the 'pop' instruction
>> right after the syscall; it's happening when Xen is preempting the
>> domain in the hypercall and then reloads the segment registers to run
>> that vCPU again later.
>>
>> [ 44185.225289] WARN: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000abbd76060
>>
>> The update_descriptor hypercall args (rdi, rsi) were 0xabbd76060 and 0
>> respectively — it was setting a descriptor at that address, to zero.
>>
>> Xen then failed to load the selector 0x63 into the %gs register (since
>> that descriptor has just been wiped?), leaving it zero.
>>
>> [ 44185.225256] WARN: xen_failsafe_callback from xen_hypercall_update_descriptor+0xa/0x40
>> [ 44185.225263] WARN: DS: 2b/2b ES: 2b/2b FS: 0/0 GS:0/63
>>
>> This is on context switch from a 32-bit task to idle. So
>> xen_failsafe_callback is returning to the "faulting" instruction, with
>> a comment saying "Retry the IRET", but in fact is just continuing on
>> its merry way with %gs unexpectedly set to zero.
>>
>> In fact I think this is probably fine in practice, since it's about to
>> get explicitly set a few lines further down in __context_switch(). But
>> it's odd enough, and far enough away from what's actually said by the
>> comments, that I'm utterly unsure.
>>
>> In xen_load_tls() we explicitly only do the lazy_load_gs(0) for the
>> 32-bit kernel. Is that really right?
>
> Basically - what is happening is that xen_load_tls() is invalidating the
> %gs selector while %gs is still non-NUL.
>
> If this happens to intersect with a vcpu reschedule, %gs (being non-NUL)
> takes precedence over KERNGSBASE, and faults when Xen tries to reload
> it. This results in the failsafe callback being invoked.
>
> I think the correct course of action is to use xen_load_gs_index(0)
> (poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs)
> before using update_descriptor() to invalidate the segment.
>
> That will reset %gs to 0 without touching KERNGSBASE, and can be queued
> in the same multicall as the update_descriptor() hypercall.
Sounds good to me as long as we skip it on native.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-06 18:49 ` [Xen-devel] " Andy Lutomirski
2018-12-06 20:27 ` David Woodhouse
@ 2018-12-06 20:27 ` David Woodhouse
2018-12-07 12:18 ` David Woodhouse
2018-12-07 12:18 ` [Xen-devel] " David Woodhouse
1 sibling, 2 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-06 20:27 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper
Cc: Juergen Gross, Thomas Gleixner, Denys Vlasenko, Josh Poimboeuf,
Peter Zijlstra, Greg KH, Dave Hansen, M. Vefa Bicakci,
Dominik Brodowski, Sarah Newman, Linus Torvalds, Borislav Petkov,
stable, Brian Gerst, xen-devel, Boris Ostrovsky, H. Peter Anvin,
Ingo Molnar
[-- Attachment #1: Type: text/plain, Size: 3611 bytes --]
On Thu, 2018-12-06 at 10:49 -0800, Andy Lutomirski wrote:
> > On Dec 6, 2018, at 9:36 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > Basically - what is happening is that xen_load_tls() is invalidating the
> > %gs selector while %gs is still non-NUL.
> >
> > If this happens to intersect with a vcpu reschedule, %gs (being non-NUL)
> > takes precedence over KERNGSBASE, and faults when Xen tries to reload
> > it. This results in the failsafe callback being invoked.
> >
> > I think the correct course of action is to use xen_load_gs_index(0)
> > (poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs)
> > before using update_descriptor() to invalidate the segment.
> >
> > That will reset %gs to 0 without touching KERNGSBASE, and can be queued
> > in the same multicall as the update_descriptor() hypercall.
>
> Sounds good to me as long as we skip it on native.
Like this? The other option is just to declare that we don't care. On
the rare occasion that it does happen to preempt and then take the trap
on reloading, xen_failsafe_callback is actually doing the right thing
and just leaving %gs as zero. We'd just need to fix the comments so
they explicitly note this case is handled there too. At the moment it
just says 'Retry the IRET', as I noted before.
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index ef05bea7010d..e8b383b24246 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -520,4 +520,15 @@ MULTI_stack_switch(struct multicall_entry *mcl,
trace_xen_mc_entry(mcl, 2);
}
+static inline void
+MULTI_set_segment_base(struct multicall_entry *mcl,
+ int reg, unsigned long value)
+{
+ mcl->op = __HYPERVISOR_set_segment_base;
+ mcl->args[0] = reg;
+ mcl->args[1] = value;
+
+ trace_xen_mc_entry(mcl, 2);
+}
+
#endif /* _ASM_X86_XEN_HYPERCALL_H */
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 2f6787fc7106..722f1f51e20c 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -527,6 +527,8 @@ static void load_TLS_descriptor(struct thread_struct *t,
static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
{
+ xen_mc_batch();
+
/*
* XXX sleazy hack: If we're being called in a lazy-cpu zone
* and lazy gs handling is enabled, it means we're in a
@@ -537,24 +539,24 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
* This will go away as soon as Xen has been modified to not
* save/restore %gs for normal hypercalls.
*
- * On x86_64, this hack is not used for %gs, because gs points
- * to KERNEL_GS_BASE (and uses it for PDA references), so we
- * must not zero %gs on x86_64
- *
* For x86_64, we need to zero %fs, otherwise we may get an
* exception between the new %fs descriptor being loaded and
- * %fs being effectively cleared at __switch_to().
+ * %fs being effectively cleared at __switch_to(). We can't
+ * just zero %gs, but we do need to clear the selector in
+ * case of a Xen vCPU context switch before it gets reloaded
+ * which would also cause a fault.
*/
if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) {
#ifdef CONFIG_X86_32
lazy_load_gs(0);
#else
+ struct multicall_space mc = __xen_mc_entry(0);
+ MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
+
loadsegment(fs, 0);
#endif
}
- xen_mc_batch();
-
load_TLS_descriptor(t, cpu, 0);
load_TLS_descriptor(t, cpu, 1);
load_TLS_descriptor(t, cpu, 2);
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-06 18:49 ` [Xen-devel] " Andy Lutomirski
@ 2018-12-06 20:27 ` David Woodhouse
2018-12-06 20:27 ` [Xen-devel] " David Woodhouse
1 sibling, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-06 20:27 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper
Cc: Juergen Gross, Denys Vlasenko, Sarah Newman, Peter Zijlstra,
Greg KH, Dave Hansen, M. Vefa Bicakci, Brian Gerst,
Dominik Brodowski, Ingo Molnar, Borislav Petkov, stable,
H. Peter Anvin, Josh Poimboeuf, xen-devel, Thomas Gleixner,
Linus Torvalds, Boris Ostrovsky
[-- Attachment #1.1: Type: text/plain, Size: 3611 bytes --]
On Thu, 2018-12-06 at 10:49 -0800, Andy Lutomirski wrote:
> > On Dec 6, 2018, at 9:36 AM, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> > Basically - what is happening is that xen_load_tls() is invalidating the
> > %gs selector while %gs is still non-NUL.
> >
> > If this happens to intersect with a vcpu reschedule, %gs (being non-NUL)
> > takes precedence over KERNGSBASE, and faults when Xen tries to reload
> > it. This results in the failsafe callback being invoked.
> >
> > I think the correct course of action is to use xen_load_gs_index(0)
> > (poorly named - it is a hypercall which does swapgs; mov to %gs; swapgs)
> > before using update_descriptor() to invalidate the segment.
> >
> > That will reset %gs to 0 without touching KERNGSBASE, and can be queued
> > in the same multicall as the update_descriptor() hypercall.
>
> Sounds good to me as long as we skip it on native.
Like this? The other option is just to declare that we don't care. On
the rare occasion that it does happen to preempt and then take the trap
on reloading, xen_failsafe_callback is actually doing the right thing
and just leaving %gs as zero. We'd just need to fix the comments so
they explicitly note this case is handled there too. At the moment it
just says 'Retry the IRET', as I noted before.
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index ef05bea7010d..e8b383b24246 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -520,4 +520,15 @@ MULTI_stack_switch(struct multicall_entry *mcl,
trace_xen_mc_entry(mcl, 2);
}
+static inline void
+MULTI_set_segment_base(struct multicall_entry *mcl,
+ int reg, unsigned long value)
+{
+ mcl->op = __HYPERVISOR_set_segment_base;
+ mcl->args[0] = reg;
+ mcl->args[1] = value;
+
+ trace_xen_mc_entry(mcl, 2);
+}
+
#endif /* _ASM_X86_XEN_HYPERCALL_H */
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 2f6787fc7106..722f1f51e20c 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -527,6 +527,8 @@ static void load_TLS_descriptor(struct thread_struct *t,
static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
{
+ xen_mc_batch();
+
/*
* XXX sleazy hack: If we're being called in a lazy-cpu zone
* and lazy gs handling is enabled, it means we're in a
@@ -537,24 +539,24 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
* This will go away as soon as Xen has been modified to not
* save/restore %gs for normal hypercalls.
*
- * On x86_64, this hack is not used for %gs, because gs points
- * to KERNEL_GS_BASE (and uses it for PDA references), so we
- * must not zero %gs on x86_64
- *
* For x86_64, we need to zero %fs, otherwise we may get an
* exception between the new %fs descriptor being loaded and
- * %fs being effectively cleared at __switch_to().
+ * %fs being effectively cleared at __switch_to(). We can't
+ * just zero %gs, but we do need to clear the selector in
+ * case of a Xen vCPU context switch before it gets reloaded
+ * which would also cause a fault.
*/
if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) {
#ifdef CONFIG_X86_32
lazy_load_gs(0);
#else
+ struct multicall_space mc = __xen_mc_entry(0);
+ MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
+
loadsegment(fs, 0);
#endif
}
- xen_mc_batch();
-
load_TLS_descriptor(t, cpu, 0);
load_TLS_descriptor(t, cpu, 1);
load_TLS_descriptor(t, cpu, 2);
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-06 20:27 ` [Xen-devel] " David Woodhouse
2018-12-07 12:18 ` David Woodhouse
@ 2018-12-07 12:18 ` David Woodhouse
2018-12-07 20:08 ` David Woodhouse
2018-12-07 20:08 ` [Xen-devel] " David Woodhouse
1 sibling, 2 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-07 12:18 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper
Cc: Juergen Gross, Thomas Gleixner, Denys Vlasenko, Josh Poimboeuf,
Peter Zijlstra, Greg KH, Dave Hansen, M. Vefa Bicakci,
Dominik Brodowski, Sarah Newman, Linus Torvalds, Borislav Petkov,
stable, Brian Gerst, xen-devel, Boris Ostrovsky, H. Peter Anvin,
Ingo Molnar
[-- Attachment #1: Type: text/plain, Size: 1761 bytes --]
On Thu, 2018-12-06 at 20:27 +0000, David Woodhouse wrote:
> On Thu, 2018-12-06 at 10:49 -0800, Andy Lutomirski wrote:
> > > On Dec 6, 2018, at 9:36 AM, Andrew Cooper <
> > > andrew.cooper3@citrix.com> wrote:
> > > Basically - what is happening is that xen_load_tls() is
> > > invalidating the
> > > %gs selector while %gs is still non-NUL.
> > >
> > > If this happens to intersect with a vcpu reschedule, %gs (being
> > > non-NUL)
> > > takes precedence over KERNGSBASE, and faults when Xen tries to
> > > reload
> > > it. This results in the failsafe callback being invoked.
> > >
> > > I think the correct course of action is to use
> > > xen_load_gs_index(0)
> > > (poorly named - it is a hypercall which does swapgs; mov to %gs;
> > > swapgs)
> > > before using update_descriptor() to invalidate the segment.
> > >
> > > That will reset %gs to 0 without touching KERNGSBASE, and can be
> > > queued
> > > in the same multicall as the update_descriptor() hypercall.
> >
> > Sounds good to me as long as we skip it on native.
>
> Like this?
> #else
> + struct multicall_space mc = __xen_mc_entry(0);
> + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
> +
> loadsegment(fs, 0);
> #endif
That seems to boot and run, at least.
I'm going to experiment with sticking a SCHEDOP_yield in the batch
*after* the update_descriptor requests, to see if I can trigger the
original problem a bit quicker than my current method — which involves
running a hundred machines for a day or two.
Still wondering if the better fix is just to adjust the comments to
admit that xen_failsafe_callback catches the race condition and fixes
it up perfectly, by just letting the %gs selector be zero for a while?
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-06 20:27 ` [Xen-devel] " David Woodhouse
@ 2018-12-07 12:18 ` David Woodhouse
2018-12-07 12:18 ` [Xen-devel] " David Woodhouse
1 sibling, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-07 12:18 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper
Cc: Juergen Gross, Denys Vlasenko, Sarah Newman, Peter Zijlstra,
Greg KH, Dave Hansen, M. Vefa Bicakci, Brian Gerst,
Dominik Brodowski, Ingo Molnar, Borislav Petkov, stable,
H. Peter Anvin, Josh Poimboeuf, xen-devel, Thomas Gleixner,
Linus Torvalds, Boris Ostrovsky
[-- Attachment #1.1: Type: text/plain, Size: 1761 bytes --]
On Thu, 2018-12-06 at 20:27 +0000, David Woodhouse wrote:
> On Thu, 2018-12-06 at 10:49 -0800, Andy Lutomirski wrote:
> > > On Dec 6, 2018, at 9:36 AM, Andrew Cooper <
> > > andrew.cooper3@citrix.com> wrote:
> > > Basically - what is happening is that xen_load_tls() is
> > > invalidating the
> > > %gs selector while %gs is still non-NUL.
> > >
> > > If this happens to intersect with a vcpu reschedule, %gs (being
> > > non-NUL)
> > > takes precedence over KERNGSBASE, and faults when Xen tries to
> > > reload
> > > it. This results in the failsafe callback being invoked.
> > >
> > > I think the correct course of action is to use
> > > xen_load_gs_index(0)
> > > (poorly named - it is a hypercall which does swapgs; mov to %gs;
> > > swapgs)
> > > before using update_descriptor() to invalidate the segment.
> > >
> > > That will reset %gs to 0 without touching KERNGSBASE, and can be
> > > queued
> > > in the same multicall as the update_descriptor() hypercall.
> >
> > Sounds good to me as long as we skip it on native.
>
> Like this?
> #else
> + struct multicall_space mc = __xen_mc_entry(0);
> + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
> +
> loadsegment(fs, 0);
> #endif
That seems to boot and run, at least.
I'm going to experiment with sticking a SCHEDOP_yield in the batch
*after* the update_descriptor requests, to see if I can trigger the
original problem a bit quicker than my current method — which involves
running a hundred machines for a day or two.
Still wondering if the better fix is just to adjust the comments to
admit that xen_failsafe_callback catches the race condition and fixes
it up perfectly, by just letting the %gs selector be zero for a while?
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Xen-devel] Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-07 12:18 ` [Xen-devel] " David Woodhouse
2018-12-07 20:08 ` David Woodhouse
@ 2018-12-07 20:08 ` David Woodhouse
2018-12-07 22:15 ` [PATCH] x86/xen: Clear user %gs before updating segment descriptors David Woodhouse
2018-12-07 23:15 ` David Woodhouse
1 sibling, 2 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-07 20:08 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper
Cc: Juergen Gross, Thomas Gleixner, Denys Vlasenko, Josh Poimboeuf,
Peter Zijlstra, Greg KH, Dave Hansen, M. Vefa Bicakci,
Dominik Brodowski, Sarah Newman, Linus Torvalds, Borislav Petkov,
stable, Brian Gerst, xen-devel, Boris Ostrovsky, H. Peter Anvin,
Ingo Molnar
[-- Attachment #1: Type: text/plain, Size: 4186 bytes --]
On Fri, 2018-12-07 at 12:18 +0000, David Woodhouse wrote:
>
> > #else
> > + struct multicall_space mc = __xen_mc_entry(0);
> > + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
> > +
> > loadsegment(fs, 0);
> > #endif
>
> That seems to boot and run, at least.
>
> I'm going to experiment with sticking a SCHEDOP_yield in the batch
> *after* the update_descriptor requests, to see if I can trigger the
> original problem a bit quicker than my current method — which involves
> running a hundred machines for a day or two.
That SCHEDOP_yield and some debug output in xen_failsafe_callback shows
that it's nice and easy to reproduce now without the above
MULTI_set_segment_base() call. And stopped happening when I add the
MULTI_set_segment_base() call back again. I'll call that 'tested'.
But now we're making a hypercall to clear user %gs even in the case
where none of the descriptors have changed; we should probably stop
doing that...
Testing this (incremental) variant now.
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -688,7 +688,7 @@ static inline bool desc_equal(const struct desc_struct *d1,
}
static void load_TLS_descriptor(struct thread_struct *t,
- unsigned int cpu, unsigned int i)
+ unsigned int cpu, unsigned int i, int flush_gs)
{
struct desc_struct *shadow = &per_cpu(shadow_tls_desc, cpu).desc[i];
struct desc_struct *gdt;
@@ -698,6 +698,15 @@ static void load_TLS_descriptor(struct thread_struct *t,
if (desc_equal(shadow, &t->tls_array[i]))
return;
+ /*
+ * If the current user %gs points to a descriptor we're changing,
+ * zero it first to avoid taking a fault if Xen preempts this
+ * vCPU between now and the time that %gs is later loaded with
+ * the new value.
+ */
+ if ((flush_gs >> 3) == GDT_ENTRY_TLS_MIN + i)
+ MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
+
*shadow = t->tls_array[i];
gdt = get_cpu_gdt_table(cpu);
@@ -709,7 +718,7 @@ static void load_TLS_descriptor(struct thread_struct *t,
static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
{
- xen_mc_batch();
+ u16 flush_gs = 0;
/*
* XXX sleazy hack: If we're being called in a lazy-cpu zone
@@ -723,17 +732,19 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
*
* For x86_64, we need to zero %fs, otherwise we may get an
* exception between the new %fs descriptor being loaded and
- * %fs being effectively cleared at __switch_to(). We can't
- * just zero %gs, but we do need to clear the selector in
- * case of a Xen vCPU context switch before it gets reloaded
- * which would also cause a fault.
+ * %fs being effectively cleared at __switch_to().
+ *
+ * We may also need to zero %gs, if it refers to a descriptor
+ * which we are clearing. Otherwise a Xen vCPU context switch
+ * before it gets reloaded could also cause a fault. Since
+ * clearing the user %gs is another hypercall, do that only if
+ * it's necessary.
*/
if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) {
#ifdef CONFIG_X86_32
lazy_load_gs(0);
#else
- struct multicall_space mc = __xen_mc_entry(0);
- MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
+ savesegment(gs, flush_gs);
loadsegment(fs, 0);
#endif
@@ -741,9 +752,9 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
xen_mc_batch();
- load_TLS_descriptor(t, cpu, 0);
- load_TLS_descriptor(t, cpu, 1);
- load_TLS_descriptor(t, cpu, 2);
+ load_TLS_descriptor(t, cpu, 0, flush_gs);
+ load_TLS_descriptor(t, cpu, 1, flush_gs);
+ load_TLS_descriptor(t, cpu, 2, flush_gs);
{
struct multicall_space mc = __xen_mc_entry(0);
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree
2018-12-07 12:18 ` [Xen-devel] " David Woodhouse
@ 2018-12-07 20:08 ` David Woodhouse
2018-12-07 20:08 ` [Xen-devel] " David Woodhouse
1 sibling, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-07 20:08 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper
Cc: Juergen Gross, Denys Vlasenko, Sarah Newman, Peter Zijlstra,
Greg KH, Dave Hansen, M. Vefa Bicakci, Brian Gerst,
Dominik Brodowski, Ingo Molnar, Borislav Petkov, stable,
H. Peter Anvin, Josh Poimboeuf, xen-devel, Thomas Gleixner,
Linus Torvalds, Boris Ostrovsky
[-- Attachment #1.1: Type: text/plain, Size: 4186 bytes --]
On Fri, 2018-12-07 at 12:18 +0000, David Woodhouse wrote:
>
> > #else
> > + struct multicall_space mc = __xen_mc_entry(0);
> > + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
> > +
> > loadsegment(fs, 0);
> > #endif
>
> That seems to boot and run, at least.
>
> I'm going to experiment with sticking a SCHEDOP_yield in the batch
> *after* the update_descriptor requests, to see if I can trigger the
> original problem a bit quicker than my current method — which involves
> running a hundred machines for a day or two.
That SCHEDOP_yield and some debug output in xen_failsafe_callback shows
that it's nice and easy to reproduce now without the above
MULTI_set_segment_base() call. And stopped happening when I add the
MULTI_set_segment_base() call back again. I'll call that 'tested'.
But now we're making a hypercall to clear user %gs even in the case
where none of the descriptors have changed; we should probably stop
doing that...
Testing this (incremental) variant now.
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -688,7 +688,7 @@ static inline bool desc_equal(const struct desc_struct *d1,
}
static void load_TLS_descriptor(struct thread_struct *t,
- unsigned int cpu, unsigned int i)
+ unsigned int cpu, unsigned int i, int flush_gs)
{
struct desc_struct *shadow = &per_cpu(shadow_tls_desc, cpu).desc[i];
struct desc_struct *gdt;
@@ -698,6 +698,15 @@ static void load_TLS_descriptor(struct thread_struct *t,
if (desc_equal(shadow, &t->tls_array[i]))
return;
+ /*
+ * If the current user %gs points to a descriptor we're changing,
+ * zero it first to avoid taking a fault if Xen preempts this
+ * vCPU between now and the time that %gs is later loaded with
+ * the new value.
+ */
+ if ((flush_gs >> 3) == GDT_ENTRY_TLS_MIN + i)
+ MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
+
*shadow = t->tls_array[i];
gdt = get_cpu_gdt_table(cpu);
@@ -709,7 +718,7 @@ static void load_TLS_descriptor(struct thread_struct *t,
static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
{
- xen_mc_batch();
+ u16 flush_gs = 0;
/*
* XXX sleazy hack: If we're being called in a lazy-cpu zone
@@ -723,17 +732,19 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
*
* For x86_64, we need to zero %fs, otherwise we may get an
* exception between the new %fs descriptor being loaded and
- * %fs being effectively cleared at __switch_to(). We can't
- * just zero %gs, but we do need to clear the selector in
- * case of a Xen vCPU context switch before it gets reloaded
- * which would also cause a fault.
+ * %fs being effectively cleared at __switch_to().
+ *
+ * We may also need to zero %gs, if it refers to a descriptor
+ * which we are clearing. Otherwise a Xen vCPU context switch
+ * before it gets reloaded could also cause a fault. Since
+ * clearing the user %gs is another hypercall, do that only if
+ * it's necessary.
*/
if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) {
#ifdef CONFIG_X86_32
lazy_load_gs(0);
#else
- struct multicall_space mc = __xen_mc_entry(0);
- MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
+ savesegment(gs, flush_gs);
loadsegment(fs, 0);
#endif
@@ -741,9 +752,9 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
xen_mc_batch();
- load_TLS_descriptor(t, cpu, 0);
- load_TLS_descriptor(t, cpu, 1);
- load_TLS_descriptor(t, cpu, 2);
+ load_TLS_descriptor(t, cpu, 0, flush_gs);
+ load_TLS_descriptor(t, cpu, 1, flush_gs);
+ load_TLS_descriptor(t, cpu, 2, flush_gs);
{
struct multicall_space mc = __xen_mc_entry(0);
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] x86/xen: Clear user %gs before updating segment descriptors
2018-12-07 20:08 ` [Xen-devel] " David Woodhouse
@ 2018-12-07 22:15 ` David Woodhouse
2018-12-07 23:15 ` David Woodhouse
1 sibling, 0 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-07 22:15 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper, Juergen Gross, Thomas Gleixner,
x86, xen-devel
During a context switch, if clearing a descriptor which is currently
referenced by the old process's user %gs, if Xen preempts the vCPU
before %gs is set for the new process, a fault may occur.
This fault actually seems to be fairly harmless; xen_failsafe_callback
will just return to the "faulting" instruction (the random one after the
vCPU was preempted) with the offending segment register zeroed, and then
it'll get set again later during the context switch. But it's cleaner
to avoid it.
If the descriptor referenced by the %gs selector is being touched,
then include a request to zero the user %gs in the multicall too.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
arch/x86/include/asm/xen/hypercall.h | 11 ++++++++
arch/x86/xen/enlighten_pv.c | 42 +++++++++++++++++++++-------
2 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index ef05bea7010d..e8b383b24246 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -520,4 +520,15 @@ MULTI_stack_switch(struct multicall_entry *mcl,
trace_xen_mc_entry(mcl, 2);
}
+static inline void
+MULTI_set_segment_base(struct multicall_entry *mcl,
+ int reg, unsigned long value)
+{
+ mcl->op = __HYPERVISOR_set_segment_base;
+ mcl->args[0] = reg;
+ mcl->args[1] = value;
+
+ trace_xen_mc_entry(mcl, 2);
+}
+
#endif /* _ASM_X86_XEN_HYPERCALL_H */
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 2f6787fc7106..e502d12ffd17 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -506,7 +506,7 @@ static inline bool desc_equal(const struct desc_struct *d1,
}
static void load_TLS_descriptor(struct thread_struct *t,
- unsigned int cpu, unsigned int i)
+ unsigned int cpu, unsigned int i, int flush_gs)
{
struct desc_struct *shadow = &per_cpu(shadow_tls_desc, cpu).desc[i];
struct desc_struct *gdt;
@@ -516,6 +516,17 @@ static void load_TLS_descriptor(struct thread_struct *t,
if (desc_equal(shadow, &t->tls_array[i]))
return;
+ /*
+ * If the current user %gs points to a descriptor we're changing,
+ * zero it first to avoid taking a fault if Xen preempts this
+ * vCPU between now and the time that %gs is later loaded with
+ * the new value.
+ */
+ if ((flush_gs >> 3) == GDT_ENTRY_TLS_MIN + i) {
+ mc = __xen_mc_entry(0);
+ MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
+ }
+
*shadow = t->tls_array[i];
gdt = get_cpu_gdt_rw(cpu);
@@ -527,6 +538,8 @@ static void load_TLS_descriptor(struct thread_struct *t,
static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
{
+ u16 flush_gs = 0;
+
/*
* XXX sleazy hack: If we're being called in a lazy-cpu zone
* and lazy gs handling is enabled, it means we're in a
@@ -537,27 +550,36 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
* This will go away as soon as Xen has been modified to not
* save/restore %gs for normal hypercalls.
*
- * On x86_64, this hack is not used for %gs, because gs points
- * to KERNEL_GS_BASE (and uses it for PDA references), so we
- * must not zero %gs on x86_64
- *
* For x86_64, we need to zero %fs, otherwise we may get an
* exception between the new %fs descriptor being loaded and
* %fs being effectively cleared at __switch_to().
+ *
+ * We may also need to zero %gs, if it refers to a descriptor
+ * which we are clearing. Otherwise a Xen vCPU context switch
+ * before it gets reloaded could also cause a fault. Since
+ * clearing the user %gs is another hypercall, do that only if
+ * it's necessary.
+ *
+ * Note: These "faults" end up in xen_failsafe_callback(),
+ * which just returns immediately to the "faulting" instruction
+ * (i.e. the random one after Xen preempted this vCPU) with
+ * the offending segment register zeroed. Which is actually
+ * a perfectly safe thing to happen anyway, as it'll be loaded
+ * again shortly. So maybe we needn't bother?
*/
if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) {
#ifdef CONFIG_X86_32
lazy_load_gs(0);
#else
+ savesegment(gs, flush_gs);
+
loadsegment(fs, 0);
#endif
}
- xen_mc_batch();
-
- load_TLS_descriptor(t, cpu, 0);
- load_TLS_descriptor(t, cpu, 1);
- load_TLS_descriptor(t, cpu, 2);
+ load_TLS_descriptor(t, cpu, 0, flush_gs);
+ load_TLS_descriptor(t, cpu, 1, flush_gs);
+ load_TLS_descriptor(t, cpu, 2, flush_gs);
xen_mc_issue(PARAVIRT_LAZY_CPU);
}
--
2.17.1
Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH] x86/xen: Clear user %gs before updating segment descriptors
2018-12-07 20:08 ` [Xen-devel] " David Woodhouse
2018-12-07 22:15 ` [PATCH] x86/xen: Clear user %gs before updating segment descriptors David Woodhouse
@ 2018-12-07 23:15 ` David Woodhouse
2018-12-07 23:27 ` Andy Lutomirski
2018-12-10 16:31 ` Boris Ostrovsky
1 sibling, 2 replies; 23+ messages in thread
From: David Woodhouse @ 2018-12-07 23:15 UTC (permalink / raw)
To: Andy Lutomirski, Andrew Cooper, Juergen Gross, Thomas Gleixner,
x86, xen-devel, dwmw2
During a context switch, if clearing a descriptor which is currently
referenced by the old process's user %gs, if Xen preempts the vCPU
before %gs is set for the new process, a fault may occur.
This fault actually seems to be fairly harmless; xen_failsafe_callback
will just return to the "faulting" instruction (the random one after the
vCPU was preempted) with the offending segment register zeroed, and then
it'll get set again later during the context switch. But it's cleaner
to avoid it.
If the descriptor referenced by the %gs selector is being touched,
then include a request to zero the user %gs in the multicall too.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
v2: Don't accidentally remove the call to xen_mc_batch().
arch/x86/include/asm/xen/hypercall.h | 11 ++++++++
arch/x86/xen/enlighten_pv.c | 40 ++++++++++++++++++++++------
2 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
index ef05bea7010d..e8b383b24246 100644
--- a/arch/x86/include/asm/xen/hypercall.h
+++ b/arch/x86/include/asm/xen/hypercall.h
@@ -520,4 +520,15 @@ MULTI_stack_switch(struct multicall_entry *mcl,
trace_xen_mc_entry(mcl, 2);
}
+static inline void
+MULTI_set_segment_base(struct multicall_entry *mcl,
+ int reg, unsigned long value)
+{
+ mcl->op = __HYPERVISOR_set_segment_base;
+ mcl->args[0] = reg;
+ mcl->args[1] = value;
+
+ trace_xen_mc_entry(mcl, 2);
+}
+
#endif /* _ASM_X86_XEN_HYPERCALL_H */
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index 2f6787fc7106..2eb9827dab4b 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -506,7 +506,7 @@ static inline bool desc_equal(const struct desc_struct *d1,
}
static void load_TLS_descriptor(struct thread_struct *t,
- unsigned int cpu, unsigned int i)
+ unsigned int cpu, unsigned int i, int flush_gs)
{
struct desc_struct *shadow = &per_cpu(shadow_tls_desc, cpu).desc[i];
struct desc_struct *gdt;
@@ -516,6 +516,17 @@ static void load_TLS_descriptor(struct thread_struct *t,
if (desc_equal(shadow, &t->tls_array[i]))
return;
+ /*
+ * If the current user %gs points to a descriptor we're changing,
+ * zero it first to avoid taking a fault if Xen preempts this
+ * vCPU between now and the time that %gs is later loaded with
+ * the new value.
+ */
+ if ((flush_gs >> 3) == GDT_ENTRY_TLS_MIN + i) {
+ mc = __xen_mc_entry(0);
+ MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
+ }
+
*shadow = t->tls_array[i];
gdt = get_cpu_gdt_rw(cpu);
@@ -527,6 +538,8 @@ static void load_TLS_descriptor(struct thread_struct *t,
static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
{
+ u16 flush_gs = 0;
+
/*
* XXX sleazy hack: If we're being called in a lazy-cpu zone
* and lazy gs handling is enabled, it means we're in a
@@ -537,27 +550,38 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
* This will go away as soon as Xen has been modified to not
* save/restore %gs for normal hypercalls.
*
- * On x86_64, this hack is not used for %gs, because gs points
- * to KERNEL_GS_BASE (and uses it for PDA references), so we
- * must not zero %gs on x86_64
- *
* For x86_64, we need to zero %fs, otherwise we may get an
* exception between the new %fs descriptor being loaded and
* %fs being effectively cleared at __switch_to().
+ *
+ * We may also need to zero %gs, if it refers to a descriptor
+ * which we are clearing. Otherwise a Xen vCPU context switch
+ * before it gets reloaded could also cause a fault. Since
+ * clearing the user %gs is another hypercall, do that only if
+ * it's necessary.
+ *
+ * Note: These "faults" end up in xen_failsafe_callback(),
+ * which just returns immediately to the "faulting" instruction
+ * (i.e. the random one after Xen preempted this vCPU) with
+ * the offending segment register zeroed. Which is actually
+ * a perfectly safe thing to happen anyway, as it'll be loaded
+ * again shortly. So maybe we needn't bother?
*/
if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) {
#ifdef CONFIG_X86_32
lazy_load_gs(0);
#else
+ savesegment(gs, flush_gs);
+
loadsegment(fs, 0);
#endif
}
xen_mc_batch();
- load_TLS_descriptor(t, cpu, 0);
- load_TLS_descriptor(t, cpu, 1);
- load_TLS_descriptor(t, cpu, 2);
+ load_TLS_descriptor(t, cpu, 0, flush_gs);
+ load_TLS_descriptor(t, cpu, 1, flush_gs);
+ load_TLS_descriptor(t, cpu, 2, flush_gs);
xen_mc_issue(PARAVIRT_LAZY_CPU);
}
--
2.17.1
Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] x86/xen: Clear user %gs before updating segment descriptors
2018-12-07 23:15 ` David Woodhouse
@ 2018-12-07 23:27 ` Andy Lutomirski
2018-12-10 16:31 ` Boris Ostrovsky
1 sibling, 0 replies; 23+ messages in thread
From: Andy Lutomirski @ 2018-12-07 23:27 UTC (permalink / raw)
To: Woodhouse, David
Cc: Juergen Gross, Andrew Cooper, X86 ML, Andrew Lutomirski,
xen-devel, Thomas Gleixner, David Woodhouse
On Fri, Dec 7, 2018 at 3:15 PM David Woodhouse <dwmw@amazon.co.uk> wrote:
>
> During a context switch, if clearing a descriptor which is currently
> referenced by the old process's user %gs, if Xen preempts the vCPU
> before %gs is set for the new process, a fault may occur.
>
> This fault actually seems to be fairly harmless; xen_failsafe_callback
> will just return to the "faulting" instruction (the random one after the
> vCPU was preempted) with the offending segment register zeroed, and then
> it'll get set again later during the context switch. But it's cleaner
> to avoid it.
>
> If the descriptor referenced by the %gs selector is being touched,
> then include a request to zero the user %gs in the multicall too.
Fine with me.
>
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> v2: Don't accidentally remove the call to xen_mc_batch().
>
> arch/x86/include/asm/xen/hypercall.h | 11 ++++++++
> arch/x86/xen/enlighten_pv.c | 40 ++++++++++++++++++++++------
> 2 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h
> index ef05bea7010d..e8b383b24246 100644
> --- a/arch/x86/include/asm/xen/hypercall.h
> +++ b/arch/x86/include/asm/xen/hypercall.h
> @@ -520,4 +520,15 @@ MULTI_stack_switch(struct multicall_entry *mcl,
> trace_xen_mc_entry(mcl, 2);
> }
>
> +static inline void
> +MULTI_set_segment_base(struct multicall_entry *mcl,
> + int reg, unsigned long value)
> +{
> + mcl->op = __HYPERVISOR_set_segment_base;
> + mcl->args[0] = reg;
> + mcl->args[1] = value;
> +
> + trace_xen_mc_entry(mcl, 2);
> +}
> +
> #endif /* _ASM_X86_XEN_HYPERCALL_H */
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index 2f6787fc7106..2eb9827dab4b 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -506,7 +506,7 @@ static inline bool desc_equal(const struct desc_struct *d1,
> }
>
> static void load_TLS_descriptor(struct thread_struct *t,
> - unsigned int cpu, unsigned int i)
> + unsigned int cpu, unsigned int i, int flush_gs)
> {
> struct desc_struct *shadow = &per_cpu(shadow_tls_desc, cpu).desc[i];
> struct desc_struct *gdt;
> @@ -516,6 +516,17 @@ static void load_TLS_descriptor(struct thread_struct *t,
> if (desc_equal(shadow, &t->tls_array[i]))
> return;
>
> + /*
> + * If the current user %gs points to a descriptor we're changing,
> + * zero it first to avoid taking a fault if Xen preempts this
> + * vCPU between now and the time that %gs is later loaded with
> + * the new value.
> + */
> + if ((flush_gs >> 3) == GDT_ENTRY_TLS_MIN + i) {
> + mc = __xen_mc_entry(0);
> + MULTI_set_segment_base(mc.mc, SEGBASE_GS_USER_SEL, 0);
> + }
> +
> *shadow = t->tls_array[i];
>
> gdt = get_cpu_gdt_rw(cpu);
> @@ -527,6 +538,8 @@ static void load_TLS_descriptor(struct thread_struct *t,
>
> static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
> {
> + u16 flush_gs = 0;
> +
> /*
> * XXX sleazy hack: If we're being called in a lazy-cpu zone
> * and lazy gs handling is enabled, it means we're in a
> @@ -537,27 +550,38 @@ static void xen_load_tls(struct thread_struct *t, unsigned int cpu)
> * This will go away as soon as Xen has been modified to not
> * save/restore %gs for normal hypercalls.
> *
> - * On x86_64, this hack is not used for %gs, because gs points
> - * to KERNEL_GS_BASE (and uses it for PDA references), so we
> - * must not zero %gs on x86_64
> - *
> * For x86_64, we need to zero %fs, otherwise we may get an
> * exception between the new %fs descriptor being loaded and
> * %fs being effectively cleared at __switch_to().
> + *
> + * We may also need to zero %gs, if it refers to a descriptor
> + * which we are clearing. Otherwise a Xen vCPU context switch
> + * before it gets reloaded could also cause a fault. Since
> + * clearing the user %gs is another hypercall, do that only if
> + * it's necessary.
> + *
> + * Note: These "faults" end up in xen_failsafe_callback(),
> + * which just returns immediately to the "faulting" instruction
> + * (i.e. the random one after Xen preempted this vCPU) with
> + * the offending segment register zeroed. Which is actually
> + * a perfectly safe thing to happen anyway, as it'll be loaded
> + * again shortly. So maybe we needn't bother?
> */
> if (paravirt_get_lazy_mode() == PARAVIRT_LAZY_CPU) {
> #ifdef CONFIG_X86_32
> lazy_load_gs(0);
> #else
> + savesegment(gs, flush_gs);
> +
> loadsegment(fs, 0);
> #endif
> }
>
> xen_mc_batch();
>
> - load_TLS_descriptor(t, cpu, 0);
> - load_TLS_descriptor(t, cpu, 1);
> - load_TLS_descriptor(t, cpu, 2);
> + load_TLS_descriptor(t, cpu, 0, flush_gs);
> + load_TLS_descriptor(t, cpu, 1, flush_gs);
> + load_TLS_descriptor(t, cpu, 2, flush_gs);
>
> xen_mc_issue(PARAVIRT_LAZY_CPU);
> }
> --
> 2.17.1
>
>
>
>
> Amazon Development Centre (London) Ltd. Registered in England and Wales with registration number 04543232 with its registered office at 1 Principal Place, Worship Street, London EC2A 2FA, United Kingdom.
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] x86/xen: Clear user %gs before updating segment descriptors
2018-12-07 23:15 ` David Woodhouse
2018-12-07 23:27 ` Andy Lutomirski
@ 2018-12-10 16:31 ` Boris Ostrovsky
1 sibling, 0 replies; 23+ messages in thread
From: Boris Ostrovsky @ 2018-12-10 16:31 UTC (permalink / raw)
To: David Woodhouse, Andy Lutomirski, Andrew Cooper, Juergen Gross,
Thomas Gleixner, x86, xen-devel, dwmw2
On 12/7/18 6:15 PM, David Woodhouse wrote:
>
> - load_TLS_descriptor(t, cpu, 0);
> - load_TLS_descriptor(t, cpu, 1);
> - load_TLS_descriptor(t, cpu, 2);
> + load_TLS_descriptor(t, cpu, 0, flush_gs);
> + load_TLS_descriptor(t, cpu, 1, flush_gs);
> + load_TLS_descriptor(t, cpu, 2, flush_gs);
Since you are changing these lines anyway, can you do a loop over
[0..GDT_ENTRY_TLS_ENTRIES)?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-12-10 16:32 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-22 7:19 Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree gregkh
2018-11-28 14:56 ` [Xen-devel] " David Woodhouse
2018-11-28 15:43 ` Sasha Levin
2018-11-28 15:43 ` Sasha Levin
2018-11-28 16:44 ` Andy Lutomirski
2018-11-28 16:44 ` [Xen-devel] " Andy Lutomirski
2018-12-06 17:10 ` David Woodhouse
2018-12-06 17:36 ` Andrew Cooper
2018-12-06 17:36 ` [Xen-devel] " Andrew Cooper
2018-12-06 18:49 ` Andy Lutomirski
2018-12-06 18:49 ` [Xen-devel] " Andy Lutomirski
2018-12-06 20:27 ` David Woodhouse
2018-12-06 20:27 ` [Xen-devel] " David Woodhouse
2018-12-07 12:18 ` David Woodhouse
2018-12-07 12:18 ` [Xen-devel] " David Woodhouse
2018-12-07 20:08 ` David Woodhouse
2018-12-07 20:08 ` [Xen-devel] " David Woodhouse
2018-12-07 22:15 ` [PATCH] x86/xen: Clear user %gs before updating segment descriptors David Woodhouse
2018-12-07 23:15 ` David Woodhouse
2018-12-07 23:27 ` Andy Lutomirski
2018-12-10 16:31 ` Boris Ostrovsky
2018-12-06 17:10 ` Patch "x86/entry/64: Remove %ebx handling from error_entry/exit" has been added to the 4.9-stable tree David Woodhouse
2018-11-28 14:56 ` David Woodhouse
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.