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.

NB: Tony has seen odd behavior when stress-testing injected machine checks with this series applied. I suspect that it's a bug in something else, possibly his BIOS. Bugs in this series shouldn't be ruled out, though.

Andy Lutomirski (3):
uprobes, x86: Fix _TIF_UPROBE vs _TIF_NOTIFY_RESUME x86, entry: Switch stacks on a paranoid entry from userspace sched, x86: Check that we're on the right stack in schedule and
__might_sleep

 Documentation/x86/entry_64.txt         | 18 ++++---
 Documentation/x86/x86_64/kernel-stacks |  8 ++--
 arch/x86/Kconfig                       |  1 +
 arch/x86/include/asm/thread_info.h     | 19 +++++++-
 arch/x86/kernel/entry_64.S             | 86 ++++++++++++++++++----------------
 arch/x86/kernel/irq_32.c               | 13 ++---
 arch/x86/kernel/traps.c                | 23 ++-------
 include/linux/thread_info.h            |  7 +++
 kernel/Kconfig.locks                   |  3 ++
 kernel/events/uprobes.c                |  1 -
 kernel/sched/core.c                    | 14 ++++--
 11 files changed, 109 insertions(+), 84 deletions(-)

1.9.3

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.

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

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							\

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);
 		}
 	}
 

1.9.3

This causes all non-NMI, non-double-fault kernel entries from userspace to run on the normal kernel stack. Double-fault is exempt to minimize confusion if we double-fault directly from userspace due to a bad kernel stack.

This is, suprisingly, simpler and shorter than the current code. It 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.

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

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.

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

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);

1.9.3

On x86, sleeping while on an IST or irq stack has a surprisingly good chance of working, but it can also fail dramatically. Add an arch hook to allow schedule and __might_sleep to catch sleeping on the wrong stack.

This will also catch do_exit from a funny stack, which could leave an IST stack shifted or an NMI nesting count incremented.

---
 arch/x86/Kconfig                   |  1 +
 arch/x86/include/asm/thread_info.h | 17 +++++++++++++++++
 arch/x86/kernel/irq_32.c           | 13 +++----------
 include/linux/thread_info.h        |  7 +++++++
 kernel/Kconfig.locks               |  3 +++
 kernel/sched/core.c                | 14 ++++++++++----
 6 files changed, 41 insertions(+), 14 deletions(-)

index ded8a6774ac9..a811286636d2 100644
@@ -137,6 +137,7 @@ config X86
 	select HAVE_ACPI_APEI_NMI if ACPI
 	select ACPI_LEGACY_TABLES_LOOKUP if ACPI
 	select X86_FEATURE_NAMES if PROC_FS
+	select HAVE_ARCH_SCHEDULE_ALLOWED
 
 config INSTRUCTION_DECODER
 	def_bool y

index 547e344a6dc6..05701f132473 100644
@@ -170,6 +170,23 @@ static inline struct thread_info *current_thread_info(void)
 	return ti;
 }
 
+static inline unsigned long current_stack_pointer(void)
+{
+	unsigned long sp;
+#ifdef CONFIG_X86_64
+	asm("mov %%rsp,%0" : "=g" (sp));
+#else
+	asm("mov %%esp,%0" : "=g" (sp));
+#endif
+	return sp;
+}
+
+static inline bool arch_schedule_allowed(void)
+{
+	return ((current_stack_pointer() ^ this_cpu_read_stable(kernel_stack))
+		& ~(THREAD_SIZE - 1)) == 0;
+}
+
 #else /* !__ASSEMBLY__ */
 
 /* how to get the thread information struct from ASM */

index 63ce838e5a54..28d28f5eb8f4 100644
@@ -69,16 +69,9 @@ static void call_on_stack(void *func, void *stack)
 		     : "memory", "cc", "edx", "ecx", "eax");
 }
 
-/* how to get the current stack pointer from C */
-#define current_stack_pointer ({		\
-	unsigned long sp;			\
-	asm("mov %%esp,%0" : "=g" (sp));	\
-	sp;					\
-})
-
 static inline void *current_stack(void)
 {
-	return (void *)(current_stack_pointer & ~(THREAD_SIZE - 1));
+	return (void *)(current_stack_pointer() & ~(THREAD_SIZE - 1));
 }
 
 static inline int

@@ -103,7 +96,7 @@ execute_on_irq_stack(int overflow, struct irq_desc *desc, int irq)
 
 	/* Save the next esp at the bottom of the stack */
 	prev_esp = (u32 *)irqstk;
-	*prev_esp = current_stack_pointer;
+	*prev_esp = current_stack_pointer();
 
 	if (unlikely(overflow))
 		call_on_stack(print_stack_overflow, isp);

@@ -156,7 +149,7 @@ void do_softirq_own_stack(void)
 
 	/* Push the previous esp onto the stack */
 	prev_esp = (u32 *)irqstk;
-	*prev_esp = current_stack_pointer;
+	*prev_esp = current_stack_pointer();
 
 	call_on_stack(__do_softirq, isp);
 }

index ff307b548ed3..6deaf7e97009 100644
@@ -145,6 +145,13 @@ static inline bool test_and_clear_restore_sigmask(void)
 #error "no set_restore_sigmask() provided and default one won't work"
 #endif
 
+#ifndef CONFIG_HAVE_ARCH_SCHEDULE_ALLOWED
+static inline bool arch_schedule_allowed(void)
+{
+	return true;
+}
+#endif
+
 #endif	/* __KERNEL__ */
 
 #endif /* _LINUX_THREAD_INFO_H */

index 76768ee812b2..2714dc34695a 100644
@@ -237,3 +237,6 @@ config ARCH_USE_QUEUE_RWLOCK
 config QUEUE_RWLOCK
 	def_bool y if ARCH_USE_QUEUE_RWLOCK
 	depends on SMP
+
+config HAVE_ARCH_SCHEDULE_ALLOWED
+       bool
\ No newline at end of file

index 240157c13ddc..e51ab65a9750 100644
@@ -2705,8 +2705,12 @@ static inline void schedule_debug(struct task_struct *prev)
 	 * Test if we are atomic. Since do_exit() needs to call into
 	 * schedule() atomically, we ignore that path. Otherwise whine
 	 * if we are scheduling when we should not.
+	 *
+	 * If architectural conditions for scheduling are not met,
+	 * complain even if we are in do_exit.
 	 */
-	if (unlikely(in_atomic_preempt_off() && prev->state != TASK_DEAD))
+	if (unlikely((in_atomic_preempt_off() && prev->state != TASK_DEAD) ||
+		     !arch_schedule_allowed()))
 		__schedule_bug(prev);
 	rcu_sleep_check();
 

@@ -7200,10 +7204,12 @@ static inline int preempt_count_equals(int preempt_offset)
 void __might_sleep(const char *file, int line, int preempt_offset)
 {
 	static unsigned long prev_jiffy;	/* ratelimiting */
+	bool arch_ok;
 
 	rcu_sleep_check(); /* WARN_ON_ONCE() by default, no rate limit reqd. */
+	arch_ok = arch_schedule_allowed();
 	if ((preempt_count_equals(preempt_offset) && !irqs_disabled() &&
-	     !is_idle_task(current)) ||
+	     !is_idle_task(current) && arch_ok) ||
 	    system_state != SYSTEM_RUNNING || oops_in_progress)
 		return;
 	if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)

@@ -7214,8 +7220,8 @@ void __might_sleep(const char *file, int line, int preempt_offset)
 		"BUG: sleeping function called from invalid context at %s:%d\n",
 			file, line);
 	printk(KERN_ERR
-		"in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
-			in_atomic(), irqs_disabled(),
+		"in_atomic(): %d, irqs_disabled(): %d, arch_schedule_allowed: %d, pid: %d, name: %s\n",
+			in_atomic(), irqs_disabled(), (int)arch_ok,
 			current->pid, current->comm);
 
 	debug_show_held_locks(current);

1.9.3

Why doesn't the normal in_interrupt() test catch this?

Linus

It catches scheduling on an irq stack, assuming that all of the irq_count stuff is working correctly. I don't think it catches sleeping on an IST stack.

--Andy

Andy Lutomirski
AMA Capital Management, LLC

The exception handlers which use the IST stacks don't necessarily set irq count. Maybe they should.

-Andi

[email protected] -- Speaking for myself only.

Hmm. I think they should. Since they clearly must not schedule, as they use a percpu stack.

Which exceptions use IST?

[ grep grep ]

Looks like stack, doublefault, nmi, debug and mce. And yes, I really think they should all raise the irq count if they don't already. Rather than add random arch-specific "let's check that we're on the right stack" code to the might-sleep stuff, just use the one we have.

Linus

Does that include nmi? I'm a bit afraid of touching that code.

It's certainly easy enough to bump irq_count in the paranoid entries.

--Andy

Andy Lutomirski
AMA Capital Management, LLC

NMI is kind of special, since it's really not supposed to touch 'current' or anything like that, and that's how we do preempt-count (and that's where irq-count is) right now.

I would prefer to have preempt_count be a percpu variable rather than a per-thread one, but there are historical reasons for that horror. Oh well.

It definitely shouldn't be done from the assembly code. Usually it's "irq_enter/exit()" that does it, but for NMI you'd presumably just do it explicitly from do_nmi() or similar. But NMI really is nastier than other cases, see above.

(That said, I thought we did it anyway, but now that I look I can't find it. So I was probably just smoking something)

Linus

preempt_count is a per cpu variable at least on x86 since:

commit c2daa3bed53a 'sched, x86: Provide a per-cpu preempt_count implementation'

do_nmi()
nmi_enter()

    preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET); 

Certainly something you should not smoke again :)

Thanks,

tglx

Oh, goodie. I hadn't tracked that it went in, I just remembered the discussion. I should have looked.

And apparently we do it in nmi_enter too, so NMI is already ok. Don't know about the others.

Linus

My only real objection is that it's going to be ugly and error prone. It'll have to be something like:

if (!user_mode_vm(regs))

    irq_enter();

...

if (!user_mode_vm(regs))

    irq_exit();

because the whole point of this series is to make the IST entries not be atomic when they come from userspace. We can wrap this in an inline cond_ist_enter/cond_ist_exit or whatever, but still, blech.

Tony already sent the follow-up patch that will make do_machine_check enable interrupts if it came from userspace.

--Andy

Andy Lutomirski
AMA Capital Management, LLC

No.

Andy, you need to lay off the drugs.

NMI absolutely *has* to be atomic. The whole "oh, there's a per-core NMI flag and it disables all other NMI's and interrupts" kind of enforces that.

Trust me. Talking about being able to preempt the NMI handler is just crazy talk.

Linus

No drugs, just imprecision. This series doesn't change NMI handling at all. It only changes machine_check int3, debug, and stack_segment. (Why is #SS using IST stacks anyway?)

So my point stands: if machine_check is going to be conditionally atomic, then that condition needs to be expressed somewhere. And machine_check really does need to sleep, and it does so today. It's just that, in current kernels, it pretends to be atomic, but then it fakes everyone out before returning (look for sync_regs in entry_64.S) and becomes non-atomic part-way through, mediated by a special TIF flag. I think that the current conditional sync_regs thing is even crazier than switching stacks depending on regs->cs, so I'm trying to reduce craziness.

With this series applied, machine_check is honest: if it came from user space, it doesn't even pretend to be atomic.

And I'm not even making these things preemptable by default. They still run with irqs off. They're just allowed to turn irqs off *if user_mode_vm(regs)* if they want.

I engage in crazy talk all the time, but I'm not *that* crazy.

--Andy

.. ok, we were talking about adding an explicit preemption count to nmi, and then you wanted to make that conditional, that kind of freaked me out.

I'd still prefer to keep that knowledge in one place, rather than adding *another* completely ad-hoc thing in addition to what we already have.

Also, I really don't think it should be about the particular stack you're using. Sure, if a debug fault happens in user space, the fault handler could sleep if it runs on the regular stack, but our "might_sleep()" are about catching things that *could* be problematic, even if the sleep never happens. And so, might_sleep() _should_ actually trigger, even if it's not using the IST stack, because *if* the debug exception happened in kernel space, then we should warn.

So I'd actually *prefer* to have special hacks that perhaps then "undo" the preemption count if the code expressly tests for "did this happen in user space, then I know I'm safe". But then it's an *explicit* thing, not something that just magically works because nobody even thought about it, and the trap happened in user space.

See the argument? I'd *rather* see code like

/* Magic */
if (user_mode(regs)) {
.. verify that we're using the normal kernel stack .. enable interrupts, enable preemption
.. this is the explicit special case and it is aware .. of being special
}

even if on the face of it it looks hacky. But an *explicit* hack is preferable to something that just "happens" to work only for the user-mode case.

Linus

I guess I jumped around in the conversation a bit...

So we'd do, in do_machine_check:

irq_enter();

do atomic stuff;

ist_stop_being_atomic(regs);
local_irq_enable();
...
local_irq_disable();
ist_start_being_atomic_again();

irq_exit();

and we'd have something like:

void ist_stop_being_atomic(struct pt_regs *regs)
{
BUG_ON(!user_mode_vm(regs));
--irq_count;
}

I'm very hesitant to use irq_enter for this, though. I think we want just the irq_count part. Maybe ist_enter() and ist_exit()? I think that we really don't want to go anywhere near the accounting stuff in irq_enter from an IST handler if !user_mode_vm(regs). Doing it from asm is somewhat less error prone, although I guess we already rely on the IDT entries themselves being in sync with the paranoid idtentry setting.

--Andy

Oh, I agree. I mentioned irq_enter/exit() just as the place the preempt count increment normally happens. I have nothing against doing it manually for these things that really aren't normal interrupts.

Linus

It may need to be considerably more complicated to keep RCU happy, though. In fact, I think that this code is already wrong.

I'll write something and cc Paul to get some feedback.

--Andy

Well, why would I want to go atomic again? We just do the minimally needed atomic stuff, irq_exit() and then do the rest.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Because ist_exit will get confused otherwise, and you still need to call something on the way out for context tracking.

Yes, I could rearrange it a bit. Will ponder.

--Andy

We made most of those preemptible in -rt and changed it away from the IST. I never got a good explanation from anyone for why they were IST in the first place - histeric accident or such.

Feel free to clean this up too!

Thanks,

Ingo

Resurrecting an old thread:

The outcome of this discussion was that ist_enter now raises HARDIRQ_COUNT. I think this is causing a problem. If a user program enables TF, it generates a bunch of debug exceptions. The handlers raise the IRQ count and do stuff, and apparently some of that stuff can raise a softirq. (I have no idea where the softirq is being raised.) The softirq code notices that we're in_interrupt and doesn't wake ksoftirqd because it thinks we're about to exit the interrupt and process the softirq. But we don't, which causes occasional warnings and confuses things (and me!).

So how do we fix it? If we stop raising HARDIRQ_COUNT (and apply $SUBJECT?), then raise_softirq will wake ksoftirqd and life is good. But this seems a bit silly, since, if we entered the ist exception handler from a context with irqs on and softirqs enabled, we *could* plausibly handle the softirq right away -- we're on an essentially empty stack. (Of course, it's a *small* stack, since it could be the IST stack.)

Or we could just let ksoftirqd do its thing and stop raising HARDIRQ_COUNT. We could add a new preempt count field just for IST (yuck). We could try to hijack a different preempt count field (NMI?). But I kind of like the idea of just reinstating the original patch of explicitly checking that we're on a safe stack in schedule and __might_sleep, since that is the actual condition we care about.

--Andy

Ping? I can still trigger this fairly easily on 4.6.

--Andy

.. I haven't seen a patch from you, last I saw that was kind of what I expected.

That said, I still despise your patch. Why can't you just fix "in_interrupt()" and be done with it. The original patch was like 50 lines of changes for somethinig that feels like it should be a one-liner.

And no, we don't add idiotic new config symbols for things like "I have this one-liner trivial arch helper". What we do is to just test for such a helper with "#ifdef" (and if it's a inline function we do #define xyz xyz" so that the #ifdef works).

So the original patch in this thread is still off the table, especially since there was absolutely no explanation for why it should be such a crazy complicated thing.

What exactly is it you are nervous about scheduling in NMI's? I agree that that would be disastrous, but it's not supposed to actually happen.

Linus

It's not the NMIs I'm worried about -- they do crazy stuff, but it's self-contained crazy stuff, and NMIs can *never* schedule, so they're unlikely to have issues here. It's the things that are a bit more ambiguous. For example, MCE handlers sometimes do schedule, and we allow it if they use the right helpers and check the right conditions. The original patch lets us print a big fat warning if they screw it up.

I can't modify in_interrupt for the same reason that the current code is broken: if in_interrupt() returns true, that's a promise that we'll call invoke_softirq via irq_exit in a timely manner. Doing this from a weird ultra-atomic context that interrupted kernel code that had IF=0 would be bad. IOW, the whole in_interrupt() mechanism was designed until the entirely reasonably assumption that interrupts only happen when interrupts are on. These special interrupt-like things (NMI, MCE, debug) can happen asynchronously with interrupts *off*, and the result is a mess.

What about this silly fix? (Pardon the probable whitespace damage.)

commit b3e89c652bdf6a7d5a23b094ae921f193e62c534

Author: Andy Lutomirski <[email protected]>
Date:   Mon May 23 19:07:21 2016 -0700

x86/traps: Don't for in_interrupt() to return true in IST handlers

Forcing in_interrupt() to return true if we're not in a bona fide interrupt confuses the softirq code.

IST context")

index d1590486204a..9c1d948d5d8b 100644
@@ -96,6 +96,19 @@ static inline void cond_local_irq_disable(struct

pt_regs *regs)

         local_irq_disable();

}

+/*
+ * We want to cause in_atomic() to return true while in an IST handler
+ * so that attempts to schedule will warn. We also want to detect buggy
+ * code that does preempt_enable(); preempt_disable() in an IST handler,
+ * so we want in_atomic to still return true if that happens.
+ *
+ * We cannot add use HARDIRQ_OFFSET or otherwise cause in_interrupt() to
+ * return true: the softirq code assumes that in_interrupt() only
+ * returns true if we will soon execute softirqs, and we can't do that
+ * if an IST entry interrupts kernel code with interrupts disabled.
+ */
+#define IST_OFFSET (3 * PREEMPT_OFFSET)
+
void ist_enter(struct pt_regs *regs)

 {
     if (user_mode(regs)) {

@@ -116,7 +129,7 @@ void ist_enter(struct pt_regs *regs)
* on x86_64 and entered from user mode, in which case we're
* still atomic unless ist_begin_non_atomic is called. */
- preempt_count_add(HARDIRQ_OFFSET);
+ preempt_count_add(IST_OFFSET);

     /* This code is a bit fragile.  Test it. */
     RCU_LOCKDEP_WARN(!rcu_is_watching(), "ist_enter didn't work");

@@ -124,7 +137,7 @@ void ist_enter(struct pt_regs *regs)

void ist_exit(struct pt_regs *regs)
{
- preempt_count_sub(HARDIRQ_OFFSET);
+ preempt_count_sub(IST_OFFSET);

     if (!user_mode(regs))
         rcu_nmi_exit();

That looks fine to me, and has a reason for it.

That said, I'm not convinced about the preempt_enable/preempt_disable
excuse: that would be horribly buggy anyway, and has nothing to do with IST's. Why protect against random insane bugs?

So the "3*" certainly doesn't hurt, but and I won't argue more against it, but it looks pretty magical and made-up. Might as well just add/subtract one. You can never protect against people who intentionally write buggy code, and if it's protecting against unintentional buggy code I don't see why the IST case is so special when it's just a tiny fraction of code..

Linus

v3 did 3.5x better than earlier ones ... survived overnight but died at 91724 injection/consumption/recovery cycles just now. Different symptom, instead of losing some cpus, there was a fatal machine check (PCC=1 and OVER=1 bits set in the machine check bank). This might be from a known issue.
Not sure if this was due to some improvement in the code, or because I changed the system configuration by pulling out all the memory except for that on memory controller 0 on node 0. Our BIOS team had told me they'd seen some instability in the injection code on fully populated systems.

I did instrument the synchronization in mce_start(). I was a bit worried that with ever increasing numbers of cpus the 100ns delay between pounding on atomic ops on mce_callin might not be enough. But it seems we are not in trouble yet. Slowest synchronization recorded took 1.8M TSC cycles. Mean is 500K cycles. So my gut feeling that the one second timeout was very conservative is correct.

-Tony

We now switch to the kernel stack when a machine check interrupts during user mode. This means that we can perform recovery actions in the tail of do_machine_check(). So say goodbye to TIF_MCE_NOTIFY, mce_save_info(), mce_find_info() and mce_notify_process()

---

Obviously this is dependent on Andy's patch series in
git://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git x86/paranoid

 arch/x86/include/asm/mce.h         |   1 -
 arch/x86/include/asm/thread_info.h |   4 +-
 arch/x86/kernel/cpu/mcheck/mce.c   | 106 ++++++++-----------------------------
 arch/x86/kernel/signal.c           |   6 ---
 4 files changed, 23 insertions(+), 94 deletions(-)

index 958b90f761e5..be5a51a4a219 100644
@@ -187,7 +187,6 @@ enum mcp_flags {
 void machine_check_poll(enum mcp_flags flags, mce_banks_t *b);
 
 int mce_notify_irq(void);
-void mce_notify_process(void);
 
 DECLARE_PER_CPU(struct mce, injectm);
 

index 05701f132473..55a63f114a74 100644
@@ -75,7 +75,6 @@ struct thread_info {
 #define TIF_SYSCALL_EMU		6	/* syscall emulation active */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SECCOMP		8	/* secure computing */
-#define TIF_MCE_NOTIFY		10	/* notify userspace of an MCE */
 #define TIF_USER_RETURN_NOTIFY	11	/* notify kernel of userspace return */
 #define TIF_UPROBE		12	/* breakpointed or singlestepping */
 #define TIF_NOTSC		16	/* TSC is not accessible in userland */

@@ -100,7 +99,6 @@ struct thread_info {
 #define _TIF_SYSCALL_EMU	(1 << TIF_SYSCALL_EMU)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
 #define _TIF_SECCOMP		(1 << TIF_SECCOMP)
-#define _TIF_MCE_NOTIFY		(1 << TIF_MCE_NOTIFY)
 #define _TIF_USER_RETURN_NOTIFY	(1 << TIF_USER_RETURN_NOTIFY)
 #define _TIF_UPROBE		(1 << TIF_UPROBE)
 #define _TIF_NOTSC		(1 << TIF_NOTSC)

@@ -140,7 +138,7 @@ struct thread_info {
 
 /* Only used for 64 bit */
 #define _TIF_DO_NOTIFY_MASK						\
-	(_TIF_SIGPENDING | _TIF_MCE_NOTIFY | _TIF_NOTIFY_RESUME |	\
+	(_TIF_SIGPENDING | _TIF_NOTIFY_RESUME |				\
 	 _TIF_USER_RETURN_NOTIFY | _TIF_UPROBE)
 
 /* flags to check in __switch_to() */

index ed37a768d0fc..2a33c8f68319 100644
@@ -740,12 +740,6 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
 {
 	user_exit();
 
-#ifdef CONFIG_X86_MCE
-	/* notify userspace of pending MCEs */
-	if (thread_info_flags & _TIF_MCE_NOTIFY)
-		mce_notify_process();
-#endif /* CONFIG_X86_64 && CONFIG_X86_MCE */
-
 	if (thread_info_flags & _TIF_UPROBE)
 		uprobe_notify_resume(regs);
 

2.1.0