[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

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

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

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

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

[Paul Walmsley]

On Thu, 25 Jun 2026, Nam Cao wrote:

> 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.

Since it's a fix, I queued Rui's resend of Samuel's patch for v7.2-rc, 
after cleaning it up along the lines of Nam's suggestions.

Michael, if you think those changes are worth a followup patch for the 
list to consider, maybe you can send it as a separate patch?


- Paul

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

[Samuel Holland]

On 2026-06-25 3:49 AM, Nam Cao wrote:
> 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.

The reason for choosing the definitions in the patch is to match the ABI[1],
which defines the fields at negative offsets from the frame pointer: "This puts
the return address at fp - PTRSZ/8, and the previous frame pointer at fp - 2 *
PTRSZ/8"

In call_on_irq_stack(), the frame pointer is (sp + STACKFRAME_SIZE_ON_STACK). So
the restore code could equivalently be written as:

    REG_L    ra, STACKFRAME_RA(s0)
    REG_L    s0, STACKFRAME_FP(s0)

which is nice and clean and works everywhere, but the save code can't be as
clean due to needing both the old and new frame pointer value at the same time.
So it either needs a temporary register for the new stack pointer, or to do the
calculation in the assembly, and I chose the latter; and I preferred to make the
save/restore code visually consistent.

[1]:
https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#frame-pointer-convention

> 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.

Indeed, a later patch in the original series allocated space for both struct
stackframe and struct pt_regs at the same time.

Regards,
Samuel