* [PATCH v6 01/10] powerpc/uaccess: Add unsafe_copy_from_user
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
@ 2021-02-21 1:23 ` Christopher M. Riedl
2021-02-23 17:15 ` Christophe Leroy
2021-02-21 1:23 ` [PATCH v6 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user() Christopher M. Riedl
` (8 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Daniel Axtens
Just wrap __copy_tofrom_user() for the usual 'unsafe' pattern which
accepts a label to goto on error.
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
Reviewed-by: Daniel Axtens <dja@axtens.net>
---
arch/powerpc/include/asm/uaccess.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 78e2a3990eab..33b2de642120 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -487,6 +487,9 @@ user_write_access_begin(const void __user *ptr, size_t len)
#define unsafe_put_user(x, p, e) \
__unsafe_put_user_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
+#define unsafe_copy_from_user(d, s, l, e) \
+ unsafe_op_wrap(__copy_tofrom_user((__force void __user *)d, s, l), e)
+
#define unsafe_copy_to_user(d, s, l, e) \
do { \
u8 __user *_dst = (u8 __user *)(d); \
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v6 01/10] powerpc/uaccess: Add unsafe_copy_from_user
2021-02-21 1:23 ` [PATCH v6 01/10] powerpc/uaccess: Add unsafe_copy_from_user Christopher M. Riedl
@ 2021-02-23 17:15 ` Christophe Leroy
2021-02-24 18:01 ` Christopher M. Riedl
0 siblings, 1 reply; 17+ messages in thread
From: Christophe Leroy @ 2021-02-23 17:15 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev; +Cc: Daniel Axtens
Le 21/02/2021 à 02:23, Christopher M. Riedl a écrit :
> Just wrap __copy_tofrom_user() for the usual 'unsafe' pattern which
> accepts a label to goto on error.
>
> Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
> ---
> arch/powerpc/include/asm/uaccess.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 78e2a3990eab..33b2de642120 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -487,6 +487,9 @@ user_write_access_begin(const void __user *ptr, size_t len)
> #define unsafe_put_user(x, p, e) \
> __unsafe_put_user_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
>
> +#define unsafe_copy_from_user(d, s, l, e) \
> + unsafe_op_wrap(__copy_tofrom_user((__force void __user *)d, s, l), e)
> +
Could we perform same as unsafe_copy_to_user() instead of calling an external function which is
banned in principle inside uaccess blocks ?
> #define unsafe_copy_to_user(d, s, l, e) \
> do { \
> u8 __user *_dst = (u8 __user *)(d); \
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 01/10] powerpc/uaccess: Add unsafe_copy_from_user
2021-02-23 17:15 ` Christophe Leroy
@ 2021-02-24 18:01 ` Christopher M. Riedl
0 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-24 18:01 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Daniel Axtens
On Tue Feb 23, 2021 at 11:15 AM CST, Christophe Leroy wrote:
>
>
> Le 21/02/2021 à 02:23, Christopher M. Riedl a écrit :
> > Just wrap __copy_tofrom_user() for the usual 'unsafe' pattern which
> > accepts a label to goto on error.
> >
> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
> > Reviewed-by: Daniel Axtens <dja@axtens.net>
> > ---
> > arch/powerpc/include/asm/uaccess.h | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> > index 78e2a3990eab..33b2de642120 100644
> > --- a/arch/powerpc/include/asm/uaccess.h
> > +++ b/arch/powerpc/include/asm/uaccess.h
> > @@ -487,6 +487,9 @@ user_write_access_begin(const void __user *ptr, size_t len)
> > #define unsafe_put_user(x, p, e) \
> > __unsafe_put_user_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
> >
> > +#define unsafe_copy_from_user(d, s, l, e) \
> > + unsafe_op_wrap(__copy_tofrom_user((__force void __user *)d, s, l), e)
> > +
>
> Could we perform same as unsafe_copy_to_user() instead of calling an
> external function which is
> banned in principle inside uaccess blocks ?
Yup, with your patch to move the barrier_nospec() into the allowance
helpers this makes sense now. I just tried it and performance does not
change significantly w/ either radix or hash translation. I will include
this change in the next spin - thanks!
>
>
> > #define unsafe_copy_to_user(d, s, l, e) \
> > do { \
> > u8 __user *_dst = (u8 __user *)(d); \
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user()
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
2021-02-21 1:23 ` [PATCH v6 01/10] powerpc/uaccess: Add unsafe_copy_from_user Christopher M. Riedl
@ 2021-02-21 1:23 ` Christopher M. Riedl
2021-02-21 1:23 ` [PATCH v6 03/10] powerpc/signal64: Remove non-inline calls from setup_sigcontext() Christopher M. Riedl
` (7 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Daniel Axtens
Reuse the "safe" implementation from signal.c but call unsafe_get_user()
directly in a loop to avoid the intermediate copy into a local buffer.
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
Reviewed-by: Daniel Axtens <dja@axtens.net>
---
arch/powerpc/kernel/signal.h | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index 2559a681536e..d8dd76b1dc94 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -53,6 +53,26 @@ 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 *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)
+
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
#define unsafe_copy_ckfpr_to_user(to, task, label) do { \
struct task_struct *__t = task; \
@@ -80,6 +100,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 +139,8 @@ copy_ckfpr_from_user(struct task_struct *task, void __user *from)
#else
#define unsafe_copy_fpr_to_user(to, task, label) do { } while (0)
+#define unsafe_copy_fpr_from_user(task, from, label) do { } while (0)
+
static inline unsigned long
copy_fpr_to_user(void __user *to, struct task_struct *task)
{
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 03/10] powerpc/signal64: Remove non-inline calls from setup_sigcontext()
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
2021-02-21 1:23 ` [PATCH v6 01/10] powerpc/uaccess: Add unsafe_copy_from_user Christopher M. Riedl
2021-02-21 1:23 ` [PATCH v6 02/10] powerpc/signal: Add unsafe_copy_{vsx, fpr}_from_user() Christopher M. Riedl
@ 2021-02-21 1:23 ` Christopher M. Riedl
2021-02-21 1:23 ` [PATCH v6 04/10] powerpc: Reference parameter in MSR_TM_ACTIVE() macro Christopher M. Riedl
` (6 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:23 UTC (permalink / raw)
To: linuxppc-dev
The majority of setup_sigcontext() can be refactored to execute in an
"unsafe" context assuming an open uaccess window except for some
non-inline function calls. Move these out into a separate
prepare_setup_sigcontext() function which must be called first and
before opening up a uaccess window. Non-inline function calls should be
avoided during a uaccess window for a few reasons:
- KUAP should be enabled for as much kernel code as possible.
Opening a uaccess window disables KUAP which means any code
executed during this time contributes to a potential attack
surface.
- Non-inline functions default to traceable which means they are
instrumented for ftrace. This adds more code which could run
with KUAP disabled.
- Powerpc does not currently support the objtool UACCESS checks.
All code running with uaccess must be audited manually which
means: less code -> less work -> fewer problems (in theory).
A follow-up commit converts setup_sigcontext() to be "unsafe".
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index f9e4a1ac440f..6ca546192cbf 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -79,6 +79,24 @@ static elf_vrreg_t __user *sigcontext_vmx_regs(struct sigcontext __user *sc)
}
#endif
+static void prepare_setup_sigcontext(struct task_struct *tsk)
+{
+#ifdef CONFIG_ALTIVEC
+ /* save altivec registers */
+ if (tsk->thread.used_vr)
+ flush_altivec_to_thread(tsk);
+ if (cpu_has_feature(CPU_FTR_ALTIVEC))
+ tsk->thread.vrsave = mfspr(SPRN_VRSAVE);
+#endif /* CONFIG_ALTIVEC */
+
+ flush_fp_to_thread(tsk);
+
+#ifdef CONFIG_VSX
+ if (tsk->thread.used_vsr)
+ flush_vsx_to_thread(tsk);
+#endif /* CONFIG_VSX */
+}
+
/*
* Set up the sigcontext for the signal frame.
*/
@@ -97,7 +115,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
*/
#ifdef CONFIG_ALTIVEC
elf_vrreg_t __user *v_regs = sigcontext_vmx_regs(sc);
- unsigned long vrsave;
#endif
struct pt_regs *regs = tsk->thread.regs;
unsigned long msr = regs->msr;
@@ -112,7 +129,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
/* save altivec registers */
if (tsk->thread.used_vr) {
- flush_altivec_to_thread(tsk);
/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
33 * sizeof(vector128));
@@ -124,17 +140,10 @@ static long setup_sigcontext(struct sigcontext __user *sc,
/* We always copy to/from vrsave, it's 0 if we don't have or don't
* use altivec.
*/
- vrsave = 0;
- if (cpu_has_feature(CPU_FTR_ALTIVEC)) {
- vrsave = mfspr(SPRN_VRSAVE);
- tsk->thread.vrsave = vrsave;
- }
-
- err |= __put_user(vrsave, (u32 __user *)&v_regs[33]);
+ err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
#else /* CONFIG_ALTIVEC */
err |= __put_user(0, &sc->v_regs);
#endif /* CONFIG_ALTIVEC */
- flush_fp_to_thread(tsk);
/* copy fpr regs and fpscr */
err |= copy_fpr_to_user(&sc->fp_regs, tsk);
@@ -150,7 +159,6 @@ static long setup_sigcontext(struct sigcontext __user *sc,
* VMX data.
*/
if (tsk->thread.used_vsr && ctx_has_vsx_region) {
- flush_vsx_to_thread(tsk);
v_regs += ELF_NVRREG;
err |= copy_vsx_to_user(v_regs, tsk);
/* set MSR_VSX in the MSR value in the frame to
@@ -655,6 +663,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
ctx_has_vsx_region = 1;
if (old_ctx != NULL) {
+ prepare_setup_sigcontext(current);
if (!access_ok(old_ctx, ctx_size)
|| setup_sigcontext(&old_ctx->uc_mcontext, current, 0, NULL, 0,
ctx_has_vsx_region)
@@ -842,6 +851,7 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
#endif
{
err |= __put_user(0, &frame->uc.uc_link);
+ prepare_setup_sigcontext(tsk);
err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
NULL, (unsigned long)ksig->ka.sa.sa_handler,
1);
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 04/10] powerpc: Reference parameter in MSR_TM_ACTIVE() macro
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
` (2 preceding siblings ...)
2021-02-21 1:23 ` [PATCH v6 03/10] powerpc/signal64: Remove non-inline calls from setup_sigcontext() Christopher M. Riedl
@ 2021-02-21 1:23 ` Christopher M. Riedl
2021-02-21 1:23 ` [PATCH v6 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block Christopher M. Riedl
` (5 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Daniel Axtens
Unlike the other MSR_TM_* macros, MSR_TM_ACTIVE does not reference or
use its parameter unless CONFIG_PPC_TRANSACTIONAL_MEM is defined. This
causes an 'unused variable' compile warning unless the variable is also
guarded with CONFIG_PPC_TRANSACTIONAL_MEM.
Reference but do nothing with the argument in the macro to avoid a
potential compile warning.
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
Reviewed-by: Daniel Axtens <dja@axtens.net>
---
arch/powerpc/include/asm/reg.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index da103e92c112..1be20bc8dce2 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -124,7 +124,7 @@
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
#define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */
#else
-#define MSR_TM_ACTIVE(x) 0
+#define MSR_TM_ACTIVE(x) ((void)(x), 0)
#endif
#if defined(CONFIG_PPC_BOOK3S_64)
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
` (3 preceding siblings ...)
2021-02-21 1:23 ` [PATCH v6 04/10] powerpc: Reference parameter in MSR_TM_ACTIVE() macro Christopher M. Riedl
@ 2021-02-21 1:23 ` Christopher M. Riedl
2021-02-21 1:23 ` [PATCH v6 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext() Christopher M. Riedl
` (4 subsequent siblings)
9 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:23 UTC (permalink / raw)
To: linuxppc-dev
Both rt_sigreturn() and handle_rt_signal_64() contain TM-related ifdefs
which break-up an if/else block. Provide stubs for the ifdef-guarded TM
functions and remove the need for an ifdef in rt_sigreturn().
Rework the remaining TM ifdef in handle_rt_signal64() similar to
commit f1cf4f93de2f ("powerpc/signal32: Remove ifdefery in middle of if/else").
Unlike in the commit for ppc32, the ifdef can't be removed entirely
since uc_transact in sigframe depends on CONFIG_PPC_TRANSACTIONAL_MEM.
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
arch/powerpc/kernel/process.c | 3 +-
arch/powerpc/kernel/signal_64.c | 102 ++++++++++++++++----------------
2 files changed, 54 insertions(+), 51 deletions(-)
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 924d023dad0a..08c3fbe45921 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1117,9 +1117,10 @@ void restore_tm_state(struct pt_regs *regs)
regs->msr |= msr_diff;
}
-#else
+#else /* !CONFIG_PPC_TRANSACTIONAL_MEM */
#define tm_recheckpoint_new_task(new)
#define __switch_to_tm(prev, new)
+void tm_reclaim_current(uint8_t cause) {}
#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
static inline void save_sprs(struct thread_struct *t)
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 6ca546192cbf..bd8d210c9115 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -594,6 +594,12 @@ static long restore_tm_sigcontexts(struct task_struct *tsk,
return err;
}
+#else /* !CONFIG_PPC_TRANSACTIONAL_MEM */
+static long restore_tm_sigcontexts(struct task_struct *tsk, struct sigcontext __user *sc,
+ struct sigcontext __user *tm_sc)
+{
+ return -EINVAL;
+}
#endif
/*
@@ -710,9 +716,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
struct pt_regs *regs = current_pt_regs();
struct ucontext __user *uc = (struct ucontext __user *)regs->gpr[1];
sigset_t set;
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
unsigned long msr;
-#endif
/* Always make any pending restarted system calls return -EINTR */
current->restart_block.fn = do_no_restart_syscall;
@@ -724,48 +728,50 @@ SYSCALL_DEFINE0(rt_sigreturn)
goto badframe;
set_current_blocked(&set);
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
- /*
- * If there is a transactional state then throw it away.
- * The purpose of a sigreturn is to destroy all traces of the
- * signal frame, this includes any transactional state created
- * within in. We only check for suspended as we can never be
- * active in the kernel, we are active, there is nothing better to
- * do than go ahead and Bad Thing later.
- * The cause is not important as there will never be a
- * recheckpoint so it's not user visible.
- */
- if (MSR_TM_SUSPENDED(mfmsr()))
- tm_reclaim_current(0);
+ if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM)) {
+ /*
+ * If there is a transactional state then throw it away.
+ * The purpose of a sigreturn is to destroy all traces of the
+ * signal frame, this includes any transactional state created
+ * within in. We only check for suspended as we can never be
+ * active in the kernel, we are active, there is nothing better to
+ * do than go ahead and Bad Thing later.
+ * The cause is not important as there will never be a
+ * recheckpoint so it's not user visible.
+ */
+ if (MSR_TM_SUSPENDED(mfmsr()))
+ tm_reclaim_current(0);
- /*
- * Disable MSR[TS] bit also, so, if there is an exception in the
- * code below (as a page fault in copy_ckvsx_to_user()), it does
- * not recheckpoint this task if there was a context switch inside
- * the exception.
- *
- * A major page fault can indirectly call schedule(). A reschedule
- * process in the middle of an exception can have a side effect
- * (Changing the CPU MSR[TS] state), since schedule() is called
- * with the CPU MSR[TS] disable and returns with MSR[TS]=Suspended
- * (switch_to() calls tm_recheckpoint() for the 'new' process). In
- * this case, the process continues to be the same in the CPU, but
- * the CPU state just changed.
- *
- * This can cause a TM Bad Thing, since the MSR in the stack will
- * have the MSR[TS]=0, and this is what will be used to RFID.
- *
- * Clearing MSR[TS] state here will avoid a recheckpoint if there
- * is any process reschedule in kernel space. The MSR[TS] state
- * does not need to be saved also, since it will be replaced with
- * the MSR[TS] that came from user context later, at
- * restore_tm_sigcontexts.
- */
- regs->msr &= ~MSR_TS_MASK;
+ /*
+ * Disable MSR[TS] bit also, so, if there is an exception in the
+ * code below (as a page fault in copy_ckvsx_to_user()), it does
+ * not recheckpoint this task if there was a context switch inside
+ * the exception.
+ *
+ * A major page fault can indirectly call schedule(). A reschedule
+ * process in the middle of an exception can have a side effect
+ * (Changing the CPU MSR[TS] state), since schedule() is called
+ * with the CPU MSR[TS] disable and returns with MSR[TS]=Suspended
+ * (switch_to() calls tm_recheckpoint() for the 'new' process). In
+ * this case, the process continues to be the same in the CPU, but
+ * the CPU state just changed.
+ *
+ * This can cause a TM Bad Thing, since the MSR in the stack will
+ * have the MSR[TS]=0, and this is what will be used to RFID.
+ *
+ * Clearing MSR[TS] state here will avoid a recheckpoint if there
+ * is any process reschedule in kernel space. The MSR[TS] state
+ * does not need to be saved also, since it will be replaced with
+ * the MSR[TS] that came from user context later, at
+ * restore_tm_sigcontexts.
+ */
+ regs->msr &= ~MSR_TS_MASK;
- if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
- goto badframe;
- if (MSR_TM_ACTIVE(msr)) {
+ if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
+ goto badframe;
+ }
+
+ if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) && MSR_TM_ACTIVE(msr)) {
/* We recheckpoint on return. */
struct ucontext __user *uc_transact;
@@ -778,9 +784,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
&uc_transact->uc_mcontext))
goto badframe;
- } else
-#endif
- {
+ } else {
/*
* Fall through, for non-TM restore
*
@@ -818,10 +822,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
unsigned long newsp = 0;
long err = 0;
struct pt_regs *regs = tsk->thread.regs;
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
/* Save the thread's msr before get_tm_stackpointer() changes it */
unsigned long msr = regs->msr;
-#endif
frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
if (!access_ok(frame, sizeof(*frame)))
@@ -836,8 +838,9 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
/* Create the ucontext. */
err |= __put_user(0, &frame->uc.uc_flags);
err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
-#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+
if (MSR_TM_ACTIVE(msr)) {
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
/* The ucontext_t passed to userland points to the second
* ucontext_t (for transactional state) with its uc_link ptr.
*/
@@ -847,9 +850,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
tsk, ksig->sig, NULL,
(unsigned long)ksig->ka.sa.sa_handler,
msr);
- } else
#endif
- {
+ } else {
err |= __put_user(0, &frame->uc.uc_link);
prepare_setup_sigcontext(tsk);
err |= setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext()
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
` (4 preceding siblings ...)
2021-02-21 1:23 ` [PATCH v6 05/10] powerpc/signal64: Remove TM ifdefery in middle of if/else block Christopher M. Riedl
@ 2021-02-21 1:23 ` Christopher M. Riedl
2021-02-23 17:12 ` Christophe Leroy
2021-02-21 1:23 ` [PATCH v6 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext() Christopher M. Riedl
` (3 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:23 UTC (permalink / raw)
To: linuxppc-dev
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 by replacing all uaccess functions with their 'unsafe' versions.
Modify the callers to first open, call unsafe_setup_sigcontext() and
then close the uaccess window.
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 bd8d210c9115..3faaa736ed62 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -101,9 +101,13 @@ static void prepare_setup_sigcontext(struct task_struct *tsk)
* Set up the sigcontext for the signal frame.
*/
-static long setup_sigcontext(struct sigcontext __user *sc,
- struct task_struct *tsk, int signr, sigset_t *set,
- unsigned long handler, int ctx_has_vsx_region)
+#define unsafe_setup_sigcontext(sc, tsk, signr, set, handler, \
+ ctx_has_vsx_region, e) \
+ unsafe_op_wrap(__unsafe_setup_sigcontext(sc, tsk, signr, set, \
+ handler, ctx_has_vsx_region), e)
+static long notrace __unsafe_setup_sigcontext(struct sigcontext __user *sc,
+ struct task_struct *tsk, int signr, sigset_t *set,
+ unsigned long handler, int ctx_has_vsx_region)
{
/* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
* process never used altivec yet (MSR_VEC is zero in pt_regs of
@@ -118,20 +122,19 @@ static long setup_sigcontext(struct sigcontext __user *sc,
#endif
struct pt_regs *regs = tsk->thread.regs;
unsigned long msr = regs->msr;
- long err = 0;
/* Force usr to alway see softe as 1 (interrupts enabled) */
unsigned long softe = 0x1;
BUG_ON(tsk != current);
#ifdef CONFIG_ALTIVEC
- err |= __put_user(v_regs, &sc->v_regs);
+ unsafe_put_user(v_regs, &sc->v_regs, efault_out);
/* save altivec registers */
if (tsk->thread.used_vr) {
/* Copy 33 vec registers (vr0..31 and vscr) to the stack */
- err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
- 33 * sizeof(vector128));
+ unsafe_copy_to_user(v_regs, &tsk->thread.vr_state,
+ 33 * sizeof(vector128), efault_out);
/* set MSR_VEC in the MSR value in the frame to indicate that sc->v_reg)
* contains valid data.
*/
@@ -140,12 +143,12 @@ static long setup_sigcontext(struct sigcontext __user *sc,
/* We always copy to/from vrsave, it's 0 if we don't have or don't
* use altivec.
*/
- err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
+ unsafe_put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33], efault_out);
#else /* CONFIG_ALTIVEC */
- err |= __put_user(0, &sc->v_regs);
+ unsafe_put_user(0, &sc->v_regs, efault_out);
#endif /* CONFIG_ALTIVEC */
/* copy fpr regs and fpscr */
- err |= copy_fpr_to_user(&sc->fp_regs, tsk);
+ unsafe_copy_fpr_to_user(&sc->fp_regs, tsk, efault_out);
/*
* Clear the MSR VSX bit to indicate there is no valid state attached
@@ -160,24 +163,27 @@ static long setup_sigcontext(struct sigcontext __user *sc,
*/
if (tsk->thread.used_vsr && ctx_has_vsx_region) {
v_regs += ELF_NVRREG;
- err |= copy_vsx_to_user(v_regs, tsk);
+ unsafe_copy_vsx_to_user(v_regs, tsk, efault_out);
/* set MSR_VSX in the MSR value in the frame to
* indicate that sc->vs_reg) contains valid data.
*/
msr |= MSR_VSX;
}
#endif /* CONFIG_VSX */
- err |= __put_user(&sc->gp_regs, &sc->regs);
+ unsafe_put_user(&sc->gp_regs, &sc->regs, efault_out);
WARN_ON(!FULL_REGS(regs));
- err |= __copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE);
- err |= __put_user(msr, &sc->gp_regs[PT_MSR]);
- err |= __put_user(softe, &sc->gp_regs[PT_SOFTE]);
- err |= __put_user(signr, &sc->signal);
- err |= __put_user(handler, &sc->handler);
+ unsafe_copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE, efault_out);
+ unsafe_put_user(msr, &sc->gp_regs[PT_MSR], efault_out);
+ unsafe_put_user(softe, &sc->gp_regs[PT_SOFTE], efault_out);
+ unsafe_put_user(signr, &sc->signal, efault_out);
+ unsafe_put_user(handler, &sc->handler, efault_out);
if (set != NULL)
- err |= __put_user(set->sig[0], &sc->oldmask);
+ unsafe_put_user(set->sig[0], &sc->oldmask, efault_out);
- return err;
+ return 0;
+
+efault_out:
+ return -EFAULT;
}
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
@@ -670,12 +676,15 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
if (old_ctx != NULL) {
prepare_setup_sigcontext(current);
- 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,
- ¤t->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, ¤t->blocked,
+ sizeof(sigset_t), efault_out);
+
+ user_write_access_end();
}
if (new_ctx == NULL)
return 0;
@@ -704,6 +713,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;
}
@@ -854,9 +867,13 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
} else {
err |= __put_user(0, &frame->uc.uc_link);
prepare_setup_sigcontext(tsk);
- 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->uc.uc_mcontext,
+ sizeof(frame->uc.uc_mcontext)))
+ return -EFAULT;
+ err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk,
+ ksig->sig, NULL,
+ (unsigned long)ksig->ka.sa.sa_handler, 1);
+ user_write_access_end();
}
err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
if (err)
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v6 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext()
2021-02-21 1:23 ` [PATCH v6 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext() Christopher M. Riedl
@ 2021-02-23 17:12 ` Christophe Leroy
2021-02-24 22:00 ` Christopher M. Riedl
0 siblings, 1 reply; 17+ messages in thread
From: Christophe Leroy @ 2021-02-23 17:12 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
Le 21/02/2021 à 02:23, Christopher M. Riedl a écrit :
> 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 by replacing all uaccess functions with their 'unsafe' versions.
> Modify the callers to first open, call unsafe_setup_sigcontext() and
> then close the uaccess window.
Do you plan to also convert setup_tm_sigcontexts() ?
It would allow to then remove copy_fpr_to_user() and copy_ckfpr_to_user() and maybe other functions too.
Christophe
>
> 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 bd8d210c9115..3faaa736ed62 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -101,9 +101,13 @@ static void prepare_setup_sigcontext(struct task_struct *tsk)
> * Set up the sigcontext for the signal frame.
> */
>
> -static long setup_sigcontext(struct sigcontext __user *sc,
> - struct task_struct *tsk, int signr, sigset_t *set,
> - unsigned long handler, int ctx_has_vsx_region)
> +#define unsafe_setup_sigcontext(sc, tsk, signr, set, handler, \
> + ctx_has_vsx_region, e) \
> + unsafe_op_wrap(__unsafe_setup_sigcontext(sc, tsk, signr, set, \
> + handler, ctx_has_vsx_region), e)
> +static long notrace __unsafe_setup_sigcontext(struct sigcontext __user *sc,
> + struct task_struct *tsk, int signr, sigset_t *set,
> + unsigned long handler, int ctx_has_vsx_region)
> {
> /* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
> * process never used altivec yet (MSR_VEC is zero in pt_regs of
> @@ -118,20 +122,19 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> #endif
> struct pt_regs *regs = tsk->thread.regs;
> unsigned long msr = regs->msr;
> - long err = 0;
> /* Force usr to alway see softe as 1 (interrupts enabled) */
> unsigned long softe = 0x1;
>
> BUG_ON(tsk != current);
>
> #ifdef CONFIG_ALTIVEC
> - err |= __put_user(v_regs, &sc->v_regs);
> + unsafe_put_user(v_regs, &sc->v_regs, efault_out);
>
> /* save altivec registers */
> if (tsk->thread.used_vr) {
> /* Copy 33 vec registers (vr0..31 and vscr) to the stack */
> - err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
> - 33 * sizeof(vector128));
> + unsafe_copy_to_user(v_regs, &tsk->thread.vr_state,
> + 33 * sizeof(vector128), efault_out);
> /* set MSR_VEC in the MSR value in the frame to indicate that sc->v_reg)
> * contains valid data.
> */
> @@ -140,12 +143,12 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> /* We always copy to/from vrsave, it's 0 if we don't have or don't
> * use altivec.
> */
> - err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> + unsafe_put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33], efault_out);
> #else /* CONFIG_ALTIVEC */
> - err |= __put_user(0, &sc->v_regs);
> + unsafe_put_user(0, &sc->v_regs, efault_out);
> #endif /* CONFIG_ALTIVEC */
> /* copy fpr regs and fpscr */
> - err |= copy_fpr_to_user(&sc->fp_regs, tsk);
> + unsafe_copy_fpr_to_user(&sc->fp_regs, tsk, efault_out);
>
> /*
> * Clear the MSR VSX bit to indicate there is no valid state attached
> @@ -160,24 +163,27 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> */
> if (tsk->thread.used_vsr && ctx_has_vsx_region) {
> v_regs += ELF_NVRREG;
> - err |= copy_vsx_to_user(v_regs, tsk);
> + unsafe_copy_vsx_to_user(v_regs, tsk, efault_out);
> /* set MSR_VSX in the MSR value in the frame to
> * indicate that sc->vs_reg) contains valid data.
> */
> msr |= MSR_VSX;
> }
> #endif /* CONFIG_VSX */
> - err |= __put_user(&sc->gp_regs, &sc->regs);
> + unsafe_put_user(&sc->gp_regs, &sc->regs, efault_out);
> WARN_ON(!FULL_REGS(regs));
> - err |= __copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE);
> - err |= __put_user(msr, &sc->gp_regs[PT_MSR]);
> - err |= __put_user(softe, &sc->gp_regs[PT_SOFTE]);
> - err |= __put_user(signr, &sc->signal);
> - err |= __put_user(handler, &sc->handler);
> + unsafe_copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE, efault_out);
> + unsafe_put_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> + unsafe_put_user(softe, &sc->gp_regs[PT_SOFTE], efault_out);
> + unsafe_put_user(signr, &sc->signal, efault_out);
> + unsafe_put_user(handler, &sc->handler, efault_out);
> if (set != NULL)
> - err |= __put_user(set->sig[0], &sc->oldmask);
> + unsafe_put_user(set->sig[0], &sc->oldmask, efault_out);
>
> - return err;
> + return 0;
> +
> +efault_out:
> + return -EFAULT;
> }
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> @@ -670,12 +676,15 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
>
> if (old_ctx != NULL) {
> prepare_setup_sigcontext(current);
> - 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,
> - ¤t->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, ¤t->blocked,
> + sizeof(sigset_t), efault_out);
> +
> + user_write_access_end();
> }
> if (new_ctx == NULL)
> return 0;
> @@ -704,6 +713,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;
> }
>
>
> @@ -854,9 +867,13 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> } else {
> err |= __put_user(0, &frame->uc.uc_link);
> prepare_setup_sigcontext(tsk);
> - 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->uc.uc_mcontext,
> + sizeof(frame->uc.uc_mcontext)))
> + 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)
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext()
2021-02-23 17:12 ` Christophe Leroy
@ 2021-02-24 22:00 ` Christopher M. Riedl
0 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-24 22:00 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
On Tue Feb 23, 2021 at 11:12 AM CST, Christophe Leroy wrote:
>
>
> Le 21/02/2021 à 02:23, Christopher M. Riedl a écrit :
> > 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 by replacing all uaccess functions with their 'unsafe' versions.
> > Modify the callers to first open, call unsafe_setup_sigcontext() and
> > then close the uaccess window.
>
> Do you plan to also convert setup_tm_sigcontexts() ?
> It would allow to then remove copy_fpr_to_user() and
> copy_ckfpr_to_user() and maybe other functions too.
I don't intend to convert the TM functions as part of this series.
Partially because I've been "threatened" with TM ownership for touching
the code :) and also because TM enhancements are a pretty low priority I
think.
>
> Christophe
>
> >
> > 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 bd8d210c9115..3faaa736ed62 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -101,9 +101,13 @@ static void prepare_setup_sigcontext(struct task_struct *tsk)
> > * Set up the sigcontext for the signal frame.
> > */
> >
> > -static long setup_sigcontext(struct sigcontext __user *sc,
> > - struct task_struct *tsk, int signr, sigset_t *set,
> > - unsigned long handler, int ctx_has_vsx_region)
> > +#define unsafe_setup_sigcontext(sc, tsk, signr, set, handler, \
> > + ctx_has_vsx_region, e) \
> > + unsafe_op_wrap(__unsafe_setup_sigcontext(sc, tsk, signr, set, \
> > + handler, ctx_has_vsx_region), e)
> > +static long notrace __unsafe_setup_sigcontext(struct sigcontext __user *sc,
> > + struct task_struct *tsk, int signr, sigset_t *set,
> > + unsigned long handler, int ctx_has_vsx_region)
> > {
> > /* When CONFIG_ALTIVEC is set, we _always_ setup v_regs even if the
> > * process never used altivec yet (MSR_VEC is zero in pt_regs of
> > @@ -118,20 +122,19 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> > #endif
> > struct pt_regs *regs = tsk->thread.regs;
> > unsigned long msr = regs->msr;
> > - long err = 0;
> > /* Force usr to alway see softe as 1 (interrupts enabled) */
> > unsigned long softe = 0x1;
> >
> > BUG_ON(tsk != current);
> >
> > #ifdef CONFIG_ALTIVEC
> > - err |= __put_user(v_regs, &sc->v_regs);
> > + unsafe_put_user(v_regs, &sc->v_regs, efault_out);
> >
> > /* save altivec registers */
> > if (tsk->thread.used_vr) {
> > /* Copy 33 vec registers (vr0..31 and vscr) to the stack */
> > - err |= __copy_to_user(v_regs, &tsk->thread.vr_state,
> > - 33 * sizeof(vector128));
> > + unsafe_copy_to_user(v_regs, &tsk->thread.vr_state,
> > + 33 * sizeof(vector128), efault_out);
> > /* set MSR_VEC in the MSR value in the frame to indicate that sc->v_reg)
> > * contains valid data.
> > */
> > @@ -140,12 +143,12 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> > /* We always copy to/from vrsave, it's 0 if we don't have or don't
> > * use altivec.
> > */
> > - err |= __put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> > + unsafe_put_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33], efault_out);
> > #else /* CONFIG_ALTIVEC */
> > - err |= __put_user(0, &sc->v_regs);
> > + unsafe_put_user(0, &sc->v_regs, efault_out);
> > #endif /* CONFIG_ALTIVEC */
> > /* copy fpr regs and fpscr */
> > - err |= copy_fpr_to_user(&sc->fp_regs, tsk);
> > + unsafe_copy_fpr_to_user(&sc->fp_regs, tsk, efault_out);
> >
> > /*
> > * Clear the MSR VSX bit to indicate there is no valid state attached
> > @@ -160,24 +163,27 @@ static long setup_sigcontext(struct sigcontext __user *sc,
> > */
> > if (tsk->thread.used_vsr && ctx_has_vsx_region) {
> > v_regs += ELF_NVRREG;
> > - err |= copy_vsx_to_user(v_regs, tsk);
> > + unsafe_copy_vsx_to_user(v_regs, tsk, efault_out);
> > /* set MSR_VSX in the MSR value in the frame to
> > * indicate that sc->vs_reg) contains valid data.
> > */
> > msr |= MSR_VSX;
> > }
> > #endif /* CONFIG_VSX */
> > - err |= __put_user(&sc->gp_regs, &sc->regs);
> > + unsafe_put_user(&sc->gp_regs, &sc->regs, efault_out);
> > WARN_ON(!FULL_REGS(regs));
> > - err |= __copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE);
> > - err |= __put_user(msr, &sc->gp_regs[PT_MSR]);
> > - err |= __put_user(softe, &sc->gp_regs[PT_SOFTE]);
> > - err |= __put_user(signr, &sc->signal);
> > - err |= __put_user(handler, &sc->handler);
> > + unsafe_copy_to_user(&sc->gp_regs, regs, GP_REGS_SIZE, efault_out);
> > + unsafe_put_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> > + unsafe_put_user(softe, &sc->gp_regs[PT_SOFTE], efault_out);
> > + unsafe_put_user(signr, &sc->signal, efault_out);
> > + unsafe_put_user(handler, &sc->handler, efault_out);
> > if (set != NULL)
> > - err |= __put_user(set->sig[0], &sc->oldmask);
> > + unsafe_put_user(set->sig[0], &sc->oldmask, efault_out);
> >
> > - return err;
> > + return 0;
> > +
> > +efault_out:
> > + return -EFAULT;
> > }
> >
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > @@ -670,12 +676,15 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
> >
> > if (old_ctx != NULL) {
> > prepare_setup_sigcontext(current);
> > - 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,
> > - ¤t->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, ¤t->blocked,
> > + sizeof(sigset_t), efault_out);
> > +
> > + user_write_access_end();
> > }
> > if (new_ctx == NULL)
> > return 0;
> > @@ -704,6 +713,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;
> > }
> >
> >
> > @@ -854,9 +867,13 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
> > } else {
> > err |= __put_user(0, &frame->uc.uc_link);
> > prepare_setup_sigcontext(tsk);
> > - 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->uc.uc_mcontext,
> > + sizeof(frame->uc.uc_mcontext)))
> > + 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)
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
` (5 preceding siblings ...)
2021-02-21 1:23 ` [PATCH v6 06/10] powerpc/signal64: Replace setup_sigcontext() w/ unsafe_setup_sigcontext() Christopher M. Riedl
@ 2021-02-21 1:23 ` Christopher M. Riedl
2021-02-23 17:36 ` Christophe Leroy
2021-02-21 1:23 ` [PATCH v6 08/10] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches Christopher M. Riedl
` (2 subsequent siblings)
9 siblings, 1 reply; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:23 UTC (permalink / raw)
To: linuxppc-dev
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 by replacing all uaccess functions with their 'unsafe'
versions. Modify the callers to first open, call
unsafe_restore_sigcontext(), and then close the uaccess window.
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 3faaa736ed62..76b525261f61 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -326,14 +326,14 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
/*
* Restore the sigcontext from the signal frame.
*/
-
-static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
- struct sigcontext __user *sc)
+#define unsafe_restore_sigcontext(tsk, set, sig, sc, e) \
+ unsafe_op_wrap(__unsafe_restore_sigcontext(tsk, set, sig, sc), e)
+static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_t *set,
+ int sig, struct sigcontext __user *sc)
{
#ifdef CONFIG_ALTIVEC
elf_vrreg_t __user *v_regs;
#endif
- unsigned long err = 0;
unsigned long save_r13 = 0;
unsigned long msr;
struct pt_regs *regs = tsk->thread.regs;
@@ -348,27 +348,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
save_r13 = regs->gpr[13];
/* copy the GPRs */
- err |= __copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr));
- err |= __get_user(regs->nip, &sc->gp_regs[PT_NIP]);
+ unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr),
+ efault_out);
+ unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
/* get MSR separately, transfer the LE bit if doing signal return */
- err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
+ unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
if (sig)
regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
- err |= __get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3]);
- err |= __get_user(regs->ctr, &sc->gp_regs[PT_CTR]);
- err |= __get_user(regs->link, &sc->gp_regs[PT_LNK]);
- err |= __get_user(regs->xer, &sc->gp_regs[PT_XER]);
- err |= __get_user(regs->ccr, &sc->gp_regs[PT_CCR]);
+ unsafe_get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3], efault_out);
+ unsafe_get_user(regs->ctr, &sc->gp_regs[PT_CTR], efault_out);
+ unsafe_get_user(regs->link, &sc->gp_regs[PT_LNK], efault_out);
+ unsafe_get_user(regs->xer, &sc->gp_regs[PT_XER], efault_out);
+ unsafe_get_user(regs->ccr, &sc->gp_regs[PT_CCR], efault_out);
/* Don't allow userspace to set SOFTE */
set_trap_norestart(regs);
- err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
- err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
- err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
+ unsafe_get_user(regs->dar, &sc->gp_regs[PT_DAR], efault_out);
+ unsafe_get_user(regs->dsisr, &sc->gp_regs[PT_DSISR], efault_out);
+ unsafe_get_user(regs->result, &sc->gp_regs[PT_RESULT], efault_out);
if (!sig)
regs->gpr[13] = save_r13;
if (set != NULL)
- err |= __get_user(set->sig[0], &sc->oldmask);
+ unsafe_get_user(set->sig[0], &sc->oldmask, efault_out);
/*
* Force reload of FP/VEC.
@@ -378,29 +379,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
regs->msr &= ~(MSR_FP | MSR_FE0 | MSR_FE1 | MSR_VEC | MSR_VSX);
#ifdef CONFIG_ALTIVEC
- err |= __get_user(v_regs, &sc->v_regs);
- if (err)
- return err;
+ unsafe_get_user(v_regs, &sc->v_regs, efault_out);
if (v_regs && !access_ok(v_regs, 34 * sizeof(vector128)))
return -EFAULT;
/* Copy 33 vec registers (vr0..31 and vscr) from the stack */
if (v_regs != NULL && (msr & MSR_VEC) != 0) {
- err |= __copy_from_user(&tsk->thread.vr_state, v_regs,
- 33 * sizeof(vector128));
+ unsafe_copy_from_user(&tsk->thread.vr_state, v_regs,
+ 33 * sizeof(vector128), efault_out);
tsk->thread.used_vr = true;
} else if (tsk->thread.used_vr) {
memset(&tsk->thread.vr_state, 0, 33 * sizeof(vector128));
}
/* Always get VRSAVE back */
if (v_regs != NULL)
- err |= __get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
+ unsafe_get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33],
+ efault_out);
else
tsk->thread.vrsave = 0;
if (cpu_has_feature(CPU_FTR_ALTIVEC))
mtspr(SPRN_VRSAVE, tsk->thread.vrsave);
#endif /* CONFIG_ALTIVEC */
/* restore floating point */
- err |= copy_fpr_from_user(tsk, &sc->fp_regs);
+ unsafe_copy_fpr_from_user(tsk, &sc->fp_regs, efault_out);
#ifdef CONFIG_VSX
/*
* Get additional VSX data. Update v_regs to point after the
@@ -409,14 +409,17 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
*/
v_regs += ELF_NVRREG;
if ((msr & MSR_VSX) != 0) {
- err |= copy_vsx_from_user(tsk, v_regs);
+ unsafe_copy_vsx_from_user(tsk, v_regs, efault_out);
tsk->thread.used_vsr = true;
} else {
for (i = 0; i < 32 ; i++)
tsk->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
}
#endif
- return err;
+ return 0;
+
+efault_out:
+ return -EFAULT;
}
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
@@ -707,8 +710,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);
@@ -811,8 +820,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->uc_mcontext, sizeof(uc->uc_mcontext)))
+ return -EFAULT;
+ if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
+ user_read_access_end();
goto badframe;
+ }
+ user_read_access_end();
}
if (restore_altstack(&uc->uc_stack))
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v6 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()
2021-02-21 1:23 ` [PATCH v6 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext() Christopher M. Riedl
@ 2021-02-23 17:36 ` Christophe Leroy
2021-02-25 3:54 ` Christopher M. Riedl
0 siblings, 1 reply; 17+ messages in thread
From: Christophe Leroy @ 2021-02-23 17:36 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
Le 21/02/2021 à 02:23, Christopher M. Riedl a écrit :
> 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 by replacing all uaccess functions with their 'unsafe'
> versions. Modify the callers to first open, call
> unsafe_restore_sigcontext(), and then close the uaccess window.
>
> 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 3faaa736ed62..76b525261f61 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -326,14 +326,14 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
> /*
> * Restore the sigcontext from the signal frame.
> */
> -
> -static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> - struct sigcontext __user *sc)
> +#define unsafe_restore_sigcontext(tsk, set, sig, sc, e) \
> + unsafe_op_wrap(__unsafe_restore_sigcontext(tsk, set, sig, sc), e)
unsafe_op_wrap() was not initially meant to be used outside of uaccess.h
In the begining, it has been copied from include/linux/uaccess.h and was used
for unsafe_put_user(), unsafe_get_user() and unsafe_copy_to_user(). After other changes, only
unsafe_get_user() is still using it and I'm going to drop unsafe_op_wrap() soon.
I'd prefer if you can do the same as unsafe_save_general_regs() and others in signal_32.c
> +static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_t *set,
> + int sig, struct sigcontext __user *sc)
> {
> #ifdef CONFIG_ALTIVEC
> elf_vrreg_t __user *v_regs;
> #endif
> - unsigned long err = 0;
> unsigned long save_r13 = 0;
> unsigned long msr;
> struct pt_regs *regs = tsk->thread.regs;
> @@ -348,27 +348,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> save_r13 = regs->gpr[13];
>
> /* copy the GPRs */
> - err |= __copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr));
> - err |= __get_user(regs->nip, &sc->gp_regs[PT_NIP]);
> + unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr),
> + efault_out);
I think it would be better to keep the above on a single line for readability.
Nowadays we tolerate 100 chars lines for cases like this one.
> + unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
> /* get MSR separately, transfer the LE bit if doing signal return */
> - err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
> + unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> if (sig)
> regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
> - err |= __get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3]);
> - err |= __get_user(regs->ctr, &sc->gp_regs[PT_CTR]);
> - err |= __get_user(regs->link, &sc->gp_regs[PT_LNK]);
> - err |= __get_user(regs->xer, &sc->gp_regs[PT_XER]);
> - err |= __get_user(regs->ccr, &sc->gp_regs[PT_CCR]);
> + unsafe_get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3], efault_out);
> + unsafe_get_user(regs->ctr, &sc->gp_regs[PT_CTR], efault_out);
> + unsafe_get_user(regs->link, &sc->gp_regs[PT_LNK], efault_out);
> + unsafe_get_user(regs->xer, &sc->gp_regs[PT_XER], efault_out);
> + unsafe_get_user(regs->ccr, &sc->gp_regs[PT_CCR], efault_out);
> /* Don't allow userspace to set SOFTE */
> set_trap_norestart(regs);
> - err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
> - err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
> - err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
> + unsafe_get_user(regs->dar, &sc->gp_regs[PT_DAR], efault_out);
> + unsafe_get_user(regs->dsisr, &sc->gp_regs[PT_DSISR], efault_out);
> + unsafe_get_user(regs->result, &sc->gp_regs[PT_RESULT], efault_out);
>
> if (!sig)
> regs->gpr[13] = save_r13;
> if (set != NULL)
> - err |= __get_user(set->sig[0], &sc->oldmask);
> + unsafe_get_user(set->sig[0], &sc->oldmask, efault_out);
>
> /*
> * Force reload of FP/VEC.
> @@ -378,29 +379,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> regs->msr &= ~(MSR_FP | MSR_FE0 | MSR_FE1 | MSR_VEC | MSR_VSX);
>
> #ifdef CONFIG_ALTIVEC
> - err |= __get_user(v_regs, &sc->v_regs);
> - if (err)
> - return err;
> + unsafe_get_user(v_regs, &sc->v_regs, efault_out);
> if (v_regs && !access_ok(v_regs, 34 * sizeof(vector128)))
> return -EFAULT;
> /* Copy 33 vec registers (vr0..31 and vscr) from the stack */
> if (v_regs != NULL && (msr & MSR_VEC) != 0) {
> - err |= __copy_from_user(&tsk->thread.vr_state, v_regs,
> - 33 * sizeof(vector128));
> + unsafe_copy_from_user(&tsk->thread.vr_state, v_regs,
> + 33 * sizeof(vector128), efault_out);
> tsk->thread.used_vr = true;
> } else if (tsk->thread.used_vr) {
> memset(&tsk->thread.vr_state, 0, 33 * sizeof(vector128));
> }
> /* Always get VRSAVE back */
> if (v_regs != NULL)
> - err |= __get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> + unsafe_get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33],
> + efault_out);
Same, would be better on a single line I think.
> else
> tsk->thread.vrsave = 0;
> if (cpu_has_feature(CPU_FTR_ALTIVEC))
> mtspr(SPRN_VRSAVE, tsk->thread.vrsave);
> #endif /* CONFIG_ALTIVEC */
> /* restore floating point */
> - err |= copy_fpr_from_user(tsk, &sc->fp_regs);
> + unsafe_copy_fpr_from_user(tsk, &sc->fp_regs, efault_out);
> #ifdef CONFIG_VSX
> /*
> * Get additional VSX data. Update v_regs to point after the
> @@ -409,14 +409,17 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> */
> v_regs += ELF_NVRREG;
> if ((msr & MSR_VSX) != 0) {
> - err |= copy_vsx_from_user(tsk, v_regs);
> + unsafe_copy_vsx_from_user(tsk, v_regs, efault_out);
> tsk->thread.used_vsr = true;
> } else {
> for (i = 0; i < 32 ; i++)
> tsk->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
> }
> #endif
> - return err;
> + return 0;
> +
> +efault_out:
> + return -EFAULT;
> }
>
> #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> @@ -707,8 +710,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);
> @@ -811,8 +820,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->uc_mcontext, sizeof(uc->uc_mcontext)))
> + 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))
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v6 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext()
2021-02-23 17:36 ` Christophe Leroy
@ 2021-02-25 3:54 ` Christopher M. Riedl
0 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-25 3:54 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
On Tue Feb 23, 2021 at 11:36 AM CST, Christophe Leroy wrote:
>
>
> Le 21/02/2021 à 02:23, Christopher M. Riedl a écrit :
> > 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 by replacing all uaccess functions with their 'unsafe'
> > versions. Modify the callers to first open, call
> > unsafe_restore_sigcontext(), and then close the uaccess window.
> >
> > 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 3faaa736ed62..76b525261f61 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -326,14 +326,14 @@ static long setup_tm_sigcontexts(struct sigcontext __user *sc,
> > /*
> > * Restore the sigcontext from the signal frame.
> > */
> > -
> > -static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > - struct sigcontext __user *sc)
> > +#define unsafe_restore_sigcontext(tsk, set, sig, sc, e) \
> > + unsafe_op_wrap(__unsafe_restore_sigcontext(tsk, set, sig, sc), e)
>
> unsafe_op_wrap() was not initially meant to be used outside of uaccess.h
>
> In the begining, it has been copied from include/linux/uaccess.h and was
> used
> for unsafe_put_user(), unsafe_get_user() and unsafe_copy_to_user().
> After other changes, only
> unsafe_get_user() is still using it and I'm going to drop
> unsafe_op_wrap() soon.
>
> I'd prefer if you can do the same as unsafe_save_general_regs() and
> others in signal_32.c
Sounds good, will change this in the next version (and also the wrapper
around unsafe_setup_sigcontext()).
>
> > +static long notrace __unsafe_restore_sigcontext(struct task_struct *tsk, sigset_t *set,
> > + int sig, struct sigcontext __user *sc)
> > {
> > #ifdef CONFIG_ALTIVEC
> > elf_vrreg_t __user *v_regs;
> > #endif
> > - unsigned long err = 0;
> > unsigned long save_r13 = 0;
> > unsigned long msr;
> > struct pt_regs *regs = tsk->thread.regs;
> > @@ -348,27 +348,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > save_r13 = regs->gpr[13];
> >
> > /* copy the GPRs */
> > - err |= __copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr));
> > - err |= __get_user(regs->nip, &sc->gp_regs[PT_NIP]);
> > + unsafe_copy_from_user(regs->gpr, sc->gp_regs, sizeof(regs->gpr),
> > + efault_out);
>
> I think it would be better to keep the above on a single line for
> readability.
> Nowadays we tolerate 100 chars lines for cases like this one.
Ok, changed this (and the line you mention further below) in the next
version.
>
> > + unsafe_get_user(regs->nip, &sc->gp_regs[PT_NIP], efault_out);
> > /* get MSR separately, transfer the LE bit if doing signal return */
> > - err |= __get_user(msr, &sc->gp_regs[PT_MSR]);
> > + unsafe_get_user(msr, &sc->gp_regs[PT_MSR], efault_out);
> > if (sig)
> > regs->msr = (regs->msr & ~MSR_LE) | (msr & MSR_LE);
> > - err |= __get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3]);
> > - err |= __get_user(regs->ctr, &sc->gp_regs[PT_CTR]);
> > - err |= __get_user(regs->link, &sc->gp_regs[PT_LNK]);
> > - err |= __get_user(regs->xer, &sc->gp_regs[PT_XER]);
> > - err |= __get_user(regs->ccr, &sc->gp_regs[PT_CCR]);
> > + unsafe_get_user(regs->orig_gpr3, &sc->gp_regs[PT_ORIG_R3], efault_out);
> > + unsafe_get_user(regs->ctr, &sc->gp_regs[PT_CTR], efault_out);
> > + unsafe_get_user(regs->link, &sc->gp_regs[PT_LNK], efault_out);
> > + unsafe_get_user(regs->xer, &sc->gp_regs[PT_XER], efault_out);
> > + unsafe_get_user(regs->ccr, &sc->gp_regs[PT_CCR], efault_out);
> > /* Don't allow userspace to set SOFTE */
> > set_trap_norestart(regs);
> > - err |= __get_user(regs->dar, &sc->gp_regs[PT_DAR]);
> > - err |= __get_user(regs->dsisr, &sc->gp_regs[PT_DSISR]);
> > - err |= __get_user(regs->result, &sc->gp_regs[PT_RESULT]);
> > + unsafe_get_user(regs->dar, &sc->gp_regs[PT_DAR], efault_out);
> > + unsafe_get_user(regs->dsisr, &sc->gp_regs[PT_DSISR], efault_out);
> > + unsafe_get_user(regs->result, &sc->gp_regs[PT_RESULT], efault_out);
> >
> > if (!sig)
> > regs->gpr[13] = save_r13;
> > if (set != NULL)
> > - err |= __get_user(set->sig[0], &sc->oldmask);
> > + unsafe_get_user(set->sig[0], &sc->oldmask, efault_out);
> >
> > /*
> > * Force reload of FP/VEC.
> > @@ -378,29 +379,28 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > regs->msr &= ~(MSR_FP | MSR_FE0 | MSR_FE1 | MSR_VEC | MSR_VSX);
> >
> > #ifdef CONFIG_ALTIVEC
> > - err |= __get_user(v_regs, &sc->v_regs);
> > - if (err)
> > - return err;
> > + unsafe_get_user(v_regs, &sc->v_regs, efault_out);
> > if (v_regs && !access_ok(v_regs, 34 * sizeof(vector128)))
> > return -EFAULT;
> > /* Copy 33 vec registers (vr0..31 and vscr) from the stack */
> > if (v_regs != NULL && (msr & MSR_VEC) != 0) {
> > - err |= __copy_from_user(&tsk->thread.vr_state, v_regs,
> > - 33 * sizeof(vector128));
> > + unsafe_copy_from_user(&tsk->thread.vr_state, v_regs,
> > + 33 * sizeof(vector128), efault_out);
> > tsk->thread.used_vr = true;
> > } else if (tsk->thread.used_vr) {
> > memset(&tsk->thread.vr_state, 0, 33 * sizeof(vector128));
> > }
> > /* Always get VRSAVE back */
> > if (v_regs != NULL)
> > - err |= __get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33]);
> > + unsafe_get_user(tsk->thread.vrsave, (u32 __user *)&v_regs[33],
> > + efault_out);
>
> Same, would be better on a single line I think.
>
> > else
> > tsk->thread.vrsave = 0;
> > if (cpu_has_feature(CPU_FTR_ALTIVEC))
> > mtspr(SPRN_VRSAVE, tsk->thread.vrsave);
> > #endif /* CONFIG_ALTIVEC */
> > /* restore floating point */
> > - err |= copy_fpr_from_user(tsk, &sc->fp_regs);
> > + unsafe_copy_fpr_from_user(tsk, &sc->fp_regs, efault_out);
> > #ifdef CONFIG_VSX
> > /*
> > * Get additional VSX data. Update v_regs to point after the
> > @@ -409,14 +409,17 @@ static long restore_sigcontext(struct task_struct *tsk, sigset_t *set, int sig,
> > */
> > v_regs += ELF_NVRREG;
> > if ((msr & MSR_VSX) != 0) {
> > - err |= copy_vsx_from_user(tsk, v_regs);
> > + unsafe_copy_vsx_from_user(tsk, v_regs, efault_out);
> > tsk->thread.used_vsr = true;
> > } else {
> > for (i = 0; i < 32 ; i++)
> > tsk->thread.fp_state.fpr[i][TS_VSRLOWOFFSET] = 0;
> > }
> > #endif
> > - return err;
> > + return 0;
> > +
> > +efault_out:
> > + return -EFAULT;
> > }
> >
> > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> > @@ -707,8 +710,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);
> > @@ -811,8 +820,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->uc_mcontext, sizeof(uc->uc_mcontext)))
> > + 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))
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v6 08/10] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
` (6 preceding siblings ...)
2021-02-21 1:23 ` [PATCH v6 07/10] powerpc/signal64: Replace restore_sigcontext() w/ unsafe_restore_sigcontext() Christopher M. Riedl
@ 2021-02-21 1:23 ` Christopher M. Riedl
2021-02-21 1:24 ` [PATCH v6 09/10] powerpc/signal64: Rewrite rt_sigreturn() " Christopher M. Riedl
2021-02-21 1:24 ` [PATCH v6 10/10] powerpc/signal: Use __get_user() to copy sigset_t Christopher M. Riedl
9 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:23 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Daniel Axtens
From: Daniel Axtens <dja@axtens.net>
Add uaccess blocks and use the 'unsafe' versions of functions doing user
access where possible to reduce the number of times uaccess has to be
opened/closed.
There is no 'unsafe' version of copy_siginfo_to_user, so move it
slightly to allow for a "longer" uaccess block.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Co-developed-by: Christopher M. Riedl <cmr@codefail.de>
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
arch/powerpc/kernel/signal_64.c | 56 ++++++++++++++++++++-------------
1 file changed, 35 insertions(+), 21 deletions(-)
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 76b525261f61..4bf73731533f 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -853,45 +853,52 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
unsigned long msr = regs->msr;
frame = get_sigframe(ksig, tsk, sizeof(*frame), 0);
- if (!access_ok(frame, sizeof(*frame)))
- goto badframe;
- err |= __put_user(&frame->info, &frame->pinfo);
- err |= __put_user(&frame->uc, &frame->puc);
- err |= copy_siginfo_to_user(&frame->info, &ksig->info);
- if (err)
+ /* This only applies when calling unsafe_setup_sigcontext() and must be
+ * called before opening the uaccess window.
+ */
+ if (!MSR_TM_ACTIVE(msr))
+ prepare_setup_sigcontext(tsk);
+
+ if (!user_write_access_begin(frame, sizeof(*frame)))
goto badframe;
+ unsafe_put_user(&frame->info, &frame->pinfo, badframe_block);
+ unsafe_put_user(&frame->uc, &frame->puc, badframe_block);
+
/* Create the ucontext. */
- err |= __put_user(0, &frame->uc.uc_flags);
- err |= __save_altstack(&frame->uc.uc_stack, regs->gpr[1]);
+ unsafe_put_user(0, &frame->uc.uc_flags, badframe_block);
+ unsafe_save_altstack(&frame->uc.uc_stack, regs->gpr[1], badframe_block);
if (MSR_TM_ACTIVE(msr)) {
#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
/* The ucontext_t passed to userland points to the second
* ucontext_t (for transactional state) with its uc_link ptr.
*/
- err |= __put_user(&frame->uc_transact, &frame->uc.uc_link);
+ unsafe_put_user(&frame->uc_transact, &frame->uc.uc_link, badframe_block);
+
+ user_write_access_end();
+
err |= setup_tm_sigcontexts(&frame->uc.uc_mcontext,
&frame->uc_transact.uc_mcontext,
tsk, ksig->sig, NULL,
(unsigned long)ksig->ka.sa.sa_handler,
msr);
+
+ if (!user_write_access_begin(&frame->uc.uc_sigmask,
+ sizeof(frame->uc.uc_sigmask)))
+ goto badframe;
+
#endif
} else {
- err |= __put_user(0, &frame->uc.uc_link);
- prepare_setup_sigcontext(tsk);
- if (!user_write_access_begin(&frame->uc.uc_mcontext,
- sizeof(frame->uc.uc_mcontext)))
- return -EFAULT;
- err |= __unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk,
- ksig->sig, NULL,
- (unsigned long)ksig->ka.sa.sa_handler, 1);
- user_write_access_end();
+ unsafe_put_user(0, &frame->uc.uc_link, badframe_block);
+ unsafe_setup_sigcontext(&frame->uc.uc_mcontext, tsk, ksig->sig,
+ NULL, (unsigned long)ksig->ka.sa.sa_handler,
+ 1, badframe_block);
}
- err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
- if (err)
- goto badframe;
+
+ unsafe_copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set), badframe_block);
+ user_write_access_end();
/* Make sure signal handler doesn't get spurious FP exceptions */
tsk->thread.fp_state.fpscr = 0;
@@ -906,6 +913,11 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
regs->nip = (unsigned long) &frame->tramp[0];
}
+
+ /* Save the siginfo outside of the unsafe block. */
+ if (copy_siginfo_to_user(&frame->info, &ksig->info))
+ goto badframe;
+
/* Allocate a dummy caller frame for the signal handler. */
newsp = ((unsigned long)frame) - __SIGNAL_FRAMESIZE;
err |= put_user(regs->gpr[1], (unsigned long __user *)newsp);
@@ -945,6 +957,8 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set,
return 0;
+badframe_block:
+ user_write_access_end();
badframe:
signal_fault(current, regs, "handle_rt_signal64", frame);
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 09/10] powerpc/signal64: Rewrite rt_sigreturn() to minimise uaccess switches
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
` (7 preceding siblings ...)
2021-02-21 1:23 ` [PATCH v6 08/10] powerpc/signal64: Rewrite handle_rt_signal64() to minimise uaccess switches Christopher M. Riedl
@ 2021-02-21 1:24 ` Christopher M. Riedl
2021-02-21 1:24 ` [PATCH v6 10/10] powerpc/signal: Use __get_user() to copy sigset_t Christopher M. Riedl
9 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:24 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Daniel Axtens
From: Daniel Axtens <dja@axtens.net>
Add uaccess blocks and use the 'unsafe' versions of functions doing user
access where possible to reduce the number of times uaccess has to be
opened/closed.
Signed-off-by: Daniel Axtens <dja@axtens.net>
Co-developed-by: Christopher M. Riedl <cmr@codefail.de>
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
arch/powerpc/kernel/signal_64.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 4bf73731533f..3dd89f99e26f 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -821,11 +821,11 @@ SYSCALL_DEFINE0(rt_sigreturn)
*/
current->thread.regs->msr &= ~MSR_TS_MASK;
if (!user_read_access_begin(&uc->uc_mcontext, sizeof(uc->uc_mcontext)))
- return -EFAULT;
- if (__unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext)) {
- user_read_access_end();
goto badframe;
- }
+
+ unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext,
+ badframe_block);
+
user_read_access_end();
}
@@ -835,6 +835,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
set_thread_flag(TIF_RESTOREALL);
return 0;
+badframe_block:
+ user_read_access_end();
badframe:
signal_fault(current, regs, "rt_sigreturn", uc);
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v6 10/10] powerpc/signal: Use __get_user() to copy sigset_t
2021-02-21 1:23 [PATCH v6 00/10] Improve signal performance on PPC64 with KUAP Christopher M. Riedl
` (8 preceding siblings ...)
2021-02-21 1:24 ` [PATCH v6 09/10] powerpc/signal64: Rewrite rt_sigreturn() " Christopher M. Riedl
@ 2021-02-21 1:24 ` Christopher M. Riedl
9 siblings, 0 replies; 17+ messages in thread
From: Christopher M. Riedl @ 2021-02-21 1:24 UTC (permalink / raw)
To: linuxppc-dev
Usually sigset_t is exactly 8B which is a "trivial" size and does not
warrant using __copy_from_user(). Use __get_user() directly in
anticipation of future work to remove the trivial size optimizations
from __copy_from_user().
The ppc32 implementation of get_sigset_t() previously called
copy_from_user() which, unlike __copy_from_user(), calls access_ok().
Replacing this w/ __get_user() (no access_ok()) is fine here since both
callsites in signal_32.c are preceded by an earlier access_ok().
Signed-off-by: Christopher M. Riedl <cmr@codefail.de>
---
arch/powerpc/kernel/signal.h | 7 +++++++
arch/powerpc/kernel/signal_32.c | 2 +-
arch/powerpc/kernel/signal_64.c | 4 ++--
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kernel/signal.h b/arch/powerpc/kernel/signal.h
index d8dd76b1dc94..1393876f3814 100644
--- a/arch/powerpc/kernel/signal.h
+++ b/arch/powerpc/kernel/signal.h
@@ -19,6 +19,13 @@ extern int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
extern int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
struct task_struct *tsk);
+static inline int __get_user_sigset(sigset_t *dst, const sigset_t __user *src)
+{
+ BUILD_BUG_ON(sizeof(sigset_t) != sizeof(u64));
+
+ return __get_user(dst->sig[0], (u64 __user *)&src->sig[0]);
+}
+
#ifdef CONFIG_VSX
extern unsigned long copy_vsx_to_user(void __user *to,
struct task_struct *task);
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 75ee918a120a..c505b444a613 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -144,7 +144,7 @@ static inline int restore_general_regs(struct pt_regs *regs,
static inline int get_sigset_t(sigset_t *set, const sigset_t __user *uset)
{
- return copy_from_user(set, uset, sizeof(*uset));
+ return __get_user_sigset(set, uset);
}
#define to_user_ptr(p) ((unsigned long)(p))
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 3dd89f99e26f..dc5bac727a62 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -707,7 +707,7 @@ SYSCALL_DEFINE3(swapcontext, struct ucontext __user *, old_ctx,
* We kill the task with a SIGSEGV in this situation.
*/
- if (__copy_from_user(&set, &new_ctx->uc_sigmask, sizeof(set)))
+ if (__get_user_sigset(&set, &new_ctx->uc_sigmask))
do_exit(SIGSEGV);
set_current_blocked(&set);
@@ -746,7 +746,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
if (!access_ok(uc, sizeof(*uc)))
goto badframe;
- if (__copy_from_user(&set, &uc->uc_sigmask, sizeof(set)))
+ if (__get_user_sigset(&set, &uc->uc_sigmask))
goto badframe;
set_current_blocked(&set);
--
2.26.1
^ permalink raw reply related [flat|nested] 17+ messages in thread