LinuxLists.cc - [PATCH 0/4] riscv: Frame pointer fixes and enhancements

2024-05-30 00:17:44

Subject: [PATCH 0/4] riscv: Frame pointer fixes and enhancements

This series improves frame pointer support in the RISC-V kernel. Patch 1
fixes a bug in 32-bit kernels. Patch 2 prepares for patch 3, which fixes
several places where the kernel clobbers the frame pointer. Patch 4
saves a frame record in the exception entry assembly so the kernel can
unwind past exceptions. Here's an example of what the result looks like:

[ 150.315343] sysrq: Show backtrace of all active CPUs
[ 150.319827] sysrq: CPU0:
[ 150.322063] Call Trace:
[ 150.324684] [<ffffffff800059fa>] show_stack+0x2c/0x38
[ 150.329757] [<ffffffff803e0228>] sysrq_handle_showallcpus+0x78/0xbc
[ 150.336014] [<ffffffff803dfc52>] __handle_sysrq+0x152/0x15e
[ 150.341601] [<ffffffff803e0a88>] write_sysrq_trigger+0xa4/0xba
[ 150.347221] [<ffffffff80182638>] proc_reg_write+0x3e/0x80
[ 150.352787] [<ffffffff8012885e>] vfs_write+0xd8/0x302
[ 150.357625] [<ffffffff80128ba8>] ksys_write+0x58/0xaa
[ 150.362856] [<ffffffff80128c10>] __riscv_sys_write+0x16/0x1e
[ 150.368317] [<ffffffff80005650>] syscall_handler+0x1c/0x28
[ 150.373986] [<ffffffff807372c8>] do_trap_ecall_u+0x64/0xb0
[ 150.379431] [<ffffffff8073e91c>] ret_from_exception+0x0/0x70
[ 150.385207] sysrq: CPU1: backtrace skipped as idling
[ 150.385210] sysrq: CPU2: backtrace skipped as idling
[ 150.385213] sysrq: CPU3:
[ 150.397294] Call Trace:
[ 150.399717] [<ffffffff800059fa>] show_stack+0x2c/0x38
[ 150.404756] [<ffffffff803e02e6>] showacpu+0x56/0x84
[ 150.409620] [<ffffffff8009959e>] __flush_smp_call_function_queue+0x13a/0x1d2
[ 150.416657] [<ffffffff8009968c>] generic_smp_call_function_single_interrupt+0xe/0x16
[ 150.424390] [<ffffffff8000886c>] handle_IPI+0x38/0x74
[ 150.429424] [<ffffffff800659ce>] handle_percpu_devid_irq+0x82/0x106
[ 150.435678] [<ffffffff80060b72>] generic_handle_domain_irq+0x1c/0x2a
[ 150.442019] [<ffffffff8006b9de>] ipi_mux_process+0x6e/0xd6
[ 150.447490] [<ffffffff8000af48>] sbi_ipi_handle+0x30/0x46
[ 150.452876] [<ffffffff80060b72>] generic_handle_domain_irq+0x1c/0x2a
[ 150.459217] [<ffffffff80341936>] riscv_intc_irq+0x22/0x60
[ 150.464602] [<ffffffff807373b0>] handle_riscv_irq+0x30/0x4e
[ 150.470161] [<ffffffff8073736a>] do_irq+0x1a/0x30
[ 150.474851] [<ffffffff8073e91c>] ret_from_exception+0x0/0x70
[ 150.480497] [<ffffffff8033c5a0>] __percpu_counter_sum+0xbe/0xd0
[ 150.486403] [<ffffffff8033c5a0>] __percpu_counter_sum+0xbe/0xd0
[ 150.492310] [<ffffffff8000eac0>] __mmdrop+0xc8/0x208
[ 150.497260] [<ffffffff80038922>] finish_task_switch+0x120/0x198
[ 150.503167] [<ffffffff80739668>] __schedule+0x444/0x6c8
[ 150.508378] [<ffffffff80739a16>] preempt_schedule_common+0x18/0x32
[ 150.514546] [<ffffffff80739222>] preempt_schedule+0x22/0x24
[ 150.520105] [<ffffffff803e026a>] sysrq_handle_showallcpus+0xba/0xbc
[ 150.526359] [<ffffffff803dfc52>] __handle_sysrq+0x152/0x15e
[ 150.531917] [<ffffffff803e0a88>] write_sysrq_trigger+0xa4/0xba
[ 150.537737] [<ffffffff80182638>] proc_reg_write+0x3e/0x80
[ 150.543122] [<ffffffff8012885e>] vfs_write+0xd8/0x302
[ 150.548160] [<ffffffff80128ba8>] ksys_write+0x58/0xaa
[ 150.553197] [<ffffffff80128c10>] __riscv_sys_write+0x16/0x1e
[ 150.558847] [<ffffffff80005650>] syscall_handler+0x1c/0x28
[ 150.564316] [<ffffffff807372c8>] do_trap_ecall_u+0x64/0xb0
[ 150.569788] [<ffffffff8073e91c>] ret_from_exception+0x0/0x70

Samuel Holland (4):
riscv: Fix 32-bit call_on_irq_stack() frame pointer ABI
riscv: entry: Balance vector context nesting
riscv: entry: Do not clobber the frame pointer
riscv: entry: Save a frame record for exceptions

arch/riscv/include/asm/processor.h | 9 ++++-
arch/riscv/include/asm/ptrace.h | 5 +++
arch/riscv/include/asm/stacktrace.h | 5 ---
arch/riscv/kernel/asm-offsets.c | 10 ++---
arch/riscv/kernel/entry.S | 60 +++++++++++++++--------------
arch/riscv/kernel/head.S | 6 +--
arch/riscv/kernel/process.c | 5 +--
arch/riscv/kernel/stacktrace.c | 9 -----
8 files changed, 53 insertions(+), 56 deletions(-)

--
2.44.1

2024-05-30 00:17:50

by Samuel Holland

[permalink] [raw]

Subject: [PATCH 1/4] riscv: Fix 32-bit call_on_irq_stack() frame pointer ABI

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 <[email protected]>
---

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 b09ca5f944f7..84c056f5ee09 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -487,8 +487,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_DYNAMIC_FTRACE_WITH_ARGS
DEFINE(FREGS_SIZE_ON_STACK, ALIGN(sizeof(struct ftrace_regs), STACK_ALIGN));
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 68a24cf9481a..4c5b22cb7381 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -246,8 +246,8 @@ SYM_CODE_END(ret_from_fork)
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 */
@@ -265,8 +265,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.44.1

2024-05-30 00:18:01

by Samuel Holland

[permalink] [raw]

Subject: [PATCH 2/4] riscv: entry: Balance vector context nesting

Vector context management is the last thing done before jumping to C
code, so it should be the first thing done after returning from C code.
This also improves efficiency: riscv_v_context_nesting_end() clobbers
the saved value of the status CSR, so currently ret_from_exception()
must reload it. This is not necessary if riscv_v_context_nesting_end()
is called first.

Signed-off-by: Samuel Holland <[email protected]>
---

arch/riscv/kernel/entry.S | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index 4c5b22cb7381..d13d1aad7649 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -120,6 +120,11 @@ ASM_NOKPROBE(handle_exception)
* - ret_from_fork
*/
SYM_CODE_START_NOALIGN(ret_from_exception)
+#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE
+ move a0, sp
+ call riscv_v_context_nesting_end
+#endif
+
REG_L s0, PT_STATUS(sp)
#ifdef CONFIG_RISCV_M_MODE
/* the MPP value is too large to be used as an immediate arg for addi */
@@ -143,10 +148,6 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
*/
csrw CSR_SCRATCH, tp
1:
-#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE
- move a0, sp
- call riscv_v_context_nesting_end
-#endif
REG_L a0, PT_STATUS(sp)
/*
* The current load reservation is effectively part of the processor's
--
2.44.1

2024-05-30 00:18:16

by Samuel Holland

[permalink] [raw]

Subject: [PATCH 3/4] riscv: entry: Do not clobber the frame pointer

s0 is reserved for the frame pointer, so it should not be used as a
temporary register. Clobbering the frame pointer breaks stack traces.

- In handle_exception() and ret_from_exception(), use a2 for the saved
stack pointer. a2 is chosen because r2 is the stack pointer register.
- In ret_from_exception(), use s1 for the saved status CSR value. Avoid
clobbering s1 in the privilege mode check so it does not need to be
reloaded later in the function.
- Use s1 and s2 in ret_from_fork() instead of s0 and s1. The entire
p->thread.s array is zeroed at the beginning of copy_thread(), so the
registers do not need to be zeroed separately for kernel threads.

Signed-off-by: Samuel Holland <[email protected]>
---

arch/riscv/kernel/entry.S | 29 ++++++++++++++---------------
arch/riscv/kernel/process.c | 5 ++---
2 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index d13d1aad7649..bd1c5621df45 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -58,13 +58,13 @@ SYM_CODE_START(handle_exception)
*/
li t0, SR_SUM | SR_FS_VS

- REG_L s0, TASK_TI_USER_SP(tp)
+ REG_L a2, TASK_TI_USER_SP(tp)
csrrc s1, CSR_STATUS, t0
csrr s2, CSR_EPC
csrr s3, CSR_TVAL
csrr s4, CSR_CAUSE
csrr s5, CSR_SCRATCH
- REG_S s0, PT_SP(sp)
+ REG_S a2, PT_SP(sp)
REG_S s1, PT_STATUS(sp)
REG_S s2, PT_EPC(sp)
REG_S s3, PT_BADADDR(sp)
@@ -125,19 +125,19 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
call riscv_v_context_nesting_end
#endif

- REG_L s0, PT_STATUS(sp)
+ REG_L s1, PT_STATUS(sp)
#ifdef CONFIG_RISCV_M_MODE
/* the MPP value is too large to be used as an immediate arg for addi */
li t0, SR_MPP
- and s0, s0, t0
+ and t0, s1, t0
#else
- andi s0, s0, SR_SPP
+ andi t0, s1, SR_SPP
#endif
- bnez s0, 1f
+ bnez t0, 1f

/* Save unwound kernel stack pointer in thread_info */
- addi s0, sp, PT_SIZE_ON_STACK
- REG_S s0, TASK_TI_KERNEL_SP(tp)
+ addi t0, sp, PT_SIZE_ON_STACK
+ REG_S t0, TASK_TI_KERNEL_SP(tp)

/* Save the kernel shadow call stack pointer */
scs_save_current
@@ -148,7 +148,6 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
*/
csrw CSR_SCRATCH, tp
1:
- REG_L a0, PT_STATUS(sp)
/*
* The current load reservation is effectively part of the processor's
* state, in the sense that load reservations cannot be shared between
@@ -169,7 +168,7 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
REG_L a2, PT_EPC(sp)
REG_SC x0, a2, PT_EPC(sp)

- csrw CSR_STATUS, a0
+ csrw CSR_STATUS, s1
csrw CSR_EPC, a2

REG_L x1, PT_RA(sp)
@@ -207,13 +206,13 @@ SYM_CODE_START_LOCAL(handle_kernel_stack_overflow)
REG_S x5, PT_T0(sp)
save_from_x6_to_x31

- REG_L s0, TASK_TI_KERNEL_SP(tp)
+ REG_L a2, TASK_TI_KERNEL_SP(tp)
csrr s1, CSR_STATUS
csrr s2, CSR_EPC
csrr s3, CSR_TVAL
csrr s4, CSR_CAUSE
csrr s5, CSR_SCRATCH
- REG_S s0, PT_SP(sp)
+ REG_S a2, PT_SP(sp)
REG_S s1, PT_STATUS(sp)
REG_S s2, PT_EPC(sp)
REG_S s3, PT_BADADDR(sp)
@@ -227,10 +226,10 @@ ASM_NOKPROBE(handle_kernel_stack_overflow)

SYM_CODE_START(ret_from_fork)
call schedule_tail
- beqz s0, 1f /* not from kernel thread */
+ beqz s1, 1f /* not from kernel thread */
/* Call fn(arg) */
- move a0, s1
- jalr s0
+ move a0, s2
+ jalr s1
1:
move a0, sp /* pt_regs */
la ra, ret_from_exception
diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
index e4bc61c4e58a..5512c31e1256 100644
--- a/arch/riscv/kernel/process.c
+++ b/arch/riscv/kernel/process.c
@@ -208,8 +208,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
/* Supervisor/Machine, irqs on: */
childregs->status = SR_PP | SR_PIE;

- p->thread.s[0] = (unsigned long)args->fn;
- p->thread.s[1] = (unsigned long)args->fn_arg;
+ p->thread.s[1] = (unsigned long)args->fn;
+ p->thread.s[2] = (unsigned long)args->fn_arg;
} else {
*childregs = *(current_pt_regs());
/* Turn off status.VS */
@@ -219,7 +219,6 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
if (clone_flags & CLONE_SETTLS)
childregs->tp = tls;
childregs->a0 = 0; /* Return value of fork() */
- p->thread.s[0] = 0;
}
p->thread.riscv_v_flags = 0;
if (has_vector())
--
2.44.1

2024-05-30 00:18:32

by Samuel Holland

[permalink] [raw]

Subject: [PATCH 4/4] riscv: entry: Save a frame record for exceptions

This follows the frame pointer ABI and allows stack traces to cross
exception boundaries without a special case in the stack walking code.

Signed-off-by: Samuel Holland <[email protected]>
---

arch/riscv/include/asm/processor.h | 9 +++++++--
arch/riscv/include/asm/ptrace.h | 5 +++++
arch/riscv/include/asm/stacktrace.h | 5 -----
arch/riscv/kernel/asm-offsets.c | 6 +++---
arch/riscv/kernel/entry.S | 16 ++++++++++------
arch/riscv/kernel/head.S | 6 ++----
arch/riscv/kernel/stacktrace.c | 9 ---------
7 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h
index 68c3432dc6ea..ccbb1e363c7f 100644
--- a/arch/riscv/include/asm/processor.h
+++ b/arch/riscv/include/asm/processor.h
@@ -144,9 +144,14 @@ static inline void arch_thread_struct_whitelist(unsigned long *offset,
.align_ctl = PR_UNALIGN_NOPRINT, \
}

+#ifdef CONFIG_FRAME_POINTER
+#define EXCEPTION_FRAME_SIZE ALIGN(sizeof(struct pt_regs) + sizeof(struct stackframe), STACK_ALIGN)
+#else
+#define EXCEPTION_FRAME_SIZE ALIGN(sizeof(struct pt_regs), STACK_ALIGN)
+#endif
+
#define task_pt_regs(tsk) \
- ((struct pt_regs *)(task_stack_page(tsk) + THREAD_SIZE \
- - ALIGN(sizeof(struct pt_regs), STACK_ALIGN)))
+ ((struct pt_regs *)(task_stack_page(tsk) + THREAD_SIZE - EXCEPTION_FRAME_SIZE))

#define KSTK_EIP(tsk) (task_pt_regs(tsk)->epc)
#define KSTK_ESP(tsk) (task_pt_regs(tsk)->sp)
diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
index b5b0adcc85c1..f475f6acec49 100644
--- a/arch/riscv/include/asm/ptrace.h
+++ b/arch/riscv/include/asm/ptrace.h
@@ -12,6 +12,11 @@

#ifndef __ASSEMBLY__

+struct stackframe {
+ unsigned long fp;
+ unsigned long ra;
+};
+
struct pt_regs {
unsigned long epc;
unsigned long ra;
diff --git a/arch/riscv/include/asm/stacktrace.h b/arch/riscv/include/asm/stacktrace.h
index b1495a7e06ce..3019558f747c 100644
--- a/arch/riscv/include/asm/stacktrace.h
+++ b/arch/riscv/include/asm/stacktrace.h
@@ -6,11 +6,6 @@
#include <linux/sched.h>
#include <asm/ptrace.h>

-struct stackframe {
- unsigned long fp;
- unsigned long ra;
-};
-
extern void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
bool (*fn)(void *, unsigned long), void *arg);
extern void dump_backtrace(struct pt_regs *regs, struct task_struct *task,
diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c
index 84c056f5ee09..582b52713e93 100644
--- a/arch/riscv/kernel/asm-offsets.c
+++ b/arch/riscv/kernel/asm-offsets.c
@@ -477,10 +477,10 @@ void asm_offsets(void)
);

/*
- * We allocate a pt_regs on the stack when entering the kernel. This
- * ensures the alignment is sane.
+ * We allocate a pt_regs and possibly a stackframe on the stack when
+ * entering the kernel. This ensures the alignment is sane.
*/
- DEFINE(PT_SIZE_ON_STACK, ALIGN(sizeof(struct pt_regs), STACK_ALIGN));
+ DEFINE(EXCEPTION_FRAME_SIZE, EXCEPTION_FRAME_SIZE);

OFFSET(KERNEL_MAP_VIRT_ADDR, kernel_mapping, virt_addr);
OFFSET(SBI_HART_BOOT_TASK_PTR_OFFSET, sbi_hart_boot_data, task_ptr);
diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
index bd1c5621df45..cdb58ce32cbb 100644
--- a/arch/riscv/kernel/entry.S
+++ b/arch/riscv/kernel/entry.S
@@ -33,7 +33,7 @@ SYM_CODE_START(handle_exception)
REG_S sp, TASK_TI_KERNEL_SP(tp)

#ifdef CONFIG_VMAP_STACK
- addi sp, sp, -(PT_SIZE_ON_STACK)
+ addi sp, sp, -EXCEPTION_FRAME_SIZE
srli sp, sp, THREAD_SHIFT
andi sp, sp, 0x1
bnez sp, handle_kernel_stack_overflow
@@ -43,7 +43,7 @@ SYM_CODE_START(handle_exception)
.Lsave_context:
REG_S sp, TASK_TI_USER_SP(tp)
REG_L sp, TASK_TI_KERNEL_SP(tp)
- addi sp, sp, -(PT_SIZE_ON_STACK)
+ addi sp, sp, -EXCEPTION_FRAME_SIZE
REG_S x1, PT_RA(sp)
REG_S x3, PT_GP(sp)
REG_S x5, PT_T0(sp)
@@ -83,6 +83,12 @@ SYM_CODE_START(handle_exception)
/* Load the kernel shadow call stack pointer if coming from userspace */
scs_load_current_if_task_changed s5

+#ifdef CONFIG_FRAME_POINTER
+ REG_S ra, (EXCEPTION_FRAME_SIZE + STACKFRAME_RA)(sp)
+ REG_S s0, (EXCEPTION_FRAME_SIZE + STACKFRAME_FP)(sp)
+ addi s0, sp, EXCEPTION_FRAME_SIZE
+#endif
+
#ifdef CONFIG_RISCV_ISA_V_PREEMPTIVE
move a0, sp
call riscv_v_context_nesting_start
@@ -136,7 +142,7 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
bnez t0, 1f

/* Save unwound kernel stack pointer in thread_info */
- addi t0, sp, PT_SIZE_ON_STACK
+ addi t0, sp, EXCEPTION_FRAME_SIZE
REG_S t0, TASK_TI_KERNEL_SP(tp)

/* Save the kernel shadow call stack pointer */
@@ -192,14 +198,12 @@ SYM_CODE_START_LOCAL(handle_kernel_stack_overflow)
/* we reach here from kernel context, sscratch must be 0 */
csrrw x31, CSR_SCRATCH, x31
asm_per_cpu sp, overflow_stack, x31
- li x31, OVERFLOW_STACK_SIZE
+ li x31, OVERFLOW_STACK_SIZE - EXCEPTION_FRAME_SIZE
add sp, sp, x31
/* zero out x31 again and restore x31 */
xor x31, x31, x31
csrrw x31, CSR_SCRATCH, x31

- addi sp, sp, -(PT_SIZE_ON_STACK)
-
//save context to overflow stack
REG_S x1, PT_RA(sp)
REG_S x3, PT_GP(sp)
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 4236a69c35cb..09ee5e6c2a98 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -290,9 +290,8 @@ SYM_CODE_START(_start_kernel)

/* Initialize page tables and relocate to virtual addresses */
la tp, init_task
- la sp, init_thread_union + THREAD_SIZE
+ la sp, init_thread_union + THREAD_SIZE - EXCEPTION_FRAME_SIZE
XIP_FIXUP_OFFSET sp
- addi sp, sp, -PT_SIZE_ON_STACK
scs_load_init_stack
#ifdef CONFIG_BUILTIN_DTB
la a0, __dtb_start
@@ -310,8 +309,7 @@ SYM_CODE_START(_start_kernel)
call .Lsetup_trap_vector
/* Restore C environment */
la tp, init_task
- la sp, init_thread_union + THREAD_SIZE
- addi sp, sp, -PT_SIZE_ON_STACK
+ la sp, init_thread_union + THREAD_SIZE - EXCEPTION_FRAME_SIZE
scs_load_current

#ifdef CONFIG_KASAN
diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c
index 528ec7cc9a62..6be8f8942f6b 100644
--- a/arch/riscv/kernel/stacktrace.c
+++ b/arch/riscv/kernel/stacktrace.c
@@ -16,8 +16,6 @@

#ifdef CONFIG_FRAME_POINTER

-extern asmlinkage void ret_from_exception(void);
-
static inline int fp_is_valid(unsigned long fp, unsigned long sp)
{
unsigned long low, high;
@@ -70,13 +68,6 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs,
fp = frame->fp;
pc = ftrace_graph_ret_addr(current, NULL, frame->ra,
&frame->ra);
- if (pc == (unsigned long)ret_from_exception) {
- if (unlikely(!__kernel_text_address(pc) || !fn(arg, pc)))
- break;
-
- pc = ((struct pt_regs *)sp)->epc;
- fp = ((struct pt_regs *)sp)->s0;
- }
}

}
--
2.44.1

2024-05-30 13:36:38

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 4/4] riscv: entry: Save a frame record for exceptions

2024-05-30 14:23:19

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 4/4] riscv: entry: Save a frame record for exceptions

Hi Samuel,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc1 next-20240529]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Samuel-Holland/riscv-Fix-32-bit-call_on_irq_stack-frame-pointer-ABI/20240530-081923
base: linus/master
patch link: https://lore.kernel.org/r/20240530001733.1407654-5-samuel.holland%40sifive.com
patch subject: [PATCH 4/4] riscv: entry: Save a frame record for exceptions
config: riscv-randconfig-001-20240530 (https://download.01.org/0day-ci/archive/20240530/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project bafda89a0944d947fc4b3b5663185e07a397ac30)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240530/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

>> arch/riscv/kernel/probes/rethook_trampoline.S:79:15: error: operand must be a symbol with %lo/%pcrel_lo/%tprel_lo modifier or an integer in the range [-2048, 2047]
addi sp, sp, -(PT_SIZE_ON_STACK)
^
arch/riscv/kernel/probes/rethook_trampoline.S:90:15: error: operand must be a symbol with %lo/%pcrel_lo/%tprel_lo modifier or an integer in the range [-2048, 2047]
addi sp, sp, PT_SIZE_ON_STACK
^

vim +79 arch/riscv/kernel/probes/rethook_trampoline.S

c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 9
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 10 .text
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 11 .altmacro
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 12
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 13 .macro save_all_base_regs
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 14 REG_S x1, PT_RA(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 15 REG_S x3, PT_GP(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 16 REG_S x4, PT_TP(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 17 REG_S x5, PT_T0(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 18 REG_S x6, PT_T1(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 19 REG_S x7, PT_T2(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 20 REG_S x8, PT_S0(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 21 REG_S x9, PT_S1(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 22 REG_S x10, PT_A0(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 23 REG_S x11, PT_A1(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 24 REG_S x12, PT_A2(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 25 REG_S x13, PT_A3(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 26 REG_S x14, PT_A4(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 27 REG_S x15, PT_A5(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 28 REG_S x16, PT_A6(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 29 REG_S x17, PT_A7(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 30 REG_S x18, PT_S2(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 31 REG_S x19, PT_S3(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 32 REG_S x20, PT_S4(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 33 REG_S x21, PT_S5(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 34 REG_S x22, PT_S6(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 35 REG_S x23, PT_S7(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 36 REG_S x24, PT_S8(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 37 REG_S x25, PT_S9(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 38 REG_S x26, PT_S10(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 39 REG_S x27, PT_S11(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 40 REG_S x28, PT_T3(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 41 REG_S x29, PT_T4(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 42 REG_S x30, PT_T5(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 43 REG_S x31, PT_T6(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 44 .endm
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 45
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 46 .macro restore_all_base_regs
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 47 REG_L x3, PT_GP(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 48 REG_L x4, PT_TP(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 49 REG_L x5, PT_T0(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 50 REG_L x6, PT_T1(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 51 REG_L x7, PT_T2(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 52 REG_L x8, PT_S0(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 53 REG_L x9, PT_S1(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 54 REG_L x10, PT_A0(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 55 REG_L x11, PT_A1(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 56 REG_L x12, PT_A2(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 57 REG_L x13, PT_A3(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 58 REG_L x14, PT_A4(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 59 REG_L x15, PT_A5(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 60 REG_L x16, PT_A6(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 61 REG_L x17, PT_A7(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 62 REG_L x18, PT_S2(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 63 REG_L x19, PT_S3(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 64 REG_L x20, PT_S4(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 65 REG_L x21, PT_S5(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 66 REG_L x22, PT_S6(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 67 REG_L x23, PT_S7(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 68 REG_L x24, PT_S8(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 69 REG_L x25, PT_S9(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 70 REG_L x26, PT_S10(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 71 REG_L x27, PT_S11(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 72 REG_L x28, PT_T3(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 73 REG_L x29, PT_T4(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 74 REG_L x30, PT_T5(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 75 REG_L x31, PT_T6(sp)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 76 .endm
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 77
76329c693924d8 arch/riscv/kernel/probes/rethook_trampoline.S Cl?ment L?ger 2023-10-24 78 SYM_CODE_START(arch_rethook_trampoline)
c22b0bcb1dd024 arch/riscv/kernel/probes/kprobes_trampoline.S Guo Ren 2020-12-17 @79 addi sp, sp, -(PT_SIZE_ON_STACK)

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2024-06-11 05:44:49

by Andy Chiu

[permalink] [raw]

Subject: Re: [PATCH 3/4] riscv: entry: Do not clobber the frame pointer

Hi Samuel,

On Thu, May 30, 2024 at 8:17 AM Samuel Holland
<[email protected]> wrote:
>
> s0 is reserved for the frame pointer, so it should not be used as a
> temporary register. Clobbering the frame pointer breaks stack traces.
>
> - In handle_exception() and ret_from_exception(), use a2 for the saved
> stack pointer. a2 is chosen because r2 is the stack pointer register.
> - In ret_from_exception(), use s1 for the saved status CSR value. Avoid
> clobbering s1 in the privilege mode check so it does not need to be
> reloaded later in the function.
> - Use s1 and s2 in ret_from_fork() instead of s0 and s1. The entire
> p->thread.s array is zeroed at the beginning of copy_thread(), so the
> registers do not need to be zeroed separately for kernel threads.
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> arch/riscv/kernel/entry.S | 29 ++++++++++++++---------------
> arch/riscv/kernel/process.c | 5 ++---
> 2 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> index d13d1aad7649..bd1c5621df45 100644
> --- a/arch/riscv/kernel/entry.S
> +++ b/arch/riscv/kernel/entry.S
> @@ -58,13 +58,13 @@ SYM_CODE_START(handle_exception)
> */
> li t0, SR_SUM | SR_FS_VS
>
> - REG_L s0, TASK_TI_USER_SP(tp)
> + REG_L a2, TASK_TI_USER_SP(tp)
> csrrc s1, CSR_STATUS, t0
> csrr s2, CSR_EPC
> csrr s3, CSR_TVAL
> csrr s4, CSR_CAUSE
> csrr s5, CSR_SCRATCH
> - REG_S s0, PT_SP(sp)
> + REG_S a2, PT_SP(sp)
> REG_S s1, PT_STATUS(sp)
> REG_S s2, PT_EPC(sp)
> REG_S s3, PT_BADADDR(sp)
> @@ -125,19 +125,19 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
> call riscv_v_context_nesting_end
> #endif
>
> - REG_L s0, PT_STATUS(sp)
> + REG_L s1, PT_STATUS(sp)
> #ifdef CONFIG_RISCV_M_MODE
> /* the MPP value is too large to be used as an immediate arg for addi */
> li t0, SR_MPP
> - and s0, s0, t0
> + and t0, s1, t0
> #else
> - andi s0, s0, SR_SPP
> + andi t0, s1, SR_SPP
> #endif
> - bnez s0, 1f
> + bnez t0, 1f
>
> /* Save unwound kernel stack pointer in thread_info */
> - addi s0, sp, PT_SIZE_ON_STACK
> - REG_S s0, TASK_TI_KERNEL_SP(tp)
> + addi t0, sp, PT_SIZE_ON_STACK
> + REG_S t0, TASK_TI_KERNEL_SP(tp)
>
> /* Save the kernel shadow call stack pointer */
> scs_save_current
> @@ -148,7 +148,6 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
> */
> csrw CSR_SCRATCH, tp
> 1:
> - REG_L a0, PT_STATUS(sp)
> /*
> * The current load reservation is effectively part of the processor's
> * state, in the sense that load reservations cannot be shared between
> @@ -169,7 +168,7 @@ SYM_CODE_START_NOALIGN(ret_from_exception)
> REG_L a2, PT_EPC(sp)
> REG_SC x0, a2, PT_EPC(sp)
>
> - csrw CSR_STATUS, a0
> + csrw CSR_STATUS, s1
> csrw CSR_EPC, a2
>
> REG_L x1, PT_RA(sp)
> @@ -207,13 +206,13 @@ SYM_CODE_START_LOCAL(handle_kernel_stack_overflow)
> REG_S x5, PT_T0(sp)
> save_from_x6_to_x31
>
> - REG_L s0, TASK_TI_KERNEL_SP(tp)
> + REG_L a2, TASK_TI_KERNEL_SP(tp)
> csrr s1, CSR_STATUS
> csrr s2, CSR_EPC
> csrr s3, CSR_TVAL
> csrr s4, CSR_CAUSE
> csrr s5, CSR_SCRATCH
> - REG_S s0, PT_SP(sp)
> + REG_S a2, PT_SP(sp)
> REG_S s1, PT_STATUS(sp)
> REG_S s2, PT_EPC(sp)
> REG_S s3, PT_BADADDR(sp)
> @@ -227,10 +226,10 @@ ASM_NOKPROBE(handle_kernel_stack_overflow)
>
> SYM_CODE_START(ret_from_fork)
> call schedule_tail
> - beqz s0, 1f /* not from kernel thread */
> + beqz s1, 1f /* not from kernel thread */
> /* Call fn(arg) */
> - move a0, s1
> - jalr s0
> + move a0, s2
> + jalr s1
> 1:
> move a0, sp /* pt_regs */
> la ra, ret_from_exception
> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index e4bc61c4e58a..5512c31e1256 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -208,8 +208,8 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> /* Supervisor/Machine, irqs on: */
> childregs->status = SR_PP | SR_PIE;
>
> - p->thread.s[0] = (unsigned long)args->fn;
> - p->thread.s[1] = (unsigned long)args->fn_arg;
> + p->thread.s[1] = (unsigned long)args->fn;
> + p->thread.s[2] = (unsigned long)args->fn_arg;
> } else {
> *childregs = *(current_pt_regs());
> /* Turn off status.VS */
> @@ -219,7 +219,6 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> if (clone_flags & CLONE_SETTLS)
> childregs->tp = tls;
> childregs->a0 = 0; /* Return value of fork() */
> - p->thread.s[0] = 0;
> }
> p->thread.riscv_v_flags = 0;
> if (has_vector())
> --
> 2.44.1
>

Reviewed-by: Andy Chiu <[email protected]>

Cheers,
Andy