[PATCH] riscv: Fix 32-bit call_on_irq_stack() frame pointer ABI

[PATCH] riscv: Fix 32-bit call_on_irq_stack() frame pointer ABI

[Rui Qi]

From: Samuel Holland <samuel.holland@sifive.com>

call_on_irq_stack() uses struct member offsets to set up its link in the
frame record list. On riscv32, struct stackframe is the wrong size to
maintain stack pointer alignment, so STACKFRAME_SIZE_ON_STACK includes
padding. However, the ABI requires the frame record to be placed
immediately below the address stored in s0, so the padding must come
before the struct members.

Fix the layout by making STACKFRAME_FP and STACKFRAME_RA the negative
offsets from s0, instead of the positive offsets from sp.

Fixes: 82982fdd5133 ("riscv: Deduplicate IRQ stack switching")
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
Signed-off-by: Rui Qi <qirui.001@bytedance.com>
Link: https://lore.kernel.org/all/20240530001733.1407654-2-samuel.holland@sifive.com/
This is a resubmission of Samuel Holland's patch [1] which was
posted in May 2024 but never merged. The bug still exists in the
current tree.

[1] https://lore.kernel.org/all/20240530001733.1407654-2-samuel.holland@sifive.com/
---
 arch/riscv/kernel/asm-offsets.c | 4 ++--
 arch/riscv/kernel/entry.S       | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index af827448a609..a75f0cfea1e9 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -501,8 +501,8 @@ void asm_offsets(void)
 	OFFSET(SBI_HART_BOOT_STACK_PTR_OFFSET, sbi_hart_boot_data, stack_ptr);
 
 	DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct stackframe), STACK_ALIGN));
-	OFFSET(STACKFRAME_FP, stackframe, fp);
-	OFFSET(STACKFRAME_RA, stackframe, ra);
+	DEFINE(STACKFRAME_FP, offsetof(struct stackframe, fp) - sizeof(struct stackframe));
+	DEFINE(STACKFRAME_RA, offsetof(struct stackframe, ra) - sizeof(struct stackframe));
 #ifdef CONFIG_FUNCTION_TRACER
 	DEFINE(FTRACE_OPS_FUNC,		offsetof(struct ftrace_ops, func));
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index c6988983cdf7..08df724e13b9 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -386,8 +386,8 @@ SYM_CODE_END(ret_from_fork_user_asm)
 SYM_FUNC_START(call_on_irq_stack)
 	/* Create a frame record to save ra and s0 (fp) */
 	addi	sp, sp, -STACKFRAME_SIZE_ON_STACK
-	REG_S	ra, STACKFRAME_RA(sp)
-	REG_S	s0, STACKFRAME_FP(sp)
+	REG_S	ra, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_RA)(sp)
+	REG_S	s0, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_FP)(sp)
 	addi	s0, sp, STACKFRAME_SIZE_ON_STACK
 
 	/* Switch to the per-CPU shadow call stack */
@@ -405,8 +405,8 @@ SYM_FUNC_START(call_on_irq_stack)
 
 	/* Switch back to the thread stack and restore ra and s0 */
 	addi	sp, s0, -STACKFRAME_SIZE_ON_STACK
-	REG_L	ra, STACKFRAME_RA(sp)
-	REG_L	s0, STACKFRAME_FP(sp)
+	REG_L	ra, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_RA)(sp)
+	REG_L	s0, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_FP)(sp)
 	addi	sp, sp, STACKFRAME_SIZE_ON_STACK
 
 	ret
-- 
2.20.1

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

Re: [PATCH] riscv: Fix 32-bit call_on_irq_stack() frame pointer ABI

[Nam Cao]

"Rui Qi" <qirui.001@bytedance.com> writes:
> This is a resubmission of Samuel Holland's patch [1] which was
> posted in May 2024 but never merged. The bug still exists in the
> current tree.
>
> [1] https://lore.kernel.org/all/20240530001733.1407654-2-samuel.holland@sifive.com/

I don't think we should keep this part in the commit message. And
Matthew's Reviewed-by should be included.

But I leave that to the maintainers, the diff looks correct to me.

Reviewed-by: Nam Cao <namcao@linutronix.de>

Nam

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

Re: [PATCH] riscv: Fix 32-bit call_on_irq_stack() frame pointer ABI

[Michael Ellerman]

On 24/6/26 9:31 pm, Rui Qi wrote:
> From: Samuel Holland <samuel.holland@sifive.com>
> 
> call_on_irq_stack() uses struct member offsets to set up its link in the
> frame record list. On riscv32, struct stackframe is the wrong size to
> maintain stack pointer alignment, so STACKFRAME_SIZE_ON_STACK includes
> padding. However, the ABI requires the frame record to be placed
> immediately below the address stored in s0, so the padding must come
> before the struct members.
> 
> Fix the layout by making STACKFRAME_FP and STACKFRAME_RA the negative
> offsets from s0, instead of the positive offsets from sp.

The fact that all uses of the defines need to add back STACKFRAME_SIZE_ON_STACK
makes me think the defines don't have the most useful values.

If the values were offset + padding then the uses could be left unchanged.

> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
> index af827448a609..a75f0cfea1e9 100644
> --- a/arch/riscv/kernel/asm-offsets.c
> +++ b/arch/riscv/kernel/asm-offsets.c
> @@ -501,8 +501,8 @@ void asm_offsets(void)
>   	OFFSET(SBI_HART_BOOT_STACK_PTR_OFFSET, sbi_hart_boot_data, stack_ptr);
>   
>   	DEFINE(STACKFRAME_SIZE_ON_STACK, ALIGN(sizeof(struct stackframe), STACK_ALIGN));
> -	OFFSET(STACKFRAME_FP, stackframe, fp);
> -	OFFSET(STACKFRAME_RA, stackframe, ra);
> +	DEFINE(STACKFRAME_FP, offsetof(struct stackframe, fp) - sizeof(struct stackframe));
> +	DEFINE(STACKFRAME_RA, offsetof(struct stackframe, ra) - sizeof(struct stackframe));

ie. if they were defined as:

         DEFINE(STACKFRAME_FP, ALIGN(sizeof(struct stackframe), STACK_ALIGN) -
                               sizeof(struct stackframe) + offsetof(struct stackframe, fp));
         DEFINE(STACKFRAME_RA, ALIGN(sizeof(struct stackframe), STACK_ALIGN) -
                               sizeof(struct stackframe) + offsetof(struct stackframe, ra));

STACKFRAME_SIZE_ON_STACK - sizeof(stackframe) computes the size of the padding,
and then plus the offset gets you the right location.

That would allow the call sites to still use STACKFRAME_RA(sp).

I would probably also rename them to ON_STACK_RA or something, to make it clear
they are not offsets into struct stackframe any more, but maybe that's bike shedding :)

cheers

>   #ifdef CONFIG_FUNCTION_TRACER
>   	DEFINE(FTRACE_OPS_FUNC,		offsetof(struct ftrace_ops, func));
>   #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index c6988983cdf7..08df724e13b9 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -386,8 +386,8 @@ SYM_CODE_END(ret_from_fork_user_asm)
>   SYM_FUNC_START(call_on_irq_stack)
>   	/* Create a frame record to save ra and s0 (fp) */
>   	addi	sp, sp, -STACKFRAME_SIZE_ON_STACK
> -	REG_S	ra, STACKFRAME_RA(sp)
> -	REG_S	s0, STACKFRAME_FP(sp)
> +	REG_S	ra, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_RA)(sp)
> +	REG_S	s0, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_FP)(sp)
>   	addi	s0, sp, STACKFRAME_SIZE_ON_STACK
>   
>   	/* Switch to the per-CPU shadow call stack */
> @@ -405,8 +405,8 @@ SYM_FUNC_START(call_on_irq_stack)
>   
>   	/* Switch back to the thread stack and restore ra and s0 */
>   	addi	sp, s0, -STACKFRAME_SIZE_ON_STACK
> -	REG_L	ra, STACKFRAME_RA(sp)
> -	REG_L	s0, STACKFRAME_FP(sp)
> +	REG_L	ra, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_RA)(sp)
> +	REG_L	s0, (STACKFRAME_SIZE_ON_STACK + STACKFRAME_FP)(sp)
>   	addi	sp, sp, STACKFRAME_SIZE_ON_STACK
>   
>   	ret

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

Re: [PATCH] riscv: Fix 32-bit call_on_irq_stack() frame pointer ABI

[Nam Cao]

Michael Ellerman <mpe@kernel.org> writes:
> On 24/6/26 9:31 pm, Rui Qi wrote:
>> call_on_irq_stack() uses struct member offsets to set up its link in the
>> frame record list. On riscv32, struct stackframe is the wrong size to
>> maintain stack pointer alignment, so STACKFRAME_SIZE_ON_STACK includes
>> padding. However, the ABI requires the frame record to be placed
>> immediately below the address stored in s0, so the padding must come
>> before the struct members.
>> 
>> Fix the layout by making STACKFRAME_FP and STACKFRAME_RA the negative
>> offsets from s0, instead of the positive offsets from sp.
>
> The fact that all uses of the defines need to add back STACKFRAME_SIZE_ON_STACK
> makes me think the defines don't have the most useful values.
>
> If the values were offset + padding then the uses could be left unchanged.

At the moment we only have only one user which has nothing except the
stack frame on the stack. Other potential users do not necessarily add back
STACKFRAME_SIZE_ON_STACK.

The offset value depends on the function. So I think it's better to
leave that offset calculation to users.

Nam

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