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

[Christophe Leroy]

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

-                       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