[PATCH] riscv: prevent pt_regs corruption for secondary idle threads

[PATCH] riscv: prevent pt_regs corruption for secondary idle threads

[Sergey Matyukevich]

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>
---
 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);
 }
 
-- 
2.44.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: prevent pt_regs corruption for secondary idle threads

[Sergey Matyukevich]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: prevent pt_regs corruption for secondary idle threads

[Alexandre Ghiti]

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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv