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

STALE2085d SUPERSEDED

Revision v1 of 7 in this series.

Revisions (7)
  1. v1 current
  2. v2 [diff vs current]
  3. v3 [diff vs current]
  4. v4 [diff vs current]
  5. v5 [diff vs current]
  6. v6 [diff vs current]
  7. v7 [diff vs current]

32 messages, 5 authors, 2021-02-08 · open the first message on its own page

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

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:22:54

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

29 more lines

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

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:24:56

From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
 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 5/8] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:26:49

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 <redacted>
---
 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 4/8] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext()

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:29:06

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 <redacted>
---
 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 2/8] powerpc/signal: Add unsafe_copy_{vsx,fpr}_from_user()

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:31:47

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 <redacted>
---
 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 8/8] powerpc/signal64: Rewrite rt_sigreturn() to minimise uaccess switches

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:39:34

From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
 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

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

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:43:26

From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
 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 1/8] powerpc/uaccess: Add unsafe_copy_from_user

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:52:51

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 <redacted>
---
 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 3/8] powerpc: Mark functions called inside uaccess blocks w/ 'notrace'

From: Christopher M. Riedl <hidden>
Date: 2020-10-15 15:57:28

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 <redacted>
---
 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

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

From: Christoph Hellwig <hch@infradead.org>
Date: 2020-10-16 06:54:22

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'

From: Christoph Hellwig <hch@infradead.org>
Date: 2020-10-16 06:56:21

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'

From: Christophe Leroy <hidden>
Date: 2020-10-16 07:04:12


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
quoted hunk
Signed-off-by: Christopher M. Riedl <redacted>
---
  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

84 more quoted lines

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

From: Peter Zijlstra <peterz@infradead.org>
Date: 2020-10-16 09:41:47

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:
quoted
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

From: Christophe Leroy <hidden>
Date: 2020-10-16 13:18:21


Le 16/10/2020 à 08:54, Christoph Hellwig a écrit :
On Thu, Oct 15, 2020 at 10:01:52AM -0500, Christopher M. Riedl wrote:
quoted
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 1/8] powerpc/uaccess: Add unsafe_copy_from_user

From: Christophe Leroy <hidden>
Date: 2020-10-16 13:21:15


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
quoted hunk
Signed-off-by: Christopher M. Riedl <redacted>
---
  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;

36 more quoted lines

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

From: Christophe Leroy <hidden>
Date: 2020-10-16 13:51:41


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

quoted hunk
Signed-off-by: Christopher M. Riedl <redacted>
---
  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)
+

25 more quoted lines

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

From: Christophe Leroy <hidden>
Date: 2020-10-16 13:59:25


Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted hunk
From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
  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) |

18 more quoted lines

This flush should be done outside the user_write_access block.
quoted hunk
  
-	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

From: Christophe Leroy <hidden>
Date: 2020-10-16 14:05:30


Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted hunk
From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
  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]);

11 more quoted lines

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 ?
quoted hunk
  		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();

26 more quoted lines


Christophe

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

From: Christophe Leroy <hidden>
Date: 2020-10-16 14:13:03


Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted hunk
From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
  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 ?
quoted hunk
  
-		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 ?
quoted hunk
-			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'

From: Christopher M. Riedl <hidden>
Date: 2020-10-20 01:59:25

On Fri Oct 16, 2020 at 4:02 AM CDT, Christophe Leroy wrote:

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
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
quoted
Signed-off-by: Christopher M. Riedl <redacted>
---
  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);
  

86 more quoted lines

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

From: Christopher M. Riedl <hidden>
Date: 2020-10-20 02:40:29

On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
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

quoted
Signed-off-by: Christopher M. Riedl <redacted>
---
  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++)				\

28 more quoted lines

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

From: Christopher M. Riedl <hidden>
Date: 2020-10-20 02:42:50

On Fri Oct 16, 2020 at 10:56 AM CDT, Christophe Leroy wrote:

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
  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 */

24 more quoted lines

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.
quoted
  
-	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

From: Christopher M. Riedl <hidden>
Date: 2020-10-20 02:44:58

On Fri Oct 16, 2020 at 11:00 AM CDT, Christophe Leroy wrote:

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
  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);
  

19 more quoted lines

Hopefully yes - next spin!
quoted
  		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]);

30 more quoted lines

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

From: Christopher M. Riedl <hidden>
Date: 2020-10-20 02:46:47

On Fri Oct 16, 2020 at 11:07 AM CDT, Christophe Leroy wrote:

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
  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();

20 more quoted lines

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.
quoted
-			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

From: Christopher M. Riedl <hidden>
Date: 2020-10-20 03:07:18

On Fri Oct 16, 2020 at 10:17 AM CDT, Christophe Leroy wrote:

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
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
quoted
Signed-off-by: Christopher M. Riedl <redacted>
---
  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();

39 more quoted lines

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

From: Christophe Leroy <hidden>
Date: 2020-10-20 05:04:55


Le 20/10/2020 à 04:42, Christopher M. Riedl a écrit :
On Fri Oct 16, 2020 at 10:56 AM CDT, Christophe Leroy wrote:
quoted

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
From: Daniel Axtens <redacted>

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 <redacted>
Signed-off-by: Christopher M. Riedl <redacted>
---
   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);

30 more quoted lines

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
quoted
quoted
   
-	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'

From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2020-10-20 07:34:14

Peter Zijlstra [off-list ref] writes:
On Fri, Oct 16, 2020 at 07:56:16AM +0100, Christoph Hellwig wrote:
quoted
On Thu, Oct 15, 2020 at 10:01:54AM -0500, Christopher M. Riedl wrote:
quoted
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()

From: Christophe Leroy <hidden>
Date: 2021-02-06 16:36:12


Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
quoted

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
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++)                            \

14 more quoted lines

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()

From: Christopher M. Riedl <hidden>
Date: 2021-02-06 17:55:15

On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:

Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
quoted
On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
quoted

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
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;                           \

20 more quoted lines

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()

From: Christophe Leroy <hidden>
Date: 2021-02-07 10:13:35


Le 06/02/2021 à 18:39, Christopher M. Riedl a écrit :
On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:
quoted

Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
quoted
On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
quoted

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
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;                                 \

28 more quoted lines

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()

From: Christopher M. Riedl <hidden>
Date: 2021-02-08 17:15:16

On Sun Feb 7, 2021 at 4:12 AM CST, Christophe Leroy wrote:

Le 06/02/2021 à 18:39, Christopher M. Riedl a écrit :
quoted
On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:
quoted

Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
quoted
On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
quoted

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
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:

36 more quoted lines

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()

From: Christophe Leroy <hidden>
Date: 2021-02-08 17:21:09


Le 08/02/2021 à 18:14, Christopher M. Riedl a écrit :
On Sun Feb 7, 2021 at 4:12 AM CST, Christophe Leroy wrote:
quoted

Le 06/02/2021 à 18:39, Christopher M. Riedl a écrit :
quoted
On Sat Feb 6, 2021 at 10:32 AM CST, Christophe Leroy wrote:
quoted

Le 20/10/2020 à 04:01, Christopher M. Riedl a écrit :
quoted
On Fri Oct 16, 2020 at 10:48 AM CDT, Christophe Leroy wrote:
quoted

Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
quoted
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.

53 more quoted lines

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

Christophe
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help