Preemption must be disabled before enabling interrupts in do_trap on x86_64 because the stack in use for int3 and debug is a per CPU stack set by th IST. But in 32bit, the stack still belongs to the current task and there is no problem in scheduling out the task.

Keep preemption enabled on X86_32 when enabling interrupts for do_trap().

Signed-off-by Steven Rostedt <[email protected]>

arch/x86/kernel/traps.c +9 -0

Index: linux-rt.git/arch/x86/kernel/traps.c
===================================================================
@@ -89,7 +89,14 @@ static inline void conditional_sti(struc
 
 static inline void preempt_conditional_sti(struct pt_regs *regs)
 {
+#ifdef CONFIG_X86_64
+	/*
+	 * X86_64 uses a per CPU stack for certain traps like int3.
+	 * We must disable preemption, otherwise we can corrupt the
+	 * stack if the task is scheduled out with this stack.
+	 */
 	inc_preempt_count();
+#endif
 	if (regs->flags & X86_EFLAGS_IF)
 		local_irq_enable();
 }

@@ -104,7 +111,9 @@ static inline void preempt_conditional_c
 {
 	if (regs->flags & X86_EFLAGS_IF)
 		local_irq_disable();
+#ifdef CONFIG_X86_64
 	dec_preempt_count();
+#endif
 }
 
 static void __kprobes

Masami Hiramatsu

2012-01-26 08:39 UTC #2 lore raw

Re: [PATCH RT 1/2 v2] x86: Do not disable preemption in int3 on 32bit

Quoted 9 lines · Steven Rostedt

Preemption must be disabled before enabling interrupts in do_trap on x86_64 because the stack in use for int3 and debug is a per CPU stack set by th IST. But in 32bit, the stack still belongs to the current task and there is no problem in scheduling out the task.
Keep preemption enabled on X86_32 when enabling interrupts for do_trap().
Signed-off-by Steven Rostedt <[email protected]>

Hmm, I think you'd better change caller side, because the function itself has "preempt" in its name. I think it can easily mislead other people.

Thank you,

Quoted 31 lines

Index: linux-rt.git/arch/x86/kernel/traps.c
===================================================================

arch/x86/kernel/traps.c +9 -0

@@ -89,7 +89,14 @@ static inline void conditional_sti(struc
 
 static inline void preempt_conditional_sti(struct pt_regs *regs)
 {
+#ifdef CONFIG_X86_64
+	/*
+	 * X86_64 uses a per CPU stack for certain traps like int3.
+	 * We must disable preemption, otherwise we can corrupt the
+	 * stack if the task is scheduled out with this stack.
+	 */
 	inc_preempt_count();
+#endif
 	if (regs->flags & X86_EFLAGS_IF)
 		local_irq_enable();
 }

@@ -104,7 +111,9 @@ static inline void preempt_conditional_c
 {
 	if (regs->flags & X86_EFLAGS_IF)
 		local_irq_disable();
+#ifdef CONFIG_X86_64
 	dec_preempt_count();
+#endif
 }
 
 static void __kprobes

Signature · 4 lines

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Steven Rostedt

author reviewer

2012-01-26 13:33 UTC #3 lore raw

Re: [PATCH RT 1/2 v2] x86: Do not disable preemption in int3 on 32bit

Quoted 6 lines · Masami Hiramatsu

Quoted 5 lines · Steven Rostedt
Preemption must be disabled before enabling interrupts in do_trap on x86_64 because the stack in use for int3 and debug is a per CPU stack set by th IST. But in 32bit, the stack still belongs to the current task and there is no problem in scheduling out the task.

Quoted 8 lines

Quoted 4 lines
Keep preemption enabled on X86_32 when enabling interrupts for do_trap().
Signed-off-by Steven Rostedt <[email protected]>
Hmm, I think you'd better change caller side, because the function itself has "preempt" in its name. I think it can easily mislead other people.

How about if we rename it to

conditional_preempt_sti/cli() ?

Then it can be both a conditional preempt as well as interrupts being disabled. The condition on preempt is "is 32 bits? don't preempt :
preempt";

-- Steve

Masami Hiramatsu

2012-01-26 14:00 UTC #4 lore raw

Re: [PATCH RT 1/2 v2] x86: Do not disable preemption in int3 on 32bit

Quoted 16 lines · Steven Rostedt

Quoted 6 lines · Masami Hiramatsu
Quoted 5 lines · Steven Rostedt
Preemption must be disabled before enabling interrupts in do_trap on x86_64 because the stack in use for int3 and debug is a per CPU stack set by th IST. But in 32bit, the stack still belongs to the current task and there is no problem in scheduling out the task.
Quoted 8 lines
Quoted 4 lines
Keep preemption enabled on X86_32 when enabling interrupts for do_trap().
Signed-off-by Steven Rostedt <[email protected]>
Hmm, I think you'd better change caller side, because the function itself has "preempt" in its name. I think it can easily mislead other people.

Quoted 7 lines

How about if we rename it to
conditional_preempt_sti/cli() ?
Then it can be both a conditional preempt as well as interrupts being disabled. The condition on preempt is "is 32 bits? don't preempt :
preempt";

Better. If the functions are only for signaling, how about using the "signal" in name? :)

conditional_sti/cli_for_signal()

Thanks,

Signature · 4 lines

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Steven Rostedt

author reviewer

2012-01-26 14:22 UTC #5 lore raw

Re: [PATCH RT 1/2 v2] x86: Do not disable preemption in int3 on 32bit

Quoted 4 lines · Masami Hiramatsu

Better. If the functions are only for signaling, how about using the "signal" in name? :)
conditional_sti/cli_for_signal()

I don't think they are only for signals. The conditional_sti/cli is for all callers of do_trap(). But those callers that run on an IST stack use the preempt_conditional_sti/cli() code. Perhaps we should call it:

conditional_sti/cli_ist() ?

Actually, I think this is the best name. The "preempt_" one, didn't give any annotation about why it was disabling preemption. It was done because the stack is on the IST and we can not schedule out. Thus, if we add the "_ist()" to it, not only does it annotate why this call is special, but also allows x86_32, which does not have an IST, not to have to disable preemption.

-- Steve

Masami Hiramatsu

2012-01-27 09:36 UTC #6 lore raw

Re: [PATCH RT 1/2 v2] x86: Do not disable preemption in int3 on 32bit

Quoted 11 lines · Steven Rostedt

Quoted 4 lines · Masami Hiramatsu
Better. If the functions are only for signaling, how about using the "signal" in name? :)
conditional_sti/cli_for_signal()
I don't think they are only for signals. The conditional_sti/cli is for all callers of do_trap(). But those callers that run on an IST stack use the preempt_conditional_sti/cli() code. Perhaps we should call it:

Quoted 8 lines

conditional_sti/cli_ist() ?
Actually, I think this is the best name. The "preempt_" one, didn't give any annotation about why it was disabling preemption. It was done because the stack is on the IST and we can not schedule out. Thus, if we add the "_ist()" to it, not only does it annotate why this call is special, but also allows x86_32, which does not have an IST, not to have to disable preemption.

I agree with your opinion :)

Thanks,

Signature · 4 lines

Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: [email protected]

Steven Rostedt

author reviewer

2012-01-25 14:32 UTC #7 lore raw

PATCH v2 2/2 preempt-rt/x86: Delay calling signals in int3

In reply to Steven Rostedt

On x86_64 we must disable preemption before we enable interrupts for int3 and debugging, because the current task is using a per CPU debug stack defined by the IST. If we schedule out, another task can come in and use the same stack and cause the stack to be corrupted and crash the kernel on return.

When CONFIG_PREEMPT_RT_FULL is enabled, spin_locks become mutexes, and one of these is the spin lock used in signal handling.

Some of the debug code (int3) causes do_trap() to send a signal. This function calls a spin lock that has been converted to a mutex and has the possibility to sleep. If this happens, the above issues with the corrupted stack is possible.

Instead of calling the signal right away, for PREEMPT_RT and x86_64, the signal information is stored on the stacks task_struct and a new TIF flag is set (TIF_FORCE_SIG_TRAP). On exit of the exception, in paranoid_exit, if NEED_RESCHED is set, the task stack is switched back to the kernel stack and interrupts is enabled. In this code the TIF_FORCE_SIG_TRAP is also checked and a function is called to do the force_sig() in a context that may schedule.

Note, to get into this path, the NEED_RESCHED flag is also set. But as this only happens in debug context, an extra schedule should not be an issue.

Signed-off-by Steven Rostedt <[email protected]>

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

Index: linux-rt.git/arch/x86/include/asm/thread_info.h
===================================================================
@@ -95,6 +95,7 @@ struct thread_info {
 #define TIF_BLOCKSTEP		25	/* set when we want DEBUGCTLMSR_BTF */
 #define TIF_LAZY_MMU_UPDATES	27	/* task is updating the mmu lazily */
 #define TIF_SYSCALL_TRACEPOINT	28	/* syscall tracepoint instrumentation */
+#define TIF_FORCE_SIG_TRAP	29	/* force a signal coming back from trap */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_NOTIFY_RESUME	(1 << TIF_NOTIFY_RESUME)

@@ -117,6 +118,7 @@ struct thread_info {
 #define _TIF_BLOCKSTEP		(1 << TIF_BLOCKSTEP)
 #define _TIF_LAZY_MMU_UPDATES	(1 << TIF_LAZY_MMU_UPDATES)
 #define _TIF_SYSCALL_TRACEPOINT	(1 << TIF_SYSCALL_TRACEPOINT)
+#define _TIF_FORCE_SIG_TRAP	(1 << TIF_FORCE_SIG_TRAP)
 
 /* work to do in syscall_trace_enter() */
 #define _TIF_WORK_SYSCALL_ENTRY	\

@@ -266,5 +268,14 @@ extern void arch_task_cache_init(void);
 extern void free_thread_info(struct thread_info *ti);
 extern int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
 #define arch_task_cache_init arch_task_cache_init
+
+struct siginfo;
+/*
+ * Hacks for RT to get around signal processing with int3 and do_debug.
+ */
+void
+force_sig_info_rt(int sig, struct siginfo *info, struct task_struct *p, int rt);
+void send_sigtrap_rt(struct task_struct *tsk, struct pt_regs *regs,
+		     int error_code, int si_code);
 #endif
 #endif /* _ASM_X86_THREAD_INFO_H */

arch/x86/kernel/entry_64.S +7 -0

Index: linux-rt.git/arch/x86/kernel/entry_64.S
===================================================================
@@ -1391,6 +1391,13 @@ paranoid_userspace:
 paranoid_schedule:
 	TRACE_IRQS_ON
 	ENABLE_INTERRUPTS(CLBR_ANY)
+#ifdef CONFIG_PREEMPT_RT_FULL
+	movl TI_flags(%rcx),%ebx
+	testl $_TIF_FORCE_SIG_TRAP,%ebx
+	jz paranoid_do_schedule
+	call do_force_sig_trap
+paranoid_do_schedule:
+#endif
 	call schedule
 	DISABLE_INTERRUPTS(CLBR_ANY)
 	TRACE_IRQS_OFF

arch/x86/kernel/ptrace.c +20 -3

Index: linux-rt.git/arch/x86/kernel/ptrace.c
===================================================================
@@ -1341,14 +1341,31 @@ void user_single_step_siginfo(struct tas
 	fill_sigtrap_info(tsk, regs, 0, TRAP_BRKPT, info);
 }
 
-void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
-					 int error_code, int si_code)
+static void __send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
+		  int error_code, int si_code, int rt)
 {
 	struct siginfo info;
 
 	fill_sigtrap_info(tsk, regs, error_code, si_code, &info);
 	/* Send us the fake SIGTRAP */
-	force_sig_info(SIGTRAP, &info, tsk);
+	force_sig_info_rt(SIGTRAP, &info, tsk, rt);
+}
+
+void send_sigtrap(struct task_struct *tsk, struct pt_regs *regs,
+		  int error_code, int si_code)
+{
+	__send_sigtrap(tsk, regs, error_code, si_code, 0);
+}
+
+void send_sigtrap_rt(struct task_struct *tsk, struct pt_regs *regs,
+		  int error_code, int si_code)
+{
+#if defined(CONFIG_X86_64) && defined(CONFIG_PREEMPT_RT_FULL)
+	int rt = 1;
+#else
+	int rt = 0;
+#endif
+	__send_sigtrap(tsk, regs, error_code, si_code, rt);
 }

arch/x86/kernel/traps.c +86 -5

122 lines not on this page · raw message

include/linux/sched.h +7 -1

Index: linux-rt.git/include/linux/sched.h
===================================================================
@@ -1600,10 +1600,16 @@ struct task_struct {
 	struct rcu_head put_rcu;
 	int softirq_nestcnt;
 #endif
-#if defined CONFIG_PREEMPT_RT_FULL && defined CONFIG_HIGHMEM
+#if defined CONFIG_PREEMPT_RT_FULL
+#ifdef CONFIG_X86_64
+	struct siginfo stored_info;
+	int stored_info_set;
+#endif
+#ifdef CONFIG_HIGHMEM
 	int kmap_idx;
 	pte_t kmap_pte[KM_TYPE_NR];
 #endif
+#endif /* CONFIG_PREEMPT_RT_FULL */
 };
 
 #ifdef CONFIG_PREEMPT_RT_FULL