Do not directly access the uc_sigmask member.
This is preparation for a sparc64 fix.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/include/host/aarch64/host-signal.h  |  5 +++++
 linux-user/include/host/alpha/host-signal.h    |  5 +++++
 linux-user/include/host/arm/host-signal.h      |  5 +++++
 linux-user/include/host/i386/host-signal.h     |  5 +++++
 .../include/host/loongarch64/host-signal.h     |  5 +++++
 linux-user/include/host/mips/host-signal.h     |  5 +++++
 linux-user/include/host/ppc/host-signal.h      |  5 +++++
 linux-user/include/host/riscv/host-signal.h    |  5 +++++
 linux-user/include/host/s390/host-signal.h     |  5 +++++
 linux-user/include/host/sparc/host-signal.h    |  5 +++++
 linux-user/include/host/x86_64/host-signal.h   |  5 +++++
 linux-user/signal.c                            | 18 ++++++++----------
 12 files changed, 63 insertions(+), 10 deletions(-)

diff --git a/linux-user/include/host/aarch64/host-signal.h b/linux-user/include/host/aarch64/host-signal.h
index 9770b36dc1..76ab078069 100644
--- a/linux-user/include/host/aarch64/host-signal.h
+++ b/linux-user/include/host/aarch64/host-signal.h
@@ -40,6 +40,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.pc = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     struct _aarch64_ctx *hdr;
diff --git a/linux-user/include/host/alpha/host-signal.h b/linux-user/include/host/alpha/host-signal.h
index f4c942948a..a44d670f2b 100644
--- a/linux-user/include/host/alpha/host-signal.h
+++ b/linux-user/include/host/alpha/host-signal.h
@@ -21,6 +21,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.sc_pc = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     uint32_t *pc = (uint32_t *)host_signal_pc(uc);
diff --git a/linux-user/include/host/arm/host-signal.h b/linux-user/include/host/arm/host-signal.h
index 6c095773c0..bbeb4ffefb 100644
--- a/linux-user/include/host/arm/host-signal.h
+++ b/linux-user/include/host/arm/host-signal.h
@@ -21,6 +21,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.arm_pc = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     /*
diff --git a/linux-user/include/host/i386/host-signal.h b/linux-user/include/host/i386/host-signal.h
index abe1ece5c9..fd36f06bda 100644
--- a/linux-user/include/host/i386/host-signal.h
+++ b/linux-user/include/host/i386/host-signal.h
@@ -21,6 +21,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.gregs[REG_EIP] = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     return uc->uc_mcontext.gregs[REG_TRAPNO] == 0xe
diff --git a/linux-user/include/host/loongarch64/host-signal.h b/linux-user/include/host/loongarch64/host-signal.h
index 7effa24251..a9dfe0c688 100644
--- a/linux-user/include/host/loongarch64/host-signal.h
+++ b/linux-user/include/host/loongarch64/host-signal.h
@@ -21,6 +21,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.__pc = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     const uint32_t *pinsn = (const uint32_t *)host_signal_pc(uc);
diff --git a/linux-user/include/host/mips/host-signal.h b/linux-user/include/host/mips/host-signal.h
index c666ed8c3f..ff840dd491 100644
--- a/linux-user/include/host/mips/host-signal.h
+++ b/linux-user/include/host/mips/host-signal.h
@@ -21,6 +21,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.pc = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 #if defined(__misp16) || defined(__mips_micromips)
 #error "Unsupported encoding"
 #endif
diff --git a/linux-user/include/host/ppc/host-signal.h b/linux-user/include/host/ppc/host-signal.h
index 1d8e658ff7..730a321d98 100644
--- a/linux-user/include/host/ppc/host-signal.h
+++ b/linux-user/include/host/ppc/host-signal.h
@@ -21,6 +21,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.regs->nip = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     return uc->uc_mcontext.regs->trap != 0x400
diff --git a/linux-user/include/host/riscv/host-signal.h b/linux-user/include/host/riscv/host-signal.h
index a4f170efb0..aceae544f2 100644
--- a/linux-user/include/host/riscv/host-signal.h
+++ b/linux-user/include/host/riscv/host-signal.h
@@ -21,6 +21,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.__gregs[REG_PC] = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     /*
diff --git a/linux-user/include/host/s390/host-signal.h b/linux-user/include/host/s390/host-signal.h
index a524f2ab00..e454cea54a 100644
--- a/linux-user/include/host/s390/host-signal.h
+++ b/linux-user/include/host/s390/host-signal.h
@@ -21,6 +21,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.psw.addr = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     uint16_t *pinsn = (uint16_t *)host_signal_pc(uc);
diff --git a/linux-user/include/host/sparc/host-signal.h b/linux-user/include/host/sparc/host-signal.h
index 7342936071..158918f2ec 100644
--- a/linux-user/include/host/sparc/host-signal.h
+++ b/linux-user/include/host/sparc/host-signal.h
@@ -29,6 +29,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
 #endif
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     uint32_t insn = *(uint32_t *)host_signal_pc(uc);
diff --git a/linux-user/include/host/x86_64/host-signal.h b/linux-user/include/host/x86_64/host-signal.h
index c71d597eb2..d64d076625 100644
--- a/linux-user/include/host/x86_64/host-signal.h
+++ b/linux-user/include/host/x86_64/host-signal.h
@@ -20,6 +20,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
     uc->uc_mcontext.gregs[REG_RIP] = pc;
 }
 
+static inline void *host_signal_mask(ucontext_t *uc)
+{
+    return &uc->uc_sigmask;
+}
+
 static inline bool host_signal_write(siginfo_t *info, ucontext_t *uc)
 {
     return uc->uc_mcontext.gregs[REG_TRAPNO] == 0xe
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 32854bb375..0c61459d4a 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -820,6 +820,7 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
     int guest_sig;
     uintptr_t pc = 0;
     bool sync_sig = false;
+    void *sigmask = host_signal_mask(uc);
 
     /*
      * Non-spoofed SIGSEGV and SIGBUS are synchronous, and need special
@@ -849,8 +850,7 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
             if (info->si_code == SEGV_ACCERR && h2g_valid(host_addr)) {
                 /* If this was a write to a TB protected page, restart. */
                 if (is_write &&
-                    handle_sigsegv_accerr_write(cpu, &uc->uc_sigmask,
-                                                pc, guest_addr)) {
+                    handle_sigsegv_accerr_write(cpu, sigmask, pc, guest_addr)) {
                     return;
                 }
 
@@ -865,10 +865,10 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
                 }
             }
 
-            sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
+            sigprocmask(SIG_SETMASK, sigmask, NULL);
             cpu_loop_exit_sigsegv(cpu, guest_addr, access_type, maperr, pc);
         } else {
-            sigprocmask(SIG_SETMASK, &uc->uc_sigmask, NULL);
+            sigprocmask(SIG_SETMASK, sigmask, NULL);
             if (info->si_code == BUS_ADRALN) {
                 cpu_loop_exit_sigbus(cpu, guest_addr, access_type, pc);
             }
@@ -909,17 +909,15 @@ static void host_signal_handler(int host_sig, siginfo_t *info, void *puc)
      * now and it getting out to the main loop. Signals will be
      * unblocked again in process_pending_signals().
      *
-     * WARNING: we cannot use sigfillset() here because the uc_sigmask
+     * WARNING: we cannot use sigfillset() here because the sigmask
      * field is a kernel sigset_t, which is much smaller than the
      * libc sigset_t which sigfillset() operates on. Using sigfillset()
      * would write 0xff bytes off the end of the structure and trash
      * data on the struct.
-     * We can't use sizeof(uc->uc_sigmask) either, because the libc
-     * headers define the struct field with the wrong (too large) type.
      */
-    memset(&uc->uc_sigmask, 0xff, SIGSET_T_SIZE);
-    sigdelset(&uc->uc_sigmask, SIGSEGV);
-    sigdelset(&uc->uc_sigmask, SIGBUS);
+    memset(sigmask, 0xff, SIGSET_T_SIZE);
+    sigdelset(sigmask, SIGSEGV);
+    sigdelset(sigmask, SIGBUS);
 
     /* interrupt the virtual CPU as soon as possible */
     cpu_exit(thread_cpu);
-- 
2.25.1

On 8/2/22 08:12, Richard Henderson wrote:
> Do not directly access the uc_sigmask member.
> This is preparation for a sparc64 fix.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/include/host/aarch64/host-signal.h  |  5 +++++
>   linux-user/include/host/alpha/host-signal.h    |  5 +++++
>   linux-user/include/host/arm/host-signal.h      |  5 +++++
>   linux-user/include/host/i386/host-signal.h     |  5 +++++
>   .../include/host/loongarch64/host-signal.h     |  5 +++++
>   linux-user/include/host/mips/host-signal.h     |  5 +++++
>   linux-user/include/host/ppc/host-signal.h      |  5 +++++
>   linux-user/include/host/riscv/host-signal.h    |  5 +++++
>   linux-user/include/host/s390/host-signal.h     |  5 +++++
>   linux-user/include/host/sparc/host-signal.h    |  5 +++++
>   linux-user/include/host/x86_64/host-signal.h   |  5 +++++
>   linux-user/signal.c                            | 18 ++++++++----------
>   12 files changed, 63 insertions(+), 10 deletions(-)

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

On Tue, 8 Feb 2022 at 08:03, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Do not directly access the uc_sigmask member.
> This is preparation for a sparc64 fix.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/include/host/aarch64/host-signal.h  |  5 +++++
>  linux-user/include/host/alpha/host-signal.h    |  5 +++++
>  linux-user/include/host/arm/host-signal.h      |  5 +++++
>  linux-user/include/host/i386/host-signal.h     |  5 +++++
>  .../include/host/loongarch64/host-signal.h     |  5 +++++
>  linux-user/include/host/mips/host-signal.h     |  5 +++++
>  linux-user/include/host/ppc/host-signal.h      |  5 +++++
>  linux-user/include/host/riscv/host-signal.h    |  5 +++++
>  linux-user/include/host/s390/host-signal.h     |  5 +++++
>  linux-user/include/host/sparc/host-signal.h    |  5 +++++
>  linux-user/include/host/x86_64/host-signal.h   |  5 +++++
>  linux-user/signal.c                            | 18 ++++++++----------
>  12 files changed, 63 insertions(+), 10 deletions(-)
>
> diff --git a/linux-user/include/host/aarch64/host-signal.h b/linux-user/include/host/aarch64/host-signal.h
> index 9770b36dc1..76ab078069 100644
> --- a/linux-user/include/host/aarch64/host-signal.h
> +++ b/linux-user/include/host/aarch64/host-signal.h
> @@ -40,6 +40,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
>      uc->uc_mcontext.pc = pc;
>  }
>
> +static inline void *host_signal_mask(ucontext_t *uc)
> +{
> +    return &uc->uc_sigmask;
> +}

Why void* rather than sigset_t* ?

thanks
-- PMM

On 2/8/22 21:59, Peter Maydell wrote:
> On Tue, 8 Feb 2022 at 08:03, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Do not directly access the uc_sigmask member.
>> This is preparation for a sparc64 fix.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   linux-user/include/host/aarch64/host-signal.h  |  5 +++++
>>   linux-user/include/host/alpha/host-signal.h    |  5 +++++
>>   linux-user/include/host/arm/host-signal.h      |  5 +++++
>>   linux-user/include/host/i386/host-signal.h     |  5 +++++
>>   .../include/host/loongarch64/host-signal.h     |  5 +++++
>>   linux-user/include/host/mips/host-signal.h     |  5 +++++
>>   linux-user/include/host/ppc/host-signal.h      |  5 +++++
>>   linux-user/include/host/riscv/host-signal.h    |  5 +++++
>>   linux-user/include/host/s390/host-signal.h     |  5 +++++
>>   linux-user/include/host/sparc/host-signal.h    |  5 +++++
>>   linux-user/include/host/x86_64/host-signal.h   |  5 +++++
>>   linux-user/signal.c                            | 18 ++++++++----------
>>   12 files changed, 63 insertions(+), 10 deletions(-)
>>
>> diff --git a/linux-user/include/host/aarch64/host-signal.h b/linux-user/include/host/aarch64/host-signal.h
>> index 9770b36dc1..76ab078069 100644
>> --- a/linux-user/include/host/aarch64/host-signal.h
>> +++ b/linux-user/include/host/aarch64/host-signal.h
>> @@ -40,6 +40,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
>>       uc->uc_mcontext.pc = pc;
>>   }
>>
>> +static inline void *host_signal_mask(ucontext_t *uc)
>> +{
>> +    return &uc->uc_sigmask;
>> +}
> 
> Why void* rather than sigset_t* ?

Because it's not really a sigset_t, even when it is in ucontext_t.
It's a kernel_sigset_t, as per the comment in host_signal_handler.

When it comes to sparc64 in the last patch, the glibc type will turn out to be unsigned long.

Got a better suggestion?


r~

On Tue, 8 Feb 2022 at 11:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/8/22 21:59, Peter Maydell wrote:
> > On Tue, 8 Feb 2022 at 08:03, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> Do not directly access the uc_sigmask member.
> >> This is preparation for a sparc64 fix.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>   linux-user/include/host/aarch64/host-signal.h  |  5 +++++
> >>   linux-user/include/host/alpha/host-signal.h    |  5 +++++
> >>   linux-user/include/host/arm/host-signal.h      |  5 +++++
> >>   linux-user/include/host/i386/host-signal.h     |  5 +++++
> >>   .../include/host/loongarch64/host-signal.h     |  5 +++++
> >>   linux-user/include/host/mips/host-signal.h     |  5 +++++
> >>   linux-user/include/host/ppc/host-signal.h      |  5 +++++
> >>   linux-user/include/host/riscv/host-signal.h    |  5 +++++
> >>   linux-user/include/host/s390/host-signal.h     |  5 +++++
> >>   linux-user/include/host/sparc/host-signal.h    |  5 +++++
> >>   linux-user/include/host/x86_64/host-signal.h   |  5 +++++
> >>   linux-user/signal.c                            | 18 ++++++++----------
> >>   12 files changed, 63 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/linux-user/include/host/aarch64/host-signal.h b/linux-user/include/host/aarch64/host-signal.h
> >> index 9770b36dc1..76ab078069 100644
> >> --- a/linux-user/include/host/aarch64/host-signal.h
> >> +++ b/linux-user/include/host/aarch64/host-signal.h
> >> @@ -40,6 +40,11 @@ static inline void host_signal_set_pc(ucontext_t *uc, uintptr_t pc)
> >>       uc->uc_mcontext.pc = pc;
> >>   }
> >>
> >> +static inline void *host_signal_mask(ucontext_t *uc)
> >> +{
> >> +    return &uc->uc_sigmask;
> >> +}
> >
> > Why void* rather than sigset_t* ?
>
> Because it's not really a sigset_t, even when it is in ucontext_t.
> It's a kernel_sigset_t, as per the comment in host_signal_handler.

I guess. handle_sigsegv_accerr_write()'s prototype just calls
it a sigset_t, though.

One approach would be to define a typedef for this kernel sigset_t,
and have wrappers for sigdelset() and sigprocmask() which take that
type rather than the libc sigset_t (plus a sigfillset that wrapped
the memset). Then we would avoid the potential bug that the
WARNING comment is talking about, because code wouldn't have
a thing that it could pass to sigfillset() without getting a
compiler complaint about the type. But maybe that's too heavyweight.

For the purposes of this series,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM