i386: Execute stack overflow warning on interrupt stack

i386: Execute stack overflow warning on interrupt stack

arch/x86/kernel/irq_32.c +8 -0

@@ -61,6 +61,26 @@ static union irq_ctx *hardirq_ctx[NR_CPU
 static union irq_ctx *softirq_ctx[NR_CPUS] __read_mostly;
 #endif
 
+static void stack_overflow(void)
+{
+	printk("low stack detected by irq handler\n");
+	dump_stack();
+}
+
+static inline void call_on_stack2(void *func, unsigned long stack,
+			   unsigned long arg1, unsigned long arg2)

Quoted 2 lines

+ : "memory", "cc", "ecx');

Am I seeing wrong, or is it a ' after the ecx?

i386: Execute stack overflow warning on interrupt stack

arch/x86/kernel/irq_32.c static union irq_ctx *hardirq_ctx[NR_CPU · line 66

Quoted 3 lines · Andi Kleen

arch/x86/kernel/irq_32.c +3 -0

+static void stack_overflow(void)
+{
+	printk("low stack detected by irq handler\n");

Needs a KERN_ERR

arch/x86/kernel/irq_32.c unsigned int do_IRQ(struct pt_regs *regs · line 150

Quoted 6 lines

arch/x86/kernel/irq_32.c +5 -0

+		/* Execute warning on interrupt stack */
+		if (unlikely(overflow))
+			call_on_stack2(stack_overflow, isp, 0, 0);
+
+		call_on_stack2(desc->handle_irq, isp, irq, desc);

arch/x86/kernel/irq_32.c:148: warning: passing argument 2 of ‘call_on_stack2’ makes integer from pointer without a cast arch/x86/kernel/irq_32.c:150: warning: passing argument 2 of ‘call_on_stack2’ makes integer from pointer without a cast arch/x86/kernel/irq_32.c:150: warning: passing argument 4 of ‘call_on_stack2’ makes integer from pointer without a cast

arch/x86/kernel/irq_32.c unsigned int do_IRQ(struct pt_regs *regs · line 154

Quoted 5 lines

arch/x86/kernel/irq_32.c +2 -1

 	} else
 #endif
-		desc->handle_irq(irq, desc);
+	{
+		/* AK: Slightly bogus here */

Bogus comment. This applies to both the !4KSTACKS and the overflow of the irq stack in the 4KSTACKS case.

arch/x86/kernel/irq_32.c unsigned int do_IRQ(struct pt_regs *regs · line 155

Quoted 1 line

arch/x86/kernel/irq_32.c +1 -0

+		if (overflow)

unlikely(overflow) ?

Quoted 6 lines · Thomas Gleixner

arch/x86/kernel/irq_32.c static union irq_ctx *hardirq_ctx[NR_CPU · line 66

Quoted 3 lines · Andi Kleen

arch/x86/kernel/irq_32.c +3 -0

+static void stack_overflow(void)
+{
+	printk("low stack detected by irq handler\n");

Needs a KERN_ERR

Just moving code. If there is one added it should be in another patch.

Besides if anything it's a KERN_WARN I guess.

Quoted 10 lines

arch/x86/kernel/irq_32.c unsigned int do_IRQ(struct pt_regs *regs · line 150

Quoted 6 lines

arch/x86/kernel/irq_32.c +5 -0

+		/* Execute warning on interrupt stack */
+		if (unlikely(overflow))
+			call_on_stack2(stack_overflow, isp, 0, 0);
+
+		call_on_stack2(desc->handle_irq, isp, irq, desc);

arch/x86/kernel/irq_32.c:148: warning: passing argument 2 of ‘call_on_stack2’ makes integer from pointer without a cast arch/x86/kernel/irq_32.c:150: warning: passing argument 2 of ‘call_on_stack2’ makes integer from pointer without a cast arch/x86/kernel/irq_32.c:150: warning: passing argument 4 of ‘call_on_stack2’ makes integer from pointer without a cast

Hmm, strange. I don't see that here

CC arch/x86/kernel/irq_32.o
CC arch/x86/kernel/time_32.o

gcc version 4.1.2 20061115 (prerelease) (SUSE Linux)

What compiler are you using? Or did you change anything? (I know you like to do that)

Quoted 8 lines

arch/x86/kernel/irq_32.c unsigned int do_IRQ(struct pt_regs *regs · line 154

Quoted 5 lines

arch/x86/kernel/irq_32.c +2 -1

 	} else
 #endif
-		desc->handle_irq(irq, desc);
+	{
+		/* AK: Slightly bogus here */

Bogus comment. This applies to both the !4KSTACKS and the overflow of the irq stack in the 4KSTACKS case.

The comment refers to that the check here doesn't check the process stack, but the interrupt stack. In fact if the interrupt stack is near overflow we should probably just reject the interrupt? Although that might cause hangs too. Or perhaps just enlarge it [that is now possible with i386 pda with some effort]. Anyways it is probably not an interesting case because nested interrupts are rare.

Quoted 3 lines

arch/x86/kernel/irq_32.c unsigned int do_IRQ(struct pt_regs *regs · line 155

Quoted 1 line

arch/x86/kernel/irq_32.c +1 -0

+		if (overflow)

unlikely(overflow) ?

Doesn't matter really. The whole branch is unlikely.

Quoted 7 lines · Andi Kleen

Quoted 5 lines · Thomas Gleixner

arch/x86/kernel/irq_32.c static union irq_ctx *hardirq_ctx[NR_CPU · line 66

Quoted 3 lines · Andi Kleen

arch/x86/kernel/irq_32.c +3 -0

+static void stack_overflow(void)
+{
+	printk("low stack detected by irq handler\n");

Needs a KERN_ERR

Just moving code. If there is one added it should be in another patch.

Err, you are not moving code. The printk is pretty different and adding the KERN_xx in the same go is nothing which makes the patch harder to understand.

Quoted 1 line

Besides if anything it's a KERN_WARN I guess.

KERN_WARN is fine, even if I consider a stack overflow as an error.

Err, it checks the process stack when 4KSTACKS=n

Quoted 1 line

Err, it checks the process stack when 4KSTACKS=n

Ok then please add the two changes if you feel strongly about that (to the latest version I sent).

You always edit my patches anyways (usually driving me crazy when I have dependent patches because nothing applies anymore when all the variables got renamed like you often do) so I don't see any reason why you can't do that here.

Hi Andi,

Quoted 5 lines · Andi Kleen

Quoted 1 line

Err, it checks the process stack when 4KSTACKS=n

Ok then please add the two changes if you feel strongly about that (to the latest version I sent).

Quoted 4 lines

You always edit my patches anyways (usually driving me crazy when I have dependent patches because nothing applies anymore when all the variables got renamed like you often do) so I don't see any reason why you can't do that here.

Heh, do you mean to say that all this time I should have just asked Andrew to fix all the patches I've submitted rather take the trouble to do that myself after the review? ;-)

i386: Execute stack overflow warning on interrupt stack

[Repost. This was posted deep in the 4K flame war some time ago. Probably very few people read it completely, so the patch was missed.]

Previously the reporting printk would run on the process stack, which risks overflow an already low stack. Instead execute it on the interrupt stack. This makes it more likely for the printk to make it actually out.

It adds one not taken test/branch more to the interrupt path when stack overflow checking is enabled. We could avoid that by duplicating more code, but that seemed not worth it.

Based on an observation by Eric Sandeen.

Signed-off-by Andi Kleen <[email protected]>

Index: linux/arch/x86/kernel/irq_32.c
===================================================================

arch/x86/kernel/irq_32.c +31 -17

+	dump_stack();
+}
+
+static inline void call_on_stack2(void *func, unsigned long stack,
+			   unsigned long arg1, unsigned long arg2)
+{
+	unsigned long bx;
+	asm volatile(
+			"       xchgl  %%ebx,%%esp    \n"
+			"       call   *%%edi         \n"
+			"       movl   %%ebx,%%esp    \n"
+			: "=a" (arg1), "=d" (arg2), "=b" (bx)
+			:  "0" (arg1),   "1" (arg2),  "2" (stack),
+			   "D" (func)
+			: "memory", "cc", "ecx');
+}
+
 /*
  * do_IRQ handles all normal device IRQ's (the special
  * SMP cross-CPU interrupts have their own specific

@@ -76,6 +96,7 @@ unsigned int do_IRQ(struct pt_regs *regs
 	union irq_ctx *curctx, *irqctx;
 	u32 *isp;
 #endif
+	int overflow = 0;
 
 	if (unlikely((unsigned)irq >= NR_IRQS)) {
 		printk(KERN_EMERG "%s: cannot handle IRQ %d\n",

@@ -92,11 +113,8 @@ unsigned int do_IRQ(struct pt_regs *regs
 
 		__asm__ __volatile__("andl %%esp,%0" :
 					"=r" (sp) : "0" (THREAD_SIZE - 1));
-		if (unlikely(sp < (sizeof(struct thread_info) + STACK_WARN))) {
-			printk("do_IRQ: stack overflow: %ld\n",
-				sp - sizeof(struct thread_info));
-			dump_stack();
-		}
+		if (unlikely(sp < (sizeof(struct thread_info) + STACK_WARN)))
+			overflow = 1;
 	}
 #endif
 

@@ -112,8 +130,6 @@ unsigned int do_IRQ(struct pt_regs *regs
 	 * current stack (which is the irq stack already after all)
 	 */
 	if (curctx != irqctx) {
-		int arg1, arg2, bx;
-
 		/* build the stack frame on the IRQ stack */
 		isp = (u32*) ((char*)irqctx + sizeof(*irqctx));
 		irqctx->tinfo.task = curctx->tinfo.task;

@@ -127,18 +143,19 @@ unsigned int do_IRQ(struct pt_regs *regs
 			(irqctx->tinfo.preempt_count & ~SOFTIRQ_MASK) |
 			(curctx->tinfo.preempt_count & SOFTIRQ_MASK);
 
-		asm volatile(
-			"       xchgl  %%ebx,%%esp    \n"
-			"       call   *%%edi         \n"
-			"       movl   %%ebx,%%esp    \n"
-			: "=a" (arg1), "=d" (arg2), "=b" (bx)
-			:  "0" (irq),   "1" (desc),  "2" (isp),
-			   "D" (desc->handle_irq)
-			: "memory", "cc", "ecx"
-		);
+		/* Execute warning on interrupt stack */
+		if (unlikely(overflow))
+			call_on_stack2(stack_overflow, isp, 0, 0);
+
+		call_on_stack2(desc->handle_irq, isp, irq, desc);
 	} else
 #endif
-		desc->handle_irq(irq, desc);
+	{
+		/* AK: Slightly bogus here. Just return? */
+		if (overflow)
+			stack_overflow();
+ 		desc->handle_irq(irq, desc);
+	}
 
 	irq_exit();
 	set_irq_regs(old_regs);

We've lost the information about how close we are... that'd be nice to keep if possible....

Can we keep the old printk string and pass the remaining stack in as an arg?