[PATCH] powerpc/64: Initialise thread_info for emergency stacks

Emergency stacks have their thread_info mostly uninitialised, which in
particular means garbage preempt_count values.

Emergency stack code runs with interrupts disabled entirely, and is
used very rarely, so this has been unnoticed so far. It was found by a
proposed new powerpc watchdog that takes a soft-NMI directly from the
masked_interrupt handler and using the emergency stack. That crashed at
BUG_ON(in_nmi()) in nmi_enter(). preempt_count()s were found to be
garbage.

Reported-by: Abdul Haleem <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---

FYI, this bug looks to be breaking linux-next on some powerpc
boxes due to interaction with a proposed new powerpc watchdog
driver Andrew has in his tree:

http://marc.info/?l=linuxppc-embedded&m=149794320519941&w=2

 arch/powerpc/include/asm/thread_info.h | 19 +++++++++++++++++++
 arch/powerpc/kernel/setup_64.c         |  6 +++---
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index a941cc6fc3e9..5995e4b2996d 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h

@@ -54,6 +54,7 @@ struct thread_info {
 	.task =		&tsk,			\
 	.cpu =		0,			\
 	.preempt_count = INIT_PREEMPT_COUNT,	\
+	.local_flags =	0,			\
 	.flags =	0,			\
 }
 

@@ -62,6 +63,24 @@ struct thread_info {
 
 #define THREAD_SIZE_ORDER	(THREAD_SHIFT - PAGE_SHIFT)
 
+/*
+ * Emergency stacks are used for a range of things, from asynchronous
+ * NMIs (system reset, machine check) to synchronous, process context.
+ * Set HARDIRQ_OFFSET because we don't know exactly what context we
+ * come from or if it had a valid stack, which is about the best we
+ * can do.
+ * TODO: what to do with accounting?
+ */
+#define emstack_init_thread_info(ti, c)		\
+do {						\
+	(ti)->task = NULL;			\
+	(ti)->cpu = (c);			\
+	(ti)->preempt_count = HARDIRQ_OFFSET;	\
+	(ti)->local_flags = 0;			\
+	(ti)->flags = 0;			\
+	klp_init_thread_info(ti);		\
+} while (0)
+
 /* how to get the thread information struct from C */
 static inline struct thread_info *current_thread_info(void)
 {

diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index f35ff9dea4fb..54c4336655f8 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c

@@ -639,18 +639,18 @@ void __init emergency_stack_init(void)
 	for_each_possible_cpu(i) {
 		struct thread_info *ti;
 		ti = __va(memblock_alloc_base(THREAD_SIZE, THREAD_SIZE, limit));
-		klp_init_thread_info(ti);
+		emstack_init_thread_info(ti, i);
 		paca[i].emergency_sp = (void *)ti + THREAD_SIZE;
 
 #ifdef CONFIG_PPC_BOOK3S_64
 		/* emergency stack for NMI exception handling. */
 		ti = __va(memblock_alloc_base(THREAD_SIZE, THREAD_SIZE, limit));
-		klp_init_thread_info(ti);
+		emstack_init_thread_info(ti, i);
 		paca[i].nmi_emergency_sp = (void *)ti + THREAD_SIZE;
 
 		/* emergency stack for machine check exception handling. */
 		ti = __va(memblock_alloc_base(THREAD_SIZE, THREAD_SIZE, limit));
-		klp_init_thread_info(ti);
+		emstack_init_thread_info(ti, i);
 		paca[i].mc_emergency_sp = (void *)ti + THREAD_SIZE;
 #endif
 	}

-- 
2.11.0

Nicholas Piggin [off-list ref] writes:

Why don't we just bzero() the whole thing? Like we do for the other
stacks?

cheers

On Wed, 21 Jun 2017 12:01:37 +1000
Michael Ellerman [off-list ref] wrote:

Nicholas Piggin [off-list ref] writes:

quoted

diff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h
index a941cc6fc3e9..5995e4b2996d 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h

@@ -62,6 +63,24 @@ struct thread_info {
 
 #define THREAD_SIZE_ORDER	(THREAD_SHIFT - PAGE_SHIFT)
 
+/*
+ * Emergency stacks are used for a range of things, from asynchronous
+ * NMIs (system reset, machine check) to synchronous, process context.
+ * Set HARDIRQ_OFFSET because we don't know exactly what context we
+ * come from or if it had a valid stack, which is about the best we
+ * can do.
+ * TODO: what to do with accounting?
+ */
+#define emstack_init_thread_info(ti, c)		\
+do {						\
+	(ti)->task = NULL;			\
+	(ti)->cpu = (c);			\
+	(ti)->preempt_count = HARDIRQ_OFFSET;	\
+	(ti)->local_flags = 0;			\
+	(ti)->flags = 0;			\
+	klp_init_thread_info(ti);		\
+} while (0)  

Why don't we just bzero() the whole thing? Like we do for the other
stacks?

Wouldn't hurt to.

Thanks,
Nick

On Tue, 2017-06-20 at 23:58 +1000, Nicholas Piggin wrote:

Emergency stacks have their thread_info mostly uninitialised, which in
particular means garbage preempt_count values.

Emergency stack code runs with interrupts disabled entirely, and is
used very rarely, so this has been unnoticed so far. It was found by a
proposed new powerpc watchdog that takes a soft-NMI directly from the
masked_interrupt handler and using the emergency stack. That crashed at
BUG_ON(in_nmi()) in nmi_enter(). preempt_count()s were found to be
garbage.

Reported-by: Abdul Haleem <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---

FYI, this bug looks to be breaking linux-next on some powerpc
boxes due to interaction with a proposed new powerpc watchdog
driver Andrew has in his tree:

http://marc.info/?l=linuxppc-embedded&m=149794320519941&w=2

 arch/powerpc/include/asm/thread_info.h | 19 +++++++++++++++++++
 arch/powerpc/kernel/setup_64.c         |  6 +++---
 2 files changed, 22 insertions(+), 3 deletions(-)

Hi Nicholas,

Thanks for the patch, Verified on next-20170621 and PowerPC bare-metal
boots fine with your patch

Tested-by: Abdul Haleem <redacted>

Thanks for all your support.

-- 
Regard's

Abdul Haleem
IBM Linux Technology Centre