The standard RISC-V ABIs all require 16-byte stack alignment. We're
only calling that one function on the shadow stack so I doubt it'd
result in a real issue, but might as well keep this lined up.
Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
Signed-off-by: Palmer Dabbelt <[email protected]>
---
arch/riscv/kernel/traps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index be54ccea8c47..acdfcacd7e57 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
* 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)];
+long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
asmlinkage unsigned long get_overflow_stack(void)
{
return (unsigned long)this_cpu_ptr(overflow_stack) +
--
2.38.1
It took me a while to page all this back in when trying to review the
recent spin_shadow_stack, so I figured I'd just write up some comments.
Signed-off-by: Palmer Dabbelt <[email protected]>
---
arch/riscv/kernel/traps.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index acdfcacd7e57..336d4aadadb1 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -200,18 +200,18 @@ void __init trap_init(void)
}
#ifdef CONFIG_VMAP_STACK
+/*
+ * Extra stack space that allows us to provide panic messages when the kernel
+ * has overflowed its stack.
+ */
static 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).
+ * A temporary stack for use by handle_kernel_stack_overflow. This is used so
+ * we can call into C code to get the per-hart overflow stack. Usage of this
+ * stack must be protected by spin_shadow_stack.
*/
long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
-asmlinkage unsigned long get_overflow_stack(void)
-{
- return (unsigned long)this_cpu_ptr(overflow_stack) +
- OVERFLOW_STACK_SIZE;
-}
/*
* A pseudo spinlock to protect the shadow stack from being used by multiple
@@ -222,6 +222,12 @@ asmlinkage unsigned long get_overflow_stack(void)
*/
unsigned long spin_shadow_stack;
+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)
{
unsigned long tsk_stk = (unsigned long)current->stack;
--
2.38.1
On Tue, 29 Nov 2022 18:56:48 PST (-0800), Khem Raj wrote:
> On Tue, Nov 29, 2022 at 6:50 PM Palmer Dabbelt <[email protected]> wrote:
>>
>> On Tue, 29 Nov 2022 18:47:55 PST (-0800), Khem Raj wrote:
>> > Hi Palmer
>> >
>> > On Tue, Nov 29, 2022 at 6:36 PM Palmer Dabbelt <[email protected]> wrote:
>> >>
>> >> The standard RISC-V ABIs all require 16-byte stack alignment. We're
>> >> only calling that one function on the shadow stack so I doubt it'd
>> >> result in a real issue, but might as well keep this lined up.
>> >
>> > Is 16-byte alignment required on rv32 as well ?
>>
>> For the standard ABIs that's the case, it's so the Q extension can spill
>> without aligning the stack. There's also at least a proposed embedded
>> ABI that has just XLEN (32-bit on rv32) alignment, as the bigger stack
>> alignment has an impact on some use cases.
>
> Thanks, so in this case 16byte will be valid for both rv64/rv32 here.
Yes, though the long-alignment wouldn't break anything because we don't
have Q support and we're just calling that one function -- it's not like
the compiler is actively checking for 16-byte alignment or anything,
it's just assuming it.
Still best to keep things to the spec where we can, though.
>> >> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
>> >> Signed-off-by: Palmer Dabbelt <[email protected]>
>> >> ---
>> >> arch/riscv/kernel/traps.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> >> index be54ccea8c47..acdfcacd7e57 100644
>> >> --- a/arch/riscv/kernel/traps.c
>> >> +++ b/arch/riscv/kernel/traps.c
>> >> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
>> >> * 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)];
>> >> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
>> >> asmlinkage unsigned long get_overflow_stack(void)
>> >> {
>> >> return (unsigned long)this_cpu_ptr(overflow_stack) +
>> >> --
>> >> 2.38.1
>> >>
>> >>
>> >> _______________________________________________
>> >> linux-riscv mailing list
>> >> [email protected]
>> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, Nov 29, 2022 at 6:50 PM Palmer Dabbelt <[email protected]> wrote:
>
> On Tue, 29 Nov 2022 18:47:55 PST (-0800), Khem Raj wrote:
> > Hi Palmer
> >
> > On Tue, Nov 29, 2022 at 6:36 PM Palmer Dabbelt <[email protected]> wrote:
> >>
> >> The standard RISC-V ABIs all require 16-byte stack alignment. We're
> >> only calling that one function on the shadow stack so I doubt it'd
> >> result in a real issue, but might as well keep this lined up.
> >
> > Is 16-byte alignment required on rv32 as well ?
>
> For the standard ABIs that's the case, it's so the Q extension can spill
> without aligning the stack. There's also at least a proposed embedded
> ABI that has just XLEN (32-bit on rv32) alignment, as the bigger stack
> alignment has an impact on some use cases.
Thanks, so in this case 16byte will be valid for both rv64/rv32 here.
>
> >> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
> >> Signed-off-by: Palmer Dabbelt <[email protected]>
> >> ---
> >> arch/riscv/kernel/traps.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> >> index be54ccea8c47..acdfcacd7e57 100644
> >> --- a/arch/riscv/kernel/traps.c
> >> +++ b/arch/riscv/kernel/traps.c
> >> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
> >> * 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)];
> >> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
> >> asmlinkage unsigned long get_overflow_stack(void)
> >> {
> >> return (unsigned long)this_cpu_ptr(overflow_stack) +
> >> --
> >> 2.38.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> [email protected]
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, 29 Nov 2022 18:47:55 PST (-0800), Khem Raj wrote:
> Hi Palmer
>
> On Tue, Nov 29, 2022 at 6:36 PM Palmer Dabbelt <[email protected]> wrote:
>>
>> The standard RISC-V ABIs all require 16-byte stack alignment. We're
>> only calling that one function on the shadow stack so I doubt it'd
>> result in a real issue, but might as well keep this lined up.
>
> Is 16-byte alignment required on rv32 as well ?
For the standard ABIs that's the case, it's so the Q extension can spill
without aligning the stack. There's also at least a proposed embedded
ABI that has just XLEN (32-bit on rv32) alignment, as the bigger stack
alignment has an impact on some use cases.
>> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
>> Signed-off-by: Palmer Dabbelt <[email protected]>
>> ---
>> arch/riscv/kernel/traps.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index be54ccea8c47..acdfcacd7e57 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
>> * 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)];
>> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
>> asmlinkage unsigned long get_overflow_stack(void)
>> {
>> return (unsigned long)this_cpu_ptr(overflow_stack) +
>> --
>> 2.38.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
Hi Palmer
On Tue, Nov 29, 2022 at 6:36 PM Palmer Dabbelt <[email protected]> wrote:
>
> The standard RISC-V ABIs all require 16-byte stack alignment. We're
> only calling that one function on the shadow stack so I doubt it'd
> result in a real issue, but might as well keep this lined up.
Is 16-byte alignment required on rv32 as well ?
>
> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
> Signed-off-by: Palmer Dabbelt <[email protected]>
> ---
> arch/riscv/kernel/traps.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index be54ccea8c47..acdfcacd7e57 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
> * 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)];
> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
> asmlinkage unsigned long get_overflow_stack(void)
> {
> return (unsigned long)this_cpu_ptr(overflow_stack) +
> --
> 2.38.1
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Wed, Nov 30, 2022 at 10:35 AM Palmer Dabbelt <[email protected]> wrote:
>
> It took me a while to page all this back in when trying to review the
> recent spin_shadow_stack, so I figured I'd just write up some comments.
>
> Signed-off-by: Palmer Dabbelt <[email protected]>
> ---
> arch/riscv/kernel/traps.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index acdfcacd7e57..336d4aadadb1 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -200,18 +200,18 @@ void __init trap_init(void)
> }
>
> #ifdef CONFIG_VMAP_STACK
> +/*
> + * Extra stack space that allows us to provide panic messages when the kernel
> + * has overflowed its stack.
> + */
> static 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).
> + * A temporary stack for use by handle_kernel_stack_overflow. This is used so
> + * we can call into C code to get the per-hart overflow stack. Usage of this
> + * stack must be protected by spin_shadow_stack.
Reviewed-by: Guo Ren <[email protected]>
> */
> long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
> -asmlinkage unsigned long get_overflow_stack(void)
> -{
> - return (unsigned long)this_cpu_ptr(overflow_stack) +
> - OVERFLOW_STACK_SIZE;
> -}
>
> /*
> * A pseudo spinlock to protect the shadow stack from being used by multiple
> @@ -222,6 +222,12 @@ asmlinkage unsigned long get_overflow_stack(void)
> */
> unsigned long spin_shadow_stack;
>
> +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)
> {
> unsigned long tsk_stk = (unsigned long)current->stack;
> --
> 2.38.1
>
--
Best Regards
Guo Ren
On Tue, Nov 29, 2022 at 06:35:14PM -0800, Palmer Dabbelt wrote:
> The standard RISC-V ABIs all require 16-byte stack alignment. We're
> only calling that one function on the shadow stack so I doubt it'd
> result in a real issue, but might as well keep this lined up.
>
> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
> Signed-off-by: Palmer Dabbelt <[email protected]>
Reviewed-by: Jisheng Zhang <[email protected]>
> ---
> arch/riscv/kernel/traps.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index be54ccea8c47..acdfcacd7e57 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
> * 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)];
> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
> asmlinkage unsigned long get_overflow_stack(void)
> {
> return (unsigned long)this_cpu_ptr(overflow_stack) +
> --
> 2.38.1
>
On Tue, Nov 29, 2022 at 06:35:15PM -0800, Palmer Dabbelt wrote:
> It took me a while to page all this back in when trying to review the
> recent spin_shadow_stack, so I figured I'd just write up some comments.
>
> Signed-off-by: Palmer Dabbelt <[email protected]>
Reviewed-by: Jisheng Zhang <[email protected]>
> ---
> arch/riscv/kernel/traps.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index acdfcacd7e57..336d4aadadb1 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -200,18 +200,18 @@ void __init trap_init(void)
> }
>
> #ifdef CONFIG_VMAP_STACK
> +/*
> + * Extra stack space that allows us to provide panic messages when the kernel
> + * has overflowed its stack.
> + */
> static 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).
> + * A temporary stack for use by handle_kernel_stack_overflow. This is used so
> + * we can call into C code to get the per-hart overflow stack. Usage of this
> + * stack must be protected by spin_shadow_stack.
> */
> long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
> -asmlinkage unsigned long get_overflow_stack(void)
> -{
> - return (unsigned long)this_cpu_ptr(overflow_stack) +
> - OVERFLOW_STACK_SIZE;
> -}
>
> /*
> * A pseudo spinlock to protect the shadow stack from being used by multiple
> @@ -222,6 +222,12 @@ asmlinkage unsigned long get_overflow_stack(void)
> */
> unsigned long spin_shadow_stack;
>
> +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)
> {
> unsigned long tsk_stk = (unsigned long)current->stack;
> --
> 2.38.1
>
Hello:
This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <[email protected]>:
On Tue, 29 Nov 2022 18:35:14 -0800 you wrote:
> The standard RISC-V ABIs all require 16-byte stack alignment. We're
> only calling that one function on the shadow stack so I doubt it'd
> result in a real issue, but might as well keep this lined up.
>
> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
> Signed-off-by: Palmer Dabbelt <[email protected]>
>
> [...]
Here is the summary with links:
- [1/2] RISC-V: Align the shadow stack
https://git.kernel.org/riscv/c/b003b3b77d65
- [2/2] RISC-V: Add some comments about the shadow and overflow stacks
https://git.kernel.org/riscv/c/de57ecc47610
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
On Tue, 29 Nov 2022 18:35:14 -0800, Palmer Dabbelt wrote:
> The standard RISC-V ABIs all require 16-byte stack alignment. We're
> only calling that one function on the shadow stack so I doubt it'd
> result in a real issue, but might as well keep this lined up.
>
>
Applied, thanks!
[1/2] RISC-V: Align the shadow stack
https://git.kernel.org/palmer/c/c3ec1e8964fb
[2/2] RISC-V: Add some comments about the shadow and overflow stacks
https://git.kernel.org/palmer/c/de57ecc47610
Best regards,
--
Palmer Dabbelt <[email protected]>