Received-SPF: pass (google.com: domain of linux-kernel+bounces-194799-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Samuel Holland <samuel.holland@sifive.com>
To: Palmer Dabbelt <palmer@dabbelt.com>
Cc: linux-kernel@vger.kernel.org,
	Andy Chiu <andy.chiu@sifive.com>,
	linux-riscv@lists.infradead.org,
	Matthew Bystrin <dev.mbstr@gmail.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	Samuel Holland <samuel.holland@sifive.com>
Subject: [PATCH 3/4] riscv: entry: Do not clobber the frame pointer
Date: Wed, 29 May 2024 17:15:58 -0700
Message-ID: <20240530001733.1407654-4-samuel.holland@sifive.com>
In-Reply-To: <20240530001733.1407654-1-samuel.holland@sifive.com>
References: <20240530001733.1407654-1-samuel.holland@sifive.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

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 <samuel.holland@sifive.com>
---

 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