[RFC PATCH 0/5] x86: check stack overflows more reliably

[RFC PATCH 0/5] x86: check stack overflows more reliably

[Mitsuo Hayasaka]

Hi,

This patch series adds the following three features about stack
overflow checking. The (2) and (3) features work if their options
are enabled.

(1) add user mode vm check
    The kernel stack overflow is checked in stack_overflow_check(),
    which may wrongly detect the overflow if the user stack pointer
    pointed to the kernel stack accidentally. To avoid this misdetection,
    bail out early if the user stack is used.

(2) check stack overflow in detail
    Currently, only kernel stack is checked for the overflow,
    which is not sufficient for enterprise systems. To enhance
    reliability, expand stack overflow checking to IRQ and 
    exception stacks optionally. This is disabled by default
    in Kconfig.

(3) panic on stack overflow
    Currently, kernel messages are output on the detection of 
    stack overflow. Similarly, its's not sufficient for enterprise
    systems since it may corrupt data. To enhance reliability,
    cause a panic for the overflows according to the sysctl parameter.
    This is disabled by default.

Thanks,

---

Mitsuo Hayasaka (5):
      x86: change range of stack overflow checking
      x86: panic on detection of stack overflow
      x86: add a sysctl parameter to panic on stack overflow
      x86: check stack overflow in detail
      x86: add user_mode_vm check in stack_overflow_check


 Documentation/sysctl/kernel.txt |   13 +++++++++++
 arch/x86/Kconfig.debug          |   11 +++++++++
 arch/x86/kernel/irq_32.c        |    2 ++
 arch/x86/kernel/irq_64.c        |   46 ++++++++++++++++++++++++++++++++++++---
 include/linux/kernel.h          |    1 +
 include/linux/sysctl.h          |    1 +
 kernel/sysctl.c                 |    9 ++++++++
 kernel/sysctl_binary.c          |    1 +
 8 files changed, 80 insertions(+), 4 deletions(-)

-- 
Mitsuo Hayasaka (mitsuo.hayasaka.hu@hitachi.com)

[RFC PATCH 1/5] x86: add user_mode_vm check in stack_overflow_check

[Mitsuo Hayasaka]

The kernel stack overflow is checked in stack_overflow_check(),
which may wrongly detect the overflow if the stack pointer
pointed to the kernel stack accidentally.

This patch adds user-mode-vm checking before it to avoid this
misdetection and bails out early if the user stack is used.

Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---

 arch/x86/kernel/irq_64.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
index acf8fbf..69bca46 100644
--- a/arch/x86/kernel/irq_64.c
+++ b/arch/x86/kernel/irq_64.c
@@ -38,6 +38,9 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 #ifdef CONFIG_DEBUG_STACKOVERFLOW
 	u64 curbase = (u64)task_stack_page(current);
 
+	if (user_mode_vm(regs))
+		return;
+
 	WARN_ONCE(regs->sp >= curbase &&
 		  regs->sp <= curbase + THREAD_SIZE &&
 		  regs->sp <  curbase + sizeof(struct thread_info) +

Re: [RFC PATCH 1/5] x86: add user_mode_vm check in stack_overflow_check

[Konrad Rzeszutek Wilk]

On Mon, Nov 07, 2011 at 02:52:35PM +0900, Mitsuo Hayasaka wrote:
> The kernel stack overflow is checked in stack_overflow_check(),
> which may wrongly detect the overflow if the stack pointer
> pointed to the kernel stack accidentally.

I think you mean to say 'points'.

How do we accidently point the stack pointer to the kernel stack?

> 
> This patch adds user-mode-vm checking before it to avoid this
> misdetection and bails out early if the user stack is used.
> 
> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
> 
>  arch/x86/kernel/irq_64.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
> index acf8fbf..69bca46 100644
> --- a/arch/x86/kernel/irq_64.c
> +++ b/arch/x86/kernel/irq_64.c
> @@ -38,6 +38,9 @@ static inline void stack_overflow_check(struct pt_regs *regs)
>  #ifdef CONFIG_DEBUG_STACKOVERFLOW
>  	u64 curbase = (u64)task_stack_page(current);
>  
> +	if (user_mode_vm(regs))
> +		return;
> +
>  	WARN_ONCE(regs->sp >= curbase &&
>  		  regs->sp <= curbase + THREAD_SIZE &&
>  		  regs->sp <  curbase + sizeof(struct thread_info) +
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 1/5] x86: add user_mode_vm check in stack_overflow_check

[HAYASAKA Mitsuo]

(2011/11/11 4:52), Konrad Rzeszutek Wilk wrote:
> On Mon, Nov 07, 2011 at 02:52:35PM +0900, Mitsuo Hayasaka wrote:
>> The kernel stack overflow is checked in stack_overflow_check(),
>> which may wrongly detect the overflow if the stack pointer
>> pointed to the kernel stack accidentally.
> 
> I think you mean to say 'points'.

Yes. Thank you for your correction.

> 
> How do we accidently point the stack pointer to the kernel stack?

I guess it may happen due to a kind of stack overflow, although
I've not succeeded to `accidentally' point the stack pointer to
the kernel stack yet...

At least, we can intentionally cause the overflow message, using the
following program in user space although the overflow does not occur.

=============================================
#include <stdio.h>
#include <sys/types.h>
#include <unistd.h>

int main(int argc, char *argv[]) {
	unsigned long long dummyRSP;

	printf("PID:%d\n", getpid());
	printf("Input dummyRSP address: ");
	scanf("%Lx", &dummyRSP);
	printf("DummyRSP address is %Lx\n", dummyRSP);
	puts("Replace RSP with dummyRSP...");
	__asm__ ("movq %0,%%rsp": : "r" (dummyRSP));
	while(1) ;
}
=============================================

We need to give this program a dummy RSP address that must point to
an overflow address in kernel stack and can be gotten using tools such
as crash. These steps are summarized as follows.

(1) Execute this program and get the pid.
(2) Execute crash and put "task <pid>"
(3) Get the address indicated by stack field of task_struct
(4) Input the address to this program

The big problem is that user space program can directly control the
stack overflow checking in kernel space, regardless of intentional or
accidental operation. In other words, the kernel stack overflow is
never detected after execution of this program.

So, I would like to fix this problem.

> 
>>
>> This patch adds user-mode-vm checking before it to avoid this
>> misdetection and bails out early if the user stack is used.
>>
>> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> ---
>>
>>  arch/x86/kernel/irq_64.c |    3 +++
>>  1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
>> index acf8fbf..69bca46 100644
>> --- a/arch/x86/kernel/irq_64.c
>> +++ b/arch/x86/kernel/irq_64.c
>> @@ -38,6 +38,9 @@ static inline void stack_overflow_check(struct pt_regs *regs)
>>  #ifdef CONFIG_DEBUG_STACKOVERFLOW
>>  	u64 curbase = (u64)task_stack_page(current);
>>  
>> +	if (user_mode_vm(regs))
>> +		return;
>> +
>>  	WARN_ONCE(regs->sp >= curbase &&
>>  		  regs->sp <= curbase + THREAD_SIZE &&
>>  		  regs->sp <  curbase + sizeof(struct thread_info) +
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 

[RFC PATCH 2/5] x86: check stack overflow in detail

[Mitsuo Hayasaka]

Currently, only kernel stack is checked for the overflow, which is
not suffcient for enterprise systems. To enhance reliability,
it is required to check the IRQ and exception stacks, as well.

This patch optionally checks all the stack types and will cause
messages of stacks in detail when free stack space drops below a
certain limit except user stack. This is disabled by default in
Kconfig.

Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---

 arch/x86/Kconfig.debug   |   11 +++++++++++
 arch/x86/kernel/irq_64.c |   28 ++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index bf56e17..0d98beb 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -66,6 +66,17 @@ config DEBUG_STACKOVERFLOW
 	  This option will cause messages to be printed if free stack space
 	  drops below a certain limit.
 
+config DEBUG_STACKOVERFLOW_DETAIL
+	bool "Check for stack overflows in detail"
+	depends on DEBUG_KERNEL && DEBUG_STACKOVERFLOW
+	depends on X86_64
+	---help---
+	  Say Y here if you want to check the overflows of IRQ and
+	  exception stacks in addition to kernel stack. This option will
+	  cause messages of the stacks in detail when free stack space
+	  drops below a certain limit.
+	  If in doubt, say "N".
+
 config X86_PTDUMP
 	bool "Export kernel pagetable layout to userspace via debugfs"
 	depends on DEBUG_KERNEL
diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
index 69bca46..530cfd0 100644
--- a/arch/x86/kernel/irq_64.c
+++ b/arch/x86/kernel/irq_64.c
@@ -37,10 +37,37 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 {
 #ifdef CONFIG_DEBUG_STACKOVERFLOW
 	u64 curbase = (u64)task_stack_page(current);
+#ifdef CONFIG_DEBUG_STACKOVERFLOW_DETAIL
+	struct orig_ist *oist;
+	u64 irq_stack_top, irq_stack_bottom;
+	u64 estack_top, estack_bottom;
+#endif
 
 	if (user_mode_vm(regs))
 		return;
 
+#ifdef CONFIG_DEBUG_STACKOVERFLOW_DETAIL
+	if (regs->sp >= curbase + sizeof(struct thread_info) +
+				sizeof(struct pt_regs) + 128 &&
+		regs->sp <= curbase + THREAD_SIZE)
+		return;
+
+	irq_stack_top = (u64)__get_cpu_var(irq_stack_union.irq_stack);
+	irq_stack_bottom = (u64)__get_cpu_var(irq_stack_ptr);
+	if (regs->sp >= irq_stack_top && regs->sp <= irq_stack_bottom)
+		return;
+
+	oist = &__get_cpu_var(orig_ist);
+	estack_top = (u64)oist->ist[0] - EXCEPTION_STKSZ;
+	estack_bottom = (u64)oist->ist[N_EXCEPTION_STACKS - 1];
+	if (regs->sp >= estack_top && regs->sp <= estack_bottom)
+		return;
+
+	WARN_ONCE(1, "do_IRQ: %s near or already stack stack overflow (cur:%Lx,sp:%lx,irq stk top-bottom:%Lx-%Lx,exception stk top-bottom:%Lx-%Lx)\n",
+		current->comm, curbase, regs->sp,
+		irq_stack_top, irq_stack_bottom,
+		estack_top, estack_bottom);
+#else
 	WARN_ONCE(regs->sp >= curbase &&
 		  regs->sp <= curbase + THREAD_SIZE &&
 		  regs->sp <  curbase + sizeof(struct thread_info) +
@@ -48,6 +75,7 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 
 		  "do_IRQ: %s near stack overflow (cur:%Lx,sp:%lx)\n",
 			current->comm, curbase, regs->sp);
+#endif /* CONFIG_DEBUG_STACKOVERFLOW_DETAIL */
 #endif
 }
 

[RFC PATCH 3/5] x86: add a sysctl parameter to panic on stack overflow

[Mitsuo Hayasaka]

This patch adds kernel.panic_on_stackoverflow sysctl parameter,
which can cause a panic when detecting the stack overflow.

Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Randy Dunlap <rdunlap@xenotime.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---

 Documentation/sysctl/kernel.txt |   13 +++++++++++++
 arch/x86/kernel/irq_64.c        |    2 ++
 include/linux/kernel.h          |    1 +
 include/linux/sysctl.h          |    1 +
 kernel/sysctl.c                 |    9 +++++++++
 kernel/sysctl_binary.c          |    1 +
 6 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 704e474..eb8d263 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -48,6 +48,7 @@ show up in /proc/sys/kernel:
 - panic
 - panic_on_oops
 - panic_on_unrecovered_nmi
+- panic_on_stackoverflow
 - pid_max
 - powersave-nap               [ PPC only ]
 - printk
@@ -385,6 +386,18 @@ Controls the kernel's behaviour when an oops or BUG is encountered.
 
 ==============================================================
 
+panic_on_stackoverflow:
+
+Controls the kernel's behaviour when detecting stack overflow.
+This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
+
+0: try to continue operation.
+
+1: panic immediately.
+
+==============================================================
+
+
 pid_max:
 
 PID allocation wrap value.  When the kernel's next PID value
diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
index 530cfd0..d720813 100644
--- a/arch/x86/kernel/irq_64.c
+++ b/arch/x86/kernel/irq_64.c
@@ -20,6 +20,8 @@
 #include <asm/idle.h>
 #include <asm/apic.h>
 
+int sysctl_panic_on_stackoverflow;
+
 DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat);
 EXPORT_PER_CPU_SYMBOL(irq_stat);
 
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 8eefcf7..19b3595 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -339,6 +339,7 @@ extern int panic_timeout;
 extern int panic_on_oops;
 extern int panic_on_unrecovered_nmi;
 extern int panic_on_io_nmi;
+extern int sysctl_panic_on_stackoverflow;
 extern const char *print_tainted(void);
 extern void add_taint(unsigned flag);
 extern int test_taint(unsigned flag);
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
index 9a1ec10..4b9dc3d 100644
--- a/include/linux/sysctl.h
+++ b/include/linux/sysctl.h
@@ -153,6 +153,7 @@ enum
 	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
 	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
 	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
+	KERN_PANIC_ON_STACKOVERFLOW = 77, /* int: panic on stack overflow */
 };
 
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 11d65b5..08a18f9 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -792,6 +792,15 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= proc_dointvec,
 	},
+#if defined(CONFIG_DEBUG_STACKOVERFLOW)
+	{
+		.procname	= "panic_on_stackoverflow",
+		.data		= &sysctl_panic_on_stackoverflow,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec,
+	},
+#endif
 	{
 		.procname	= "bootloader_type",
 		.data		= &bootloader_type,
diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index 6318b51..24fc654 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
 	{ CTL_INT,	KERN_COMPAT_LOG,		"compat-log" },
 	{ CTL_INT,	KERN_MAX_LOCK_DEPTH,		"max_lock_depth" },
 	{ CTL_INT,	KERN_PANIC_ON_NMI,		"panic_on_unrecovered_nmi" },
+	{ CTL_INT,	KERN_PANIC_ON_STACKOVERFLOW,	"panic_on_stackoverflow" },
 	{}
 };
 

Re: [RFC PATCH 3/5] x86: add a sysctl parameter to panic on stack overflow

[Konrad Rzeszutek Wilk]

On Mon, Nov 07, 2011 at 02:53:01PM +0900, Mitsuo Hayasaka wrote:
> This patch adds kernel.panic_on_stackoverflow sysctl parameter,
> which can cause a panic when detecting the stack overflow.

Why would we want to panic?
> 
> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
> Cc: Randy Dunlap <rdunlap@xenotime.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
> 
>  Documentation/sysctl/kernel.txt |   13 +++++++++++++
>  arch/x86/kernel/irq_64.c        |    2 ++
>  include/linux/kernel.h          |    1 +
>  include/linux/sysctl.h          |    1 +
>  kernel/sysctl.c                 |    9 +++++++++
>  kernel/sysctl_binary.c          |    1 +
>  6 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 704e474..eb8d263 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -48,6 +48,7 @@ show up in /proc/sys/kernel:
>  - panic
>  - panic_on_oops
>  - panic_on_unrecovered_nmi
> +- panic_on_stackoverflow
>  - pid_max
>  - powersave-nap               [ PPC only ]
>  - printk
> @@ -385,6 +386,18 @@ Controls the kernel's behaviour when an oops or BUG is encountered.
>  
>  ==============================================================
>  
> +panic_on_stackoverflow:
> +
> +Controls the kernel's behaviour when detecting stack overflow.
> +This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
> +
> +0: try to continue operation.
> +
> +1: panic immediately.
> +
> +==============================================================
> +
> +
>  pid_max:
>  
>  PID allocation wrap value.  When the kernel's next PID value
> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
> index 530cfd0..d720813 100644
> --- a/arch/x86/kernel/irq_64.c
> +++ b/arch/x86/kernel/irq_64.c
> @@ -20,6 +20,8 @@
>  #include <asm/idle.h>
>  #include <asm/apic.h>
>  
> +int sysctl_panic_on_stackoverflow;

__read_mostly

> +
>  DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat);
>  EXPORT_PER_CPU_SYMBOL(irq_stat);
>  
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 8eefcf7..19b3595 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -339,6 +339,7 @@ extern int panic_timeout;
>  extern int panic_on_oops;
>  extern int panic_on_unrecovered_nmi;
>  extern int panic_on_io_nmi;
> +extern int sysctl_panic_on_stackoverflow;
>  extern const char *print_tainted(void);
>  extern void add_taint(unsigned flag);
>  extern int test_taint(unsigned flag);
> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
> index 9a1ec10..4b9dc3d 100644
> --- a/include/linux/sysctl.h
> +++ b/include/linux/sysctl.h
> @@ -153,6 +153,7 @@ enum
>  	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
>  	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
>  	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
> +	KERN_PANIC_ON_STACKOVERFLOW = 77, /* int: panic on stack overflow */
>  };
>  
>  
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 11d65b5..08a18f9 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -792,6 +792,15 @@ static struct ctl_table kern_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec,
>  	},
> +#if defined(CONFIG_DEBUG_STACKOVERFLOW)
> +	{
> +		.procname	= "panic_on_stackoverflow",
> +		.data		= &sysctl_panic_on_stackoverflow,
> +		.maxlen		= sizeof(int),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec,
> +	},
> +#endif
>  	{
>  		.procname	= "bootloader_type",
>  		.data		= &bootloader_type,
> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
> index 6318b51..24fc654 100644
> --- a/kernel/sysctl_binary.c
> +++ b/kernel/sysctl_binary.c
> @@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
>  	{ CTL_INT,	KERN_COMPAT_LOG,		"compat-log" },
>  	{ CTL_INT,	KERN_MAX_LOCK_DEPTH,		"max_lock_depth" },
>  	{ CTL_INT,	KERN_PANIC_ON_NMI,		"panic_on_unrecovered_nmi" },
> +	{ CTL_INT,	KERN_PANIC_ON_STACKOVERFLOW,	"panic_on_stackoverflow" },

This shouldn't be wrapped in the #ifdef?

>  	{}
>  };
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 3/5] x86: add a sysctl parameter to panic on stack overflow

[HAYASAKA Mitsuo]

(2011/11/11 4:55), Konrad Rzeszutek Wilk wrote:
> On Mon, Nov 07, 2011 at 02:53:01PM +0900, Mitsuo Hayasaka wrote:
>> This patch adds kernel.panic_on_stackoverflow sysctl parameter,
>> which can cause a panic when detecting the stack overflow.
> 
> Why would we want to panic?

This is because in general a stack overflow may corrupt data and
the additional corruption may occur due to reading of them unless
systems stop. This option is useful for systems that need a high
reliability, although it is disabled by default.

In addition, it is also useful for analyzing the reason why it
occurred using kdump which is a crash dumping mechanism.

>>
>> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
>> Cc: Randy Dunlap <rdunlap@xenotime.net>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> ---
>>
>>  Documentation/sysctl/kernel.txt |   13 +++++++++++++
>>  arch/x86/kernel/irq_64.c        |    2 ++
>>  include/linux/kernel.h          |    1 +
>>  include/linux/sysctl.h          |    1 +
>>  kernel/sysctl.c                 |    9 +++++++++
>>  kernel/sysctl_binary.c          |    1 +
>>  6 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
>> index 704e474..eb8d263 100644
>> --- a/Documentation/sysctl/kernel.txt
>> +++ b/Documentation/sysctl/kernel.txt
>> @@ -48,6 +48,7 @@ show up in /proc/sys/kernel:
>>  - panic
>>  - panic_on_oops
>>  - panic_on_unrecovered_nmi
>> +- panic_on_stackoverflow
>>  - pid_max
>>  - powersave-nap               [ PPC only ]
>>  - printk
>> @@ -385,6 +386,18 @@ Controls the kernel's behaviour when an oops or BUG is encountered.
>>  
>>  ==============================================================
>>  
>> +panic_on_stackoverflow:
>> +
>> +Controls the kernel's behaviour when detecting stack overflow.
>> +This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
>> +
>> +0: try to continue operation.
>> +
>> +1: panic immediately.
>> +
>> +==============================================================
>> +
>> +
>>  pid_max:
>>  
>>  PID allocation wrap value.  When the kernel's next PID value
>> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
>> index 530cfd0..d720813 100644
>> --- a/arch/x86/kernel/irq_64.c
>> +++ b/arch/x86/kernel/irq_64.c
>> @@ -20,6 +20,8 @@
>>  #include <asm/idle.h>
>>  #include <asm/apic.h>
>>  
>> +int sysctl_panic_on_stackoverflow;
> 
> __read_mostly
> 
>> +
>>  DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat);
>>  EXPORT_PER_CPU_SYMBOL(irq_stat);
>>  
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 8eefcf7..19b3595 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -339,6 +339,7 @@ extern int panic_timeout;
>>  extern int panic_on_oops;
>>  extern int panic_on_unrecovered_nmi;
>>  extern int panic_on_io_nmi;
>> +extern int sysctl_panic_on_stackoverflow;
>>  extern const char *print_tainted(void);
>>  extern void add_taint(unsigned flag);
>>  extern int test_taint(unsigned flag);
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index 9a1ec10..4b9dc3d 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -153,6 +153,7 @@ enum
>>  	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
>>  	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
>>  	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
>> +	KERN_PANIC_ON_STACKOVERFLOW = 77, /* int: panic on stack overflow */
>>  };
>>  
>>  
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 11d65b5..08a18f9 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -792,6 +792,15 @@ static struct ctl_table kern_table[] = {
>>  		.mode		= 0644,
>>  		.proc_handler	= proc_dointvec,
>>  	},
>> +#if defined(CONFIG_DEBUG_STACKOVERFLOW)
>> +	{
>> +		.procname	= "panic_on_stackoverflow",
>> +		.data		= &sysctl_panic_on_stackoverflow,
>> +		.maxlen		= sizeof(int),
>> +		.mode		= 0644,
>> +		.proc_handler	= proc_dointvec,
>> +	},
>> +#endif
>>  	{
>>  		.procname	= "bootloader_type",
>>  		.data		= &bootloader_type,
>> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
>> index 6318b51..24fc654 100644
>> --- a/kernel/sysctl_binary.c
>> +++ b/kernel/sysctl_binary.c
>> @@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
>>  	{ CTL_INT,	KERN_COMPAT_LOG,		"compat-log" },
>>  	{ CTL_INT,	KERN_MAX_LOCK_DEPTH,		"max_lock_depth" },
>>  	{ CTL_INT,	KERN_PANIC_ON_NMI,		"panic_on_unrecovered_nmi" },
>> +	{ CTL_INT,	KERN_PANIC_ON_STACKOVERFLOW,	"panic_on_stackoverflow" },
> 
> This shouldn't be wrapped in the #ifdef?
> 
>>  	{}
>>  };
>>  
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC PATCH 3/5] x86: add a sysctl parameter to panic on stack overflow

[HAYASAKA Mitsuo]

Hi Konrad,

>> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
>> index 530cfd0..d720813 100644
>> --- a/arch/x86/kernel/irq_64.c
>> +++ b/arch/x86/kernel/irq_64.c
>> @@ -20,6 +20,8 @@
>>  #include <asm/idle.h>
>>  #include <asm/apic.h>
>>  
>> +int sysctl_panic_on_stackoverflow;
> __read_mostly

I don't mind if I add __read_mostly here, but I think it's not
necessary because this variable is seldom read as well.


>> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
>> index 6318b51..24fc654 100644
>> --- a/kernel/sysctl_binary.c
>> +++ b/kernel/sysctl_binary.c
>> @@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
>>  	{ CTL_INT,	KERN_COMPAT_LOG,		"compat-log" },
>>  	{ CTL_INT,	KERN_MAX_LOCK_DEPTH,		"max_lock_depth" },
>>  	{ CTL_INT,	KERN_PANIC_ON_NMI,		"panic_on_unrecovered_nmi" },
>> +	{ CTL_INT,	KERN_PANIC_ON_STACKOVERFLOW,	"panic_on_stackoverflow" },
> This shouldn't be wrapped in the #ifdef?
>

It seems that we do not need to add this entry to bin_kern_table[]
according to the following information and patch series:

[Removing binary sysctl]
	http://lwn.net/Articles/361453/

[Removal of binary sysctl support]
	http://lwn.net/Articles/361001/

So, I'm going to remove this changes for sysctl.h and sysctl_binary.c.

Thanks


(2011/11/11 4:55), Konrad Rzeszutek Wilk wrote:
> On Mon, Nov 07, 2011 at 02:53:01PM +0900, Mitsuo Hayasaka wrote:
>> This patch adds kernel.panic_on_stackoverflow sysctl parameter,
>> which can cause a panic when detecting the stack overflow.
> 
> Why would we want to panic?
>>
>> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
>> Cc: Randy Dunlap <rdunlap@xenotime.net>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> ---
>>
>>  Documentation/sysctl/kernel.txt |   13 +++++++++++++
>>  arch/x86/kernel/irq_64.c        |    2 ++
>>  include/linux/kernel.h          |    1 +
>>  include/linux/sysctl.h          |    1 +
>>  kernel/sysctl.c                 |    9 +++++++++
>>  kernel/sysctl_binary.c          |    1 +
>>  6 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
>> index 704e474..eb8d263 100644
>> --- a/Documentation/sysctl/kernel.txt
>> +++ b/Documentation/sysctl/kernel.txt
>> @@ -48,6 +48,7 @@ show up in /proc/sys/kernel:
>>  - panic
>>  - panic_on_oops
>>  - panic_on_unrecovered_nmi
>> +- panic_on_stackoverflow
>>  - pid_max
>>  - powersave-nap               [ PPC only ]
>>  - printk
>> @@ -385,6 +386,18 @@ Controls the kernel's behaviour when an oops or BUG is encountered.
>>  
>>  ==============================================================
>>  
>> +panic_on_stackoverflow:
>> +
>> +Controls the kernel's behaviour when detecting stack overflow.
>> +This file shows up if CONFIG_DEBUG_STACKOVERFLOW is enabled.
>> +
>> +0: try to continue operation.
>> +
>> +1: panic immediately.
>> +
>> +==============================================================
>> +
>> +
>>  pid_max:
>>  
>>  PID allocation wrap value.  When the kernel's next PID value
>> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
>> index 530cfd0..d720813 100644
>> --- a/arch/x86/kernel/irq_64.c
>> +++ b/arch/x86/kernel/irq_64.c
>> @@ -20,6 +20,8 @@
>>  #include <asm/idle.h>
>>  #include <asm/apic.h>
>>  
>> +int sysctl_panic_on_stackoverflow;
> 
> __read_mostly
> 
>> +
>>  DEFINE_PER_CPU_SHARED_ALIGNED(irq_cpustat_t, irq_stat);
>>  EXPORT_PER_CPU_SYMBOL(irq_stat);
>>  
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index 8eefcf7..19b3595 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -339,6 +339,7 @@ extern int panic_timeout;
>>  extern int panic_on_oops;
>>  extern int panic_on_unrecovered_nmi;
>>  extern int panic_on_io_nmi;
>> +extern int sysctl_panic_on_stackoverflow;
>>  extern const char *print_tainted(void);
>>  extern void add_taint(unsigned flag);
>>  extern int test_taint(unsigned flag);
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index 9a1ec10..4b9dc3d 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -153,6 +153,7 @@ enum
>>  	KERN_MAX_LOCK_DEPTH=74, /* int: rtmutex's maximum lock depth */
>>  	KERN_NMI_WATCHDOG=75, /* int: enable/disable nmi watchdog */
>>  	KERN_PANIC_ON_NMI=76, /* int: whether we will panic on an unrecovered */
>> +	KERN_PANIC_ON_STACKOVERFLOW = 77, /* int: panic on stack overflow */
>>  };
>>  
>>  
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 11d65b5..08a18f9 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -792,6 +792,15 @@ static struct ctl_table kern_table[] = {
>>  		.mode		= 0644,
>>  		.proc_handler	= proc_dointvec,
>>  	},
>> +#if defined(CONFIG_DEBUG_STACKOVERFLOW)
>> +	{
>> +		.procname	= "panic_on_stackoverflow",
>> +		.data		= &sysctl_panic_on_stackoverflow,
>> +		.maxlen		= sizeof(int),
>> +		.mode		= 0644,
>> +		.proc_handler	= proc_dointvec,
>> +	},
>> +#endif
>>  	{
>>  		.procname	= "bootloader_type",
>>  		.data		= &bootloader_type,
>> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
>> index 6318b51..24fc654 100644
>> --- a/kernel/sysctl_binary.c
>> +++ b/kernel/sysctl_binary.c
>> @@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
>>  	{ CTL_INT,	KERN_COMPAT_LOG,		"compat-log" },
>>  	{ CTL_INT,	KERN_MAX_LOCK_DEPTH,		"max_lock_depth" },
>>  	{ CTL_INT,	KERN_PANIC_ON_NMI,		"panic_on_unrecovered_nmi" },
>> +	{ CTL_INT,	KERN_PANIC_ON_STACKOVERFLOW,	"panic_on_stackoverflow" },
> 
> This shouldn't be wrapped in the #ifdef?
> 
>>  	{}
>>  };
>>  
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [RFC PATCH 3/5] x86: add a sysctl parameter to panic on stack overflow

[Konrad Rzeszutek Wilk]

On Thu, Nov 17, 2011 at 04:11:07PM +0900, HAYASAKA Mitsuo wrote:
> Hi Konrad,
> 
> >> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
> >> index 530cfd0..d720813 100644
> >> --- a/arch/x86/kernel/irq_64.c
> >> +++ b/arch/x86/kernel/irq_64.c
> >> @@ -20,6 +20,8 @@
> >>  #include <asm/idle.h>
> >>  #include <asm/apic.h>
> >>  
> >> +int sysctl_panic_on_stackoverflow;
> > __read_mostly
> 
> I don't mind if I add __read_mostly here, but I think it's not
> necessary because this variable is seldom read as well.

The definition of __read_mostly is that it moves this variable to a cold cache area.
Which is what you want.

> 
> 
> >> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
> >> index 6318b51..24fc654 100644
> >> --- a/kernel/sysctl_binary.c
> >> +++ b/kernel/sysctl_binary.c
> >> @@ -137,6 +137,7 @@ static const struct bin_table bin_kern_table[] = {
> >>  	{ CTL_INT,	KERN_COMPAT_LOG,		"compat-log" },
> >>  	{ CTL_INT,	KERN_MAX_LOCK_DEPTH,		"max_lock_depth" },
> >>  	{ CTL_INT,	KERN_PANIC_ON_NMI,		"panic_on_unrecovered_nmi" },
> >> +	{ CTL_INT,	KERN_PANIC_ON_STACKOVERFLOW,	"panic_on_stackoverflow" },
> > This shouldn't be wrapped in the #ifdef?
> >
> 
> It seems that we do not need to add this entry to bin_kern_table[]
> according to the following information and patch series:
> 
> [Removing binary sysctl]
> 	http://lwn.net/Articles/361453/
> 
> [Removal of binary sysctl support]
> 	http://lwn.net/Articles/361001/
> 
> So, I'm going to remove this changes for sysctl.h and sysctl_binary.c.

<nods>

Re: [RFC PATCH 3/5] x86: add a sysctl parameter to panic on stack overflow

[H. Peter Anvin]

On 11/17/2011 08:00 AM, Konrad Rzeszutek Wilk wrote:
>>
>> I don't mind if I add __read_mostly here, but I think it's not
>> necessary because this variable is seldom read as well.
> 
> The definition of __read_mostly is that it moves this variable to a cold cache area.
> Which is what you want.
> 

Not really.  __read_mostly moves a variable to an area that is expected
to spent most its time in the cache-shared state and rarely be
invalidated.  It does NOT mean that it is cold.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[RFC PATCH 4/5] x86: panic on detection of stack overflow

[Mitsuo Hayasaka]

Currently, messages are just output on the detection of stack overflow,
which is not sufficient for enterprise systems since it may corrupt data.
To enhance reliability, it is required to stop the systems.

This patch causes a panic according to a sysctl parameter
panic_on_stackoverflow when detecting it. It is disabled by default.

Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---

 arch/x86/kernel/irq_32.c |    2 ++
 arch/x86/kernel/irq_64.c |   16 +++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
index 7209070..e16e99eb 100644
--- a/arch/x86/kernel/irq_32.c
+++ b/arch/x86/kernel/irq_32.c
@@ -43,6 +43,8 @@ static void print_stack_overflow(void)
 {
 	printk(KERN_WARNING "low stack detected by irq handler\n");
 	dump_stack();
+	if (sysctl_panic_on_stackoverflow)
+		panic("low stack detected by irq handler - check messages\n");
 }
 
 #else
diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
index d720813..f7baedd 100644
--- a/arch/x86/kernel/irq_64.c
+++ b/arch/x86/kernel/irq_64.c
@@ -69,14 +69,20 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 		current->comm, curbase, regs->sp,
 		irq_stack_top, irq_stack_bottom,
 		estack_top, estack_bottom);
+	if (sysctl_panic_on_stackoverflow)
+		panic("low stack detected by irq handler - check messages\n");
 #else
-	WARN_ONCE(regs->sp >= curbase &&
-		  regs->sp <= curbase + THREAD_SIZE &&
-		  regs->sp <  curbase + sizeof(struct thread_info) +
-					sizeof(struct pt_regs) + 128,
-
+	if (regs->sp >= curbase &&
+		regs->sp <= curbase + THREAD_SIZE &&
+		regs->sp <  curbase + sizeof(struct thread_info) +
+					sizeof(struct pt_regs) + 128) {
+		WARN_ONCE(1,
 		  "do_IRQ: %s near stack overflow (cur:%Lx,sp:%lx)\n",
 			current->comm, curbase, regs->sp);
+		if (sysctl_panic_on_stackoverflow)
+			panic("low stack detected by irq handler - check messages\n");
+	}
+
 #endif /* CONFIG_DEBUG_STACKOVERFLOW_DETAIL */
 #endif
 }

Re: [RFC PATCH 4/5] x86: panic on detection of stack overflow

[Konrad Rzeszutek Wilk]

On Mon, Nov 07, 2011 at 02:53:08PM +0900, Mitsuo Hayasaka wrote:
> Currently, messages are just output on the detection of stack overflow,
> which is not sufficient for enterprise systems since it may corrupt data.
> To enhance reliability, it is required to stop the systems.

Why not just make the stack_overflow_check() return a value that it should
not handle the IRQ and perhaps silence (disable_chip) the IRQ line?

That will still let the system run, albeit .. without certain parts
not working right.. So perhaps re-enable the chip later on?

Or is there really no way to recover from this?
> 
> This patch causes a panic according to a sysctl parameter
> panic_on_stackoverflow when detecting it. It is disabled by default.
> 
> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
> 
>  arch/x86/kernel/irq_32.c |    2 ++
>  arch/x86/kernel/irq_64.c |   16 +++++++++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
> index 7209070..e16e99eb 100644
> --- a/arch/x86/kernel/irq_32.c
> +++ b/arch/x86/kernel/irq_32.c
> @@ -43,6 +43,8 @@ static void print_stack_overflow(void)
>  {
>  	printk(KERN_WARNING "low stack detected by irq handler\n");
>  	dump_stack();
> +	if (sysctl_panic_on_stackoverflow)
> +		panic("low stack detected by irq handler - check messages\n");
>  }
>  
>  #else
> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
> index d720813..f7baedd 100644
> --- a/arch/x86/kernel/irq_64.c
> +++ b/arch/x86/kernel/irq_64.c
> @@ -69,14 +69,20 @@ static inline void stack_overflow_check(struct pt_regs *regs)
>  		current->comm, curbase, regs->sp,
>  		irq_stack_top, irq_stack_bottom,
>  		estack_top, estack_bottom);
> +	if (sysctl_panic_on_stackoverflow)
> +		panic("low stack detected by irq handler - check messages\n");
>  #else
> -	WARN_ONCE(regs->sp >= curbase &&
> -		  regs->sp <= curbase + THREAD_SIZE &&
> -		  regs->sp <  curbase + sizeof(struct thread_info) +
> -					sizeof(struct pt_regs) + 128,
> -
> +	if (regs->sp >= curbase &&
> +		regs->sp <= curbase + THREAD_SIZE &&
> +		regs->sp <  curbase + sizeof(struct thread_info) +
> +					sizeof(struct pt_regs) + 128) {
> +		WARN_ONCE(1,
>  		  "do_IRQ: %s near stack overflow (cur:%Lx,sp:%lx)\n",
>  			current->comm, curbase, regs->sp);
> +		if (sysctl_panic_on_stackoverflow)
> +			panic("low stack detected by irq handler - check messages\n");
> +	}
> +
>  #endif /* CONFIG_DEBUG_STACKOVERFLOW_DETAIL */
>  #endif
>  }
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH 4/5] x86: panic on detection of stack overflow

[HAYASAKA Mitsuo]

(2011/11/11 4:59), Konrad Rzeszutek Wilk wrote:
> On Mon, Nov 07, 2011 at 02:53:08PM +0900, Mitsuo Hayasaka wrote:
>> Currently, messages are just output on the detection of stack overflow,
>> which is not sufficient for enterprise systems since it may corrupt data.
>> To enhance reliability, it is required to stop the systems.
> 
> Why not just make the stack_overflow_check() return a value that it should
> not handle the IRQ and perhaps silence (disable_chip) the IRQ line?
> 
> That will still let the system run, albeit .. without certain parts
> not working right.. So perhaps re-enable the chip later on?
> 
> Or is there really no way to recover from this?

I understood that you mentioned the overflow handling of IRQ stack, right?

I think it is interesting but in this patch I'd like to focus on
causing a panic for the overflows of kernel, IRQ and exception stacks.
Of course, I will consider it as the future works.

This option is enabled only if the sysctl parameter is changed
in the same manner as other panic_on_XXX parameters.

Also, I have concerned about the additional corruption caused by
reading of the corrupted data due to the overflows of kernel, IRQ
and exception stacks. This may happen unless systems stop, and
is unacceptable for systems that need a high reliability.


>>
>> This patch causes a panic according to a sysctl parameter
>> panic_on_stackoverflow when detecting it. It is disabled by default.
>>
>> Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> ---
>>
>>  arch/x86/kernel/irq_32.c |    2 ++
>>  arch/x86/kernel/irq_64.c |   16 +++++++++++-----
>>  2 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
>> index 7209070..e16e99eb 100644
>> --- a/arch/x86/kernel/irq_32.c
>> +++ b/arch/x86/kernel/irq_32.c
>> @@ -43,6 +43,8 @@ static void print_stack_overflow(void)
>>  {
>>  	printk(KERN_WARNING "low stack detected by irq handler\n");
>>  	dump_stack();
>> +	if (sysctl_panic_on_stackoverflow)
>> +		panic("low stack detected by irq handler - check messages\n");
>>  }
>>  
>>  #else
>> diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
>> index d720813..f7baedd 100644
>> --- a/arch/x86/kernel/irq_64.c
>> +++ b/arch/x86/kernel/irq_64.c
>> @@ -69,14 +69,20 @@ static inline void stack_overflow_check(struct pt_regs *regs)
>>  		current->comm, curbase, regs->sp,
>>  		irq_stack_top, irq_stack_bottom,
>>  		estack_top, estack_bottom);
>> +	if (sysctl_panic_on_stackoverflow)
>> +		panic("low stack detected by irq handler - check messages\n");
>>  #else
>> -	WARN_ONCE(regs->sp >= curbase &&
>> -		  regs->sp <= curbase + THREAD_SIZE &&
>> -		  regs->sp <  curbase + sizeof(struct thread_info) +
>> -					sizeof(struct pt_regs) + 128,
>> -
>> +	if (regs->sp >= curbase &&
>> +		regs->sp <= curbase + THREAD_SIZE &&
>> +		regs->sp <  curbase + sizeof(struct thread_info) +
>> +					sizeof(struct pt_regs) + 128) {
>> +		WARN_ONCE(1,
>>  		  "do_IRQ: %s near stack overflow (cur:%Lx,sp:%lx)\n",
>>  			current->comm, curbase, regs->sp);
>> +		if (sysctl_panic_on_stackoverflow)
>> +			panic("low stack detected by irq handler - check messages\n");
>> +	}
>> +
>>  #endif /* CONFIG_DEBUG_STACKOVERFLOW_DETAIL */
>>  #endif
>>  }
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 

[RFC PATCH 5/5] x86: change range of stack overflow checking

[Mitsuo Hayasaka]

The original stack overflow checking checks if a stack pointer is
less than or equal to (curbase + THREAD_SIZE) as the 2nd condition.
However, it is not necessary since the 3rd one checks if the pointer
is lower than available stack size. This patch removes the 2nd one.

Signed-off-by: Mitsuo Hayasaka <mitsuo.hayasaka.hu@hitachi.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---

 arch/x86/kernel/irq_64.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/irq_64.c b/arch/x86/kernel/irq_64.c
index f7baedd..d85b338 100644
--- a/arch/x86/kernel/irq_64.c
+++ b/arch/x86/kernel/irq_64.c
@@ -73,7 +73,6 @@ static inline void stack_overflow_check(struct pt_regs *regs)
 		panic("low stack detected by irq handler - check messages\n");
 #else
 	if (regs->sp >= curbase &&
-		regs->sp <= curbase + THREAD_SIZE &&
 		regs->sp <  curbase + sizeof(struct thread_info) +
 					sizeof(struct pt_regs) + 128) {
 		WARN_ONCE(1,

Re: [RFC PATCH 0/5] x86: check stack overflows more reliably

[Pekka Enberg]

On Mon, Nov 7, 2011 at 7:51 AM, Mitsuo Hayasaka
<mitsuo.hayasaka.hu@hitachi.com> wrote:
> (2) check stack overflow in detail
>    Currently, only kernel stack is checked for the overflow,
>    which is not sufficient for enterprise systems. To enhance
>    reliability, expand stack overflow checking to IRQ and
>    exception stacks optionally. This is disabled by default
>    in Kconfig.

This sounds useful. What's the reason for not enabling this by
default? Performance regressions?

                        Pekka

Re: [RFC PATCH 0/5] x86: check stack overflows more reliably

[HAYASAKA Mitsuo]

Hi Pekka,

Thank you for your comments.

(2011/11/07 16:00), Pekka Enberg wrote:
> On Mon, Nov 7, 2011 at 7:51 AM, Mitsuo Hayasaka
> <mitsuo.hayasaka.hu@hitachi.com> wrote:
>> (2) check stack overflow in detail
>>    Currently, only kernel stack is checked for the overflow,
>>    which is not sufficient for enterprise systems. To enhance
>>    reliability, expand stack overflow checking to IRQ and
>>    exception stacks optionally. This is disabled by default
>>    in Kconfig.
> 
> This sounds useful. What's the reason for not enabling this by
> default? Performance regressions?

I'm worried about performance regressions because this patch checks 
a stack overflow in detail.

However, I guess there is no problem for enabling it by default 
since this option is for debug and appears only if a DEBUG_STACKOVERFLOW
option is enabled.

So, I'd like to send the revised patch if it does not have any further problem.


>                         Pekka
> 
> 

Re: [RFC PATCH 0/5] x86: check stack overflows more reliably

[Jason Baron]

On Tue, Nov 08, 2011 at 04:34:28PM +0900, HAYASAKA Mitsuo wrote:
> Hi Pekka,
> 
> Thank you for your comments.
> 
> (2011/11/07 16:00), Pekka Enberg wrote:
> > On Mon, Nov 7, 2011 at 7:51 AM, Mitsuo Hayasaka
> > <mitsuo.hayasaka.hu@hitachi.com> wrote:
> >> (2) check stack overflow in detail
> >>    Currently, only kernel stack is checked for the overflow,
> >>    which is not sufficient for enterprise systems. To enhance
> >>    reliability, expand stack overflow checking to IRQ and
> >>    exception stacks optionally. This is disabled by default
> >>    in Kconfig.
> > 
> > This sounds useful. What's the reason for not enabling this by
> > default? Performance regressions?
> 
> I'm worried about performance regressions because this patch checks 
> a stack overflow in detail.
> 
> However, I guess there is no problem for enabling it by default 
> since this option is for debug and appears only if a DEBUG_STACKOVERFLOW
> option is enabled.
> 
> So, I'd like to send the revised patch if it does not have any further problem.
> 
> 

Another thought might be to make stack_overflow_check() depend on a jump
label. Its not something that going to be switch on/off often, and then perhaps
we wouldn't even need DEBUG_STACKOVERFLOW...It seems like a good
use-case to me.

Thanks,

-Jason

Re: [RFC PATCH 0/5] x86: check stack overflows more reliably

[HAYASAKA Mitsuo]

Hi Jason,

> Another thought might be to make stack_overflow_check() depend on a jump
> label. Its not something that going to be switch on/off often, and then perhaps
> we wouldn't even need DEBUG_STACKOVERFLOW...It seems like a good
> use-case to me.

It is interesting to use a jump label for stack overflow checking...
However, I'd like to implement this detail-check simply using
DEBUG_STACKOVERFLOW option because I guess stack_overflow_check() will
be seldom switched on/off after the system operation starts, as you said.

In addition, I will change the default overflow checking to the
detail-check instead of the original one if the option is enabled in
Kconfig. This is because it turned out that the additional checking
overhead is negligible (about 17 cycles) from the evaluation below.


[Evaluation]

The performance of the detail-check was compared to original one
which checks kernel stack only, on the following conditions.

- Measure the worst performance using tsc.
  In the detail-check, all stack type were checked for every IRQ
  even if the stack pointer pointed to all available stacks.
  That is, the patch was changed a little for this evaluation.
- Calculate the average from the 30,000 IRQ evaluations.

The results show the performance regression of the detail-check
for a IRQ is 17 cycles compared to the original one.


	| Original | Detail Check |
-----------------------------------
Average	|    49    |      66      |
(cycles)

I think this overhead can be ignored.

Thanks

(2011/11/18 1:59), Jason Baron wrote:
> On Tue, Nov 08, 2011 at 04:34:28PM +0900, HAYASAKA Mitsuo wrote:
>> Hi Pekka,
>>
>> Thank you for your comments.
>>
>> (2011/11/07 16:00), Pekka Enberg wrote:
>>> On Mon, Nov 7, 2011 at 7:51 AM, Mitsuo Hayasaka
>>> <mitsuo.hayasaka.hu@hitachi.com> wrote:
>>>> (2) check stack overflow in detail
>>>>    Currently, only kernel stack is checked for the overflow,
>>>>    which is not sufficient for enterprise systems. To enhance
>>>>    reliability, expand stack overflow checking to IRQ and
>>>>    exception stacks optionally. This is disabled by default
>>>>    in Kconfig.
>>>
>>> This sounds useful. What's the reason for not enabling this by
>>> default? Performance regressions?
>>
>> I'm worried about performance regressions because this patch checks 
>> a stack overflow in detail.
>>
>> However, I guess there is no problem for enabling it by default 
>> since this option is for debug and appears only if a DEBUG_STACKOVERFLOW
>> option is enabled.
>>
>> So, I'd like to send the revised patch if it does not have any further problem.
>>
>>
> 
> Another thought might be to make stack_overflow_check() depend on a jump
> label. Its not something that going to be switch on/off often, and then perhaps
> we wouldn't even need DEBUG_STACKOVERFLOW...It seems like a good
> use-case to me.
> 
> Thanks,
> 
> -Jason
>