[PATCH] i386: Execute stack overflow warning on interrupt stack

[PATCH] i386: Execute stack overflow warning on interrupt stack

[Andi Kleen]

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" (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);

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack

[Jiri Slaby]

On 05/02/2008 11:18 AM, Andi Kleen wrote:
> i386: Execute stack overflow warning on interrupt stack
[...]
> --- 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');

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

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack II

[Andi Kleen]

On Fri, May 02, 2008 at 11:18:06AM +0200, Andi Kleen wrote:
> i386: Execute stack overflow warning on interrupt stack
[...] 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 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 */
+		if (overflow)
+			stack_overflow();
+ 		desc->handle_irq(irq, desc);
+	}
 
 	irq_exit();
 	set_irq_regs(old_regs);

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack

[Andi Kleen]

>> +            : "memory", "cc", "ecx');
> 
> 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

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack II

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1014 bytes --]

On Fri, 2 May 2008, Andi Kleen wrote:
> +static void stack_overflow(void)
> +{
> +	printk("low stack detected by irq handler\n");

Needs a KERN_ERR

> +		/* 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

>  	} 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.

> +		if (overflow)

unlikely(overflow) ?

> +			stack_overflow();
> + 		desc->handle_irq(irq, desc);

Thanks,

	tglx

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack II

[Andi Kleen]

Thomas Gleixner wrote:
> On Fri, 2 May 2008, Andi Kleen wrote:
>> +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.

> 
>> +		/* 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)


>>  	} 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.

>> +		if (overflow)
> 
> unlikely(overflow) ?

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

-Andi

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack II

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2368 bytes --]

On Mon, 5 May 2008, Andi Kleen wrote:
> Thomas Gleixner wrote:
> > On Fri, 2 May 2008, Andi Kleen wrote:
> >> +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.

> Besides if anything it's a KERN_WARN I guess.

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

> > 
> >> +		/* 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)

I noticed on review and just compiled the unmodified patch with 4KSTACKS=y. 
 
> >>  	} 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.

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

> >> +		if (overflow)
> > 
> > unlikely(overflow) ?
> 
> Doesn't matter really. The whole branch is unlikely.

There is no branch with 4KSTACKS=n

Thanks,

	tglx

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack II

[Eric Sandeen]

Thomas Gleixner wrote:
> On Mon, 5 May 2008, Andi Kleen wrote:
>> Thomas Gleixner wrote:
>>> On Fri, 2 May 2008, Andi Kleen wrote:
>>>> +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.
> 
>> Besides if anything it's a KERN_WARN I guess.
> 
> KERN_WARN is fine, even if I consider a stack overflow as an error.

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

-Eric

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack II

[Andi Kleen]

> 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.

-Andi

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack II

[Pekka Enberg]

Hi Andi,

On Mon, May 5, 2008 at 4:29 PM, Andi Kleen <andi@firstfloor.org> wrote:
>  > 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.

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

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack

[Eric Sandeen]

Andi Kleen wrote:
> 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");

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?

-Eric

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

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack II

[Andi Kleen]

Pekka Enberg wrote:
> Hi Andi,
> 
> On Mon, May 5, 2008 at 4:29 PM, Andi Kleen <andi@firstfloor.org> wrote:
>>  > 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.
> 
> 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

Re: [PATCH] i386: Execute stack overflow warning on interrupt stack

[Andi Kleen]

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

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
dropped it.

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

It would have complicated the patch.

-Andi