[PATCH 0/8] Improve signal performance on PPC64 with KUAP

[PATCH 0/8] 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 Christopher 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 systems.

This series applies on top of Christophe Leroy's work for PPC32 [1] (I'm
sure patchwork won't be too happy about that).

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 adds 'notrace'
to any functions expected to be called in a uaccess block context.
Normally functions called in such a context should be inlined, but this
is not feasible everywhere. Marking them 'notrace' should provide _some_
protection against leaving the user access window open.

The next three patches rewrite some of the signal64 helper functions to
be 'unsafe'. Finally, the last two 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 a P9 Blackbird system. Note
that currently hash does not support KUAP and is therefore used as the
"baseline" comparison. Bigger numbers are better:

	signal1_threads -t1 -s10

	|                 | hash   | radix  |
	| --------------- | ------ | ------ |
	| linuxppc/next   | 289014 | 158408 |
	| unsafe-signal64 | 298506 | 253053 |

[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

Christopher M. Riedl (5):
  powerpc/uaccess: Add unsafe_copy_from_user
  powerpc/signal: Add unsafe_copy_{vsx,fpr}_from_user()
  powerpc: Mark functions called inside uaccess blocks w/ 'notrace'
  powerpc/signal64: Replace setup_sigcontext() w/
    unsafe_setup_sigcontext()
  powerpc/signal64: Replace restore_sigcontext() w/
    unsafe_restore_sigcontext()

Daniel Axtens (3):
  powerpc/signal64: Replace setup_trampoline() w/
    unsafe_setup_trampoline()
  powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess
    switches
  powerpc/signal64: Rewrite rt_sigreturn() to minimise uaccess switches

 arch/powerpc/include/asm/uaccess.h |  28 ++--
 arch/powerpc/kernel/process.c      |  20 +--
 arch/powerpc/kernel/signal.h       |  33 +++++
 arch/powerpc/kernel/signal_64.c    | 216 +++++++++++++++++------------
 arch/powerpc/mm/mem.c              |   4 +-
 5 files changed, 194 insertions(+), 107 deletions(-)

-- 
2.28.0

[PATCH 1/8] powerpc/uaccess: Add unsafe_copy_from_user

[Christopher M. Riedl]

Implement raw_copy_from_user_allowed() which assumes that userspace read
access is open. Use this new function to implement raw_copy_from_user().
Finally, wrap the new function to follow the usual "unsafe_" convention
of taking a label argument. The new raw_copy_from_user_allowed() calls
__copy_tofrom_user() internally, but this is still safe to call in user
access blocks formed with user_*_access_begin()/user_*_access_end()
since asm functions are not instrumented for tracing.

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

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 26781b044932..66940b4eb692 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -418,38 +418,45 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
 }
 #endif /* __powerpc64__ */
 
-static inline unsigned long raw_copy_from_user(void *to,
-		const void __user *from, unsigned long n)
+static inline unsigned long
+raw_copy_from_user_allowed(void *to, const void __user *from, unsigned long n)
 {
-	unsigned long ret;
 	if (__builtin_constant_p(n) && (n <= 8)) {
-		ret = 1;
+		unsigned long ret = 1;
 
 		switch (n) {
 		case 1:
 			barrier_nospec();
-			__get_user_size(*(u8 *)to, from, 1, ret);
+			__get_user_size_allowed(*(u8 *)to, from, 1, ret);
 			break;
 		case 2:
 			barrier_nospec();
-			__get_user_size(*(u16 *)to, from, 2, ret);
+			__get_user_size_allowed(*(u16 *)to, from, 2, ret);
 			break;
 		case 4:
 			barrier_nospec();
-			__get_user_size(*(u32 *)to, from, 4, ret);
+			__get_user_size_allowed(*(u32 *)to, from, 4, ret);
 			break;
 		case 8:
 			barrier_nospec();
-			__get_user_size(*(u64 *)to, from, 8, ret);
+			__get_user_size_allowed(*(u64 *)to, from, 8, ret);
 			break;
 		}
 		if (ret == 0)
 			return 0;
 	}
 
+	return __copy_tofrom_user((__force void __user *)to, from, n);
+}
+
+static inline unsigned long
+raw_copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	unsigned long ret;
+
 	barrier_nospec();
 	allow_read_from_user(from, n);
-	ret = __copy_tofrom_user((__force void __user *)to, from, n);
+	ret = raw_copy_from_user_allowed(to, from, n);
 	prevent_read_from_user(from, n);
 	return ret;
 }
@@ -571,6 +578,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(raw_copy_from_user_allowed(d, s, l), e)
+
 #define unsafe_copy_to_user(d, s, l, e) \
 do {									\
 	u8 __user *_dst = (u8 __user *)(d);				\
-- 
2.28.0

[PATCH 2/8] 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. Unlike the
unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
cannot use unsafe_get_user() directly to bypass the local buffer since
doing so significantly reduces signal handling performance.

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

diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index 2559a681536e..e9aaeac0da37 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -53,6 +53,33 @@ 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, ELF_NFPREG * sizeof(double),	\
+				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,					\
+				ELF_NVSRHALFREG * sizeof(double),	\
+				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 +107,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 +146,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.28.0

[PATCH 3/8] powerpc: Mark functions called inside uaccess blocks w/ 'notrace'

[Christopher M. Riedl]

Functions called between user_*_access_begin() and user_*_access_end()
should be either inlined or marked 'notrace' to prevent leaving
userspace access exposed. Mark any such functions relevant to signal
handling so that subsequent patches can call them inside uaccess blocks.

Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/process.c | 20 ++++++++++----------
 arch/powerpc/mm/mem.c         |  4 ++--
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index ba2c987b8403..bf5d9654bd2c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -84,7 +84,7 @@ extern unsigned long _get_SP(void);
  */
 bool tm_suspend_disabled __ro_after_init = false;
 
-static void check_if_tm_restore_required(struct task_struct *tsk)
+static void notrace check_if_tm_restore_required(struct task_struct *tsk)
 {
 	/*
 	 * If we are saving the current thread's registers, and the
@@ -151,7 +151,7 @@ void notrace __msr_check_and_clear(unsigned long bits)
 EXPORT_SYMBOL(__msr_check_and_clear);
 
 #ifdef CONFIG_PPC_FPU
-static void __giveup_fpu(struct task_struct *tsk)
+static void notrace __giveup_fpu(struct task_struct *tsk)
 {
 	unsigned long msr;
 
@@ -163,7 +163,7 @@ static void __giveup_fpu(struct task_struct *tsk)
 	tsk->thread.regs->msr = msr;
 }
 
-void giveup_fpu(struct task_struct *tsk)
+void notrace giveup_fpu(struct task_struct *tsk)
 {
 	check_if_tm_restore_required(tsk);
 
@@ -177,7 +177,7 @@ EXPORT_SYMBOL(giveup_fpu);
  * Make sure the floating-point register state in the
  * the thread_struct is up to date for task tsk.
  */
-void flush_fp_to_thread(struct task_struct *tsk)
+void notrace flush_fp_to_thread(struct task_struct *tsk)
 {
 	if (tsk->thread.regs) {
 		/*
@@ -234,7 +234,7 @@ static inline void __giveup_fpu(struct task_struct *tsk) { }
 #endif /* CONFIG_PPC_FPU */
 
 #ifdef CONFIG_ALTIVEC
-static void __giveup_altivec(struct task_struct *tsk)
+static void notrace __giveup_altivec(struct task_struct *tsk)
 {
 	unsigned long msr;
 
@@ -246,7 +246,7 @@ static void __giveup_altivec(struct task_struct *tsk)
 	tsk->thread.regs->msr = msr;
 }
 
-void giveup_altivec(struct task_struct *tsk)
+void notrace giveup_altivec(struct task_struct *tsk)
 {
 	check_if_tm_restore_required(tsk);
 
@@ -285,7 +285,7 @@ EXPORT_SYMBOL(enable_kernel_altivec);
  * Make sure the VMX/Altivec register state in the
  * the thread_struct is up to date for task tsk.
  */
-void flush_altivec_to_thread(struct task_struct *tsk)
+void notrace flush_altivec_to_thread(struct task_struct *tsk)
 {
 	if (tsk->thread.regs) {
 		preempt_disable();
@@ -300,7 +300,7 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread);
 #endif /* CONFIG_ALTIVEC */
 
 #ifdef CONFIG_VSX
-static void __giveup_vsx(struct task_struct *tsk)
+static void notrace __giveup_vsx(struct task_struct *tsk)
 {
 	unsigned long msr = tsk->thread.regs->msr;
 
@@ -317,7 +317,7 @@ static void __giveup_vsx(struct task_struct *tsk)
 		__giveup_altivec(tsk);
 }
 
-static void giveup_vsx(struct task_struct *tsk)
+static void notrace giveup_vsx(struct task_struct *tsk)
 {
 	check_if_tm_restore_required(tsk);
 
@@ -352,7 +352,7 @@ void enable_kernel_vsx(void)
 }
 EXPORT_SYMBOL(enable_kernel_vsx);
 
-void flush_vsx_to_thread(struct task_struct *tsk)
+void notrace flush_vsx_to_thread(struct task_struct *tsk)
 {
 	if (tsk->thread.regs) {
 		preempt_disable();
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index ddc32cc1b6cf..da2345a2abc6 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -378,7 +378,7 @@ static inline bool flush_coherent_icache(unsigned long addr)
  * @start: the start address
  * @stop: the stop address (exclusive)
  */
-static void invalidate_icache_range(unsigned long start, unsigned long stop)
+static void notrace invalidate_icache_range(unsigned long start, unsigned long stop)
 {
 	unsigned long shift = l1_icache_shift();
 	unsigned long bytes = l1_icache_bytes();
@@ -402,7 +402,7 @@ static void invalidate_icache_range(unsigned long start, unsigned long stop)
  * @start: the start address
  * @stop: the stop address (exclusive)
  */
-void flush_icache_range(unsigned long start, unsigned long stop)
+void notrace flush_icache_range(unsigned long start, unsigned long stop)
 {
 	if (flush_coherent_icache(start))
 		return;
-- 
2.28.0

[PATCH 4/8] 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 | 71 ++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 7df088b9ad0f..26934ceeb925 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -83,9 +83,13 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
  * 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
@@ -101,21 +105,20 @@ 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) {
 		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));
+		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.
 		 */
@@ -130,13 +133,13 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 		tsk->thread.vrsave = vrsave;
 	}
 
-	err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
+	unsafe_put_user(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 */
 	flush_fp_to_thread(tsk);
 	/* 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
@@ -152,24 +155,27 @@ static long setup_sigcontext(struct sigcontext __user *sc,
 	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);
+		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
@@ -655,12 +661,15 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
 		ctx_has_vsx_region = 1;
 
 	if (old_ctx != NULL) {
-		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;
@@ -689,6 +698,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;
 }
 
 
@@ -842,9 +855,13 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 #endif
 	{
 		err |= __put_user(0, &frame->uc.uc_link);
-		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.28.0

[PATCH 5/8] 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 26934ceeb925..bd92064e5576 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -318,14 +318,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;
@@ -340,27 +340,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.
@@ -370,29 +371,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
@@ -401,14 +401,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
@@ -692,8 +695,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);
@@ -798,8 +807,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.28.0

[PATCH 6/8] powerpc/signal64: Replace setup_trampoline() w/ unsafe_setup_trampoline()

[Christopher M. Riedl]

From: Daniel Axtens <dja@axtens.net>

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

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

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

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index bd92064e5576..6d4f7a5c4fbf 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -600,30 +600,33 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
 /*
  * Setup the trampoline code on the stack
  */
-static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp)
+#define unsafe_setup_trampoline(syscall, tramp, e) \
+	unsafe_op_wrap(__unsafe_setup_trampoline(syscall, tramp), e)
+static long notrace __unsafe_setup_trampoline(unsigned int syscall,
+					unsigned int __user *tramp)
 {
 	int i;
-	long err = 0;
 
 	/* bctrl # call the handler */
-	err |= __put_user(PPC_INST_BCTRL, &tramp[0]);
+	unsafe_put_user(PPC_INST_BCTRL, &tramp[0], err);
 	/* addi r1, r1, __SIGNAL_FRAMESIZE  # Pop the dummy stackframe */
-	err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
-			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]);
+	unsafe_put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
+			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1], err);
 	/* li r0, __NR_[rt_]sigreturn| */
-	err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]);
+	unsafe_put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2], err);
 	/* sc */
-	err |= __put_user(PPC_INST_SC, &tramp[3]);
+	unsafe_put_user(PPC_INST_SC, &tramp[3], err);
 
 	/* Minimal traceback info */
 	for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++)
-		err |= __put_user(0, &tramp[i]);
+		unsafe_put_user(0, &tramp[i], err);
 
-	if (!err)
-		flush_icache_range((unsigned long) &tramp[0],
-			   (unsigned long) &tramp[TRAMP_SIZE]);
+	flush_icache_range((unsigned long)&tramp[0],
+			   (unsigned long)&tramp[TRAMP_SIZE]);
 
-	return err;
+	return 0;
+err:
+	return 1;
 }
 
 /*
@@ -888,7 +891,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) {
 		regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
 	} else {
-		err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
+		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
+			return -EFAULT;
+		err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
+		user_write_access_end();
 		if (err)
 			goto badframe;
 		regs->nip = (unsigned long) &frame->tramp[0];
-- 
2.28.0

[PATCH 7/8] 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>
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 54 ++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 6d4f7a5c4fbf..3b97e3681a8f 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -843,46 +843,42 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	/* 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)))
+	if (!user_write_access_begin(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)
-		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);
+
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
 	if (MSR_TM_ACTIVE(msr)) {
 		/* 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;
+
 	} else
 #endif
 	{
-		err |= __put_user(0, &frame->uc.uc_link);
-
-		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);
 
 	/* Make sure signal handler doesn't get spurious FP exceptions */
 	tsk->thread.fp_state.fpscr = 0;
@@ -891,15 +887,17 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
 	if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) {
 		regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
 	} else {
-		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
-			return -EFAULT;
-		err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
-		user_write_access_end();
-		if (err)
-			goto badframe;
+		unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0],
+					badframe_block);
 		regs->nip = (unsigned long) &frame->tramp[0];
 	}
 
+	user_write_access_end();
+
+	/* 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);
@@ -939,6 +937,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.28.0

[PATCH 8/8] 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>
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
 arch/powerpc/kernel/signal_64.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 3b97e3681a8f..0f4ff7a5bfc1 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -779,18 +779,22 @@ SYSCALL_DEFINE0(rt_sigreturn)
 	 */
 	regs->msr &= ~MSR_TS_MASK;
 
-	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
+	if (!user_read_access_begin(uc, sizeof(*uc)))
 		goto badframe;
+
+	unsafe_get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR], badframe_block);
+
 	if (MSR_TM_ACTIVE(msr)) {
 		/* We recheckpoint on return. */
 		struct ucontext __user *uc_transact;
 
 		/* 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;
@@ -810,12 +814,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
 		 * causing a TM bad thing.
 		 */
 		current->thread.regs->msr &= ~MSR_TS_MASK;
+
+#ifndef CONFIG_PPC_TRANSACTIONAL_MEM
 		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;
-		}
+#endif
+		unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext,
+					  badframe_block);
 		user_read_access_end();
 	}
 
@@ -825,6 +830,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.28.0

Re: [PATCH 1/8] powerpc/uaccess: Add unsafe_copy_from_user

[Christoph Hellwig]

On Thu, Oct 15, 2020 at 10:01:52AM -0500, Christopher M. Riedl wrote:
> Implement raw_copy_from_user_allowed() which assumes that userspace read
> access is open. Use this new function to implement raw_copy_from_user().
> Finally, wrap the new function to follow the usual "unsafe_" convention
> of taking a label argument. The new raw_copy_from_user_allowed() calls
> __copy_tofrom_user() internally, but this is still safe to call in user
> access blocks formed with user_*_access_begin()/user_*_access_end()
> since asm functions are not instrumented for tracing.

Please also add a fallback unsafe_copy_from_user to linux/uaccess.h
so this can be used as a generic API.

Re: [PATCH 3/8] powerpc: Mark functions called inside uaccess blocks w/ 'notrace'

[Christoph Hellwig]

On Thu, Oct 15, 2020 at 10:01:54AM -0500, Christopher M. Riedl wrote:
> Functions called between user_*_access_begin() and user_*_access_end()
> should be either inlined or marked 'notrace' to prevent leaving
> userspace access exposed. Mark any such functions relevant to signal
> handling so that subsequent patches can call them inside uaccess blocks.

I don't think running this much code with uaccess enabled is a good
idea.  Please refactor the code to reduce the criticial sections with
uaccess enabled.

Btw, does powerpc already have the objtool validation that we don't
accidentally jump out of unsafe uaccess critical sections?

Re: [PATCH 3/8] powerpc: Mark functions called inside uaccess blocks w/ 'notrace'

[Christophe Leroy]

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> Functions called between user_*_access_begin() and user_*_access_end()
> should be either inlined or marked 'notrace' to prevent leaving
> userspace access exposed. Mark any such functions relevant to signal
> handling so that subsequent patches can call them inside uaccess blocks.

Is it enough to mark it "notrace" ? I see that when I activate KASAN, there are still KASAN calls in 
those functions.

In my series for 32 bits, I re-ordered stuff in order to do all those calls before doing the 
_access_begin(), can't you do the same on PPC64 ? (See 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/f6eac65781b4a57220477c8864bca2b57f29a5d5.1597770847.git.christophe.leroy@csgroup.eu/)

Christophe

> 
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>   arch/powerpc/kernel/process.c | 20 ++++++++++----------
>   arch/powerpc/mm/mem.c         |  4 ++--
>   2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index ba2c987b8403..bf5d9654bd2c 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -84,7 +84,7 @@ extern unsigned long _get_SP(void);
>    */
>   bool tm_suspend_disabled __ro_after_init = false;
>   
> -static void check_if_tm_restore_required(struct task_struct *tsk)
> +static void notrace check_if_tm_restore_required(struct task_struct *tsk)
>   {
>   	/*
>   	 * If we are saving the current thread's registers, and the
> @@ -151,7 +151,7 @@ void notrace __msr_check_and_clear(unsigned long bits)
>   EXPORT_SYMBOL(__msr_check_and_clear);
>   
>   #ifdef CONFIG_PPC_FPU
> -static void __giveup_fpu(struct task_struct *tsk)
> +static void notrace __giveup_fpu(struct task_struct *tsk)
>   {
>   	unsigned long msr;
>   
> @@ -163,7 +163,7 @@ static void __giveup_fpu(struct task_struct *tsk)
>   	tsk->thread.regs->msr = msr;
>   }
>   
> -void giveup_fpu(struct task_struct *tsk)
> +void notrace giveup_fpu(struct task_struct *tsk)
>   {
>   	check_if_tm_restore_required(tsk);
>   
> @@ -177,7 +177,7 @@ EXPORT_SYMBOL(giveup_fpu);
>    * Make sure the floating-point register state in the
>    * the thread_struct is up to date for task tsk.
>    */
> -void flush_fp_to_thread(struct task_struct *tsk)
> +void notrace flush_fp_to_thread(struct task_struct *tsk)
>   {
>   	if (tsk->thread.regs) {
>   		/*
> @@ -234,7 +234,7 @@ static inline void __giveup_fpu(struct task_struct *tsk) { }
>   #endif /* CONFIG_PPC_FPU */
>   
>   #ifdef CONFIG_ALTIVEC
> -static void __giveup_altivec(struct task_struct *tsk)
> +static void notrace __giveup_altivec(struct task_struct *tsk)
>   {
>   	unsigned long msr;
>   
> @@ -246,7 +246,7 @@ static void __giveup_altivec(struct task_struct *tsk)
>   	tsk->thread.regs->msr = msr;
>   }
>   
> -void giveup_altivec(struct task_struct *tsk)
> +void notrace giveup_altivec(struct task_struct *tsk)
>   {
>   	check_if_tm_restore_required(tsk);
>   
> @@ -285,7 +285,7 @@ EXPORT_SYMBOL(enable_kernel_altivec);
>    * Make sure the VMX/Altivec register state in the
>    * the thread_struct is up to date for task tsk.
>    */
> -void flush_altivec_to_thread(struct task_struct *tsk)
> +void notrace flush_altivec_to_thread(struct task_struct *tsk)
>   {
>   	if (tsk->thread.regs) {
>   		preempt_disable();
> @@ -300,7 +300,7 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread);
>   #endif /* CONFIG_ALTIVEC */
>   
>   #ifdef CONFIG_VSX
> -static void __giveup_vsx(struct task_struct *tsk)
> +static void notrace __giveup_vsx(struct task_struct *tsk)
>   {
>   	unsigned long msr = tsk->thread.regs->msr;
>   
> @@ -317,7 +317,7 @@ static void __giveup_vsx(struct task_struct *tsk)
>   		__giveup_altivec(tsk);
>   }
>   
> -static void giveup_vsx(struct task_struct *tsk)
> +static void notrace giveup_vsx(struct task_struct *tsk)
>   {
>   	check_if_tm_restore_required(tsk);
>   
> @@ -352,7 +352,7 @@ void enable_kernel_vsx(void)
>   }
>   EXPORT_SYMBOL(enable_kernel_vsx);
>   
> -void flush_vsx_to_thread(struct task_struct *tsk)
> +void notrace flush_vsx_to_thread(struct task_struct *tsk)
>   {
>   	if (tsk->thread.regs) {
>   		preempt_disable();
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index ddc32cc1b6cf..da2345a2abc6 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -378,7 +378,7 @@ static inline bool flush_coherent_icache(unsigned long addr)
>    * @start: the start address
>    * @stop: the stop address (exclusive)
>    */
> -static void invalidate_icache_range(unsigned long start, unsigned long stop)
> +static void notrace invalidate_icache_range(unsigned long start, unsigned long stop)
>   {
>   	unsigned long shift = l1_icache_shift();
>   	unsigned long bytes = l1_icache_bytes();
> @@ -402,7 +402,7 @@ static void invalidate_icache_range(unsigned long start, unsigned long stop)
>    * @start: the start address
>    * @stop: the stop address (exclusive)
>    */
> -void flush_icache_range(unsigned long start, unsigned long stop)
> +void notrace flush_icache_range(unsigned long start, unsigned long stop)
>   {
>   	if (flush_coherent_icache(start))
>   		return;
> 

Re: [PATCH 3/8] powerpc: Mark functions called inside uaccess blocks w/ 'notrace'

[Peter Zijlstra]

On Fri, Oct 16, 2020 at 07:56:16AM +0100, Christoph Hellwig wrote:
> On Thu, Oct 15, 2020 at 10:01:54AM -0500, Christopher M. Riedl wrote:
> > Functions called between user_*_access_begin() and user_*_access_end()
> > should be either inlined or marked 'notrace' to prevent leaving
> > userspace access exposed. Mark any such functions relevant to signal
> > handling so that subsequent patches can call them inside uaccess blocks.
> 
> I don't think running this much code with uaccess enabled is a good
> idea.  Please refactor the code to reduce the criticial sections with
> uaccess enabled.
> 
> Btw, does powerpc already have the objtool validation that we don't
> accidentally jump out of unsafe uaccess critical sections?

It does not, there was some effort on that a while ago, but I suspect
they're waiting for the ARM64 effort to land and build on that.

Re: [PATCH 1/8] powerpc/uaccess: Add unsafe_copy_from_user

[Christophe Leroy]

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> Implement raw_copy_from_user_allowed() which assumes that userspace read
> access is open. Use this new function to implement raw_copy_from_user().
> Finally, wrap the new function to follow the usual "unsafe_" convention
> of taking a label argument. The new raw_copy_from_user_allowed() calls
> __copy_tofrom_user() internally, but this is still safe to call in user
> access blocks formed with user_*_access_begin()/user_*_access_end()
> since asm functions are not instrumented for tracing.

Would objtool accept that if it was implemented on powerpc ?

__copy_tofrom_user() is a function which is optimised for larger memory copies (using dcbz, etc ...)
Do we need such an optimisation for unsafe_copy_from_user() ? Or can we do a simple loop as done for 
unsafe_copy_to_user() instead ?

Christophe

> 
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>   arch/powerpc/include/asm/uaccess.h | 28 +++++++++++++++++++---------
>   1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 26781b044932..66940b4eb692 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -418,38 +418,45 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
>   }
>   #endif /* __powerpc64__ */
>   
> -static inline unsigned long raw_copy_from_user(void *to,
> -		const void __user *from, unsigned long n)
> +static inline unsigned long
> +raw_copy_from_user_allowed(void *to, const void __user *from, unsigned long n)
>   {
> -	unsigned long ret;
>   	if (__builtin_constant_p(n) && (n <= 8)) {
> -		ret = 1;
> +		unsigned long ret = 1;
>   
>   		switch (n) {
>   		case 1:
>   			barrier_nospec();
> -			__get_user_size(*(u8 *)to, from, 1, ret);
> +			__get_user_size_allowed(*(u8 *)to, from, 1, ret);
>   			break;
>   		case 2:
>   			barrier_nospec();
> -			__get_user_size(*(u16 *)to, from, 2, ret);
> +			__get_user_size_allowed(*(u16 *)to, from, 2, ret);
>   			break;
>   		case 4:
>   			barrier_nospec();
> -			__get_user_size(*(u32 *)to, from, 4, ret);
> +			__get_user_size_allowed(*(u32 *)to, from, 4, ret);
>   			break;
>   		case 8:
>   			barrier_nospec();
> -			__get_user_size(*(u64 *)to, from, 8, ret);
> +			__get_user_size_allowed(*(u64 *)to, from, 8, ret);
>   			break;
>   		}
>   		if (ret == 0)
>   			return 0;
>   	}
>   
> +	return __copy_tofrom_user((__force void __user *)to, from, n);
> +}
> +
> +static inline unsigned long
> +raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> +{
> +	unsigned long ret;
> +
>   	barrier_nospec();
>   	allow_read_from_user(from, n);
> -	ret = __copy_tofrom_user((__force void __user *)to, from, n);
> +	ret = raw_copy_from_user_allowed(to, from, n);
>   	prevent_read_from_user(from, n);
>   	return ret;
>   }
> @@ -571,6 +578,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(raw_copy_from_user_allowed(d, s, l), e)
> +
>   #define unsafe_copy_to_user(d, s, l, e) \
>   do {									\
>   	u8 __user *_dst = (u8 __user *)(d);				\
> 

Re: [PATCH 1/8] powerpc/uaccess: Add unsafe_copy_from_user

[Christophe Leroy]

Le 16/10/2020 à 08:54, Christoph Hellwig a écrit :
> On Thu, Oct 15, 2020 at 10:01:52AM -0500, Christopher M. Riedl wrote:
>> Implement raw_copy_from_user_allowed() which assumes that userspace read
>> access is open. Use this new function to implement raw_copy_from_user().
>> Finally, wrap the new function to follow the usual "unsafe_" convention
>> of taking a label argument. The new raw_copy_from_user_allowed() calls
>> __copy_tofrom_user() internally, but this is still safe to call in user
>> access blocks formed with user_*_access_begin()/user_*_access_end()
>> since asm functions are not instrumented for tracing.
> 
> Please also add a fallback unsafe_copy_from_user to linux/uaccess.h
> so this can be used as a generic API.
> 

I guess this can be done in a separate patch independant of that series ?

Christophe

Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()

[Christophe Leroy]

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> Reuse the "safe" implementation from signal.c except for calling
> unsafe_copy_from_user() to copy into a local buffer. Unlike the
> unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
> cannot use unsafe_get_user() directly to bypass the local buffer since
> doing so significantly reduces signal handling performance.

Why can't the functions use unsafe_get_user(), why does it significantly reduces signal handling 
performance ? How much significant ? I would expect that not going through an intermediate memory 
area would be more efficient

Christophe


> 
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>   arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> index 2559a681536e..e9aaeac0da37 100644
> --- a/arch/powerpc/kernel/signal.h
> +++ b/arch/powerpc/kernel/signal.h
> @@ -53,6 +53,33 @@ 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, ELF_NFPREG * sizeof(double),	\
> +				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,					\
> +				ELF_NVSRHALFREG * sizeof(double),	\
> +				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 +107,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 +146,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)
>   {
> 

Re: [PATCH 6/8] powerpc/signal64: Replace setup_trampoline() w/ unsafe_setup_trampoline()

[Christophe Leroy]

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> From: Daniel Axtens <dja@axtens.net>
> 
> Previously setup_trampoline() performed a costly KUAP switch on every
> uaccess operation. These repeated uaccess switches cause a significant
> drop in signal handling performance.
> 
> Rewrite setup_trampoline() to assume that a userspace write access
> window is open. Replace all uaccess functions with their 'unsafe'
> versions to avoid the repeated uaccess switches.
> 
> Signed-off-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>   arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++-------------
>   1 file changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index bd92064e5576..6d4f7a5c4fbf 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -600,30 +600,33 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
>   /*
>    * Setup the trampoline code on the stack
>    */
> -static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp)
> +#define unsafe_setup_trampoline(syscall, tramp, e) \
> +	unsafe_op_wrap(__unsafe_setup_trampoline(syscall, tramp), e)
> +static long notrace __unsafe_setup_trampoline(unsigned int syscall,
> +					unsigned int __user *tramp)
>   {
>   	int i;
> -	long err = 0;
>   
>   	/* bctrl # call the handler */
> -	err |= __put_user(PPC_INST_BCTRL, &tramp[0]);
> +	unsafe_put_user(PPC_INST_BCTRL, &tramp[0], err);
>   	/* addi r1, r1, __SIGNAL_FRAMESIZE  # Pop the dummy stackframe */
> -	err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
> -			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]);
> +	unsafe_put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
> +			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1], err);
>   	/* li r0, __NR_[rt_]sigreturn| */
> -	err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]);
> +	unsafe_put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2], err);
>   	/* sc */
> -	err |= __put_user(PPC_INST_SC, &tramp[3]);
> +	unsafe_put_user(PPC_INST_SC, &tramp[3], err);
>   
>   	/* Minimal traceback info */
>   	for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++)
> -		err |= __put_user(0, &tramp[i]);
> +		unsafe_put_user(0, &tramp[i], err);
>   
> -	if (!err)
> -		flush_icache_range((unsigned long) &tramp[0],
> -			   (unsigned long) &tramp[TRAMP_SIZE]);
> +	flush_icache_range((unsigned long)&tramp[0],
> +			   (unsigned long)&tramp[TRAMP_SIZE]);

This flush should be done outside the user_write_access block.

>   
> -	return err;
> +	return 0;
> +err:
> +	return 1;
>   }
>   
>   /*
> @@ -888,7 +891,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   	if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) {
>   		regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
>   	} else {
> -		err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
> +		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
> +			return -EFAULT;
> +		err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
> +		user_write_access_end();
>   		if (err)
>   			goto badframe;
>   		regs->nip = (unsigned long) &frame->tramp[0];
> 

Christophe

Re: [PATCH 7/8] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches

[Christophe Leroy]

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> 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>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>   arch/powerpc/kernel/signal_64.c | 54 ++++++++++++++++-----------------
>   1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 6d4f7a5c4fbf..3b97e3681a8f 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -843,46 +843,42 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   	/* 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)))
> +	if (!user_write_access_begin(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)
> -		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);
> +
>   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>   	if (MSR_TM_ACTIVE(msr)) {
>   		/* 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();

Whaou. Doing this inside an #ifdef sequence is dirty.
Can you reorganise code to avoid that and to avoid nesting #ifdef/#endif and the if/else as I did in 
signal32 ?

>   		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;
> +
>   	} else
>   #endif
>   	{
> -		err |= __put_user(0, &frame->uc.uc_link);
> -
> -		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);
>   
>   	/* Make sure signal handler doesn't get spurious FP exceptions */
>   	tsk->thread.fp_state.fpscr = 0;
> @@ -891,15 +887,17 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>   	if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) {
>   		regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
>   	} else {
> -		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
> -			return -EFAULT;
> -		err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
> -		user_write_access_end();
> -		if (err)
> -			goto badframe;
> +		unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0],
> +					badframe_block);
>   		regs->nip = (unsigned long) &frame->tramp[0];
>   	}
>   
> +	user_write_access_end();
> +
> +	/* 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);
> @@ -939,6 +937,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);
>   
> 


Christophe

Re: [PATCH 8/8] powerpc/signal64: Rewrite rt_sigreturn() to minimise uaccess switches

[Christophe Leroy]

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> 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>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> ---
>   arch/powerpc/kernel/signal_64.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 3b97e3681a8f..0f4ff7a5bfc1 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -779,18 +779,22 @@ SYSCALL_DEFINE0(rt_sigreturn)
>   	 */
>   	regs->msr &= ~MSR_TS_MASK;
>   
> -	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> +	if (!user_read_access_begin(uc, sizeof(*uc)))
>   		goto badframe;
> +
> +	unsafe_get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR], badframe_block);
> +
>   	if (MSR_TM_ACTIVE(msr)) {
>   		/* We recheckpoint on return. */
>   		struct ucontext __user *uc_transact;
>   
>   		/* 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();

user_access_end() only in the if branch ?

>   
> -		if (__get_user(uc_transact, &uc->uc_link))
> -			goto badframe;
>   		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
>   					   &uc_transact->uc_mcontext))
>   			goto badframe;
> @@ -810,12 +814,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
>   		 * causing a TM bad thing.
>   		 */
>   		current->thread.regs->msr &= ~MSR_TS_MASK;
> +
> +#ifndef CONFIG_PPC_TRANSACTIONAL_MEM
>   		if (!user_read_access_begin(uc, sizeof(*uc)))

The matching user_read_access_end() is not in the same #ifndef ? That's dirty and hard to follow. 
Can you re-organise the code to avoid all those nesting ?

> -			return -EFAULT;
> -		if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
> -			user_read_access_end();
>   			goto badframe;
> -		}
> +#endif
> +		unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext,
> +					  badframe_block);
>   		user_read_access_end();
>   	}
>   
> @@ -825,6 +830,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);
>   
> 

Christophe

Re: [PATCH 3/8] powerpc: Mark functions called inside uaccess blocks w/ 'notrace'

[Christopher M. Riedl]

On Fri Oct 16, 2020 at 4:02 AM CDT, Christophe Leroy wrote:
>
>
> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> > Functions called between user_*_access_begin() and user_*_access_end()
> > should be either inlined or marked 'notrace' to prevent leaving
> > userspace access exposed. Mark any such functions relevant to signal
> > handling so that subsequent patches can call them inside uaccess blocks.
>
> Is it enough to mark it "notrace" ? I see that when I activate KASAN,
> there are still KASAN calls in
> those functions.
>

Maybe not enough after all :(

> In my series for 32 bits, I re-ordered stuff in order to do all those
> calls before doing the
> _access_begin(), can't you do the same on PPC64 ? (See
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/f6eac65781b4a57220477c8864bca2b57f29a5d5.1597770847.git.christophe.leroy@csgroup.eu/)
>

Yes, I will give this another shot in the next spin.

> Christophe
>
> > 
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >   arch/powerpc/kernel/process.c | 20 ++++++++++----------
> >   arch/powerpc/mm/mem.c         |  4 ++--
> >   2 files changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index ba2c987b8403..bf5d9654bd2c 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -84,7 +84,7 @@ extern unsigned long _get_SP(void);
> >    */
> >   bool tm_suspend_disabled __ro_after_init = false;
> >   
> > -static void check_if_tm_restore_required(struct task_struct *tsk)
> > +static void notrace check_if_tm_restore_required(struct task_struct *tsk)
> >   {
> >   	/*
> >   	 * If we are saving the current thread's registers, and the
> > @@ -151,7 +151,7 @@ void notrace __msr_check_and_clear(unsigned long bits)
> >   EXPORT_SYMBOL(__msr_check_and_clear);
> >   
> >   #ifdef CONFIG_PPC_FPU
> > -static void __giveup_fpu(struct task_struct *tsk)
> > +static void notrace __giveup_fpu(struct task_struct *tsk)
> >   {
> >   	unsigned long msr;
> >   
> > @@ -163,7 +163,7 @@ static void __giveup_fpu(struct task_struct *tsk)
> >   	tsk->thread.regs->msr = msr;
> >   }
> >   
> > -void giveup_fpu(struct task_struct *tsk)
> > +void notrace giveup_fpu(struct task_struct *tsk)
> >   {
> >   	check_if_tm_restore_required(tsk);
> >   
> > @@ -177,7 +177,7 @@ EXPORT_SYMBOL(giveup_fpu);
> >    * Make sure the floating-point register state in the
> >    * the thread_struct is up to date for task tsk.
> >    */
> > -void flush_fp_to_thread(struct task_struct *tsk)
> > +void notrace flush_fp_to_thread(struct task_struct *tsk)
> >   {
> >   	if (tsk->thread.regs) {
> >   		/*
> > @@ -234,7 +234,7 @@ static inline void __giveup_fpu(struct task_struct *tsk) { }
> >   #endif /* CONFIG_PPC_FPU */
> >   
> >   #ifdef CONFIG_ALTIVEC
> > -static void __giveup_altivec(struct task_struct *tsk)
> > +static void notrace __giveup_altivec(struct task_struct *tsk)
> >   {
> >   	unsigned long msr;
> >   
> > @@ -246,7 +246,7 @@ static void __giveup_altivec(struct task_struct *tsk)
> >   	tsk->thread.regs->msr = msr;
> >   }
> >   
> > -void giveup_altivec(struct task_struct *tsk)
> > +void notrace giveup_altivec(struct task_struct *tsk)
> >   {
> >   	check_if_tm_restore_required(tsk);
> >   
> > @@ -285,7 +285,7 @@ EXPORT_SYMBOL(enable_kernel_altivec);
> >    * Make sure the VMX/Altivec register state in the
> >    * the thread_struct is up to date for task tsk.
> >    */
> > -void flush_altivec_to_thread(struct task_struct *tsk)
> > +void notrace flush_altivec_to_thread(struct task_struct *tsk)
> >   {
> >   	if (tsk->thread.regs) {
> >   		preempt_disable();
> > @@ -300,7 +300,7 @@ EXPORT_SYMBOL_GPL(flush_altivec_to_thread);
> >   #endif /* CONFIG_ALTIVEC */
> >   
> >   #ifdef CONFIG_VSX
> > -static void __giveup_vsx(struct task_struct *tsk)
> > +static void notrace __giveup_vsx(struct task_struct *tsk)
> >   {
> >   	unsigned long msr = tsk->thread.regs->msr;
> >   
> > @@ -317,7 +317,7 @@ static void __giveup_vsx(struct task_struct *tsk)
> >   		__giveup_altivec(tsk);
> >   }
> >   
> > -static void giveup_vsx(struct task_struct *tsk)
> > +static void notrace giveup_vsx(struct task_struct *tsk)
> >   {
> >   	check_if_tm_restore_required(tsk);
> >   
> > @@ -352,7 +352,7 @@ void enable_kernel_vsx(void)
> >   }
> >   EXPORT_SYMBOL(enable_kernel_vsx);
> >   
> > -void flush_vsx_to_thread(struct task_struct *tsk)
> > +void notrace flush_vsx_to_thread(struct task_struct *tsk)
> >   {
> >   	if (tsk->thread.regs) {
> >   		preempt_disable();
> > diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> > index ddc32cc1b6cf..da2345a2abc6 100644
> > --- a/arch/powerpc/mm/mem.c
> > +++ b/arch/powerpc/mm/mem.c
> > @@ -378,7 +378,7 @@ static inline bool flush_coherent_icache(unsigned long addr)
> >    * @start: the start address
> >    * @stop: the stop address (exclusive)
> >    */
> > -static void invalidate_icache_range(unsigned long start, unsigned long stop)
> > +static void notrace invalidate_icache_range(unsigned long start, unsigned long stop)
> >   {
> >   	unsigned long shift = l1_icache_shift();
> >   	unsigned long bytes = l1_icache_bytes();
> > @@ -402,7 +402,7 @@ static void invalidate_icache_range(unsigned long start, unsigned long stop)
> >    * @start: the start address
> >    * @stop: the stop address (exclusive)
> >    */
> > -void flush_icache_range(unsigned long start, unsigned long stop)
> > +void notrace flush_icache_range(unsigned long start, unsigned long stop)
> >   {
> >   	if (flush_coherent_icache(start))
> >   		return;
> > 

Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx,fpr}_from_user()

[Christopher M. Riedl]

On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
>
>
> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> > Reuse the "safe" implementation from signal.c except for calling
> > unsafe_copy_from_user() to copy into a local buffer. Unlike the
> > unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
> > cannot use unsafe_get_user() directly to bypass the local buffer since
> > doing so significantly reduces signal handling performance.
>
> Why can't the functions use unsafe_get_user(), why does it significantly
> reduces signal handling
> performance ? How much significant ? I would expect that not going
> through an intermediate memory
> area would be more efficient
>

Here is a comparison, 'unsafe-signal64-regs' avoids the intermediate buffer:

	|                      | hash   | radix  |
	| -------------------- | ------ | ------ |
	| linuxppc/next        | 289014 | 158408 |
	| unsafe-signal64      | 298506 | 253053 |
	| unsafe-signal64-regs | 254898 | 220831 |

I have not figured out the 'why' yet. As you mentioned in your series,
technically calling __copy_tofrom_user() is overkill for these
operations. The only obvious difference between unsafe_put_user() and
unsafe_get_user() is that we don't have asm-goto for the 'get' variant.
Instead we wrap with unsafe_op_wrap() which inserts a conditional and
then goto to the label.

Implemenations:

	#define unsafe_copy_fpr_from_user(task, from, label)   do {            \
	       struct task_struct *__t = task;                                 \
	       u64 __user *buf = (u64 __user *)from;                           \
	       int i;                                                          \
									       \
	       for (i = 0; i < ELF_NFPREG - 1; i++)                            \
		       unsafe_get_user(__t->thread.TS_FPR(i), &buf[i], label); \
	       unsafe_get_user(__t->thread.fp_state.fpscr, &buf[i], label);    \
	} while (0)

	#define unsafe_copy_vsx_from_user(task, from, label)   do {            \
	       struct task_struct *__t = task;                                 \
	       u64 __user *buf = (u64 __user *)from;                           \
	       int i;                                                          \
									       \
	       for (i = 0; i < ELF_NVSRHALFREG ; i++)                          \
		       unsafe_get_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
				       &buf[i], label);                        \
	} while (0)

> Christophe
>
>
> > 
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >   arch/powerpc/kernel/signal.h | 33 +++++++++++++++++++++++++++++++++
> >   1 file changed, 33 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
> > index 2559a681536e..e9aaeac0da37 100644
> > --- a/arch/powerpc/kernel/signal.h
> > +++ b/arch/powerpc/kernel/signal.h
> > @@ -53,6 +53,33 @@ 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, ELF_NFPREG * sizeof(double),	\
> > +				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,					\
> > +				ELF_NVSRHALFREG * sizeof(double),	\
> > +				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 +107,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 +146,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)
> >   {
> > 

Re: [PATCH 6/8] powerpc/signal64: Replace setup_trampoline() w/ unsafe_setup_trampoline()

[Christopher M. Riedl]

On Fri Oct 16, 2020 at 10:56 AM CDT, Christophe Leroy wrote:
>
>
> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> > From: Daniel Axtens <dja@axtens.net>
> > 
> > Previously setup_trampoline() performed a costly KUAP switch on every
> > uaccess operation. These repeated uaccess switches cause a significant
> > drop in signal handling performance.
> > 
> > Rewrite setup_trampoline() to assume that a userspace write access
> > window is open. Replace all uaccess functions with their 'unsafe'
> > versions to avoid the repeated uaccess switches.
> > 
> > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >   arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++-------------
> >   1 file changed, 19 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index bd92064e5576..6d4f7a5c4fbf 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -600,30 +600,33 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
> >   /*
> >    * Setup the trampoline code on the stack
> >    */
> > -static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp)
> > +#define unsafe_setup_trampoline(syscall, tramp, e) \
> > +	unsafe_op_wrap(__unsafe_setup_trampoline(syscall, tramp), e)
> > +static long notrace __unsafe_setup_trampoline(unsigned int syscall,
> > +					unsigned int __user *tramp)
> >   {
> >   	int i;
> > -	long err = 0;
> >   
> >   	/* bctrl # call the handler */
> > -	err |= __put_user(PPC_INST_BCTRL, &tramp[0]);
> > +	unsafe_put_user(PPC_INST_BCTRL, &tramp[0], err);
> >   	/* addi r1, r1, __SIGNAL_FRAMESIZE  # Pop the dummy stackframe */
> > -	err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
> > -			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]);
> > +	unsafe_put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
> > +			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1], err);
> >   	/* li r0, __NR_[rt_]sigreturn| */
> > -	err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]);
> > +	unsafe_put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2], err);
> >   	/* sc */
> > -	err |= __put_user(PPC_INST_SC, &tramp[3]);
> > +	unsafe_put_user(PPC_INST_SC, &tramp[3], err);
> >   
> >   	/* Minimal traceback info */
> >   	for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++)
> > -		err |= __put_user(0, &tramp[i]);
> > +		unsafe_put_user(0, &tramp[i], err);
> >   
> > -	if (!err)
> > -		flush_icache_range((unsigned long) &tramp[0],
> > -			   (unsigned long) &tramp[TRAMP_SIZE]);
> > +	flush_icache_range((unsigned long)&tramp[0],
> > +			   (unsigned long)&tramp[TRAMP_SIZE]);
>
> This flush should be done outside the user_write_access block.
>

Hmm, I suppose that means setup_trampoline() cannot be completely
"unsafe". I'll see if I can re-arrange the code which calls this
function to avoid an additional uaccess block instead and push the
start()/end() into setup_trampoline() directly.

> >   
> > -	return err;
> > +	return 0;
> > +err:
> > +	return 1;
> >   }
> >   
> >   /*
> > @@ -888,7 +891,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >   	if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) {
> >   		regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
> >   	} else {
> > -		err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
> > +		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
> > +			return -EFAULT;
> > +		err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
> > +		user_write_access_end();
> >   		if (err)
> >   			goto badframe;
> >   		regs->nip = (unsigned long) &frame->tramp[0];
> > 
>
> Christophe

Re: [PATCH 7/8] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches

[Christopher M. Riedl]

On Fri Oct 16, 2020 at 11:00 AM CDT, Christophe Leroy wrote:
>
>
> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> > 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>
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >   arch/powerpc/kernel/signal_64.c | 54 ++++++++++++++++-----------------
> >   1 file changed, 27 insertions(+), 27 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index 6d4f7a5c4fbf..3b97e3681a8f 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -843,46 +843,42 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >   	/* 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)))
> > +	if (!user_write_access_begin(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)
> > -		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);
> > +
> >   #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> >   	if (MSR_TM_ACTIVE(msr)) {
> >   		/* 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();
>
> Whaou. Doing this inside an #ifdef sequence is dirty.
> Can you reorganise code to avoid that and to avoid nesting #ifdef/#endif
> and the if/else as I did in
> signal32 ?

Hopefully yes - next spin!

>
> >   		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;
> > +
> >   	} else
> >   #endif
> >   	{
> > -		err |= __put_user(0, &frame->uc.uc_link);
> > -
> > -		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);
> >   
> >   	/* Make sure signal handler doesn't get spurious FP exceptions */
> >   	tsk->thread.fp_state.fpscr = 0;
> > @@ -891,15 +887,17 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> >   	if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) {
> >   		regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
> >   	} else {
> > -		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
> > -			return -EFAULT;
> > -		err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
> > -		user_write_access_end();
> > -		if (err)
> > -			goto badframe;
> > +		unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0],
> > +					badframe_block);
> >   		regs->nip = (unsigned long) &frame->tramp[0];
> >   	}
> >   
> > +	user_write_access_end();
> > +
> > +	/* 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);
> > @@ -939,6 +937,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);
> >   
> > 
>
>
> Christophe

Re: [PATCH 8/8] powerpc/signal64: Rewrite rt_sigreturn() to minimise uaccess switches

[Christopher M. Riedl]

On Fri Oct 16, 2020 at 11:07 AM CDT, Christophe Leroy wrote:
>
>
> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> > 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>
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >   arch/powerpc/kernel/signal_64.c | 23 +++++++++++++++--------
> >   1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> > index 3b97e3681a8f..0f4ff7a5bfc1 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -779,18 +779,22 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >   	 */
> >   	regs->msr &= ~MSR_TS_MASK;
> >   
> > -	if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> > +	if (!user_read_access_begin(uc, sizeof(*uc)))
> >   		goto badframe;
> > +
> > +	unsafe_get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR], badframe_block);
> > +
> >   	if (MSR_TM_ACTIVE(msr)) {
> >   		/* We recheckpoint on return. */
> >   		struct ucontext __user *uc_transact;
> >   
> >   		/* 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();
>
> user_access_end() only in the if branch ?
>
> >   
> > -		if (__get_user(uc_transact, &uc->uc_link))
> > -			goto badframe;
> >   		if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
> >   					   &uc_transact->uc_mcontext))
> >   			goto badframe;
> > @@ -810,12 +814,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
> >   		 * causing a TM bad thing.
> >   		 */
> >   		current->thread.regs->msr &= ~MSR_TS_MASK;
> > +
> > +#ifndef CONFIG_PPC_TRANSACTIONAL_MEM
> >   		if (!user_read_access_begin(uc, sizeof(*uc)))
>
> The matching user_read_access_end() is not in the same #ifndef ? That's
> dirty and hard to follow.
> Can you re-organise the code to avoid all those nesting ?

Yes, thanks for pointing this out. I really wanted to avoid changing too
much of the logic inside these functions. But I suppose I ended up
creating a mess - I will fix this in the next spin.

>
> > -			return -EFAULT;
> > -		if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
> > -			user_read_access_end();
> >   			goto badframe;
> > -		}
> > +#endif
> > +		unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext,
> > +					  badframe_block);
> >   		user_read_access_end();
> >   	}
> >   
> > @@ -825,6 +830,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);
> >   
> > 
>
> Christophe

Re: [PATCH 1/8] powerpc/uaccess: Add unsafe_copy_from_user

[Christopher M. Riedl]

On Fri Oct 16, 2020 at 10:17 AM CDT, Christophe Leroy wrote:
>
>
> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> > Implement raw_copy_from_user_allowed() which assumes that userspace read
> > access is open. Use this new function to implement raw_copy_from_user().
> > Finally, wrap the new function to follow the usual "unsafe_" convention
> > of taking a label argument. The new raw_copy_from_user_allowed() calls
> > __copy_tofrom_user() internally, but this is still safe to call in user
> > access blocks formed with user_*_access_begin()/user_*_access_end()
> > since asm functions are not instrumented for tracing.
>
> Would objtool accept that if it was implemented on powerpc ?
>
> __copy_tofrom_user() is a function which is optimised for larger memory
> copies (using dcbz, etc ...)
> Do we need such an optimisation for unsafe_copy_from_user() ? Or can we
> do a simple loop as done for
> unsafe_copy_to_user() instead ?

I tried using a simple loop based on your unsafe_copy_to_user()
implementation. Similar to the copy_{vsx,fpr}_from_user() results there
is a hit to signal handling performance. The results with the loop are
in the 'unsafe-signal64-copy' column:

	|                      | hash   | radix  |
	| -------------------- | ------ | ------ |
	| linuxppc/next        | 289014 | 158408 |
	| unsafe-signal64      | 298506 | 253053 |
	| unsafe-signal64-copy | 197029 | 177002 |

Similar to the copy_{vsx,fpr}_from_user() patch I don't fully understand
why this performs so badly yet.

Implementation:

	unsafe_copy_from_user(d, s, l, e) \
	do {                                                                   \
	       u8 *_dst = (u8 *)(d);                                           \
	       const u8 __user *_src = (u8 __user*)(s);                                \
	       size_t _len = (l);                                              \
	       int _i;                                                         \
									       \
	       for (_i = 0; _i < (_len & ~(sizeof(long) - 1)); _i += sizeof(long))             \
		       unsafe_get_user(*(long*)(_dst + _i), (long __user *)(_src + _i), e);    \
	       if (IS_ENABLED(CONFIG_PPC64) && (_len & 4)) {                   \
		       unsafe_get_user(*(u32*)(_dst + _i), (u32 __user *)(_src + _i), e);      \
		       _i += 4;                                                \
	       }                                                               \
	       if (_len & 2) {                                                 \
		       unsafe_get_user(*(u16*)(_dst + _i), (u16 __user *)(_src + _i), e);      \
		       _i += 2;                                                \
	       }                                                               \
	       if (_len & 1)                                                   \
		       unsafe_get_user(*(u8*)(_dst + _i), (u8 __user *)(_src + _i), e);        \
	} while (0)

>
> Christophe
>
> > 
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > ---
> >   arch/powerpc/include/asm/uaccess.h | 28 +++++++++++++++++++---------
> >   1 file changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 26781b044932..66940b4eb692 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -418,38 +418,45 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
> >   }
> >   #endif /* __powerpc64__ */
> >   
> > -static inline unsigned long raw_copy_from_user(void *to,
> > -		const void __user *from, unsigned long n)
> > +static inline unsigned long
> > +raw_copy_from_user_allowed(void *to, const void __user *from, unsigned long n)
> >   {
> > -	unsigned long ret;
> >   	if (__builtin_constant_p(n) && (n <= 8)) {
> > -		ret = 1;
> > +		unsigned long ret = 1;
> >   
> >   		switch (n) {
> >   		case 1:
> >   			barrier_nospec();
> > -			__get_user_size(*(u8 *)to, from, 1, ret);
> > +			__get_user_size_allowed(*(u8 *)to, from, 1, ret);
> >   			break;
> >   		case 2:
> >   			barrier_nospec();
> > -			__get_user_size(*(u16 *)to, from, 2, ret);
> > +			__get_user_size_allowed(*(u16 *)to, from, 2, ret);
> >   			break;
> >   		case 4:
> >   			barrier_nospec();
> > -			__get_user_size(*(u32 *)to, from, 4, ret);
> > +			__get_user_size_allowed(*(u32 *)to, from, 4, ret);
> >   			break;
> >   		case 8:
> >   			barrier_nospec();
> > -			__get_user_size(*(u64 *)to, from, 8, ret);
> > +			__get_user_size_allowed(*(u64 *)to, from, 8, ret);
> >   			break;
> >   		}
> >   		if (ret == 0)
> >   			return 0;
> >   	}
> >   
> > +	return __copy_tofrom_user((__force void __user *)to, from, n);
> > +}
> > +
> > +static inline unsigned long
> > +raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> > +{
> > +	unsigned long ret;
> > +
> >   	barrier_nospec();
> >   	allow_read_from_user(from, n);
> > -	ret = __copy_tofrom_user((__force void __user *)to, from, n);
> > +	ret = raw_copy_from_user_allowed(to, from, n);
> >   	prevent_read_from_user(from, n);
> >   	return ret;
> >   }
> > @@ -571,6 +578,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(raw_copy_from_user_allowed(d, s, l), e)
> > +
> >   #define unsafe_copy_to_user(d, s, l, e) \
> >   do {									\
> >   	u8 __user *_dst = (u8 __user *)(d);				\
> > 

Re: [PATCH 6/8] powerpc/signal64: Replace setup_trampoline() w/ unsafe_setup_trampoline()

[Christophe Leroy]

Le 20/10/2020 à 04:42, Christopher M. Riedl a écrit :
> On Fri Oct 16, 2020 at 10:56 AM CDT, Christophe Leroy wrote:
>>
>>
>> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
>>> From: Daniel Axtens <dja@axtens.net>
>>>
>>> Previously setup_trampoline() performed a costly KUAP switch on every
>>> uaccess operation. These repeated uaccess switches cause a significant
>>> drop in signal handling performance.
>>>
>>> Rewrite setup_trampoline() to assume that a userspace write access
>>> window is open. Replace all uaccess functions with their 'unsafe'
>>> versions to avoid the repeated uaccess switches.
>>>
>>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>>> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
>>> ---
>>>    arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++-------------
>>>    1 file changed, 19 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
>>> index bd92064e5576..6d4f7a5c4fbf 100644
>>> --- a/arch/powerpc/kernel/signal_64.c
>>> +++ b/arch/powerpc/kernel/signal_64.c
>>> @@ -600,30 +600,33 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
>>>    /*
>>>     * Setup the trampoline code on the stack
>>>     */
>>> -static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp)
>>> +#define unsafe_setup_trampoline(syscall, tramp, e) \
>>> +	unsafe_op_wrap(__unsafe_setup_trampoline(syscall, tramp), e)
>>> +static long notrace __unsafe_setup_trampoline(unsigned int syscall,
>>> +					unsigned int __user *tramp)
>>>    {
>>>    	int i;
>>> -	long err = 0;
>>>    
>>>    	/* bctrl # call the handler */
>>> -	err |= __put_user(PPC_INST_BCTRL, &tramp[0]);
>>> +	unsafe_put_user(PPC_INST_BCTRL, &tramp[0], err);
>>>    	/* addi r1, r1, __SIGNAL_FRAMESIZE  # Pop the dummy stackframe */
>>> -	err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
>>> -			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]);
>>> +	unsafe_put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) |
>>> +			  (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1], err);
>>>    	/* li r0, __NR_[rt_]sigreturn| */
>>> -	err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]);
>>> +	unsafe_put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2], err);
>>>    	/* sc */
>>> -	err |= __put_user(PPC_INST_SC, &tramp[3]);
>>> +	unsafe_put_user(PPC_INST_SC, &tramp[3], err);
>>>    
>>>    	/* Minimal traceback info */
>>>    	for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++)
>>> -		err |= __put_user(0, &tramp[i]);
>>> +		unsafe_put_user(0, &tramp[i], err);
>>>    
>>> -	if (!err)
>>> -		flush_icache_range((unsigned long) &tramp[0],
>>> -			   (unsigned long) &tramp[TRAMP_SIZE]);
>>> +	flush_icache_range((unsigned long)&tramp[0],
>>> +			   (unsigned long)&tramp[TRAMP_SIZE]);
>>
>> This flush should be done outside the user_write_access block.
>>
> 
> Hmm, I suppose that means setup_trampoline() cannot be completely
> "unsafe". I'll see if I can re-arrange the code which calls this
> function to avoid an additional uaccess block instead and push the
> start()/end() into setup_trampoline() directly.

I think we shouldn't put too much effort on setup_trampoline(). Nowadays 99.999% of applications use 
the VDSO. Using the trampoline on stack requires to unmap the VDSO and remap the STACK RW. That's 
really a corner case, I think it would be good enough to call it outside the main access begin/end 
block, and let it do its own access_begin/end.

This corner functionnality can be tested using the sigreturn_vdso selftest in selftests/powerpc/signal/

Christophe

> 
>>>    
>>> -	return err;
>>> +	return 0;
>>> +err:
>>> +	return 1;
>>>    }
>>>    
>>>    /*
>>> @@ -888,7 +891,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
>>>    	if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) {
>>>    		regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp;
>>>    	} else {
>>> -		err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
>>> +		if (!user_write_access_begin(frame, sizeof(struct rt_sigframe)))
>>> +			return -EFAULT;
>>> +		err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]);
>>> +		user_write_access_end();
>>>    		if (err)
>>>    			goto badframe;
>>>    		regs->nip = (unsigned long) &frame->tramp[0];
>>>
>>
>> Christophe
> 

Re: [PATCH 3/8] powerpc: Mark functions called inside uaccess blocks w/ 'notrace'

[Michael Ellerman]

Peter Zijlstra <peterz@infradead.org> writes:
> On Fri, Oct 16, 2020 at 07:56:16AM +0100, Christoph Hellwig wrote:
>> On Thu, Oct 15, 2020 at 10:01:54AM -0500, Christopher M. Riedl wrote:
>> > Functions called between user_*_access_begin() and user_*_access_end()
>> > should be either inlined or marked 'notrace' to prevent leaving
>> > userspace access exposed. Mark any such functions relevant to signal
>> > handling so that subsequent patches can call them inside uaccess blocks.
>> 
>> I don't think running this much code with uaccess enabled is a good
>> idea.  Please refactor the code to reduce the criticial sections with
>> uaccess enabled.
>> 
>> Btw, does powerpc already have the objtool validation that we don't
>> accidentally jump out of unsafe uaccess critical sections?
>
> It does not, there was some effort on that a while ago, but I suspect
> they're waiting for the ARM64 effort to land and build on that.

Right, we don't have objtool support.

We would definitely like objtool support at least for this uaccess
checking, I'm sure we have some escapes.

There was someone working on it in their own-time but last I heard that
was still WIP.

I didn't realise the ARM64 support was still not merged, so yeah having
that land first would probably simplify things, but we still need
someone who has time to work on it.

cheers

Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()

[Christophe Leroy]

Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
> On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
>>
>>
>> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
>>> Reuse the "safe" implementation from signal.c except for calling
>>> unsafe_copy_from_user() to copy into a local buffer. Unlike the
>>> unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
>>> cannot use unsafe_get_user() directly to bypass the local buffer since
>>> doing so significantly reduces signal handling performance.
>>
>> Why can't the functions use unsafe_get_user(), why does it significantly
>> reduces signal handling
>> performance ? How much significant ? I would expect that not going
>> through an intermediate memory
>> area would be more efficient
>>
> 
> Here is a comparison, 'unsafe-signal64-regs' avoids the intermediate buffer:
> 
> 	|                      | hash   | radix  |
> 	| -------------------- | ------ | ------ |
> 	| linuxppc/next        | 289014 | 158408 |
> 	| unsafe-signal64      | 298506 | 253053 |
> 	| unsafe-signal64-regs | 254898 | 220831 |
> 
> I have not figured out the 'why' yet. As you mentioned in your series,
> technically calling __copy_tofrom_user() is overkill for these
> operations. The only obvious difference between unsafe_put_user() and
> unsafe_get_user() is that we don't have asm-goto for the 'get' variant.
> Instead we wrap with unsafe_op_wrap() which inserts a conditional and
> then goto to the label.
> 
> Implemenations:
> 
> 	#define unsafe_copy_fpr_from_user(task, from, label)   do {            \
> 	       struct task_struct *__t = task;                                 \
> 	       u64 __user *buf = (u64 __user *)from;                           \
> 	       int i;                                                          \
> 									       \
> 	       for (i = 0; i < ELF_NFPREG - 1; i++)                            \
> 		       unsafe_get_user(__t->thread.TS_FPR(i), &buf[i], label); \
> 	       unsafe_get_user(__t->thread.fp_state.fpscr, &buf[i], label);    \
> 	} while (0)
> 
> 	#define unsafe_copy_vsx_from_user(task, from, label)   do {            \
> 	       struct task_struct *__t = task;                                 \
> 	       u64 __user *buf = (u64 __user *)from;                           \
> 	       int i;                                                          \
> 									       \
> 	       for (i = 0; i < ELF_NVSRHALFREG ; i++)                          \
> 		       unsafe_get_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
> 				       &buf[i], label);                        \
> 	} while (0)
> 

Do you have CONFIG_PROVE_LOCKING or CONFIG_DEBUG_ATOMIC_SLEEP enabled in your config ?

If yes, could you try together with the patch from Alexey 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210204121612.32721-1-aik@ozlabs.ru/ ?

Thanks
Christophe

Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx,fpr}_from_user()

[Christopher M. Riedl]

On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:
>
>
> Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
> > On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
> >>
> >>
> >> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> >>> Reuse the "safe" implementation from signal.c except for calling
> >>> unsafe_copy_from_user() to copy into a local buffer. Unlike the
> >>> unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
> >>> cannot use unsafe_get_user() directly to bypass the local buffer since
> >>> doing so significantly reduces signal handling performance.
> >>
> >> Why can't the functions use unsafe_get_user(), why does it significantly
> >> reduces signal handling
> >> performance ? How much significant ? I would expect that not going
> >> through an intermediate memory
> >> area would be more efficient
> >>
> > 
> > Here is a comparison, 'unsafe-signal64-regs' avoids the intermediate buffer:
> > 
> > 	|                      | hash   | radix  |
> > 	| -------------------- | ------ | ------ |
> > 	| linuxppc/next        | 289014 | 158408 |
> > 	| unsafe-signal64      | 298506 | 253053 |
> > 	| unsafe-signal64-regs | 254898 | 220831 |
> > 
> > I have not figured out the 'why' yet. As you mentioned in your series,
> > technically calling __copy_tofrom_user() is overkill for these
> > operations. The only obvious difference between unsafe_put_user() and
> > unsafe_get_user() is that we don't have asm-goto for the 'get' variant.
> > Instead we wrap with unsafe_op_wrap() which inserts a conditional and
> > then goto to the label.
> > 
> > Implemenations:
> > 
> > 	#define unsafe_copy_fpr_from_user(task, from, label)   do {            \
> > 	       struct task_struct *__t = task;                                 \
> > 	       u64 __user *buf = (u64 __user *)from;                           \
> > 	       int i;                                                          \
> > 									       \
> > 	       for (i = 0; i < ELF_NFPREG - 1; i++)                            \
> > 		       unsafe_get_user(__t->thread.TS_FPR(i), &buf[i], label); \
> > 	       unsafe_get_user(__t->thread.fp_state.fpscr, &buf[i], label);    \
> > 	} while (0)
> > 
> > 	#define unsafe_copy_vsx_from_user(task, from, label)   do {            \
> > 	       struct task_struct *__t = task;                                 \
> > 	       u64 __user *buf = (u64 __user *)from;                           \
> > 	       int i;                                                          \
> > 									       \
> > 	       for (i = 0; i < ELF_NVSRHALFREG ; i++)                          \
> > 		       unsafe_get_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
> > 				       &buf[i], label);                        \
> > 	} while (0)
> > 
>
> Do you have CONFIG_PROVE_LOCKING or CONFIG_DEBUG_ATOMIC_SLEEP enabled in
> your config ?

I don't have these set in my config (ppc64le_defconfig). I think I
figured this out - the reason for the lower signal throughput is the
barrier_nospec() in __get_user_nocheck(). When looping we incur that
cost on every iteration. Commenting it out results in signal performance
of ~316K w/ hash on the unsafe-signal64-regs branch. Obviously the
barrier is there for a reason but it is quite costly.

This also explains why the copy_{fpr,vsx}_to_user() direction does not
suffer from the slowdown because there is no need for barrier_nospec().
>
> If yes, could you try together with the patch from Alexey
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20210204121612.32721-1-aik@ozlabs.ru/
> ?
>
> Thanks
> Christophe

Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()

[Christophe Leroy]

Le 06/02/2021 à 18:39, Christopher M. Riedl a écrit :
> On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:
>>
>>
>> Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
>>> On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
>>>>> Reuse the "safe" implementation from signal.c except for calling
>>>>> unsafe_copy_from_user() to copy into a local buffer. Unlike the
>>>>> unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
>>>>> cannot use unsafe_get_user() directly to bypass the local buffer since
>>>>> doing so significantly reduces signal handling performance.
>>>>
>>>> Why can't the functions use unsafe_get_user(), why does it significantly
>>>> reduces signal handling
>>>> performance ? How much significant ? I would expect that not going
>>>> through an intermediate memory
>>>> area would be more efficient
>>>>
>>>
>>> Here is a comparison, 'unsafe-signal64-regs' avoids the intermediate buffer:
>>>
>>> 	|                      | hash   | radix  |
>>> 	| -------------------- | ------ | ------ |
>>> 	| linuxppc/next        | 289014 | 158408 |
>>> 	| unsafe-signal64      | 298506 | 253053 |
>>> 	| unsafe-signal64-regs | 254898 | 220831 |
>>>
>>> I have not figured out the 'why' yet. As you mentioned in your series,
>>> technically calling __copy_tofrom_user() is overkill for these
>>> operations. The only obvious difference between unsafe_put_user() and
>>> unsafe_get_user() is that we don't have asm-goto for the 'get' variant.
>>> Instead we wrap with unsafe_op_wrap() which inserts a conditional and
>>> then goto to the label.
>>>
>>> Implemenations:
>>>
>>> 	#define unsafe_copy_fpr_from_user(task, from, label)   do {            \
>>> 	       struct task_struct *__t = task;                                 \
>>> 	       u64 __user *buf = (u64 __user *)from;                           \
>>> 	       int i;                                                          \
>>> 									       \
>>> 	       for (i = 0; i < ELF_NFPREG - 1; i++)                            \
>>> 		       unsafe_get_user(__t->thread.TS_FPR(i), &buf[i], label); \
>>> 	       unsafe_get_user(__t->thread.fp_state.fpscr, &buf[i], label);    \
>>> 	} while (0)
>>>
>>> 	#define unsafe_copy_vsx_from_user(task, from, label)   do {            \
>>> 	       struct task_struct *__t = task;                                 \
>>> 	       u64 __user *buf = (u64 __user *)from;                           \
>>> 	       int i;                                                          \
>>> 									       \
>>> 	       for (i = 0; i < ELF_NVSRHALFREG ; i++)                          \
>>> 		       unsafe_get_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
>>> 				       &buf[i], label);                        \
>>> 	} while (0)
>>>
>>
>> Do you have CONFIG_PROVE_LOCKING or CONFIG_DEBUG_ATOMIC_SLEEP enabled in
>> your config ?
> 
> I don't have these set in my config (ppc64le_defconfig). I think I
> figured this out - the reason for the lower signal throughput is the
> barrier_nospec() in __get_user_nocheck(). When looping we incur that
> cost on every iteration. Commenting it out results in signal performance
> of ~316K w/ hash on the unsafe-signal64-regs branch. Obviously the
> barrier is there for a reason but it is quite costly.

Interesting.

Can you try with the patch I just sent out 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/c72f014730823b413528e90ab6c4d3bcb79f8497.1612692067.git.christophe.leroy@csgroup.eu/

Thanks
Christophe

Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx,fpr}_from_user()

[Christopher M. Riedl]

On Sun Feb 7, 2021 at 4:12 AM CST, Christophe Leroy wrote:
>
>
> Le 06/02/2021 à 18:39, Christopher M. Riedl a écrit :
> > On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:
> >>
> >>
> >> Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
> >>> On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
> >>>>
> >>>>
> >>>> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> >>>>> Reuse the "safe" implementation from signal.c except for calling
> >>>>> unsafe_copy_from_user() to copy into a local buffer. Unlike the
> >>>>> unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
> >>>>> cannot use unsafe_get_user() directly to bypass the local buffer since
> >>>>> doing so significantly reduces signal handling performance.
> >>>>
> >>>> Why can't the functions use unsafe_get_user(), why does it significantly
> >>>> reduces signal handling
> >>>> performance ? How much significant ? I would expect that not going
> >>>> through an intermediate memory
> >>>> area would be more efficient
> >>>>
> >>>
> >>> Here is a comparison, 'unsafe-signal64-regs' avoids the intermediate buffer:
> >>>
> >>> 	|                      | hash   | radix  |
> >>> 	| -------------------- | ------ | ------ |
> >>> 	| linuxppc/next        | 289014 | 158408 |
> >>> 	| unsafe-signal64      | 298506 | 253053 |
> >>> 	| unsafe-signal64-regs | 254898 | 220831 |
> >>>
> >>> I have not figured out the 'why' yet. As you mentioned in your series,
> >>> technically calling __copy_tofrom_user() is overkill for these
> >>> operations. The only obvious difference between unsafe_put_user() and
> >>> unsafe_get_user() is that we don't have asm-goto for the 'get' variant.
> >>> Instead we wrap with unsafe_op_wrap() which inserts a conditional and
> >>> then goto to the label.
> >>>
> >>> Implemenations:
> >>>
> >>> 	#define unsafe_copy_fpr_from_user(task, from, label)   do {            \
> >>> 	       struct task_struct *__t = task;                                 \
> >>> 	       u64 __user *buf = (u64 __user *)from;                           \
> >>> 	       int i;                                                          \
> >>> 									       \
> >>> 	       for (i = 0; i < ELF_NFPREG - 1; i++)                            \
> >>> 		       unsafe_get_user(__t->thread.TS_FPR(i), &buf[i], label); \
> >>> 	       unsafe_get_user(__t->thread.fp_state.fpscr, &buf[i], label);    \
> >>> 	} while (0)
> >>>
> >>> 	#define unsafe_copy_vsx_from_user(task, from, label)   do {            \
> >>> 	       struct task_struct *__t = task;                                 \
> >>> 	       u64 __user *buf = (u64 __user *)from;                           \
> >>> 	       int i;                                                          \
> >>> 									       \
> >>> 	       for (i = 0; i < ELF_NVSRHALFREG ; i++)                          \
> >>> 		       unsafe_get_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
> >>> 				       &buf[i], label);                        \
> >>> 	} while (0)
> >>>
> >>
> >> Do you have CONFIG_PROVE_LOCKING or CONFIG_DEBUG_ATOMIC_SLEEP enabled in
> >> your config ?
> > 
> > I don't have these set in my config (ppc64le_defconfig). I think I
> > figured this out - the reason for the lower signal throughput is the
> > barrier_nospec() in __get_user_nocheck(). When looping we incur that
> > cost on every iteration. Commenting it out results in signal performance
> > of ~316K w/ hash on the unsafe-signal64-regs branch. Obviously the
> > barrier is there for a reason but it is quite costly.
>
> Interesting.
>
> Can you try with the patch I just sent out
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/c72f014730823b413528e90ab6c4d3bcb79f8497.1612692067.git.christophe.leroy@csgroup.eu/

Yeah that patch solves the problem. Using unsafe_get_user() in a loop is
actually faster on radix than using the intermediary buffer step. A
summary of results below (unsafe-signal64-v6 uses unsafe_get_user() and
avoids the local buffer):

        |                                  | hash   | radix  |
        | -------------------------------- | ------ | ------ |
        | unsafe-signal64-v5               | 194533 | 230089 |
        | unsafe-signal64-v6               | 176739 | 202840 |
        | unsafe-signal64-v5+barrier patch | 203037 | 234936 |
        | unsafe-signal64-v6+barrier patch | 205484 | 241030 |

I am still expecting some comments/feedback on my v5 before sending out
v6. Should I include your patch in my series as well?

>
> Thanks
> Christophe

Re: [PATCH 2/8] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()

[Christophe Leroy]

Le 08/02/2021 à 18:14, Christopher M. Riedl a écrit :
> On Sun Feb 7, 2021 at 4:12 AM CST, Christophe Leroy wrote:
>>
>>
>> Le 06/02/2021 à 18:39, Christopher M. Riedl a écrit :
>>> On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
>>>>> On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
>>>>>>
>>>>>>
>>>>>> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
>>>>>>> Reuse the "safe" implementation from signal.c except for calling
>>>>>>> unsafe_copy_from_user() to copy into a local buffer. Unlike the
>>>>>>> unsafe_copy_{vsx,fpr}_to_user() functions the "copy from" functions
>>>>>>> cannot use unsafe_get_user() directly to bypass the local buffer since
>>>>>>> doing so significantly reduces signal handling performance.
>>>>>>
>>>>>> Why can't the functions use unsafe_get_user(), why does it significantly
>>>>>> reduces signal handling
>>>>>> performance ? How much significant ? I would expect that not going
>>>>>> through an intermediate memory
>>>>>> area would be more efficient
>>>>>>
>>>>>
>>>>> Here is a comparison, 'unsafe-signal64-regs' avoids the intermediate buffer:
>>>>>
>>>>> 	|                      | hash   | radix  |
>>>>> 	| -------------------- | ------ | ------ |
>>>>> 	| linuxppc/next        | 289014 | 158408 |
>>>>> 	| unsafe-signal64      | 298506 | 253053 |
>>>>> 	| unsafe-signal64-regs | 254898 | 220831 |
>>>>>
>>>>> I have not figured out the 'why' yet. As you mentioned in your series,
>>>>> technically calling __copy_tofrom_user() is overkill for these
>>>>> operations. The only obvious difference between unsafe_put_user() and
>>>>> unsafe_get_user() is that we don't have asm-goto for the 'get' variant.
>>>>> Instead we wrap with unsafe_op_wrap() which inserts a conditional and
>>>>> then goto to the label.
>>>>>
>>>>> Implemenations:
>>>>>
>>>>> 	#define unsafe_copy_fpr_from_user(task, from, label)   do {            \
>>>>> 	       struct task_struct *__t = task;                                 \
>>>>> 	       u64 __user *buf = (u64 __user *)from;                           \
>>>>> 	       int i;                                                          \
>>>>> 									       \
>>>>> 	       for (i = 0; i < ELF_NFPREG - 1; i++)                            \
>>>>> 		       unsafe_get_user(__t->thread.TS_FPR(i), &buf[i], label); \
>>>>> 	       unsafe_get_user(__t->thread.fp_state.fpscr, &buf[i], label);    \
>>>>> 	} while (0)
>>>>>
>>>>> 	#define unsafe_copy_vsx_from_user(task, from, label)   do {            \
>>>>> 	       struct task_struct *__t = task;                                 \
>>>>> 	       u64 __user *buf = (u64 __user *)from;                           \
>>>>> 	       int i;                                                          \
>>>>> 									       \
>>>>> 	       for (i = 0; i < ELF_NVSRHALFREG ; i++)                          \
>>>>> 		       unsafe_get_user(__t->thread.fp_state.fpr[i][TS_VSRLOWOFFSET], \
>>>>> 				       &buf[i], label);                        \
>>>>> 	} while (0)
>>>>>
>>>>
>>>> Do you have CONFIG_PROVE_LOCKING or CONFIG_DEBUG_ATOMIC_SLEEP enabled in
>>>> your config ?
>>>
>>> I don't have these set in my config (ppc64le_defconfig). I think I
>>> figured this out - the reason for the lower signal throughput is the
>>> barrier_nospec() in __get_user_nocheck(). When looping we incur that
>>> cost on every iteration. Commenting it out results in signal performance
>>> of ~316K w/ hash on the unsafe-signal64-regs branch. Obviously the
>>> barrier is there for a reason but it is quite costly.
>>
>> Interesting.
>>
>> Can you try with the patch I just sent out
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/c72f014730823b413528e90ab6c4d3bcb79f8497.1612692067.git.christophe.leroy@csgroup.eu/
> 
> Yeah that patch solves the problem. Using unsafe_get_user() in a loop is
> actually faster on radix than using the intermediary buffer step. A
> summary of results below (unsafe-signal64-v6 uses unsafe_get_user() and
> avoids the local buffer):
> 
>          |                                  | hash   | radix  |
>          | -------------------------------- | ------ | ------ |
>          | unsafe-signal64-v5               | 194533 | 230089 |
>          | unsafe-signal64-v6               | 176739 | 202840 |
>          | unsafe-signal64-v5+barrier patch | 203037 | 234936 |
>          | unsafe-signal64-v6+barrier patch | 205484 | 241030 |
> 
> I am still expecting some comments/feedback on my v5 before sending out
> v6. Should I include your patch in my series as well?
> 

My patch is now flagged "under review" in patchwork so I suppose Michael picked it already.

Christophe