Double fault when single-stepping compat task with PREEMPT_RT

Double fault when single-stepping compat task with PREEMPT_RT

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 6249 bytes --]

On Tue, 2013-09-24 at 13:43 -0700, Brian Silverman wrote:
[...]
> I got down to a really simple program that reproduces this bug:
> 
> 
> #include <sys/syscall.h>
> #include <unistd.h>
> int main() {
>   // I've tried SYS_getpid, SYS_write, and SYS_read here too.
>   syscall(SYS_gettid);
> }
> 
> 
> Any syscall I put in there seems to give the same results. In order
> for it to trigger the bug, you have to compile it with `gcc -m32
> whatever.c` (I'm testing with the standard Wheezy gcc (4:4.7.2-1) and
> gdb (7.4.1+dfsg-0.1)). I would imagine that something in gcc and/or
> gdb is contributing to this too.
> 
> 
> I also minimized the gdb commands down to:
> 
> 
> break main
> run
> record

I assume this enables single-stepping.

> continue
[...]

I can reproduce this in VMs running the latest Debian RT kernel versions
(based on 3.2.51-rt72, and on 3.10.11 with the 3.10.10-rt7 patch).

As Brian says, x86_64 userland on x86_64 kernel works, and similarly for
i386 on i386.  So it is specifically the 'compat' case that's broken.

Here's what I got:

[   68.394276] double fault: 0000 [#1] PREEMPT SMP 
[   68.394304] Modules linked in: rfcomm bnep bluetooth rfkill crc16 nfsd auth_rpcgss oid_registry nfs_acl nfs lockd dns_resolver fscache sunrpc loop fuse joydev hid_generic usbhid hid snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_page_alloc snd_seq snd_seq_device snd_timer snd processor evdev ttm drm_kms_helper thermal_sys psmouse soundcore drm i2c_piix4 serio_raw virtio_balloon button i2c_core pcspkr microcode ext3 mbcache jbd sg sr_mod cdrom ata_generic virtio_net virtio_blk floppy uhci_hcd ata_piix ehci_hcd libata usbcore virtio_pci scsi_mod usb_common virtio_ring virtio
[   68.394307] CPU: 0 PID: 3044 Comm: bug723180 Not tainted 3.10-3-rt-amd64 #1 Debian 3.10.11-1
[   68.394307] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   68.394309] task: ffff88001df34300 ti: ffff88001b564000 task.ti: ffff88001b564000
[   68.394316] RIP: 0010:[<ffffffff8139ec30>]  [<ffffffff8139ec30>] native_irq_enable_sysexit+0x10/0x10
[   68.394317] RSP: 0018:0000000000000000  EFLAGS: 00010192
[   68.394318] RAX: 00000000000000e0 RBX: 0000000000000000 RCX: 000000000804842b
[   68.394319] RDX: 00000000f7fc7000 RSI: 0000000008048420 RDI: 0000000000000000
[   68.394319] RBP: 00000000ffffd53c R08: 0000000000000000 R09: 0000000000000000
[   68.394320] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[   68.394320] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[   68.394321] FS:  0000000000000000(0000) GS:ffff88001fc00000(0063) knlGS:00000000f7e1b900
[   68.394322] CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
[   68.394323] CR2: fffffffffffffff8 CR3: 000000001f121000 CR4: 00000000000006f0
[   68.394326] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   68.394329] DR3: 0000000000000000 DR6: 00000000ffff4ff0 DR7: 0000000000000400
[   68.394329] Stack:
[   68.394331]  ffff88001fc05e68 ffff88001fc05f58 0000000000000ac0 0000000000000000
[   68.394332]  0000000000000000 0000000000000000 0000000000000040 ffffffff8100ef56
[   68.394334]  0000000081837a35 00000000ffffffff ffff88001fc05f58 ffffffff814011e8
[   68.394334] Call Trace:
[   68.394336]  <#DF> 
[   68.394340]  [<ffffffff8100ef56>] ? show_regs+0x6d/0x1bd
[   68.394343]  [<ffffffff81399cbe>] ? __die+0x9e/0xdb
[   68.394345]  [<ffffffff8100fbe9>] ? die+0x3d/0x56
[   68.394346]  [<ffffffff8100de24>] ? do_double_fault+0x5c/0x5e
[   68.394348]  [<ffffffff8139e888>] ? double_fault+0x28/0x30
[   68.394350]  [<ffffffff8139ec30>] ? native_irq_enable_sysexit+0x10/0x10
[   68.394351]  <<EOE>> 
[   68.394361] Code: 1f 84 00 00 00 00 00 0f 1f 40 00 0f 01 f8 0f 07 66 66 2e 0f 1f 84 00 00 00 00 00 0f 01 f8 fb 0f 35 66 2e 0f 1f 84 00 00 00 00 00 <0f> 01 f8 65 48 8b 24 25 e0 a7 00 00 48 83 c4 28 fb 0f 1f 80 00 
[   68.394362] RIP  [<ffffffff8139ec30>] native_irq_enable_sysexit+0x10/0x10
[   68.394362]  RSP <0000000000000000>
[   68.394385] ---[ end trace 0000000000000002 ]---
[   68.394434] ------------[ cut here ]------------
[   68.394442] WARNING: at /build/linux-BPzSEt/linux-3.10.11/debian/build/source_rt/kernel/smp.c:244 smp_call_function_single+0x71/0x157()
[   68.394454] Modules linked in: rfcomm bnep bluetooth rfkill crc16 nfsd auth_rpcgss oid_registry nfs_acl nfs lockd dns_resolver fscache sunrpc loop fuse joydev hid_generic usbhid hid snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_page_alloc snd_seq snd_seq_device snd_timer snd processor evdev ttm drm_kms_helper thermal_sys psmouse soundcore drm i2c_piix4 serio_raw virtio_balloon button i2c_core pcspkr microcode ext3 mbcache jbd sg sr_mod cdrom ata_generic virtio_net virtio_blk floppy uhci_hcd ata_piix ehci_hcd libata usbcore virtio_pci scsi_mod usb_common virtio_ring virtio
[   68.394456] CPU: 0 PID: 3044 Comm: bug723180 Tainted: G      D      3.10-3-rt-amd64 #1 Debian 3.10.11-1
[   68.394459] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
[   68.394460]  0000000000000000 ffffffff8103cced 0000000000000000 0000000000000000
[   68.394461]  0000000000000000 ffffffff810c08d9 ffff88001fc05e50 ffffffff810830f6
[   68.394462]  0000000000000000 0000000000000000 0000000000000000 0000000000000000
[   68.394463] Call Trace:
[   68.394466]  <#DF>  [<ffffffff8103cced>] ? warn_slowpath_common+0x5b/0x70
[   68.394470]  [<ffffffff810c08d9>] ? perf_cgroup_exit+0x16/0x16
[   68.394472]  [<ffffffff810830f6>] ? smp_call_function_single+0x71/0x157
[   68.394473]  [<ffffffff810bff63>] ? task_function_call+0x42/0x4c
[   68.394475]  [<ffffffff810c3f87>] ? perf_cgroup_switch+0x141/0x141
[   68.394477]  [<ffffffff810924af>] ? cgroup_exit+0xc8/0xd3
[   68.394478]  [<ffffffff81041cbd>] ? do_exit+0x404/0x946
[   68.394480]  [<ffffffff81399c1b>] ? oops_end+0xa9/0xae
[   68.394482]  [<ffffffff8100de24>] ? do_double_fault+0x5c/0x5e
[   68.394484]  [<ffffffff8139e888>] ? double_fault+0x28/0x30
[   68.394485]  [<ffffffff8139ec30>] ? native_irq_enable_sysexit+0x10/0x10
[   68.394486]  <<EOE>> 
[   68.394486] ---[ end trace 0000000000000003 ]---

Ben.

-- 
Ben Hutchings
Humans are not rational beings; they are rationalising beings.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: Double fault when single-stepping compat task with PREEMPT_RT

[Sebastian Andrzej Siewior]

On 09/25/2013 03:24 PM, Ben Hutchings wrote:
> On Tue, 2013-09-24 at 13:43 -0700, Brian Silverman wrote: [...]
>> I got down to a really simple program that reproduces this bug:
>> 
>> 
>> #include <sys/syscall.h> #include <unistd.h> int main() { // I've
>> tried SYS_getpid, SYS_write, and SYS_read here too. 
>> syscall(SYS_gettid); }

Brian, thank you for this excellent stripped down test case.
I think I know what is going on, will dig more later.

> Ben.
> 

Sebastian

[PATCH] Revert "x86: Disable IST stacks for debug/int 3/stack fault for PREEMPT_RT"

[Sebastian Andrzej Siewior]

where do I start. Let me explain what is going on here. The code
sequence
| pushf
| pop    %edx
| or     $0x1,%dh
| push   %edx
| mov    $0xe0,%eax
| popf
| sysenter

triggers the bug. On 64bit kernel we see the double fault (with 32bit and
64bit userland) and on 32bit kernel there is no problem. The reporter said
that double fault does not happen on 64bit kernel with 64bit userland and
this is because in that case the VDSO uses the "syscall" interface instead
of "sysenter".

The bug. "popf" loads the flags with the TF bit set which enables
"single stepping" and this leads to a debug exception. Usually on 64bit
we have a special IST stack for the debug exception. Due to patch [0] we
do not use the IST stack but the kernel stack instead. On 64bit the
sysenter instruction starts in kernel with the stack address NULL. The
code sequence above enters the debug exception (TF flag) after the
sysenter instruction was executed which sets the stack pointer to NULL
and we have a fault (it seems that the debug exception saves some bytes
on the stack).
To fix the double fault I'm going to drop patch [0]. It is completely
pointless. In do_debug() and do_stack_segment() we disable preemption
which means the task can't leave the CPU. So it does not matter if we run
on IST or on kernel stack.
There is a patch [1] which drops preempt_disable() call for a 32bit
kernel but not for 64bit so there should be no regression.
And [1] seems valid even for this code sequence. We enter the debug
exception with a 256bytes long per cpu stack and migrate to the kernel
stack before calling do_debug().

[0] x86-disable-debug-stack.patch
[1] fix-rt-int3-x86_32-3.2-rt.patch

Reported-by: Brian Silverman <bsilver16384@gmail.com>
Cc: Andi Kleen <andi@firstfloor.org>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/include/asm/page_64_types.h | 21 ++++++---------------
 arch/x86/kernel/cpu/common.c         |  2 --
 arch/x86/kernel/dumpstack_64.c       |  4 ----
 3 files changed, 6 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
index 695e04d..43dcd80 100644
--- a/arch/x86/include/asm/page_64_types.h
+++ b/arch/x86/include/asm/page_64_types.h
@@ -14,21 +14,12 @@
 #define IRQ_STACK_ORDER 2
 #define IRQ_STACK_SIZE (PAGE_SIZE << IRQ_STACK_ORDER)
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-# define STACKFAULT_STACK 0
-# define DOUBLEFAULT_STACK 1
-# define NMI_STACK 2
-# define DEBUG_STACK 0
-# define MCE_STACK 3
-# define N_EXCEPTION_STACKS 3  /* hw limit: 7 */
-#else
-# define STACKFAULT_STACK 1
-# define DOUBLEFAULT_STACK 2
-# define NMI_STACK 3
-# define DEBUG_STACK 4
-# define MCE_STACK 5
-# define N_EXCEPTION_STACKS 5  /* hw limit: 7 */
-#endif
+#define STACKFAULT_STACK 1
+#define DOUBLEFAULT_STACK 2
+#define NMI_STACK 3
+#define DEBUG_STACK 4
+#define MCE_STACK 5
+#define N_EXCEPTION_STACKS 5  /* hw limit: 7 */
 
 #define PUD_PAGE_SIZE		(_AC(1, UL) << PUD_SHIFT)
 #define PUD_PAGE_MASK		(~(PUD_PAGE_SIZE-1))
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index c0dcf06..2793d1f 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1105,9 +1105,7 @@ DEFINE_PER_CPU(struct task_struct *, fpu_owner_task);
  */
 static const unsigned int exception_stack_sizes[N_EXCEPTION_STACKS] = {
 	  [0 ... N_EXCEPTION_STACKS - 1]	= EXCEPTION_STKSZ,
-#if DEBUG_STACK > 0
 	  [DEBUG_STACK - 1]			= DEBUG_STKSZ
-#endif
 };
 
 static DEFINE_PER_CPU_PAGE_ALIGNED(char, exception_stacks
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 52b4bcd..addb207 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -21,14 +21,10 @@
 		(N_EXCEPTION_STACKS + DEBUG_STKSZ/EXCEPTION_STKSZ - 2)
 
 static char x86_stack_ids[][8] = {
-#if DEBUG_STACK > 0
 		[ DEBUG_STACK-1			]	= "#DB",
-#endif
 		[ NMI_STACK-1			]	= "NMI",
 		[ DOUBLEFAULT_STACK-1		]	= "#DF",
-#if STACKFAULT_STACK > 0
 		[ STACKFAULT_STACK-1		]	= "#SS",
-#endif
 		[ MCE_STACK-1			]	= "#MC",
 #if DEBUG_STKSZ > EXCEPTION_STKSZ
 		[ N_EXCEPTION_STACKS ...
-- 
1.8.5.2

Re: [PATCH] Revert "x86: Disable IST stacks for debug/int 3/stack fault for PREEMPT_RT"

[Andi Kleen]

On Fri, Jan 03, 2014 at 02:55:48PM +0100, Sebastian Andrzej Siewior wrote:
> where do I start. Let me explain what is going on here. The code
> sequence

Yes the IST stacks are needed for correctness, even in more cases than
the example below. You cannot just disable them, just because you don't
like them.

-Andi

Re: [PATCH] Revert "x86: Disable IST stacks for debug/int 3/stack fault for PREEMPT_RT"

[Mike Galbraith]

On Sat, 2014-01-04 at 19:18 +0100, Andi Kleen wrote: 
> On Fri, Jan 03, 2014 at 02:55:48PM +0100, Sebastian Andrzej Siewior wrote:
> > where do I start. Let me explain what is going on here. The code
> > sequence
> 
> Yes the IST stacks are needed for correctness, even in more cases than
> the example below. You cannot just disable them, just because you don't
> like them.

You had a better reason than dislike.

<quote> 
From: Andi Kleen <ak@suse.de>
Date: Fri, 3 Jul 2009 08:44:10 -0500
Subject: [PATCH 209/303] x86: Disable IST stacks for debug/int 3/stack fault for PREEMPT_RT

Normally the x86-64 trap handlers for debug/int 3/stack fault run
on a special interrupt stack to make them more robust
when dealing with kernel code.

The PREEMPT_RT kernel can sleep in locks even while allocating
GFP_ATOMIC memory. When one of these trap handlers needs to send
real time signals for ptrace it allocates memory and could then
try to to schedule.  But it is not allowed to schedule on a
IST stack. This can cause warnings and hangs.

This patch disables the IST stacks for these handlers for PREEMPT_RT
kernel. Instead let them run on the normal process stack.

...

A better solution would be to use similar logic as the NMI "paranoid"
path: check if signal is for user space, if yes go back to entry.S, switch stack,
call sync_regs, then do the signal sending etc.

</quote>

Or perhaps tell sleeping locks to spin in annoying spots?  I converted
rt spinlocks globally to preemptible spinning locks (wasn't pretty, but
worked), so seems that could work.

-Mike

Re: [PATCH] Revert "x86: Disable IST stacks for debug/int 3/stack fault for PREEMPT_RT"

[Mike Galbraith]

On Sun, 2014-01-05 at 05:45 +0100, Mike Galbraith wrote:

> Or perhaps tell sleeping locks to spin in annoying spots?  I converted
> rt spinlocks globally to preemptible spinning locks (wasn't pretty, but
> worked), so seems that could work.

Bah.  Nope, if lock owner is on your cpu, you have to yield.

Re: [PATCH] Revert "x86: Disable IST stacks for debug/int 3/stack fault for PREEMPT_RT"

[Andi Kleen]

On Sun, Jan 05, 2014 at 05:45:47AM +0100, Mike Galbraith wrote:
> On Sat, 2014-01-04 at 19:18 +0100, Andi Kleen wrote: 
> > On Fri, Jan 03, 2014 at 02:55:48PM +0100, Sebastian Andrzej Siewior wrote:
> > > where do I start. Let me explain what is going on here. The code
> > > sequence
> > 
> > Yes the IST stacks are needed for correctness, even in more cases than
> > the example below. You cannot just disable them, just because you don't
> > like them.
> 
> You had a better reason than dislike.

Ah true it was me. Good point. I forgot all about that.

Probably it needs some form of the NMI style paranoid_* switch.

-Andi

Re: [PATCH] Revert "x86: Disable IST stacks for debug/int 3/stack fault for PREEMPT_RT"

[Sebastian Andrzej Siewior]

* Andi Kleen | 2014-01-04 19:18:07 [+0100]:

>On Fri, Jan 03, 2014 at 02:55:48PM +0100, Sebastian Andrzej Siewior wrote:
>> where do I start. Let me explain what is going on here. The code
>> sequence
>
>Yes the IST stacks are needed for correctness, even in more cases than
>the example below. You cannot just disable them, just because you don't
>like them.

Andi, you were the Author of that patch.
I plan to migrate from the IST stack to the kernel stack so I can enable
preemption. This is he case on 32bit. You mention more cases than this.
Could you please give me some examples so I can consider them?

>-Andi

Sebastian