On Fri Oct 16, 2020 at 11:07 AM CDT, Christophe Leroy wrote:
>
>
> Le 15/10/2020 à 17:01, Christopher M. Riedl a écrit :
> > From: Daniel Axtens <d...@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 <d...@axtens.net>
> > Signed-off-by: Christopher M. Riedl <c...@codefail.de>
> > ---
> > arch/powerpc/kernel/signal_64.c | 23 +++++++++++++++--------
> > 1 file changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/signal_64.c
> > b/arch/powerpc/kernel/signal_64.c
> > index 3b97e3681a8f..0f4ff7a5bfc1 100644
> > --- a/arch/powerpc/kernel/signal_64.c
> > +++ b/arch/powerpc/kernel/signal_64.c
> > @@ -779,18 +779,22 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > */
> > regs->msr &= ~MSR_TS_MASK;
> >
> > - if (__get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR]))
> > + if (!user_read_access_begin(uc, sizeof(*uc)))
> > goto badframe;
> > +
> > + unsafe_get_user(msr, &uc->uc_mcontext.gp_regs[PT_MSR], badframe_block);
> > +
> > if (MSR_TM_ACTIVE(msr)) {
> > /* We recheckpoint on return. */
> > struct ucontext __user *uc_transact;
> >
> > /* Trying to start TM on non TM system */
> > if (!cpu_has_feature(CPU_FTR_TM))
> > - goto badframe;
> > + goto badframe_block;
> > +
> > + unsafe_get_user(uc_transact, &uc->uc_link, badframe_block);
> > + user_read_access_end();
>
> user_access_end() only in the if branch ?
>
> >
> > - if (__get_user(uc_transact, &uc->uc_link))
> > - goto badframe;
> > if (restore_tm_sigcontexts(current, &uc->uc_mcontext,
> > &uc_transact->uc_mcontext))
> > goto badframe;
> > @@ -810,12 +814,13 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > * causing a TM bad thing.
> > */
> > current->thread.regs->msr &= ~MSR_TS_MASK;
> > +
> > +#ifndef CONFIG_PPC_TRANSACTIONAL_MEM
> > if (!user_read_access_begin(uc, sizeof(*uc)))
>
> The matching user_read_access_end() is not in the same #ifndef ? That's
> dirty and hard to follow.
> Can you re-organise the code to avoid all those nesting ?
Yes, thanks for pointing this out. I really wanted to avoid changing too
much of the logic inside these functions. But I suppose I ended up
creating a mess - I will fix this in the next spin.
>
> > - return -EFAULT;
> > - if (__unsafe_restore_sigcontext(current, NULL, 1,
> > &uc->uc_mcontext)) {
> > - user_read_access_end();
> > goto badframe;
> > - }
> > +#endif
> > + unsafe_restore_sigcontext(current, NULL, 1, &uc->uc_mcontext,
> > + badframe_block);
> > user_read_access_end();
> > }
> >
> > @@ -825,6 +830,8 @@ SYSCALL_DEFINE0(rt_sigreturn)
> > set_thread_flag(TIF_RESTOREALL);
> > return 0;
> >
> > +badframe_block:
> > + user_read_access_end();
> > badframe:
> > signal_fault(current, regs, "rt_sigreturn", uc);
> >
> >
>
> Christophe
