UML - Fix irqstack crash

PATCH In review Jeff Dike 3 participants 5 messages Sep 18, 2007 – Sep 26, 2007 User-mode Linux (UML)

Share & tools

Jeff Dike

author

2007-09-18 23:33 UTC #0 lore raw

PATCH 1/1 UML - Fix irqstack crash

This patch fixes a crash caused by an interrupt coming in when an IRQ stack is being torn down. When this happens, handle_signal will loop, setting up the IRQ stack again because the tearing down had finished, and handling whatever signals had come in.

However, to_irq_stack returns a mask of pending signals to be handled, plus bit zero is set if the IRQ stack was already active, and thus shouldn't be torn down. This causes a problem because when handle_signal goes around the loop, sig will be zero, and to_irq_stack will duly set bit zero in the returned mask, faking handle_signal into believing that it shouldn't tear down the IRQ stack and return thread_info pointers back to their original values.

This will eventually cause a crash, as the IRQ stack thread_info will continue pointing to the original task_struct and an interrupt will look into it after it has been freed.

The fix is to stop passing a signal number into to_irq_stack. Rather, the pending signals mask is initialized beforehand with the bit for sig already set. References to sig in to_irq_stack can be replaced with references to the mask.

Signed-off-by Jeff Dike <[email protected]>

---
 arch/um/include/kern_util.h |    2 +-
 arch/um/kernel/irq.c        |    7 ++++---
 arch/um/os-Linux/signal.c   |    4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

arch/um/include/kern_util.h +1 -1

Index: linux-2.6.17/arch/um/include/kern_util.h
===================================================================
@@ -117,7 +117,7 @@ extern void sigio_handler(int sig, union
 
 extern void copy_sc(union uml_pt_regs *regs, void *from);
 
-unsigned long to_irq_stack(int sig, unsigned long *mask_out);
+extern unsigned long to_irq_stack(unsigned long *mask_out);
 unsigned long from_irq_stack(int nested);
 
 #endif

arch/um/kernel/irq.c +4 -3

Index: linux-2.6.17/arch/um/kernel/irq.c
===================================================================
@@ -518,13 +518,13 @@ int init_aio_irq(int irq, char *name, ir
 
 static unsigned long pending_mask;
 
-unsigned long to_irq_stack(int sig, unsigned long *mask_out)
+unsigned long to_irq_stack(unsigned long *mask_out)
 {
 	struct thread_info *ti;
 	unsigned long mask, old;
 	int nested;
 
-	mask = xchg(&pending_mask, 1 << sig);
+	mask = xchg(&pending_mask, *mask_out);
 	if(mask != 0){
 		/* If any interrupts come in at this point, we want to
 		 * make sure that their bits aren't lost by our

@@ -534,7 +534,7 @@ unsigned long to_irq_stack(int sig, unsi
 		 * and pending_mask contains a bit for each interrupt
 		 * that came in.
 		 */
-		old = 1 << sig;
+		old = *mask_out;
 		do {
 			old |= mask;
 			mask = xchg(&pending_mask, old);

@@ -550,6 +550,7 @@ unsigned long to_irq_stack(int sig, unsi
 
 		task = cpu_tasks[ti->cpu].task;
 		tti = task_thread_info(task);
+
 		*ti = *tti;
 		ti->real_thread = tti;
 		task->stack = ti;

arch/um/os-Linux/signal.c +2 -2

Index: linux-2.6.17/arch/um/os-Linux/signal.c
===================================================================
@@ -119,7 +119,7 @@ void (*handlers[_NSIG])(int sig, struct 
 
 void handle_signal(int sig, struct sigcontext *sc)
 {
-	unsigned long pending = 0;
+	unsigned long pending = 1 << sig;
 
 	do {
 		int nested, bail;

@@ -134,7 +134,7 @@ void handle_signal(int sig, struct sigco
 		 * have to return, and the upper handler will deal
 		 * with this interrupt.
 		 */
-		bail = to_irq_stack(sig, &pending);
+		bail = to_irq_stack(&pending);
 		if(bail)
 			return;

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Andrew Morton

2007-09-19 00:07 UTC #1 lore raw

arch/um/os-Linux/signal.c void (*handlers[_NSIG])(int sig, struct · line 122

Quoted 9 lines · On Tue, 18 Sep 2007 19:33:36 -0400 Jeff Dike <[email protected]> wrote:

===================================================================

arch/um/os-Linux/signal.c +1 -1

@@ -119,7 +119,7 @@ void (*handlers[_NSIG])(int sig, struct 
 
 void handle_signal(int sig, struct sigcontext *sc)
 {
-	unsigned long pending = 0;
+	unsigned long pending = 1 << sig;

You want 1UL there.

Paolo Giarrusso

2007-09-20 16:02 UTC #2 lore raw

Quoted 9 lines · Andrew Morton

On Tue, 18 Sep 2007 19:33:36 -0400
Quoted 6 lines · Jeff Dike
===================================================================
--- linux-2.6.17.orig/arch/um/os-Linux/signal.c 2007-09-09
11:15:37.000000000 -0400 +++
linux-2.6.17/arch/um/os-Linux/signal.c 2007-09-18 12:32:40.000000000
-0400 @@ -119,7 +119,7 @@ void (*handlers[_NSIG])(int sig, struct

Quoted 6 lines

arch/um/os-Linux/signal.c void (*handlers[_NSIG])(int sig, struct · line 122
Quoted 4 lines
void handle_signal(int sig, struct sigcontext *sc) {
arch/um/os-Linux/signal.c +1 -1
-	unsigned long pending = 0;
+	unsigned long pending = 1 << sig;
You want 1UL there.

Yes, indeed - or sign extension on 64bit machines would set to 1 the whole high-word.

But using long for that mask makes no difference; either int or long long (or better, either u32 or u64) should be used, given that the used signal range is the same on 32 and 64bit machines, it should be u32 for normal signals or u64 if RT-signals are also allowed.

Signature · 2 lines

"Doh!" (cit.), I've made another mistake!
Paolo Giarrusso, aka Blaisorblade

_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Jeff Dike

author

2007-09-24 20:02 UTC #3 lore raw

Quoted 7 lines · Paolo Giarrusso

Yes, indeed - or sign extension on 64bit machines would set to 1 the whole high-word.
But using long for that mask makes no difference; either int or long long (or better, either u32 or u64) should be used, given that the used signal range is the same on 32 and 64bit machines, it should be u32 for normal signals or u64 if RT-signals are also allowed.

We don't use RT signals for anything, so we could use u32. I don't see it making much difference though.

Jeff

Signature · 10 lines

Work email - jdike at linux dot intel dot com

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
User-mode-linux-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Paolo Giarrusso

2007-09-26 15:43 UTC #4 lore raw

Quoted 10 lines · Jeff Dike

Quoted 7 lines · Paolo Giarrusso
Yes, indeed - or sign extension on 64bit machines would set to 1 the whole high-word.
But using long for that mask makes no difference; either int or long long (or better, either u32 or u64) should be used, given that the used signal range is the same on 32 and 64bit machines, it should be u32 for normal signals or u64 if RT-signals are also allowed.
We don't use RT signals for anything, so we could use u32.

I wasn't sure of that.

Quoted 2 lines

I don't
see it making much difference though.

Agreed - the only difference is for cleanliness and easy verification.

Signature · 2 lines

"Doh!" (cit.), I've made another mistake!
Paolo Giarrusso, aka Blaisorblade