Re: [PATCH] arm64: make irq_stack_ptr more robust

["Shi, Yang" <yang.shi@linaro.org>]

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@linaro.org>
>> ---
>>   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@arm.com>
> Tested-by: James Morse <james.morse@arm.com>
>
>
> Thanks!
>
> James
>
>