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 <[email protected]>
Reviewed-by: Masami Hiramatsu <[email protected]>
---
v1 -> v2: Correct grammar issues in comments.
---
arch/x86/kernel/traps.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9d2073e..4281988 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -925,9 +925,17 @@ 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 it is impossible to hit an invalid stack.
+ * Using the original stack works well enough at this early
+ * stage. 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 +1013,15 @@ void __init trap_init(void)
*/
cpu_init();
+ /*
+ * X86_TRAP_DB and X86_TRAP_BP have been set
+ * in early_trap_init(). However, DEBUG_STACK works only after
+ * cpu_init() loads 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
Commit-ID: b4d8327024637cb2a1f7910dcb5d0ad7a096f473
Gitweb: http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473
Author: Wang Nan <[email protected]>
AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 26 Feb 2015 12:29:20 +0100
x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP
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 <[email protected]>
Reviewed-by: Masami Hiramatsu <[email protected]>
Acked-by: Steven Rostedt <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Cc: <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/traps.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9d2073e..4281988 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -925,9 +925,17 @@ 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 it is impossible to hit an invalid stack.
+ * Using the original stack works well enough at this early
+ * stage. 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 +1013,15 @@ void __init trap_init(void)
*/
cpu_init();
+ /*
+ * X86_TRAP_DB and X86_TRAP_BP have been set
+ * in early_trap_init(). However, DEBUG_STACK works only after
+ * cpu_init() loads 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
On Thu, Feb 26, 2015 at 5:12 AM, tip-bot for Wang Nan <[email protected]> wrote:
> Commit-ID: b4d8327024637cb2a1f7910dcb5d0ad7a096f473
> Gitweb: http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473
> Author: Wang Nan <[email protected]>
> AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800
> Committer: Ingo Molnar <[email protected]>
> CommitDate: Thu, 26 Feb 2015 12:29:20 +0100
>
> x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP
>
> 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().
>
Sorry, I'm late to the party. This patch, while technically correct
AFAICT, is really ugly, because the whole point is that it *doesn't*
use ist. In other words:
> + set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
That should be set_intr_gate(X86_TRAP_DB, &debug);
> /* 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);
Similarly, this should be set_system_gate.
> #ifdef CONFIG_X86_32
> set_intr_gate(X86_TRAP_PF, page_fault);
> #endif
> @@ -1005,6 +1013,15 @@ void __init trap_init(void)
> */
> cpu_init();
>
> + /*
> + * X86_TRAP_DB and X86_TRAP_BP have been set
> + * in early_trap_init(). However, DEBUG_STACK works only after
> + * cpu_init() loads TSS. See comments in early_trap_init().
It's not that DEBUG_STACK only works after the TSS is loaded; it's
that the IST mechanism only works after TSS is loaded.
--Andy
On 2015/2/26 23:12, Andy Lutomirski wrote:
> On Thu, Feb 26, 2015 at 5:12 AM, tip-bot for Wang Nan <[email protected]> wrote:
>> Commit-ID: b4d8327024637cb2a1f7910dcb5d0ad7a096f473
>> Gitweb: http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473
>> Author: Wang Nan <[email protected]>
>> AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800
>> Committer: Ingo Molnar <[email protected]>
>> CommitDate: Thu, 26 Feb 2015 12:29:20 +0100
>>
>> x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP
>>
>> 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().
>>
>
> Sorry, I'm late to the party. This patch, while technically correct
> AFAICT, is really ugly, because the whole point is that it *doesn't*
> use ist. In other words:
>
>> + set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
>
> That should be set_intr_gate(X86_TRAP_DB, &debug);
>
I considered this, but found that set_intr_gate() and set_intr_gate_ist(..., 0)
behaviors differently: set_intr_gate() -> _trace_set_gate() requires 'trace_##addr'
to be installed to trace_idt_table, while set_intr_gate_ist(..., 0) puts 'addr'.
into trace_idt_table() through _set_gate().
Therefore, if we want to use set_intr_gate() we need to provide a trace_debug entry
for it, which will be overwritten in trap_init() so it is in fact a useless symbol.
Anyway, I'd like to post a v3 patch follow your advise. Please keep an eye on it.
Thank you!
>> /* 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);
>
> Similarly, this should be set_system_gate.
>
>> #ifdef CONFIG_X86_32
>> set_intr_gate(X86_TRAP_PF, page_fault);
>> #endif
>> @@ -1005,6 +1013,15 @@ void __init trap_init(void)
>> */
>> cpu_init();
>>
>> + /*
>> + * X86_TRAP_DB and X86_TRAP_BP have been set
>> + * in early_trap_init(). However, DEBUG_STACK works only after
>> + * cpu_init() loads TSS. See comments in early_trap_init().
>
> It's not that DEBUG_STACK only works after the TSS is loaded; it's
> that the IST mechanism only works after TSS is loaded.
>
> --Andy
>
On Thu, Feb 26, 2015 at 6:21 PM, Wang Nan <[email protected]> wrote:
> On 2015/2/26 23:12, Andy Lutomirski wrote:
>> On Thu, Feb 26, 2015 at 5:12 AM, tip-bot for Wang Nan <[email protected]> wrote:
>>> Commit-ID: b4d8327024637cb2a1f7910dcb5d0ad7a096f473
>>> Gitweb: http://git.kernel.org/tip/b4d8327024637cb2a1f7910dcb5d0ad7a096f473
>>> Author: Wang Nan <[email protected]>
>>> AuthorDate: Thu, 26 Feb 2015 13:49:39 +0800
>>> Committer: Ingo Molnar <[email protected]>
>>> CommitDate: Thu, 26 Feb 2015 12:29:20 +0100
>>>
>>> x86/traps: Enable DEBUG_STACK after cpu_init() for TRAP_DB/BP
>>>
>>> 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().
>>>
>>
>> Sorry, I'm late to the party. This patch, while technically correct
>> AFAICT, is really ugly, because the whole point is that it *doesn't*
>> use ist. In other words:
>>
>>> + set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
>>
>> That should be set_intr_gate(X86_TRAP_DB, &debug);
>>
>
> I considered this, but found that set_intr_gate() and set_intr_gate_ist(..., 0)
> behaviors differently: set_intr_gate() -> _trace_set_gate() requires 'trace_##addr'
> to be installed to trace_idt_table, while set_intr_gate_ist(..., 0) puts 'addr'.
> into trace_idt_table() through _set_gate().
> Therefore, if we want to use set_intr_gate() we need to provide a trace_debug entry
> for it, which will be overwritten in trap_init() so it is in fact a useless symbol.
Yuck. Then IMO we should have a separate helper for this. Better
yet, we should rename set_intr_gate, because this distinction could
easily be overlooked.
>
> Anyway, I'd like to post a v3 patch follow your advise. Please keep an eye on it.
>
> Thank you!
>
>>> /* 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);
>>
>> Similarly, this should be set_system_gate.
>>
>>> #ifdef CONFIG_X86_32
>>> set_intr_gate(X86_TRAP_PF, page_fault);
>>> #endif
>>> @@ -1005,6 +1013,15 @@ void __init trap_init(void)
>>> */
>>> cpu_init();
>>>
>>> + /*
>>> + * X86_TRAP_DB and X86_TRAP_BP have been set
>>> + * in early_trap_init(). However, DEBUG_STACK works only after
>>> + * cpu_init() loads TSS. See comments in early_trap_init().
>>
>> It's not that DEBUG_STACK only works after the TSS is loaded; it's
>> that the IST mechanism only works after TSS is loaded.
>>
>> --Andy
>>
>
>
--
Andy Lutomirski
AMA Capital Management, LLC
As early_trap_init() doesn't use IST, replace set_intr_gate_ist(..., 0)
and set_system_intr_gate_ist(..., 0) with their standard counterparts.
set_intr_gate() requires a trace_debug symbol which we don't have and
won't use. Use a small macro trick as a workaround.
Signed-off-by: Wang Nan <[email protected]>
---
Hi Andy Lutomirski,
This patch is a bit tricky, but I think we don't need to define another
helper for such a small problem. What's your opinion?
Thank you!
---
arch/x86/kernel/traps.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4281988..c24434a 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -925,17 +925,22 @@ 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)
{
+
/*
- * 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 it is impossible to hit an invalid stack.
- * Using the original stack works well enough at this early
- * stage. DEBUG_STACK will be equipped after cpu_init() in
- * trap_init().
+ * Don't use IST to set 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 it is impossible to hit an invalid stack. Using the
+ * original stack works well enough at this early stage. DEBUG_STACK
+ * will be equipped after cpu_init() in trap_init().
+ *
+ * Since set_intr_gate() needs a trace_debug but we don't have it,
+ * use the following #define as a workaround.
*/
- set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
+#define trace_debug debug
+ set_intr_gate(X86_TRAP_DB, debug);
+#undef trace_debug
/* int3 can be called from all */
- set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
+ set_system_intr_gate(X86_TRAP_BP, &int3);
#ifdef CONFIG_X86_32
set_intr_gate(X86_TRAP_PF, page_fault);
#endif
@@ -1015,7 +1020,7 @@ void __init trap_init(void)
/*
* X86_TRAP_DB and X86_TRAP_BP have been set
- * in early_trap_init(). However, DEBUG_STACK works only after
+ * in early_trap_init(). However, ITS works only after
* cpu_init() loads TSS. See comments in early_trap_init().
*/
set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
--
1.8.4
As early_trap_init() doesn't use IST, replace set_intr_gate_ist() and
set_system_intr_gate_ist() with their standard counterparts.
set_intr_gate() requires a trace_debug symbol which we don't have and
won't use. This patch seprates set_intr_gate() into 2 parts, and uses
base version in early_trap_init().
Signed-off-by: Wang Nan <[email protected]>
---
Hi Andy Lutomirski,
This patch should be less tricky than previous one [1]. I also tried
to renaming all set_intr_gate(), but it causes too many code changes,
so I think you will be satisfied with this one.
Thank you!
[1] https://lkml.org/lkml/2015/2/26/770
---
arch/x86/include/asm/desc.h | 7 ++++++-
arch/x86/kernel/traps.c | 20 ++++++++++++--------
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
index a94b82e..a0bf89f 100644
--- a/arch/x86/include/asm/desc.h
+++ b/arch/x86/include/asm/desc.h
@@ -376,11 +376,16 @@ static inline void _set_gate(int gate, unsigned type, void *addr,
* Pentium F0 0F bugfix can have resulted in the mapped
* IDT being write-protected.
*/
-#define set_intr_gate(n, addr) \
+#define set_intr_gate_notrace(n, addr) \
do { \
BUG_ON((unsigned)n > 0xFF); \
_set_gate(n, GATE_INTERRUPT, (void *)addr, 0, 0, \
__KERNEL_CS); \
+ } while (0)
+
+#define set_intr_gate(n, addr) \
+ do { \
+ set_intr_gate_notrace(n, addr); \
_trace_set_gate(n, GATE_INTERRUPT, (void *)trace_##addr,\
0, 0, __KERNEL_CS); \
} while (0)
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 4281988..9965bd1 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -926,16 +926,20 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
void __init early_trap_init(void)
{
/*
- * 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 it is impossible to hit an invalid stack.
- * Using the original stack works well enough at this early
- * stage. DEBUG_STACK will be equipped after cpu_init() in
+ * Don't use IST to set 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 it is impossible to hit an invalid
+ * stack. Using the original stack works well enough at this
+ * early stage. DEBUG_STACK will be equipped after cpu_init() in
* trap_init().
+ *
+ * We don't need to set trace_idt_table like set_intr_gate(),
+ * since we don't have trace_debug and it will be reset to
+ * 'debug' in trap_init() by set_intr_gate_ist().
*/
- set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
+ set_intr_gate_notrace(X86_TRAP_DB, debug);
/* int3 can be called from all */
- set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
+ set_system_intr_gate(X86_TRAP_BP, &int3);
#ifdef CONFIG_X86_32
set_intr_gate(X86_TRAP_PF, page_fault);
#endif
@@ -1015,7 +1019,7 @@ void __init trap_init(void)
/*
* X86_TRAP_DB and X86_TRAP_BP have been set
- * in early_trap_init(). However, DEBUG_STACK works only after
+ * in early_trap_init(). However, ITS works only after
* cpu_init() loads TSS. See comments in early_trap_init().
*/
set_intr_gate_ist(X86_TRAP_DB, &debug, DEBUG_STACK);
--
1.8.4
On Fri, Feb 27, 2015 at 11:28:50AM +0800, Wang Nan wrote:
> As early_trap_init() doesn't use IST, replace set_intr_gate_ist(..., 0)
> and set_system_intr_gate_ist(..., 0) with their standard counterparts.
>
> set_intr_gate() requires a trace_debug symbol which we don't have and
> won't use. Use a small macro trick as a workaround.
>
> Signed-off-by: Wang Nan <[email protected]>
> ---
>
> Hi Andy Lutomirski,
>
> This patch is a bit tricky, but I think we don't need to define another
> helper for such a small problem. What's your opinion?
>
> Thank you!
>
> ---
> arch/x86/kernel/traps.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 4281988..c24434a 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -925,17 +925,22 @@ 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)
> {
> +
> /*
> - * 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 it is impossible to hit an invalid stack.
> - * Using the original stack works well enough at this early
> - * stage. DEBUG_STACK will be equipped after cpu_init() in
> - * trap_init().
> + * Don't use IST to set 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 it is impossible to hit an invalid stack. Using the
> + * original stack works well enough at this early stage. DEBUG_STACK
> + * will be equipped after cpu_init() in trap_init().
> + *
> + * Since set_intr_gate() needs a trace_debug but we don't have it,
> + * use the following #define as a workaround.
> */
> - set_intr_gate_ist(X86_TRAP_DB, &debug, 0);
> +#define trace_debug debug
This goes to arch/x86/include/asm/traps.h like the rest of the trace_*
defines.
> + set_intr_gate(X86_TRAP_DB, debug);
> +#undef trace_debug
> /* int3 can be called from all */
> - set_system_intr_gate_ist(X86_TRAP_BP, &int3, 0);
> + set_system_intr_gate(X86_TRAP_BP, &int3);
> #ifdef CONFIG_X86_32
> set_intr_gate(X86_TRAP_PF, page_fault);
> #endif
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--