Handle IST interrupts from userspace on the normal stack

PATCH Superseded Andy Lutomirski 2 participants 7 messages Nov 13, 2014 – Nov 14, 2014

Subsystems (4)

Documentation x86 perf events uprobes

lore

Share & tools

lore

Andy Lutomirski

author

2014-11-13 22:31 UTC #0 lore raw

v2 cover Handle IST interrupts from userspace on the normal stack

We currently run IST interrupt handlers on the IST stack. Changing it may simplify a few things. See patch 2 for details.

Patch 1 is a fix for a not-quite-bug in uprobes that Oleg noticed that would be exposed by patch 2.

Andy Lutomirski (2):
x86, entry: Switch stacks on a paranoid entry from userspace uprobes, x86: Fix _TIF_UPROBE vs _TIF_NOTIFY_RESUME

 Documentation/x86/entry_64.txt         | 18 +++++---
 Documentation/x86/x86_64/kernel-stacks |  8 ++--
 arch/x86/include/asm/thread_info.h     |  2 +-
 arch/x86/kernel/entry_64.S             | 82 +++++++++++++++++-----------------
 arch/x86/kernel/traps.c                | 23 +++-------
 kernel/events/uprobes.c                |  1 -
 6 files changed, 65 insertions(+), 69 deletions(-)

Signature · 1 line

1.9.3

Andy Lutomirski

author

2014-11-13 22:31 UTC #1 lore raw

PATCH v2 1/2 x86, entry: Switch stacks on a paranoid entry from userspace

This causes all non-NMI kernel entries from userspace to run on the normal kernel stack.

This is, suprisingly, simpler and shorter than the current code. I removes the IMO rather frightening paranoid_userspace path, and it make sync_regs much simpler.

There is no risk of stack overflow due to this change -- the kernel stack that we switch to is empty.

This may enable the machine check code to be simplified at some point, because a machine check from userspace will be able to safely enable interrupts. It will also allow the upcoming fsgsbase code to be simplified, because it doesn't need to worry about usergs when scheduling in paranoid_exit, as that code no longer exists.

Cc Oleg Nesterov <[email protected]>

Cc Andi Kleen <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 Documentation/x86/entry_64.txt         | 18 +++++---
 Documentation/x86/x86_64/kernel-stacks |  8 ++--
 arch/x86/kernel/entry_64.S             | 82 +++++++++++++++++-----------------
 arch/x86/kernel/traps.c                | 23 +++-------
 4 files changed, 64 insertions(+), 67 deletions(-)

Documentation/x86/entry_64.txt +12 -6

index bc7226ef5055..8e53217e6d40 100644
@@ -75,9 +75,6 @@ The expensive (paranoid) way is to read back the MSR_GS_BASE value
 	xorl %ebx,%ebx
 1:	ret
 
-and the whole paranoid non-paranoid macro complexity is about whether
-to suffer that RDMSR cost.
-
 If we are at an interrupt or user-trap/gate-alike boundary then we can
 use the faster check: the stack will be a reliable indicator of
 whether SWAPGS was already done: if we see that we are a secondary

@@ -90,6 +87,15 @@ which might have triggered right after a normal entry wrote CS to the
 stack but before we executed SWAPGS, then the only safe way to check
 for GS is the slower method: the RDMSR.
 
-So we try only to mark those entry methods 'paranoid' that absolutely
-need the more expensive check for the GS base - and we generate all
-'normal' entry points with the regular (faster) entry macros.
+Therefore, super-atomic entries (except NMI, which is handled separately)
+must use idtentry with paranoid=1 to handle gsbase correctly.  This
+triggers three main behavior changes:
+
+ - Interrupt entry will use the slower gsbase check.
+ - Interrupt entry from user mode will switch off the IST stack.
+ - Interrupt exit to kernel mode will not attempt to reschedule.
+
+We try to only use IST entries and the paranoid entry code for vectors
+that absolutely need the more expensive check for the GS base - and we
+generate all 'normal' entry points with the regular (faster) paranoid=0
+variant.

Documentation/x86/x86_64/kernel-stacks +5 -3

index a01eec5d1d0b..e3c8a49d1a2f 100644
@@ -40,9 +40,11 @@ An IST is selected by a non-zero value in the IST field of an
 interrupt-gate descriptor.  When an interrupt occurs and the hardware
 loads such a descriptor, the hardware automatically sets the new stack
 pointer based on the IST value, then invokes the interrupt handler.  If
-software wants to allow nested IST interrupts then the handler must
-adjust the IST values on entry to and exit from the interrupt handler.
-(This is occasionally done, e.g. for debug exceptions.)
+the interrupt came from user mode, then the interrupt handler prologue
+will switch back to the per-thread stack.  If software wants to allow
+nested IST interrupts then the handler must adjust the IST values on
+entry to and exit from the interrupt handler.  (This is occasionally
+done, e.g. for debug exceptions.)
 
 Events with different IST codes (i.e. with different stacks) can be
 nested.  For example, a debug interrupt can safely be interrupted by an

arch/x86/kernel/entry_64.S +42 ~~-40~~

113 lines not on this page · raw message

arch/x86/kernel/traps.c +5 ~~-18~~

index 0d0e922fafc1..837843513ac3 100644
@@ -375,27 +375,14 @@ NOKPROBE_SYMBOL(do_int3);
 
 #ifdef CONFIG_X86_64
 /*
- * Help handler running on IST stack to switch back to user stack
- * for scheduling or signal handling. The actual stack switch is done in
- * entry.S
+ * Help handler running on IST stack to switch off the IST stack if the
+ * interrupted code was in user mode. The actual stack switch is done in
+ * entry_64.S
  */
 asmlinkage __visible struct pt_regs *sync_regs(struct pt_regs *eregs)
 {
-	struct pt_regs *regs = eregs;
-	/* Did already sync */
-	if (eregs == (struct pt_regs *)eregs->sp)
-		;
-	/* Exception from user space */
-	else if (user_mode(eregs))
-		regs = task_pt_regs(current);
-	/*
-	 * Exception from kernel and interrupts are enabled. Move to
-	 * kernel process stack.
-	 */
-	else if (eregs->flags & X86_EFLAGS_IF)
-		regs = (struct pt_regs *)(eregs->sp -= sizeof(struct pt_regs));
-	if (eregs != regs)
-		*regs = *eregs;
+	struct pt_regs *regs = task_pt_regs(current);
+	*regs = *eregs;
 	return regs;
 }
 NOKPROBE_SYMBOL(sync_regs);

Signature · 1 line

1.9.3

Andy Lutomirski

author

2014-11-13 22:31 UTC #2 lore raw

PATCH v2 2/2 uprobes, x86: Fix _TIF_UPROBE vs _TIF_NOTIFY_RESUME

In reply to Andy Lutomirski

x86 call do_notify_resume on paranoid returns if TIF_UPROBE is set but not on non-paranoid returns. I suspect that this is a mistake and that the code only works because int3 is paranoid.

Setting _TIF_NOTIFY_RESUME in the uprobe code was probably a workaround for the x86 bug. With that bug fixed, we can remove _TIF_NOTIFY_RESUME from the uprobes code.

Cc Srikar Dronamraju <[email protected]>

Reported-by Oleg Nesterov <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/thread_info.h | 2 +-
 kernel/events/uprobes.c            | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

arch/x86/include/asm/thread_info.h +1 -1

index 854053889d4d..547e344a6dc6 100644
@@ -141,7 +141,7 @@ struct thread_info {
 /* Only used for 64 bit */
 #define _TIF_DO_NOTIFY_MASK						\
 	(_TIF_SIGPENDING | _TIF_MCE_NOTIFY | _TIF_NOTIFY_RESUME |	\
-	 _TIF_USER_RETURN_NOTIFY)
+	 _TIF_USER_RETURN_NOTIFY | _TIF_UPROBE)
 
 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW							\

kernel/events/uprobes.c +0 -1

index 1d0af8a2c646..ed8f2cde34c5 100644
@@ -1640,7 +1640,6 @@ bool uprobe_deny_signal(void)
 		if (__fatal_signal_pending(t) || arch_uprobe_xol_was_trapped(t)) {
 			utask->state = UTASK_SSTEP_TRAPPED;
 			set_tsk_thread_flag(t, TIF_UPROBE);
-			set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
 		}
 	}

Signature · 1 line

1.9.3

Srikar Dronamraju

2014-11-14 06:09 UTC #3 lore raw

Re: [PATCH v2 2/2] uprobes, x86: Fix _TIF_UPROBE vs _TIF_NOTIFY_RESUME

Quoted 22 lines · Andy Lutomirski

x86 call do_notify_resume on paranoid returns if TIF_UPROBE is set but not on non-paranoid returns. I suspect that this is a mistake and that the code only works because int3 is paranoid.
Setting _TIF_NOTIFY_RESUME in the uprobe code was probably a workaround for the x86 bug. With that bug fixed, we can remove
_TIF_NOTIFY_RESUME from the uprobes code.
Cc Srikar Dronamraju <[email protected]>
Reported-by Oleg Nesterov <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 2 +-
 kernel/events/uprobes.c            | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)
arch/x86/include/asm/thread_info.h +0 -0
index 854053889d4d..547e344a6dc6 100644

arch/x86/include/asm/thread_info.h struct thread_info { · line 144

Quoted 5 lines

arch/x86/include/asm/thread_info.h +1 -1

@@ -141,7 +141,7 @@ struct thread_info {
 /* Only used for 64 bit */
 #define _TIF_DO_NOTIFY_MASK						\
 	(_TIF_SIGPENDING | _TIF_MCE_NOTIFY | _TIF_NOTIFY_RESUME |	\
-	 _TIF_USER_RETURN_NOTIFY)
+	 _TIF_USER_RETURN_NOTIFY | _TIF_UPROBE)

The comment above says only for 64 bit. So would this still work for i386?

Quoted 18 lines

arch/x86/include/asm/thread_info.h +0 -0

 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW							\

kernel/events/uprobes.c +0 -1

index 1d0af8a2c646..ed8f2cde34c5 100644
@@ -1640,7 +1640,6 @@ bool uprobe_deny_signal(void)
 		if (__fatal_signal_pending(t) || arch_uprobe_xol_was_trapped(t)) {
 			utask->state = UTASK_SSTEP_TRAPPED;
 			set_tsk_thread_flag(t, TIF_UPROBE);
-			set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
 		}
 	}

Signature · 1 line

1.9.3

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Andy Lutomirski

author

2014-11-14 07:01 UTC #4 lore raw

Re: [PATCH v2 2/2] uprobes, x86: Fix _TIF_UPROBE vs _TIF_NOTIFY_RESUME

Quoted 19 lines · Srikar Dronamraju

Quoted 17 lines · Andy Lutomirski
x86 call do_notify_resume on paranoid returns if TIF_UPROBE is set but not on non-paranoid returns. I suspect that this is a mistake and that the code only works because int3 is paranoid.
Setting _TIF_NOTIFY_RESUME in the uprobe code was probably a workaround for the x86 bug. With that bug fixed, we can remove
_TIF_NOTIFY_RESUME from the uprobes code.
Cc Srikar Dronamraju <[email protected]>
Reported-by Oleg Nesterov <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 2 +-
 kernel/events/uprobes.c            | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

Quoted 15 lines

arch/x86/include/asm/thread_info.h struct thread_info { · line 144
Quoted 10 lines
arch/x86/include/asm/thread_info.h +1 -1
index 854053889d4d..547e344a6dc6 100644
@@ -141,7 +141,7 @@ struct thread_info {
 /* Only used for 64 bit */
 #define _TIF_DO_NOTIFY_MASK                                          \
      (_TIF_SIGPENDING | _TIF_MCE_NOTIFY | _TIF_NOTIFY_RESUME |       \
-      _TIF_USER_RETURN_NOTIFY)
+      _TIF_USER_RETURN_NOTIFY | _TIF_UPROBE)
The comment above says only for 64 bit. So would this still work for i386?

i386 seems to look at _TIF_WORK_MASK (which includes _TIF_UPROBE) for everything except syscalls and at _TIF_WORK_SYSCALL_EXIT for syscall return (which does not include _TIF_UPROBE). Is that okay?

--Andy

Quoted 23 lines

Quoted 16 lines

arch/x86/include/asm/thread_info.h +0 -0

 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW                                                      \

kernel/events/uprobes.c +0 -2

index 1d0af8a2c646..ed8f2cde34c5 100644
@@ -1640,7 +1640,6 @@ bool uprobe_deny_signal(void)
              if (__fatal_signal_pending(t) || arch_uprobe_xol_was_trapped(t)) {
                      utask->state = UTASK_SSTEP_TRAPPED;
                      set_tsk_thread_flag(t, TIF_UPROBE);
-                     set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
              }
      }
 
--

Quoted 2 lines

1.9.3

--
Thanks and Regards
Srikar Dronamraju

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Srikar Dronamraju

Acked-by

2014-11-14 07:16 UTC #5 lore raw

Re: [PATCH v2 2/2] uprobes, x86: Fix _TIF_UPROBE vs _TIF_NOTIFY_RESUME

Quoted 37 lines · Andy Lutomirski

Quoted 19 lines · Srikar Dronamraju
Quoted 17 lines · Andy Lutomirski
x86 call do_notify_resume on paranoid returns if TIF_UPROBE is set but not on non-paranoid returns. I suspect that this is a mistake and that the code only works because int3 is paranoid.
Setting _TIF_NOTIFY_RESUME in the uprobe code was probably a workaround for the x86 bug. With that bug fixed, we can remove
_TIF_NOTIFY_RESUME from the uprobes code.
Cc Srikar Dronamraju <[email protected]>
Reported-by Oleg Nesterov <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/thread_info.h | 2 +-
 kernel/events/uprobes.c            | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)
Quoted 15 lines
arch/x86/include/asm/thread_info.h struct thread_info { · line 144
Quoted 10 lines
arch/x86/include/asm/thread_info.h +1 -1
index 854053889d4d..547e344a6dc6 100644
@@ -141,7 +141,7 @@ struct thread_info {
 /* Only used for 64 bit */
 #define _TIF_DO_NOTIFY_MASK                                          \
      (_TIF_SIGPENDING | _TIF_MCE_NOTIFY | _TIF_NOTIFY_RESUME |       \
-      _TIF_USER_RETURN_NOTIFY)
+      _TIF_USER_RETURN_NOTIFY | _TIF_UPROBE)
The comment above says only for 64 bit. So would this still work for i386?

Quoted 4 lines

i386 seems to look at _TIF_WORK_MASK (which includes _TIF_UPROBE) for everything except syscalls and at _TIF_WORK_SYSCALL_EXIT for syscall return (which does not include _TIF_UPROBE). Is that okay?

Ok.. That expains (please add my ack to your v3)

Acked-by Srikar Dronamraju <[email protected]>

Quoted 32 lines

--Andy

Quoted 23 lines

Quoted 16 lines

arch/x86/include/asm/thread_info.h +0 -0

 /* flags to check in __switch_to() */
 #define _TIF_WORK_CTXSW                                                      \

kernel/events/uprobes.c +0 -2

index 1d0af8a2c646..ed8f2cde34c5 100644
@@ -1640,7 +1640,6 @@ bool uprobe_deny_signal(void)
              if (__fatal_signal_pending(t) || arch_uprobe_xol_was_trapped(t)) {
                      utask->state = UTASK_SSTEP_TRAPPED;
                      set_tsk_thread_flag(t, TIF_UPROBE);
-                     set_tsk_thread_flag(t, TIF_NOTIFY_RESUME);
              }
      }
 
--

Quoted 2 lines

1.9.3

--
Thanks and Regards
Srikar Dronamraju

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Signature · 2 lines

Thanks and Regards
Srikar Dronamraju

Andy Lutomirski

author

2014-11-13 23:19 UTC #6 lore raw

In reply to Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently run IST interrupt handlers on the IST stack. Changing it may simplify a few things. See patch 2 for details.
Patch 1 is a fix for a not-quite-bug in uprobes that Oleg noticed that would be exposed by patch 2.
Andy Lutomirski (2):
x86, entry: Switch stacks on a paranoid entry from userspace uprobes, x86: Fix _TIF_UPROBE vs _TIF_NOTIFY_RESUME

I reversed the order of the patches -- oops. If I send v3, I'll fix that.

--Andy

Quoted 11 lines

 Documentation/x86/entry_64.txt         | 18 +++++---
 Documentation/x86/x86_64/kernel-stacks |  8 ++--
 arch/x86/include/asm/thread_info.h     |  2 +-
 arch/x86/kernel/entry_64.S             | 82 +++++++++++++++++-----------------
 arch/x86/kernel/traps.c                | 23 +++-------
 kernel/events/uprobes.c                |  1 -
 6 files changed, 65 insertions(+), 69 deletions(-)

--
1.9.3

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC