[PATCH] x86, traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP.

[PATCH] x86, traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP.

[Wang Nan]

Before this patch early_trap_init() installs DEBUG_STACK for X86_TRAP_BP
and X86_TRAP_DB. However, DEBUG_STACK doesn't work correctly until
cpu_init() <-- trap_init().

This patch passes 0 to set_intr_gate_ist() and
set_system_intr_gate_ist() instead of DEBUG_STACK to let it use same
stack as kernel, and installs DEBUG_STACK for them in trap_init().

As core runs at ring 0 between early_trap_init() and trap_init(), there
is no chance to get a bad stack before trap_init().

As NMI is also enabled in trap_init(), we don't need to care about
is_debug_stack() and related things used in arch/x86/kernel/nmi.c.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
---
 arch/x86/kernel/traps.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9d2073e..a9b8640 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -925,9 +925,16 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
 /* Set of traps needed for early debugging. */
 void __init early_trap_init(void)
 {
-	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
+	/*
+	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
+	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
+	 * runs at ring 0 so there should be impossible to hit a invalid
+	 * stack. Use original stack is enough. DEBUG_STACK will be
+	 * equipped after cpu_init() in trap_init().
+	 */
+	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
 	/* int3 can be called from all */
-	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
+	set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
 #ifdef CONFIG_X86_32
 	set_intr_gate(X86_TRAP_PF, page_fault);
 #endif
@@ -1005,6 +1012,15 @@ void __init trap_init(void)
 	 */
 	cpu_init();
 
+	/*
+	 * X86_TRAP_DB and X86_TRAP_BP have been setup
+	 * in early_trap_init(). However, DEBUG_STACK works only after
+	 * cpu_init() load TSS. See comments in early_trap_init().
+	 */
+	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
+	/* int3 can be called from all */
+	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
+
 	x86_init.irqs.trap_init();
 
 #ifdef CONFIG_X86_64
-- 
1.8.4

Re: [PATCH] x86, traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP.

[Steven Rostedt]

On Thu, 26 Feb 2015 11:57:56 +0800
Wang Nan <wangnan0@huawei.com> wrote:

> Before this patch early_trap_init() installs DEBUG_STACK for X86_TRAP_BP
> and X86_TRAP_DB. However, DEBUG_STACK doesn't work correctly until
> cpu_init() <-- trap_init().
> 
> This patch passes 0 to set_intr_gate_ist() and
> set_system_intr_gate_ist() instead of DEBUG_STACK to let it use same
> stack as kernel, and installs DEBUG_STACK for them in trap_init().
> 
> As core runs at ring 0 between early_trap_init() and trap_init(), there
> is no chance to get a bad stack before trap_init().
> 
> As NMI is also enabled in trap_init(), we don't need to care about
> is_debug_stack() and related things used in arch/x86/kernel/nmi.c.

Looks fine to me, except for some grammar issues in the comments.

> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  arch/x86/kernel/traps.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 9d2073e..a9b8640 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -925,9 +925,16 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
>  /* Set of traps needed for early debugging. */
>  void __init early_trap_init(void)
>  {
> -	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
> +	/*
> +	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
> +	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
> +	 * runs at ring 0 so there should be impossible to hit a invalid

 "so it should be impossible", although I might prefer, "it is
 impossible to", but nothing is ever guaranteed.

 "an invalid"


> +	 * stack. Use original stack is enough. DEBUG_STACK will be

 "Using the original stack works well enough at this early stage."

> +	 * equipped after cpu_init() in trap_init().
> +	 */
> +	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
>  	/* int3 can be called from all */
> -	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
> +	set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
>  #ifdef CONFIG_X86_32
>  	set_intr_gate(X86_TRAP_PF, page_fault);
>  #endif
> @@ -1005,6 +1012,15 @@ void __init trap_init(void)
>  	 */
>  	cpu_init();
>  
> +	/*
> +	 * X86_TRAP_DB and X86_TRAP_BP have been setup
> +	 * in early_trap_init(). However, DEBUG_STACK works only after
> +	 * cpu_init() load TSS. See comments in early_trap_init().

  "cpu_init() loads TSS"

-- Steve

> +	 */
> +	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
> +	/* int3 can be called from all */
> +	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
> +
>  	x86_init.irqs.trap_init();
>  
>  #ifdef CONFIG_X86_64

Re: [PATCH] x86, traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP.

[Masami Hiramatsu]

(2015/02/26 12:57), Wang Nan wrote:
> Before this patch early_trap_init() installs DEBUG_STACK for X86_TRAP_BP
> and X86_TRAP_DB. However, DEBUG_STACK doesn't work correctly until
> cpu_init() <-- trap_init().
> 
> This patch passes 0 to set_intr_gate_ist() and
> set_system_intr_gate_ist() instead of DEBUG_STACK to let it use same
> stack as kernel, and installs DEBUG_STACK for them in trap_init().
> 
> As core runs at ring 0 between early_trap_init() and trap_init(), there
> is no chance to get a bad stack before trap_init().

Thanks for finding the problem on it! :)
Agreed, until initializing DEBUG_STACK, it should not be used.

> 
> As NMI is also enabled in trap_init(), we don't need to care about
> is_debug_stack() and related things used in arch/x86/kernel/nmi.c.

Looks good to me, at least its code side. Please fix the comment according
to Steven's suggestion.
And feel free to add my reviewed-by for this.

Reviewed-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Thank you,

> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> ---
>  arch/x86/kernel/traps.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 9d2073e..a9b8640 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -925,9 +925,16 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
>  /* Set of traps needed for early debugging. */
>  void __init early_trap_init(void)
>  {
> -	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
> +	/*
> +	 * Don't set ist to DEBUG_STACK as it doesn't work until TSS is
> +	 * ready in cpu_init() <-- trap_init(). Before trap_init(), CPU
> +	 * runs at ring 0 so there should be impossible to hit a invalid
> +	 * stack. Use original stack is enough. DEBUG_STACK will be
> +	 * equipped after cpu_init() in trap_init().
> +	 */
> +	set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
>  	/* int3 can be called from all */
> -	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
> +	set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
>  #ifdef CONFIG_X86_32
>  	set_intr_gate(X86_TRAP_PF, page_fault);
>  #endif
> @@ -1005,6 +1012,15 @@ void __init trap_init(void)
>  	 */
>  	cpu_init();
>  
> +	/*
> +	 * X86_TRAP_DB and X86_TRAP_BP have been setup
> +	 * in early_trap_init(). However, DEBUG_STACK works only after
> +	 * cpu_init() load TSS. See comments in early_trap_init().
> +	 */
> +	set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
> +	/* int3 can be called from all */
> +	set_system_intr_gate_ist(X86_TRAP_BP, &int3, DEBUG_STACK);
> +
>  	x86_init.irqs.trap_init();
>  
>  #ifdef CONFIG_X86_64
> 


-- 
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com