Re: [PATCH] powerpc/64: Initialise thread_info for emergency stacks
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> Signed-off-by: Nicholas Piggin > --- > > 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=149794320519941=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 Thanks for all your support. -- Regard's Abdul Haleem IBM Linux Technology Centre
Re: [PATCH] powerpc/64: Initialise thread_info for emergency stacks
On Wed, 21 Jun 2017 12:01:37 +1000 Michael Ellermanwrote: > Nicholas Piggin writes: > > > 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
Re: [PATCH] powerpc/64: Initialise thread_info for emergency stacks
Nicholas Pigginwrites: > 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? cheers
[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 HaleemSigned-off-by: Nicholas Piggin --- 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=149794320519941=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 = , \ .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