[PATCH v5 00/10] Improve signal performance on PPC64 with KUAP

[PATCH v5 00/10] Improve signal performance on PPC64 with KUAP

[Christopher M. Riedl]

As reported by Anton, there is a large penalty to signal handling
performance on radix systems using KUAP. The signal handling code
performs many user access operations, each of which needs to switch the
KUAP permissions bit to open and then close user access. This involves a
costly 'mtspr' operation [0].

There is existing work done on x86 and by Christophe Leroy for PPC32 to
instead open up user access in "blocks" using user_*_access_{begin,end}.
We can do the same in PPC64 to bring performance back up on KUAP-enabled
radix and now also hash MMU systems [1].

Hash MMU KUAP support along with uaccess flush has landed in linuxppc/next
since the last revision. This series also provides a large benefit on hash
with KUAP. However, in the hash implementation of KUAP the user AMR is
always restored during system_call_exception() which cannot be avoided.
Fewer user access switches naturally also result in less uaccess flushing.

The first two patches add some needed 'unsafe' versions of copy-from
functions. While these do not make use of asm-goto they still allow for
avoiding the repeated uaccess switches.

The third patch moves functions called by setup_sigcontext() into a new
prepare_setup_sigcontext() to simplify converting setup_sigcontext()
into an 'unsafe' version which assumes an open uaccess window later.

The fourth and fifths patches clean-up some of the Transactional Memory
ifdef stuff to simplify using uaccess blocks later.

The next two patches rewrite some of the signal64 helper functions to
be 'unsafe'. Finally, the last three patches update the main signal
handling functions to make use of the new 'unsafe' helpers and eliminate
some additional uaccess switching.

I used the will-it-scale signal1 benchmark to measure and compare
performance [2]. The below results are from running a minimal
kernel+initramfs QEMU/KVM guest on a POWER9 Blackbird:

	signal1_threads -t1 -s10

	|                              | hash   | radix  |
	| ---------------------------- | ------ | ------ |
	| linuxppc/next                | 117667 | 135752 |
	| linuxppc/next w/o KUAP+KUEP  | 225273 | 227567 |
	| unsafe-signal64              | 193402 | 230983 |

[0]: https://github.com/linuxppc/issues/issues/277
[1]: https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=196278
[2]: https://github.com/antonblanchard/will-it-scale/blob/master/tests/signal1.c

v5:	* Use sizeof(buf) in copy_{vsx,fpr}_from_user() (Thanks David Laight)
	* Rebase on latest linuxppc/next

v4:	* Fix issues identified by Christophe Leroy (Thanks for review)
	* Use __get_user() directly to copy the 8B sigset_t

v3:	* Rebase on latest linuxppc/next
	* Reword confusing commit messages
	* Add missing comma in macro in signal.h which broke compiles without
	  CONFIG_ALTIVEC
	* Validate hash KUAP signal performance improvements

v2:	* Rebase on latest linuxppc/next + Christophe Leroy's PPC32
	  signal series
	* Simplify/remove TM ifdefery similar to PPC32 series and clean
	  up the uaccess begin/end calls
	* Isolate non-inline functions so they are not called when
	  uaccess window is open

Christopher M. Riedl (8):
  powerpc/uaccess: Add unsafe_copy_from_user
  powerpc/signal: Add unsafe_copy_{vsx,fpr}_from_user()
  powerpc/signal64: Move non-inline functions out of setup_sigcontext()
  powerpc: Reference param in MSR_TM_ACTIVE() macro
  powerpc/signal64: Remove TM ifdefery in middle of if/else block
  powerpc/signal64: Replace setup_sigcontext() w/
    unsafe_setup_sigcontext()
  powerpc/signal64: Replace restore_sigcontext() w/
    unsafe_restore_sigcontext()
  powerpc/signal64: Use __get_user() to copy sigset_t

Daniel Axtens (2):
  powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess
    switches
  powerpc/signal64: Rewrite rt_sigreturn() to minimise uaccess switches

 arch/powerpc/include/asm/reg.h     |   2 +-
 arch/powerpc/include/asm/uaccess.h |   3 +
 arch/powerpc/kernel/signal.h       |  30 ++++
 arch/powerpc/kernel/signal_64.c    | 251 ++++++++++++++++++-----------
 4 files changed, 193 insertions(+), 93 deletions(-)

-- 
2.26.1

[PATCH v5 01/10] powerpc/uaccess: Add unsafe_copy_from_user

[Christopher M. Riedl]

Just wrap __copy_tofrom_user() for the usual 'unsafe' pattern which
takes in a label to goto on error.

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/include/asm/uaccess.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 501c9a79038c..036e82eefac9 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -542,6 +542,9 @@ user_write_access_begin(const void __user *ptr, size_t len)
 #define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
 #define unsafe_put_user(x, p, e) __put_user_goto(x, p, e)
 
+#define unsafe_copy_from_user(d, s, l, e) \
+	unsafe_op_wrap(__copy_tofrom_user((__force void __user *)d, s, l), e)
+
 #define unsafe_copy_to_user(d, s, l, e) \
 do {									\
 	u8 __user *_dst = (u8 __user *)(d);				\
-- 
2.26.1

Re: [PATCH v5 01/10] powerpc/uaccess: Add unsafe_copy_from_user

[Daniel Axtens]

Hi Chris,

Pending anything that sparse reported (which I haven't checked), this
looks ok to me.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

> Just wrap __copy_tofrom_user() for the usual 'unsafe' pattern which
> takes in a label to goto on error.
>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/include/asm/uaccess.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 501c9a79038c..036e82eefac9 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -542,6 +542,9 @@ user_write_access_begin(const void __user *ptr, size_t len)
>  #define unsafe_get_user(x, p, e) unsafe_op_wrap(__get_user_allowed(x, p), e)
>  #define unsafe_put_user(x, p, e) __put_user_goto(x, p, e)
>  
> +#define unsafe_copy_from_user(d, s, l, e) \
> +	unsafe_op_wrap(__copy_tofrom_user((__force void __user *)d, s, l), e)
> +
>  #define unsafe_copy_to_user(d, s, l, e) \
>  do {									\
>  	u8 __user *_dst = (u8 __user *)(d);				\
> -- 
> 2.26.1

[PATCH v5 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()

[Christopher M. Riedl]

Reuse the "safe" implementation from signal.c except for calling
unsafe_copy_from_user() to copy into a local buffer.

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal.h | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index 2559a681536e..7dfc536c78ef 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -53,6 +53,30 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
 				&buf[i], label);\
 } while (0)
 
+#define unsafe_copy_fpr_from_user(task, from, label)	do {		\
+	struct task_struct *__t = task;					\
+	u64 __user *__f = (u64 __user *)from;				\
+	u64 buf[ELF_NFPREG];						\
+	int i;								\
+									\
+	unsafe_copy_from_user(buf, __f, sizeof(buf), label);		\
+	for (i = 0; i < ELF_NFPREG - 1; i++)				\
+		__t->thread.TS_FPR(i) = buf[i];				\
+	__t->thread.fp_state.fpscr = buf[i];				\
+} while (0)
+
+#define unsafe_copy_vsx_from_user(task, from, label)	do {		\
+	struct task_struct *__t = task;					\
+	u64 __user *__f = (u64 __user *)from;				\
+	u64 buf[ELF_NVSRHALFREG];					\
+	int i;								\
+									\
+	unsafe_copy_from_user(buf, __f, sizeof(buf), label);		\
+	for (i = 0; i < ELF_NVSRHALFREG ; i++)				\
+		__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i];	\
+} while (0)
+
+
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 #define unsafe_copy_ckfpr_to_user(to, task, label)	do {		\
 	struct task_struct *__t = task;					\
@@ -80,6 +104,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
 	unsafe_copy_to_user(to, (task)->thread.fp_state.fpr,	\
 			    ELF_NFPREG * sizeof(double), label)
 
+#define unsafe_copy_fpr_from_user(task, from, label)			\
+	unsafe_copy_from_user((task)->thread.fp_state.fpr, from,	\
+			    ELF_NFPREG * sizeof(double), label)
+
 static inline unsigned long
 copy_fpr_to_user(void __user *to, struct task_struct *task)
 {
@@ -115,6 +143,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from)
 #else
 #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
 
+#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0)
+
 static inline unsigned long
 copy_fpr_to_user(void __user *to, struct task_struct *task)
 {
-- 
2.26.1

Re: [PATCH v5 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()

[Daniel Axtens]

Hi Christopher,

I have checked that each implementation matches the corresponding
*_to_user implementation. We've had some debate about whether the
overarching implementation in the to/from pairs (especially where things
go via a bounce buffer) can be simplified - but that's probably not
really something that this patch set should do.

On that basis:
Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

> Reuse the "safe" implementation from signal.c except for calling
> unsafe_copy_from_user() to copy into a local buffer.
>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/kernel/signal.h | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> index 2559a681536e..7dfc536c78ef 100644
> --- a/arch/powerpc/kernel/signal.h
> +++ b/arch/powerpc/kernel/signal.h
> @@ -53,6 +53,30 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
>  				&buf[i], label);\
>  } while (0)
>  
> +#define unsafe_copy_fpr_from_user(task, from, label)	do {		\
> +	struct task_struct *__t = task;					\
> +	u64 __user *__f = (u64 __user *)from;				\
> +	u64 buf[ELF_NFPREG];						\
> +	int i;								\
> +									\
> +	unsafe_copy_from_user(buf, __f, sizeof(buf), label);		\
> +	for (i = 0; i < ELF_NFPREG - 1; i++)				\
> +		__t->thread.TS_FPR(i) = buf[i];				\
> +	__t->thread.fp_state.fpscr = buf[i];				\
> +} while (0)
> +
> +#define unsafe_copy_vsx_from_user(task, from, label)	do {		\
> +	struct task_struct *__t = task;					\
> +	u64 __user *__f = (u64 __user *)from;				\
> +	u64 buf[ELF_NVSRHALFREG];					\
> +	int i;								\
> +									\
> +	unsafe_copy_from_user(buf, __f, sizeof(buf), label);		\
> +	for (i = 0; i < ELF_NVSRHALFREG ; i++)				\
> +		__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = buf[i];	\
> +} while (0)
> +
> +
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  #define unsafe_copy_ckfpr_to_user(to, task, label)	do {		\
>  	struct task_struct *__t = task;					\
> @@ -80,6 +104,10 @@ unsigned long copy_ckfpr_from_user(struct task_struct *task, void __user *from);
>  	unsafe_copy_to_user(to, (task)->thread.fp_state.fpr,	\
>  			    ELF_NFPREG * sizeof(double), label)
>  
> +#define unsafe_copy_fpr_from_user(task, from, label)			\
> +	unsafe_copy_from_user((task)->thread.fp_state.fpr, from,	\
> +			    ELF_NFPREG * sizeof(double), label)
> +
>  static inline unsigned long
>  copy_fpr_to_user(void __user *to, struct task_struct *task)
>  {
> @@ -115,6 +143,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from)
>  #else
>  #define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
>  
> +#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0)
> +
>  static inline unsigned long
>  copy_fpr_to_user(void __user *to, struct task_struct *task)
>  {
> -- 
> 2.26.1

[PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext()

[Christopher M. Riedl]

There are non-inline functions which get called in setup_sigcontext() to
save register state to the thread struct. Move these functions into a
separate prepare_setup_sigcontext() function so that
setup_sigcontext() can be refactored later into an "unsafe" version
which assumes an open uaccess window. Non-inline functions should be
avoided when uaccess is open.

The majority of setup_sigcontext() can be refactored to execute in an
"unsafe" context (uaccess window is opened) except for some non-inline
functions. Move these out into a separate prepare_setup_sigcontext()
function which must be called first and before opening up a uaccess
window. A follow-up commit converts setup_sigcontext() to be "unsafe".

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index f9e4a1ac440f..b211a8ea4f6e 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -79,6 +79,24 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
 }
 #endif
 
+static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_region)
+{
+#ifdef CONFIG_ALTIVEC
+	/* save altivec registers */
+	if (tsk->thread.used_vr)
+		flush_altivec_to_thread(tsk);
+	if (cpu_has_feature(CPU_FTR_ALTIVEC))
+		tsk->thread.vrsave = mfspr(SPRN_VRSAVE);
+#endif /* CONFIG_ALTIVEC */
+
+	flush_fp_to_thread(tsk);
+
+#ifdef CONFIG_VSX
+	if (tsk->thread.used_vsr && ctx_has_vsx_region)
+		flush_vsx_to_thread(tsk);
+#endif /* CONFIG_VSX */
+}
+
 /*
  * Set up the sigcontext for the signal frame.
  */
@@ -97,7 +115,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 	 */
 #ifdef CONFIG_ALTIVEC
 	elf_vrreg_t __user *v_regs = sigcontext_vmx_regs(sc);
-	unsigned long vrsave;
 #endif
 	struct pt_regs *regs = tsk->thread.regs;
 	unsigned long msr = regs->msr;
@@ -112,7 +129,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 
 	/* save altivec registers */
 	if (tsk->thread.used_vr) {
-		flush_altivec_to_thread(tsk);
 		/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
 		err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
 				      33 * sizeof(vector128));
@@ -124,17 +140,10 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 	/* We always copy to/from vrsave, it's 0 if we don't have or don't
 	 * use altivec.
 	 */
-	vrsave = 0;
-	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
-		vrsave = mfspr(SPRN_VRSAVE);
-		tsk->thread.vrsave = vrsave;
-	}
-
-	err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
+	err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
 #else /* CONFIG_ALTIVEC */
 	err |= __put_user(0, &sc->v_regs);
 #endif /* CONFIG_ALTIVEC */
-	flush_fp_to_thread(tsk);
 	/* copy fpr regs and fpscr */
 	err |= copy_fpr_to_user(&sc->fp_regs, tsk);
 
@@ -150,7 +159,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 	 * VMX data.
 	 */
 	if (tsk->thread.used_vsr && ctx_has_vsx_region) {
-		flush_vsx_to_thread(tsk);
 		v_regs += ELF_NVRREG;
 		err |= copy_vsx_to_user(v_regs, tsk);
 		/* set MSR_VSX in the MSR value in the frame to
@@ -655,6 +663,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
 		ctx_has_vsx_region = 1;
 
 	if (old_ctx != NULL) {
+		prepare_setup_sigcontext(current, ctx_has_vsx_region);
 		if (!access_ok(old_ctx, ctx_size)
 		    || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
 					ctx_has_vsx_region)
@@ -842,6 +851,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 #endif
 	{
 		err |= __put_user(0, &frame->uc.uc_link);
+		prepare_setup_sigcontext(tsk, 1);
 		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
 					NULL, (unsigned long)ksig->ka.sa.sa_handler,
 					1);
-- 
2.26.1

Re: [PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext()

[Daniel Axtens]

Hi Chris,

These two paragraphs are a little confusing and they seem slightly
repetitive. But I get the general idea. Two specific comments below:

> There are non-inline functions which get called in setup_sigcontext() to
> save register state to the thread struct. Move these functions into a
> separate prepare_setup_sigcontext() function so that
> setup_sigcontext() can be refactored later into an "unsafe" version
> which assumes an open uaccess window. Non-inline functions should be
> avoided when uaccess is open.

Why do we want to avoid non-inline functions? We came up with:

 - we want KUAP protection for as much of the kernel as possible: each
   extra bit of code run with the window open is another piece of attack
   surface.
   
 - non-inline functions default to traceable, which means we could end
   up ftracing while uaccess is enabled. That's a pretty big hole in the
   defences that KUAP provides.

I think we've also had problems with the window being opened or closed
unexpectedly by various bits of code? So the less code runs in uaccess
context the less likely that is to occur.
 
> The majority of setup_sigcontext() can be refactored to execute in an
> "unsafe" context (uaccess window is opened) except for some non-inline
> functions. Move these out into a separate prepare_setup_sigcontext()
> function which must be called first and before opening up a uaccess
> window. A follow-up commit converts setup_sigcontext() to be "unsafe".

This was a bit confusing until we realise that you're moving the _calls_
to the non-inline functions out, not the non-inline functions themselves.

> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index f9e4a1ac440f..b211a8ea4f6e 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -79,6 +79,24 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
>  }
>  #endif
>  
> +static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_region)

ctx_has_vsx_region should probably be a bool? Although setup_sigcontext
also has it as an int so I guess that's arguable, and maybe it's better
to stick with this for constency.

> +{
> +#ifdef CONFIG_ALTIVEC
> +	/* save altivec registers */
> +	if (tsk->thread.used_vr)
> +		flush_altivec_to_thread(tsk);
> +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> +		tsk->thread.vrsave = mfspr(SPRN_VRSAVE);
> +#endif /* CONFIG_ALTIVEC */
> +
> +	flush_fp_to_thread(tsk);
> +
> +#ifdef CONFIG_VSX
> +	if (tsk->thread.used_vsr && ctx_has_vsx_region)
> +		flush_vsx_to_thread(tsk);
> +#endif /* CONFIG_VSX */

Alternatively, given that this is the only use of ctx_has_vsx_region,
mpe suggested that perhaps we could drop it entirely and always
flush_vsx if used_vsr. The function is only ever called with either
`current` or wth ctx_has_vsx_region set to 1, so in either case I think
that's safe? I'm not sure if it would have performance implications.

Should we move this and the altivec ifdef to IS_ENABLED(CONFIG_VSX) etc?
I'm not sure if that runs into any problems with things like 'used_vsr'
only being defined if CONFIG_VSX is set, but I thought I'd ask.


> +}
> +
>  /*
>   * Set up the sigcontext for the signal frame.
>   */
> @@ -97,7 +115,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>  	 */
>  #ifdef CONFIG_ALTIVEC
>  	elf_vrreg_t __user *v_regs = sigcontext_vmx_regs(sc);
> -	unsigned long vrsave;
>  #endif
>  	struct pt_regs *regs = tsk->thread.regs;
>  	unsigned long msr = regs->msr;
> @@ -112,7 +129,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>  
>  	/* save altivec registers */
>  	if (tsk->thread.used_vr) {
> -		flush_altivec_to_thread(tsk);
>  		/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
>  		err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
>  				      33 * sizeof(vector128));
> @@ -124,17 +140,10 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>  	/* We always copy to/from vrsave, it's 0 if we don't have or don't
>  	 * use altivec.
>  	 */
> -	vrsave = 0;
> -	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
> -		vrsave = mfspr(SPRN_VRSAVE);
> -		tsk->thread.vrsave = vrsave;
> -	}
> -
> -	err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
> +	err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);

Previously, if !cpu_has_feature(ALTIVEC), v_regs[33] had vrsave stored,
which was set to 0 explicitly. Now we store thread.vrsave instead of the
local vrsave. That should be safe - it is initalised to 0 elsewhere.

So you don't have to do anything here, this is just letting you know
that we checked it and thought about it.

>  #else /* CONFIG_ALTIVEC */
>  	err |= __put_user(0, &sc->v_regs);
>  #endif /* CONFIG_ALTIVEC */
> -	flush_fp_to_thread(tsk);
>  	/* copy fpr regs and fpscr */
>  	err |= copy_fpr_to_user(&sc->fp_regs, tsk);
>  
> @@ -150,7 +159,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>  	 * VMX data.
>  	 */
>  	if (tsk->thread.used_vsr && ctx_has_vsx_region) {
> -		flush_vsx_to_thread(tsk);
>  		v_regs += ELF_NVRREG;
>  		err |= copy_vsx_to_user(v_regs, tsk);
>  		/* set MSR_VSX in the MSR value in the frame to
> @@ -655,6 +663,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>  		ctx_has_vsx_region = 1;
>  
>  	if (old_ctx != NULL) {
> +		prepare_setup_sigcontext(current, ctx_has_vsx_region);
>  		if (!access_ok(old_ctx, ctx_size)
>  		    || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
>  					ctx_has_vsx_region)

I had a think about whether there was a problem with bubbling
prepare_setup_sigcontext over the access_ok() test, but given that
prepare_setup_sigcontext(current ...) doesn't access any of old_ctx, I'm
satisfied that it's OK - no changes needed.


> @@ -842,6 +851,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  #endif
>  	{
>  		err |= __put_user(0, &frame->uc.uc_link);
> +		prepare_setup_sigcontext(tsk, 1);

Why do we call with ctx_has_vsx_region = 1 here?  It's not immediately
clear to me why this is correct, but mpe and Mikey seem pretty convinced
that it is.

>  		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
>  					NULL, (unsigned long)ksig->ka.sa.sa_handler,
>  					1);


Finally, it's a bit hard to figure out where to put this, but we spent
some time making sure that the various things you moved into the
prepare_setup_sigcontext() function were called under the same
circumstances as they were before, and there were no concerns there.

Kind regards,
Daniel

Re: [PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext()

[Christopher M. Riedl]

On Sun Feb 7, 2021 at 10:44 PM CST, Daniel Axtens wrote:
> Hi Chris,
>
> These two paragraphs are a little confusing and they seem slightly
> repetitive. But I get the general idea. Two specific comments below:

Umm... yeah only one of those was supposed to be sent. I will reword
this for the next spin and address the comment below about how it is
not entirely clear that the inline functions are being moved out.

>
> > There are non-inline functions which get called in setup_sigcontext() to
> > save register state to the thread struct. Move these functions into a
> > separate prepare_setup_sigcontext() function so that
> > setup_sigcontext() can be refactored later into an "unsafe" version
> > which assumes an open uaccess window. Non-inline functions should be
> > avoided when uaccess is open.
>
> Why do we want to avoid non-inline functions? We came up with:
>
> - we want KUAP protection for as much of the kernel as possible: each
> extra bit of code run with the window open is another piece of attack
> surface.
>    
> - non-inline functions default to traceable, which means we could end
> up ftracing while uaccess is enabled. That's a pretty big hole in the
> defences that KUAP provides.
>
> I think we've also had problems with the window being opened or closed
> unexpectedly by various bits of code? So the less code runs in uaccess
> context the less likely that is to occur.

That is my understanding as well.

>  
> > The majority of setup_sigcontext() can be refactored to execute in an
> > "unsafe" context (uaccess window is opened) except for some non-inline
> > functions. Move these out into a separate prepare_setup_sigcontext()
> > function which must be called first and before opening up a uaccess
> > window. A follow-up commit converts setup_sigcontext() to be "unsafe".
>
> This was a bit confusing until we realise that you're moving the _calls_
> to the non-inline functions out, not the non-inline functions
> themselves.
>
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >  arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++-----------
> >  1 file changed, 21 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index f9e4a1ac440f..b211a8ea4f6e 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -79,6 +79,24 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
> >  }
> >  #endif
> >  
> > +static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_region)
>
> ctx_has_vsx_region should probably be a bool? Although setup_sigcontext
> also has it as an int so I guess that's arguable, and maybe it's better
> to stick with this for constency.

I've been told not to introduce unrelated changes in my patches before
so chose to keep this as an int for consistency.

>
> > +{
> > +#ifdef CONFIG_ALTIVEC
> > +	/* save altivec registers */
> > +	if (tsk->thread.used_vr)
> > +		flush_altivec_to_thread(tsk);
> > +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> > +		tsk->thread.vrsave = mfspr(SPRN_VRSAVE);
> > +#endif /* CONFIG_ALTIVEC */
> > +
> > +	flush_fp_to_thread(tsk);
> > +
> > +#ifdef CONFIG_VSX
> > +	if (tsk->thread.used_vsr && ctx_has_vsx_region)
> > +		flush_vsx_to_thread(tsk);
> > +#endif /* CONFIG_VSX */
>
> Alternatively, given that this is the only use of ctx_has_vsx_region,
> mpe suggested that perhaps we could drop it entirely and always
> flush_vsx if used_vsr. The function is only ever called with either
> `current` or wth ctx_has_vsx_region set to 1, so in either case I think
> that's safe? I'm not sure if it would have performance implications.

I think that could work as long as we can guarantee that the context
passed to swapcontext will always be sufficiently sized if used_vsr,
which I think *has* to be the case?

>
> Should we move this and the altivec ifdef to IS_ENABLED(CONFIG_VSX) etc?
> I'm not sure if that runs into any problems with things like 'used_vsr'
> only being defined if CONFIG_VSX is set, but I thought I'd ask.

That's why I didn't use IS_ENABLED(CONFIG_...) here - all of these
field (used_vr, vrsave, used_vsr) declarations are guarded by #ifdefs :/

>
>
> > +}
> > +
> >  /*
> >   * Set up the sigcontext for the signal frame.
> >   */
> > @@ -97,7 +115,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >  	 */
> >  #ifdef CONFIG_ALTIVEC
> >  	elf_vrreg_t __user *v_regs = sigcontext_vmx_regs(sc);
> > -	unsigned long vrsave;
> >  #endif
> >  	struct pt_regs *regs = tsk->thread.regs;
> >  	unsigned long msr = regs->msr;
> > @@ -112,7 +129,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >  
> >  	/* save altivec registers */
> >  	if (tsk->thread.used_vr) {
> > -		flush_altivec_to_thread(tsk);
> >  		/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
> >  		err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
> >  				      33 * sizeof(vector128));
> > @@ -124,17 +140,10 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >  	/* We always copy to/from vrsave, it's 0 if we don't have or don't
> >  	 * use altivec.
> >  	 */
> > -	vrsave = 0;
> > -	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
> > -		vrsave = mfspr(SPRN_VRSAVE);
> > -		tsk->thread.vrsave = vrsave;
> > -	}
> > -
> > -	err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
> > +	err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
>
> Previously, if !cpu_has_feature(ALTIVEC), v_regs[33] had vrsave stored,
> which was set to 0 explicitly. Now we store thread.vrsave instead of the
> local vrsave. That should be safe - it is initalised to 0 elsewhere.
>
> So you don't have to do anything here, this is just letting you know
> that we checked it and thought about it.

Thanks! I thought about adding a comment/note here as I had to convince
myself that thread.vrsave is indeed initialized to 0 before making this
change as well. I will mention it in the word-smithed commit message for
posterity.

>
> >  #else /* CONFIG_ALTIVEC */
> >  	err |= __put_user(0, &sc->v_regs);
> >  #endif /* CONFIG_ALTIVEC */
> > -	flush_fp_to_thread(tsk);
> >  	/* copy fpr regs and fpscr */
> >  	err |= copy_fpr_to_user(&sc->fp_regs, tsk);
> >  
> > @@ -150,7 +159,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >  	 * VMX data.
> >  	 */
> >  	if (tsk->thread.used_vsr && ctx_has_vsx_region) {
> > -		flush_vsx_to_thread(tsk);
> >  		v_regs += ELF_NVRREG;
> >  		err |= copy_vsx_to_user(v_regs, tsk);
> >  		/* set MSR_VSX in the MSR value in the frame to
> > @@ -655,6 +663,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> >  		ctx_has_vsx_region = 1;
> >  
> >  	if (old_ctx != NULL) {
> > +		prepare_setup_sigcontext(current, ctx_has_vsx_region);
> >  		if (!access_ok(old_ctx, ctx_size)
> >  		    || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
> >  					ctx_has_vsx_region)
>
> I had a think about whether there was a problem with bubbling
> prepare_setup_sigcontext over the access_ok() test, but given that
> prepare_setup_sigcontext(current ...) doesn't access any of old_ctx, I'm
> satisfied that it's OK - no changes needed.

Not sure I understand what you mean by 'bubbling over'?

>
>
> > @@ -842,6 +851,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >  #endif
> >  	{
> >  		err |= __put_user(0, &frame->uc.uc_link);
> > +		prepare_setup_sigcontext(tsk, 1);
>
> Why do we call with ctx_has_vsx_region = 1 here? It's not immediately
> clear to me why this is correct, but mpe and Mikey seem pretty convinced
> that it is.

I think it's because we always have a "complete" sigcontext w/ the VSX
save area here, unlike in swapcontext where we have to check. Also, the
following unsafe_setup_sigcontext() is called with ctx_has_vsx_region=1
so assumes that the VSX data was copied by prepare_setup_sigcontext().

>
> >  		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
> >  					NULL, (unsigned long)ksig->ka.sa.sa_handler,
> >  					1);
>
>
> Finally, it's a bit hard to figure out where to put this, but we spent
> some time making sure that the various things you moved into the
> prepare_setup_sigcontext() function were called under the same
> circumstances as they were before, and there were no concerns there.

Thanks for reviewing and double checking my work :)

>
> Kind regards,
> Daniel

Re: [PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext()

[Daniel Axtens]

"Christopher M. Riedl" <cmr@codefail.de> writes:

> On Sun Feb 7, 2021 at 10:44 PM CST, Daniel Axtens wrote:
>> Hi Chris,
>>
>> These two paragraphs are a little confusing and they seem slightly
>> repetitive. But I get the general idea. Two specific comments below:
>
> Umm... yeah only one of those was supposed to be sent. I will reword
> this for the next spin and address the comment below about how it is
> not entirely clear that the inline functions are being moved out.
>
>>
>> > There are non-inline functions which get called in setup_sigcontext() to
>> > save register state to the thread struct. Move these functions into a
>> > separate prepare_setup_sigcontext() function so that
>> > setup_sigcontext() can be refactored later into an "unsafe" version
>> > which assumes an open uaccess window. Non-inline functions should be
>> > avoided when uaccess is open.
>>
>> Why do we want to avoid non-inline functions? We came up with:
>>
>> - we want KUAP protection for as much of the kernel as possible: each
>> extra bit of code run with the window open is another piece of attack
>> surface.
>>    
>> - non-inline functions default to traceable, which means we could end
>> up ftracing while uaccess is enabled. That's a pretty big hole in the
>> defences that KUAP provides.
>>
>> I think we've also had problems with the window being opened or closed
>> unexpectedly by various bits of code? So the less code runs in uaccess
>> context the less likely that is to occur.
>
> That is my understanding as well.
>
>>  
>> > The majority of setup_sigcontext() can be refactored to execute in an
>> > "unsafe" context (uaccess window is opened) except for some non-inline
>> > functions. Move these out into a separate prepare_setup_sigcontext()
>> > function which must be called first and before opening up a uaccess
>> > window. A follow-up commit converts setup_sigcontext() to be "unsafe".
>>
>> This was a bit confusing until we realise that you're moving the _calls_
>> to the non-inline functions out, not the non-inline functions
>> themselves.
>>
>> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
>> > ---
>> >  arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++-----------
>> >  1 file changed, 21 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>> > index f9e4a1ac440f..b211a8ea4f6e 100644
>> > --- a/arch/powerpc/kernel/signal_64.c
>> > +++ b/arch/powerpc/kernel/signal_64.c
>> > @@ -79,6 +79,24 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
>> >  }
>> >  #endif
>> >  
>> > +static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_region)
>>
>> ctx_has_vsx_region should probably be a bool? Although setup_sigcontext
>> also has it as an int so I guess that's arguable, and maybe it's better
>> to stick with this for constency.
>
> I've been told not to introduce unrelated changes in my patches before
> so chose to keep this as an int for consistency.

Seems reasonable.

>
>>
>> > +{
>> > +#ifdef CONFIG_ALTIVEC
>> > +	/* save altivec registers */
>> > +	if (tsk->thread.used_vr)
>> > +		flush_altivec_to_thread(tsk);
>> > +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
>> > +		tsk->thread.vrsave = mfspr(SPRN_VRSAVE);
>> > +#endif /* CONFIG_ALTIVEC */
>> > +
>> > +	flush_fp_to_thread(tsk);
>> > +
>> > +#ifdef CONFIG_VSX
>> > +	if (tsk->thread.used_vsr && ctx_has_vsx_region)
>> > +		flush_vsx_to_thread(tsk);
>> > +#endif /* CONFIG_VSX */
>>
>> Alternatively, given that this is the only use of ctx_has_vsx_region,
>> mpe suggested that perhaps we could drop it entirely and always
>> flush_vsx if used_vsr. The function is only ever called with either
>> `current` or wth ctx_has_vsx_region set to 1, so in either case I think
>> that's safe? I'm not sure if it would have performance implications.
>
> I think that could work as long as we can guarantee that the context
> passed to swapcontext will always be sufficiently sized if used_vsr,
> which I think *has* to be the case?

I think you're always guaranteed that you'll have a big enough one
in your kernel thread, which is what you end up writing to, iiuc?

>>
>> Should we move this and the altivec ifdef to IS_ENABLED(CONFIG_VSX) etc?
>> I'm not sure if that runs into any problems with things like 'used_vsr'
>> only being defined if CONFIG_VSX is set, but I thought I'd ask.
>
> That's why I didn't use IS_ENABLED(CONFIG_...) here - all of these
> field (used_vr, vrsave, used_vsr) declarations are guarded by #ifdefs :/

Dang. Oh well.
>
>>
>>
>> > +}
>> > +
>> >  /*
>> >   * Set up the sigcontext for the signal frame.
>> >   */
>> > @@ -97,7 +115,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> >  	 */
>> >  #ifdef CONFIG_ALTIVEC
>> >  	elf_vrreg_t __user *v_regs = sigcontext_vmx_regs(sc);
>> > -	unsigned long vrsave;
>> >  #endif
>> >  	struct pt_regs *regs = tsk->thread.regs;
>> >  	unsigned long msr = regs->msr;
>> > @@ -112,7 +129,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> >  
>> >  	/* save altivec registers */
>> >  	if (tsk->thread.used_vr) {
>> > -		flush_altivec_to_thread(tsk);
>> >  		/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
>> >  		err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
>> >  				      33 * sizeof(vector128));
>> > @@ -124,17 +140,10 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> >  	/* We always copy to/from vrsave, it's 0 if we don't have or don't
>> >  	 * use altivec.
>> >  	 */
>> > -	vrsave = 0;
>> > -	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
>> > -		vrsave = mfspr(SPRN_VRSAVE);
>> > -		tsk->thread.vrsave = vrsave;
>> > -	}
>> > -
>> > -	err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
>> > +	err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
>>
>> Previously, if !cpu_has_feature(ALTIVEC), v_regs[33] had vrsave stored,
>> which was set to 0 explicitly. Now we store thread.vrsave instead of the
>> local vrsave. That should be safe - it is initalised to 0 elsewhere.
>>
>> So you don't have to do anything here, this is just letting you know
>> that we checked it and thought about it.
>
> Thanks! I thought about adding a comment/note here as I had to convince
> myself that thread.vrsave is indeed initialized to 0 before making this
> change as well. I will mention it in the word-smithed commit message for
> posterity.
>
>>
>> >  #else /* CONFIG_ALTIVEC */
>> >  	err |= __put_user(0, &sc->v_regs);
>> >  #endif /* CONFIG_ALTIVEC */
>> > -	flush_fp_to_thread(tsk);
>> >  	/* copy fpr regs and fpscr */
>> >  	err |= copy_fpr_to_user(&sc->fp_regs, tsk);
>> >  
>> > @@ -150,7 +159,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>> >  	 * VMX data.
>> >  	 */
>> >  	if (tsk->thread.used_vsr && ctx_has_vsx_region) {
>> > -		flush_vsx_to_thread(tsk);
>> >  		v_regs += ELF_NVRREG;
>> >  		err |= copy_vsx_to_user(v_regs, tsk);
>> >  		/* set MSR_VSX in the MSR value in the frame to
>> > @@ -655,6 +663,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>> >  		ctx_has_vsx_region = 1;
>> >  
>> >  	if (old_ctx != NULL) {
>> > +		prepare_setup_sigcontext(current, ctx_has_vsx_region);
>> >  		if (!access_ok(old_ctx, ctx_size)
>> >  		    || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
>> >  					ctx_has_vsx_region)
>>
>> I had a think about whether there was a problem with bubbling
>> prepare_setup_sigcontext over the access_ok() test, but given that
>> prepare_setup_sigcontext(current ...) doesn't access any of old_ctx, I'm
>> satisfied that it's OK - no changes needed.
>
> Not sure I understand what you mean by 'bubbling over'?


Yeah sorry, overly flowery language there. I mean that the accesses that
prepare_setup_sigcontext does have moved up - like a bubble in fluid -
from after access_ok to before access_ok.

Kind regards,
Daniel
>>
>>
>> > @@ -842,6 +851,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>> >  #endif
>> >  	{
>> >  		err |= __put_user(0, &frame->uc.uc_link);
>> > +		prepare_setup_sigcontext(tsk, 1);
>>
>> Why do we call with ctx_has_vsx_region = 1 here? It's not immediately
>> clear to me why this is correct, but mpe and Mikey seem pretty convinced
>> that it is.
>
> I think it's because we always have a "complete" sigcontext w/ the VSX
> save area here, unlike in swapcontext where we have to check. Also, the
> following unsafe_setup_sigcontext() is called with ctx_has_vsx_region=1
> so assumes that the VSX data was copied by prepare_setup_sigcontext().
>
>>
>> >  		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
>> >  					NULL, (unsigned long)ksig->ka.sa.sa_handler,
>> >  					1);
>>
>>
>> Finally, it's a bit hard to figure out where to put this, but we spent
>> some time making sure that the various things you moved into the
>> prepare_setup_sigcontext() function were called under the same
>> circumstances as they were before, and there were no concerns there.
>
> Thanks for reviewing and double checking my work :)
>
>>
>> Kind regards,
>> Daniel

Re: [PATCH v5 03/10] powerpc/signal64: Move non-inline functions out of setup_sigcontext()

[Christopher M. Riedl]

On Wed Feb 10, 2021 at 3:06 PM CST, Daniel Axtens wrote:
> "Christopher M. Riedl" <cmr@codefail.de> writes:
>
> > On Sun Feb 7, 2021 at 10:44 PM CST, Daniel Axtens wrote:
> >> Hi Chris,
> >>
> >> These two paragraphs are a little confusing and they seem slightly
> >> repetitive. But I get the general idea. Two specific comments below:
> >
> > Umm... yeah only one of those was supposed to be sent. I will reword
> > this for the next spin and address the comment below about how it is
> > not entirely clear that the inline functions are being moved out.
> >
> >>
> >> > There are non-inline functions which get called in setup_sigcontext() to
> >> > save register state to the thread struct. Move these functions into a
> >> > separate prepare_setup_sigcontext() function so that
> >> > setup_sigcontext() can be refactored later into an "unsafe" version
> >> > which assumes an open uaccess window. Non-inline functions should be
> >> > avoided when uaccess is open.
> >>
> >> Why do we want to avoid non-inline functions? We came up with:
> >>
> >> - we want KUAP protection for as much of the kernel as possible: each
> >> extra bit of code run with the window open is another piece of attack
> >> surface.
> >>    
> >> - non-inline functions default to traceable, which means we could end
> >> up ftracing while uaccess is enabled. That's a pretty big hole in the
> >> defences that KUAP provides.
> >>
> >> I think we've also had problems with the window being opened or closed
> >> unexpectedly by various bits of code? So the less code runs in uaccess
> >> context the less likely that is to occur.
> >
> > That is my understanding as well.
> >
> >>  
> >> > The majority of setup_sigcontext() can be refactored to execute in an
> >> > "unsafe" context (uaccess window is opened) except for some non-inline
> >> > functions. Move these out into a separate prepare_setup_sigcontext()
> >> > function which must be called first and before opening up a uaccess
> >> > window. A follow-up commit converts setup_sigcontext() to be "unsafe".
> >>
> >> This was a bit confusing until we realise that you're moving the _calls_
> >> to the non-inline functions out, not the non-inline functions
> >> themselves.
> >>
> >> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> >> > ---
> >> >  arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++-----------
> >> >  1 file changed, 21 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> >> > index f9e4a1ac440f..b211a8ea4f6e 100644
> >> > --- a/arch/powerpc/kernel/signal_64.c
> >> > +++ b/arch/powerpc/kernel/signal_64.c
> >> > @@ -79,6 +79,24 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
> >> >  }
> >> >  #endif
> >> >  
> >> > +static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_region)
> >>
> >> ctx_has_vsx_region should probably be a bool? Although setup_sigcontext
> >> also has it as an int so I guess that's arguable, and maybe it's better
> >> to stick with this for constency.
> >
> > I've been told not to introduce unrelated changes in my patches before
> > so chose to keep this as an int for consistency.
>
> Seems reasonable.
>
> >
> >>
> >> > +{
> >> > +#ifdef CONFIG_ALTIVEC
> >> > +	/* save altivec registers */
> >> > +	if (tsk->thread.used_vr)
> >> > +		flush_altivec_to_thread(tsk);
> >> > +	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> >> > +		tsk->thread.vrsave = mfspr(SPRN_VRSAVE);
> >> > +#endif /* CONFIG_ALTIVEC */
> >> > +
> >> > +	flush_fp_to_thread(tsk);
> >> > +
> >> > +#ifdef CONFIG_VSX
> >> > +	if (tsk->thread.used_vsr && ctx_has_vsx_region)
> >> > +		flush_vsx_to_thread(tsk);
> >> > +#endif /* CONFIG_VSX */
> >>
> >> Alternatively, given that this is the only use of ctx_has_vsx_region,
> >> mpe suggested that perhaps we could drop it entirely and always
> >> flush_vsx if used_vsr. The function is only ever called with either
> >> `current` or wth ctx_has_vsx_region set to 1, so in either case I think
> >> that's safe? I'm not sure if it would have performance implications.
> >
> > I think that could work as long as we can guarantee that the context
> > passed to swapcontext will always be sufficiently sized if used_vsr,
> > which I think *has* to be the case?
>
> I think you're always guaranteed that you'll have a big enough one
> in your kernel thread, which is what you end up writing to, iiuc?

Ah yup you are correct. I confused myself with the comment in
swapcontext about the ctx_size. We call prepare_setup_sigcontext() with
current which will always have space. The ctx_size only matters on the
next call to setup_sigcontext() which ends up potentially copying the
VSX region to userspace (v_regs).

TL;DR - yes, I'll remove the ctx_has_vsx_region argument to
prepare_setup_sigcontext() with the next version. Thanks!

>
> >>
> >> Should we move this and the altivec ifdef to IS_ENABLED(CONFIG_VSX) etc?
> >> I'm not sure if that runs into any problems with things like 'used_vsr'
> >> only being defined if CONFIG_VSX is set, but I thought I'd ask.
> >
> > That's why I didn't use IS_ENABLED(CONFIG_...) here - all of these
> > field (used_vr, vrsave, used_vsr) declarations are guarded by #ifdefs :/
>
> Dang. Oh well.
> >
> >>
> >>
> >> > +}
> >> > +
> >> >  /*
> >> >   * Set up the sigcontext for the signal frame.
> >> >   */
> >> > @@ -97,7 +115,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >> >  	 */
> >> >  #ifdef CONFIG_ALTIVEC
> >> >  	elf_vrreg_t __user *v_regs = sigcontext_vmx_regs(sc);
> >> > -	unsigned long vrsave;
> >> >  #endif
> >> >  	struct pt_regs *regs = tsk->thread.regs;
> >> >  	unsigned long msr = regs->msr;
> >> > @@ -112,7 +129,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >> >  
> >> >  	/* save altivec registers */
> >> >  	if (tsk->thread.used_vr) {
> >> > -		flush_altivec_to_thread(tsk);
> >> >  		/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
> >> >  		err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
> >> >  				      33 * sizeof(vector128));
> >> > @@ -124,17 +140,10 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >> >  	/* We always copy to/from vrsave, it's 0 if we don't have or don't
> >> >  	 * use altivec.
> >> >  	 */
> >> > -	vrsave = 0;
> >> > -	if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
> >> > -		vrsave = mfspr(SPRN_VRSAVE);
> >> > -		tsk->thread.vrsave = vrsave;
> >> > -	}
> >> > -
> >> > -	err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
> >> > +	err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> >>
> >> Previously, if !cpu_has_feature(ALTIVEC), v_regs[33] had vrsave stored,
> >> which was set to 0 explicitly. Now we store thread.vrsave instead of the
> >> local vrsave. That should be safe - it is initalised to 0 elsewhere.
> >>
> >> So you don't have to do anything here, this is just letting you know
> >> that we checked it and thought about it.
> >
> > Thanks! I thought about adding a comment/note here as I had to convince
> > myself that thread.vrsave is indeed initialized to 0 before making this
> > change as well. I will mention it in the word-smithed commit message for
> > posterity.
> >
> >>
> >> >  #else /* CONFIG_ALTIVEC */
> >> >  	err |= __put_user(0, &sc->v_regs);
> >> >  #endif /* CONFIG_ALTIVEC */
> >> > -	flush_fp_to_thread(tsk);
> >> >  	/* copy fpr regs and fpscr */
> >> >  	err |= copy_fpr_to_user(&sc->fp_regs, tsk);
> >> >  
> >> > @@ -150,7 +159,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >> >  	 * VMX data.
> >> >  	 */
> >> >  	if (tsk->thread.used_vsr && ctx_has_vsx_region) {
> >> > -		flush_vsx_to_thread(tsk);
> >> >  		v_regs += ELF_NVRREG;
> >> >  		err |= copy_vsx_to_user(v_regs, tsk);
> >> >  		/* set MSR_VSX in the MSR value in the frame to
> >> > @@ -655,6 +663,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> >> >  		ctx_has_vsx_region = 1;
> >> >  
> >> >  	if (old_ctx != NULL) {
> >> > +		prepare_setup_sigcontext(current, ctx_has_vsx_region);
> >> >  		if (!access_ok(old_ctx, ctx_size)
> >> >  		    || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
> >> >  					ctx_has_vsx_region)
> >>
> >> I had a think about whether there was a problem with bubbling
> >> prepare_setup_sigcontext over the access_ok() test, but given that
> >> prepare_setup_sigcontext(current ...) doesn't access any of old_ctx, I'm
> >> satisfied that it's OK - no changes needed.
> >
> > Not sure I understand what you mean by 'bubbling over'?
>
>
> Yeah sorry, overly flowery language there. I mean that the accesses that
> prepare_setup_sigcontext does have moved up - like a bubble in fluid -
> from after access_ok to before access_ok.
>
> Kind regards,
> Daniel
> >>
> >>
> >> > @@ -842,6 +851,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >> >  #endif
> >> >  	{
> >> >  		err |= __put_user(0, &frame->uc.uc_link);
> >> > +		prepare_setup_sigcontext(tsk, 1);
> >>
> >> Why do we call with ctx_has_vsx_region = 1 here? It's not immediately
> >> clear to me why this is correct, but mpe and Mikey seem pretty convinced
> >> that it is.
> >
> > I think it's because we always have a "complete" sigcontext w/ the VSX
> > save area here, unlike in swapcontext where we have to check. Also, the
> > following unsafe_setup_sigcontext() is called with ctx_has_vsx_region=1
> > so assumes that the VSX data was copied by prepare_setup_sigcontext().
> >
> >>
> >> >  		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
> >> >  					NULL, (unsigned long)ksig->ka.sa.sa_handler,
> >> >  					1);
> >>
> >>
> >> Finally, it's a bit hard to figure out where to put this, but we spent
> >> some time making sure that the various things you moved into the
> >> prepare_setup_sigcontext() function were called under the same
> >> circumstances as they were before, and there were no concerns there.
> >
> > Thanks for reviewing and double checking my work :)
> >
> >>
> >> Kind regards,
> >> Daniel

[PATCH v5 04/10] powerpc: Reference param in MSR_TM_ACTIVE() macro

[Christopher M. Riedl]

Unlike the other MSR_TM_* macros, MSR_TM_ACTIVE does not reference or
use its parameter unless CONFIG_PPC_TRANSACTIONAL_MEM is defined. This
causes an 'unused variable' compile warning unless the variable is also
guarded with CONFIG_PPC_TRANSACTIONAL_MEM.

Reference but do nothing with the argument in the macro to avoid a
potential compile warning.

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/include/asm/reg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index e40a921d78f9..c5a3e856191c 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -124,7 +124,7 @@
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */
 #else
-#define MSR_TM_ACTIVE(x) 0
+#define MSR_TM_ACTIVE(x) ((void)(x), 0)
 #endif
 
 #if defined(CONFIG_PPC_BOOK3S_64)
-- 
2.26.1

Re: [PATCH v5 04/10] powerpc: Reference param in MSR_TM_ACTIVE() macro

[Daniel Axtens]

Hi Chris,

(totally bikeshedding, but I'd use the full word parameter in the
subject if you have enough spare characters.)

> Unlike the other MSR_TM_* macros, MSR_TM_ACTIVE does not reference or
> use its parameter unless CONFIG_PPC_TRANSACTIONAL_MEM is defined. This
> causes an 'unused variable' compile warning unless the variable is also
> guarded with CONFIG_PPC_TRANSACTIONAL_MEM.
>
> Reference but do nothing with the argument in the macro to avoid a
> potential compile warning.

Andrew asked why we weren't seeing these warnings already.

I was able to reproduce them with CONFIG_PPC_TRANSACTIONAL_MEM off, but
only if I compiled with W=1:

arch/powerpc/kernel/process.c: In function ‘enable_kernel_fp’:
arch/powerpc/kernel/process.c:210:16: error: variable ‘cpumsr’ set but not used [-Werror=unused-but-set-variable]
  210 |  unsigned long cpumsr;
      |                ^~~~~~

Having said that, this change seems like a good idea, squashing warnings
at W=1 is still valuable.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/include/asm/reg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index e40a921d78f9..c5a3e856191c 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -124,7 +124,7 @@
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */
>  #else
> -#define MSR_TM_ACTIVE(x) 0
> +#define MSR_TM_ACTIVE(x) ((void)(x), 0)
>  #endif
>  
>  #if defined(CONFIG_PPC_BOOK3S_64)
> -- 
> 2.26.1

[PATCH v5 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block

[Christopher M. Riedl]

Rework the messy ifdef breaking up the if-else for TM similar to
commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else").

Unlike that commit for ppc32, the ifdef can't be removed entirely since
uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM.

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index b211a8ea4f6e..8e1d804ce552 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	struct pt_regs *regs = current_pt_regs();
 	struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1];
 	sigset_t set;
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	unsigned long msr;
-#endif
 
 	/* Always make any pending restarted system calls return -EINTR */
 	current->restart_block.fn = do_no_restart_syscall;
@@ -765,7 +763,10 @@ SYSCALL_DEFINE0(rt_sigreturn)
 
 	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
 		goto badframe;
+#endif
+
 	if (MSR_TM_ACTIVE(msr)) {
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 		/* We recheckpoint on return. */
 		struct ucontext __user *uc_transact;
 
@@ -778,9 +779,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
 		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
 					   &uc_transact->uc_mcontext))
 			goto badframe;
-	} else
 #endif
-	{
+	} else {
 		/*
 		 * Fall through, for non-TM restore
 		 *
@@ -818,10 +818,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	unsigned long newsp = 0;
 	long err = 0;
 	struct pt_regs *regs = tsk->thread.regs;
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	/* Save the thread's msr before get_tm_stackpointer() changes it */
 	unsigned long msr = regs->msr;
-#endif
 
 	frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
 	if (!access_ok(frame, sizeof(*frame)))
@@ -836,8 +834,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	/* Create the ucontext.  */
 	err |= __put_user(0, &frame->uc.uc_flags);
 	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+
 	if (MSR_TM_ACTIVE(msr)) {
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 		/* The ucontext_t passed to userland points to the second
 		 * ucontext_t (for transactional state) with its uc_link ptr.
 		 */
@@ -847,9 +846,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 					    tsk, ksig->sig, NULL,
 					    (unsigned long)ksig->ka.sa.sa_handler,
 					    msr);
-	} else
 #endif
-	{
+	} else {
 		err |= __put_user(0, &frame->uc.uc_link);
 		prepare_setup_sigcontext(tsk, 1);
 		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
-- 
2.26.1

Re: [PATCH v5 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block

[Daniel Axtens]

Hi Chris,

> Rework the messy ifdef breaking up the if-else for TM similar to
> commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else").

I'm not sure what 'the messy ifdef' and 'the if-else for TM' is (yet):
perhaps you could start the commit message with a tiny bit of
background.

> Unlike that commit for ppc32, the ifdef can't be removed entirely since
> uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM.
>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/kernel/signal_64.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index b211a8ea4f6e..8e1d804ce552 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  	struct pt_regs *regs = current_pt_regs();
>  	struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1];
>  	sigset_t set;
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	unsigned long msr;
> -#endif
>  
>  	/* Always make any pending restarted system calls return -EINTR */
>  	current->restart_block.fn = do_no_restart_syscall;
> @@ -765,7 +763,10 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  
>  	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
>  		goto badframe;
> +#endif
> +
>  	if (MSR_TM_ACTIVE(msr)) {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  		/* We recheckpoint on return. */
>  		struct ucontext __user *uc_transact;
>  
> @@ -778,9 +779,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
>  					   &uc_transact->uc_mcontext))
>  			goto badframe;
> -	} else
>  #endif
> -	{
> +	} else {
>  		/*
>  		 * Fall through, for non-TM restore
>  		 *

I think you can get rid of all the ifdefs in _this function_ by
providing a couple of stubs:

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index a66f435dabbf..19059a4b798f 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1120,6 +1120,7 @@ void restore_tm_state(struct pt_regs *regs)
 #else
 #define tm_recheckpoint_new_task(new)
 #define __switch_to_tm(prev, new)
+void tm_reclaim_current(uint8_t cause) {}
 #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
 
 static inline void save_sprs(struct thread_struct *t)
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 8e1d804ce552..ed58ca019ad9 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -594,6 +594,13 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 
 	return err;
 }
+#else
+static long restore_tm_sigcontexts(struct task_struct *tsk,
+				   struct sigcontext __user *sc,
+				   struct sigcontext __user *tm_sc)
+{
+	return -EINVAL;
+}
 #endif
 
 /*
@@ -722,7 +729,6 @@ SYSCALL_DEFINE0(rt_sigreturn)
 		goto badframe;
 	set_current_blocked(&set);
 
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	/*
 	 * If there is a transactional state then throw it away.
 	 * The purpose of a sigreturn is to destroy all traces of the
@@ -763,10 +769,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
 
 	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
 		goto badframe;
-#endif
 
 	if (MSR_TM_ACTIVE(msr)) {
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 		/* We recheckpoint on return. */
 		struct ucontext __user *uc_transact;
 
@@ -779,7 +783,6 @@ SYSCALL_DEFINE0(rt_sigreturn)
 		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
 					   &uc_transact->uc_mcontext))
 			goto badframe;
-#endif
 	} else {
 		/*
 		 * Fall through, for non-TM restore

My only concern here was whether it was valid to access
 	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
if CONFIG_PPC_TRANSACTIONAL_MEM was not defined, but I didn't think of
any obvious reason why it wouldn't be...

> @@ -818,10 +818,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	unsigned long newsp = 0;
>  	long err = 0;
>  	struct pt_regs *regs = tsk->thread.regs;
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  	/* Save the thread's msr before get_tm_stackpointer() changes it */
>  	unsigned long msr = regs->msr;
> -#endif

I wondered if that comment still makes sense in the absence of the
ifdef. It is preserved in commit f1cf4f93de2f ("powerpc/signal32: Remove
ifdefery in middle of if/else"), and I can't figure out how you would
improve it, so I guess it's probably good as it is.

>  	frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
>  	if (!access_ok(frame, sizeof(*frame)))
> @@ -836,8 +834,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	/* Create the ucontext.  */
>  	err |= __put_user(0, &frame->uc.uc_flags);
>  	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +
>  	if (MSR_TM_ACTIVE(msr)) {
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>  		/* The ucontext_t passed to userland points to the second
>  		 * ucontext_t (for transactional state) with its uc_link ptr.
>  		 */
> @@ -847,9 +846,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  					    tsk, ksig->sig, NULL,
>  					    (unsigned long)ksig->ka.sa.sa_handler,
>  					    msr);
> -	} else
>  #endif
> -	{
> +	} else {
>  		err |= __put_user(0, &frame->uc.uc_link);
>  		prepare_setup_sigcontext(tsk, 1);
>  		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,

That seems reasonable to me.

Kind regards,
Daniel

Re: [PATCH v5 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block

[Christopher M. Riedl]

On Thu Feb 11, 2021 at 11:21 PM CST, Daniel Axtens wrote:
> Hi Chris,
>
> > Rework the messy ifdef breaking up the if-else for TM similar to
> > commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else").
>
> I'm not sure what 'the messy ifdef' and 'the if-else for TM' is (yet):
> perhaps you could start the commit message with a tiny bit of
> background.

Yup good point - I will reword this for the next spin.

>
> > Unlike that commit for ppc32, the ifdef can't be removed entirely since
> > uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >  arch/powerpc/kernel/signal_64.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index b211a8ea4f6e..8e1d804ce552 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -710,9 +710,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >  	struct pt_regs *regs = current_pt_regs();
> >  	struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1];
> >  	sigset_t set;
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >  	unsigned long msr;
> > -#endif
> >  
> >  	/* Always make any pending restarted system calls return -EINTR */
> >  	current->restart_block.fn = do_no_restart_syscall;
> > @@ -765,7 +763,10 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >  
> >  	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> >  		goto badframe;
> > +#endif
> > +
> >  	if (MSR_TM_ACTIVE(msr)) {
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >  		/* We recheckpoint on return. */
> >  		struct ucontext __user *uc_transact;
> >  
> > @@ -778,9 +779,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >  		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
> >  					   &uc_transact->uc_mcontext))
> >  			goto badframe;
> > -	} else
> >  #endif
> > -	{
> > +	} else {
> >  		/*
> >  		 * Fall through, for non-TM restore
> >  		 *
>
> I think you can get rid of all the ifdefs in _this function_ by
> providing a couple of stubs:
>
> diff --git a/arch/powerpc/kernel/process.c
> b/arch/powerpc/kernel/process.c
> index a66f435dabbf..19059a4b798f 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1120,6 +1120,7 @@ void restore_tm_state(struct pt_regs *regs)
> #else
> #define tm_recheckpoint_new_task(new)
> #define __switch_to_tm(prev, new)
> +void tm_reclaim_current(uint8_t cause) {}
> #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
>  
> static inline void save_sprs(struct thread_struct *t)
> diff --git a/arch/powerpc/kernel/signal_64.c
> b/arch/powerpc/kernel/signal_64.c
> index 8e1d804ce552..ed58ca019ad9 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -594,6 +594,13 @@ static long restore_tm_sigcontexts(struct
> task_struct *tsk,
>  
> return err;
> }
> +#else
> +static long restore_tm_sigcontexts(struct task_struct *tsk,
> + struct sigcontext __user *sc,
> + struct sigcontext __user *tm_sc)
> +{
> + return -EINVAL;
> +}
> #endif
>  
> /*
> @@ -722,7 +729,6 @@ SYSCALL_DEFINE0(rt_sigreturn)
> goto badframe;
> set_current_blocked(&set);
>  
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> /*
> * If there is a transactional state then throw it away.
> * The purpose of a sigreturn is to destroy all traces of the
> @@ -763,10 +769,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  
> if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> goto badframe;
> -#endif
>  
> if (MSR_TM_ACTIVE(msr)) {
> -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> /* We recheckpoint on return. */
> struct ucontext __user *uc_transact;
>  
> @@ -779,7 +783,6 @@ SYSCALL_DEFINE0(rt_sigreturn)
> if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
> &uc_transact->uc_mcontext))
> goto badframe;
> -#endif
> } else {
> /*
> * Fall through, for non-TM restore
>
> My only concern here was whether it was valid to access
> if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> if CONFIG_PPC_TRANSACTIONAL_MEM was not defined, but I didn't think of
> any obvious reason why it wouldn't be...

Hmm, we don't really need it for the non-TM case and it is another extra
uaccess. I will take your suggestion to remove the need for the other
ifdefs but might keep this one. Keeping it also makes it very clear this
call is only here for TM and possible to remove in a potentially TM-less
future :)

>
> > @@ -818,10 +818,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >  	unsigned long newsp = 0;
> >  	long err = 0;
> >  	struct pt_regs *regs = tsk->thread.regs;
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >  	/* Save the thread's msr before get_tm_stackpointer() changes it */
> >  	unsigned long msr = regs->msr;
> > -#endif
>
> I wondered if that comment still makes sense in the absence of the
> ifdef. It is preserved in commit f1cf4f93de2f ("powerpc/signal32: Remove
> ifdefery in middle of if/else"), and I can't figure out how you would
> improve it, so I guess it's probably good as it is.
>
> >  	frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
> >  	if (!access_ok(frame, sizeof(*frame)))
> > @@ -836,8 +834,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >  	/* Create the ucontext.  */
> >  	err |= __put_user(0, &frame->uc.uc_flags);
> >  	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
> > -#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > +
> >  	if (MSR_TM_ACTIVE(msr)) {
> > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >  		/* The ucontext_t passed to userland points to the second
> >  		 * ucontext_t (for transactional state) with its uc_link ptr.
> >  		 */
> > @@ -847,9 +846,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >  					    tsk, ksig->sig, NULL,
> >  					    (unsigned long)ksig->ka.sa.sa_handler,
> >  					    msr);
> > -	} else
> >  #endif
> > -	{
> > +	} else {
> >  		err |= __put_user(0, &frame->uc.uc_link);
> >  		prepare_setup_sigcontext(tsk, 1);
> >  		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
>
> That seems reasonable to me.

Thanks for the feedback!

>
> Kind regards,
> Daniel

Re: [PATCH v5 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block

[Michael Ellerman]

"Christopher M. Riedl" <cmr@codefail.de> writes:
> On Thu Feb 11, 2021 at 11:21 PM CST, Daniel Axtens wrote:
...
>>
>> My only concern here was whether it was valid to access
>> if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
>> if CONFIG_PPC_TRANSACTIONAL_MEM was not defined, but I didn't think of
>> any obvious reason why it wouldn't be...
>
> Hmm, we don't really need it for the non-TM case and it is another extra
> uaccess. I will take your suggestion to remove the need for the other
> ifdefs but might keep this one. Keeping it also makes it very clear this
> call is only here for TM and possible to remove in a potentially TM-less
> future :)

Yep I agree.

cheers

[PATCH v5 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext()

[Christopher M. Riedl]

Previously setup_sigcontext() performed a costly KUAP switch on every
uaccess operation. These repeated uaccess switches cause a significant
drop in signal handling performance.

Rewrite setup_sigcontext() to assume that a userspace write access window
is open. Replace all uaccess functions with their 'unsafe' versions
which avoid the repeated uaccess switches.

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 70 ++++++++++++++++++++-------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 8e1d804ce552..4248e4489ff1 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -101,9 +101,13 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
  * Set up the sigcontext for the signal frame.
  */
 
-static long setup_sigcontext(struct sigcontext __user *sc,
-		struct task_struct *tsk, int signr, sigset_t *set,
-		unsigned long handler, int ctx_has_vsx_region)
+#define unsafe_setup_sigcontext(sc, tsk, signr, set, handler,		\
+				ctx_has_vsx_region, e)			\
+	unsafe_op_wrap(__unsafe_setup_sigcontext(sc, tsk, signr, set,	\
+			handler, ctx_has_vsx_region), e)
+static long notrace __unsafe_setup_sigcontext(struct sigcontext __user *sc,
+					struct task_struct *tsk, int signr, sigset_t *set,
+					unsigned long handler, int ctx_has_vsx_region)
 {
 	/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
 	 * process never used altivec yet (MSR_VEC is zero in pt_regs of
@@ -118,20 +122,19 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 #endif
 	struct pt_regs *regs = tsk->thread.regs;
 	unsigned long msr = regs->msr;
-	long err = 0;
 	/* Force usr to alway see softe as 1 (interrupts enabled) */
 	unsigned long softe = 0x1;
 
 	BUG_ON(tsk != current);
 
 #ifdef CONFIG_ALTIVEC
-	err |= __put_user(v_regs, &sc->v_regs);
+	unsafe_put_user(v_regs, &sc->v_regs, efault_out);
 
 	/* save altivec registers */
 	if (tsk->thread.used_vr) {
 		/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
-		err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
-				      33 * sizeof(vector128));
+		unsafe_copy_to_user(v_regs, &tsk->thread.vr_state,
+				    33 * sizeof(vector128), efault_out);
 		/* set MSR_VEC in the MSR value in the frame to indicate that sc->v_reg)
 		 * contains valid data.
 		 */
@@ -140,12 +143,12 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 	/* We always copy to/from vrsave, it's 0 if we don't have or don't
 	 * use altivec.
 	 */
-	err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
+	unsafe_put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33], efault_out);
 #else /* CONFIG_ALTIVEC */
-	err |= __put_user(0, &sc->v_regs);
+	unsafe_put_user(0, &sc->v_regs, efault_out);
 #endif /* CONFIG_ALTIVEC */
 	/* copy fpr regs and fpscr */
-	err |= copy_fpr_to_user(&sc->fp_regs, tsk);
+	unsafe_copy_fpr_to_user(&sc->fp_regs, tsk, efault_out);
 
 	/*
 	 * Clear the MSR VSX bit to indicate there is no valid state attached
@@ -160,24 +163,27 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 	 */
 	if (tsk->thread.used_vsr && ctx_has_vsx_region) {
 		v_regs += ELF_NVRREG;
-		err |= copy_vsx_to_user(v_regs, tsk);
+		unsafe_copy_vsx_to_user(v_regs, tsk, efault_out);
 		/* set MSR_VSX in the MSR value in the frame to
 		 * indicate that sc->vs_reg) contains valid data.
 		 */
 		msr |= MSR_VSX;
 	}
 #endif /* CONFIG_VSX */
-	err |= __put_user(&sc->gp_regs, &sc->regs);
+	unsafe_put_user(&sc->gp_regs, &sc->regs, efault_out);
 	WARN_ON(!FULL_REGS(regs));
-	err |= __copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE);
-	err |= __put_user(msr, &sc->gp_regs[PT_MSR]);
-	err |= __put_user(softe, &sc->gp_regs[PT_SOFTE]);
-	err |= __put_user(signr, &sc->signal);
-	err |= __put_user(handler, &sc->handler);
+	unsafe_copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE, efault_out);
+	unsafe_put_user(msr, &sc->gp_regs[PT_MSR], efault_out);
+	unsafe_put_user(softe, &sc->gp_regs[PT_SOFTE], efault_out);
+	unsafe_put_user(signr, &sc->signal, efault_out);
+	unsafe_put_user(handler, &sc->handler, efault_out);
 	if (set != NULL)
-		err |=  __put_user(set->sig[0], &sc->oldmask);
+		unsafe_put_user(set->sig[0], &sc->oldmask, efault_out);
 
-	return err;
+	return 0;
+
+efault_out:
+	return -EFAULT;
 }
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
@@ -664,12 +670,15 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
 
 	if (old_ctx != NULL) {
 		prepare_setup_sigcontext(current, ctx_has_vsx_region);
-		if (!access_ok(old_ctx, ctx_size)
-		    || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
-					ctx_has_vsx_region)
-		    || __copy_to_user(&old_ctx->uc_sigmask,
-				      &current->blocked, sizeof(sigset_t)))
+		if (!user_write_access_begin(old_ctx, ctx_size))
 			return -EFAULT;
+
+		unsafe_setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL,
+					0, ctx_has_vsx_region, efault_out);
+		unsafe_copy_to_user(&old_ctx->uc_sigmask, &current->blocked,
+				    sizeof(sigset_t), efault_out);
+
+		user_write_access_end();
 	}
 	if (new_ctx == NULL)
 		return 0;
@@ -698,6 +707,10 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
 	/* This returns like rt_sigreturn */
 	set_thread_flag(TIF_RESTOREALL);
 	return 0;
+
+efault_out:
+	user_write_access_end();
+	return -EFAULT;
 }
 
 
@@ -850,9 +863,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	} else {
 		err |= __put_user(0, &frame->uc.uc_link);
 		prepare_setup_sigcontext(tsk, 1);
-		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
-					NULL, (unsigned long)ksig->ka.sa.sa_handler,
-					1);
+		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
+			return -EFAULT;
+		err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk,
+						ksig->sig, NULL,
+						(unsigned long)ksig->ka.sa.sa_handler, 1);
+		user_write_access_end();
 	}
 	err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
 	if (err)
-- 
2.26.1

Re: [PATCH v5 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext()

[Daniel Axtens]

Hi Chris,

> Previously setup_sigcontext() performed a costly KUAP switch on every
> uaccess operation. These repeated uaccess switches cause a significant
> drop in signal handling performance.
>
> Rewrite setup_sigcontext() to assume that a userspace write access window
> is open. Replace all uaccess functions with their 'unsafe' versions
> which avoid the repeated uaccess switches.

Just to clarify the commit message a bit: you're also changing the
callers of the old safe versions to first open the window, then call the
unsafe variants, then close the window again.

> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/kernel/signal_64.c | 70 ++++++++++++++++++++-------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 8e1d804ce552..4248e4489ff1 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -101,9 +101,13 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
>   * Set up the sigcontext for the signal frame.
>   */
>  
> -static long setup_sigcontext(struct sigcontext __user *sc,
> -		struct task_struct *tsk, int signr, sigset_t *set,
> -		unsigned long handler, int ctx_has_vsx_region)
> +#define unsafe_setup_sigcontext(sc, tsk, signr, set, handler,		\
> +				ctx_has_vsx_region, e)			\
> +	unsafe_op_wrap(__unsafe_setup_sigcontext(sc, tsk, signr, set,	\
> +			handler, ctx_has_vsx_region), e)
> +static long notrace __unsafe_setup_sigcontext(struct sigcontext __user *sc,
> +					struct task_struct *tsk, int signr, sigset_t *set,
> +					unsigned long handler, int ctx_has_vsx_region)
>  {
>  	/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
>  	 * process never used altivec yet (MSR_VEC is zero in pt_regs of
> @@ -118,20 +122,19 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>  #endif
>  	struct pt_regs *regs = tsk->thread.regs;
>  	unsigned long msr = regs->msr;
> -	long err = 0;
>  	/* Force usr to alway see softe as 1 (interrupts enabled) */
>  	unsigned long softe = 0x1;
>  
>  	BUG_ON(tsk != current);
>  
>  #ifdef CONFIG_ALTIVEC
> -	err |= __put_user(v_regs, &sc->v_regs);
> +	unsafe_put_user(v_regs, &sc->v_regs, efault_out);
>  
>  	/* save altivec registers */
>  	if (tsk->thread.used_vr) {
>  		/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
> -		err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
> -				      33 * sizeof(vector128));
> +		unsafe_copy_to_user(v_regs, &tsk->thread.vr_state,
> +				    33 * sizeof(vector128), efault_out);
>  		/* set MSR_VEC in the MSR value in the frame to indicate that sc->v_reg)
>  		 * contains valid data.
>  		 */
> @@ -140,12 +143,12 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>  	/* We always copy to/from vrsave, it's 0 if we don't have or don't
>  	 * use altivec.
>  	 */
> -	err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> +	unsafe_put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33], efault_out);
>  #else /* CONFIG_ALTIVEC */
> -	err |= __put_user(0, &sc->v_regs);
> +	unsafe_put_user(0, &sc->v_regs, efault_out);
>  #endif /* CONFIG_ALTIVEC */
>  	/* copy fpr regs and fpscr */
> -	err |= copy_fpr_to_user(&sc->fp_regs, tsk);
> +	unsafe_copy_fpr_to_user(&sc->fp_regs, tsk, efault_out);
>  
>  	/*
>  	 * Clear the MSR VSX bit to indicate there is no valid state attached
> @@ -160,24 +163,27 @@ static long setup_sigcontext(struct sigcontext __user *sc,
>  	 */
>  	if (tsk->thread.used_vsr && ctx_has_vsx_region) {
>  		v_regs += ELF_NVRREG;
> -		err |= copy_vsx_to_user(v_regs, tsk);
> +		unsafe_copy_vsx_to_user(v_regs, tsk, efault_out);
>  		/* set MSR_VSX in the MSR value in the frame to
>  		 * indicate that sc->vs_reg) contains valid data.
>  		 */
>  		msr |= MSR_VSX;
>  	}
>  #endif /* CONFIG_VSX */
> -	err |= __put_user(&sc->gp_regs, &sc->regs);
> +	unsafe_put_user(&sc->gp_regs, &sc->regs, efault_out);
>  	WARN_ON(!FULL_REGS(regs));
> -	err |= __copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE);
> -	err |= __put_user(msr, &sc->gp_regs[PT_MSR]);
> -	err |= __put_user(softe, &sc->gp_regs[PT_SOFTE]);
> -	err |= __put_user(signr, &sc->signal);
> -	err |= __put_user(handler, &sc->handler);
> +	unsafe_copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE, efault_out);
> +	unsafe_put_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> +	unsafe_put_user(softe, &sc->gp_regs[PT_SOFTE], efault_out);
> +	unsafe_put_user(signr, &sc->signal, efault_out);
> +	unsafe_put_user(handler, &sc->handler, efault_out);
>  	if (set != NULL)
> -		err |=  __put_user(set->sig[0], &sc->oldmask);
> +		unsafe_put_user(set->sig[0], &sc->oldmask, efault_out);
>  
> -	return err;
> +	return 0;
> +
> +efault_out:
> +	return -EFAULT;
>  }
>  
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> @@ -664,12 +670,15 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>  
>  	if (old_ctx != NULL) {
>  		prepare_setup_sigcontext(current, ctx_has_vsx_region);
> -		if (!access_ok(old_ctx, ctx_size)
> -		    || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
> -					ctx_has_vsx_region)
> -		    || __copy_to_user(&old_ctx->uc_sigmask,
> -				      &current->blocked, sizeof(sigset_t)))
> +		if (!user_write_access_begin(old_ctx, ctx_size))
>  			return -EFAULT;

I notice we get rid of access_ok, but that's fine because
user_write_access_begin includes access_ok since Linus decided that was
a good idea.

> +
> +		unsafe_setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL,
> +					0, ctx_has_vsx_region, efault_out);
> +		unsafe_copy_to_user(&old_ctx->uc_sigmask, &current->blocked,
> +				    sizeof(sigset_t), efault_out);
> +
> +		user_write_access_end();
>  	}
>  	if (new_ctx == NULL)
>  		return 0;
> @@ -698,6 +707,10 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>  	/* This returns like rt_sigreturn */
>  	set_thread_flag(TIF_RESTOREALL);
>  	return 0;
> +
> +efault_out:
> +	user_write_access_end();
> +	return -EFAULT;
>  }
>  
>  
> @@ -850,9 +863,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>  	} else {
>  		err |= __put_user(0, &frame->uc.uc_link);
>  		prepare_setup_sigcontext(tsk, 1);
> -		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
> -					NULL, (unsigned long)ksig->ka.sa.sa_handler,
> -					1);
> +		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
> +			return -EFAULT;

Here you're opening a window for all of `frame`...

> +		err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk,
... but here you're only passing in frame->uc.uc_mcontext for writing.

ISTR that the underlying AMR switch is fully on / fully off so I don't
think it really matters, but in this case should you be calling
user_write_access_begin() with &frame->uc.uc_mcontext and the size of
that?

> +						ksig->sig, NULL,
> +						(unsigned long)ksig->ka.sa.sa_handler, 1);
> +		user_write_access_end();
>  	}
>  	err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
>  	if (err)


Apart from the size thing, everything looks good to me. I checked that
all the things you've changed from safe to unsafe pass the same
parameters, and they all looked good to me.

With those caveats,
 Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

Re: [PATCH v5 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext()

[Christopher M. Riedl]

On Thu Feb 11, 2021 at 11:41 PM CST, Daniel Axtens wrote:
> Hi Chris,
>
> > Previously setup_sigcontext() performed a costly KUAP switch on every
> > uaccess operation. These repeated uaccess switches cause a significant
> > drop in signal handling performance.
> >
> > Rewrite setup_sigcontext() to assume that a userspace write access window
> > is open. Replace all uaccess functions with their 'unsafe' versions
> > which avoid the repeated uaccess switches.
>
> Just to clarify the commit message a bit: you're also changing the
> callers of the old safe versions to first open the window, then call the
> unsafe variants, then close the window again.

Noted!

>
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >  arch/powerpc/kernel/signal_64.c | 70 ++++++++++++++++++++-------------
> >  1 file changed, 43 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index 8e1d804ce552..4248e4489ff1 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -101,9 +101,13 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
> >   * Set up the sigcontext for the signal frame.
> >   */
> >  
> > -static long setup_sigcontext(struct sigcontext __user *sc,
> > -		struct task_struct *tsk, int signr, sigset_t *set,
> > -		unsigned long handler, int ctx_has_vsx_region)
> > +#define unsafe_setup_sigcontext(sc, tsk, signr, set, handler,		\
> > +				ctx_has_vsx_region, e)			\
> > +	unsafe_op_wrap(__unsafe_setup_sigcontext(sc, tsk, signr, set,	\
> > +			handler, ctx_has_vsx_region), e)
> > +static long notrace __unsafe_setup_sigcontext(struct sigcontext __user *sc,
> > +					struct task_struct *tsk, int signr, sigset_t *set,
> > +					unsigned long handler, int ctx_has_vsx_region)
> >  {
> >  	/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
> >  	 * process never used altivec yet (MSR_VEC is zero in pt_regs of
> > @@ -118,20 +122,19 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >  #endif
> >  	struct pt_regs *regs = tsk->thread.regs;
> >  	unsigned long msr = regs->msr;
> > -	long err = 0;
> >  	/* Force usr to alway see softe as 1 (interrupts enabled) */
> >  	unsigned long softe = 0x1;
> >  
> >  	BUG_ON(tsk != current);
> >  
> >  #ifdef CONFIG_ALTIVEC
> > -	err |= __put_user(v_regs, &sc->v_regs);
> > +	unsafe_put_user(v_regs, &sc->v_regs, efault_out);
> >  
> >  	/* save altivec registers */
> >  	if (tsk->thread.used_vr) {
> >  		/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
> > -		err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
> > -				      33 * sizeof(vector128));
> > +		unsafe_copy_to_user(v_regs, &tsk->thread.vr_state,
> > +				    33 * sizeof(vector128), efault_out);
> >  		/* set MSR_VEC in the MSR value in the frame to indicate that sc->v_reg)
> >  		 * contains valid data.
> >  		 */
> > @@ -140,12 +143,12 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >  	/* We always copy to/from vrsave, it's 0 if we don't have or don't
> >  	 * use altivec.
> >  	 */
> > -	err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> > +	unsafe_put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33], efault_out);
> >  #else /* CONFIG_ALTIVEC */
> > -	err |= __put_user(0, &sc->v_regs);
> > +	unsafe_put_user(0, &sc->v_regs, efault_out);
> >  #endif /* CONFIG_ALTIVEC */
> >  	/* copy fpr regs and fpscr */
> > -	err |= copy_fpr_to_user(&sc->fp_regs, tsk);
> > +	unsafe_copy_fpr_to_user(&sc->fp_regs, tsk, efault_out);
> >  
> >  	/*
> >  	 * Clear the MSR VSX bit to indicate there is no valid state attached
> > @@ -160,24 +163,27 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> >  	 */
> >  	if (tsk->thread.used_vsr && ctx_has_vsx_region) {
> >  		v_regs += ELF_NVRREG;
> > -		err |= copy_vsx_to_user(v_regs, tsk);
> > +		unsafe_copy_vsx_to_user(v_regs, tsk, efault_out);
> >  		/* set MSR_VSX in the MSR value in the frame to
> >  		 * indicate that sc->vs_reg) contains valid data.
> >  		 */
> >  		msr |= MSR_VSX;
> >  	}
> >  #endif /* CONFIG_VSX */
> > -	err |= __put_user(&sc->gp_regs, &sc->regs);
> > +	unsafe_put_user(&sc->gp_regs, &sc->regs, efault_out);
> >  	WARN_ON(!FULL_REGS(regs));
> > -	err |= __copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE);
> > -	err |= __put_user(msr, &sc->gp_regs[PT_MSR]);
> > -	err |= __put_user(softe, &sc->gp_regs[PT_SOFTE]);
> > -	err |= __put_user(signr, &sc->signal);
> > -	err |= __put_user(handler, &sc->handler);
> > +	unsafe_copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE, efault_out);
> > +	unsafe_put_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> > +	unsafe_put_user(softe, &sc->gp_regs[PT_SOFTE], efault_out);
> > +	unsafe_put_user(signr, &sc->signal, efault_out);
> > +	unsafe_put_user(handler, &sc->handler, efault_out);
> >  	if (set != NULL)
> > -		err |=  __put_user(set->sig[0], &sc->oldmask);
> > +		unsafe_put_user(set->sig[0], &sc->oldmask, efault_out);
> >  
> > -	return err;
> > +	return 0;
> > +
> > +efault_out:
> > +	return -EFAULT;
> >  }
> >  
> >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > @@ -664,12 +670,15 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> >  
> >  	if (old_ctx != NULL) {
> >  		prepare_setup_sigcontext(current, ctx_has_vsx_region);
> > -		if (!access_ok(old_ctx, ctx_size)
> > -		    || setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
> > -					ctx_has_vsx_region)
> > -		    || __copy_to_user(&old_ctx->uc_sigmask,
> > -				      &current->blocked, sizeof(sigset_t)))
> > +		if (!user_write_access_begin(old_ctx, ctx_size))
> >  			return -EFAULT;
>
> I notice we get rid of access_ok, but that's fine because
> user_write_access_begin includes access_ok since Linus decided that was
> a good idea.
>
> > +
> > +		unsafe_setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL,
> > +					0, ctx_has_vsx_region, efault_out);
> > +		unsafe_copy_to_user(&old_ctx->uc_sigmask, &current->blocked,
> > +				    sizeof(sigset_t), efault_out);
> > +
> > +		user_write_access_end();
> >  	}
> >  	if (new_ctx == NULL)
> >  		return 0;
> > @@ -698,6 +707,10 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> >  	/* This returns like rt_sigreturn */
> >  	set_thread_flag(TIF_RESTOREALL);
> >  	return 0;
> > +
> > +efault_out:
> > +	user_write_access_end();
> > +	return -EFAULT;
> >  }
> >  
> >  
> > @@ -850,9 +863,12 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >  	} else {
> >  		err |= __put_user(0, &frame->uc.uc_link);
> >  		prepare_setup_sigcontext(tsk, 1);
> > -		err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
> > -					NULL, (unsigned long)ksig->ka.sa.sa_handler,
> > -					1);
> > +		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
> > +			return -EFAULT;
>
> Here you're opening a window for all of `frame`...
>
> > +		err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk,
> ... but here you're only passing in frame->uc.uc_mcontext for writing.
>
> ISTR that the underlying AMR switch is fully on / fully off so I don't
> think it really matters, but in this case should you be calling
> user_write_access_begin() with &frame->uc.uc_mcontext and the size of
> that?

You are correct that it doesn't _really_ matter w/ the current
implementation of KUAP/AMR. It's still inconsistent and could cause
problems in the future so I will fix this to pass the proper size.

>
> > +						ksig->sig, NULL,
> > +						(unsigned long)ksig->ka.sa.sa_handler, 1);
> > +		user_write_access_end();
> >  	}
> >  	err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
> >  	if (err)
>
>
> Apart from the size thing, everything looks good to me. I checked that
> all the things you've changed from safe to unsafe pass the same
> parameters, and they all looked good to me.

Thanks!

>
> With those caveats,
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Kind regards,
> Daniel

[PATCH v5 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()

[Christopher M. Riedl]

Previously restore_sigcontext() performed a costly KUAP switch on every
uaccess operation. These repeated uaccess switches cause a significant
drop in signal handling performance.

Rewrite restore_sigcontext() to assume that a userspace read access
window is open. Replace all uaccess functions with their 'unsafe'
versions which avoid the repeated uaccess switches.

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 68 ++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 4248e4489ff1..d668f8af18fe 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -326,14 +326,14 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
 /*
  * Restore the sigcontext from the signal frame.
  */
-
-static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
-			      struct sigcontext __user *sc)
+#define unsafe_restore_sigcontext(tsk, set, sig, sc, e) \
+	unsafe_op_wrap(__unsafe_restore_sigcontext(tsk, set, sig, sc), e)
+static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_t *set,
+						int sig, struct sigcontext __user *sc)
 {
 #ifdef CONFIG_ALTIVEC
 	elf_vrreg_t __user *v_regs;
 #endif
-	unsigned long err = 0;
 	unsigned long save_r13 = 0;
 	unsigned long msr;
 	struct pt_regs *regs = tsk->thread.regs;
@@ -348,27 +348,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
 		save_r13 = regs->gpr[13];
 
 	/* copy the GPRs */
-	err |= __copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr));
-	err |= __get_user(regs->nip, &sc->gp_regs[PT_NIP]);
+	unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr),
+			      efault_out);
+	unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
 	/* get MSR separately, transfer the LE bit if doing signal return */
-	err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
+	unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
 	if (sig)
 		regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
-	err |= __get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3]);
-	err |= __get_user(regs->ctr, &sc->gp_regs[PT_CTR]);
-	err |= __get_user(regs->link, &sc->gp_regs[PT_LNK]);
-	err |= __get_user(regs->xer, &sc->gp_regs[PT_XER]);
-	err |= __get_user(regs->ccr, &sc->gp_regs[PT_CCR]);
+	unsafe_get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3], efault_out);
+	unsafe_get_user(regs->ctr, &sc->gp_regs[PT_CTR], efault_out);
+	unsafe_get_user(regs->link, &sc->gp_regs[PT_LNK], efault_out);
+	unsafe_get_user(regs->xer, &sc->gp_regs[PT_XER], efault_out);
+	unsafe_get_user(regs->ccr, &sc->gp_regs[PT_CCR], efault_out);
 	/* Don't allow userspace to set SOFTE */
 	set_trap_norestart(regs);
-	err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
-	err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
-	err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
+	unsafe_get_user(regs->dar, &sc->gp_regs[PT_DAR], efault_out);
+	unsafe_get_user(regs->dsisr, &sc->gp_regs[PT_DSISR], efault_out);
+	unsafe_get_user(regs->result, &sc->gp_regs[PT_RESULT], efault_out);
 
 	if (!sig)
 		regs->gpr[13] = save_r13;
 	if (set != NULL)
-		err |=  __get_user(set->sig[0], &sc->oldmask);
+		unsafe_get_user(set->sig[0], &sc->oldmask, efault_out);
 
 	/*
 	 * Force reload of FP/VEC.
@@ -378,29 +379,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
 	regs->msr &= ~(MSR_FP | MSR_FE0 | MSR_FE1 | MSR_VEC | MSR_VSX);
 
 #ifdef CONFIG_ALTIVEC
-	err |= __get_user(v_regs, &sc->v_regs);
-	if (err)
-		return err;
+	unsafe_get_user(v_regs, &sc->v_regs, efault_out);
 	if (v_regs && !access_ok(v_regs, 34 * sizeof(vector128)))
 		return -EFAULT;
 	/* Copy 33 vec registers (vr0..31 and vscr) from the stack */
 	if (v_regs != NULL && (msr & MSR_VEC) != 0) {
-		err |= __copy_from_user(&tsk->thread.vr_state, v_regs,
-					33 * sizeof(vector128));
+		unsafe_copy_from_user(&tsk->thread.vr_state, v_regs,
+				      33 * sizeof(vector128), efault_out);
 		tsk->thread.used_vr = true;
 	} else if (tsk->thread.used_vr) {
 		memset(&tsk->thread.vr_state, 0, 33 * sizeof(vector128));
 	}
 	/* Always get VRSAVE back */
 	if (v_regs != NULL)
-		err |= __get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
+		unsafe_get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33],
+				efault_out);
 	else
 		tsk->thread.vrsave = 0;
 	if (cpu_has_feature(CPU_FTR_ALTIVEC))
 		mtspr(SPRN_VRSAVE, tsk->thread.vrsave);
 #endif /* CONFIG_ALTIVEC */
 	/* restore floating point */
-	err |= copy_fpr_from_user(tsk, &sc->fp_regs);
+	unsafe_copy_fpr_from_user(tsk, &sc->fp_regs, efault_out);
 #ifdef CONFIG_VSX
 	/*
 	 * Get additional VSX data. Update v_regs to point after the
@@ -409,14 +409,17 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
 	 */
 	v_regs += ELF_NVRREG;
 	if ((msr & MSR_VSX) != 0) {
-		err |= copy_vsx_from_user(tsk, v_regs);
+		unsafe_copy_vsx_from_user(tsk, v_regs, efault_out);
 		tsk->thread.used_vsr = true;
 	} else {
 		for (i = 0; i < 32 ; i++)
 			tsk->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
 	}
 #endif
-	return err;
+	return 0;
+
+efault_out:
+	return -EFAULT;
 }
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
@@ -701,8 +704,14 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
 	if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
 		do_exit(SIGSEGV);
 	set_current_blocked(&set);
-	if (restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext))
+
+	if (!user_read_access_begin(new_ctx, ctx_size))
+		return -EFAULT;
+	if (__unsafe_restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext)) {
+		user_read_access_end();
 		do_exit(SIGSEGV);
+	}
+	user_read_access_end();
 
 	/* This returns like rt_sigreturn */
 	set_thread_flag(TIF_RESTOREALL);
@@ -807,8 +816,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
 		 * causing a TM bad thing.
 		 */
 		current->thread.regs->msr &= ~MSR_TS_MASK;
-		if (restore_sigcontext(current, NULL, 1, &uc->uc_mcontext))
+		if (!user_read_access_begin(uc, sizeof(*uc)))
+			return -EFAULT;
+		if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
+			user_read_access_end();
 			goto badframe;
+		}
+		user_read_access_end();
 	}
 
 	if (restore_altstack(&uc->uc_stack))
-- 
2.26.1

Re: [PATCH v5 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()

[Daniel Axtens]

Hi Chris,

> Previously restore_sigcontext() performed a costly KUAP switch on every
> uaccess operation. These repeated uaccess switches cause a significant
> drop in signal handling performance.
>
> Rewrite restore_sigcontext() to assume that a userspace read access
> window is open. Replace all uaccess functions with their 'unsafe'
> versions which avoid the repeated uaccess switches.
>

Much of the same comments apply here as to the last patch:
 - the commit message might be improved by adding that you are also
   changing the calling functions to open the uaccess window before
   calling into the new unsafe functions

 - I have checked that the safe to unsafe conversions look right.

 - I think you're opening too wide a window in user_read_access_begin,
   it seems to me that it could be reduced to just the
   uc_mcontext. (Again, not that it makes a difference with the current
   HW.)

Kind regards,
Daniel

> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/kernel/signal_64.c | 68 ++++++++++++++++++++-------------
>  1 file changed, 41 insertions(+), 27 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 4248e4489ff1..d668f8af18fe 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -326,14 +326,14 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
>  /*
>   * Restore the sigcontext from the signal frame.
>   */
> -
> -static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> -			      struct sigcontext __user *sc)
> +#define unsafe_restore_sigcontext(tsk, set, sig, sc, e) \
> +	unsafe_op_wrap(__unsafe_restore_sigcontext(tsk, set, sig, sc), e)
> +static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_t *set,
> +						int sig, struct sigcontext __user *sc)
>  {
>  #ifdef CONFIG_ALTIVEC
>  	elf_vrreg_t __user *v_regs;
>  #endif
> -	unsigned long err = 0;
>  	unsigned long save_r13 = 0;
>  	unsigned long msr;
>  	struct pt_regs *regs = tsk->thread.regs;
> @@ -348,27 +348,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
>  		save_r13 = regs->gpr[13];
>  
>  	/* copy the GPRs */
> -	err |= __copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr));
> -	err |= __get_user(regs->nip, &sc->gp_regs[PT_NIP]);
> +	unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr),
> +			      efault_out);
> +	unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
>  	/* get MSR separately, transfer the LE bit if doing signal return */
> -	err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
> +	unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
>  	if (sig)
>  		regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
> -	err |= __get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3]);
> -	err |= __get_user(regs->ctr, &sc->gp_regs[PT_CTR]);
> -	err |= __get_user(regs->link, &sc->gp_regs[PT_LNK]);
> -	err |= __get_user(regs->xer, &sc->gp_regs[PT_XER]);
> -	err |= __get_user(regs->ccr, &sc->gp_regs[PT_CCR]);
> +	unsafe_get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3], efault_out);
> +	unsafe_get_user(regs->ctr, &sc->gp_regs[PT_CTR], efault_out);
> +	unsafe_get_user(regs->link, &sc->gp_regs[PT_LNK], efault_out);
> +	unsafe_get_user(regs->xer, &sc->gp_regs[PT_XER], efault_out);
> +	unsafe_get_user(regs->ccr, &sc->gp_regs[PT_CCR], efault_out);
>  	/* Don't allow userspace to set SOFTE */
>  	set_trap_norestart(regs);
> -	err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
> -	err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
> -	err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
> +	unsafe_get_user(regs->dar, &sc->gp_regs[PT_DAR], efault_out);
> +	unsafe_get_user(regs->dsisr, &sc->gp_regs[PT_DSISR], efault_out);
> +	unsafe_get_user(regs->result, &sc->gp_regs[PT_RESULT], efault_out);
>  
>  	if (!sig)
>  		regs->gpr[13] = save_r13;
>  	if (set != NULL)
> -		err |=  __get_user(set->sig[0], &sc->oldmask);
> +		unsafe_get_user(set->sig[0], &sc->oldmask, efault_out);
>  
>  	/*
>  	 * Force reload of FP/VEC.
> @@ -378,29 +379,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
>  	regs->msr &= ~(MSR_FP | MSR_FE0 | MSR_FE1 | MSR_VEC | MSR_VSX);
>  
>  #ifdef CONFIG_ALTIVEC
> -	err |= __get_user(v_regs, &sc->v_regs);
> -	if (err)
> -		return err;
> +	unsafe_get_user(v_regs, &sc->v_regs, efault_out);
>  	if (v_regs && !access_ok(v_regs, 34 * sizeof(vector128)))
>  		return -EFAULT;
>  	/* Copy 33 vec registers (vr0..31 and vscr) from the stack */
>  	if (v_regs != NULL && (msr & MSR_VEC) != 0) {
> -		err |= __copy_from_user(&tsk->thread.vr_state, v_regs,
> -					33 * sizeof(vector128));
> +		unsafe_copy_from_user(&tsk->thread.vr_state, v_regs,
> +				      33 * sizeof(vector128), efault_out);
>  		tsk->thread.used_vr = true;
>  	} else if (tsk->thread.used_vr) {
>  		memset(&tsk->thread.vr_state, 0, 33 * sizeof(vector128));
>  	}
>  	/* Always get VRSAVE back */
>  	if (v_regs != NULL)
> -		err |= __get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> +		unsafe_get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33],
> +				efault_out);
>  	else
>  		tsk->thread.vrsave = 0;
>  	if (cpu_has_feature(CPU_FTR_ALTIVEC))
>  		mtspr(SPRN_VRSAVE, tsk->thread.vrsave);
>  #endif /* CONFIG_ALTIVEC */
>  	/* restore floating point */
> -	err |= copy_fpr_from_user(tsk, &sc->fp_regs);
> +	unsafe_copy_fpr_from_user(tsk, &sc->fp_regs, efault_out);
>  #ifdef CONFIG_VSX
>  	/*
>  	 * Get additional VSX data. Update v_regs to point after the
> @@ -409,14 +409,17 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
>  	 */
>  	v_regs += ELF_NVRREG;
>  	if ((msr & MSR_VSX) != 0) {
> -		err |= copy_vsx_from_user(tsk, v_regs);
> +		unsafe_copy_vsx_from_user(tsk, v_regs, efault_out);
>  		tsk->thread.used_vsr = true;
>  	} else {
>  		for (i = 0; i < 32 ; i++)
>  			tsk->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
>  	}
>  #endif
> -	return err;
> +	return 0;
> +
> +efault_out:
> +	return -EFAULT;
>  }
>  
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> @@ -701,8 +704,14 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>  	if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
>  		do_exit(SIGSEGV);
>  	set_current_blocked(&set);
> -	if (restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext))
> +
> +	if (!user_read_access_begin(new_ctx, ctx_size))
> +		return -EFAULT;
> +	if (__unsafe_restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext)) {
> +		user_read_access_end();
>  		do_exit(SIGSEGV);
> +	}
> +	user_read_access_end();
>  
>  	/* This returns like rt_sigreturn */
>  	set_thread_flag(TIF_RESTOREALL);
> @@ -807,8 +816,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  		 * causing a TM bad thing.
>  		 */
>  		current->thread.regs->msr &= ~MSR_TS_MASK;
> -		if (restore_sigcontext(current, NULL, 1, &uc->uc_mcontext))
> +		if (!user_read_access_begin(uc, sizeof(*uc)))
> +			return -EFAULT;
> +		if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
> +			user_read_access_end();
>  			goto badframe;
> +		}
> +		user_read_access_end();
>  	}
>  
>  	if (restore_altstack(&uc->uc_stack))
> -- 
> 2.26.1

Re: [PATCH v5 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()

[Christopher M. Riedl]

On Fri Feb 12, 2021 at 3:17 PM CST, Daniel Axtens wrote:
> Hi Chris,
>
> > Previously restore_sigcontext() performed a costly KUAP switch on every
> > uaccess operation. These repeated uaccess switches cause a significant
> > drop in signal handling performance.
> >
> > Rewrite restore_sigcontext() to assume that a userspace read access
> > window is open. Replace all uaccess functions with their 'unsafe'
> > versions which avoid the repeated uaccess switches.
> >
>
> Much of the same comments apply here as to the last patch:
> - the commit message might be improved by adding that you are also
> changing the calling functions to open the uaccess window before
> calling into the new unsafe functions
>
> - I have checked that the safe to unsafe conversions look right.
>
> - I think you're opening too wide a window in user_read_access_begin,
> it seems to me that it could be reduced to just the
> uc_mcontext. (Again, not that it makes a difference with the current
> HW.)

Ok, I'll fix these in the next version as well.

>
> Kind regards,
> Daniel
>
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >  arch/powerpc/kernel/signal_64.c | 68 ++++++++++++++++++++-------------
> >  1 file changed, 41 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index 4248e4489ff1..d668f8af18fe 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -326,14 +326,14 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
> >  /*
> >   * Restore the sigcontext from the signal frame.
> >   */
> > -
> > -static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > -			      struct sigcontext __user *sc)
> > +#define unsafe_restore_sigcontext(tsk, set, sig, sc, e) \
> > +	unsafe_op_wrap(__unsafe_restore_sigcontext(tsk, set, sig, sc), e)
> > +static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_t *set,
> > +						int sig, struct sigcontext __user *sc)
> >  {
> >  #ifdef CONFIG_ALTIVEC
> >  	elf_vrreg_t __user *v_regs;
> >  #endif
> > -	unsigned long err = 0;
> >  	unsigned long save_r13 = 0;
> >  	unsigned long msr;
> >  	struct pt_regs *regs = tsk->thread.regs;
> > @@ -348,27 +348,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> >  		save_r13 = regs->gpr[13];
> >  
> >  	/* copy the GPRs */
> > -	err |= __copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr));
> > -	err |= __get_user(regs->nip, &sc->gp_regs[PT_NIP]);
> > +	unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr),
> > +			      efault_out);
> > +	unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
> >  	/* get MSR separately, transfer the LE bit if doing signal return */
> > -	err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
> > +	unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> >  	if (sig)
> >  		regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
> > -	err |= __get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3]);
> > -	err |= __get_user(regs->ctr, &sc->gp_regs[PT_CTR]);
> > -	err |= __get_user(regs->link, &sc->gp_regs[PT_LNK]);
> > -	err |= __get_user(regs->xer, &sc->gp_regs[PT_XER]);
> > -	err |= __get_user(regs->ccr, &sc->gp_regs[PT_CCR]);
> > +	unsafe_get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3], efault_out);
> > +	unsafe_get_user(regs->ctr, &sc->gp_regs[PT_CTR], efault_out);
> > +	unsafe_get_user(regs->link, &sc->gp_regs[PT_LNK], efault_out);
> > +	unsafe_get_user(regs->xer, &sc->gp_regs[PT_XER], efault_out);
> > +	unsafe_get_user(regs->ccr, &sc->gp_regs[PT_CCR], efault_out);
> >  	/* Don't allow userspace to set SOFTE */
> >  	set_trap_norestart(regs);
> > -	err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
> > -	err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
> > -	err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
> > +	unsafe_get_user(regs->dar, &sc->gp_regs[PT_DAR], efault_out);
> > +	unsafe_get_user(regs->dsisr, &sc->gp_regs[PT_DSISR], efault_out);
> > +	unsafe_get_user(regs->result, &sc->gp_regs[PT_RESULT], efault_out);
> >  
> >  	if (!sig)
> >  		regs->gpr[13] = save_r13;
> >  	if (set != NULL)
> > -		err |=  __get_user(set->sig[0], &sc->oldmask);
> > +		unsafe_get_user(set->sig[0], &sc->oldmask, efault_out);
> >  
> >  	/*
> >  	 * Force reload of FP/VEC.
> > @@ -378,29 +379,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> >  	regs->msr &= ~(MSR_FP | MSR_FE0 | MSR_FE1 | MSR_VEC | MSR_VSX);
> >  
> >  #ifdef CONFIG_ALTIVEC
> > -	err |= __get_user(v_regs, &sc->v_regs);
> > -	if (err)
> > -		return err;
> > +	unsafe_get_user(v_regs, &sc->v_regs, efault_out);
> >  	if (v_regs && !access_ok(v_regs, 34 * sizeof(vector128)))
> >  		return -EFAULT;
> >  	/* Copy 33 vec registers (vr0..31 and vscr) from the stack */
> >  	if (v_regs != NULL && (msr & MSR_VEC) != 0) {
> > -		err |= __copy_from_user(&tsk->thread.vr_state, v_regs,
> > -					33 * sizeof(vector128));
> > +		unsafe_copy_from_user(&tsk->thread.vr_state, v_regs,
> > +				      33 * sizeof(vector128), efault_out);
> >  		tsk->thread.used_vr = true;
> >  	} else if (tsk->thread.used_vr) {
> >  		memset(&tsk->thread.vr_state, 0, 33 * sizeof(vector128));
> >  	}
> >  	/* Always get VRSAVE back */
> >  	if (v_regs != NULL)
> > -		err |= __get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> > +		unsafe_get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33],
> > +				efault_out);
> >  	else
> >  		tsk->thread.vrsave = 0;
> >  	if (cpu_has_feature(CPU_FTR_ALTIVEC))
> >  		mtspr(SPRN_VRSAVE, tsk->thread.vrsave);
> >  #endif /* CONFIG_ALTIVEC */
> >  	/* restore floating point */
> > -	err |= copy_fpr_from_user(tsk, &sc->fp_regs);
> > +	unsafe_copy_fpr_from_user(tsk, &sc->fp_regs, efault_out);
> >  #ifdef CONFIG_VSX
> >  	/*
> >  	 * Get additional VSX data. Update v_regs to point after the
> > @@ -409,14 +409,17 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> >  	 */
> >  	v_regs += ELF_NVRREG;
> >  	if ((msr & MSR_VSX) != 0) {
> > -		err |= copy_vsx_from_user(tsk, v_regs);
> > +		unsafe_copy_vsx_from_user(tsk, v_regs, efault_out);
> >  		tsk->thread.used_vsr = true;
> >  	} else {
> >  		for (i = 0; i < 32 ; i++)
> >  			tsk->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
> >  	}
> >  #endif
> > -	return err;
> > +	return 0;
> > +
> > +efault_out:
> > +	return -EFAULT;
> >  }
> >  
> >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > @@ -701,8 +704,14 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> >  	if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
> >  		do_exit(SIGSEGV);
> >  	set_current_blocked(&set);
> > -	if (restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext))
> > +
> > +	if (!user_read_access_begin(new_ctx, ctx_size))
> > +		return -EFAULT;
> > +	if (__unsafe_restore_sigcontext(current, NULL, 0, &new_ctx->uc_mcontext)) {
> > +		user_read_access_end();
> >  		do_exit(SIGSEGV);
> > +	}
> > +	user_read_access_end();
> >  
> >  	/* This returns like rt_sigreturn */
> >  	set_thread_flag(TIF_RESTOREALL);
> > @@ -807,8 +816,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >  		 * causing a TM bad thing.
> >  		 */
> >  		current->thread.regs->msr &= ~MSR_TS_MASK;
> > -		if (restore_sigcontext(current, NULL, 1, &uc->uc_mcontext))
> > +		if (!user_read_access_begin(uc, sizeof(*uc)))
> > +			return -EFAULT;
> > +		if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
> > +			user_read_access_end();
> >  			goto badframe;
> > +		}
> > +		user_read_access_end();
> >  	}
> >  
> >  	if (restore_altstack(&uc->uc_stack))
> > -- 
> > 2.26.1

[PATCH v5 08/10] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches

[Christopher M. Riedl]

From: Daniel Axtens <dja@axtens.net>

Add uaccess blocks and use the 'unsafe' versions of functions doing user
access where possible to reduce the number of times uaccess has to be
opened/closed.

There is no 'unsafe' version of copy_siginfo_to_user, so move it
slightly to allow for a "longer" uaccess block.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Co-developed-by: Christopher M. Riedl <cmr@codefail.de>
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 54 +++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index d668f8af18fe..a471e97589a8 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -849,44 +849,51 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	unsigned long msr = regs->msr;
 
 	frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
-	if (!access_ok(frame, sizeof(*frame)))
-		goto badframe;
 
-	err |= __put_user(&frame->info, &frame->pinfo);
-	err |= __put_user(&frame->uc, &frame->puc);
-	err |= copy_siginfo_to_user(&frame->info, &ksig->info);
-	if (err)
+	/* This only applies when calling unsafe_setup_sigcontext() and must be
+	 * called before opening the uaccess window.
+	 */
+	if (!MSR_TM_ACTIVE(msr))
+		prepare_setup_sigcontext(tsk, 1);
+
+	if (!user_write_access_begin(frame, sizeof(*frame)))
 		goto badframe;
 
+	unsafe_put_user(&frame->info, &frame->pinfo, badframe_block);
+	unsafe_put_user(&frame->uc, &frame->puc, badframe_block);
+
 	/* Create the ucontext.  */
-	err |= __put_user(0, &frame->uc.uc_flags);
-	err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
+	unsafe_put_user(0, &frame->uc.uc_flags, badframe_block);
+	unsafe_save_altstack(&frame->uc.uc_stack, regs->gpr[1], badframe_block);
 
 	if (MSR_TM_ACTIVE(msr)) {
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 		/* The ucontext_t passed to userland points to the second
 		 * ucontext_t (for transactional state) with its uc_link ptr.
 		 */
-		err |= __put_user(&frame->uc_transact, &frame->uc.uc_link);
+		unsafe_put_user(&frame->uc_transact, &frame->uc.uc_link, badframe_block);
+
+		user_write_access_end();
+
 		err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
 					    &frame->uc_transact.uc_mcontext,
 					    tsk, ksig->sig, NULL,
 					    (unsigned long)ksig->ka.sa.sa_handler,
 					    msr);
+
+		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
+			goto badframe;
+
 #endif
 	} else {
-		err |= __put_user(0, &frame->uc.uc_link);
-		prepare_setup_sigcontext(tsk, 1);
-		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
-			return -EFAULT;
-		err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk,
-						ksig->sig, NULL,
-						(unsigned long)ksig->ka.sa.sa_handler, 1);
-		user_write_access_end();
+		unsafe_put_user(0, &frame->uc.uc_link, badframe_block);
+		unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
+					NULL, (unsigned long)ksig->ka.sa.sa_handler,
+					1, badframe_block);
 	}
-	err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
-	if (err)
-		goto badframe;
+
+	unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
+	user_write_access_end();
 
 	/* Make sure signal handler doesn't get spurious FP exceptions */
 	tsk->thread.fp_state.fpscr = 0;
@@ -901,6 +908,11 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 		regs->nip = (unsigned long) &frame->tramp[0];
 	}
 
+
+	/* Save the siginfo outside of the unsafe block. */
+	if (copy_siginfo_to_user(&frame->info, &ksig->info))
+		goto badframe;
+
 	/* Allocate a dummy caller frame for the signal handler. */
 	newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
 	err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
@@ -940,6 +952,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 
 	return 0;
 
+badframe_block:
+	user_write_access_end();
 badframe:
 	signal_fault(current, regs, "handle_rt_signal64", frame);
 
-- 
2.26.1

[PATCH v5 09/10] powerpc/signal64: Rewrite rt_sigreturn() to minimise uaccess switches

[Christopher M. Riedl]

From: Daniel Axtens <dja@axtens.net>

Add uaccess blocks and use the 'unsafe' versions of functions doing user
access where possible to reduce the number of times uaccess has to be
opened/closed.

Signed-off-by: Daniel Axtens <dja@axtens.net>
Co-developed-by: Christopher M. Riedl <cmr@codefail.de>
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index a471e97589a8..817b64e1e409 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -782,9 +782,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	 * restore_tm_sigcontexts.
 	 */
 	regs->msr &= ~MSR_TS_MASK;
+#endif
 
-	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
+	if (!user_read_access_begin(uc, sizeof(*uc)))
 		goto badframe;
+
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	unsafe_get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR], badframe_block);
 #endif
 
 	if (MSR_TM_ACTIVE(msr)) {
@@ -794,10 +798,12 @@ SYSCALL_DEFINE0(rt_sigreturn)
 
 		/* Trying to start TM on non TM system */
 		if (!cpu_has_feature(CPU_FTR_TM))
-			goto badframe;
+			goto badframe_block;
+
+		unsafe_get_user(uc_transact, &uc->uc_link, badframe_block);
+
+		user_read_access_end();
 
-		if (__get_user(uc_transact, &uc->uc_link))
-			goto badframe;
 		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
 					   &uc_transact->uc_mcontext))
 			goto badframe;
@@ -816,12 +822,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
 		 * causing a TM bad thing.
 		 */
 		current->thread.regs->msr &= ~MSR_TS_MASK;
-		if (!user_read_access_begin(uc, sizeof(*uc)))
-			return -EFAULT;
-		if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
-			user_read_access_end();
-			goto badframe;
-		}
+		unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext,
+					  badframe_block);
+
 		user_read_access_end();
 	}
 
@@ -831,6 +834,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	set_thread_flag(TIF_RESTOREALL);
 	return 0;
 
+badframe_block:
+	user_read_access_end();
 badframe:
 	signal_fault(current, regs, "rt_sigreturn", uc);
 
-- 
2.26.1

[PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t

[Christopher M. Riedl]

Usually sigset_t is exactly 8B which is a "trivial" size and does not
warrant using __copy_from_user(). Use __get_user() directly in
anticipation of future work to remove the trivial size optimizations
from __copy_from_user(). Calling __get_user() also results in a small
boost to signal handling throughput here.

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 817b64e1e409..42fdc4a7ff72 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -97,6 +97,14 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
 #endif /* CONFIG_VSX */
 }
 
+static inline int get_user_sigset(sigset_t *dst, const sigset_t *src)
+{
+	if (sizeof(sigset_t) <= 8)
+		return __get_user(dst->sig[0], &src->sig[0]);
+	else
+		return __copy_from_user(dst, src, sizeof(sigset_t));
+}
+
 /*
  * Set up the sigcontext for the signal frame.
  */
@@ -701,8 +709,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
 	 * We kill the task with a SIGSEGV in this situation.
 	 */
 
-	if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
+	if (get_user_sigset(&set, &new_ctx->uc_sigmask))
 		do_exit(SIGSEGV);
+
 	set_current_blocked(&set);
 
 	if (!user_read_access_begin(new_ctx, ctx_size))
@@ -740,8 +749,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	if (!access_ok(uc, sizeof(*uc)))
 		goto badframe;
 
-	if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
+	if (get_user_sigset(&set, &uc->uc_sigmask))
 		goto badframe;
+
 	set_current_blocked(&set);
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-- 
2.26.1

Re: [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t

[Christopher M. Riedl]

On Wed Feb 3, 2021 at 12:43 PM CST, Christopher M. Riedl wrote:
> Usually sigset_t is exactly 8B which is a "trivial" size and does not
> warrant using __copy_from_user(). Use __get_user() directly in
> anticipation of future work to remove the trivial size optimizations
> from __copy_from_user(). Calling __get_user() also results in a small
> boost to signal handling throughput here.
>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>

This patch triggered sparse warnings about 'different address spaces'.
This minor fixup cleans that up:

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 42fdc4a7ff72..1dfda6403e14 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -97,7 +97,7 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
 #endif /* CONFIG_VSX */
 }

-static inline int get_user_sigset(sigset_t *dst, const sigset_t *src)
+static inline int get_user_sigset(sigset_t *dst, const sigset_t __user *src)
 {
	if (sizeof(sigset_t) <= 8)
		return __get_user(dst->sig[0], &src->sig[0]);

Re: [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t

[Christophe Leroy]

"Christopher M. Riedl" <cmr@codefail.de> a écrit :

> Usually sigset_t is exactly 8B which is a "trivial" size and does not
> warrant using __copy_from_user(). Use __get_user() directly in
> anticipation of future work to remove the trivial size optimizations
> from __copy_from_user(). Calling __get_user() also results in a small
> boost to signal handling throughput here.
>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/kernel/signal_64.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c  
> b/arch/powerpc/kernel/signal_64.c
> index 817b64e1e409..42fdc4a7ff72 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -97,6 +97,14 @@ static void prepare_setup_sigcontext(struct  
> task_struct *tsk, int ctx_has_vsx_re
>  #endif /* CONFIG_VSX */
>  }
>
> +static inline int get_user_sigset(sigset_t *dst, const sigset_t *src)

Should be called __get_user_sigset() as it is a helper for __get_user()

> +{
> +	if (sizeof(sigset_t) <= 8)

We should always use __get_user(), see below.

> +		return __get_user(dst->sig[0], &src->sig[0]);

I think the above will not work on ppc32, it will only copy 4 bytes.
You must cast the source to u64*

> +	else
> +		return __copy_from_user(dst, src, sizeof(sigset_t));

I see no point in keeping this alternative. Today sigset_ t is fixed.
If you fear one day someone might change it to something different  
than a u64, just add a BUILD_BUG_ON(sizeof(sigset_t) != sizeof(u64));

> +}
> +
>  /*
>   * Set up the sigcontext for the signal frame.
>   */
> @@ -701,8 +709,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext  
> __user *, old_ctx,
>  	 * We kill the task with a SIGSEGV in this situation.
>  	 */
>
> -	if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
> +	if (get_user_sigset(&set, &new_ctx->uc_sigmask))
>  		do_exit(SIGSEGV);
> +

This white space is not part of the change, keep patches to the  
minimum, avoid cosmetic

>  	set_current_blocked(&set);
>
>  	if (!user_read_access_begin(new_ctx, ctx_size))
> @@ -740,8 +749,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  	if (!access_ok(uc, sizeof(*uc)))
>  		goto badframe;
>
> -	if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
> +	if (get_user_sigset(&set, &uc->uc_sigmask))
>  		goto badframe;
> +

Same

>  	set_current_blocked(&set);
>
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> --
> 2.26.1

Re: [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t

[Christopher M. Riedl]

On Tue Feb 9, 2021 at 3:45 PM CST, Christophe Leroy wrote:
> "Christopher M. Riedl" <cmr@codefail.de> a écrit :
>
> > Usually sigset_t is exactly 8B which is a "trivial" size and does not
> > warrant using __copy_from_user(). Use __get_user() directly in
> > anticipation of future work to remove the trivial size optimizations
> > from __copy_from_user(). Calling __get_user() also results in a small
> > boost to signal handling throughput here.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >  arch/powerpc/kernel/signal_64.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c  
> > b/arch/powerpc/kernel/signal_64.c
> > index 817b64e1e409..42fdc4a7ff72 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -97,6 +97,14 @@ static void prepare_setup_sigcontext(struct  
> > task_struct *tsk, int ctx_has_vsx_re
> >  #endif /* CONFIG_VSX */
> >  }
> >
> > +static inline int get_user_sigset(sigset_t *dst, const sigset_t *src)
>
> Should be called __get_user_sigset() as it is a helper for __get_user()

Ok makes sense.

>
> > +{
> > +	if (sizeof(sigset_t) <= 8)
>
> We should always use __get_user(), see below.
>
> > +		return __get_user(dst->sig[0], &src->sig[0]);
>
> I think the above will not work on ppc32, it will only copy 4 bytes.
> You must cast the source to u64*

Well this is signal_64.c :) Looks like ppc32 needs the same thing so
I'll just move this into signal.h and use it for both. 

The only exception would be the COMPAT case in signal_32.c which ends up
calling the common get_compat_sigset(). Updating that is probably
outside the scope of this series.

>
> > +	else
> > +		return __copy_from_user(dst, src, sizeof(sigset_t));
>
> I see no point in keeping this alternative. Today sigset_ t is fixed.
> If you fear one day someone might change it to something different
> than a u64, just add a BUILD_BUG_ON(sizeof(sigset_t) != sizeof(u64));

Ah yes that is much better - thanks for the suggestion.

>
> > +}
> > +
> >  /*
> >   * Set up the sigcontext for the signal frame.
> >   */
> > @@ -701,8 +709,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext  
> > __user *, old_ctx,
> >  	 * We kill the task with a SIGSEGV in this situation.
> >  	 */
> >
> > -	if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
> > +	if (get_user_sigset(&set, &new_ctx->uc_sigmask))
> >  		do_exit(SIGSEGV);
> > +
>
> This white space is not part of the change, keep patches to the
> minimum, avoid cosmetic

Just a (bad?) habit on my part that I missed - I'll remove this one and
the one further below.

>
> >  	set_current_blocked(&set);
> >
> >  	if (!user_read_access_begin(new_ctx, ctx_size))
> > @@ -740,8 +749,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >  	if (!access_ok(uc, sizeof(*uc)))
> >  		goto badframe;
> >
> > -	if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
> > +	if (get_user_sigset(&set, &uc->uc_sigmask))
> >  		goto badframe;
> > +
>
> Same
>
> >  	set_current_blocked(&set);
> >
> >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > --
> > 2.26.1

Re: [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t

[Daniel Axtens]

"Christopher M. Riedl" <cmr@codefail.de> writes:

> Usually sigset_t is exactly 8B which is a "trivial" size and does not
> warrant using __copy_from_user(). Use __get_user() directly in
> anticipation of future work to remove the trivial size optimizations
> from __copy_from_user(). Calling __get_user() also results in a small
> boost to signal handling throughput here.

Modulo the comments from Christophe, this looks good to me. It seems to
do what it says on the tin.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Do you know if this patch is responsible for the slightly increase in
radix performance you observed in your cover letter? The rest of the
series shouldn't really make things faster than the no-KUAP case...

Kind regards,
Daniel

>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>  arch/powerpc/kernel/signal_64.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 817b64e1e409..42fdc4a7ff72 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -97,6 +97,14 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
>  #endif /* CONFIG_VSX */
>  }
>  
> +static inline int get_user_sigset(sigset_t *dst, const sigset_t *src)
> +{
> +	if (sizeof(sigset_t) <= 8)
> +		return __get_user(dst->sig[0], &src->sig[0]);
> +	else
> +		return __copy_from_user(dst, src, sizeof(sigset_t));
> +}
> +
>  /*
>   * Set up the sigcontext for the signal frame.
>   */
> @@ -701,8 +709,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>  	 * We kill the task with a SIGSEGV in this situation.
>  	 */
>  
> -	if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
> +	if (get_user_sigset(&set, &new_ctx->uc_sigmask))
>  		do_exit(SIGSEGV);
> +
>  	set_current_blocked(&set);
>  
>  	if (!user_read_access_begin(new_ctx, ctx_size))
> @@ -740,8 +749,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
>  	if (!access_ok(uc, sizeof(*uc)))
>  		goto badframe;
>  
> -	if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
> +	if (get_user_sigset(&set, &uc->uc_sigmask))
>  		goto badframe;
> +
>  	set_current_blocked(&set);
>  
>  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> -- 
> 2.26.1

Re: [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t

[Christopher M. Riedl]

On Fri Feb 12, 2021 at 3:21 PM CST, Daniel Axtens wrote:
> "Christopher M. Riedl" <cmr@codefail.de> writes:
>
> > Usually sigset_t is exactly 8B which is a "trivial" size and does not
> > warrant using __copy_from_user(). Use __get_user() directly in
> > anticipation of future work to remove the trivial size optimizations
> > from __copy_from_user(). Calling __get_user() also results in a small
> > boost to signal handling throughput here.
>
> Modulo the comments from Christophe, this looks good to me. It seems to
> do what it says on the tin.
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Do you know if this patch is responsible for the slightly increase in
> radix performance you observed in your cover letter? The rest of the
> series shouldn't really make things faster than the no-KUAP case...

No, this patch just results in a really small improvement overall.

I looked over this again and I think the reason for the speedup is that
my implementation of unsafe_copy_from_user() in this series calls
__copy_tofrom_user() directly avoiding a barrier_nospec(). This speeds
up all the unsafe_get_from_user() calls in this version.

Skipping the barrier_nospec() like that is incorrect, but Christophe
recently sent a patch which moves barrier_nospec() into the uaccess
allowance helpers. It looks like mpe has already accepted that patch:

https://lists.ozlabs.org/pipermail/linuxppc-dev/2021-February/223959.html

That means that my implementation of unsafe_copy_from_user() is _now_
correct _and_ faster. We do not need to call barrier_nospec() since the
precondition for the 'unsafe' routine is that we called one of the
helpers to open up a uaccess window earlier.

>
> Kind regards,
> Daniel
>
> >
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >  arch/powerpc/kernel/signal_64.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index 817b64e1e409..42fdc4a7ff72 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -97,6 +97,14 @@ static void prepare_setup_sigcontext(struct task_struct *tsk, int ctx_has_vsx_re
> >  #endif /* CONFIG_VSX */
> >  }
> >  
> > +static inline int get_user_sigset(sigset_t *dst, const sigset_t *src)
> > +{
> > +	if (sizeof(sigset_t) <= 8)
> > +		return __get_user(dst->sig[0], &src->sig[0]);
> > +	else
> > +		return __copy_from_user(dst, src, sizeof(sigset_t));
> > +}
> > +
> >  /*
> >   * Set up the sigcontext for the signal frame.
> >   */
> > @@ -701,8 +709,9 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> >  	 * We kill the task with a SIGSEGV in this situation.
> >  	 */
> >  
> > -	if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
> > +	if (get_user_sigset(&set, &new_ctx->uc_sigmask))
> >  		do_exit(SIGSEGV);
> > +
> >  	set_current_blocked(&set);
> >  
> >  	if (!user_read_access_begin(new_ctx, ctx_size))
> > @@ -740,8 +749,9 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >  	if (!access_ok(uc, sizeof(*uc)))
> >  		goto badframe;
> >  
> > -	if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
> > +	if (get_user_sigset(&set, &uc->uc_sigmask))
> >  		goto badframe;
> > +
> >  	set_current_blocked(&set);
> >  
> >  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > -- 
> > 2.26.1

Re: [PATCH v5 10/10] powerpc/signal64: Use __get_user() to copy sigset_t

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4980 bytes --]

Hi "Christopher,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.11 next-20210215]
[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]

url:    https://github.com/0day-ci/linux/commits/Christopher-M-Riedl/Improve-signal-performance-on-PPC64-with-KUAP/20210204-025741
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc64-randconfig-s031-20210215 (attached as .config)
compiler: powerpc64le-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-215-g0fb77bb6-dirty
        # https://github.com/0day-ci/linux/commit/c6e40a98c1ba2f91d123ef23844732b97ea96fb3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christopher-M-Riedl/Improve-signal-performance-on-PPC64-with-KUAP/20210204-025741
        git checkout c6e40a98c1ba2f91d123ef23844732b97ea96fb3
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=powerpc64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> arch/powerpc/kernel/signal_64.c:752:36: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected struct sigset_t const [usertype] *src @@     got struct sigset_t [noderef] __user * @@
   arch/powerpc/kernel/signal_64.c:752:36: sparse:     expected struct sigset_t const [usertype] *src
   arch/powerpc/kernel/signal_64.c:752:36: sparse:     got struct sigset_t [noderef] __user *
   arch/powerpc/kernel/signal_64.c:712:36: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected struct sigset_t const [usertype] *src @@     got struct sigset_t [noderef] __user * @@
   arch/powerpc/kernel/signal_64.c:712:36: sparse:     expected struct sigset_t const [usertype] *src
   arch/powerpc/kernel/signal_64.c:712:36: sparse:     got struct sigset_t [noderef] __user *
>> arch/powerpc/kernel/signal_64.c:103:24: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected unsigned long const [noderef] __user *__gu_addr @@     got unsigned long const * @@
   arch/powerpc/kernel/signal_64.c:103:24: sparse:     expected unsigned long const [noderef] __user *__gu_addr
   arch/powerpc/kernel/signal_64.c:103:24: sparse:     got unsigned long const *
>> arch/powerpc/kernel/signal_64.c:105:46: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got struct sigset_t const [usertype] *src @@
   arch/powerpc/kernel/signal_64.c:105:46: sparse:     expected void const [noderef] __user *from
   arch/powerpc/kernel/signal_64.c:105:46: sparse:     got struct sigset_t const [usertype] *src
>> arch/powerpc/kernel/signal_64.c:103:24: sparse: sparse: incorrect type in initializer (different address spaces) @@     expected unsigned long const [noderef] __user *__gu_addr @@     got unsigned long const * @@
   arch/powerpc/kernel/signal_64.c:103:24: sparse:     expected unsigned long const [noderef] __user *__gu_addr
   arch/powerpc/kernel/signal_64.c:103:24: sparse:     got unsigned long const *
>> arch/powerpc/kernel/signal_64.c:105:46: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const [noderef] __user *from @@     got struct sigset_t const [usertype] *src @@
   arch/powerpc/kernel/signal_64.c:105:46: sparse:     expected void const [noderef] __user *from
   arch/powerpc/kernel/signal_64.c:105:46: sparse:     got struct sigset_t const [usertype] *src

vim +752 arch/powerpc/kernel/signal_64.c

   733	
   734	
   735	/*
   736	 * Do a signal return; undo the signal stack.
   737	 */
   738	
   739	SYSCALL_DEFINE0(rt_sigreturn)
   740	{
   741		struct pt_regs *regs = current_pt_regs();
   742		struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1];
   743		sigset_t set;
   744		unsigned long msr;
   745	
   746		/* Always make any pending restarted system calls return -EINTR */
   747		current->restart_block.fn = do_no_restart_syscall;
   748	
   749		if (!access_ok(uc, sizeof(*uc)))
   750			goto badframe;
   751	
 > 752		if (get_user_sigset(&set, &uc->uc_sigmask))
   753			goto badframe;
   754	
   755		set_current_blocked(&set);
   756	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 30676 bytes --]