[PATCH 0/2] arm: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

[PATCH 0/2] arm: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

[Zhen Lei]

I'm analyzing a strange problem these days, and I find that there are some areas in
panic_bad_stack() that can be optimized. That is, replace this_cpu_* with raw_cpu_* .

Just optimization, it is unlikely to cause the following exception nesting, because of
"lr : __bad_stack+0x88/0x8c".

[20220819163739]Unable to handle kernel paging request at virtual address f7ffff94901b8048
[20220819163739]Mem abort info:
[20220819163739]  ESR = 0x96000004
[20220819163739]  EC = 0x25: DABT (current EL), IL = 32 bits
[20220819163739]  SET = 0, FnV = 0
[20220819163739]  EA = 0, S1PTW = 0
[20220819163739]Data abort info:
[20220819163739]  ISV = 0, ISS = 0x00000004
[20220819163739]  CM = 0, WnR = 0
[20220819163739][f7ffff94901b8048] address between user and kernel address ranges
[20220819163739]Internal error: Oops: 96000004 [#1] PREEMPT SMP
[20220819163739]Modules linked in: ...
[20220819163740]CPU: 2 PID: 1272 Comm: 00002SWDLMain Tainted: G        W  O      5.10.0 #1
[20220819163740]Hardware name: hisilicon,hi1213-fpga (DT)
[20220819163740]pstate: 000003c5 (nzcv DAIF -PAN -UAO -TCO BTYPE=--)
[20220819163740]pc : __bad_stack+0x4c/0x8c
[20220819163740]lr : __bad_stack+0x88/0x8c
[20220819163740]sp : ffffff953ffa8160
[20220819163740]x29: f7ffff953ffa8120 x28: f7ffff94901b8040 
[20220819163740]x27: ffffffeb72ea6940 x26: ffffffebeee6cf10 
[20220819163740]x25: ffffffebef627000 x24: 0000000000000000 
[20220819163740]x23: 00000000600003c5 x22: f7ffffebeee11904 
[20220819163740]x21: ffffff953ffa82b0 x20: 0000007fffffffff 
[20220819163740]x19: f7ffffc0133ab898 x18: 0000000000000000 
[20220819163740]x17: 0000000000000000 x16: ffffffebef32f0a0 
[20220819163740]x15: 00000000624057a0 x14: 953325a7da350fb3 
[20220819163740]x13: 09bbbe32ce2b3c11 x12: c15a0e2d1991997b 
[20220819163740]x11: 0bc8be839e7850d0 x10: cafa1cb223203045 
[20220819163740]x9 : f36bed299e5840dc x8 : ffffffc0133aba48 
[20220819163740]x7 : ffffff953b1b0480 x6 : ffffffebef3e1000 
[20220819163740]x5 : 0000000000000000 x4 : 0000000000000001 
[20220819163740]x3 : f7ffffc0133ab750 x2 : 0000000000000025 
[20220819163740]x1 : 0000000096000004 x0 : ffffff953ffa8160 
[20220819163740]Call trace:
[20220819163740] __bad_stack+0x4c/0x8c
[20220819163740]Code: a90d6ffa a90e77fc 910543f5 d538411c (f9400794) 
[20220819163740]---[ end trace 07532bfa2c24851c ]---
[20220819163740]Kernel panic - not syncing: Oops: Fatal exception


Zhen Lei (2):
  arm64/traps: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()
  ARM: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

 arch/arm/kernel/traps.c   | 4 ++--
 arch/arm64/kernel/traps.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/2] arm64/traps: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

[Zhen Lei]

The hardware automatically disable the IRQ interrupt before jumping to the
interrupt or exception vector. Therefore, the preempt_disable() operation
in this_cpu_read() after macro expansion is unnecessary. In fact, before
commit 8168f098867f ("arm64: entry: split bad stack entry"), the operation
this_cpu_read() precedes arm64_enter_nmi(). If set_preempt_need_resched()
is called before stack overflow, this_cpu_read() may trigger scheduling,
see pseudocode below.

Pseudocode of this_cpu_read(xx) when CONFIG_PREEMPTION=y:
preempt_disable_notrace();
raw_cpu_read(xx);
if (unlikely(__preempt_count_dec_and_test()))
	__preempt_schedule_notrace();

Therefore, use raw_cpu_* instead of this_cpu_* to eliminate potential
hazards. At the very least, it reduces a few lines of assembly code.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 arch/arm64/kernel/traps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index b7fed33981f7b76..e6b6f4650e3d895 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -871,8 +871,8 @@ DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack)
 void panic_bad_stack(struct pt_regs *regs, unsigned long esr, unsigned long far)
 {
 	unsigned long tsk_stk = (unsigned long)current->stack;
-	unsigned long irq_stk = (unsigned long)this_cpu_read(irq_stack_ptr);
-	unsigned long ovf_stk = (unsigned long)this_cpu_ptr(overflow_stack);
+	unsigned long irq_stk = (unsigned long)raw_cpu_read(irq_stack_ptr);
+	unsigned long ovf_stk = (unsigned long)raw_cpu_ptr(overflow_stack);
 
 	console_verbose();
 	pr_emerg("Insufficient stack space to handle exception!");
-- 
2.25.1

[PATCH 2/2] ARM: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

[Zhen Lei]

The hardware automatically disable the IRQ interrupt before jumping to the
interrupt or exception vector. Therefore, the preempt_disable() operation
in this_cpu_read() after macro expansion is unnecessary. In fact, function
this_cpu_read() may trigger scheduling, see pseudocode below.

Pseudocode of this_cpu_read(xx):
preempt_disable_notrace();
raw_cpu_read(xx);
if (unlikely(__preempt_count_dec_and_test()))
	__preempt_schedule_notrace();

Therefore, use raw_cpu_* instead of this_cpu_* to eliminate potential
hazards. At the very least, it reduces a few lines of assembly code.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
KernelVersion: v6.0-rc2
 arch/arm/kernel/traps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 1518a1f443ff866..d5903d790cf3b7e 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -927,9 +927,9 @@ asmlinkage void handle_bad_stack(struct pt_regs *regs)
 {
 	unsigned long tsk_stk = (unsigned long)current->stack;
 #ifdef CONFIG_IRQSTACKS
-	unsigned long irq_stk = (unsigned long)this_cpu_read(irq_stack_ptr);
+	unsigned long irq_stk = (unsigned long)raw_cpu_read(irq_stack_ptr);
 #endif
-	unsigned long ovf_stk = (unsigned long)this_cpu_read(overflow_stack_ptr);
+	unsigned long ovf_stk = (unsigned long)raw_cpu_read(overflow_stack_ptr);
 
 	console_verbose();
 	pr_emerg("Insufficient stack space to handle exception!");
-- 
2.25.1

Re: [PATCH 1/2] arm64/traps: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

[Mark Rutland]

On Thu, Aug 25, 2022 at 02:31:53PM +0800, Zhen Lei wrote:
> The hardware automatically disable the IRQ interrupt before jumping to the
> interrupt or exception vector. Therefore, the preempt_disable() operation
> in this_cpu_read() after macro expansion is unnecessary. In fact, before
> commit 8168f098867f ("arm64: entry: split bad stack entry"), the operation
> this_cpu_read() precedes arm64_enter_nmi(). If set_preempt_need_resched()
> is called before stack overflow, this_cpu_read() may trigger scheduling,
> see pseudocode below.
> 
> Pseudocode of this_cpu_read(xx) when CONFIG_PREEMPTION=y:
> preempt_disable_notrace();
> raw_cpu_read(xx);
> if (unlikely(__preempt_count_dec_and_test()))
> 	__preempt_schedule_notrace();

Ok, but in mainline we have commit 8168f098867f; so we cannot reach here
without having fiddled with the preempt count.

Are you saying that some stable kernel is broken because it lacks commit
8168f098867f? Is so, I think the right fix is to backport commit 8168f098867f,
and that is then irrelevant to this change.

> Therefore, use raw_cpu_* instead of this_cpu_* to eliminate potential
> hazards. At the very least, it reduces a few lines of assembly code.

I'm happy to use raw_cpu_*() here, to minimize the work we have to do, any any
risks with e.g. instrumentation, but as above I don't think the case mentioned
in the commit message is relevant.

Thanks,
Mark.

> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>  arch/arm64/kernel/traps.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index b7fed33981f7b76..e6b6f4650e3d895 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -871,8 +871,8 @@ DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack)
>  void panic_bad_stack(struct pt_regs *regs, unsigned long esr, unsigned long far)
>  {
>  	unsigned long tsk_stk = (unsigned long)current->stack;
> -	unsigned long irq_stk = (unsigned long)this_cpu_read(irq_stack_ptr);
> -	unsigned long ovf_stk = (unsigned long)this_cpu_ptr(overflow_stack);
> +	unsigned long irq_stk = (unsigned long)raw_cpu_read(irq_stack_ptr);
> +	unsigned long ovf_stk = (unsigned long)raw_cpu_ptr(overflow_stack);
>  
>  	console_verbose();
>  	pr_emerg("Insufficient stack space to handle exception!");
> -- 
> 2.25.1
> 

Re: [PATCH 2/2] ARM: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

[Mark Rutland]

On Thu, Aug 25, 2022 at 02:31:54PM +0800, Zhen Lei wrote:
> The hardware automatically disable the IRQ interrupt before jumping to the
> interrupt or exception vector. Therefore, the preempt_disable() operation
> in this_cpu_read() after macro expansion is unnecessary. In fact, function
> this_cpu_read() may trigger scheduling, see pseudocode below.
> 
> Pseudocode of this_cpu_read(xx):
> preempt_disable_notrace();
> raw_cpu_read(xx);
> if (unlikely(__preempt_count_dec_and_test()))
> 	__preempt_schedule_notrace();
> 
> Therefore, use raw_cpu_* instead of this_cpu_* to eliminate potential
> hazards. At the very least, it reduces a few lines of assembly code.

I think if scheduling is a problem here, something should increment the
preempt_count as is done on arm64, since any other operation in this function
could end up causing preemption.

Regardless, I also think it's sensible to use raw_cpu_*() here, but I don't
think that actually fixes the problem the commit message describes.

Thanks,
Mark.

> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
> KernelVersion: v6.0-rc2
>  arch/arm/kernel/traps.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 1518a1f443ff866..d5903d790cf3b7e 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -927,9 +927,9 @@ asmlinkage void handle_bad_stack(struct pt_regs *regs)
>  {
>  	unsigned long tsk_stk = (unsigned long)current->stack;
>  #ifdef CONFIG_IRQSTACKS
> -	unsigned long irq_stk = (unsigned long)this_cpu_read(irq_stack_ptr);
> +	unsigned long irq_stk = (unsigned long)raw_cpu_read(irq_stack_ptr);
>  #endif
> -	unsigned long ovf_stk = (unsigned long)this_cpu_read(overflow_stack_ptr);
> +	unsigned long ovf_stk = (unsigned long)raw_cpu_read(overflow_stack_ptr);
>  
>  	console_verbose();
>  	pr_emerg("Insufficient stack space to handle exception!");
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] arm64/traps: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

[Leizhen (ThunderTown)]

On 2022/8/25 21:29, Mark Rutland wrote:
> On Thu, Aug 25, 2022 at 02:31:53PM +0800, Zhen Lei wrote:
>> The hardware automatically disable the IRQ interrupt before jumping to the
>> interrupt or exception vector. Therefore, the preempt_disable() operation
>> in this_cpu_read() after macro expansion is unnecessary. In fact, before
>> commit 8168f098867f ("arm64: entry: split bad stack entry"), the operation
>> this_cpu_read() precedes arm64_enter_nmi(). If set_preempt_need_resched()
>> is called before stack overflow, this_cpu_read() may trigger scheduling,
>> see pseudocode below.
>>
>> Pseudocode of this_cpu_read(xx) when CONFIG_PREEMPTION=y:
>> preempt_disable_notrace();
>> raw_cpu_read(xx);
>> if (unlikely(__preempt_count_dec_and_test()))
>> 	__preempt_schedule_notrace();
> 
> Ok, but in mainline we have commit 8168f098867f; so we cannot reach here
> without having fiddled with the preempt count.
> 
> Are you saying that some stable kernel is broken because it lacks commit
> 8168f098867f? Is so, I think the right fix is to backport commit 8168f098867f,
> and that is then irrelevant to this change.

Yes, after backport commit 8168f098867f, the risk is gone.

> 
>> Therefore, use raw_cpu_* instead of this_cpu_* to eliminate potential
>> hazards. At the very least, it reduces a few lines of assembly code.
> 
> I'm happy to use raw_cpu_*() here, to minimize the work we have to do, any any
> risks with e.g. instrumentation, but as above I don't think the case mentioned
> in the commit message is relevant.

OK, I will delete the description about risk.

> 
> Thanks,
> Mark.
> 
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>  arch/arm64/kernel/traps.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index b7fed33981f7b76..e6b6f4650e3d895 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -871,8 +871,8 @@ DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack)
>>  void panic_bad_stack(struct pt_regs *regs, unsigned long esr, unsigned long far)
>>  {
>>  	unsigned long tsk_stk = (unsigned long)current->stack;
>> -	unsigned long irq_stk = (unsigned long)this_cpu_read(irq_stack_ptr);
>> -	unsigned long ovf_stk = (unsigned long)this_cpu_ptr(overflow_stack);
>> +	unsigned long irq_stk = (unsigned long)raw_cpu_read(irq_stack_ptr);
>> +	unsigned long ovf_stk = (unsigned long)raw_cpu_ptr(overflow_stack);
>>  
>>  	console_verbose();
>>  	pr_emerg("Insufficient stack space to handle exception!");
>> -- 
>> 2.25.1
>>
> .
> 

-- 
Regards,
  Zhen Lei

Re: [PATCH 2/2] ARM: Replace this_cpu_* with raw_cpu_* in panic_bad_stack()

[Leizhen (ThunderTown)]

On 2022/8/25 21:32, Mark Rutland wrote:
> On Thu, Aug 25, 2022 at 02:31:54PM +0800, Zhen Lei wrote:
>> The hardware automatically disable the IRQ interrupt before jumping to the
>> interrupt or exception vector. Therefore, the preempt_disable() operation
>> in this_cpu_read() after macro expansion is unnecessary. In fact, function
>> this_cpu_read() may trigger scheduling, see pseudocode below.
>>
>> Pseudocode of this_cpu_read(xx):
>> preempt_disable_notrace();
>> raw_cpu_read(xx);
>> if (unlikely(__preempt_count_dec_and_test()))
>> 	__preempt_schedule_notrace();
>>
>> Therefore, use raw_cpu_* instead of this_cpu_* to eliminate potential
>> hazards. At the very least, it reduces a few lines of assembly code.
> 
> I think if scheduling is a problem here, something should increment the
> preempt_count as is done on arm64, since any other operation in this function
> could end up causing preemption.

Yes, right. Sorry, I'm stuck in this_cpu_read()'s analysis.

> 
> Regardless, I also think it's sensible to use raw_cpu_*() here, but I don't
> think that actually fixes the problem the commit message describes.

OK, I will delete the description about risk. The risk I mentioned in the
commit message was mainly to show that using raw_cpu_read() would be better
than using this_cpu_read() in this case.

> 
> Thanks,
> Mark.
> 
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>> KernelVersion: v6.0-rc2
>>  arch/arm/kernel/traps.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
>> index 1518a1f443ff866..d5903d790cf3b7e 100644
>> --- a/arch/arm/kernel/traps.c
>> +++ b/arch/arm/kernel/traps.c
>> @@ -927,9 +927,9 @@ asmlinkage void handle_bad_stack(struct pt_regs *regs)
>>  {
>>  	unsigned long tsk_stk = (unsigned long)current->stack;
>>  #ifdef CONFIG_IRQSTACKS
>> -	unsigned long irq_stk = (unsigned long)this_cpu_read(irq_stack_ptr);
>> +	unsigned long irq_stk = (unsigned long)raw_cpu_read(irq_stack_ptr);
>>  #endif
>> -	unsigned long ovf_stk = (unsigned long)this_cpu_read(overflow_stack_ptr);
>> +	unsigned long ovf_stk = (unsigned long)raw_cpu_read(overflow_stack_ptr);
>>  
>>  	console_verbose();
>>  	pr_emerg("Insufficient stack space to handle exception!");
>> -- 
>> 2.25.1
>>
> .
> 

-- 
Regards,
  Zhen Lei