Re: [PATCH] arm64: make irq_stack_ptr more robust

[Shi, Yang]

On 2/12/2016 5:47 AM, James Morse wrote:
> Hi!
>
> On 11/02/16 21:53, Yang Shi wrote:
> > Switching between stacks is only valid if we are tracing ourselves while on the
> > irq_stack, so it is only valid when in current and non-preemptible context,
> > otherwise is is just zeroed off.
>
> Given it was picked up with CONFIG_DEBUG_PREEMPT:
>
> Fixes: 132cd887b5c5 ("arm64: Modify stack trace and dump for use with irq_stack")

Wii add in v2.

> > Signed-off-by: Yang Shi <yang.shi@xxxxxxxxxx>
> > ---
> >   arch/arm64/kernel/stacktrace.c | 13 ++++++-------
> >   arch/arm64/kernel/traps.c      | 11 ++++++++++-
> >   2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> > index 12a18cb..d9751a4 100644
> > --- a/arch/arm64/kernel/stacktrace.c
> > +++ b/arch/arm64/kernel/stacktrace.c
> > @@ -44,14 +44,13 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)
> >   	unsigned long irq_stack_ptr;
> >
> >   	/*
> > -	 * Use raw_smp_processor_id() to avoid false-positives from
> > -	 * CONFIG_DEBUG_PREEMPT. get_wchan() calls unwind_frame() on sleeping
> > -	 * task stacks, we can be pre-empted in this case, so
> > -	 * {raw_,}smp_processor_id() may give us the wrong value. Sleeping
> > -	 * tasks can't ever be on an interrupt stack, so regardless of cpu,
> > -	 * the checks will always fail.
> > +	 * Switching between stacks is valid when tracing current and in
> > +	 * non-preemptible context.
> >   	 */
> > -	irq_stack_ptr = IRQ_STACK_PTR(raw_smp_processor_id());
> > +	if (tsk == current && !preemptible())
> > +		irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
> > +	else
> > +		irq_stack_ptr = 0;
> >
> >   	low  = frame->sp;
> >   	/* irq stacks are not THREAD_SIZE aligned */
> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index cbedd72..7d8db3a 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -146,9 +146,18 @@ static void dump_instr(const char *lvl, struct pt_regs *regs)
> >   static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
> >   {
> >   	struct stackframe frame;
> > -	unsigned long irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
> > +	unsigned long irq_stack_ptr;
> >   	int skip;
> >
> > +	/*
> > +	 * Switching between  stacks is valid when tracing current and in
>
> Nit: Two spaces: "between[ ][ ]stacks"

Will fix in v2.

> > +	 * non-preemptible context.
> > +	 */
> > +	if (tsk == current && !preemptible())
> > +		irq_stack_ptr = IRQ_STACK_PTR(smp_processor_id());
> > +	else
> > +		irq_stack_ptr = 0;
> > +
> >   	pr_debug("%s(regs = %p tsk = %p)\n", __func__, regs, tsk);
> >
> >   	if (!tsk)
>
> Neither file includes 'linux/preempt.h' for the definition of preemptible().
> (I can't talk: I should have included smp.h for smp_processor_id())

I tried to build the kernel with preempt and without preempt, both 
works. And, I saw arch/arm64/include/asm/Kbuild has:

generic-y += preempt.h

So, it sounds preempt.h has been included by default.

Thanks,
Yang

> Acked-by: James Morse <james.morse@xxxxxxx>
> Tested-by: James Morse <james.morse@xxxxxxx>
>
>
> Thanks!
>
> James