| Message ID | 20220208071237.319844-3-richard.henderson@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux-user: Fixes for sparc64 host | expand |
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
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);
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(-)