[PATCH] arm64: make irq_stack_ptr more robust

[PATCH] arm64: make irq_stack_ptr more robust

[Yang Shi]

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.

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
+	 * 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)
-- 
2.0.2

Re: [PATCH] arm64: make irq_stack_ptr more robust

[James Morse]

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")


> 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"


> +	 * 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())


Acked-by: James Morse <james.morse@arm.com>
Tested-by: James Morse <james.morse@arm.com>


Thanks!

James

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@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
>
>

Re: [PATCH] arm64: make irq_stack_ptr more robust

[Will Deacon]

On Fri, Feb 12, 2016 at 09:38:51AM -0800, Shi, Yang wrote:
> On 2/12/2016 5:47 AM, James Morse wrote:
> >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.

I already queued the patch with this and the whitespace fix, so no need
for a v2.

Will

Re: [PATCH] arm64: make irq_stack_ptr more robust

[Shi, Yang]

On 2/12/2016 9:41 AM, Will Deacon wrote:
> On Fri, Feb 12, 2016 at 09:38:51AM -0800, Shi, Yang wrote:
>> On 2/12/2016 5:47 AM, James Morse wrote:
>>> 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.
>
> I already queued the patch with this and the whitespace fix, so no need
> for a v2.

Thanks so much.

Yang

>
> Will
>

Reinitiazling an arm machine without restarting the kernel or rebooting the machine

[Rudici Cazeao]

All,
I have the following arch machine defined as follows:
MACHINE_START(TRANSCEDE, "Transcede 2200/3300")
      /* Maintainer: Intel Corporation */
      .boot_params  = PHYS_OFFSET + 0x00000100,
      .map_io       = transcede_map_io,
      .init_irq     = transcede_init_irq,
      .timer        = &transcede_timer,
      .init_machine = transcede_init,
      .reserve      = transcede_reserve,
MACHINE_END

I would like to be reable to reinitialize the machine in the state that it was in when it had firt booted up whithout actually rebooting or restarting the kernel.

How can I go about this?


I was thinking about restarting from the following statement from init/main.c

setup_arch(&command_line);


Is this the right way to go?


Thanks,

Re: [PATCH] arm64: make irq_stack_ptr more robust

[Shi, Yang]

On 2/12/2016 9:38 AM, Shi, Yang wrote:
> 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.

In addition, linux/sched.h, which is included by both traps.c and 
stacktrace.c, includes preempt.h already.

Yang

>
> Thanks,
> Yang
>
>>
>>
>> Acked-by: James Morse <james.morse@arm.com>
>> Tested-by: James Morse <james.morse@arm.com>
>>
>>
>> Thanks!
>>
>> James
>>
>>
>