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

[PATCH v2 0/7] 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.

Sparc had another bug, where the ka_restorer field was
written to user memory.

Version 2 splits patch 2 into 6.


r~


Richard Henderson (7):
  linux-user/alpha: Fix rt sigframe return
  linux-user/alpha: Rename the sigaction restorer field
  linux-user: Pass ka_restorer to do_sigaction
  linux-user: Honor TARGET_ARCH_HAS_SA_RESTORER in do_syscall
  linux-user/alpha: Define TARGET_ARCH_HAS_KA_RESTORER
  linux-user/alpha: Share code for TARGET_NR_sigaction
  linux-user: Tidy TARGET_NR_rt_sigaction

 linux-user/alpha/target_signal.h |   1 +
 linux-user/syscall_defs.h        |  29 ++-------
 linux-user/alpha/signal.c        |  10 +--
 linux-user/signal.c              |   5 +-
 linux-user/syscall.c             | 107 ++++++++-----------------------
 5 files changed, 43 insertions(+), 109 deletions(-)

-- 
2.25.1

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

[Richard Henderson]

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

Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
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 v2 1/7] linux-user/alpha: Fix rt sigframe return

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> We incorrectly used the offset of the non-rt sigframe.
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH v2 2/7] linux-user/alpha: Rename the sigaction restorer field

[Richard Henderson]

Use ka_restorer, in line with TARGET_ARCH_HAS_KA_RESTORER
vs TARGET_ARCH_HAS_SA_RESTORER, since Alpha passes this
field as a syscall argument.

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

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 25be414727..693d4f3788 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -519,7 +519,7 @@ struct target_sigaction {
     abi_ulong _sa_handler;
     abi_ulong sa_flags;
     target_sigset_t sa_mask;
-    abi_ulong sa_restorer;
+    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/syscall.c b/linux-user/syscall.c
index 95d79ddc43..ee21eb5e6f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8989,7 +8989,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 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;
+                act.ka_restorer = 0;
                 unlock_user_struct(old_act, arg2, 0);
                 pact = &act;
             }
@@ -9085,7 +9085,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 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;
+                act.ka_restorer = arg5;
                 unlock_user_struct(rt_act, arg2, 0);
                 pact = &act;
             }
-- 
2.25.1

Re: [PATCH v2 2/7] linux-user/alpha: Rename the sigaction restorer field

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Use ka_restorer, in line with TARGET_ARCH_HAS_KA_RESTORER
> vs TARGET_ARCH_HAS_SA_RESTORER, since Alpha passes this
> field as a syscall argument.

I'm still slightly confused - but that's to be expected from signals :-/

Anyway I understand that the SA_RESTORER points to the vdso trampoline
(at least according to man sigreturn). What is ka_restorer - if this the
in sigframe restorer?

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/syscall_defs.h | 2 +-
>  linux-user/alpha/signal.c | 8 ++++----
>  linux-user/syscall.c      | 4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 25be414727..693d4f3788 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -519,7 +519,7 @@ struct target_sigaction {
>      abi_ulong _sa_handler;
>      abi_ulong sa_flags;
>      target_sigset_t sa_mask;
> -    abi_ulong sa_restorer;
> +    abi_ulong ka_restorer;
>  };

Maybe this is something we could expand a little on in the difference
between the two here (or maybe in the later commit?).


>  #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/syscall.c b/linux-user/syscall.c
> index 95d79ddc43..ee21eb5e6f 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8989,7 +8989,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>                  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;
> +                act.ka_restorer = 0;
>                  unlock_user_struct(old_act, arg2, 0);
>                  pact = &act;
>              }
> @@ -9085,7 +9085,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>                  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;
> +                act.ka_restorer = arg5;
>                  unlock_user_struct(rt_act, arg2, 0);
>                  pact = &act;
>              }

Otherwise looks fine to me:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH v2 2/7] linux-user/alpha: Rename the sigaction restorer field

[Richard Henderson]

On 4/23/21 3:16 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> Use ka_restorer, in line with TARGET_ARCH_HAS_KA_RESTORER
>> vs TARGET_ARCH_HAS_SA_RESTORER, since Alpha passes this
>> field as a syscall argument.
> 
> I'm still slightly confused - but that's to be expected from signals :-/
> 
> Anyway I understand that the SA_RESTORER points to the vdso trampoline
> (at least according to man sigreturn). What is ka_restorer - if this the
> in sigframe restorer?

Both sa_restorer and ka_restorer pre-date the vdso trampoline.  They allow libc 
to register a trampoline itself.

The difference is that sa_restorer is a field in struct sigaction, and 
ka_restorer is an extra argument to the sigaction syscall.  Different targets 
used different approaches.

>> -    abi_ulong sa_restorer;
>> +    abi_ulong ka_restorer;
>>   };
> 
> Maybe this is something we could expand a little on in the difference
> between the two here (or maybe in the later commit?).

Perhaps elsewhere.  This change is simply to make alpha line up with the 
generic code that will replace it.


r~

[PATCH v2 3/7] linux-user: Pass ka_restorer to do_sigaction

[Richard Henderson]

The value of ka_restorer needs to be saved in sigact_table.
At the moment, the attempt to save it in do_syscall is
improperly clobbering user memory.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall_defs.h |  2 +-
 linux-user/signal.c       |  5 ++++-
 linux-user/syscall.c      | 19 ++++++-------------
 3 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 693d4f3788..e4aaf8412f 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"
 
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 7eecec46c4..44a5012930 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,9 @@ 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);
+#endif
+#ifdef 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 ee21eb5e6f..36169a0ded 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8989,11 +8989,10 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 act._sa_handler = old_act->_sa_handler;
                 target_siginitset(&act.sa_mask, old_act->sa_mask);
                 act.sa_flags = old_act->sa_flags;
-                act.ka_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))
                     return -TARGET_EFAULT;
@@ -9017,7 +9016,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 +9039,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;
@@ -9085,11 +9081,10 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 act._sa_handler = rt_act->_sa_handler;
                 act.sa_mask = rt_act->sa_mask;
                 act.sa_flags = rt_act->sa_flags;
-                act.ka_restorer = arg5;
                 unlock_user_struct(rt_act, arg2, 0);
                 pact = &act;
             }
-            ret = get_errno(do_sigaction(arg1, pact, &oact));
+            ret = get_errno(do_sigaction(arg1, pact, &oact, arg5));
             if (!is_error(ret) && arg3) {
                 if (!lock_user_struct(VERIFY_WRITE, rt_act, arg3, 0))
                     return -TARGET_EFAULT;
@@ -9104,6 +9099,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
             target_ulong sigsetsize = arg5;
 #else
             target_ulong sigsetsize = arg4;
+            target_ulong restorer = 0;
 #endif
             struct target_sigaction *act;
             struct target_sigaction *oact;
@@ -9115,9 +9111,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 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;
             }
@@ -9128,7 +9121,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 }
             } else
                 oact = NULL;
-            ret = get_errno(do_sigaction(arg1, act, oact));
+            ret = get_errno(do_sigaction(arg1, act, oact, restorer));
 	rt_sigaction_fail:
             if (act)
                 unlock_user_struct(act, arg2, 0);
-- 
2.25.1

Re: [PATCH v2 3/7] linux-user: Pass ka_restorer to do_sigaction

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> The value of ka_restorer needs to be saved in sigact_table.

It would be nice to add a comment to sigact_table to describe what it
is. This is some of the oldest code in the tree so there seems to be a
fair bit of implicit knowledge is the way things are done.

> At the moment, the attempt to save it in do_syscall is
> improperly clobbering user memory.

Otherwise to my slightly confused eyes it looks pretty sane:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH v2 4/7] linux-user: Honor TARGET_ARCH_HAS_SA_RESTORER in do_syscall

[Richard Henderson]

Do not access a field that may not be present.  This will
become an issue when sharing more code in the next patch.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 36169a0ded..89d641856c 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9038,7 +9038,9 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 act._sa_handler = old_act->_sa_handler;
                 target_siginitset(&act.sa_mask, old_act->sa_mask);
                 act.sa_flags = old_act->sa_flags;
+#ifdef TARGET_ARCH_HAS_SA_RESTORER
                 act.sa_restorer = old_act->sa_restorer;
+#endif
                 unlock_user_struct(old_act, arg2, 0);
                 pact = &act;
             } else {
@@ -9051,7 +9053,9 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 old_act->_sa_handler = oact._sa_handler;
                 old_act->sa_mask = oact.sa_mask.sig[0];
                 old_act->sa_flags = oact.sa_flags;
+#ifdef TARGET_ARCH_HAS_SA_RESTORER
                 old_act->sa_restorer = oact.sa_restorer;
+#endif
                 unlock_user_struct(old_act, arg3, 1);
             }
 #endif
-- 
2.25.1

Re: [PATCH v2 4/7] linux-user: Honor TARGET_ARCH_HAS_SA_RESTORER in do_syscall

[Philippe Mathieu-Daudé]

On 4/23/21 1:02 AM, Richard Henderson wrote:
> Do not access a field that may not be present.  This will
> become an issue when sharing more code in the next patch.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/syscall.c | 4 ++++
>  1 file changed, 4 insertions(+)

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

Re: [PATCH v2 4/7] linux-user: Honor TARGET_ARCH_HAS_SA_RESTORER in do_syscall

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Do not access a field that may not be present.  This will
> become an issue when sharing more code in the next patch.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH v2 5/7] linux-user/alpha: Define TARGET_ARCH_HAS_KA_RESTORER

[Richard Henderson]

This means that we can share the TARGET_NR_rt_sigaction code,
and the target_rt_sigaction structure is unused.  Untangling
the ifdefs so that target_sigaction can be shared will wait
until the next patch.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/alpha/target_signal.h |  1 +
 linux-user/syscall_defs.h        |  6 ------
 linux-user/syscall.c             | 37 ++++++--------------------------
 3 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/linux-user/alpha/target_signal.h b/linux-user/alpha/target_signal.h
index 0b90d3a897..250642913e 100644
--- a/linux-user/alpha/target_signal.h
+++ b/linux-user/alpha/target_signal.h
@@ -92,6 +92,7 @@ typedef struct target_sigaltstack {
 #define TARGET_GEN_SUBRNG7     -25
 
 #define TARGET_ARCH_HAS_SETUP_FRAME
+#define TARGET_ARCH_HAS_KA_RESTORER
 
 /* bit-flags */
 #define TARGET_SS_AUTODISARM (1U << 31) /* disable sas during sighandling */
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index e4aaf8412f..7a1d3b239c 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -507,12 +507,6 @@ 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.  */
 struct target_sigaction {
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 89d641856c..216ee4ca47 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9064,41 +9064,17 @@ 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;
-                unlock_user_struct(rt_act, arg2, 0);
-                pact = &act;
-            }
-            ret = get_errno(do_sigaction(arg1, pact, &oact, arg5));
-            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
@@ -9131,7 +9107,6 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                 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 v2 5/7] linux-user/alpha: Define TARGET_ARCH_HAS_KA_RESTORER

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> This means that we can share the TARGET_NR_rt_sigaction code,
> and the target_rt_sigaction structure is unused.  Untangling
> the ifdefs so that target_sigaction can be shared will wait
> until the next patch.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

[PATCH v2 6/7] linux-user/alpha: Share code for TARGET_NR_sigaction

[Richard Henderson]

There's no longer a difference between the alpha code and
the generic code.

There is a type difference in target_old_sigaction.sa_flags,
which can be resolved with a very much smaller ifdef, which
allows us to finish sharing the target_sigaction definition.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall_defs.h | 21 ++++++---------------
 linux-user/syscall.c      | 23 +----------------------
 2 files changed, 7 insertions(+), 37 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index 7a1d3b239c..18b031a2f6 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -501,21 +501,12 @@ int do_sigaction(int sig, const struct target_sigaction *act,
 #endif
 
 #if defined(TARGET_ALPHA)
-struct target_old_sigaction {
-    abi_ulong _sa_handler;
-    abi_ulong sa_mask;
-    int32_t sa_flags;
-};
+typedef int32_t target_old_sa_flags;
+#else
+typedef abi_ulong target_old_sa_flags;
+#endif
 
-/* This is the struct used inside the kernel.  The ka_restorer
-   field comes from the 5th argument to sys_rt_sigaction.  */
-struct target_sigaction {
-    abi_ulong _sa_handler;
-    abi_ulong sa_flags;
-    target_sigset_t sa_mask;
-    abi_ulong ka_restorer;
-};
-#elif defined(TARGET_MIPS)
+#if defined(TARGET_MIPS)
 struct target_sigaction {
 	uint32_t	sa_flags;
 #if defined(TARGET_ABI_MIPSN32)
@@ -533,7 +524,7 @@ struct target_sigaction {
 struct target_old_sigaction {
         abi_ulong _sa_handler;
         abi_ulong sa_mask;
-        abi_ulong sa_flags;
+        target_old_sa_flags sa_flags;
 #ifdef TARGET_ARCH_HAS_SA_RESTORER
         abi_ulong sa_restorer;
 #endif
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 216ee4ca47..9bcd485423 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8980,28 +8980,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
 #ifdef TARGET_NR_sigaction
     case TARGET_NR_sigaction:
         {
-#if defined(TARGET_ALPHA)
-            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))
-                    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;
-                unlock_user_struct(old_act, arg2, 0);
-                pact = &act;
-            }
-            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;
-                old_act->_sa_handler = oact._sa_handler;
-                old_act->sa_mask = oact.sa_mask.sig[0];
-                old_act->sa_flags = oact.sa_flags;
-                unlock_user_struct(old_act, arg3, 1);
-            }
-#elif defined(TARGET_MIPS)
+#if defined(TARGET_MIPS)
 	    struct target_sigaction act, oact, *pact, *old_act;
 
 	    if (arg2) {
-- 
2.25.1

Re: [PATCH v2 6/7] linux-user/alpha: Share code for TARGET_NR_sigaction

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> There's no longer a difference between the alpha code and
> the generic code.
>
> There is a type difference in target_old_sigaction.sa_flags,
> which can be resolved with a very much smaller ifdef, which
> allows us to finish sharing the target_sigaction definition.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

Re: [PATCH v2 6/7] linux-user/alpha: Share code for TARGET_NR_sigaction

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> There's no longer a difference between the alpha code and
> the generic code.
>
> There is a type difference in target_old_sigaction.sa_flags,
> which can be resolved with a very much smaller ifdef, which
> allows us to finish sharing the target_sigaction definition.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


-- 
Alex Bennée

[PATCH v2 7/7] linux-user: Tidy TARGET_NR_rt_sigaction

[Richard Henderson]

Initialize variables instead of elses.
Use an else instead of a goto.
Add braces.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9bcd485423..c7c3257f40 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9060,32 +9060,26 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
             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;
-                }
-            } 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, restorer));
-	rt_sigaction_fail:
-            if (act)
+            }
+            if (act) {
                 unlock_user_struct(act, arg2, 0);
-            if (oact)
-                unlock_user_struct(oact, arg3, 1);
+            }
         }
         return ret;
 #ifdef TARGET_NR_sgetmask /* not on alpha */
-- 
2.25.1

Re: [PATCH v2 7/7] linux-user: Tidy TARGET_NR_rt_sigaction

[Philippe Mathieu-Daudé]

On 4/23/21 1:02 AM, Richard Henderson wrote:
> Initialize variables instead of elses.
> Use an else instead of a goto.
> Add braces.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/syscall.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)

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

Re: [PATCH v2 7/7] linux-user: Tidy TARGET_NR_rt_sigaction

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> Initialize variables instead of elses.
> Use an else instead of a goto.
> Add braces.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/syscall.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9bcd485423..c7c3257f40 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9060,32 +9060,26 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>              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;
> -                }
> -            } 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);
>                  }

This does make me idly wonder if there is a way to handle unlocking in a
similar way to our autofree and LOCK_GUARD stuff. But that's not getting
in the way of approving of this improvement.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210422230227.314751-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: 20210422230227.314751-1-richard.henderson@linaro.org
Subject: [PATCH v2 0/7] 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
 * [new tag]         patchew/20210422230227.314751-1-richard.henderson@linaro.org -> patchew/20210422230227.314751-1-richard.henderson@linaro.org
Switched to a new branch 'test'
9eee746 linux-user: Tidy TARGET_NR_rt_sigaction
46c2541 linux-user/alpha: Share code for TARGET_NR_sigaction
c69776b linux-user/alpha: Define TARGET_ARCH_HAS_KA_RESTORER
de9e5c2 linux-user: Honor TARGET_ARCH_HAS_SA_RESTORER in do_syscall
57bd960 linux-user: Pass ka_restorer to do_sigaction
ef4054e linux-user/alpha: Rename the sigaction restorer field
df4fac9 linux-user/alpha: Fix rt sigframe return

=== OUTPUT BEGIN ===
1/7 Checking commit df4fac977c4c (linux-user/alpha: Fix rt sigframe return)
2/7 Checking commit ef4054e42574 (linux-user/alpha: Rename the sigaction restorer field)
3/7 Checking commit 57bd9604ef18 (linux-user: Pass ka_restorer to do_sigaction)
ERROR: code indent should never use tabs
#64: FILE: linux-user/syscall.c:9019:
+^I    ret = get_errno(do_sigaction(arg1, pact, &oact, 0));$

total: 1 errors, 0 warnings, 97 lines checked

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

4/7 Checking commit de9e5c267f85 (linux-user: Honor TARGET_ARCH_HAS_SA_RESTORER in do_syscall)
5/7 Checking commit c69776bf2b07 (linux-user/alpha: Define TARGET_ARCH_HAS_KA_RESTORER)
6/7 Checking commit 46c2541b617d (linux-user/alpha: Share code for TARGET_NR_sigaction)
7/7 Checking commit 9eee7464d318 (linux-user: Tidy TARGET_NR_rt_sigaction)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210422230227.314751-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

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

[Laurent Vivier]

Le 23/04/2021 à 01:02, Richard Henderson a écrit :
> 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.
> 
> Sparc had another bug, where the ka_restorer field was
> written to user memory.
> 
> Version 2 splits patch 2 into 6.
> 
> 
> r~
> 
> 
> Richard Henderson (7):
>   linux-user/alpha: Fix rt sigframe return
>   linux-user/alpha: Rename the sigaction restorer field
>   linux-user: Pass ka_restorer to do_sigaction
>   linux-user: Honor TARGET_ARCH_HAS_SA_RESTORER in do_syscall
>   linux-user/alpha: Define TARGET_ARCH_HAS_KA_RESTORER
>   linux-user/alpha: Share code for TARGET_NR_sigaction
>   linux-user: Tidy TARGET_NR_rt_sigaction
> 
>  linux-user/alpha/target_signal.h |   1 +
>  linux-user/syscall_defs.h        |  29 ++-------
>  linux-user/alpha/signal.c        |  10 +--
>  linux-user/signal.c              |   5 +-
>  linux-user/syscall.c             | 107 ++++++++-----------------------
>  5 files changed, 43 insertions(+), 109 deletions(-)
> 


Applied to my linux-user-for-6.1 branch.

Thanks,
Laurent