Messages in this thread | | | From | Guo Ren <> | Date | Thu, 24 Nov 2022 17:53:45 +0800 | Subject | Re: [PATCH v2] riscv: VMAP_STACK overflow detection thread-safe |
| |
On Thu, Nov 24, 2022 at 5:48 PM Deepak Gupta <debug@rivosinc.com> wrote: > > commit 31da94c25aea ("riscv: add VMAP_STACK overflow detection") added > support for CONFIG_VMAP_STACK. If overflow is detected, CPU switches to > `shadow_stack` temporarily before switching finally to per-cpu > `overflow_stack`. > > If two CPUs/harts are racing and end up in over flowing kernel stack, one > or both will end up corrupting each other state because `shadow_stack` is > not per-cpu. This patch optimizes per-cpu overflow stack switch by > directly picking per-cpu `overflow_stack` and gets rid of `shadow_stack`. > > Following are the changes in this patch > > - Defines an asm macro to obtain per-cpu symbols in destination > register. > - In entry.S, when overflow is detected, per-cpu overflow stack is > located using per-cpu asm macro. Computing per-cpu symbol requires > a temporary register. x31 is saved away into CSR_SCRATCH > (CSR_SCRATCH is anyways zero since we're in kernel). > > Please see Links for additional relevant disccussion and alternative > solution. > > Tested by `echo EXHAUST_STACK > /sys/kernel/debug/provoke-crash/DIRECT` > Kernel crash log below > > Insufficient stack space to handle exception!/debug/provoke-crash/DIRECT > Task stack: [0xff20000010a98000..0xff20000010a9c000] > Overflow stack: [0xff600001f7d98370..0xff600001f7d99370] > CPU: 1 PID: 205 Comm: bash Not tainted 6.1.0-rc2-00001-g328a1f96f7b9 #34 > Hardware name: riscv-virtio,qemu (DT) > epc : __memset+0x60/0xfc > ra : recursive_loop+0x48/0xc6 [lkdtm] > epc : ffffffff808de0e4 ra : ffffffff0163a752 sp : ff20000010a97e80 > gp : ffffffff815c0330 tp : ff600000820ea280 t0 : ff20000010a97e88 > t1 : 000000000000002e t2 : 3233206874706564 s0 : ff20000010a982b0 > s1 : 0000000000000012 a0 : ff20000010a97e88 a1 : 0000000000000000 > a2 : 0000000000000400 a3 : ff20000010a98288 a4 : 0000000000000000 > a5 : 0000000000000000 a6 : fffffffffffe43f0 a7 : 00007fffffffffff > s2 : ff20000010a97e88 s3 : ffffffff01644680 s4 : ff20000010a9be90 > s5 : ff600000842ba6c0 s6 : 00aaaaaac29e42b0 s7 : 00fffffff0aa3684 > s8 : 00aaaaaac2978040 s9 : 0000000000000065 s10: 00ffffff8a7cad10 > s11: 00ffffff8a76a4e0 t3 : ffffffff815dbaf4 t4 : ffffffff815dbaf4 > t5 : ffffffff815dbab8 t6 : ff20000010a9bb48 > status: 0000000200000120 badaddr: ff20000010a97e88 cause: 000000000000000f > Kernel panic - not syncing: Kernel stack overflow > CPU: 1 PID: 205 Comm: bash Not tainted 6.1.0-rc2-00001-g328a1f96f7b9 #34 > Hardware name: riscv-virtio,qemu (DT) > Call Trace: > [<ffffffff80006754>] dump_backtrace+0x30/0x38 > [<ffffffff808de798>] show_stack+0x40/0x4c > [<ffffffff808ea2a8>] dump_stack_lvl+0x44/0x5c > [<ffffffff808ea2d8>] dump_stack+0x18/0x20 > [<ffffffff808dec06>] panic+0x126/0x2fe > [<ffffffff800065ea>] walk_stackframe+0x0/0xf0 > [<ffffffff0163a752>] recursive_loop+0x48/0xc6 [lkdtm] > SMP: stopping secondary CPUs > ---[ end Kernel panic - not syncing: Kernel stack overflow ]--- > > Cc: Guo Ren <guoren@kernel.org> > Cc: Jisheng Zhang <jszhang@kernel.org> > Link: https://lore.kernel.org/linux-riscv/Y347B0x4VUNOd6V7@xhacker/T/#t > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > > --- > v1 --> v2: > - asm macro to locate per-cpu symbol requires a temp reg. > When stack overflow happens, in trap handler we don't have spare regs > except sp. > v1 had a place holder in `thread_info` to spill a register. > v2 instead uses CSR_SCRATCH register because it's free to use. > > - v2 made per-cpu macro more readable. > - v2 fixed a bug that would've broken 32bit support. > > - v1 called it a fix over 31da94c25aea. v2 calls it alternative/ > optimization solution > --- > arch/riscv/include/asm/asm.h | 17 ++++++++++ > arch/riscv/kernel/asm-offsets.c | 1 + > arch/riscv/kernel/entry.S | 57 ++++++--------------------------- > arch/riscv/kernel/traps.c | 12 +------ > 4 files changed, 29 insertions(+), 58 deletions(-) > > diff --git a/arch/riscv/include/asm/asm.h b/arch/riscv/include/asm/asm.h > index 1b471ff73178..1eb479cb9ae4 100644 > --- a/arch/riscv/include/asm/asm.h > +++ b/arch/riscv/include/asm/asm.h > @@ -69,6 +69,7 @@ > > #ifdef __ASSEMBLY__ > > +#include <asm/asm-offsets.h> > /* Common assembly source macros */ > > /* > @@ -80,6 +81,22 @@ > .endr > .endm > > +#ifdef CONFIG_32BIT > +#define PER_CPU_OFFSET_SHIFT 2 > +#else > +#define PER_CPU_OFFSET_SHIFT 3 > +#endif > + > +.macro asm_per_cpu dst sym tmp > + REG_L \tmp, TASK_TI_CPU_NUM(tp) > + slli \tmp, \tmp, PER_CPU_OFFSET_SHIFT > + la \dst, __per_cpu_offset > + add \dst, \dst, \tmp > + REG_L \tmp, 0(\dst) > + la \dst, \sym > + add \dst, \dst, \tmp > +.endm > + > #endif /* __ASSEMBLY__ */ > > #endif /* _ASM_RISCV_ASM_H */ > diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c > index df9444397908..a7da051159cf 100644 > --- a/arch/riscv/kernel/asm-offsets.c > +++ b/arch/riscv/kernel/asm-offsets.c > @@ -38,6 +38,7 @@ void asm_offsets(void) > OFFSET(TASK_TI_KERNEL_SP, task_struct, thread_info.kernel_sp); > OFFSET(TASK_TI_USER_SP, task_struct, thread_info.user_sp); > > + OFFSET(TASK_TI_CPU_NUM, task_struct, thread_info.cpu); Why not TASK_TI_CPU ?
> OFFSET(TASK_THREAD_F0, task_struct, thread.fstate.f[0]); > OFFSET(TASK_THREAD_F1, task_struct, thread.fstate.f[1]); > OFFSET(TASK_THREAD_F2, task_struct, thread.fstate.f[2]); > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S > index b9eda3fcbd6d..2e90d9ccddd0 100644 > --- a/arch/riscv/kernel/entry.S > +++ b/arch/riscv/kernel/entry.S > @@ -10,9 +10,11 @@ > #include <asm/asm.h> > #include <asm/csr.h> > #include <asm/unistd.h> > +#include <asm/page.h> > #include <asm/thread_info.h> > #include <asm/asm-offsets.h> > #include <asm/errata_list.h> > +#include <linux/sizes.h> > > #if !IS_ENABLED(CONFIG_PREEMPTION) > .set resume_kernel, restore_all > @@ -404,54 +406,15 @@ handle_syscall_trace_exit: > > #ifdef CONFIG_VMAP_STACK > handle_kernel_stack_overflow: > - la sp, shadow_stack > - addi sp, sp, SHADOW_OVERFLOW_STACK_SIZE > + /* we reach here from kernel context, sscratch must be 0 */ > + csrrw x31, CSR_SCRATCH, x31 > + asm_per_cpu sp, overflow_stack, x31 > + li x31, OVERFLOW_STACK_SIZE > + add sp, sp, x31 > + /* zero out x31 again and restore x31 */ > + xor x31, x31, x31 > + csrrw x31, CSR_SCRATCH, x31 > > - //save caller register to shadow stack > - addi sp, sp, -(PT_SIZE_ON_STACK) > - REG_S x1, PT_RA(sp) > - REG_S x5, PT_T0(sp) > - REG_S x6, PT_T1(sp) > - REG_S x7, PT_T2(sp) > - REG_S x10, PT_A0(sp) > - REG_S x11, PT_A1(sp) > - REG_S x12, PT_A2(sp) > - REG_S x13, PT_A3(sp) > - REG_S x14, PT_A4(sp) > - REG_S x15, PT_A5(sp) > - REG_S x16, PT_A6(sp) > - REG_S x17, PT_A7(sp) > - REG_S x28, PT_T3(sp) > - REG_S x29, PT_T4(sp) > - REG_S x30, PT_T5(sp) > - REG_S x31, PT_T6(sp) > - > - la ra, restore_caller_reg > - tail get_overflow_stack > - > -restore_caller_reg: > - //save per-cpu overflow stack > - REG_S a0, -8(sp) > - //restore caller register from shadow_stack > - REG_L x1, PT_RA(sp) > - REG_L x5, PT_T0(sp) > - REG_L x6, PT_T1(sp) > - REG_L x7, PT_T2(sp) > - REG_L x10, PT_A0(sp) > - REG_L x11, PT_A1(sp) > - REG_L x12, PT_A2(sp) > - REG_L x13, PT_A3(sp) > - REG_L x14, PT_A4(sp) > - REG_L x15, PT_A5(sp) > - REG_L x16, PT_A6(sp) > - REG_L x17, PT_A7(sp) > - REG_L x28, PT_T3(sp) > - REG_L x29, PT_T4(sp) > - REG_L x30, PT_T5(sp) > - REG_L x31, PT_T6(sp) > - > - //load per-cpu overflow stack > - REG_L sp, -8(sp) > addi sp, sp, -(PT_SIZE_ON_STACK) > > //save context to overflow stack > diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c > index f3e96d60a2ff..eef3a87514c7 100644 > --- a/arch/riscv/kernel/traps.c > +++ b/arch/riscv/kernel/traps.c > @@ -208,18 +208,8 @@ int is_valid_bugaddr(unsigned long pc) > #endif /* CONFIG_GENERIC_BUG */ > > #ifdef CONFIG_VMAP_STACK > -static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], > +DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)], > overflow_stack)__aligned(16); > -/* > - * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used > - * to get per-cpu overflow stack(get_overflow_stack). > - */ > -long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)]; > -asmlinkage unsigned long get_overflow_stack(void) > -{ > - return (unsigned long)this_cpu_ptr(overflow_stack) + > - OVERFLOW_STACK_SIZE; > -} > > asmlinkage void handle_bad_stack(struct pt_regs *regs) > { > -- > 2.25.1 >
-- Best Regards Guo Ren
| |