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 <redacted>
---
arch/arm64/kernel/traps.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -871,8 +871,8 @@ DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack)voidpanic_bad_stack(structpt_regs*regs,unsignedlongesr,unsignedlongfar){unsignedlongtsk_stk=(unsignedlong)current->stack;-unsignedlongirq_stk=(unsignedlong)this_cpu_read(irq_stack_ptr);-unsignedlongovf_stk=(unsignedlong)this_cpu_ptr(overflow_stack);+unsignedlongirq_stk=(unsignedlong)raw_cpu_read(irq_stack_ptr);+unsignedlongovf_stk=(unsignedlong)raw_cpu_ptr(overflow_stack);console_verbose();pr_emerg("Insufficient stack space to handle exception!");
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 <redacted>
---
KernelVersion: v6.0-rc2
arch/arm/kernel/traps.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Mark Rutland <mark.rutland@arm.com> Date: 2022-08-25 13:30:16
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.
@@ -871,8 +871,8 @@ DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack)voidpanic_bad_stack(structpt_regs*regs,unsignedlongesr,unsignedlongfar){unsignedlongtsk_stk=(unsignedlong)current->stack;-unsignedlongirq_stk=(unsignedlong)this_cpu_read(irq_stack_ptr);-unsignedlongovf_stk=(unsignedlong)this_cpu_ptr(overflow_stack);+unsignedlongirq_stk=(unsignedlong)raw_cpu_read(irq_stack_ptr);+unsignedlongovf_stk=(unsignedlong)raw_cpu_ptr(overflow_stack);console_verbose();pr_emerg("Insufficient stack space to handle exception!");
From: Mark Rutland <mark.rutland@arm.com> Date: 2022-08-25 13:34:28
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.
On Thu, Aug 25, 2022 at 02:31:53PM +0800, Zhen Lei wrote:
quoted
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.
quoted
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.
@@ -871,8 +871,8 @@ DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], overflow_stack)voidpanic_bad_stack(structpt_regs*regs,unsignedlongesr,unsignedlongfar){unsignedlongtsk_stk=(unsignedlong)current->stack;-unsignedlongirq_stk=(unsignedlong)this_cpu_read(irq_stack_ptr);-unsignedlongovf_stk=(unsignedlong)this_cpu_ptr(overflow_stack);+unsignedlongirq_stk=(unsignedlong)raw_cpu_read(irq_stack_ptr);+unsignedlongovf_stk=(unsignedlong)raw_cpu_ptr(overflow_stack);console_verbose();pr_emerg("Insufficient stack space to handle exception!");
On Thu, Aug 25, 2022 at 02:31:54PM +0800, Zhen Lei wrote:
quoted
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.