diff mbox series

[v3] riscv: Discard vector state on syscalls

Message ID 20230629062730.985184-1-bjorn@kernel.org (mailing list archive)
State Superseded
Headers show
Series [v3] riscv: Discard vector state on syscalls | 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 488833ccdcac
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 6 and now 6
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 fail Failed to build the tree with this patch.
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 120 this patch: 120
conchuod/build_rv32_defconfig fail Build failed
conchuod/dtb_warn_rv64 success Errors and warnings before: 20 this patch: 20
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch warning CHECK: Lines should not end with a '('
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Björn Töpel June 29, 2023, 6:27 a.m. UTC 
From: Björn Töpel <bjorn@rivosinc.com>

The RISC-V vector specification states:
  Executing a system call causes all caller-saved vector registers
  (v0-v31, vl, vtype) and vstart to become unspecified.

The vector registers are set to all 1s, vill is set (invalid), and the
vector status is set to Dirty.

That way we can prevent userspace from accidentally relying on the
stated save.

Rémi pointed out [1] that writing to the registers might be
superfluous, and setting vill is sufficient.

Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1]
Suggested-by: Darius Rad <darius@bluespec.com>
Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
Suggested-by: Rémi Denis-Courmont <remi@remlab.net>
Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---

v2->v3:
  Set state to Dirty after discard, for proper ptrace() handling
  (Andy)

v1->v2:
  Proper register restore for initial state (Andy)
  Set registers to 1s, and not 0s (Darius)

---
 arch/riscv/include/asm/vector.h | 33 +++++++++++++++++++++++++++++++++
 arch/riscv/kernel/traps.c       |  2 ++
 2 files changed, 35 insertions(+)


base-commit: 488833ccdcac118da16701f4ee0673b20ba47fe3

Comments

Conor Dooley June 29, 2023, 7:16 a.m. UTC | #1 
Hey,

On Thu, Jun 29, 2023 at 08:27:30AM +0200, Björn Töpel wrote:
> From: Björn Töpel <bjorn@rivosinc.com>
> 
> The RISC-V vector specification states:
>   Executing a system call causes all caller-saved vector registers
>   (v0-v31, vl, vtype) and vstart to become unspecified.
> 
> The vector registers are set to all 1s, vill is set (invalid), and the
> vector status is set to Dirty.
> 
> That way we can prevent userspace from accidentally relying on the
> stated save.
> 
> Rémi pointed out [1] that writing to the registers might be
> superfluous, and setting vill is sufficient.
> 
> Link: https://lore.kernel.org/linux-riscv/12784326.9UPPK3MAeB@basile.remlab.net/ # [1]
> Suggested-by: Darius Rad <darius@bluespec.com>
> Suggested-by: Palmer Dabbelt <palmer@rivosinc.com>
> Suggested-by: Rémi Denis-Courmont <remi@remlab.net>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>

clang allmodconfig and rv32_defconfig fail to build with this patch,
according to patchwork:
../arch/riscv/kernel/traps.c:299:3: error: call to undeclared function 'riscv_v_vstate_discard'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]

Cheers,
Conor.

> ---
> 
> v2->v3:
>   Set state to Dirty after discard, for proper ptrace() handling
>   (Andy)
> 
> v1->v2:
>   Proper register restore for initial state (Andy)
>   Set registers to 1s, and not 0s (Darius)
> 
> ---
>  arch/riscv/include/asm/vector.h | 33 +++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/traps.c       |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
> index 04c0b07bf6cd..0b23056503c5 100644
> --- a/arch/riscv/include/asm/vector.h
> +++ b/arch/riscv/include/asm/vector.h
> @@ -33,6 +33,11 @@ static inline void __riscv_v_vstate_clean(struct pt_regs *regs)
>  	regs->status = (regs->status & ~SR_VS) | SR_VS_CLEAN;
>  }
>  
> +static inline void __riscv_v_vstate_dirty(struct pt_regs *regs)
> +{
> +	regs->status = (regs->status & ~SR_VS) | SR_VS_DIRTY;
> +}
> +
>  static inline void riscv_v_vstate_off(struct pt_regs *regs)
>  {
>  	regs->status = (regs->status & ~SR_VS) | SR_VS_OFF;
> @@ -128,6 +133,34 @@ static inline void __riscv_v_vstate_restore(struct __riscv_v_ext_state *restore_
>  	riscv_v_disable();
>  }
>  
> +static inline void __riscv_v_vstate_discard(void)
> +{
> +	unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1);
> +
> +	riscv_v_enable();
> +	asm volatile (
> +		".option push\n\t"
> +		".option arch, +v\n\t"
> +		"vsetvli	%0, x0, e8, m8, ta, ma\n\t"
> +		"vmv.v.i	v0, -1\n\t"
> +		"vmv.v.i	v8, -1\n\t"
> +		"vmv.v.i	v16, -1\n\t"
> +		"vmv.v.i	v24, -1\n\t"
> +		"vsetvl		%0, x0, %1\n\t"
> +		".option pop\n\t"
> +		: "=&r" (vl) : "r" (vtype_inval) : "memory");
> +	riscv_v_disable();
> +}
> +
> +static inline void riscv_v_vstate_discard(struct pt_regs *regs)
> +{
> +	if ((regs->status & SR_VS) == SR_VS_OFF)
> +		return;
> +
> +	__riscv_v_vstate_discard();
> +	__riscv_v_vstate_dirty(regs);
> +}
> +
>  static inline void riscv_v_vstate_save(struct task_struct *task,
>  				       struct pt_regs *regs)
>  {
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index 5158961ea977..5ff63a784a6d 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -296,6 +296,8 @@ asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
>  		regs->epc += 4;
>  		regs->orig_a0 = regs->a0;
>  
> +		riscv_v_vstate_discard(regs);
> +
>  		syscall = syscall_enter_from_user_mode(regs, syscall);
>  
>  		if (syscall < NR_syscalls)
> 
> base-commit: 488833ccdcac118da16701f4ee0673b20ba47fe3
> -- 
> 2.39.2
>
kernel test robot June 29, 2023, 8:04 a.m. UTC | #2 
Hi Björn,

kernel test robot noticed the following build errors:

[auto build test ERROR on 488833ccdcac118da16701f4ee0673b20ba47fe3]

url:    https://github.com/intel-lab-lkp/linux/commits/Bj-rn-T-pel/riscv-Discard-vector-state-on-syscalls/20230629-142852
base:   488833ccdcac118da16701f4ee0673b20ba47fe3
patch link:    https://lore.kernel.org/r/20230629062730.985184-1-bjorn%40kernel.org
patch subject: [PATCH v3] riscv: Discard vector state on syscalls
config: riscv-randconfig-r042-20230629 (https://download.01.org/0day-ci/archive/20230629/202306291513.DwaMo6k7-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce: (https://download.01.org/0day-ci/archive/20230629/202306291513.DwaMo6k7-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306291513.DwaMo6k7-lkp@intel.com/

All errors (new ones prefixed by >>):

         |                                          ~~~~~~~~~~ ^
   In file included from arch/riscv/kernel/traps.c:15:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:751:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     751 |         insw(addr, buffer, count);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:105:53: note: expanded from macro 'insw'
     105 | #define insw(addr, buffer, count) __insw(PCI_IOBASE + (addr), buffer, count)
         |                                          ~~~~~~~~~~ ^
   In file included from arch/riscv/kernel/traps.c:15:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:759:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     759 |         insl(addr, buffer, count);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:106:53: note: expanded from macro 'insl'
     106 | #define insl(addr, buffer, count) __insl(PCI_IOBASE + (addr), buffer, count)
         |                                          ~~~~~~~~~~ ^
   In file included from arch/riscv/kernel/traps.c:15:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:768:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     768 |         outsb(addr, buffer, count);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:118:55: note: expanded from macro 'outsb'
     118 | #define outsb(addr, buffer, count) __outsb(PCI_IOBASE + (addr), buffer, count)
         |                                            ~~~~~~~~~~ ^
   In file included from arch/riscv/kernel/traps.c:15:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:777:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     777 |         outsw(addr, buffer, count);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:119:55: note: expanded from macro 'outsw'
     119 | #define outsw(addr, buffer, count) __outsw(PCI_IOBASE + (addr), buffer, count)
         |                                            ~~~~~~~~~~ ^
   In file included from arch/riscv/kernel/traps.c:15:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:786:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     786 |         outsl(addr, buffer, count);
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:120:55: note: expanded from macro 'outsl'
     120 | #define outsl(addr, buffer, count) __outsl(PCI_IOBASE + (addr), buffer, count)
         |                                            ~~~~~~~~~~ ^
   In file included from arch/riscv/kernel/traps.c:15:
   In file included from include/linux/kprobes.h:28:
   In file included from include/linux/ftrace.h:10:
   In file included from include/linux/trace_recursion.h:5:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/riscv/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:1134:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
    1134 |         return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
         |                                                   ~~~~~~~~~~ ^
>> arch/riscv/kernel/traps.c:299:3: error: call to undeclared function 'riscv_v_vstate_discard'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     299 |                 riscv_v_vstate_discard(regs);
         |                 ^
   arch/riscv/kernel/traps.c:299:3: note: did you mean 'riscv_v_vstate_query'?
   arch/riscv/include/asm/vector.h:206:20: note: 'riscv_v_vstate_query' declared here
     206 | static inline bool riscv_v_vstate_query(struct pt_regs *regs) { return false; }
         |                    ^
   13 warnings and 1 error generated.


vim +/riscv_v_vstate_discard +299 arch/riscv/kernel/traps.c

   290	
   291	asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
   292	{
   293		if (user_mode(regs)) {
   294			ulong syscall = regs->a7;
   295	
   296			regs->epc += 4;
   297			regs->orig_a0 = regs->a0;
   298	
 > 299			riscv_v_vstate_discard(regs);
   300	
   301			syscall = syscall_enter_from_user_mode(regs, syscall);
   302	
   303			if (syscall < NR_syscalls)
   304				syscall_handler(regs, syscall);
   305			else
   306				regs->a0 = -ENOSYS;
   307	
   308			syscall_exit_to_user_mode(regs);
   309		} else {
   310			irqentry_state_t state = irqentry_nmi_enter(regs);
   311	
   312			do_trap_error(regs, SIGILL, ILL_ILLTRP, regs->epc,
   313				"Oops - environment call from U-mode");
   314	
   315			irqentry_nmi_exit(regs, state);
   316		}
   317
kernel test robot June 29, 2023, 12:25 p.m. UTC | #3 
Hi Björn,

kernel test robot noticed the following build errors:

[auto build test ERROR on 488833ccdcac118da16701f4ee0673b20ba47fe3]

url:    https://github.com/intel-lab-lkp/linux/commits/Bj-rn-T-pel/riscv-Discard-vector-state-on-syscalls/20230629-142852
base:   488833ccdcac118da16701f4ee0673b20ba47fe3
patch link:    https://lore.kernel.org/r/20230629062730.985184-1-bjorn%40kernel.org
patch subject: [PATCH v3] riscv: Discard vector state on syscalls
config: riscv-allnoconfig (https://download.01.org/0day-ci/archive/20230629/202306292011.OGfLGBam-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230629/202306292011.OGfLGBam-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306292011.OGfLGBam-lkp@intel.com/

All errors (new ones prefixed by >>):

   arch/riscv/kernel/traps.c: In function 'do_trap_ecall_u':
>> arch/riscv/kernel/traps.c:299:17: error: implicit declaration of function 'riscv_v_vstate_discard'; did you mean 'riscv_v_vstate_restore'? [-Werror=implicit-function-declaration]
     299 |                 riscv_v_vstate_discard(regs);
         |                 ^~~~~~~~~~~~~~~~~~~~~~
         |                 riscv_v_vstate_restore
   cc1: some warnings being treated as errors


vim +299 arch/riscv/kernel/traps.c

   290	
   291	asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
   292	{
   293		if (user_mode(regs)) {
   294			ulong syscall = regs->a7;
   295	
   296			regs->epc += 4;
   297			regs->orig_a0 = regs->a0;
   298	
 > 299			riscv_v_vstate_discard(regs);
   300	
   301			syscall = syscall_enter_from_user_mode(regs, syscall);
   302	
   303			if (syscall < NR_syscalls)
   304				syscall_handler(regs, syscall);
   305			else
   306				regs->a0 = -ENOSYS;
   307	
   308			syscall_exit_to_user_mode(regs);
   309		} else {
   310			irqentry_state_t state = irqentry_nmi_enter(regs);
   311	
   312			do_trap_error(regs, SIGILL, ILL_ILLTRP, regs->epc,
   313				"Oops - environment call from U-mode");
   314	
   315			irqentry_nmi_exit(regs, state);
   316		}
   317
Björn Töpel June 29, 2023, 1:48 p.m. UTC | #4 
Conor Dooley <conor.dooley@microchip.com> writes:

> clang allmodconfig and rv32_defconfig fail to build with this patch,
> according to patchwork:
> ../arch/riscv/kernel/traps.c:299:3: error: call to undeclared function 'riscv_v_vstate_discard'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]

Ugh. Sloppy. :-(

Thank you!
Björn
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/vector.h b/arch/riscv/include/asm/vector.h
index 04c0b07bf6cd..0b23056503c5 100644
--- a/arch/riscv/include/asm/vector.h
+++ b/arch/riscv/include/asm/vector.h
@@ -33,6 +33,11 @@  static inline void __riscv_v_vstate_clean(struct pt_regs *regs)
 	regs->status = (regs->status & ~SR_VS) | SR_VS_CLEAN;
 }
 
+static inline void __riscv_v_vstate_dirty(struct pt_regs *regs)
+{
+	regs->status = (regs->status & ~SR_VS) | SR_VS_DIRTY;
+}
+
 static inline void riscv_v_vstate_off(struct pt_regs *regs)
 {
 	regs->status = (regs->status & ~SR_VS) | SR_VS_OFF;
@@ -128,6 +133,34 @@  static inline void __riscv_v_vstate_restore(struct __riscv_v_ext_state *restore_
 	riscv_v_disable();
 }
 
+static inline void __riscv_v_vstate_discard(void)
+{
+	unsigned long vl, vtype_inval = 1UL << (BITS_PER_LONG - 1);
+
+	riscv_v_enable();
+	asm volatile (
+		".option push\n\t"
+		".option arch, +v\n\t"
+		"vsetvli	%0, x0, e8, m8, ta, ma\n\t"
+		"vmv.v.i	v0, -1\n\t"
+		"vmv.v.i	v8, -1\n\t"
+		"vmv.v.i	v16, -1\n\t"
+		"vmv.v.i	v24, -1\n\t"
+		"vsetvl		%0, x0, %1\n\t"
+		".option pop\n\t"
+		: "=&r" (vl) : "r" (vtype_inval) : "memory");
+	riscv_v_disable();
+}
+
+static inline void riscv_v_vstate_discard(struct pt_regs *regs)
+{
+	if ((regs->status & SR_VS) == SR_VS_OFF)
+		return;
+
+	__riscv_v_vstate_discard();
+	__riscv_v_vstate_dirty(regs);
+}
+
 static inline void riscv_v_vstate_save(struct task_struct *task,
 				       struct pt_regs *regs)
 {
diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index 5158961ea977..5ff63a784a6d 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -296,6 +296,8 @@  asmlinkage __visible __trap_section void do_trap_ecall_u(struct pt_regs *regs)
 		regs->epc += 4;
 		regs->orig_a0 = regs->a0;
 
+		riscv_v_vstate_discard(regs);
+
 		syscall = syscall_enter_from_user_mode(regs, syscall);
 
 		if (syscall < NR_syscalls)