| Message ID | 20240424221927.900612-1-geomatsi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: prevent pt_regs corruption for secondary idle threads | expand |
Hi all, > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> > > Top of the kernel thread stack should be reserved for pt_regs. However > this is not the case for the idle threads of the secondary boot harts. > Their stacks overlap with their pt_regs, so both may get corrupted. > > Similar issue has been fixed for the primary hart, see c7cdd96eca29 > ("riscv: prevent stack corruption by reserving task_pt_regs(p) early"). > However that fix was not propagated to the secondary harts. The problem > has been noticed in some CPU hotplug tests with V enabled. The function > smp_callin stores several registers on stack, corrupting top of pt_regs > structure including status field. As a result, kernel attempted to save > or restore inexistent V context. > > Fixes: 9a2451f18663 ("RISC-V: Avoid using per cpu array for ordered booting") > Fixes: 2875fe056156 ("RISC-V: Add cpu_ops and modify default booting method") > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com> Friendly ping about this fix. Any thoughts/comments ? Regards, Sergey
Hi Sergey, On 25/04/2024 00:19, Sergey Matyukevich wrote: > From: Sergey Matyukevich <sergey.matyukevich@syntacore.com> > > Top of the kernel thread stack should be reserved for pt_regs. However > this is not the case for the idle threads of the secondary boot harts. > Their stacks overlap with their pt_regs, so both may get corrupted. > > Similar issue has been fixed for the primary hart, see c7cdd96eca29 This revision does not exist but I found the commit under the revision c7cdd96eca28. > ("riscv: prevent stack corruption by reserving task_pt_regs(p) early"). > However that fix was not propagated to the secondary harts. The problem > has been noticed in some CPU hotplug tests with V enabled. The function > smp_callin stores several registers on stack, corrupting top of pt_regs > structure including status field. As a result, kernel attempted to save > or restore inexistent V context. > > Fixes: 9a2451f18663 ("RISC-V: Avoid using per cpu array for ordered booting") > Fixes: 2875fe056156 ("RISC-V: Add cpu_ops and modify default booting method") > Signed-off-by: Sergey Matyukevich <sergey.matyukevich@syntacore.com> > --- > arch/riscv/kernel/cpu_ops_sbi.c | 2 +- > arch/riscv/kernel/cpu_ops_spinwait.c | 3 +-- > 2 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/arch/riscv/kernel/cpu_ops_sbi.c b/arch/riscv/kernel/cpu_ops_sbi.c > index 1cc7df740edd..e6fbaaf54956 100644 > --- a/arch/riscv/kernel/cpu_ops_sbi.c > +++ b/arch/riscv/kernel/cpu_ops_sbi.c > @@ -72,7 +72,7 @@ static int sbi_cpu_start(unsigned int cpuid, struct task_struct *tidle) > /* Make sure tidle is updated */ > smp_mb(); > bdata->task_ptr = tidle; > - bdata->stack_ptr = task_stack_page(tidle) + THREAD_SIZE; > + bdata->stack_ptr = task_pt_regs(tidle); > /* Make sure boot data is updated */ > smp_mb(); > hsm_data = __pa(bdata); > diff --git a/arch/riscv/kernel/cpu_ops_spinwait.c b/arch/riscv/kernel/cpu_ops_spinwait.c > index 613872b0a21a..24869eb88908 100644 > --- a/arch/riscv/kernel/cpu_ops_spinwait.c > +++ b/arch/riscv/kernel/cpu_ops_spinwait.c > @@ -34,8 +34,7 @@ static void cpu_update_secondary_bootdata(unsigned int cpuid, > > /* Make sure tidle is updated */ > smp_mb(); > - WRITE_ONCE(__cpu_spinwait_stack_pointer[hartid], > - task_stack_page(tidle) + THREAD_SIZE); > + WRITE_ONCE(__cpu_spinwait_stack_pointer[hartid], task_pt_regs(tidle)); > WRITE_ONCE(__cpu_spinwait_task_pointer[hartid], tidle); > } > With the revision fixed, you can add: Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> Thanks, Alex
diff --git a/arch/riscv/kernel/cpu_ops_sbi.c b/arch/riscv/kernel/cpu_ops_sbi.c index 1cc7df740edd..e6fbaaf54956 100644 --- a/arch/riscv/kernel/cpu_ops_sbi.c +++ b/arch/riscv/kernel/cpu_ops_sbi.c @@ -72,7 +72,7 @@ static int sbi_cpu_start(unsigned int cpuid, struct task_struct *tidle) /* Make sure tidle is updated */ smp_mb(); bdata->task_ptr = tidle; - bdata->stack_ptr = task_stack_page(tidle) + THREAD_SIZE; + bdata->stack_ptr = task_pt_regs(tidle); /* Make sure boot data is updated */ smp_mb(); hsm_data = __pa(bdata); diff --git a/arch/riscv/kernel/cpu_ops_spinwait.c b/arch/riscv/kernel/cpu_ops_spinwait.c index 613872b0a21a..24869eb88908 100644 --- a/arch/riscv/kernel/cpu_ops_spinwait.c +++ b/arch/riscv/kernel/cpu_ops_spinwait.c @@ -34,8 +34,7 @@ static void cpu_update_secondary_bootdata(unsigned int cpuid, /* Make sure tidle is updated */ smp_mb(); - WRITE_ONCE(__cpu_spinwait_stack_pointer[hartid], - task_stack_page(tidle) + THREAD_SIZE); + WRITE_ONCE(__cpu_spinwait_stack_pointer[hartid], task_pt_regs(tidle)); WRITE_ONCE(__cpu_spinwait_task_pointer[hartid], tidle); }