diff mbox series

riscv: entry: Save a0 prior syscall_enter_from_user_mode()

Message ID 20230403065207.1070974-1-bjorn@kernel.org (mailing list archive)
State Accepted
Commit 9c2598d43510eff09c658f9c0e0f921ba1871c4b
Headers show
Series riscv: entry: Save a0 prior syscall_enter_from_user_mode() | expand

Checks

Context Check Description
conchuod/cover_letter success Single patches do not need cover letters
conchuod/tree_selection success Guessed tree name to be for-next at HEAD d34a6b715a23
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 1 and now 1
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 18 this patch: 18
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 18 this patch: 18
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 3 this patch: 3
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch warning WARNING: Reported-by: should be immediately followed by Link: with a URL to the report
conchuod/source_inline success Was 0 now: 0
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success Fixes tag looks correct
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Björn Töpel April 3, 2023, 6:52 a.m. UTC 
From: Björn Töpel <bjorn@rivosinc.com>

The RISC-V calling convention passes the first argument, and the
return value in the a0 register. For this reason, the a0 register
needs some extra care; When handling syscalls, the a0 register is
saved into regs->orig_a0, so a0 can be properly restored for,
e.g. interrupted syscalls.

This functionality was broken with the introduction of the generic
entry patches. Here, a0 was saved into orig_a0 after calling
syscall_enter_from_user_mode(), which can change regs->a0 for some
paths, incorrectly restoring a0.

This is resolved, by saving a0 prior doing the
syscall_enter_from_user_mode() call.

Fixes: f0bddf50586d ("riscv: entry: Convert to generic entry")
Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
Tested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
 arch/riscv/kernel/traps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


base-commit: d34a6b715a23ccd9c9d0bc7a475bea59dc3e28b2

Comments

Conor Dooley April 3, 2023, 4:21 p.m. UTC | #1 
On Mon, Apr 03, 2023 at 08:52:07AM +0200, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> The RISC-V calling convention passes the first argument, and the
> return value in the a0 register. For this reason, the a0 register
> needs some extra care; When handling syscalls, the a0 register is
> saved into regs->orig_a0, so a0 can be properly restored for,
> e.g. interrupted syscalls.
> 
> This functionality was broken with the introduction of the generic
> entry patches. Here, a0 was saved into orig_a0 after calling
> syscall_enter_from_user_mode(), which can change regs->a0 for some
> paths, incorrectly restoring a0.
> 
> This is resolved, by saving a0 prior doing the
> syscall_enter_from_user_mode() call.
> 
> Fixes: f0bddf50586d ("riscv: entry: Convert to generic entry")
> Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Tested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>

As you pointed out, v12 did indeed have this ordering, so *deep breath*
Reported-by: Conor Dooley <conor.dooley@microchip.com>
Link: https://lore.kernel.org/linux-riscv/60ee7c26-1a70-427d-beaf-92e2989fc479@spud/
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Tested-by: Conor Dooley <conor.dooley@microchip.com>

Thanks for fixing this Björn!
> ---
>  arch/riscv/kernel/traps.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 1f4e37be7eb3..8c258b78c925 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -270,11 +270,11 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>  	if (user_mode(regs)) {
>  		ulong syscall = regs->a7;
>  
> -		syscall = syscall_enter_from_user_mode(regs, syscall);
> -
>  		regs->epc += 4;
>  		regs->orig_a0 = regs->a0;
>  
> +		syscall = syscall_enter_from_user_mode(regs, syscall);
> +
>  		if (syscall < NR_syscalls)
>  			syscall_handler(regs, syscall);
>  		else
> 
> base-commit: d34a6b715a23ccd9c9d0bc7a475bea59dc3e28b2
> -- 
> 2.37.2
>
Geert Uytterhoeven April 4, 2023, 3:06 p.m. UTC | #2 
On Mon, Apr 3, 2023 at 8:54 AM Björn Töpel <bjorn@kernel.org> wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
>
> The RISC-V calling convention passes the first argument, and the
> return value in the a0 register. For this reason, the a0 register
> needs some extra care; When handling syscalls, the a0 register is
> saved into regs->orig_a0, so a0 can be properly restored for,
> e.g. interrupted syscalls.
>
> This functionality was broken with the introduction of the generic
> entry patches. Here, a0 was saved into orig_a0 after calling
> syscall_enter_from_user_mode(), which can change regs->a0 for some
> paths, incorrectly restoring a0.
>
> This is resolved, by saving a0 prior doing the
> syscall_enter_from_user_mode() call.
>
> Fixes: f0bddf50586d ("riscv: entry: Convert to generic entry")
> Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Tested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Andy Chiu April 10, 2023, 9:41 a.m. UTC | #3 
On Mon, Apr 3, 2023 at 2:54 PM Björn Töpel <bjorn@kernel.org> wrote:
>
> From: Björn Töpel <bjorn@rivosinc.com>
>
> The RISC-V calling convention passes the first argument, and the
> return value in the a0 register. For this reason, the a0 register
> needs some extra care; When handling syscalls, the a0 register is
> saved into regs->orig_a0, so a0 can be properly restored for,
> e.g. interrupted syscalls.
>
> This functionality was broken with the introduction of the generic
> entry patches. Here, a0 was saved into orig_a0 after calling
> syscall_enter_from_user_mode(), which can change regs->a0 for some
> paths, incorrectly restoring a0.
>
> This is resolved, by saving a0 prior doing the
> syscall_enter_from_user_mode() call.
>
> Fixes: f0bddf50586d ("riscv: entry: Convert to generic entry")
> Reviewed-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Tested-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>

This fix works for me, thanks!

Tested-by: Andy Chiu <andy.chiu@sifive.com>
Palmer Dabbelt April 11, 2023, 5:39 p.m. UTC | #4 
On Mon, 03 Apr 2023 08:52:07 +0200, Björn Töpel wrote:
> The RISC-V calling convention passes the first argument, and the
> return value in the a0 register. For this reason, the a0 register
> needs some extra care; When handling syscalls, the a0 register is
> saved into regs->orig_a0, so a0 can be properly restored for,
> e.g. interrupted syscalls.
> 
> This functionality was broken with the introduction of the generic
> entry patches. Here, a0 was saved into orig_a0 after calling
> syscall_enter_from_user_mode(), which can change regs->a0 for some
> paths, incorrectly restoring a0.
> 
> [...]

Applied, thanks!

[1/1] riscv: entry: Save a0 prior syscall_enter_from_user_mode()
      https://git.kernel.org/palmer/c/9c2598d43510

Best regards,
patchwork-bot+linux-riscv@kernel.org April 11, 2023, 5:40 p.m. UTC | #5 
Hello:

This patch was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Mon,  3 Apr 2023 08:52:07 +0200 you wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> The RISC-V calling convention passes the first argument, and the
> return value in the a0 register. For this reason, the a0 register
> needs some extra care; When handling syscalls, the a0 register is
> saved into regs->orig_a0, so a0 can be properly restored for,
> e.g. interrupted syscalls.
> 
> [...]

Here is the summary with links:
  - riscv: entry: Save a0 prior syscall_enter_from_user_mode()
    https://git.kernel.org/riscv/c/9c2598d43510

You are awesome, thank you!
diff mbox series

Patch

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 1f4e37be7eb3..8c258b78c925 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -270,11 +270,11 @@  asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
 	if (user_mode(regs)) {
 		ulong syscall = regs->a7;
 
-		syscall = syscall_enter_from_user_mode(regs, syscall);
-
 		regs->epc += 4;
 		regs->orig_a0 = regs->a0;
 
+		syscall = syscall_enter_from_user_mode(regs, syscall);
+
 		if (syscall < NR_syscalls)
 			syscall_handler(regs, syscall);
 		else