[PATCH 0/2] linux-user: sigaction fixes/cleanups

[PATCH 0/2] linux-user: sigaction fixes/cleanups

[Richard Henderson]

Alpha had two bugs, one with the non-ka_restorer fallback
using the wrong offset, and the other with the ka_restorer
value getting lost in do_sigaction.

Because do_sigaction didn't see TARGET_ARCH_HAS_SA_RESTORER
(which is correct for alpha, as the field isn't present in
the structure), we didn't copy the field into our syscall
table.  Which meant that the extra code present in do_syscall
to stash the ka_restorer value was wasted.

I believe that Sparc has a similar bug, but this one was
worse because it was modifying the user's sigaction struct
that was passed in.


r~


Richard Henderson (2):
  linux-user/alpha: Fix rt sigframe return
  linux-user: Clean up sigaction ka_restorer

 linux-user/syscall_defs.h | 17 +++-----
 linux-user/alpha/signal.c | 10 ++---
 linux-user/signal.c       |  4 +-
 linux-user/syscall.c      | 90 ++++++++++++---------------------------
 4 files changed, 42 insertions(+), 79 deletions(-)

-- 
2.25.1

[PATCH 1/2] linux-user/alpha: Fix rt sigframe return

[Richard Henderson]

We incorrectly used the offset of the non-rt sigframe.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/alpha/signal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/alpha/signal.c b/linux-user/alpha/signal.c
index c5c27ce084..86f5d2276d 100644
--- a/linux-user/alpha/signal.c
+++ b/linux-user/alpha/signal.c
@@ -200,7 +200,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
                    &frame->retcode[1]);
         __put_user(INSN_CALLSYS, &frame->retcode[2]);
         /* imb(); */
-        r26 = frame_addr + offsetof(struct target_sigframe, retcode);
+        r26 = frame_addr + offsetof(struct target_rt_sigframe, retcode);
     }
 
     if (err) {
-- 
2.25.1

Re: [PATCH 1/2] linux-user/alpha: Fix rt sigframe return

[Laurent Vivier]

Le 22/04/2021 à 20:08, Richard Henderson a écrit :
> We incorrectly used the offset of the non-rt sigframe.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/alpha/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/alpha/signal.c b/linux-user/alpha/signal.c
> index c5c27ce084..86f5d2276d 100644
> --- a/linux-user/alpha/signal.c
> +++ b/linux-user/alpha/signal.c
> @@ -200,7 +200,7 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>                     &frame->retcode[1]);
>          __put_user(INSN_CALLSYS, &frame->retcode[2]);
>          /* imb(); */
> -        r26 = frame_addr + offsetof(struct target_sigframe, retcode);
> +        r26 = frame_addr + offsetof(struct target_rt_sigframe, retcode);
>      }
>  
>      if (err) {
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 1/2] linux-user/alpha: Fix rt sigframe return

[Philippe Mathieu-Daudé]

On 4/22/21 8:08 PM, Richard Henderson wrote:
> We incorrectly used the offset of the non-rt sigframe.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/alpha/signal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 2/2] linux-user: Clean up sigaction ka_restorer

[Richard Henderson]

Pass the ka_restorer value as an argument to do_sigaction,
and put it into the sigaction table.

Drop the separate TARGET_ALPHA struct target_rt_sigaction
and define TARGET_ARCH_HAS_KA_RESTORER.

Tidy up TARGET_NR_rt_sigaction, merging TARGET_ALPHA, and
TARGET_SPARC into common code.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall_defs.h | 17 +++-----
 linux-user/alpha/signal.c |  8 ++--
 linux-user/signal.c       |  4 +-
 linux-user/syscall.c      | 90 ++++++++++++---------------------------
 4 files changed, 41 insertions(+), 78 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 25be414727..226379fc9b 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -492,7 +492,7 @@ void target_to_host_old_sigset(sigset_t *sigset,
                                const abi_ulong *old_sigset);
 struct target_sigaction;
 int do_sigaction(int sig, const struct target_sigaction *act,
-                 struct target_sigaction *oact);
+                 struct target_sigaction *oact, abi_ulong ka_restorer);
 
 #include "target_signal.h"
 
@@ -507,19 +507,16 @@ struct target_old_sigaction {
     int32_t sa_flags;
 };
 
-struct target_rt_sigaction {
-    abi_ulong _sa_handler;
-    abi_ulong sa_flags;
-    target_sigset_t sa_mask;
-};
-
-/* This is the struct used inside the kernel.  The ka_restorer
-   field comes from the 5th argument to sys_rt_sigaction.  */
+#define TARGET_ARCH_HAS_KA_RESTORER 1
 struct target_sigaction {
     abi_ulong _sa_handler;
     abi_ulong sa_flags;
     target_sigset_t sa_mask;
-    abi_ulong sa_restorer;
+    /*
+     * This is used inside the kernel.  The ka_restorer field comes
+     * from the 5th argument to sys_rt_sigaction.
+     */
+    abi_ulong ka_restorer;
 };
 #elif defined(TARGET_MIPS)
 struct target_sigaction {
diff --git a/linux-user/alpha/signal.c b/linux-user/alpha/signal.c
index 86f5d2276d..3aa4b339a4 100644
--- a/linux-user/alpha/signal.c
+++ b/linux-user/alpha/signal.c
@@ -138,8 +138,8 @@ void setup_frame(int sig, struct target_sigaction *ka,
 
     setup_sigcontext(&frame->sc, env, frame_addr, set);
 
-    if (ka->sa_restorer) {
-        r26 = ka->sa_restorer;
+    if (ka->ka_restorer) {
+        r26 = ka->ka_restorer;
     } else {
         __put_user(INSN_MOV_R30_R16, &frame->retcode[0]);
         __put_user(INSN_LDI_R0 + TARGET_NR_sigreturn,
@@ -192,8 +192,8 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
         __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
     }
 
-    if (ka->sa_restorer) {
-        r26 = ka->sa_restorer;
+    if (ka->ka_restorer) {
+        r26 = ka->ka_restorer;
     } else {
         __put_user(INSN_MOV_R30_R16, &frame->retcode[0]);
         __put_user(INSN_LDI_R0 + TARGET_NR_rt_sigreturn,
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 7eecec46c4..de97cb8829 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -830,7 +830,7 @@ out:
 
 /* do_sigaction() return target values and host errnos */
 int do_sigaction(int sig, const struct target_sigaction *act,
-                 struct target_sigaction *oact)
+                 struct target_sigaction *oact, abi_ulong ka_restorer)
 {
     struct target_sigaction *k;
     struct sigaction act1;
@@ -863,6 +863,8 @@ int do_sigaction(int sig, const struct target_sigaction *act,
         __get_user(k->sa_flags, &act->sa_flags);
 #ifdef TARGET_ARCH_HAS_SA_RESTORER
         __get_user(k->sa_restorer, &act->sa_restorer);
+#elif defined(TARGET_ARCH_HAS_KA_RESTORER)
+        k->ka_restorer = ka_restorer;
 #endif
         /* To be swapped in target_to_host_sigset.  */
         k->sa_mask = act->sa_mask;
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 95d79ddc43..7c36650cbc 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8984,19 +8984,20 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
             struct target_sigaction act, oact, *pact = 0;
             struct target_old_sigaction *old_act;
             if (arg2) {
-                if (!lock_user_struct(VERIFY_READ, old_act, arg2, 1))
+                if (!lock_user_struct(VERIFY_READ, old_act, arg2, 1)) {
                     return -TARGET_EFAULT;
+                }
                 act._sa_handler = old_act->_sa_handler;
                 target_siginitset(&act.sa_mask, old_act->sa_mask);
                 act.sa_flags = old_act->sa_flags;
-                act.sa_restorer = 0;
                 unlock_user_struct(old_act, arg2, 0);
                 pact = &act;
             }
-            ret = get_errno(do_sigaction(arg1, pact, &oact));
+            ret = get_errno(do_sigaction(arg1, pact, &oact, 0));
             if (!is_error(ret) && arg3) {
-                if (!lock_user_struct(VERIFY_WRITE, old_act, arg3, 0))
+                if (!lock_user_struct(VERIFY_WRITE, old_act, arg3, 0)) {
                     return -TARGET_EFAULT;
+                }
                 old_act->_sa_handler = oact._sa_handler;
                 old_act->sa_mask = oact.sa_mask.sig[0];
                 old_act->sa_flags = oact.sa_flags;
@@ -9017,7 +9018,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
 		pact = NULL;
 	    }
 
-	    ret = get_errno(do_sigaction(arg1, pact, &oact));
+	    ret = get_errno(do_sigaction(arg1, pact, &oact, 0));
 
 	    if (!is_error(ret) && arg3) {
                 if (!lock_user_struct(VERIFY_WRITE, old_act, arg3, 0))
@@ -9040,15 +9041,12 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 target_siginitset(&act.sa_mask, old_act->sa_mask);
                 act.sa_flags = old_act->sa_flags;
                 act.sa_restorer = old_act->sa_restorer;
-#ifdef TARGET_ARCH_HAS_KA_RESTORER
-                act.ka_restorer = 0;
-#endif
                 unlock_user_struct(old_act, arg2, 0);
                 pact = &act;
             } else {
                 pact = NULL;
             }
-            ret = get_errno(do_sigaction(arg1, pact, &oact));
+            ret = get_errno(do_sigaction(arg1, pact, &oact, 0));
             if (!is_error(ret) && arg3) {
                 if (!lock_user_struct(VERIFY_WRITE, old_act, arg3, 0))
                     return -TARGET_EFAULT;
@@ -9064,77 +9062,43 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
 #endif
     case TARGET_NR_rt_sigaction:
         {
-#if defined(TARGET_ALPHA)
-            /* For Alpha and SPARC this is a 5 argument syscall, with
+            /*
+             * For Alpha and SPARC this is a 5 argument syscall, with
              * a 'restorer' parameter which must be copied into the
              * sa_restorer field of the sigaction struct.
              * For Alpha that 'restorer' is arg5; for SPARC it is arg4,
              * and arg5 is the sigsetsize.
-             * Alpha also has a separate rt_sigaction struct that it uses
-             * here; SPARC uses the usual sigaction struct.
              */
-            struct target_rt_sigaction *rt_act;
-            struct target_sigaction act, oact, *pact = 0;
-
-            if (arg4 != sizeof(target_sigset_t)) {
-                return -TARGET_EINVAL;
-            }
-            if (arg2) {
-                if (!lock_user_struct(VERIFY_READ, rt_act, arg2, 1))
-                    return -TARGET_EFAULT;
-                act._sa_handler = rt_act->_sa_handler;
-                act.sa_mask = rt_act->sa_mask;
-                act.sa_flags = rt_act->sa_flags;
-                act.sa_restorer = arg5;
-                unlock_user_struct(rt_act, arg2, 0);
-                pact = &act;
-            }
-            ret = get_errno(do_sigaction(arg1, pact, &oact));
-            if (!is_error(ret) && arg3) {
-                if (!lock_user_struct(VERIFY_WRITE, rt_act, arg3, 0))
-                    return -TARGET_EFAULT;
-                rt_act->_sa_handler = oact._sa_handler;
-                rt_act->sa_mask = oact.sa_mask;
-                rt_act->sa_flags = oact.sa_flags;
-                unlock_user_struct(rt_act, arg3, 1);
-            }
-#else
-#ifdef TARGET_SPARC
+#if defined(TARGET_ALPHA)
+            target_ulong sigsetsize = arg4;
+            target_ulong restorer = arg5;
+#elif defined (TARGET_SPARC)
             target_ulong restorer = arg4;
             target_ulong sigsetsize = arg5;
 #else
             target_ulong sigsetsize = arg4;
+            target_ulong restorer = 0;
 #endif
-            struct target_sigaction *act;
-            struct target_sigaction *oact;
+            struct target_sigaction *act = NULL;
+            struct target_sigaction *oact = NULL;
 
             if (sigsetsize != sizeof(target_sigset_t)) {
                 return -TARGET_EINVAL;
             }
-            if (arg2) {
-                if (!lock_user_struct(VERIFY_READ, act, arg2, 1)) {
-                    return -TARGET_EFAULT;
-                }
-#ifdef TARGET_ARCH_HAS_KA_RESTORER
-                act->ka_restorer = restorer;
-#endif
-            } else {
-                act = NULL;
+            if (arg2 && !lock_user_struct(VERIFY_READ, act, arg2, 1)) {
+                return -TARGET_EFAULT;
             }
-            if (arg3) {
-                if (!lock_user_struct(VERIFY_WRITE, oact, arg3, 0)) {
-                    ret = -TARGET_EFAULT;
-                    goto rt_sigaction_fail;
+            if (arg3 && !lock_user_struct(VERIFY_WRITE, oact, arg3, 0)) {
+                ret = -TARGET_EFAULT;
+            } else {
+                ret = get_errno(do_sigaction(arg1, act, oact, restorer));
+                if (oact) {
+                    unlock_user_struct(oact, arg3, 1);
                 }
-            } else
-                oact = NULL;
-            ret = get_errno(do_sigaction(arg1, act, oact));
-	rt_sigaction_fail:
-            if (act)
+            }
+            if (act) {
                 unlock_user_struct(act, arg2, 0);
-            if (oact)
-                unlock_user_struct(oact, arg3, 1);
-#endif
+            }
         }
         return ret;
 #ifdef TARGET_NR_sgetmask /* not on alpha */
-- 
2.25.1

Re: [PATCH 2/2] linux-user: Clean up sigaction ka_restorer

[Richard Henderson]

On 4/22/21 11:08 AM, Richard Henderson wrote:
> Pass the ka_restorer value as an argument to do_sigaction,
> and put it into the sigaction table.
> 
> Drop the separate TARGET_ALPHA struct target_rt_sigaction
> and define TARGET_ARCH_HAS_KA_RESTORER.
> 
> Tidy up TARGET_NR_rt_sigaction, merging TARGET_ALPHA, and
> TARGET_SPARC into common code.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

I'm going to split this patch further.
I should be able to clean up alpha further as well.


r~

Re: [PATCH 0/2] linux-user: sigaction fixes/cleanups

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210422180819.252121-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210422180819.252121-1-richard.henderson@linaro.org
Subject: [PATCH 0/2] linux-user: sigaction fixes/cleanups

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210422154427.13038-1-alex.bennee@linaro.org -> patchew/20210422154427.13038-1-alex.bennee@linaro.org
 * [new tag]         patchew/20210422180819.252121-1-richard.henderson@linaro.org -> patchew/20210422180819.252121-1-richard.henderson@linaro.org
Switched to a new branch 'test'
0bea0eb linux-user: Clean up sigaction ka_restorer
edde716 linux-user/alpha: Fix rt sigframe return

=== OUTPUT BEGIN ===
1/2 Checking commit edde716f548d (linux-user/alpha: Fix rt sigframe return)
2/2 Checking commit 0bea0eb16300 (linux-user: Clean up sigaction ka_restorer)
ERROR: code indent should never use tabs
#107: FILE: linux-user/syscall.c:9021:
+^I    ret = get_errno(do_sigaction(arg1, pact, &oact, 0));$

ERROR: space prohibited between function name and open parenthesis '('
#173: FILE: linux-user/syscall.c:9075:
+#elif defined (TARGET_SPARC)

total: 2 errors, 0 warnings, 214 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210422180819.252121-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com