This fixes two bugs in SCS initialization for secondary CPUs. First,
the SCS was not initialized at all in the spinwait boot path. Second,
the code for the SBI HSM path attempted to initialize the SCS before
enabling the MMU. However, that involves dereferencing the thread
pointer, which requires the MMU to be enabled.
Fix both issues by setting up the SCS in the common secondary entry
path, after enabling the MMU.
Fixes: d1584d791a29 ("riscv: Implement Shadow Call Stack")
Signed-off-by: Samuel Holland <[email protected]>
---
arch/riscv/kernel/head.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index b77397432403..76ace1e0b46f 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -154,7 +154,6 @@ secondary_start_sbi:
XIP_FIXUP_OFFSET a3
add a3, a3, a1
REG_L sp, (a3)
- scs_load_current
.Lsecondary_start_common:
@@ -165,6 +164,7 @@ secondary_start_sbi:
call relocate_enable_mmu
#endif
call .Lsetup_trap_vector
+ scs_load_current
tail smp_callin
#endif /* CONFIG_SMP */
--
2.42.0
On Tue, Nov 21, 2023 at 01:19:29PM -0800, Samuel Holland wrote:
> This fixes two bugs in SCS initialization for secondary CPUs. First,
> the SCS was not initialized at all in the spinwait boot path. Second,
> the code for the SBI HSM path attempted to initialize the SCS before
> enabling the MMU. However, that involves dereferencing the thread
> pointer, which requires the MMU to be enabled.
>
> Fix both issues by setting up the SCS in the common secondary entry
> path, after enabling the MMU.
I'm curious, mostly because I do not know much about the implemtnation
of the shadow call stack, but does it actually work correctly when the
kernel is built without mmu support?
>
> Fixes: d1584d791a29 ("riscv: Implement Shadow Call Stack")
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> arch/riscv/kernel/head.S | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index b77397432403..76ace1e0b46f 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -154,7 +154,6 @@ secondary_start_sbi:
> XIP_FIXUP_OFFSET a3
> add a3, a3, a1
> REG_L sp, (a3)
> - scs_load_current
>
> .Lsecondary_start_common:
>
> @@ -165,6 +164,7 @@ secondary_start_sbi:
> call relocate_enable_mmu
> #endif
> call .Lsetup_trap_vector
> + scs_load_current
> tail smp_callin
> #endif /* CONFIG_SMP */
>
> --
> 2.42.0
>
Hi Conor,
On 2023-11-23 8:06 AM, Conor Dooley wrote:
> On Tue, Nov 21, 2023 at 01:19:29PM -0800, Samuel Holland wrote:
>> This fixes two bugs in SCS initialization for secondary CPUs. First,
>> the SCS was not initialized at all in the spinwait boot path. Second,
>> the code for the SBI HSM path attempted to initialize the SCS before
>> enabling the MMU. However, that involves dereferencing the thread
>> pointer, which requires the MMU to be enabled.
>>
>> Fix both issues by setting up the SCS in the common secondary entry
>> path, after enabling the MMU.
>
> I'm curious, mostly because I do not know much about the implemtnation
> of the shadow call stack, but does it actually work correctly when the
> kernel is built without mmu support?
I imagine it would work. The SCS implementation is purely software; it stores
the return address in a stack at `gp` instead of with the rest of local
variables at `sp`.
The problem here is that we are passing a pointer between CPUs with different
views of the virtual address space (i.e. the boot CPU sees the kernel at
0xffffffff80000000 while the CPU being brought up sees it at its physical
address), and then dereferencing it. If there is no MMU support, then the
virtual address space is identity mapped on all CPUs, and there is no problem.
Regards,
Samuel
>> Fixes: d1584d791a29 ("riscv: Implement Shadow Call Stack")
>> Signed-off-by: Samuel Holland <[email protected]>
>> ---
>>
>> arch/riscv/kernel/head.S | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
>> index b77397432403..76ace1e0b46f 100644
>> --- a/arch/riscv/kernel/head.S
>> +++ b/arch/riscv/kernel/head.S
>> @@ -154,7 +154,6 @@ secondary_start_sbi:
>> XIP_FIXUP_OFFSET a3
>> add a3, a3, a1
>> REG_L sp, (a3)
>> - scs_load_current
>>
>> .Lsecondary_start_common:
>>
>> @@ -165,6 +164,7 @@ secondary_start_sbi:
>> call relocate_enable_mmu
>> #endif
>> call .Lsetup_trap_vector
>> + scs_load_current
>> tail smp_callin
>> #endif /* CONFIG_SMP */
>>
>> --
>> 2.42.0
>>
Hi Samuel,
On Tue, Nov 21, 2023 at 1:20 PM Samuel Holland
<[email protected]> wrote:
>
> This fixes two bugs in SCS initialization for secondary CPUs. First,
> the SCS was not initialized at all in the spinwait boot path. Second,
> the code for the SBI HSM path attempted to initialize the SCS before
> enabling the MMU. However, that involves dereferencing the thread
> pointer, which requires the MMU to be enabled.
>
> Fix both issues by setting up the SCS in the common secondary entry
> path, after enabling the MMU.
Thanks for the patch! Looks like my qemu setup doesn't hit this issue,
but nevertheless, the fix looks good to me.
Reviewed-by: Sami Tolvanen <[email protected]>
Sami
Hello:
This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <[email protected]>:
On Tue, 21 Nov 2023 13:19:29 -0800 you wrote:
> This fixes two bugs in SCS initialization for secondary CPUs. First,
> the SCS was not initialized at all in the spinwait boot path. Second,
> the code for the SBI HSM path attempted to initialize the SCS before
> enabling the MMU. However, that involves dereferencing the thread
> pointer, which requires the MMU to be enabled.
>
> Fix both issues by setting up the SCS in the common secondary entry
> path, after enabling the MMU.
>
> [...]
Here is the summary with links:
- riscv: Fix SMP when shadow call stacks are enabled
https://git.kernel.org/riscv/c/f40cab8e18ed
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Fri, Dec 01, 2023 at 09:40:55AM -0800, Sami Tolvanen wrote:
> Hi Samuel,
>
> On Tue, Nov 21, 2023 at 1:20 PM Samuel Holland
> <[email protected]> wrote:
> >
> > This fixes two bugs in SCS initialization for secondary CPUs. First,
> > the SCS was not initialized at all in the spinwait boot path. Second,
> > the code for the SBI HSM path attempted to initialize the SCS before
> > enabling the MMU. However, that involves dereferencing the thread
> > pointer, which requires the MMU to be enabled.
> >
> > Fix both issues by setting up the SCS in the common secondary entry
> > path, after enabling the MMU.
>
> Thanks for the patch! Looks like my qemu setup doesn't hit this issue,
> but nevertheless, the fix looks good to me.
Because there is no function call in relocate_enable_mmu :)
>
> Reviewed-by: Sami Tolvanen <[email protected]>
>
> Sami
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv