: Call do_notify_resume() with interrupts enabled

Since there is a possibility of !KPROBES int3 listeners and since DIE_TRAP is currently not being used by anybody, notify all listeners with DIE_INT3.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Srikar Dronamraju <[email protected]>

---
 arch/x86/kernel/traps.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

arch/x86/kernel/traps.c +1 -6

index 6913369..c9a4be5 100644
@@ -461,15 +461,10 @@ dotraplinkage void __kprobes do_int3(struct pt_regs *regs, long error_code)
 			== NOTIFY_STOP)
 		return;
 #endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
-#ifdef CONFIG_KPROBES
+
 	if (notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
 			== NOTIFY_STOP)
 		return;
-#else
-	if (notify_die(DIE_TRAP, "int3", regs, error_code, 3, SIGTRAP)
-			== NOTIFY_STOP)
-		return;
-#endif
 
 	preempt_conditional_sti(regs);
 	do_trap(3, SIGTRAP, "int3", regs, error_code, NULL);

Since there is a possibility of !KPROBES int3 listeners and since DIE_TRAP is currently not being used by anybody, notify all listeners with DIE_INT3.

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Srikar Dronamraju <[email protected]>

---
 arch/x86/kernel/traps.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

arch/x86/kernel/traps.c +1 -7

index 6913369..c9a4be5 100644
@@ -461,15 +461,10 @@ dotraplinkage void __kprobes do_int3(struct pt_regs *regs, long error_code)
 			== NOTIFY_STOP)
 		return;
 #endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
-#ifdef CONFIG_KPROBES
+
 	if (notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
 			== NOTIFY_STOP)
 		return;
-#else
-	if (notify_die(DIE_TRAP, "int3", regs, error_code, 3, SIGTRAP)
-			== NOTIFY_STOP)
-		return;
-#endif
 
 	preempt_conditional_sti(regs);
 	do_trap(3, SIGTRAP, "int3", regs, error_code, NULL);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 3 lines · Srikar Dronamraju

Since there is a possibility of !KPROBES int3 listeners and since DIE_TRAP is currently not being used by anybody, notify all listeners with DIE_INT3.

I'm not sure why this CONFIG_KPROBES dependency had been introduced, but it's the time to fix it. do_int3 should send DIE_INT3 always.

Acked-by Masami Hiramatsu <[email protected]>

Thank you!

Quoted 34 lines

Signed-off-by Ananth N Mavinakayanahalli <[email protected]>

Signed-off-by Srikar Dronamraju <[email protected]>

---
 arch/x86/kernel/traps.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

arch/x86/kernel/traps.c +1 -7

index 6913369..c9a4be5 100644
@@ -461,15 +461,10 @@ dotraplinkage void __kprobes do_int3(struct pt_regs *regs, long error_code)
 			== NOTIFY_STOP)
 		return;
 #endif /* CONFIG_KGDB_LOW_LEVEL_TRAP */
-#ifdef CONFIG_KPROBES
+
 	if (notify_die(DIE_INT3, "int3", regs, error_code, 3, SIGTRAP)
 			== NOTIFY_STOP)
 		return;
-#else
-	if (notify_die(DIE_TRAP, "int3", regs, error_code, 3, SIGTRAP)
-			== NOTIFY_STOP)
-		return;
-#endif
 
 	preempt_conditional_sti(regs);
 	do_trap(3, SIGTRAP, "int3", regs, error_code, NULL);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 4 lines

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

do_notify_resume() gets called with interrupts disabled on x86_32. This is different from the x86_64 behavior, where interrupts are enabled at the time.

Queries on lkml on this issue hasn't yielded any clear answer. Lets make x86_32 behave the same as x86_64, unless there is a real reason to maintain status quo.

Please refer https://lkml.org/lkml/2011/9/27/130 for more details

A similar change was suggested in arm

	https://lkml.org/lkml/2011/8/25/231

My 32-bit machine works fine (tm) with the patch below

Signed-off-by Srikar Dronamraju <[email protected]>

---
 arch/x86/kernel/entry_32.S |    4 ++++
 1 files changed, 4 insertions(+)

arch/x86/kernel/entry_32.S +4 -0

index f3f6f53..22d0e21 100644
@@ -625,6 +625,8 @@ work_notifysig:				# deal with pending signals and
 	movl %esp, %eax
 	jne work_notifysig_v86		# returning to kernel-space or
 					# vm86-space
+	TRACE_IRQS_ON
+	ENABLE_INTERRUPTS(CLBR_NONE)
 	xorl %edx, %edx
 	call do_notify_resume
 	jmp resume_userspace_sig

@@ -638,6 +640,8 @@ work_notifysig:				# deal with pending signals and
 #else
 	movl %esp, %eax
 #endif
+	TRACE_IRQS_ON
+	ENABLE_INTERRUPTS(CLBR_NONE)
 	xorl %edx, %edx
 	call do_notify_resume
 	jmp resume_userspace_sig

Quoted 4 lines · Srikar Dronamraju

do_notify_resume() gets called with interrupts disabled on x86_32. This is different from the x86_64 behavior, where interrupts are enabled at the time.

And note that do_notify_resume() paths assume that irqs are enabled. key_replace_session_keyring/get_signal_to_deliver start with _lock_irq.

IOW, I vote for this change even if I can't ack it (although it looks "obviously correct" to me). In fact refrigerator() looks buggy without this change. Yes, it enables irqs but only "by accident", via unlock_irq(). And we are going to remove this recalc_sigpending() from freezer.

Quoted 39 lines

Queries on lkml on this issue hasn't yielded any clear answer. Lets make x86_32 behave the same as x86_64, unless there is a real reason to maintain status quo.

Please refer https://lkml.org/lkml/2011/9/27/130 for more details

A similar change was suggested in arm
      https://lkml.org/lkml/2011/8/25/231

My 32-bit machine works fine (tm) with the patch below

Signed-off-by Srikar Dronamraju <[email protected]>

---
 arch/x86/kernel/entry_32.S |    4 ++++

 1 files changed, 4 insertions(+)

arch/x86/kernel/entry_32.S +0 -0

index f3f6f53..22d0e21 100644
@@ -625,6 +625,8 @@ work_notifysig:                           # deal with pending signals and

      movl %esp, %eax
      jne work_notifysig_v86          # returning to kernel-space or

                                      # vm86-space
+     TRACE_IRQS_ON
+     ENABLE_INTERRUPTS(CLBR_NONE)
      xorl %edx, %edx
      call do_notify_resume
      jmp resume_userspace_sig

arch/x86/kernel/entry_32.S +0 -0

@@ -638,6 +640,8 @@ work_notifysig:                           # deal with pending signals and
  #else

      movl %esp, %eax
 #endif
+     TRACE_IRQS_ON
+     ENABLE_INTERRUPTS(CLBR_NONE)
      xorl %edx, %edx
      call do_notify_resume
      jmp resume_userspace_sig

Ingo, Thomas, I think this is your call, but it seems valid,

I worry about are race condition like the following:

syscall enter
...
syscall returns -ERESTARTNOHAND
check for signal
signal pending, but no handler, setup for restart interrupt happens, sets need_resched
need_resched set
switch to another thread
...
something happens which queues SIGIO
switch back to this thread
check for signal
signal pending, has handler, but we've setup for a restart
return to userspace
run SIGIO handler
restart syscall

However, that doesn't solve the (probably unsolvable) case where an ERESTARTSYS syscall is interrupted by a SA_RESTART-marked handler, and while that handler is running it is then interrupted by a non-SA_RESTART- marked handler. I think that is far too an obscure case to care about though.

I've been toying with a similar patch for ARM, but I keep feeling uneasy about having interrupts enabled in this path (even though they get enabled in the depths of the signal handling code.)

I worry about are race condition like the following:

syscall enter
...
syscall returns -ERESTARTNOHAND
check for signal
signal pending, but no handler, setup for restart interrupt happens, sets need_resched
need_resched set
switch to another thread
...
something happens which queues SIGIO switch back to this thread

check for signal
signal pending, has handler, but we've setup for a restart
return to userspace
run SIGIO handler
restart syscall

This feels like it violates the expectations of the syscall being restarted - which explicitly asks to be restarted only if there wasn't a handler run.

However, that doesn't solve the (probably unsolvable) case where an ERESTARTSYS syscall is interrupted by a SA_RESTART-marked handler, and while that handler is running it is then interrupted by a non-SA_RESTART- marked handler. I think that is far too an obscure case to care about though.

Ingo, Thomas, I think this is your call, but it seems valid,

Linus

Quoted 26 lines · Srikar Dronamraju

Quoted 4 lines

do_notify_resume() gets called with interrupts disabled on x86_32. This is different from the x86_64 behavior, where interrupts are enabled at the time.

And note that do_notify_resume() paths assume that irqs are enabled. key_replace_session_keyring/get_signal_to_deliver start with _lock_irq.

IOW, I vote for this change even if I can't ack it (although it looks "obviously correct" to me). In fact refrigerator() looks buggy without this change. Yes, it enables irqs but only "by accident", via unlock_irq(). And we are going to remove this recalc_sigpending() from freezer.

Quoted 11 lines

Queries on lkml on this issue hasn't yielded any clear answer. Lets make x86_32 behave the same as x86_64, unless there is a real reason to maintain status quo.

Please refer https://lkml.org/lkml/2011/9/27/130 for more details

A similar change was suggested in arm
      https://lkml.org/lkml/2011/8/25/231

My 32-bit machine works fine (tm) with the patch below