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 <andi@firstfloor.org>

Index: linux/arch/x86/kernel/irq_32.c
===================================================================
--- linux.orig/arch/x86/kernel/irq_32.c
+++ linux/arch/x86/kernel/irq_32.c
@@ -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)
+{
+	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" (...
[ message continues ]

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?


--

If you don't want to fix it then the message was unnecessary and your
threshold was too high. So you should always fix when you see the
message and the how close information is unnecessary. That is why I

It would have complicated the patch.

-Andi

--

[...] Sorry the earlier patch was unrefreshed with a dumb typo. Correct
version appended.

---
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 <andi@firstfloor.org>

Index: linux/arch/x86/kernel/irq_32.c
===================================================================
--- linux.orig/arch/x86/kernel/irq_32.c
+++ linux/arch/x86/kernel/irq_32.c
@@ -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)
+{
+	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...
[ message continues ]

arch/x86/kernel/irq_32.c:148: warning: passing argument 2 of

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

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

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

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

-Andi
--

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

> > arch/x86/kernel/irq_32.c:148: warning: passing argument 2 of

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.

-Andi
--

Hi Andi,


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

It is quite ok for trivial changes (like adding KERN_WARN or unlikely).
And yes Andrew does this all the time too. It makes sense because for
such trivialities it is less work to just change the patch than to comment.

-Andi

--

But it hasn't overflowed yet.  It's a warning about being *close* - not
an error.

-Eric
--

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

No that was the unrefreshed version. I had the patch around for some
time and then updated and noticed Jan's change that added the ecx
clobber and added that one too but unfortunately with typo, which
was then caught, but then forgot to quilt refresh before posting. I
reposted with the correct version now.

Thanks for looking.

-Andi

--