| Message ID | 20201015150159.28933-7-cmr@codefail.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve signal performance on PPC64 with KUAP | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/merge (118be7377c97e35c33819bcb3bbbae5a42a4ac43) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/next (a2d0230b91f7e23ceb5d8fb6a9799f30517ec33a) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linus/master (3e4fb4346c781068610d03c12b16c0cfb0fd24a3) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch powerpc/fixes (0460534b532e5518c657c7d6492b9337d975eaa3) |
| snowpatch_ozlabs/apply_patch | warning | Failed to apply on branch linux-next (03d992bd2de6c7f2c9bbd4260ff104c0926ce3ff) |
| snowpatch_ozlabs/apply_patch | fail | Failed to apply to any branch |
Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit : > From: Daniel Axtens <dja@axtens.net> > > Previously setup_trampoline() performed a costly KUAP switch on every > uaccess operation. These repeated uaccess switches cause a significant > drop in signal handling performance. > > Rewrite setup_trampoline() to assume that a userspace write access > window is open. Replace all uaccess functions with their 'unsafe' > versions to avoid the repeated uaccess switches. > > Signed-off-by: Daniel Axtens <dja@axtens.net> > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > --- > arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > index bd92064e5576..6d4f7a5c4fbf 100644 > --- a/arch/powerpc/kernel/signal_64.c > +++ b/arch/powerpc/kernel/signal_64.c > @@ -600,30 +600,33 @@ static long restore_tm_sigcontexts(struct task_struct *tsk, > /* > * Setup the trampoline code on the stack > */ > -static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp) > +#define unsafe_setup_trampoline(syscall, tramp, e) \ > + unsafe_op_wrap(__unsafe_setup_trampoline(syscall, tramp), e) > +static long notrace __unsafe_setup_trampoline(unsigned int syscall, > + unsigned int __user *tramp) > { > int i; > - long err = 0; > > /* bctrl # call the handler */ > - err |= __put_user(PPC_INST_BCTRL, &tramp[0]); > + unsafe_put_user(PPC_INST_BCTRL, &tramp[0], err); > /* addi r1, r1, __SIGNAL_FRAMESIZE # Pop the dummy stackframe */ > - err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) | > - (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]); > + unsafe_put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) | > + (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1], err); > /* li r0, __NR_[rt_]sigreturn| */ > - err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]); > + unsafe_put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2], err); > /* sc */ > - err |= __put_user(PPC_INST_SC, &tramp[3]); > + unsafe_put_user(PPC_INST_SC, &tramp[3], err); > > /* Minimal traceback info */ > for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++) > - err |= __put_user(0, &tramp[i]); > + unsafe_put_user(0, &tramp[i], err); > > - if (!err) > - flush_icache_range((unsigned long) &tramp[0], > - (unsigned long) &tramp[TRAMP_SIZE]); > + flush_icache_range((unsigned long)&tramp[0], > + (unsigned long)&tramp[TRAMP_SIZE]); This flush should be done outside the user_write_access block. > > - return err; > + return 0; > +err: > + return 1; > } > > /* > @@ -888,7 +891,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) { > regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp; > } else { > - err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); > + if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) > + return -EFAULT; > + err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); > + user_write_access_end(); > if (err) > goto badframe; > regs->nip = (unsigned long) &frame->tramp[0]; > Christophe
On Fri Oct 16, 2020 at 10:56 AM CDT, Christophe Leroy wrote: > > > Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit : > > From: Daniel Axtens <dja@axtens.net> > > > > Previously setup_trampoline() performed a costly KUAP switch on every > > uaccess operation. These repeated uaccess switches cause a significant > > drop in signal handling performance. > > > > Rewrite setup_trampoline() to assume that a userspace write access > > window is open. Replace all uaccess functions with their 'unsafe' > > versions to avoid the repeated uaccess switches. > > > > Signed-off-by: Daniel Axtens <dja@axtens.net> > > Signed-off-by: Christopher M. Riedl <cmr@codefail.de> > > --- > > arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++------------- > > 1 file changed, 19 insertions(+), 13 deletions(-) > > > > diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c > > index bd92064e5576..6d4f7a5c4fbf 100644 > > --- a/arch/powerpc/kernel/signal_64.c > > +++ b/arch/powerpc/kernel/signal_64.c > > @@ -600,30 +600,33 @@ static long restore_tm_sigcontexts(struct task_struct *tsk, > > /* > > * Setup the trampoline code on the stack > > */ > > -static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp) > > +#define unsafe_setup_trampoline(syscall, tramp, e) \ > > + unsafe_op_wrap(__unsafe_setup_trampoline(syscall, tramp), e) > > +static long notrace __unsafe_setup_trampoline(unsigned int syscall, > > + unsigned int __user *tramp) > > { > > int i; > > - long err = 0; > > > > /* bctrl # call the handler */ > > - err |= __put_user(PPC_INST_BCTRL, &tramp[0]); > > + unsafe_put_user(PPC_INST_BCTRL, &tramp[0], err); > > /* addi r1, r1, __SIGNAL_FRAMESIZE # Pop the dummy stackframe */ > > - err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) | > > - (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]); > > + unsafe_put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) | > > + (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1], err); > > /* li r0, __NR_[rt_]sigreturn| */ > > - err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]); > > + unsafe_put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2], err); > > /* sc */ > > - err |= __put_user(PPC_INST_SC, &tramp[3]); > > + unsafe_put_user(PPC_INST_SC, &tramp[3], err); > > > > /* Minimal traceback info */ > > for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++) > > - err |= __put_user(0, &tramp[i]); > > + unsafe_put_user(0, &tramp[i], err); > > > > - if (!err) > > - flush_icache_range((unsigned long) &tramp[0], > > - (unsigned long) &tramp[TRAMP_SIZE]); > > + flush_icache_range((unsigned long)&tramp[0], > > + (unsigned long)&tramp[TRAMP_SIZE]); > > This flush should be done outside the user_write_access block. > Hmm, I suppose that means setup_trampoline() cannot be completely "unsafe". I'll see if I can re-arrange the code which calls this function to avoid an additional uaccess block instead and push the start()/end() into setup_trampoline() directly. > > > > - return err; > > + return 0; > > +err: > > + return 1; > > } > > > > /* > > @@ -888,7 +891,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, > > if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) { > > regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp; > > } else { > > - err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); > > + if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) > > + return -EFAULT; > > + err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); > > + user_write_access_end(); > > if (err) > > goto badframe; > > regs->nip = (unsigned long) &frame->tramp[0]; > > > > Christophe
Le 20/10/2020 à 04:42, Christopher M. Riedl a écrit : > On Fri Oct 16, 2020 at 10:56 AM CDT, Christophe Leroy wrote: >> >> >> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit : >>> From: Daniel Axtens <dja@axtens.net> >>> >>> Previously setup_trampoline() performed a costly KUAP switch on every >>> uaccess operation. These repeated uaccess switches cause a significant >>> drop in signal handling performance. >>> >>> Rewrite setup_trampoline() to assume that a userspace write access >>> window is open. Replace all uaccess functions with their 'unsafe' >>> versions to avoid the repeated uaccess switches. >>> >>> Signed-off-by: Daniel Axtens <dja@axtens.net> >>> Signed-off-by: Christopher M. Riedl <cmr@codefail.de> >>> --- >>> arch/powerpc/kernel/signal_64.c | 32 +++++++++++++++++++------------- >>> 1 file changed, 19 insertions(+), 13 deletions(-) >>> >>> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c >>> index bd92064e5576..6d4f7a5c4fbf 100644 >>> --- a/arch/powerpc/kernel/signal_64.c >>> +++ b/arch/powerpc/kernel/signal_64.c >>> @@ -600,30 +600,33 @@ static long restore_tm_sigcontexts(struct task_struct *tsk, >>> /* >>> * Setup the trampoline code on the stack >>> */ >>> -static long setup_trampoline(unsigned int syscall, unsigned int __user *tramp) >>> +#define unsafe_setup_trampoline(syscall, tramp, e) \ >>> + unsafe_op_wrap(__unsafe_setup_trampoline(syscall, tramp), e) >>> +static long notrace __unsafe_setup_trampoline(unsigned int syscall, >>> + unsigned int __user *tramp) >>> { >>> int i; >>> - long err = 0; >>> >>> /* bctrl # call the handler */ >>> - err |= __put_user(PPC_INST_BCTRL, &tramp[0]); >>> + unsafe_put_user(PPC_INST_BCTRL, &tramp[0], err); >>> /* addi r1, r1, __SIGNAL_FRAMESIZE # Pop the dummy stackframe */ >>> - err |= __put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) | >>> - (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1]); >>> + unsafe_put_user(PPC_INST_ADDI | __PPC_RT(R1) | __PPC_RA(R1) | >>> + (__SIGNAL_FRAMESIZE & 0xffff), &tramp[1], err); >>> /* li r0, __NR_[rt_]sigreturn| */ >>> - err |= __put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2]); >>> + unsafe_put_user(PPC_INST_ADDI | (syscall & 0xffff), &tramp[2], err); >>> /* sc */ >>> - err |= __put_user(PPC_INST_SC, &tramp[3]); >>> + unsafe_put_user(PPC_INST_SC, &tramp[3], err); >>> >>> /* Minimal traceback info */ >>> for (i=TRAMP_TRACEBACK; i < TRAMP_SIZE ;i++) >>> - err |= __put_user(0, &tramp[i]); >>> + unsafe_put_user(0, &tramp[i], err); >>> >>> - if (!err) >>> - flush_icache_range((unsigned long) &tramp[0], >>> - (unsigned long) &tramp[TRAMP_SIZE]); >>> + flush_icache_range((unsigned long)&tramp[0], >>> + (unsigned long)&tramp[TRAMP_SIZE]); >> >> This flush should be done outside the user_write_access block. >> > > Hmm, I suppose that means setup_trampoline() cannot be completely > "unsafe". I'll see if I can re-arrange the code which calls this > function to avoid an additional uaccess block instead and push the > start()/end() into setup_trampoline() directly. I think we shouldn't put too much effort on setup_trampoline(). Nowadays 99.999% of applications use the VDSO. Using the trampoline on stack requires to unmap the VDSO and remap the STACK RW. That's really a corner case, I think it would be good enough to call it outside the main access begin/end block, and let it do its own access_begin/end. This corner functionnality can be tested using the sigreturn_vdso selftest in selftests/powerpc/signal/ Christophe > >>> >>> - return err; >>> + return 0; >>> +err: >>> + return 1; >>> } >>> >>> /* >>> @@ -888,7 +891,10 @@ int handle_rt_signal64(struct ksignal *ksig, sigset_t *set, >>> if (vdso64_rt_sigtramp && tsk->mm->context.vdso_base) { >>> regs->nip = tsk->mm->context.vdso_base + vdso64_rt_sigtramp; >>> } else { >>> - err |= setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); >>> + if (!user_write_access_begin(frame, sizeof(struct rt_sigframe))) >>> + return -EFAULT; >>> + err |= __unsafe_setup_trampoline(__NR_rt_sigreturn, &frame->tramp[0]); >>> + user_write_access_end(); >>> if (err) >>> goto badframe; >>> regs->nip = (unsigned long) &frame->tramp[0]; >>> >> >> Christophe >
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];