[Qemu-devel] [PATCH 0/4] linux-user: fix various SIGSEGV delivery bugs

[Qemu-devel] [PATCH 0/4] linux-user: fix various SIGSEGV delivery bugs

[Peter Maydell]

This patchset fixes most of the problems with delivering SIGSEGV
noted in https://bugs.launchpad.net/qemu/+bug/1705118:
 * s390x is missing masking operations on the fault address
   passed to the guest in si_addr
 * ppc is passing the PC of the offending insn, not the data
   address, into si_addr
 * sparc fails to record the address of a data fault and so
   passes 0 into si_addr
 * sparc isn't implementing rt_sigaction correctly (it has an
   extra 'restorer' argument, similar to alpha)

It doesn't actually get the sparc testcase from that bug working,
though, because SPARC is entirely missing support for setup_rt_frame()
and do_rt_sigreturn(), so it can't deliver RT signals.

Implementing RT signal delivery is too big a bugfix for an afternoon,
so here are the simpler parts. I'll leave the signal delivery part
to somebody who cares about SPARC guests...

thanks
-- PMM

Peter Maydell (4):
  linux-user/s390x: Mask si_addr for SIGSEGV
  linux-user/ppc: Report correct fault address for data faults
  linux-user/sparc: Put address for data faults where linux-user expects
    it
  linux-user: Handle rt_sigaction correctly for SPARC

 linux-user/main.c         |  8 ++++++--
 linux-user/syscall.c      | 27 +++++++++++++++++++++++----
 target/sparc/mmu_helper.c |  8 ++++++++
 3 files changed, 37 insertions(+), 6 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] linux-user/s390x: Mask si_addr for SIGSEGV

[Peter Maydell]

For s390x, the address passed to a signal handler in the
siginfo_t si_addr field is masked (in the kernel this is done in
do_sigbus() and do_sigsegv() in arch/s390/mm/fault.c). Implement
this architecture-specific oddity in linux-user.

This is one of the issues described in
https://bugs.launchpad.net/qemu/+bug/1705118

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index aa02f25..b6dd9ef 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -3238,6 +3238,10 @@ void cpu_loop(CPUAlphaState *env)
 #endif /* TARGET_ALPHA */
 
 #ifdef TARGET_S390X
+
+/* s390x masks the fault address it reports in si_addr for SIGSEGV and SIGBUS */
+#define S390X_FAIL_ADDR_MASK -4096LL
+
 void cpu_loop(CPUS390XState *env)
 {
     CPUState *cs = CPU(s390_env_get_cpu(env));
@@ -3294,7 +3298,7 @@ void cpu_loop(CPUS390XState *env)
                 sig = TARGET_SIGSEGV;
                 /* XXX: check env->error_code */
                 n = TARGET_SEGV_MAPERR;
-                addr = env->__excp_addr;
+                addr = env->__excp_addr & S390X_FAIL_ADDR_MASK;
                 goto do_signal;
             case PGM_EXECUTE:
             case PGM_SPECIFICATION:
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/4] linux-user/s390x: Mask si_addr for SIGSEGV

[Laurent Vivier]

Le 06/11/2017 à 19:33, Peter Maydell a écrit :
> For s390x, the address passed to a signal handler in the
> siginfo_t si_addr field is masked (in the kernel this is done in
> do_sigbus() and do_sigsegv() in arch/s390/mm/fault.c). Implement
> this architecture-specific oddity in linux-user.
> 
> This is one of the issues described in
> https://bugs.launchpad.net/qemu/+bug/1705118
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index aa02f25..b6dd9ef 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3238,6 +3238,10 @@ void cpu_loop(CPUAlphaState *env)
>  #endif /* TARGET_ALPHA */
>  
>  #ifdef TARGET_S390X
> +
> +/* s390x masks the fault address it reports in si_addr for SIGSEGV and SIGBUS */
> +#define S390X_FAIL_ADDR_MASK -4096LL
> +
>  void cpu_loop(CPUS390XState *env)
>  {
>      CPUState *cs = CPU(s390_env_get_cpu(env));
> @@ -3294,7 +3298,7 @@ void cpu_loop(CPUS390XState *env)
>                  sig = TARGET_SIGSEGV;
>                  /* XXX: check env->error_code */
>                  n = TARGET_SEGV_MAPERR;
> -                addr = env->__excp_addr;
> +                addr = env->__excp_addr & S390X_FAIL_ADDR_MASK;
>                  goto do_signal;
>              case PGM_EXECUTE:
>              case PGM_SPECIFICATION:
> 

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

Re: [Qemu-devel] [PATCH 1/4] linux-user/s390x: Mask si_addr for SIGSEGV

[Philippe Mathieu-Daudé]

On 11/06/2017 03:33 PM, Peter Maydell wrote:
> For s390x, the address passed to a signal handler in the
> siginfo_t si_addr field is masked (in the kernel this is done in
> do_sigbus() and do_sigsegv() in arch/s390/mm/fault.c). Implement
> this architecture-specific oddity in linux-user.
> 
> This is one of the issues described in
> https://bugs.launchpad.net/qemu/+bug/1705118
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  linux-user/main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index aa02f25..b6dd9ef 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3238,6 +3238,10 @@ void cpu_loop(CPUAlphaState *env)
>  #endif /* TARGET_ALPHA */
>  
>  #ifdef TARGET_S390X
> +
> +/* s390x masks the fault address it reports in si_addr for SIGSEGV and SIGBUS */
> +#define S390X_FAIL_ADDR_MASK -4096LL
> +
>  void cpu_loop(CPUS390XState *env)
>  {
>      CPUState *cs = CPU(s390_env_get_cpu(env));
> @@ -3294,7 +3298,7 @@ void cpu_loop(CPUS390XState *env)
>                  sig = TARGET_SIGSEGV;
>                  /* XXX: check env->error_code */
>                  n = TARGET_SEGV_MAPERR;
> -                addr = env->__excp_addr;
> +                addr = env->__excp_addr & S390X_FAIL_ADDR_MASK;
>                  goto do_signal;
>              case PGM_EXECUTE:
>              case PGM_SPECIFICATION:
> 

Re: [Qemu-devel] [PATCH 1/4] linux-user/s390x: Mask si_addr for SIGSEGV

[Richard Henderson]

On 11/06/2017 07:33 PM, Peter Maydell wrote:
> For s390x, the address passed to a signal handler in the
> siginfo_t si_addr field is masked (in the kernel this is done in
> do_sigbus() and do_sigsegv() in arch/s390/mm/fault.c). Implement
> this architecture-specific oddity in linux-user.
> 
> This is one of the issues described in
> https://bugs.launchpad.net/qemu/+bug/1705118
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Accurate, but really seems like a s390x kernel bug.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 1/4] linux-user/s390x: Mask si_addr for SIGSEGV

[Peter Maydell]

On 8 November 2017 at 21:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 11/06/2017 07:33 PM, Peter Maydell wrote:
>> For s390x, the address passed to a signal handler in the
>> siginfo_t si_addr field is masked (in the kernel this is done in
>> do_sigbus() and do_sigsegv() in arch/s390/mm/fault.c). Implement
>> this architecture-specific oddity in linux-user.
>>
>> This is one of the issues described in
>> https://bugs.launchpad.net/qemu/+bug/1705118
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  linux-user/main.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> Accurate, but really seems like a s390x kernel bug.

The kernel code goes out of its way to do the masking, so if it's
a bug presumably it's a "retain back compat with some older
bug" thing...

thanks
-- PMM

[Qemu-devel] [PATCH 2/4] linux-user/ppc: Report correct fault address for data faults

[Peter Maydell]

For faults on loads and stores, ppc_cpu_handle_mmu_fault() in
target/ppc/user_only_helper.c stores the offending address
in env->spr[SPR_DAR]. Report this correctly to the guest
in si_addr, rather than incorrectly using the address of the
instruction that caused the fault.

This fixes the test case in
https://bugs.launchpad.net/qemu/+bug/1077116
for ppc, ppc64 and ppc64le.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index b6dd9ef..6286661 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -1420,7 +1420,7 @@ void cpu_loop(CPUPPCState *env)
                 info.si_code = TARGET_SEGV_MAPERR;
                 break;
             }
-            info._sifields._sigfault._addr = env->nip;
+            info._sifields._sigfault._addr = env->spr[SPR_DAR];
             queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
             break;
         case POWERPC_EXCP_ISI:      /* Instruction storage exception         */
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/4] linux-user/ppc: Report correct fault address for data faults

[Laurent Vivier]

Le 06/11/2017 à 19:33, Peter Maydell a écrit :
> For faults on loads and stores, ppc_cpu_handle_mmu_fault() in
> target/ppc/user_only_helper.c stores the offending address
> in env->spr[SPR_DAR]. Report this correctly to the guest
> in si_addr, rather than incorrectly using the address of the
> instruction that caused the fault.
> 
> This fixes the test case in
> https://bugs.launchpad.net/qemu/+bug/1077116
> for ppc, ppc64 and ppc64le.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/linux-user/main.c b/linux-user/main.c
> index b6dd9ef..6286661 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -1420,7 +1420,7 @@ void cpu_loop(CPUPPCState *env)
>                  info.si_code = TARGET_SEGV_MAPERR;
>                  break;
>              }
> -            info._sifields._sigfault._addr = env->nip;
> +            info._sifields._sigfault._addr = env->spr[SPR_DAR];
>              queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
>              break;
>          case POWERPC_EXCP_ISI:      /* Instruction storage exception         */
> 

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

Re: [Qemu-devel] [PATCH 2/4] linux-user/ppc: Report correct fault address for data faults

[Richard Henderson]

On 11/06/2017 07:33 PM, Peter Maydell wrote:
> For faults on loads and stores, ppc_cpu_handle_mmu_fault() in
> target/ppc/user_only_helper.c stores the offending address
> in env->spr[SPR_DAR]. Report this correctly to the guest
> in si_addr, rather than incorrectly using the address of the
> instruction that caused the fault.
> 
> This fixes the test case in
> https://bugs.launchpad.net/qemu/+bug/1077116
> for ppc, ppc64 and ppc64le.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 3/4] linux-user/sparc: Put address for data faults where linux-user expects it

[Peter Maydell]

In the user-mode-only version of sparc_cpu_handle_mmu_fault(),
we must save the fault address for a data fault into the CPU
state's mmu registers, because the code in linux-user/main.c
expects to find it there in order to populate the si_addr
field of the guest siginfo.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/sparc/mmu_helper.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
index 126ea5e..d5b6c1e 100644
--- a/target/sparc/mmu_helper.c
+++ b/target/sparc/mmu_helper.c
@@ -30,10 +30,18 @@
 int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw,
                                int mmu_idx)
 {
+    SPARCCPU *cpu = SPARC_CPU(cs);
+    CPUSPARCState *env = &cpu->env;
+
     if (rw & 2) {
         cs->exception_index = TT_TFAULT;
     } else {
         cs->exception_index = TT_DFAULT;
+#ifdef TARGET_SPARC64
+        env->dmmu.mmuregs[4] = address;
+#else
+        env->mmuregs[4] = address;
+#endif
     }
     return 1;
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH 3/4] linux-user/sparc: Put address for data faults where linux-user expects it

[Laurent Vivier]

Le 06/11/2017 à 19:33, Peter Maydell a écrit :
> In the user-mode-only version of sparc_cpu_handle_mmu_fault(),
> we must save the fault address for a data fault into the CPU
> state's mmu registers, because the code in linux-user/main.c
> expects to find it there in order to populate the si_addr
> field of the guest siginfo.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/mmu_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
> index 126ea5e..d5b6c1e 100644
> --- a/target/sparc/mmu_helper.c
> +++ b/target/sparc/mmu_helper.c
> @@ -30,10 +30,18 @@
>  int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw,
>                                 int mmu_idx)
>  {
> +    SPARCCPU *cpu = SPARC_CPU(cs);
> +    CPUSPARCState *env = &cpu->env;
> +
>      if (rw & 2) {
>          cs->exception_index = TT_TFAULT;
>      } else {
>          cs->exception_index = TT_DFAULT;
> +#ifdef TARGET_SPARC64
> +        env->dmmu.mmuregs[4] = address;
> +#else
> +        env->mmuregs[4] = address;
> +#endif
>      }
>      return 1;
>  }
> 

The softmmu version of sparc_cpu_handle_mmu_fault() also updates
mmuregs[3]. Is it needed for this one (for ucontext)?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 3/4] linux-user/sparc: Put address for data faults where linux-user expects it

[Peter Maydell]

On 7 November 2017 at 08:28, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 06/11/2017 à 19:33, Peter Maydell a écrit :
>> In the user-mode-only version of sparc_cpu_handle_mmu_fault(),
>> we must save the fault address for a data fault into the CPU
>> state's mmu registers, because the code in linux-user/main.c
>> expects to find it there in order to populate the si_addr
>> field of the guest siginfo.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  target/sparc/mmu_helper.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
>> index 126ea5e..d5b6c1e 100644
>> --- a/target/sparc/mmu_helper.c
>> +++ b/target/sparc/mmu_helper.c
>> @@ -30,10 +30,18 @@
>>  int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw,
>>                                 int mmu_idx)
>>  {
>> +    SPARCCPU *cpu = SPARC_CPU(cs);
>> +    CPUSPARCState *env = &cpu->env;
>> +
>>      if (rw & 2) {
>>          cs->exception_index = TT_TFAULT;
>>      } else {
>>          cs->exception_index = TT_DFAULT;
>> +#ifdef TARGET_SPARC64
>> +        env->dmmu.mmuregs[4] = address;
>> +#else
>> +        env->mmuregs[4] = address;
>> +#endif
>>      }
>>      return 1;
>>  }
>>
>
> The softmmu version of sparc_cpu_handle_mmu_fault() also updates
> mmuregs[3]. Is it needed for this one (for ucontext)?

Nothing in linux-user/ reads mmuregs[3], so I assume not.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/4] linux-user/sparc: Put address for data faults where linux-user expects it

[Laurent Vivier]

Le 06/11/2017 à 19:33, Peter Maydell a écrit :
> In the user-mode-only version of sparc_cpu_handle_mmu_fault(),
> we must save the fault address for a data fault into the CPU
> state's mmu registers, because the code in linux-user/main.c
> expects to find it there in order to populate the si_addr
> field of the guest siginfo.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/mmu_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
> index 126ea5e..d5b6c1e 100644
> --- a/target/sparc/mmu_helper.c
> +++ b/target/sparc/mmu_helper.c
> @@ -30,10 +30,18 @@
>  int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw,
>                                 int mmu_idx)
>  {
> +    SPARCCPU *cpu = SPARC_CPU(cs);
> +    CPUSPARCState *env = &cpu->env;
> +
>      if (rw & 2) {
>          cs->exception_index = TT_TFAULT;
>      } else {
>          cs->exception_index = TT_DFAULT;
> +#ifdef TARGET_SPARC64
> +        env->dmmu.mmuregs[4] = address;
> +#else
> +        env->mmuregs[4] = address;
> +#endif
>      }
>      return 1;
>  }
> 

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

Re: [Qemu-devel] [PATCH 3/4] linux-user/sparc: Put address for data faults where linux-user expects it

[Philippe Mathieu-Daudé]

On 11/06/2017 03:33 PM, Peter Maydell wrote:
> In the user-mode-only version of sparc_cpu_handle_mmu_fault(),
> we must save the fault address for a data fault into the CPU
> state's mmu registers, because the code in linux-user/main.c
> expects to find it there in order to populate the si_addr
> field of the guest siginfo.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

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

> ---
>  target/sparc/mmu_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target/sparc/mmu_helper.c b/target/sparc/mmu_helper.c
> index 126ea5e..d5b6c1e 100644
> --- a/target/sparc/mmu_helper.c
> +++ b/target/sparc/mmu_helper.c
> @@ -30,10 +30,18 @@
>  int sparc_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw,
>                                 int mmu_idx)
>  {
> +    SPARCCPU *cpu = SPARC_CPU(cs);
> +    CPUSPARCState *env = &cpu->env;
> +
>      if (rw & 2) {
>          cs->exception_index = TT_TFAULT;
>      } else {
>          cs->exception_index = TT_DFAULT;
> +#ifdef TARGET_SPARC64
> +        env->dmmu.mmuregs[4] = address;
> +#else
> +        env->mmuregs[4] = address;
> +#endif
>      }
>      return 1;
>  }
> 

Re: [Qemu-devel] [PATCH 3/4] linux-user/sparc: Put address for data faults where linux-user expects it

[Richard Henderson]

On 11/06/2017 07:33 PM, Peter Maydell wrote:
> In the user-mode-only version of sparc_cpu_handle_mmu_fault(),
> we must save the fault address for a data fault into the CPU
> state's mmu registers, because the code in linux-user/main.c
> expects to find it there in order to populate the si_addr
> field of the guest siginfo.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/sparc/mmu_helper.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[Qemu-devel] [PATCH 4/4] linux-user: Handle rt_sigaction correctly for SPARC

[Peter Maydell]

SPARC is like Alpha in its handling of the rt_sigaction syscall:
it takes an extra parameter 'restorer' which needs to be copied
into the sa_restorer field of the sigaction struct. The order
of the arguments differs slightly between SPARC and Alpha but
the implementation is otherwise the same. (Compare the
rt_sigaction() functions in arch/sparc/kernel/sys_sparc_64.c
and arch/alpha/kernel/signal.c.)

Note that this change is somewhat moot until SPARC acquires
support for actually delivering RT signals.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index d4497de..8beab51 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -8556,8 +8556,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
     case TARGET_NR_rt_sigaction:
         {
 #if defined(TARGET_ALPHA)
-            struct target_sigaction act, oact, *pact = 0;
+            /* 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)) {
                 ret = -TARGET_EINVAL;
@@ -8583,18 +8591,29 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 unlock_user_struct(rt_act, arg3, 1);
             }
 #else
+#ifdef TARGET_SPARC
+            target_ulong restorer = arg4;
+            target_ulong sigsetsize = arg5;
+#else
+            target_ulong sigsetsize = arg4;
+#endif
             struct target_sigaction *act;
             struct target_sigaction *oact;
 
-            if (arg4 != sizeof(target_sigset_t)) {
+            if (sigsetsize != sizeof(target_sigset_t)) {
                 ret = -TARGET_EINVAL;
                 break;
             }
             if (arg2) {
-                if (!lock_user_struct(VERIFY_READ, act, arg2, 1))
+                if (!lock_user_struct(VERIFY_READ, act, arg2, 1)) {
                     goto efault;
-            } else
+                }
+#ifdef TARGET_SPARC
+                act->sa_restorer = restorer;
+#endif
+            } else {
                 act = NULL;
+            }
             if (arg3) {
                 if (!lock_user_struct(VERIFY_WRITE, oact, arg3, 0)) {
                     ret = -TARGET_EFAULT;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 4/4] linux-user: Handle rt_sigaction correctly for SPARC

[Laurent Vivier]

Le 06/11/2017 à 19:33, Peter Maydell a écrit :
> SPARC is like Alpha in its handling of the rt_sigaction syscall:
> it takes an extra parameter 'restorer' which needs to be copied
> into the sa_restorer field of the sigaction struct. The order
> of the arguments differs slightly between SPARC and Alpha but
> the implementation is otherwise the same. (Compare the
> rt_sigaction() functions in arch/sparc/kernel/sys_sparc_64.c
> and arch/alpha/kernel/signal.c.)
> 
> Note that this change is somewhat moot until SPARC acquires
> support for actually delivering RT signals.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d4497de..8beab51 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8556,8 +8556,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_rt_sigaction:
>          {
>  #if defined(TARGET_ALPHA)
> -            struct target_sigaction act, oact, *pact = 0;
> +            /* 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)) {
>                  ret = -TARGET_EINVAL;
> @@ -8583,18 +8591,29 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  unlock_user_struct(rt_act, arg3, 1);
>              }
>  #else
> +#ifdef TARGET_SPARC
> +            target_ulong restorer = arg4;
> +            target_ulong sigsetsize = arg5;
> +#else
> +            target_ulong sigsetsize = arg4;
> +#endif
>              struct target_sigaction *act;
>              struct target_sigaction *oact;
>  
> -            if (arg4 != sizeof(target_sigset_t)) {
> +            if (sigsetsize != sizeof(target_sigset_t)) {
>                  ret = -TARGET_EINVAL;
>                  break;
>              }
>              if (arg2) {
> -                if (!lock_user_struct(VERIFY_READ, act, arg2, 1))
> +                if (!lock_user_struct(VERIFY_READ, act, arg2, 1)) {
>                      goto efault;
> -            } else
> +                }
> +#ifdef TARGET_SPARC
> +                act->sa_restorer = restorer;
> +#endif
> +            } else {
>                  act = NULL;
> +            }
>              if (arg3) {
>                  if (!lock_user_struct(VERIFY_WRITE, oact, arg3, 0)) {
>                      ret = -TARGET_EFAULT;
> 

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

Re: [Qemu-devel] [PATCH 4/4] linux-user: Handle rt_sigaction correctly for SPARC

[Philippe Mathieu-Daudé]

Hi Peter,

On 11/06/2017 03:33 PM, Peter Maydell wrote:
> SPARC is like Alpha in its handling of the rt_sigaction syscall:
> it takes an extra parameter 'restorer' which needs to be copied
> into the sa_restorer field of the sigaction struct. The order
> of the arguments differs slightly between SPARC and Alpha but
> the implementation is otherwise the same. (Compare the
> rt_sigaction() functions in arch/sparc/kernel/sys_sparc_64.c
> and arch/alpha/kernel/signal.c.)
> 
> Note that this change is somewhat moot until SPARC acquires
> support for actually delivering RT signals.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d4497de..8beab51 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -8556,8 +8556,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_rt_sigaction:
>          {

> -            struct target_sigaction act, oact, *pact = 0;
> +            /* 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.
> +             */

I find it easier to read moving the #if after the comment.
Since using restorer for sparc improve readability we can use it for
alpha as well (code is a bit more consistent):

  #if defined(TARGET_ALPHA)
               target_ulong restorer = arg5;

>              struct target_rt_sigaction *rt_act;
> +            struct target_sigaction act, oact, *pact = 0;
>  
>              if (arg4 != sizeof(target_sigset_t)) {
>                  ret = -TARGET_EINVAL;

...
                   act.sa_restorer = restorer;

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

> @@ -8583,18 +8591,29 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  unlock_user_struct(rt_act, arg3, 1);
>              }
>  #else
> +#ifdef TARGET_SPARC
> +            target_ulong restorer = arg4;
> +            target_ulong sigsetsize = arg5;
> +#else
> +            target_ulong sigsetsize = arg4;
> +#endif
>              struct target_sigaction *act;
>              struct target_sigaction *oact;
>  
> -            if (arg4 != sizeof(target_sigset_t)) {
> +            if (sigsetsize != sizeof(target_sigset_t)) {
>                  ret = -TARGET_EINVAL;
>                  break;
>              }
>              if (arg2) {
> -                if (!lock_user_struct(VERIFY_READ, act, arg2, 1))
> +                if (!lock_user_struct(VERIFY_READ, act, arg2, 1)) {
>                      goto efault;
> -            } else
> +                }
> +#ifdef TARGET_SPARC
> +                act->sa_restorer = restorer;
> +#endif
> +            } else {
>                  act = NULL;
> +            }
>              if (arg3) {
>                  if (!lock_user_struct(VERIFY_WRITE, oact, arg3, 0)) {
>                      ret = -TARGET_EFAULT;
> 

Re: [Qemu-devel] [PATCH 0/4] linux-user: fix various SIGSEGV delivery bugs

[Riku Voipio]

On Mon, Nov 06, 2017 at 06:33:22PM +0000, Peter Maydell wrote:
> This patchset fixes most of the problems with delivering SIGSEGV
> noted in https://bugs.launchpad.net/qemu/+bug/1705118:
>  * s390x is missing masking operations on the fault address
>    passed to the guest in si_addr
>  * ppc is passing the PC of the offending insn, not the data
>    address, into si_addr
>  * sparc fails to record the address of a data fault and so
>    passes 0 into si_addr
>  * sparc isn't implementing rt_sigaction correctly (it has an
>    extra 'restorer' argument, similar to alpha)
> 
> It doesn't actually get the sparc testcase from that bug working,
> though, because SPARC is entirely missing support for setup_rt_frame()
> and do_rt_sigreturn(), so it can't deliver RT signals.
> 
> Implementing RT signal delivery is too big a bugfix for an afternoon,
> so here are the simpler parts. I'll leave the signal delivery part
> to somebody who cares about SPARC guests...
> 
> thanks
> -- PMM

Thanks!

series applied to linux-user tree,

Riku

 
> Peter Maydell (4):
>   linux-user/s390x: Mask si_addr for SIGSEGV
>   linux-user/ppc: Report correct fault address for data faults
>   linux-user/sparc: Put address for data faults where linux-user expects
>     it
>   linux-user: Handle rt_sigaction correctly for SPARC
> 
>  linux-user/main.c         |  8 ++++++--
>  linux-user/syscall.c      | 27 +++++++++++++++++++++++----
>  target/sparc/mmu_helper.c |  8 ++++++++
>  3 files changed, 37 insertions(+), 6 deletions(-)
> 
> -- 
> 2.7.4
>